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

Fixed #34459 -- Fixed SearchVector() crash for parameters with % characters. #16731

Merged
merged 2 commits into from Apr 6, 2023

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Apr 5, 2023

Thanks Patryk Zawadzki for the report.

Regression in 09ffc5c.

TO DO:

  • release notes
  • test for creating and index with SearchVector (already tested in postgres_tests.test_search.SearchVectorIndexTests.test_search_vector_index)
  • add adapt_value() to psycopg_any and use it in SearchHeadline.as_sql() EDIT: unnecessary as composed options are passed in params)

@felixxm felixxm force-pushed the issur-34459 branch 4 times, most recently from 47ba207 to 69c6695 Compare April 6, 2023 04:20
# These parameters must be bound on the client side because we may
# want to create an index on this expression.
sql = connection.ops.compose_sql(sql, config_params + params + extra_params)
return sql, []
Copy link
Member Author

Choose a reason for hiding this comment

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

This was only necessary for a single test postgres_tests.test_search.SearchVectorIndexTests.test_search_vector_index, however test_search_vector_index didn't cover the way how Django handles creating new constraints. It was a temporary and bit clunky way to create a regression test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah in the end you found a nice result 👍

@felixxm felixxm changed the title [WIP] Fixed #34459 -- Fixed SearchVector() crash for parameters with % symbol. Fixed #34459 -- Fixed SearchVector() crash for parameters with % characters. Apr 6, 2023
Thanks Patryk Zawadzki for the report.

Regression in 09ffc5c.
@felixxm felixxm marked this pull request as ready for review April 6, 2023 04:46
@felixxm
Copy link
Member Author

felixxm commented Apr 6, 2023

@patrys Does it work for you?

@felixxm felixxm requested a review from a team April 6, 2023 06:30
@patrys
Copy link
Contributor

patrys commented Apr 6, 2023

@felixxm while I did not test this PR specifically, I have an identical fix in my project branch, where I copied the entire as_sql implementation to a SearchVector subclass, skipping the call to compose_sql 👍

@felixxm
Copy link
Member Author

felixxm commented Apr 6, 2023

@felixxm while I did not test this PR specifically, I have an identical fix in my project branch, where I copied the entire as_sql implementation to a SearchVector subclass, skipping the call to compose_sql +1

Thanks 👍

# These parameters must be bound on the client side because we may
# want to create an index on this expression.
sql = connection.ops.compose_sql(sql, config_params + params + extra_params)
return sql, []
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah in the end you found a nice result 👍

@felixxm
Copy link
Member Author

felixxm commented Apr 6, 2023

Thanks y'all for reviews 👍

@felixxm felixxm merged commit 4bf4222 into django:main Apr 6, 2023
26 checks passed
@felixxm felixxm deleted the issur-34459 branch April 6, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants