Add LFS object verification step after upload #2868
Conversation
lafriks
added
the
type/feature
Codecov Report
@@ Coverage Diff @@
## master #2868 +/- ##
=======================================
Coverage 26.85% 26.85%
=======================================
Files 89 89
Lines 17607 17607
=======================================
Hits 4728 4728
Misses 12193 12193
Partials 686 686Continue to review full report at Codecov.
|
tboerger
added
the
lgtm/need 2
modules/lfs/content_store.go
Outdated
| // Verify returns true if the object exists in the content store and size is correct. | ||
| func (s *ContentStore) Verify(meta *models.LFSMetaObject) bool { | ||
| path := filepath.Join(s.BasePath, transformKey(meta.Oid)) | ||
| if fi, err := os.Stat(path); os.IsNotExist(err) || fi.Size() != meta.Size { |
There was a problem hiding this comment.
What if err is not nil, but is something other than a IsNotExist error. I think we should return false in that case.
There was a problem hiding this comment.
@erhankoenig I did not want to change that as that is current behaviour also for exists method
There was a problem hiding this comment.
But if err is not nil, it could be the case that fi is nil. In that case, fi.Size() will result in a nil-pointer dereference, so I think we need to check if err is nil or not.
There was a problem hiding this comment.
That's true but I don't know if in such case we should return false
modules/lfs/server.go
Outdated
| @@ -69,6 +69,11 @@ func (v *RequestVars) ObjectLink() string { | |||
| return fmt.Sprintf("%s%s/%s/info/lfs/objects/%s", setting.AppURL, v.User, v.Repo, v.Oid) | |||
| } | |||
|
|
|||
| // VerifyLink builds a URL for verifying the object. | |||
| func (v *RequestVars) VerifyLink() string { | |||
| return fmt.Sprintf("%s%s/%s/info/lfs/verify", setting.AppURL, v.User, v.Repo) | |||
There was a problem hiding this comment.
nit: use URLJoin. The current code assumes setting.AppURL has a trailing slash, and even if that assumption always holds (for now), it'd be nicer to not have to assume it in the first place.
modules/lfs/server.go
Outdated
| @@ -320,6 +325,35 @@ func PutHandler(ctx *context.Context) { | |||
| logRequest(ctx.Req, 200) | |||
| } | |||
|
|
|||
| // VerifyHandler verify oid and it's size from the content store | |||
|
@ethantkoenig fixed |
modules/lfs/server.go
Outdated
| @@ -66,7 +68,12 @@ type ObjectError struct { | |||
|
|
|||
| // ObjectLink builds a URL linking to the object. | |||
| func (v *RequestVars) ObjectLink() string { | |||
| return fmt.Sprintf("%s%s/%s/info/lfs/objects/%s", setting.AppURL, v.User, v.Repo, v.Oid) | |||
| return path.Join(setting.AppURL, v.User, v.Repo, "info/lfs/objects", v.Oid) | |||
There was a problem hiding this comment.
You can't call path.Join on URLs (example)
There was a problem hiding this comment.
Yes, sorry thought AppURL was url without domain, you are right. I'll fix that
|
@ethantkoenig now it's fixed |
modules/lfs/server.go
Outdated
| @@ -320,6 +327,35 @@ func PutHandler(ctx *context.Context) { | |||
| logRequest(ctx.Req, 200) | |||
| } | |||
|
|
|||
| // VerifyHandler verify oid and its size from the content store | |||
| func VerifyHandler(ctx *context.Context) { | |||
|
|
|||
|
It still seems wrong to me that verification can succeed if |
tboerger
added
lgtm/need 1
|
@ethantkoenig I agree but there should probably needs to be bigger rewrite to return (bool, error) and handle that in caller functions but that is out of scope for this PR |
|
@lunny fixed |
|
LGTM |
tboerger
added
lgtm/done
|
@ethantkoenig fixed error handling for verify function |
As per recommendation - #2112 (comment)