Add facet count endpoint#1637
Conversation
|
@razvanMiu thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: 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! |
✅ Deploy Preview for plone-restapi canceled.
|
davisagli
left a comment
davisagli
left a comment
There was a problem hiding this comment.
I'm impressed by how little code it is.
It needs documentation and tests.
| > | ||
|
|
||
| <plone:service | ||
| method="POST" |
There was a problem hiding this comment.
It would be nice to support calling this as a GET with the query encoded in the querystring like we recently added for the @querystringsearch endpoint. It makes it possible to cache the result in varnish when varnish is configured to not cache any POST requests.
| brains = QuerystringSearch(self.context, self.request).getResults() | ||
|
|
||
| brains_rids = set(brain.getRID() for brain in brains) | ||
| index_rids = [rid for rid in index.documentToKeyMap()] if index else [] |
There was a problem hiding this comment.
We should probably consider adding an official API in ZCatalog to do a query and get the results as document ids rather than brains. There is some overhead to loading the brains for each result item (and it leads to more churn in the ZODB cache) so it could help performance.
Actually now that I see how it works, I wonder if it would perform better to get the counts for multiple facets in a single backend request, rather than a separate request for each facet. That way the main query would only need to be done once.
There was a problem hiding this comment.
Yes, we can make it return the count for multiple facets but we would still have to make a querystringsearch for each facet because we have to exclude the facet itself from the query.
There was a problem hiding this comment.
@razvanMiu oh right, I skipped over that part. Makes sense. Then there's probably less benefit from combining into a single request for multiple facets.
|
@davisagli I made the @querystring-search endpoint to also look for parameters and now the |
|
@razvanMiu @davisagli I am wondering if there is any performance impact of this feature. The querystring-search endpoint is THE endpoint that is used a lot and that requires caching. Facetting is something that has lots of possible different parameters that would fill the cache quickly. Plone/ZCatalog is not Solr/ES that is optimized for such use case. Just sharing my thoughts from the top of my head here... |
|
@tisto I also think that we should improve the API in ZCatalog and plone.app.querystring so that it's possible to get counts without full results, rather than copying so much code here for that purpose. |
✅ Deploy Preview for plone-restapi canceled.
|
…cet based just on the query criteria set on the block
|
@razvanMiu Did you see the comment on zopefoundation/Products.ZCatalog#149 that you need to sign the Zope contributor agreement? The Zope community won't review the PR until that is done. |
Yes, I will do that soon. |
6 participants
No description provided.