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

fix(fulltext-search): don't allow search queries of less than 3 characters (DEV-1602) #890

Merged
merged 4 commits into from
Jan 16, 2023

Conversation

mdelez
Copy link
Contributor

@mdelez mdelez commented Jan 12, 2023

resolves DEV-1602

@mdelez mdelez added the bug A bug fix label Jan 12, 2023
@mdelez mdelez self-assigned this Jan 12, 2023
@mdelez mdelez marked this pull request as ready for review January 13, 2023 12:19
@mdelez mdelez requested a review from Vijeinath January 13, 2023 12:19
@musicEnfanthen
Copy link

Sorry for jumping into this PR, but may I ask from a project perspective why this limit of 3 characters is needed? (Sorry, I do not have access to the JIRA description of the issue.)

I always thought this was a limitation of the old search engine in SALSAH? In our project, we are sometimes encountering 2-character name parts, like "Fu Du" or "Li Bai", or even catalogue work numbers like "M1" to "M9" which cannot be found with this restriction. But maybe there are still some technical reasons for the limit?

@Vijeinath
Copy link
Collaborator

Sorry for jumping into this PR, but may I ask from a project perspective why this limit of 3 characters is needed? (Sorry, I do not have access to the JIRA description of the issue.)

I always thought this was a limitation of the old search engine in SALSAH? In our project, we are sometimes encountering 2-character name parts, like "Fu Du" or "Li Bai", or even catalogue work numbers like "M1" to "M9" which cannot be found with this restriction. But maybe there are still some technical reasons for the limit?

@mdelez Maybe I wait with my review until we discussed this. This point seems valid.

@mdelez
Copy link
Contributor Author

mdelez commented Jan 16, 2023

@musicEnfanthen The API still has a requirement of at least three characters for a full text search for performance reasons. This PR makes the app handle the situation of a user searching for less than 3 characters more gracefully. If you have resources with 2 character labels, you can add an * at the end as a workaround. As long as there aren't too many resources starting with the provided 2 characters, it shouldn't be a problem.

Copy link
Collaborator

@Vijeinath Vijeinath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

@musicEnfanthen
Copy link

Ok, I see. Many thanks, @mdelez, for the explanation, and the workaround.

If you have resources with 2 character labels, you can add an * at the end as a workaround

Maybe it could be worth to add some tests for this edge cases, that is, that "aa*" really finds "aa", "aab" and "aalongword"?

@mdelez
Copy link
Contributor Author

mdelez commented Jan 16, 2023

Maybe it could be worth to add some tests for this edge cases, that is, that "aa*" really finds "aa", "aab" and "aalongword"?

This isn't something that unit tests can cover because the API calls in these tests are mocked. In other word they are fake replies because testing real API calls is more the job of e2e tests which we are currently migrating due to the e2e library we were using being deprecated.

@musicEnfanthen
Copy link

Thanks for your patience and explanations. Totally makes sense, and gives a better understanding. Many thanks!

@mdelez mdelez merged commit 5b6a131 into main Jan 16, 2023
@mdelez mdelez deleted the wip/dev-1602-search-error-less-than-3-characters branch January 16, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants