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

Read json from request BODYFILE instead of BODY. #1731

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mauritsvanrees
Copy link
Member

@mauritsvanrees mauritsvanrees commented Nov 2, 2023

This fixes a regression due to a low Zope form memory limit of 1MB used since Plone 6.0.7. See CMFPlone issue 3848 <https://github.com/plone/Products.CMFPlone/issues/3848>_ and Zope PR 1142 <https://github.com/zopefoundation/Zope/pull/1142>_.

Fixes #1730

Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for plone-restapi canceled.

Name Link
🔨 Latest commit 07821b8
🔍 Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/6543ea7088bd8300084259bb

@mister-roboto
Copy link

@mauritsvanrees thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

This fixes a regression due to a low Zope form memory limit of 1MB used since Plone 6.0.7.
See `CMFPlone issue 3848 <https://github.com/plone/Products.CMFPlone/issues/3848>`_ and `Zope PR 1142 <https://github.com/zopefoundation/Zope/pull/1142>`_.
@mauritsvanrees
Copy link
Member Author

@davisagli This might be the good direction, but lots of tests fail.
Locally (when running the tests from within the coredev 6.0 buildout) I even get an OSError because there are too many open files.

@davisagli
Copy link
Member

@mauritsvanrees Okay, let's go with #1729 for now and come back to this.

@mauritsvanrees
Copy link
Member Author

mauritsvanrees commented Nov 2, 2023

I have pushed a fix which helps: we need to make sure we read the bodyfile from the beginning.
I can't tell yet if the open files error goes away with this.

@mauritsvanrees
Copy link
Member Author

4 failures, 1 error. Not bad, but still needs some work. And it could mean just a few more test fixes, or it may mean possible breakage in third party code. So even if we go for this, it may mean a major release.
So indeed perhaps first merge and release PR #1729, use that in Plone 6.0.8, and then come back to this one.
cc @tisto.

I stop for today.

@mauritsvanrees
Copy link
Member Author

The "too many open files" error is gone at least.

@davisagli
Copy link
Member

@mauritsvanrees Not sure if this is the reason for the remaining test failures, but I think what we need to do is not always seek back to 0, but make sure that we restore the file pointer to the position it was before reading. The ValueDescriptor in Zope does this: https://github.com/zopefoundation/Zope/blob/master/src/ZPublisher/HTTPRequest.py#L1377-1379

@mauritsvanrees
Copy link
Member Author

@davisagli I restore the file position now, but that was not it.
Problem is with the @lock endpoint, where the BODYFILE is empty. json.load on an empty file gives a ValueError. So now I read the BODYFILE before passing its contents on, and use a dictionary if the contents are empty. That fixes the lock tests at least.

Yes, I said I would stop for today. ;-)

@mauritsvanrees
Copy link
Member Author

@jenkins-plone-org please run jobs

@mauritsvanrees
Copy link
Member Author

Okay, so this needs fixes in plone.volto. Maybe only updates to the tests, I don't know.

Let's go with PR #1729 for now and use that in Plone 6.0.8. It is approved already. If no one merges and releases it, I intend to do so myself Monday, and make 6.0.8 final.

Afterwards we can continue on the current PR.

@tisto
Copy link
Member

tisto commented Nov 4, 2023

@mauritsvanrees I merged #1729 and released it with plone.restapi 9.1.2. Feel free to ping me if there is anything else you would like me to do.

@mauritsvanrees
Copy link
Member Author

Thanks!

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.

json_body should not read entire request BODY
4 participants