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

Rework search query building #278

Merged
merged 4 commits into from
Dec 19, 2024
Merged

Conversation

danipran
Copy link
Contributor

@danipran danipran commented Dec 18, 2024

Description

This PR fixes several issues with the search:

  • Add traceback info for the failed SQL query
  • Use APIClient's data parameter for query parameters instead of manually inserting them in the URL
    • By using the data parameter, the query parameters get correctly encoded, e.g. & becomes %26
    • If the query parameters are typed manually as part of the URL, they need to be encoded, e.g. foo&bar needs to be foo%26bar, otherwise it's interpreted as a separator
  • Accept commas in queries (as stated in the API documentation)
  • Rework search query building
    • Separate into its own method
    • Treat pipe operator as an OR operator and not an AND operator
    • Split the query to OR and AND operators
    • Strip operands with single-quotes only (e.g. ''''' | foo becomes foo:*)
    • Treat commas, whitespace and ampersands as AND operators

Known issues:
Single-quotes might mess with the query, but the original implementation didn't really deal with them either, so left it as it is (aside from fixing some of the errors caused by 's.)

Context

Using & in a search query causes an error. Found other issues while investigating this, e.g. some part of the code was unreachable, foo|bar was interpreted as is (becoming foo|bar:* in the SQL), but foo| bar, foo |bar and foo | bar would turn into ANDs (becoming foo & bar:*)

PL-38

How Has This Been Tested?

Unit tests.

Manual Testing Instructions for Reviewers

You can test it through the API with different queries, e.g. http://localhost:8000/v2/search/?q=a%26b

To make the error more informative. Didn't include any traceback.
Use the APIClient data parameter for setting query
params instead of manually setting them in the URL.
The data parameter automatically encodes the query
params while the manual option does not. E.g. & needs
to be encoded, otherwise it's interpreted as a part
of the query string (?q=halli&museo results in two
query params, q and museo)

Also parametrize the test so one failing test won't
block the other tests. Makes it more compact, too.
- accept commas
- don't crash on ampersands
- split operands correctly
- strip operands with repeating single-quotes
- retain or operators
@danipran danipran requested review from japauliina, mhieta and a team December 18, 2024 07:49
@voneiden voneiden self-requested a review December 18, 2024 07:54
Copy link

@voneiden voneiden left a comment

Choose a reason for hiding this comment

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

Overall the proposed changes look good to me. There are some edge cases (like a&|b that can cause internal server errors in the q param but that's alright.

@danipran danipran merged commit 3b0edf8 into develop Dec 19, 2024
2 checks passed
@danipran danipran deleted the PL-38/fix-ampersand-in-query branch December 19, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants