Skip to content
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

Attempt to return metadata after a TUS PATCH #797

Closed
wants to merge 3 commits into from
Closed

Attempt to return metadata after a TUS PATCH #797

wants to merge 3 commits into from

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jun 3, 2020

  • PROBLEM: how to get at the actual file name, need to read the upload info file
  • PROBLEM: cannot return headers as tusd is already sending the response

@butonic
Copy link
Contributor

butonic commented Jun 4, 2020

This won't work because the writeHeader method flushes the cache. But we could wrap the ResponseWriter w which is passed in the hander.PatchFile(w, r) call with our own implementation that defers the WriteHeader call. Then we can add arbitrary headers when hander.PatchFile(w, r) returns and then call a FlushResponse() functon ...

w.ResponseWriter.WriteHeader(w.statusCode)
}

func (w *WrappedResponseWriter) Header() http.Header {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported method WrappedResponseWriter.Header should have comment or be unexported

// delay this
}

func (w *WrappedResponseWriter) SendResponse() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported method WrappedResponseWriter.SendResponse should have comment or be unexported

statusCode int
}

func (w *WrappedResponseWriter) WriteHeader(statusCode int) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported method WrappedResponseWriter.WriteHeader should have comment or be unexported

@@ -48,6 +52,28 @@ type svc struct {
storage storage.FS
}

type WrappedResponseWriter struct {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exported type WrappedResponseWriter should have comment or be unexported

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 9, 2020

I've pushed an attempt to wrap the response.

It works and I'm able to add headers after the TUS handler now.

The next challenge is finding the path to pass to the CS3 API's GetMD.
For this I capture the upload info from TUS' "CompleteUpload" event using a Go-routine, the approach looks really ugly.

And then I get a structure like this:

uploadStorage={"BinPath":"/var/tmp/reva/data/einstein/uploads/8f5ed05f-c94c-49b9-b14f-d9d3b70ae886","Idp":"https://localhost:9200","InternalDestination":"/var/tmp/reva/data/einstein/files/tus2/md5.txt","LogLevel":"trace","Type":"OwnCloudStore","UserId":"einstein","UserName":"einstein"}

but I haven't figured out how to resolve the correct path reference to pass the the CS3 API.
The call to TrimPrefix doesn't work here: https://github.com/cs3org/reva/pull/797/files#diff-d92e2cb18e0f9290c492defb7bb389abR143

@butonic any hints ?

@labkode
Copy link
Member

labkode commented Jun 9, 2020

@PVince81 @butonic can we have a quick meeting to discuss this workflow?

What about tomorrow around 10:30AM?

@PVince81
Copy link
Contributor Author

@labkode sounds good.

Note that this is mostly driven by client development, as the old protocol had these fields returned at the end of the upload to avoid having to do subsequent calls (which are tricky for example on iOS)

@labkode
Copy link
Member

labkode commented Jun 11, 2020

@PVince81 @butonic

The next challenge is finding the path to pass to the CS3 API's GetMD.
For this I capture the upload info from TUS' "CompleteUpload" event using a Go-routine, the approach looks really ugly.

I've been through TUS and started refactoring code in #828.

Upon this task I've found a fundamental problem on the various implementations of TUS for the storages. By extending the interface with the tusd.DataStore you are cheating and bypassing any path based operation on this upload, reason you can't get the metadata for it because the internal path is not exposed to the storage provider.

For example, if your filesystem exposes to the storage provider the path /tmp/revad/data but you're storing your uploads under /tmp/revad/uploads, you won't be able to call GetMD because the request will be relative to /tmp/revad/data and not to /tmp/revad/uploads.

So, the current implementations need to store the upload information under the configured root, that will allow to call GetMD and avoid that hack for example.

Passing internal implementation details to upper layers is a no-go.

@butonic
Copy link
Contributor

butonic commented Feb 3, 2021

won't be continued here

@butonic butonic closed this Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants