-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[VFS] Update file content #48
Conversation
2c6147b
to
749556f
Compare
749556f
to
db5215f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love that you did make Directory & FIles single struct. A few cosmetic issue.
if err != nil { | ||
// NewDirDoc is the DirDoc constructor. The given name is validated. | ||
func NewDirDoc(name, folderID string, tags []string) (doc *DirDoc, err error) { | ||
if err = checkFileName(name); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style decision to be made here :
@nono reverted this kind of set-output-value-and-return-empty in one of its PR (although it was only 1 value)
We need to pick a style and stick to it. Personnaly, I am not sure : this is cleaner than return nil, err
but the intent is less readable. When in doubt, it is better to pick more readability rather than a few less chars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.. I find named returns useful when I change the signature of a function (to have smaller diffs too..). But in the end I don't really care that much. @nono an opinion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would have to do return "", nil, err
which I think is a bit annoying..
Although for functions returning only 0 or 1 value + error, return nil, err
may be a bit cleaner...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to give a name to return values to explain what they are. But I don't like the naked returns. It's really too easy to shadow err
by mistake. This is an example of what I means: https://play.golang.org/p/0ZZ7bXElXq.
func copyOnFsAndCheckIntegrity(m *DocAttributes, fs afero.Fs, pth string, r io.ReadCloser) (written int64, md5Sum []byte, err error) { | ||
f, err := fs.Create(pth) | ||
func copyOnFsAndCheckIntegrity(pth string, givenMD5 []byte, executable bool, fs afero.Fs, r io.Reader) (written int64, md5Sum []byte, err error) { | ||
// We want to write only (O_WRONLY), create the file if it does not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is at the wrong place (should be before L279). Another comment about "magic numbers" 0755 & 644 would be cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, done: a3c6271
return | ||
} | ||
|
||
defer func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defer call should be before fs.Chmod, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, done 108945b
func getFsAndDBPrefix(c *gin.Context) (fs afero.Fs, dbPrefix string, err error) { | ||
instance := middlewares.GetInstance(c) | ||
dbPrefix = instance.GetDatabasePrefix() | ||
fs, err = instance.GetStorageProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Future] May be all this instanciating of request specific structs (instance, dbprefix, fs) could be moved to middlewares and c.MustGet.
@@ -332,7 +443,9 @@ func TestMain(m *testing.M) { | |||
router.Use(injectInstance(instance)) | |||
router.POST("/files/", CreationHandler) | |||
router.POST("/files/:folder-id", CreationHandler) | |||
router.GET("/files/:file-id", ReadHandler) | |||
router.PUT("/files/:file-id", OverwriteFileContentHandler) | |||
router.HEAD("/files/:file-id", ReadFileHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious. Do you know what we have to do to support HEAD with Gin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not seen anything specific in gin to "support" HEAD. This HEAD route is totally backed up by http.ServeContent
: https://github.com/golang/go/blob/ad26bb5e3098cbfd7c0ad9a1dc9d38c92e50f06e/src/net/http/fs.go#L266-L270
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!
This PR introduce the new route
PUT /files/:files-id
along with a new methodvfs.ModifyFileContent
.Some notes:
In order to add this feature, I also removed the intermediary (and awkward) struct
vfs.DirAttributes
that was used to stack all the contents of a creation request for files and directories. Now,web/file
can usevfs.NewFileDoc
orvfs.NewDirDoc
directly and appropriately.I also increased the gometalinter dupl-threshold, because it annoyed me on some trivial code (in test)...