implement checksums in the owncloud storage driver #1629
Conversation
C0rby
force-pushed
the
owncloud-storage-checksums
branch
from
bbf2cca to
12ed199
Compare
|
@C0rby we usually abbreviate checksum as |
|
I`d say that we try to stay consistent as much as possible. :) |
pkg/storage/fs/owncloud/owncloud.go
Outdated
| @@ -682,6 +684,14 @@ func (fs *ocfs) convertToResourceInfo(ctx context.Context, fi os.FileInfo, ip st | |||
|
|
|||
| ri.PermissionSet = fs.permissionSet(ctx, ri.Owner) | |||
|
|
|||
| // checksums | |||
| if _, ok := mdKeysMap[checksumsKey]; (!fi.IsDir()) && returnAllKeys || ok { | |||
There was a problem hiding this comment.
I like to vote for more explicit error handling here. The handling of the IsDir() case should be done explicitely as that will ease debugging later big times. Also, would'nt a log message be helpful in the case that something is a dir that should not be or vise versa?
Also, I am only 38 years in programming and still do not know quickly what A && B || C really evaluates to, how about helping me poor soul with additional parenthesis?
| appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("file not fount") | ||
| default: | ||
| appctx.GetLogger(ctx).Error().Err(err).Str("nodepath", nodePath).Str("algorithm", algo).Msg("could not read checksum") | ||
| } |
pkg/storage/fs/owncloud/upload.go
Outdated
| defer f.Close() | ||
|
|
||
| r1 := io.TeeReader(f, sh) | ||
| r2 := io.TeeReader(r1, mh) |
There was a problem hiding this comment.
Ok, bear with me, but.... short variable names have a long history of proving that they're not cool. We tried that in C already back in the dark days, paying back with lots of head scratching... So why not just spend a few more chars to get speaking names? Yes, also for dumb, short living local variables... Ok, I am not blaming you here for the golang way of doing things ;-)
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
This means that we do not require checksums, right? But if the feature "checksums" is activated, shouldn't it be an error/warning if the upload comes completely without checksum? Or is that checked elsewhere?
There was a problem hiding this comment.
Yes, as it is right now checksums are optional. I don't know if we even have the flag to "activate" checksums.
Also I'm not sure how much sense it makes to spend too much effort in the owncloud storage driver since we are only using it for the webUI acceptance tests and even them will in the near future need to switch to the ocis storage driver.
This PR is just to make the webUI tests for the Thumbnails PR pass. owncloud/ocis#1898
|
@dragotin, I must confess that I just copied the checksum code from the decomposedfs. But I can still implement your feedback nonetheless. |
C0rby
force-pushed
the
owncloud-storage-checksums
branch
from
12ed199 to
8018db6
Compare
C0rby
force-pushed
the
owncloud-storage-checksums
branch
from
8018db6 to
2dde017
Compare
No description provided.