-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Slow Searches in Listing Views #771
Changes from 23 commits
18f64ee
b138ca5
4ab8556
19f9b0e
7dae403
4a2523f
bca4152
987b2f6
27dbff3
f114715
9afab35
59ceca8
cb4fd6d
513ede4
3e45b7a
349abd6
3c2554c
f616d5f
8628d31
6c0e9c6
9e58fe5
8b8fffa
41bcc19
5f50306
364ead8
ec04e5a
bc744e8
1fa1ff1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ Changelog | |
|
||
**Fixed** | ||
|
||
- #771 Slow Searches in Listing Views | ||
|
||
**Security** | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,8 @@ | |
# Some rights reserved. See LICENSE.rst, CONTRIBUTORS.rst. | ||
|
||
from bika.lims import api | ||
from bika.lims import logger | ||
from bika.lims.interfaces import IAnalysisRequest | ||
from bika.lims.workflow import getCurrentState | ||
from plone.indexer import indexer | ||
|
||
|
||
|
@@ -28,3 +28,41 @@ def assigned_state(instance): | |
return 'unassigned' | ||
|
||
return 'assigned' | ||
|
||
|
||
@indexer(IAnalysisRequest) | ||
def listing_searchable_text(instance): | ||
""" Indexes values of desired fields for searches in listing view. All the | ||
field names added to 'plain_text_fields' will be available to search by | ||
wildcards. | ||
Please choose the searchable fields carefully and add only fields that | ||
can be useful to search by. For example, there is no need to add 'SampleId' | ||
since 'getId' of AR already contains that value. Nor 'ClientTitle' because | ||
AR's are/can be filtered by client in Clients' 'AR Listing View' | ||
:return: values of the fields defined as a string | ||
""" | ||
entries = [] | ||
plain_text_fields = ('getId', 'getContactFullName', 'getSampleTypeTitle', | ||
'getSamplePointTitle', 'getCreatorFullName', | ||
'getProfilesTitle', 'getStorageLocationTitle', | ||
'getClientOrderNumber', 'getClientReference', | ||
'getClientSampleID', 'getTemplateTitle', ) | ||
|
||
# Concatenate plain text fields as they are | ||
for field_name in plain_text_fields: | ||
try: | ||
value = api.safe_getattr(instance, field_name) | ||
except: | ||
logger.error("{} has no attribute called '{}' ".format( | ||
repr(instance), field_name)) | ||
continue | ||
|
||
if not value: | ||
continue | ||
if isinstance(value, list): | ||
value = " ".join(value) | ||
|
||
entries.append(value) | ||
|
||
# Concatenate all strings to one text blob | ||
return " ".join(entries) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @nihadness, is it possible to provide a pluggable index function which works the same like https://github.com/senaite/senaite.core/blob/master/bika/lims/browser/bika_listing.py#L1209 ? The explicit approach here lacks custom fields and therefore won't find any remarks, dates, review_title etc. Does this search works as well case insensitive? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ramonski , it could be possible to add all the fields but it is not a good approach to save all values twice. I think beside multiplying memory of AR Listing Catalog by two, it would also slow down the queries. Default Fields such as remarks, dates, review_title and etc. can be added to the list as well, if it makes sense from functional point of view. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the memory wouldn't fall into weight compared to the explicit approach here. Using the fields from the metadata would make this whole machinery quite flexible and usable for all catalogs. New searchable values could be added w/o code change by simply adding a metadata column to the catalog. Otherwise the behavior of the search in AR listing behaves differently to all the other listings which use the metadata_search method. In fact I would need to remove the listing index which comes in from the upgrade step for my customers after this release, because they are searching for columns not listed here... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes, optimizing for performance comes with a cost. With @nihadness ' approach, the cost is that indeed, you'll need to specifically define for which fields you want a On the other hand, enabling all lists to use Disabling If we agree that some lists (only 3 I can think of) might behave differently, then there is no reason to add all metadata in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is see the need for the index @xispa, no question. I only don't understand why we not simply put the output of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, maybe I'm missing here something... What is the difference between
Ok, maybe we have some bytes more in the string, but does it really count? We could maybe skip the dates, so it would be even more similar in my opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, I think just having more bytes in the strings is acceptable at some point . But I also think-considering values are saved in the list inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Searching and finding contents in the sysstem is crucial in my opinion for user acceptance. Having a quick search, which does not find the expected results, will lower this acceptance and I'd rather accept more memory/time consumption to provide a good search facility than to lose user acceptance. Also users expect to wait after they entered a search value. We should rather improve performance on pure display sites and listings. Anyhow, please consider that in your coding @nihadness and thanks for your replies so far. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. of course, I always try to consider things we talk about in PR's, very helpful for self-improvement :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just thinking (for another PR) that maybe could be nice to create a function like |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2551,6 +2551,12 @@ def getSamplingRoundUID(self): | |
else: | ||
return '' | ||
|
||
def getStorageLocationTitle(self): | ||
""" A method for AR listing catalog metadata | ||
:return: Title of Storage Location | ||
""" | ||
return self.getStorageLocation() and self.getStorageLocation().Title() or '' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are calling here a "potentailly expensive" method twice. Better do it once and keep it in a variable |
||
|
||
@security.public | ||
def getResultsRange(self): | ||
"""Returns the valid result ranges for the analyses this Analysis | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better go here through all available columns (see: https://github.com/senaite/senaite.core/blob/master/bika/lims/browser/bika_listing.py#L1163) and call a (recursive) "stringify" function to handle lists/dictionaries and other nested types as well, e.g. like here: https://github.com/senaite/senaite.publisher/blob/master/src/senaite/publisher/reportmodel.py#L257
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see the previous comment about going through all the columns. Also bear in mind that, that index lives inside the object itself, and values are not from the catalog. So our advantage here is also that we can save more values which are not metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the index get the instance as the first argument and is therefore capable to return attributes from the object to the catalog index as well.
However, it is also possible to fetch the assigned catalog of the portal_type from the archetypes_tool and have access to all the metadata columns.
So it might be indeed an advantage to have access the attributes, but I would rather use that to augment the already existing metadata columns.
A more flexible approach makes this whole effort you invest at the moment into this much more sustainable w/o further need for code changes in the core...