[WIP] Create directory #26
Conversation
|
gometalinter is not really happy for reasons I don't really agree on 馃槃.
|
| assert.Equal(t, 201, res.StatusCode) | ||
| res.Body.Close() | ||
|
|
||
| storage, _ := instance.GetStorageProvider() |
There was a problem hiding this comment.
I guess gometalinter wants you to factorize this part of the code with TestCreateDirWithSlashInName into a testDirExist(t, instance, expectedPath) function
| } | ||
|
|
||
| func TestCreateDirWithSlashInName(t *testing.T) { | ||
| res := createDir(t, "/files/123?Name=coucou/les/copains!&Type=io.cozy.folders") |
There was a problem hiding this comment.
I am not sure we want to allow creation of a folder with "/" in names as it will not work when mapping it to an actual FS (like cozy-desktop does). We should probalby just throw a 400 "no slash in name".
There was a problem hiding this comment.
Indeed. Made changes to reject illegal characters in 3ffba18
| @@ -70,3 +70,18 @@ func SetInstance() gin.HandlerFunc { | |||
| c.Set("instance", instance) | |||
| } | |||
| } | |||
|
|
|||
| // GetInstance will return the instance linked to the given gin context | |||
| func GetInstance(c *gin.Context) (*Instance, error) { | |||
There was a problem hiding this comment.
In my own PR, I use c.MustGet("instance"), which panic if there is no instance. This avoids having to test if exists and returning an error in every handler.
If the instance middleware aborts when it cant make or get the instance for a request, it should be sufficiently robust. To be discussed
There was a problem hiding this comment.
I would tend to agree. For commodity reasons and also because if we do not have an instance in the context, panicing seems the right thing to do...
|
I agree that the cyclomatic complexity isnt really helpful for CreationHandler as it is only caused by all the error handling. If we decide to keep it, we could divide this function into a |
| func parseTags(str string) []string { | ||
| var tags []string | ||
| for _, tag := range strings.Split(str, ",") { | ||
| // @TODO: more sanitization maybe ? |
There was a problem hiding this comment.
I think you can remove this TODO. I don't see a reason to restrict what the tags can be
| var ( | ||
| errDocAlreadyExists = errors.New("Directory already exists") | ||
| errDocTypeInvalid = errors.New("Invalid document type") | ||
| errIllegalFilename = errors.New("Invalid filename: empty or contains one of these illegal characters: / \\ : ? * \" |") |
There was a problem hiding this comment.
How did you choose what chars are legal / illegal?
There was a problem hiding this comment.
By looking at Windows and its naming conventions... https://msdn.microsoft.com/en-us/library/aa365247(v=vs.85).aspx (I'm missing > and < btw)...
Don't you think it may cause us problem if one day we want to sync on a win fs ?
There was a problem hiding this comment.
At least, we should forbid the null character (\0).
For sync with windows, there are things more difficult than just some invalid chars. Some filename are reserved (AUX, COM1, etc), the length of the path is limited (260 chars on NTFS), the last char can't be a space or a dot, it is not case sensitive, etc.
I don't think we should enforce all these rules on Cozy. For the legal/illegal chars, I don't mind. I was just curious how you choosed them.
There was a problem hiding this comment.
If we don't want to enforce these rules, I don't think its necessary to forbid these special characters either. I'll just leave / as an illegal one for now.
New PR: #30
| errIllegalFilename = errors.New("Invalid filename: empty or contains one of these illegal characters: / \\ : ? * \" |") | ||
| ) | ||
|
|
||
| var regFileName = regexp.MustCompile("[\\/\\\\:\\?\\*\"|]+") |
There was a problem hiding this comment.
I don't think it's necessary to escape /, ? and * in this regexp.
There was a problem hiding this comment.
Oh, and it may be simpler to use strings.IndexAny to check for forbidden chars
There was a problem hiding this comment.
Thanks, strings.ContainsAny is even more appropriate.
Still work in progress
Lots of stuff are missing and would require connections with couchdb. However since the directory creation share some codes with the uploading part, I prefer to make this PR ASAP so that we can work in parallel 馃幙 .
This PR introduces: