-
Notifications
You must be signed in to change notification settings - Fork 9
feat(query-parser, shell-bson-parser, constants): add shell helpers for legacy UUID MONGOSH-2486 #594
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
Conversation
| LegacyJavaUUID: function (u: any) { | ||
| if (u === undefined) { | ||
| // Generate a new UUID and format it. | ||
| u = new bson.UUID().toHexString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatting with @nbbeeken I think we were thinking of throwing an error here instead. This is when someone writes new LegacyJavaUUID() without a string in the quotes. Currently UUID creates a new UUID so I had initially written it to also generate a new UUID (of subtype 3).
Any other opinions on if it we should throw an error or allow it? I'll add an error throw otherwise. I'll ask Betsy on it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think this is meaningful to have, since the "naïve" solution of replacing it with new UUID() won't necessarily generate valid legacy UUIDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new UUID() won't necessarily generate valid legacy UUIDs
A little confused, yes new UUID will never make a legacy UUID. I'm suggesting that if the creation of new random UUIDs is desirable we have that mechanism in the UUID class but not in these helpers. Since this effort is intended to assist with viewing existing legacy uuids and querying them I think most of the time folks are going to have a UUID string on hand to construct one of these legacy forms.
LegacyJavaUUID(UUID());I don't feel uber strongly about this, totally fine to keep the creation support directly in the ctor. Just sharing the perspective I had on greatly discouraging this use
| snippet: "LegacyPythonUUID('${1:uuid}')", | ||
| }, | ||
| { | ||
| name: 'UUID', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| value: 'LegacyJavaUUID', | ||
| label: 'LegacyJavaUUID', | ||
| score: 1, | ||
| meta: 'bson-legacy-uuid', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| LegacyJavaUUID: function (u: any) { | ||
| if (u === undefined) { | ||
| // Generate a new UUID and format it. | ||
| u = new bson.UUID().toHexString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think this is meaningful to have, since the "naïve" solution of replacing it with new UUID() won't necessarily generate valid legacy UUIDs
| String.prototype.substring.call(hex, 14, 16) + | ||
| String.prototype.substring.call(hex, 12, 14); | ||
| const d = String.prototype.substring.call(hex, 16, 32); | ||
| hex = a + b + c + d; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't really care a lot, so feel free to just resolve this, but working with the binary buffers might be a bit clearer/easier
|
Oh, also – this is tagged for MONGOSH-2486 but we should be careful not to close this ticket when it's merged since mongosh still has its own shell-bson package that will need to be updated |
nbbeeken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh forgot to submit approve, I leave the no arg generate / not generate choice up to yall
|
Merging with the no arg generate in. |


MONGOSH-2486
Description
Adds
LegacyJavaUUID,LegacyCSharpUUID, andLegacyPythonUUIDhelpers to the query-parser, shell-bson-parser, and mongodb-constants. These are ways for folks to pass a UUID string in the canonical form00112233-4455-6677-8899-aabbccddeeffand have it encoded how the various legacy implementations did it. See the short history of UUIDs for more info.In the next ticket, COMPASS-9690, we'll be adding this to Compass and providing a setting to customize how we're rendering the Binary subtype 3. We won't be adding that in mongosh, we'll keep showing the format
Binary.createFromBase64('Y7mFuOjdS9qQh+RALxo/9g==', 3).https://www.mongodb.com/docs/languages/python/pymongo-driver/current/data-formats/uuid/#a-short-history-of-mongodb-uuids
Brief: UUID Support in Compass & Mongosh