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

Tus upload #261

Merged
merged 23 commits into from Jun 19, 2017

Conversation

Projects
None yet
6 participants
@buchi
Member

buchi commented Mar 12, 2017

Closes #79

@tisto tisto added the in progress label Mar 12, 2017

@buchi buchi referenced this pull request Mar 12, 2017

Closed

TUS Endpoint #79

@buchi buchi self-assigned this Mar 12, 2017

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Mar 17, 2017

Member

@buchi anything we can do to help here to get rid of the WIP? I might be able to allocate a few working hours early next week if you give us a hint where we could start.

Member

tisto commented Mar 17, 2017

@buchi anything we can do to help here to get rid of the WIP? I might be able to allocate a few working hours early next week if you give us a hint where we could start.

@tisto tisto added the 12 prio: high label Mar 19, 2017

@csenger

This comment has been minimized.

Show comment
Hide comment
@csenger

csenger Mar 21, 2017

Contributor

@buchi I'd add tests and docs for TUS to make the code ready to merge. Is there missing functionality before TUS can be merged, other changes you would like to be done or problems that block it from being merged?

Contributor

csenger commented Mar 21, 2017

@buchi I'd add tests and docs for TUS to make the code ready to merge. Is there missing functionality before TUS can be merged, other changes you would like to be done or problems that block it from being merged?

csenger added a commit that referenced this pull request Mar 21, 2017

csenger added a commit that referenced this pull request Mar 21, 2017

@csenger

This comment has been minimized.

Show comment
Hide comment
@csenger

csenger Mar 21, 2017

Contributor

There are several more things to fix in this branch:

errors/status codes

I corrected the status codes for some errors. I need to go through all the possible errors and add tests/correct status codes to match the spec.

uploading text content does not work

The uploader queries the content type registry with the filename + mime type. For text files this is Document and the uploader can't handle that. I don't think we want to create Documents at all, but on the other hand we should not hard code File/Image as types to create.

I added a failing test for that.

location

The spec requires the POST to respond with a location header to return the location of the generated resource:

The Server MUST acknowledge a successful upload creation with the 201 Created status. The Server MUST set the Location header to the URL of the created resource. This URL MAY be absolute or relative.

The uploader is implemented in a way to store the unfinished data on disk in a temporary directory. We return a link to the upload endpoint, e.g.http://localhost:55001/plone/testfolder/@upload/ab7aeaf37d2c40ffb5818aec52ab8150. But this is only a temporary url to look up the disc cache. We create the content object when the upload is complete. This is a violation of the spec.

To fulfill the spec we need to create the file content during the initial POST request and return the final url of the file/image as the location. The consequence would be that

  • we have several transactions to the ZODB. At least 2 (the initial POST and the PATCH to upload the file data). Potentially more
  • we might end up with empty or half uploaded files. There is a termination extension for TUS to cleanup uploads.

I see no way to circumvent that with a standard Plone site.

Contributor

csenger commented Mar 21, 2017

There are several more things to fix in this branch:

errors/status codes

I corrected the status codes for some errors. I need to go through all the possible errors and add tests/correct status codes to match the spec.

uploading text content does not work

The uploader queries the content type registry with the filename + mime type. For text files this is Document and the uploader can't handle that. I don't think we want to create Documents at all, but on the other hand we should not hard code File/Image as types to create.

I added a failing test for that.

location

The spec requires the POST to respond with a location header to return the location of the generated resource:

The Server MUST acknowledge a successful upload creation with the 201 Created status. The Server MUST set the Location header to the URL of the created resource. This URL MAY be absolute or relative.

The uploader is implemented in a way to store the unfinished data on disk in a temporary directory. We return a link to the upload endpoint, e.g.http://localhost:55001/plone/testfolder/@upload/ab7aeaf37d2c40ffb5818aec52ab8150. But this is only a temporary url to look up the disc cache. We create the content object when the upload is complete. This is a violation of the spec.

To fulfill the spec we need to create the file content during the initial POST request and return the final url of the file/image as the location. The consequence would be that

  • we have several transactions to the ZODB. At least 2 (the initial POST and the PATCH to upload the file data). Potentially more
  • we might end up with empty or half uploaded files. There is a termination extension for TUS to cleanup uploads.

I see no way to circumvent that with a standard Plone site.

@csenger csenger self-assigned this Mar 22, 2017

@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Mar 22, 2017

Member

@csenger @tisto I'll try to look at it this week and give some feedback. The implementation is not finished yet, mainly handling of edge cases and cleanup stuff is missing. So don't merge it yet! We're also missing the ability to replace files (not the Plone file object but the value of the file field in an existing file object). The TUS specification provides no information how to handle that.

@csenger regarding your remark about the location I do not agree. It's a question of interpretation though, but IMO we're fulfilling the specs. The created resource by POST is a temporary upload object and we're returning it's location. I don't see any indications that this has to be the final resource. My second point is that we have to make an implementation that fits with Plone. Creating unfinished content objects would not work well with Plone. E.g. Plone would try to index the object and the object would be visible to other users...

Member

buchi commented Mar 22, 2017

@csenger @tisto I'll try to look at it this week and give some feedback. The implementation is not finished yet, mainly handling of edge cases and cleanup stuff is missing. So don't merge it yet! We're also missing the ability to replace files (not the Plone file object but the value of the file field in an existing file object). The TUS specification provides no information how to handle that.

@csenger regarding your remark about the location I do not agree. It's a question of interpretation though, but IMO we're fulfilling the specs. The created resource by POST is a temporary upload object and we're returning it's location. I don't see any indications that this has to be the final resource. My second point is that we have to make an implementation that fits with Plone. Creating unfinished content objects would not work well with Plone. E.g. Plone would try to index the object and the object would be visible to other users...

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Apr 4, 2017

Member

@buchi did you had time to look into this yet? We really need the TUS upload in our project and I would like to get away without a custom solution on our side. :)

Member

tisto commented Apr 4, 2017

@buchi did you had time to look into this yet? We really need the TUS upload in our project and I would like to get away without a custom solution on our side. :)

buchi added a commit that referenced this pull request Apr 9, 2017

buchi added a commit that referenced this pull request Apr 9, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 9, 2017

Coverage Status

Coverage decreased (-0.9%) to 95.357% when pulling a40a2a8 on tus-upload into 7f1f8b5 on master.

coveralls commented Apr 9, 2017

Coverage Status

Coverage decreased (-0.9%) to 95.357% when pulling a40a2a8 on tus-upload into 7f1f8b5 on master.

@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Apr 9, 2017

Member

@tisto I've now implemented the TUS expiration extension which is required to cleanup files of unfinished uploads lying around. With that we have a basic implementation that could be merged regarding features. However test coverage and documentation still need some work. I won't have much time to continue here in the next days. We also need support for overwriting existing files, but that can be implemented in a separate PR.

@csenger I've fixed upload of text documents by handling IRichtText fields. This means that in a default Plone site if somebody uploads a text file, it will create a Document and not a file. To create a file one can either provide the @type property in the Upload-Metadata header or change the configuration in the content type registry.

Member

buchi commented Apr 9, 2017

@tisto I've now implemented the TUS expiration extension which is required to cleanup files of unfinished uploads lying around. With that we have a basic implementation that could be merged regarding features. However test coverage and documentation still need some work. I won't have much time to continue here in the next days. We also need support for overwriting existing files, but that can be implemented in a separate PR.

@csenger I've fixed upload of text documents by handling IRichtText fields. This means that in a default Plone site if somebody uploads a text file, it will create a Document and not a file. To create a file one can either provide the @type property in the Upload-Metadata header or change the configuration in the content type registry.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 9, 2017

Coverage Status

