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

OFS.ObjectManager.__contains__ optimisation conflicts with catalog #69

Closed
mauritsvanrees opened this issue Sep 8, 2016 · 5 comments
Closed
Labels

Comments

@mauritsvanrees
Copy link
Member

I am trying this on the Plone 5.0 development buildout. It is using Zope2 2.13.24. Instead I checked out Zope2 branch 2.13 and this resulted in a few failures on our Jenkins test server: http://jenkins.plone.org/job/plone-5.0-python-2.7/4932/
I focused on the plone.app.folder one:
http://jenkins.plone.org/job/plone-5.0-python-2.7/4932/testReport/junit/plone.app.folder.tests.test_integration/FolderReplacementTests/testGetObjPositionInParentIndex/

That can be seen in a debug session too:

$bin/instance debug
>>> 'getObjPositionInParent' in app.Plone.portal_catalog.indexes()
True
>>> app.Plone.portal_catalog.Indexes['getObjPositionInParent']
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/maurits/community/plone-coredev/5.0/src/Zope2/src/OFS/ObjectManager.py", line 784, in __getitem__
    raise KeyError, key
KeyError: 'getObjPositionInParent'

With Zope2 tag 2.13.24 this error goes away.
The breaking point is with the merge of pull request #62.

@hannosch
Copy link
Contributor

hannosch commented Sep 8, 2016

Looks like ZCatalog.Indexes is a ZCatalogIndexes class, which has a custom objectIds function:

https://github.com/zopefoundation/Products.ZCatalog/blob/master/src/Products/ZCatalog/ZCatalogIndexes.py#L79

That function overwrites objectIds and doesn't look into _objects, but a different place. The PR takes away the objectIds indirection and thus breaks this kind of use-case.

I'd be inclined to revert the PR on the 2.13 branch. For Zope 4 we can leave it in and extend the subclass contract by demanding subclasses also overwrite __contains__.

@tseaver
Copy link
Member

tseaver commented Sep 8, 2016

@hannosch SGTM

mauritsvanrees added a commit that referenced this issue Sep 8, 2016
It causes problems with ZCatalog indexes.
See #69

This reverts commit 753683e.
@malthe
Copy link
Contributor

malthe commented Sep 8, 2016

Inheritance in OOP is just wrong :-)

hannosch added a commit to zopefoundation/Products.ZCatalog that referenced this issue Sep 9, 2016
@hannosch
Copy link
Contributor

hannosch commented Sep 9, 2016

The optimization is undone on the 2.13 branch. For 4.0 I've updated the ZCatalogIndexes class to also implement __contains__. Grepping through the other Zope projects, I didn't find any more classes which only overrode objectIds, without also overriding contains.

@hannosch hannosch closed this as completed Sep 9, 2016
@mauritsvanrees
Copy link
Member Author

Grepping through Plone 5.0 I see the following that are affected, so having objectIds but not __contains__:

  • Products/DCWorkflow/ContainerTab.py
  • Products/PortalTransforms/chain.py
  • Products/ZopeVersionControl/ZopeRepository.py

@thet Is that maybe solved already in your branches for getting Plone to run on Zope 4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants