Skip to content

REP-6465 Fix handling of dotted shard keys #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

FGasper
Copy link
Collaborator

@FGasper FGasper commented Aug 13, 2025

This fixes Migration Verifier’s logic to assemble a document key given a full document and field names when any of those fields has a dot.

When the server routes a document whose collection is sharded on, e.g., {foo.bar: 1}, it does so based on the value of /foo/bar (i.e., the bar field of the top-level embedded document named foo) in the document. Migration Verifier instead assembled its document key from the value at /foo.bar (i.e., a top-level field named foo.bar).

This fixes the bug. Notably, it also avoids the problem, documented in SERVER-109340, where the server’s change stream’s documentKey inaccurately reports the values used to route the document.

@FGasper FGasper requested a review from mmcclimon August 19, 2025 18:06
@FGasper FGasper marked this pull request as ready for review August 19, 2025 18:06
@FGasper FGasper changed the title Rep 6465 fix dotted shard key REP-6465 Fix handling of dotted shard keys Aug 19, 2025
Copy link
Collaborator

@mmcclimon mmcclimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, this seems fine? I'm deeply suspicious of the aggregation pipeline because I do not feel at all qualified to review that, so you might also get a review from someone that does. The tests seem to pass though, but you might want some more cases for weird things (notably, arrays where you might get depth-traversal errors).

parts := strings.Split(field, ".")
val, err := doc.LookupErr(parts...)

if errors.Is(err, bsoncore.ErrElementNotFound) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably also want to handle bsoncore.ErrInvalidDepthTraversalError here, which is the kind of error you get if you have a shard key like a.b and the a field in the target document is an array.


require.NoError(changes.Err(), "change stream should not fail")

changes.Close(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically this is called in a defer right after we open successfully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t have it that way because it was in a loop, but I’ve rewritten it now to be a subtest, which has its own callback and so can use defer.

FGasper and others added 4 commits August 19, 2025 15:59
Co-authored-by: Michael McClimon <michael@mcclimon.org>
…gration-verifier into REP-6465-fix-dotted-shard-key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants