fix: Ensure Index.Walk fetches matching foreign keys only #20097
Conversation
|
@GeorgeMac, given your experience with implementing the index, can you review this draft PR and comment about the expected behaviour of Lines 70 to 76 in b274e15 |
stuartcarnie
force-pushed
the
sgc/issues/20096
branch
from
4f9c07c
to
9ac3a16
Compare
| func TestIndex_Walk(t *testing.T) { | ||
| t.Run("only selects exact keys", func(t *testing.T) { | ||
| ctrl := gomock.NewController(t) | ||
| t.Cleanup(ctrl.Finish) |
There was a problem hiding this comment.
take it or leave it: With 1.14+ this happens by default:
https://godoc.org/github.com/golang/mock/gomock#NewController
| t.Cleanup(ctrl.Finish) |
There was a problem hiding this comment.
I have been waiting for this, however, the docs are misleading as there is no released version of gomock with this support yet:
mock@master has the Cleanup support 👍 :
latest tagged version is mock@1.4.4, which does not have Cleanup support 👎 :
| var keys [][]byte | ||
| for ik, pk := indexCursor.Next(); ik != nil; ik, pk = indexCursor.Next() { | ||
| keys = append(keys, pk) | ||
| if fk, _, err := indexKeyParts(ik); err != nil { |
There was a problem hiding this comment.
Actually @stuartcarnie you know what I've just thought of something better.
If we curse foreignKey + "/" then we push down the actual prefix to the db.
This does make me think the foreign key needs to be escaped for "/" as well though.
There was a problem hiding this comment.
It is unclear what I should change here – should we add some code to validate that foreign keys do not have / for now?
There was a problem hiding this comment.
Spoke with @GeorgeMac and I am going to reject foreign keys with a /. Clients of the Index will be expected to partition their foreign keys with characters other than a /
This commit modifies the behaviour of the indexWalk function to ensure it parses the key parts and matches the foreign key exactly. Closes #20096
This commit adds a new index and migration to the DBRP service for retrieving all database and retention policy mappings for a single organization. This change was required to resolve an invalid assumption of the DBRP service, which relied on a prefix match of the byOrgAndDatabase kv.Index when performing search operations by organization ID only. Closes #20096
stuartcarnie
force-pushed
the
sgc/issues/20096
branch
from
d32a587
to
1921d3e
Compare
Closes #20096
This PR ensures
Influx.Walkvalidates that every matched key returned by the index prefix cursor is an exact match with theforeignKeyargument. This change broke an invalid assumption made by the DBRP Find operations when searching for mappings by organisation ID only. This was fixed in a second commit.The first commit modifies the behaviour of the internal
indexWalkfunction to ensure it parses the key parts and matches the foreign key exactly. Unit tests ensure this behaviour.The second commit adds a new index and migration to the DBRP service for finding all database and retention policy mappings for a single organisation. This change was required to resolve an invalid assumption of the DBRP service, which relied on a prefix match of the
byOrgAndDatabaseindex when performing search operations using the organisation ID only.