Add Support for Case-insensitive contains dashboard filters
#24582
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
This PR is a draft because while it solves the problem of string filters being case sensitive, it doesn't necessarily
do it in the best way.
This isn't necessarily
a bug, but it seems that there is no way for the frontend to set the :case-sensitive true/false option anyway.
For the purposes of this initial attempt at a solution, I have modified the endpoint
` POST "/:dashboard-id/dashcard/:dashcard-id/card/:card-id/query"` to automatically include an option map containing
:case-sensitive false.
The machinery to take this option into consideration already exists with the default :sql ->honeysql function in the `
metabase.driver.sql.query-processor` namespace. See the `like-clause` private function in particular.
Since the query processor is a bit opaque to me at present, I was unable to figure out if there is a proper way that
the frontend could send an options map (or key-value pair) all the way through the qp middleware to the query building
stage. I discovered that if you conj a map `{:case-sensitive false}` to the output of the `to-clause` function in
`metabase.driver.common.parameters.operators`, you will get the desired case-insensitive behavior. So, I modified the
to-clause function in this PR to appropriately conj an options map if one exists.
What I'd like to know:
- is there a super-simple way to pass an option in already that I just missed? (eg. I thought perhaps in the `[:field
13 nil]` that 'nil' could be an options map, but I couldn't get that to work for me)
- is there a middleware approach that I should consider?
- any other options to appropriately hanlde this?
If the frontend sends an options map on an options key inside the parameter, this endpoint will pass that on, so no change is needed.
Codecov Report
@@ Coverage Diff @@
## master #24582 +/- ##
==========================================
- Coverage 65.29% 65.27% -0.03%
==========================================
Files 2785 2788 +3
Lines 85187 85223 +36
Branches 10506 10524 +18
==========================================
+ Hits 55623 55629 +6
- Misses 25232 25253 +21
- Partials 4332 4341 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
iethree
force-pushed
the
case-insensitive-dashboard-filter
branch
from
03de196 to
8eb221b
Compare
…ase-sensitive-issue-21359' into case-insensitive-dashboard-filter
adam-james-v
added
the
backport
github-actions bot
pushed a commit
that referenced
this pull request
Aug 4, 2022
* Dashboard Card String Filters are now case-insensitive
This PR is a draft because while it solves the problem of string filters being case sensitive, it doesn't necessarily
do it in the best way.
This isn't necessarily
a bug, but it seems that there is no way for the frontend to set the :case-sensitive true/false option anyway.
For the purposes of this initial attempt at a solution, I have modified the endpoint
` POST "/:dashboard-id/dashcard/:dashcard-id/card/:card-id/query"` to automatically include an option map containing
:case-sensitive false.
The machinery to take this option into consideration already exists with the default :sql ->honeysql function in the `
metabase.driver.sql.query-processor` namespace. See the `like-clause` private function in particular.
Since the query processor is a bit opaque to me at present, I was unable to figure out if there is a proper way that
the frontend could send an options map (or key-value pair) all the way through the qp middleware to the query building
stage. I discovered that if you conj a map `{:case-sensitive false}` to the output of the `to-clause` function in
`metabase.driver.common.parameters.operators`, you will get the desired case-insensitive behavior. So, I modified the
to-clause function in this PR to appropriately conj an options map if one exists.
What I'd like to know:
- is there a super-simple way to pass an option in already that I just missed? (eg. I thought perhaps in the `[:field
13 nil]` that 'nil' could be an options map, but I couldn't get that to work for me)
- is there a middleware approach that I should consider?
- any other options to appropriately hanlde this?
* Revert the endpoint.
If the frontend sends an options map on an options key inside the parameter, this endpoint will pass that on, so no
change is needed.
* include parameter options in datasetQuery
Co-authored-by: Adam James <[email protected]>
metabase-bot bot
added a commit
that referenced
this pull request
Aug 4, 2022
#24609) * Dashboard Card String Filters are now case-insensitive This PR is a draft because while it solves the problem of string filters being case sensitive, it doesn't necessarily do it in the best way. This isn't necessarily a bug, but it seems that there is no way for the frontend to set the :case-sensitive true/false option anyway. For the purposes of this initial attempt at a solution, I have modified the endpoint ` POST "/:dashboard-id/dashcard/:dashcard-id/card/:card-id/query"` to automatically include an option map containing :case-sensitive false. The machinery to take this option into consideration already exists with the default :sql ->honeysql function in the ` metabase.driver.sql.query-processor` namespace. See the `like-clause` private function in particular. Since the query processor is a bit opaque to me at present, I was unable to figure out if there is a proper way that the frontend could send an options map (or key-value pair) all the way through the qp middleware to the query building stage. I discovered that if you conj a map `{:case-sensitive false}` to the output of the `to-clause` function in `metabase.driver.common.parameters.operators`, you will get the desired case-insensitive behavior. So, I modified the to-clause function in this PR to appropriately conj an options map if one exists. What I'd like to know: - is there a super-simple way to pass an option in already that I just missed? (eg. I thought perhaps in the `[:field 13 nil]` that 'nil' could be an options map, but I couldn't get that to work for me) - is there a middleware approach that I should consider? - any other options to appropriately hanlde this? * Revert the endpoint. If the frontend sends an options map on an options key inside the parameter, this endpoint will pass that on, so no change is needed. * include parameter options in datasetQuery Co-authored-by: Adam James <[email protected]> Co-authored-by: Ryan Laurie <[email protected]> Co-authored-by: Adam James <[email protected]>
|
From which version the case insensitive fix is available? |
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.
Description
This adds support for query options for dashboard parameters. The current frontend implementation simply includes the default options for each operator. The practical result is that for a "contains" operator, we now run a case-insensitive query. This also enables us to allow configurable options on the frontend in the future.
Resolves #21359
Results