Escape underscores and percent signs in filter matches in Vai #94
Conversation
|
We should have integration tests for this code? |
|
@bigkevmcd, definitely. It's just that this effort may be outside the scope of this issue. @moio, what do you think? |
|
@maxsokolovsky I get that, and maybe in this case, yes, but I don't see much value in the mocked version, it's really just testing the mocks? |
|
I'm not a fan of those mock tests and we've had discussion about them during the first iteration. We've added a bullet point (used to be in the first iteration GH issue, has been moved to here) about adding integration tests. Also iirc in the very first iteration that @moio worked on, they were reusing tests from upstream's client-go, which I think is a great idea and we should bring that back (also noted in the link). It seems like even though we have that as a todo, I doubt they'll end up being a priority so maybe we should start adding pieces of non-mock tests as we work through the rest of the tasks. Would it require a lot of effort to add a couple of tests that doesn't just check the query string, but actually runs it with an in-memory sqlite database? @maxsokolovsky |
|
@tomleb / @bigkevmcd we have an open issue to backfill tests for lasso: rancher/rancher#45243. If you want to add a comment to that issue requesting that this be part of the backfill, that would be great. However, I'm not in favor of asking @maxsokolovsky to design/implement a way to do this in order for him to submit this small PR. |
|
I agree with @MbolotSuse, this is an important topic but should not be tackled in the context of this PR. @tomleb we have control over importance - there is consensus we deem it important, therefore we will make it happen at the expense of something else. I approved this PR, and plan to merge it as soon as anybody else adds another approval. |
|
@tomleb can you please take a look and approve? |
|
@maxsokolovsky I like that plan |
maxsokolovsky
force-pushed
the
allow-special-characters-as-filters-in-vai
branch
from
07069e9 to
e42a9a5
Compare
|
Added a couple of cases to our new integration tests. |
maxsokolovsky
force-pushed
the
allow-special-characters-as-filters-in-vai
branch
from
e42a9a5 to
24a44ee
Compare
maxsokolovsky
force-pushed
the
allow-special-characters-as-filters-in-vai
branch
from
24a44ee to
ceef6a7
Compare
|
Nice job, thanks for all the collaboration! |
This includes various fixes to the UI Server-Side Pagination experimental feature: https://ranchermanager.docs.rancher.com/how-to-guides/advanced-user-guides/enable-experimental-features/ui-server-side-pagination - rancher/lasso#86 - rancher/lasso#90 - rancher/lasso#85 - rancher/lasso#92 - rancher/lasso#79 - rancher/lasso#97 - rancher/lasso#94 - rancher/lasso#98 - rancher/lasso#108 - rancher/lasso#99 It also includes improvements to code collecting metrics: - rancher/lasso#89 - rancher/lasso#95 And to test code: - rancher/lasso#100 - rancher/lasso#101 Signed-off-by: Silvio Moioli <silvio@moioli.net>
Issue: rancher/rancher#46410
This PR changes the logic of how
ListOptionIndexerbuilds a SQL query from filters.The indexer will now escape two characters:
_and%. As a result, the SQL engine will interpret them literally within aLIKEstatement, not as special characters as it normally does. For example, it treats_as a placeholder for a single character. We don't want that. We want it to escape it and look for the literal underscore in a match.I adjusted existing unit tests but haven't added any new cases, because the current test infrastructure relies on mocked values too much, so I can't assert on returned values from SQL, since those are always mocked.
Added a couple of cases to our new integration tests.