Coverage decreased (-0.9%) to 95.357% when pulling a40a2a8 on tus-upload into 7f1f8b5 on master.

coveralls commented Apr 9, 2017

Coverage Status

Coverage decreased (-0.9%) to 95.357% when pulling a40a2a8 on tus-upload into 7f1f8b5 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 10, 2017

Coverage Status

Coverage decreased (-0.9%) to 95.36% when pulling ad8af00 on tus-upload into d18cbff on master.

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-0.9%) to 95.36% when pulling ad8af00 on tus-upload into d18cbff on master.

@csenger

This comment has been minimized.

Show comment
Hide comment
@csenger

csenger Apr 12, 2017

Contributor

@buchi I see the same problems with partial writes in Plone and it's far from ideal.

The spec has no explicit mechanism for temporary upload URLs. For the upload itself it is no problem. Though we can not cover common use cases like

  • showing a link to the uploaded file
  • display the file/image after the upload finished
  • link the file we created to the content it belongs to

So say your app is an issue tracker. You create a ticket and upload attachments while creating a ticket. The app won't be able to link the attachments to the ticket as it can't get the final URL of the resource.

As a pragmatic solution we could return the new location with the final 204. Reading through the tus.io JS client an app using that library can't get the location header. And as the spec does not cover the change of the resource location when the upload is finished we can only cover these use cases within the spec if we return the final location on creation.

So with all the problems it may have on the Plone side I think we need to handle those.
The best approach I can think of is storing the upload information as non content objects in the target folder and replace them with content if the upload finishes. This way we won't interfere with content space and reserve the id.

Contributor

csenger commented Apr 12, 2017

@buchi I see the same problems with partial writes in Plone and it's far from ideal.

The spec has no explicit mechanism for temporary upload URLs. For the upload itself it is no problem. Though we can not cover common use cases like

  • showing a link to the uploaded file
  • display the file/image after the upload finished
  • link the file we created to the content it belongs to

So say your app is an issue tracker. You create a ticket and upload attachments while creating a ticket. The app won't be able to link the attachments to the ticket as it can't get the final URL of the resource.

As a pragmatic solution we could return the new location with the final 204. Reading through the tus.io JS client an app using that library can't get the location header. And as the spec does not cover the change of the resource location when the upload is finished we can only cover these use cases within the spec if we return the final location on creation.

So with all the problems it may have on the Plone side I think we need to handle those.
The best approach I can think of is storing the upload information as non content objects in the target folder and replace them with content if the upload finishes. This way we won't interfere with content space and reserve the id.

@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Apr 15, 2017

Member

@csenger I get your point but I still think that an implementation that returns the final location on the initial POST request would be much more complex and not worth the trouble it causes.

I highly favor returning the location at the final PATCH request. I've tested it with the official tus-js-client and got it working without problems. I used the provided demo and the only thing I had to change is a line in the onSuccess handler. I replaced anchor.href = upload.url; with anchor.href = upload._xhr.getResponseHeader('Location');. As this applies to integration code only and doesn't touch the library itself, I see no problem doing that. Do you have any objections against this approach?

Member

buchi commented Apr 15, 2017

@csenger I get your point but I still think that an implementation that returns the final location on the initial POST request would be much more complex and not worth the trouble it causes.

I highly favor returning the location at the final PATCH request. I've tested it with the official tus-js-client and got it working without problems. I used the provided demo and the only thing I had to change is a line in the onSuccess handler. I replaced anchor.href = upload.url; with anchor.href = upload._xhr.getResponseHeader('Location');. As this applies to integration code only and doesn't touch the library itself, I see no problem doing that. Do you have any objections against this approach?

buchi added a commit that referenced this pull request Apr 16, 2017

buchi added a commit that referenced this pull request Apr 16, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 16, 2017

Coverage Status

Coverage decreased (-0.6%) to 95.633% when pulling 9f1518d on tus-upload into 1805b3c on master.

