plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
256 stars 192 forks source link

title_or_id wakes up the real object #1688

Open Martronic-SA opened 8 years ago

Martronic-SA commented 8 years ago

calling title_or_id on a brain after a catalog query wakes up the real object, which is a performance issue.

The reason is: plone.app.contentlisting.catalog.CatalogContentListingObject

as title_or_id is not part of catalog metadata, it will make a getObject which is weird because Title and id ar part of the catalog!

def __getattr__(self, name):
    """We'll override getattr so that we can defer name lookups to the real
    underlying objects without knowing the names of all attributes.
    """

    if name.startswith('_'):
        raise AttributeError(name)
    if hasattr(aq_base(self._brain), name):
        return getattr(self._brain, name)
    elif hasattr(aq_base(self.getObject()), name):
        return getattr(aq_base(self.getObject()), name)
    else:
        raise AttributeError(name)

Maybe plone.app.contentlisting should take care of title_or_id before calling getObject.

fredvd commented 2 years ago

I found this issue when searching for something related. Checked plone.app.contentlisting, this is not fixed a.t.m. And title_or_id is something I always assumed would be fully catalog/brain operated.

I think it's reasonably safe to insert something like this to shortcut and return title_or_id from the brain before the elif:

        elif name == "title_or_id":
            title = getattr(aq_base(self._brain), 'Title', missing)
            if title and title is not missing:
                return title
            id = getattr(aq_base(self._brain), 'id', missing)
            if id and id is not missing:
                return id

@mauritsvanrees @jensens ?

jensens commented 2 years ago

Indeed!

id = getattr(aq_base(self._brain), 'id', missing)

but this should be id = getattr(aq_base(self._brain), 'getId', missing)

yurj commented 2 years ago

isn't the fix to have title_or_id in plone.app.contentlisting.catalog.CatalogContentListingObject?

    # title_or_id
    def title_or_id(self):
        title = self._brain.Title
        if title and title is not missing:
            return title
        id = self._brain.getId
        if id and id is not missing:
            return id
mauritsvanrees commented 2 years ago

The suggestion from @yurj seems good, if it works. I wonder if some code is needed in __getattr__ anyway, otherwise this new title_or_id method is not found, but maybe I am mixing it up with __getattribute__. Dunder methods are tricky animals.

Then again, if you iterate over real brains, without having it wrapped in a content listing, brain/title_or_id will still fail. But at least it is clear then: the developer gets a traceback, instead of having a slow fallback.

mauritsvanrees commented 2 years ago

Curtesy link to the code: https://github.com/plone/plone.app.contentlisting/blob/2.0.6/plone/app/contentlisting/catalog.py#L38-L51

jensens commented 1 year ago

https://github.com/plone/plone.app.contentlisting/blob/master/plone/app/contentlisting/catalog.py#L49