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.

Sign up for GitHub

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

Jump to bottom

Make primary field target adapter return early. #1851

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Make primary field target adapter return early. #1851

merged 5 commits into from
Dec 17, 2024

Conversation

mauritsvanrees
Copy link
Member

@mauritsvanrees mauritsvanrees commented Dec 12, 2024 •
edited by github-actions bot
Loading

  • When there is no primary field, do not go through all fields.
  • First compare the field name, then the permission: comparing two strings is cheaper.
  • If the field name matches, stop iterating over other fields: you have already found the field.

📚 Documentation preview 📚: https://plonerestapi--1851.org.readthedocs.build/

@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!

@mauritsvanrees
Make primary field target adapter return early.
4e928a9 
When there is no primary field, do not go through all fields.
First compare the field name, then the permission: comparing two strings is cheaper.
If the field name matches, stop iterating over other fields: you have already found the field.
@mauritsvanrees mauritsvanrees force-pushed the maurits-primary-field-target branch from 865a6a8 to 4e928a9 Compare December 12, 2024 13:47
@mauritsvanrees
Copy link
Member Author

@jenkins-plone-org please run jobs

ale-rt
ale-rt reviewed Dec 12, 2024
View reviewed changes
src/plone/restapi/serializer/dxcontent.py Outdated
@@ -220,23 +220,24 @@ def __init__(self, context, request):

def __call__(self):
primary_field_name = self.get_primary_field_name()
if not primary_field_name:
return
for schema in iterSchemata(self.context):
read_permissions = mergedTaggedValueDict(schema, READ_PERMISSIONS_KEY)

for name, field in getFields(schema).items():
Copy link
Member

Choose a reason for hiding this comment

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

Would:

field = getFields(schema).get(primary_field_name)

work, rather than looping on all the schema fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not want to change too much, but testing it seems to work, so yes, let's go for this. Change made.

davisagli
davisagli approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

This looks okay as is. The suggestion from @ale-rt also makes sense. @mauritsvanrees I'll let you decide whether to adjust it.

news/1851.bugfix Outdated Show resolved Hide resolved
@mauritsvanrees
Copy link
Member Author

I now have a totally unrelated error only on Plone 6.0 Python 3.12:

  File "/home/runner/work/plone.restapi/plone.restapi/eggs/Products.PlonePAS-8.0.4-py3.12.egg/Products/PlonePAS/config.py", line 1, in <module>
    from PIL import Image
  File "/home/runner/work/plone.restapi/plone.restapi/eggs/Pillow-9.5.0-py3.12-linux-x86_64.egg/PIL/Image.py", line 103, in <module>
    from . import _imaging as core
ImportError: libtiff.so.5: cannot open shared object file: No such file or directory

The same run last week was fine. Difference: last week this used Ubuntu 22.04. Now it uses 24.04.

In the upcoming Plone 6.0.14 I am upgrading Pillow to 11.0.0. That could magically solve it. :-/
Or should I change the workflow to use ubuntu 22 explicitly? That may be best for now. I will do so.

@mauritsvanrees
Run tests on ubuntu-22.04.
a0ba91f 
This was ubuntu-latest until last week.
With ubuntu-24.04 on Python 3.12 Pillow is not installed correctly.
See #1851 (comment)
@mauritsvanrees
Copy link
Member Author

@jenkins-plone-org please run jobs

@davisagli davisagli merged commit 93ceb88 into main Dec 17, 2024
21 of 25 checks passed
@davisagli davisagli deleted the maurits-primary-field-target branch December 17, 2024 04:56
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Dec 17, 2024
@davisagli
[fc] Repository: plone.restapi
5fbdfdc 
Branch: refs/heads/main
Date: 2024-12-16T20:56:01-08:00
Author: Maurits van Rees (mauritsvanrees) <[email protected]>
Commit: plone/plone.restapi@93ceb88

Make primary field target adapter return early. (#1851)

* Make primary field target adapter return early.

When there is no primary field, do not go through all fields.
First compare the field name, then the permission: comparing two strings is cheaper.
If the field name matches, stop iterating over other fields: you have already found the field.

* Update news/1851.bugfix

* Primary field target: get primary_field_name directly instead of iterating over all fields.

* Run tests on ubuntu-22.04.

This was ubuntu-latest until last week.
With ubuntu-24.04 on Python 3.12 Pillow is not installed correctly.
See plone/plone.restapi#1851 (comment)

* Update src/plone/restapi/serializer/dxcontent.py

---------

Co-authored-by: David Glick &lt;[email protected]&gt;

Files changed:
A news/1851.bugfix
M .github/workflows/tests.yml
M src/plone/restapi/serializer/dxcontent.py
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Dec 17, 2024
@davisagli
[fc] Repository: plone.restapi
ff061db 
Branch: refs/heads/main
Date: 2024-12-16T20:56:01-08:00
Author: Maurits van Rees (mauritsvanrees) <[email protected]>
Commit: plone/plone.restapi@93ceb88

Make primary field target adapter return early. (#1851)

* Make primary field target adapter return early.

When there is no primary field, do not go through all fields.
First compare the field name, then the permission: comparing two strings is cheaper.
If the field name matches, stop iterating over other fields: you have already found the field.

* Update news/1851.bugfix

* Primary field target: get primary_field_name directly instead of iterating over all fields.

* Run tests on ubuntu-22.04.

This was ubuntu-latest until last week.
With ubuntu-24.04 on Python 3.12 Pillow is not installed correctly.
See plone/plone.restapi#1851 (comment)

* Update src/plone/restapi/serializer/dxcontent.py

---------

Co-authored-by: David Glick &lt;[email protected]&gt;

Files changed:
A news/1851.bugfix
M .github/workflows/tests.yml
M src/plone/restapi/serializer/dxcontent.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davisagli davisagli davisagli approved these changes

@ale-rt ale-rt Awaiting requested review from ale-rt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mauritsvanrees @mister-roboto @davisagli @ale-rt