-
Notifications
You must be signed in to change notification settings - Fork 209
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
Fix support resumable + fix incorrect handling of paths which start with a / #918
Conversation
Now tests should pass |
Hey thanks for contributing. I'm going to sit down and review this this weekend. |
func (s *Server) buildMuxer() { | ||
const apiPrefix = "/storage/v1" | ||
s.mux = mux.NewRouter() | ||
s.mux = mux.NewRouter().SkipClean(true).UseEncodedPath() |
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.
Reason for this.
UseEncodedPath tells the router to match the encoded original path to the routes. For eg. "/path/foo%2Fbar/to" will match the path "/path/{var}/to".
This fix issue, that if we use names like /file.txt
, in this case it will be /download/storage/v1/b/{bucketName}/o/%2Ffile.txt
in url path, which mux trying to fix by redirect to /download/storage/v1/b/{bucketName}/o/file.txt
, which is incorrect behaviour for GCP
SkipClean - When true, if the route path is "/path//to", it will remain with the double slash. This is helpful if you have a route like: /fetch/http://xkcd.com/534/
We do not fix urls, what defined by sdk or users
@@ -211,9 +212,22 @@ func newServer(options Options) (*Server, error) { | |||
return &s, nil | |||
} | |||
|
|||
func unescapeMuxVars(vars map[string]string) map[string]string { |
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.
In some places we use r.URL.Query()
, which provide already decoded values. But mux.Vars(r)
provide values "as is" (without decoding). That is why we need decode objectName
and bucketName
ourself. This is important, because before accessing to storage we encode values and without this this lead to double encoding.
fakestorage/upload.go
Outdated
location := fmt.Sprintf( | ||
"%s/upload/storage/v1/b/%s/o?uploadType=resumable&name=%s&upload_id=%s", | ||
s.URL(), | ||
url.PathEscape(bucketName), |
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.
we have to encode values for url
@@ -362,6 +363,9 @@ func (s *Server) multipartUpload(bucketName string, r *http.Request) jsonRespons | |||
} | |||
|
|||
func (s *Server) resumableUpload(bucketName string, r *http.Request) jsonResponse { | |||
if r.URL.Query().Has("upload_id") { | |||
return s.uploadFileContent(r) |
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.
if upload_id
came - it mean we have request to upload file.
Thanks @fsouza - added comments to simplify review |
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.
Thanks for contributing! The change looks sane, but can you add some tests? Ideally something that would fail before this change, but passes with both storage backends!
@fsouza added test cases which fail in current main (cover resumable + backslashes handling): --- FAIL: TestServerClientObjectWriter (0.01s)
--- FAIL: TestServerClientObjectWriter/https_listener (0.40s)
--- FAIL: TestServerClientObjectWriter/https_listener/file_with_backslash_at_beginning (0.04s)
upload_test.go:120: storage: object doesn't exist
--- FAIL: TestServerClientObjectWriter/https_listener/file_with_backslashes_at_name (0.13s)
upload_test.go:120: storage: object doesn't exist
--- FAIL: TestServerClientObjectWriter/no_listener (0.40s)
--- FAIL: TestServerClientObjectWriter/no_listener/file_with_backslash_at_beginning (0.05s)
upload_test.go:120: storage: object doesn't exist
--- FAIL: TestServerClientObjectWriter/no_listener/file_with_backslashes_at_name (0.12s)
upload_test.go:120: storage: object doesn't exist
--- FAIL: TestServerClientObjectWriter/http_listener (0.40s)
--- FAIL: TestServerClientObjectWriter/http_listener/file_with_backslash_at_beginning (0.05s)
upload_test.go:120: storage: object doesn't exist
--- FAIL: TestServerClientObjectWriter/http_listener/file_with_backslashes_at_name (0.12s)
upload_test.go:120: storage: object doesn't exist |
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.
One last comment and we should be good to go. Thank you very much!
Co-authored-by: fsouza <[email protected]>
@fsouza fixed |
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.
Thank you!
Should fix support for
resumable
+ #349