coveralls commented Apr 16, 2017

Coverage Status

Coverage decreased (-0.6%) to 95.633% when pulling 9f1518d on tus-upload into 1805b3c on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 17, 2017

Coverage Status

Coverage increased (+0.3%) to 96.547% when pulling c5860f7 on tus-upload into 1805b3c on master.

coveralls commented Apr 17, 2017

Coverage Status

Coverage increased (+0.3%) to 96.547% when pulling c5860f7 on tus-upload into 1805b3c on master.

@tisto tisto referenced this pull request Apr 17, 2017

Closed

[WIP] Roadmap to plone.restapi 1.0 #274

11 of 19 tasks complete
@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Apr 17, 2017

Member

@buchi can we remove WIP/in progress for this PR? I would like to make some final reviews and then merge asap.

Member

tisto commented Apr 17, 2017

@buchi can we remove WIP/in progress for this PR? I would like to make some final reviews and then merge asap.

@tisto tisto closed this Apr 17, 2017

@tisto tisto reopened this Apr 17, 2017

@tisto tisto added the in progress label Apr 17, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 17, 2017

Coverage Status

Coverage increased (+0.3%) to 96.554% when pulling 5902434 on tus-upload into 3b2c9e9 on master.

coveralls commented Apr 17, 2017

Coverage Status

Coverage increased (+0.3%) to 96.554% when pulling 5902434 on tus-upload into 3b2c9e9 on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 17, 2017

Coverage Status

Coverage increased (+0.3%) to 96.554% when pulling 5902434 on tus-upload into 3b2c9e9 on master.

coveralls commented Apr 17, 2017

Coverage Status

Coverage increased (+0.3%) to 96.554% when pulling 5902434 on tus-upload into 3b2c9e9 on master.

@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Apr 18, 2017

Member

@tisto Implementation and tests should be in a good shape. Docs are still missing. I'll try to finish it this week.

Member

buchi commented Apr 18, 2017

@tisto Implementation and tests should be in a good shape. Docs are still missing. I'll try to finish it this week.

@tisto tisto added this to the 1.0a14 milestone Apr 18, 2017

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Apr 24, 2017

Member

What we are missing yet are endpoints to replace the file (field) of an existing content object. Something like POST /myfolder/myfile/@upload/field and PATCH /myfolder/myfile/@upload/field. But that's out of scope for this PR.

@buchi I think we need a way to replace an uploaded file as well if we want people to use the endpoint. I could ask one of our devs to look into it this week. Any pointers or anything we need to take into account when looking into this?

Member

tisto commented Apr 24, 2017

What we are missing yet are endpoints to replace the file (field) of an existing content object. Something like POST /myfolder/myfile/@upload/field and PATCH /myfolder/myfile/@upload/field. But that's out of scope for this PR.

@buchi I think we need a way to replace an uploaded file as well if we want people to use the endpoint. I could ask one of our devs to look into it this week. Any pointers or anything we need to take into account when looking into this?

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto May 8, 2017

Member

@buchi any update on this? I would really like to merge this asap. We can provide help with docs or even implementation if necessary.

Member

tisto commented May 8, 2017

@buchi any update on this? I would really like to merge this asap. We can provide help with docs or even implementation if necessary.

buchi added a commit that referenced this pull request May 21, 2017

buchi and others added some commits Apr 16, 2017

@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Jun 14, 2017

Member

@tisto Absolutely no objections. In fact I like "tus-upload" and "tus-replace" more. I've renamed the endpoints now.

Member

buchi commented Jun 14, 2017

@tisto Absolutely no objections. In fact I like "tus-upload" and "tus-replace" more. I've renamed the endpoints now.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling 77a2b8f on tus-upload into c59d1ab on master.

coveralls commented Jun 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling 77a2b8f on tus-upload into c59d1ab on master.

@tisto tisto requested review from jaroel and tisto Jun 14, 2017

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Jun 14, 2017

Member

@buchi awesome! Again: thank you for your fantastic work! I just updated the TUS docs. Hope you are ok with that. If not, let me know.

@jaroel could please check the docs and add examples for the tus-replace endpoint tomorrow? Maybe you can also have a final look at the PR and let us know what you think.

Tomorrow is a holiday in parts of Germany and I will be off during the day. I can merge and do a release tomorrow evening though.

Member

tisto commented Jun 14, 2017

@buchi awesome! Again: thank you for your fantastic work! I just updated the TUS docs. Hope you are ok with that. If not, let me know.

@jaroel could please check the docs and add examples for the tus-replace endpoint tomorrow? Maybe you can also have a final look at the PR and let us know what you think.

Tomorrow is a holiday in parts of Germany and I will be off during the day. I can merge and do a release tomorrow evening though.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling d415922 on tus-upload into c59d1ab on master.

coveralls commented Jun 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling d415922 on tus-upload into c59d1ab on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling d415922 on tus-upload into c59d1ab on master.

coveralls commented Jun 14, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling d415922 on tus-upload into c59d1ab on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 15, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling bebbfd9 on tus-upload into c59d1ab on master.

coveralls commented Jun 15, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling bebbfd9 on tus-upload into c59d1ab on master.

@jaroel

LGTM when the additional documentation is provided, thanks!

Show outdated Hide outdated docs/source/tusupload.rst
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling 22b02da on tus-upload into c59d1ab on master.

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling 22b02da on tus-upload into c59d1ab on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling 22b02da on tus-upload into c59d1ab on master.

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.327% when pulling 22b02da on tus-upload into c59d1ab on master.

@tisto

tisto approved these changes Jun 16, 2017

@jaroel

jaroel approved these changes Jun 16, 2017

Fix permissions for PATCH/HEAD methods in `@tus-upload` endpoint
Creating a new content item requires 'Add portal content', while replacing a
file of an existing content item requires 'Modify portal content' permission.
@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Jun 16, 2017

Member

I've added a fix for the permissions of the @tus-upload endpoint.
Can provide the missing example for @tus-replace tomorrow.

Member

buchi commented Jun 16, 2017

I've added a fix for the permissions of the @tus-upload endpoint.
Can provide the missing example for @tus-replace tomorrow.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.339% when pulling bbb3704 on tus-upload into c59d1ab on master.

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.2%) to 96.339% when pulling bbb3704 on tus-upload into c59d1ab on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 17, 2017

Coverage Status

Coverage increased (+0.2%) to 96.339% when pulling 9264324 on tus-upload into c59d1ab on master.

coveralls commented Jun 17, 2017

Coverage Status

Coverage increased (+0.2%) to 96.339% when pulling 9264324 on tus-upload into c59d1ab on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 17, 2017

Coverage Status

Coverage increased (+0.2%) to 96.339% when pulling 9264324 on tus-upload into c59d1ab on master.

coveralls commented Jun 17, 2017

Coverage Status

Coverage increased (+0.2%) to 96.339% when pulling 9264324 on tus-upload into c59d1ab on master.

@buchi

This comment has been minimized.

Show comment
Hide comment
@buchi

buchi Jun 17, 2017

Member

@tisto ready

Member

buchi commented Jun 17, 2017

@tisto ready

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Jun 17, 2017

Member

@buchi WOHOOOO!!!

Member

tisto commented Jun 17, 2017

@buchi WOHOOOO!!!

@tisto tisto merged commit 9b0b084 into master Jun 19, 2017

5 checks passed

Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 96.339%
Details

@tisto tisto deleted the tus-upload branch Jun 19, 2017

@lukasgraf

This comment has been minimized.

Show comment
Hide comment
@lukasgraf

lukasgraf Jun 19, 2017

Member

🎉 💚 🎆

Member

lukasgraf commented Jun 19, 2017

🎉 💚 🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment