You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Nice fix — the two-layer approach (narrowed LIKE + exact-match filter in shardsForKeyspace) is the right shape. Worth noting the shardsForKeyspace exact-match is what actually makes it bulletproof: the LIKE "<db>/%" pattern alone still over-matches, since psc.Database is interpolated raw and _/% are LIKE wildcards (a keyspace named foo_bar would match fooXbar/...). The exact ks != keyspace check covers that, so the result is correct regardless. 👍
A couple of minor nits:
The removed // TODO: is there a prepared statement equivalent? is still valid. The query still interpolates psc.Database into a string literal, so the parameterization concern (and the LIKE-wildcard / unescaped-quote edge) hasn't gone away. Consider keeping the TODO, or binding the parameter:
p.db.QueryContext(ctx, `show vitess_shards like ?`, psc.Database+"/%")
That also closes the case where a keyspace name contains a ".
No integration coverage of the query change itself. The unit test exercises shardsForKeyspace but not the like "<db>/%" narrowing. Since test: add gated integration test suite #88 added the gated integration suite, a sibling-keyspace case there would lock in the half of the fix the unit test can't reach. Optional, given the defensive filter makes the result correct either way.
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
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.
Fixes #90