Add numberOfFolderishDocuments to catalog metadata

[jaroel]

Add numberOfFolderishDocuments catalog metadata to allow UX affordance in folder contents in Volto.

[mister-roboto]

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

[jaroel]

Querying the catalog inside an indexer will kill any chance on having a speedy catalog rebuild. Any old DX Container is indicative of obj being a branch node instead of a leaf.

[wesleybl]

@jaroel I think this metadata will not be updated if we add a content in the container. Indexing would have to be called in the event of adding content. I saw a discussion about this at: https://community.plone.org/t/indexing-how-to-index-contained-items/17428

[jaroel]

I'm adding an indexer, not an event listener. This works just fine (tm).
Thanks for having a look though!

[wesleybl]

@jaroel surprisingly for me it works! I saw that when we add content to a container, it is reindexed. Even its modification date is changed.

@mauritsvanrees @jensens @davisagli , was that thought or is it an "accident"? Indexing is one of the most expensive things in Plone. If this has not been thought of, it would be good to remove this behavior.

[davisagli]

@wesleybl I don't think that is intentional. Unfortunately changing it could be disruptive since existing implementations are probably relying on it happening. So I would say we can change it in Plone 7 (but maybe there is a way to make it available in an opt-in fashion in Plone 6)

[mauritsvanrees]

I think it may be intentional. Think only of caching: if content is added to a container, but this does not mark the container as changed, then you will keep getting the previous version, and not see the new content.

Then again, the new content itself get indexed and causes the catalogCounter to be increased, which is usually checked in eTags by plone.app.caching. So it may not be needed for that.

[wesleybl]

@mauritsvanrees I generally don't cache with eTag. But I think it has a modified date eTag. So it might make sense to change the container modification date when content is added. Perhaps what could be improved is reindexing only the modification date, instead of reindexing all indexes.

[jensens]

Overall I think it is a bad idea to add metadata with length. And it is the wrong approach.
If you want to now if a folder has items in it, do a path query. If this is too slow, cache it. If you do not need the exact number, a boolean is enough and this speeds up the query (limit it)
I am -1 for more bloat in catalog metadata. It is a recipe for a performance bomb.

[davisagli]

@jensens I can understand not wanting to add another metadata column. But if we don't add it, then we can't add this feature. We can't do a separate path query for every item in the folder contents listing to find out whether to show a folder icon.

(IMO, we don't need the exact number of children, only whether there are 0 or 1+. Then it would be a boolean and literally 2 bytes added to the pickle of the brain, and would only need to be updated when moving from 0 to 1 or vice versa.)

I don't personally feel that strongly one way or the other. I think the feature would improve UX, and I doubt the performance impact is significant. But I also won't push for it.

[wesleybl]

I really miss a visual difference between containers and non-containers.

[djay]

@jensens in addition to what David said a straight path query wouldn't work anyway because you want to know if the page contains other folderish content types, not just images for example. So that's an even more expensive query. And you really don't want to do that once per item in a listing as David said.
But I can't really see any usecase for the number of items so a Boolean seems better.

[jensens]

@davisagli Then lets add a bool. And once a thing is considered a folder, I would stick to it, do not recalculate all the time.

As said, this is the recipe for a disaster - and given the effect (showing a different icon?) its not worth the danger.

[sneridagh]

Any update on this one? Would we want it for Plone 6.1?

[jaroel]

I need to look into this and see if I can (at low cost) just grab the length from the actual btree - it's an attribute already.

https://github.com/plone/plone.folder/blob/master/src/plone/folder/nogopip.py sorta does the same thing. Maybe I can make a FakeMetadata. Isn't that what I build plone.app.contentlisting for years ago?

Sorry, more questions that answers :)

[davisagli]

@sneridagh I wouldn't rush this one. We can add support for it in volto once the backend is implemented carefully.