Switch facet suggest request to have query in URL query params, instead of path, with proper escaping #3774
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Looks like a good catch here--thanks for this improvement. It's nice that there's an existing fixture with punctuation. One minor suggestion: could you add a new feature spec around here https://github.com/projectblacklight/blacklight/blob/main/spec/features/facets_spec.rb#L115-L123 that demonstrates that it works? |
Member
Author
Like that these punctuation can now be searched? Sure, I guess the existing fixture makes that pretty do-able. |
jrochkind
force-pushed
the
facet_suggest_in_query_params
branch
from
c731b84 to
cdee044
Compare
Member
Author
|
@seanaery done! |
jrochkind
force-pushed
the
facet_suggest_in_query_params
branch
3 times, most recently
from
ef17f4b to
a64ecc8
Compare
seanaery
approved these changes
Nov 12, 2025
…ad of path, with proper escaping It was failing prior on queries including `.` or `/`. It may have been possible to fix this with proper escaping and leave it in path component of URL -- but it's much more straightoforward to put it in query component after the `?`, and let existing escaping happening take care of it. This is a URL only used by JS fetch that shouldn't ordinarily be seen by the user, so aesthetics prob not a concern. The Blacklight CI sample data has a `topic_facet` value with a period in it (`Iran. Vizārat-i Kishvar`), and my own local data does too. I haven't seen any with `/`, but of course it's possible and should work.
jrochkind
force-pushed
the
facet_suggest_in_query_params
branch
from
a64ecc8 to
e5eab14
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was failing prior on queries including
.or/. It may have been possible to fix this with proper escaping and leave it in path component of URL (may also have to fight with rails routing to get.in path component) -- but it's much more straightoforward to put it in query component after the?, and let existing escaping happening take care of it. This is a URL only used by JS fetch that shouldn't ordinarily be seen by the user, so aesthetics prob not a concern.The Blacklight CI sample data has a
topic_facetvalue with a period in it (Iran. Vizārat-i Kishvar), and my own local data does too. I haven't seen any with/, but of course it's possible and should work.We could have made the
query_fragmentrequired for the Rails route -- but it was optional before, and current usage perhaps requires it being optional!