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
Add an exception so that JSON POST requests inside portal_resources don't fail #61
base: main
Are you sure you want to change the base?
Conversation
@djay thanks for your contribution! Could you please add a test that helps us understanding the issue? Please add a changelog entry as well, so we get a green build. |
@djay we can not merge a pr without a changelog entry that causes the build to fail. I will have to close this issue if there is no further action. |
@displacedaussie any idea what the usecase for this PR was? |
8c44fe1
to
526af88
Compare
Most probably a blocker for plone/Products.CMFPlone#2177 |
@@ -14,6 +13,7 @@ class PloneRestLayer(PloneSandboxLayer): | |||
defaultBases = (PLONE_APP_CONTENTTYPES_FIXTURE,) | |||
|
|||
def setUpZope(self, app, configurationContext): | |||
z2.installProduct(app, 'plone.app.theming') |
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.
@displacedaussie could you please elaborate why this is necessary here? I would like to keep the plone.rest test fixture as lightweight as possible.
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.
Otherwise portal_resources doesn't exist. And that is what plone.rest is messing with the traversal of.
@@ -126,3 +126,7 @@ def test_json_request_to_view_namespace_returns_view(self): | |||
self.portal[self.portal.invokeFactory('Folder', id='folder1')] | |||
obj = self.traverse('/plone/folder1/@@folder_contents') | |||
self.assertTrue(IBrowserView.providedBy(obj), 'IBrowserView expected') | |||
|
|||
def test_json_request_to_portal_resource_returns_page_template(self): |
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.
@displacedaussie returning a page template on a JSON request violates the HTTP specs (content negotiation). If portal_resource is actually doing this should be fixed in Zope/Plone, not in plone.rest.
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.
I don't believe zope/plone is https://github.com/plone/plone.app.theming/blob/master/src/plone/app/theming/browser/themefile.py#L25.
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.
@djay @displacedaussie I think we are fixing this on the wrong level. I understand why it is easier to do this fix in plone.rest instead of on Zope level. Though, Zope/Plone is clearly doing it wrong here IMHO. Did you attempt to try to fix this in Zope directly?
@buchi I would like to hear your opinion here as well if you have time ^^^. |
@@ -126,3 +126,7 @@ def test_json_request_to_view_namespace_returns_view(self): | |||
self.portal[self.portal.invokeFactory('Folder', id='folder1')] | |||
obj = self.traverse('/plone/folder1/@@folder_contents') | |||
self.assertTrue(IBrowserView.providedBy(obj), 'IBrowserView expected') | |||
|
|||
def test_json_request_to_portal_resource_returns_page_template(self): | |||
obj = self.traverse('/plone/portal_resources') |
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.
@displacedaussie this is not a POST request. Are you sure this test fails without the fix?
@tisto It does look like the test is not really testing the right thing but does indeed give a different result when the fix is not applied. I believe the source of the request that is causing the problem is https://github.com/plone/mockup/blob/master/mockup/patterns/upload/pattern.js called from https://github.com/plone/mockup/blob/master/mockup/patterns/filemanager/pattern.js called from https://github.com/plone/mockup/blob/master/mockup/patterns/thememapper/pattern.js#L319 and all that calls to You can see it is returning a JSON. I think the source of the bug is that plone.rest is overriding this and returning its own JSON? BTW, @displacedaussie doesn't work with plone anymore. @nngu6036 is new and finishing off this work. |
@djay thanks! sets Content-Type to "application/json" which is fine and should not affect plone.rest. The other resources you linked do not have a line that I could check. Let's go step by step and create a failing test that shows the problem first, then we can discuss how to solve this. @nngu6036 would you mind creating a new pull request with just a failing test that does the exact same HTTP request that the theme editor does? (you can just check the network tab of chrome and then copy the cURL. |
Haven't created a test yet but here is what the request looks like
Given the request content type is not application/json I suspect plone.rest should not affect it. |
…to fix_portal_resources
Is merging this PR just a question of having a proper test, or is there still some need for discussion? Presuming the latter, having read through the discussion so far, it seems to me that this is pretty straightforward. Since plone.rest intercepts publishing traversal to do its things, it is reasonable to assign a degree of responsibility on it to accommodate cases such as this (=helping fix broken theme file uploads that it's causing). It's already checking for a number of special cases anyway, so Just to look at both sides of the question, the other option is, as far as I can see, implement |
@petri plone.app.theming is using the accept header "application/json" and violates common HTTP and REST API best practices (replying with 200 OK for an error and sending an error or success message in the body if I am not mistaken) for a trivial call: Fixing this in plone.app.theming is trivial and introducing a workaround in plone.rest is wrong in my opinion since it blesses violating common standards and best practices that plone.rest relies on. We can not add workarounds in plone.rest just because developers lack an understanding of the basics of HTTP and REST. Therefore the quickest way to solve this issue is to fix plone.app.theming. |
I will close this PR because it is not going to be merged into plone.rest. There is a plone.app.theming issue and this is how it should be fixed: |
@djay thanks for creating this Pull Request and help improve Plone! To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass. Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:
With this simple comment all the jobs will be started automatically. Happy hacking! |
fix for #59, plone.rest breaks uploading files in the plone theme editor