Add UUID Schema Type (BSON Buffer SubType 4) #12268

hasezoey · 2022-08-12T13:28:22Z

Summary

This PR tries to add Schema Type UUIDv1 (and later UUIDv4), but is a Draft (WIP), because i think i need some help with this, because i am not familiar enough with mongoose

continuation of #3242 (which was closed by the author without comment)
fixes #3208

Current Blockages:

first UUIDv1 test fails because it times-out at doc.save, and i can figure out why doc.save times-out at that point

Questions / Notes:

i am referencing BSON Types: Binary Data for sub-types, but does anyone know what the difference between 3(UUID (old)) and 4(UUID) is?
should maybe just one UUID type be added, with options to optionally set which version, because the bson types (and seemingly mongodb) support all UUID versions?

Notes on the state of the added code:

JSDOC comments are not updated yet (have been commented-out / marked with //)
"old" code has been commented-out but not removed yet
UUIDv4 has not been added yet
more tests would be needed

Uzlopak · 2022-08-12T13:38:37Z

May I suggest something regarding UUID. I would actually prefer if you would implement a generic UUID-type. mongo is not differentiating imho between UUID v1 or v4. What we could do then, would be to assign a uuid generator to the default method of the schematype.So if somebody wants for some reason to use uuid v5 or the maybe more interesting v6, than dev can override the generator.

hasezoey · 2022-08-12T14:07:27Z

@Uzlopak thats basically what i had also suggested in Questions / Notes: section, so i would also be in favor of that

though i would like some implementation details, like should the version be required / defaulted to something like 4 or default to none, or also no version support at all, and if version support, what option name to set the version, like uuidVersion: 4?

Uzlopak · 2022-08-12T14:22:49Z

I apologize @hasezoey

I also wanted to implement at one time uuid for mongoose, so i also had once worked on that branch. So it is nice to be on the same page ;).

Regarding subtype 3 and 4 is nothing uuid itself specific, but how mongodb was storing uuids internally. See:
https://studio3t.com/knowledge-base/articles/mongodb-best-practices-uuid-data/#binary-subtypes-0x03-and-0x04

The thing is, that nodejs has builtin uuid v4. But probably v6 would be better regarding indexing, but v6 is not even implemented in the uuid package.

I expect actually no good by setting v4 as default. It could be that actually v6 is better regarding indexing (never benchmarked). But because we would decide that we ues v4 because it is builtin in node, I expect actually bike shedding. So instead to actively decide one of the uuid-versions by the implementers, they assume that v4 is probably the best for a mongo collectoin because mongoose people had decided to use it so they know more than we do.

hasezoey · 2022-08-12T14:37:50Z

The thing is, that nodejs has builtin uuid v4. But probably v6 would be better regarding indexing, but v6 is not even implemented in the uuid package.

thanks for clarifying

I expect actually no good by setting v4 as default. It could be that actually v6 is better regarding indexing (never benchmarked). But because we would decide that we ues v4 because it is builtin in node, I expect actually bike shedding. So instead to actively decide one of the uuid-versions by the implementers, they assume that v4 is probably the best for a mongo collectoin because mongoose people had decided to use it so they know more than we do.

i take from this the following:

dont set a default and just allow any uuid by default (if no specific version is set)
try to optionally support version 1-6 (ie validation, i dont know about generation)

hasezoey · 2022-08-13T11:54:32Z

with 4c50837 i have renamed it to SchemaUUID, but have not yet added extra functionality for specific versions, because for that i would like to have the tests working for now, but i still cant figure out why "doc.save" hangs

hasezoey · 2022-08-23T11:20:27Z

i think i have now resolved the errors i was encountering in the tests, it came down to using the default connection instead of the one created

hasezoey · 2022-08-23T11:31:52Z

update to the current state of the PR:

the schema type is working, both saving & retrieving
the mongodb type in the database is correct (checked with MongoDB Compass), i would like to add a test for that, but not sure how to do this
the tests pass
the jsdoc exists and is updated to the UUID type
extra options (like setting specific UUID version) are not implemented yet

Uzlopak · 2022-08-23T11:56:25Z

regarding the unit test. I think you can use directly the mongo driver and check if findOne has a raw options. Then you would get the bson Buffer. bson should be always the same structure, so you can theoretically slice the range of the field and its value and check if it has the right data.

vkarpov15

Couple of quick suggestions but otherwise looks good!

Any thoughts on keeping this in Mongoose itself versus putting uuid type in a plugin? Right now I'm leaning toward putting this in the Mongoose repo, because it doesn't add too much code weight or complexity, and because I'm starting to like adding more functionality to Mongoose vs asking people to install a bunch of plugins.

lib/schema/uuid.js

vkarpov15 · 2022-08-23T16:21:04Z

test/schema.uuid.test.js

+    assert.strictEqual(doc.x, '09190f70-3d30-11e5-8814-0f4df9a59c41');
+    await doc.save();
+
+    const query = Model.findOne({ x: '09190f70-3d30-11e5-8814-0f4df9a59c41' });


I'd like to see a test using $lte and $nin

will try to add one

i just looked into $lte, though i dont quite understand how that is supposed to work with UUID, because from what i can tell there is not "less than" or "higher than" in UUID (at least in the generic implementation and not having a specific version)

also mongodb does not list anything that $lte (or $lt, $gt, $gte) would work on UUID, do they work on buffers?

@hasezoey
Assuming that we store UUIDs as strings in MongoDB, $lt/$gt in mongo generally works with strings by sorting alphabetically. Check this mongo playground, for example.

I fail to see how that can be useful for UUIDs, though. AFAIK, UUIDs don't have any guarantees for alphabetical sorting.

Assuming that we store UUIDs as strings in MongoDB

they are not, we store them as BSON UUID Buffers (bin-type 4)

Does MongoDB support $gt, $lt, etc. for buffers? There are times when I want to sort by _id in tests to ensure consistent sort order, regardless of what that sort order is. So it would be nice to support $gt and $lt, but not strictly necessary. If MongoDB doesn't support those with buffers, let's remove support for $gt and $lt.

I think it is fine to just support uuidv4 for now. If we want to support different uuid versions, that makes me more inclined to make uuid support a plugin.

There is a plugin for mongoose 6 and uuid. But uuid is a native bson type. If it was some non-bson type then i would expect to be a plugin

Does MongoDB support $gt, $lt, etc. for buffers?

i dont quite know, but it seems at least mongoose lists it as supported for normal buffers

But uuid is a native bson type. If it was some non-bson type then i would expect to be a plugin

that is also my reasoning

I think it is fine to just support uuidv4 for now

currently we dont check for a specific version, though i can add it as a optional option like enforceVersion: 4 (or something like that)

We could just set the default field with crypto.randomUUID from node. If you want to have a different uuid version a implementer can just pass something like

const { v4, v1 } = require('uuid') new Schema({ x: { type: mongoose.Schema.Types.UUID, default: v4 }, y: [{ type: mongoose.Schema.Types.UUID, default: v1 }] })

We could just set the default field with crypto.randomUUID from node

would it be a good idea to provide a default for a specific version? when i should add it, how is this done in mongoose? (it probably is not as simple as setting .defaultOptions = { default: generateV4 }, is .auto the correct function?)

hasezoey · 2022-08-23T16:52:14Z

Any thoughts on keeping this in Mongoose itself versus putting uuid type in a plugin?

well i just did this because the old PR was just closed without comment (by the author) and it is a BSON type so i though mongoose itself should maybe support it

because they are not supported on buffers

…t null" re Automattic#12268 (comment)

…buffer

hasezoey · 2022-08-24T10:43:13Z

regarding the unit test. I think you can use directly the mongo driver and check if findOne has a raw option

added with a187b01

because it is basically a buffer

hasezoey · 2022-08-24T11:29:22Z

Update to the current state of this PR:

UUID is implemented and working
there are currently no options to set a specific uuid version to be validated against (so all version 0-9 are allowed also to be mixed)
there are no tests for $lt (and derivatives) because i dont quite know how to use this on buffers
there are no tests for $bits* operators
UUID is not added to the types yet because i have no clue where i need to add it everywhere (and auto-inference to work with string)

lib/schema/uuid.js

vkarpov15

Overall looks good. I'll do a little more review later, but I'd like to merge this into 6.7.

hasezoey · 2022-09-15T08:18:00Z

updated to add tests as "todo"s that still need to be, also merged latest master

vkarpov15 · 2022-10-24T15:43:02Z

Merging into 6.7 👍

OffensiveBias-08-145 · 2022-10-25T17:56:43Z

@vkarpov15 Looks like this got merged without types. Created an issue for it.

Fix #12593 Re: #12268 Re: #3208

feat: WIP add UUIDv1

93c2f84

feat: rename "SchemaUUIDv1" to "SchemaUUID"

4c50837

Uzlopak changed the title ~~feat: WIP add UUIDv1~~ feat: WIP add UUID Aug 13, 2022

hasezoey added 5 commits August 23, 2022 12:10

Merge branch 'master' into addUUID

7240f18

fix(uuid): change "schemaName" to be without prefix "Schema"

a0459b3

test(schema.uuid): use actual opened connection instead of default

db1076c

test(schema.uuid): remove debug logging

d7b71cf

test(schema.uuid): fix tests to pass

5e7ceba

hasezoey added 4 commits August 23, 2022 13:28

style(uuid): update jsdoc to be up-to-date comments

b8432d0

style(uuid): remove unused parameters

34c0591

style(uuid): remove old (commented-out) buffer code

6ae2800

test(schema.uuid): remove another debug logging

c53e076

vkarpov15 reviewed Aug 23, 2022

View reviewed changes

hasezoey added 3 commits August 24, 2022 12:22

fix(uuid): remove "$options" and "$regex" operators

47a0d07

because they are not supported on buffers

fix(uuid): change "_checkRequired" function to just check against "no…

c76d097

…t null" re Automattic#12268 (comment)

test(schema.uuid): check that the type in the database is actually a …

a187b01

…buffer

hasezoey added 4 commits August 24, 2022 13:03

fix(uuid): change "binaryToString" to handle input strings

7a43d4a

style(uuid): add jsdoc to helper function

6038b72

test(schema.uuid): add tests for "$in" and "$nin"

fd17d9b

test(schema.uuid): extend test to also test $all

3abfff7

fix(uuid): add "bits*" operators to "$conditionalHandlers"

a8c1380

because it is basically a buffer

vkarpov15 reviewed Aug 24, 2022

View reviewed changes

lib/schema/uuid.js Show resolved Hide resolved

hasezoey marked this pull request as ready for review September 8, 2022 07:46

vkarpov15 reviewed Sep 12, 2022

View reviewed changes

vkarpov15 added this to the 6.7 milestone Sep 12, 2022

hasezoey added 2 commits September 15, 2022 10:16

test(schema.uuid): add tests that still need to be done

4494877

Merge branch 'master' into addUUID

7fee4f2

hasezoey changed the title ~~feat: WIP add UUID~~ Add SchemaType UUID Sep 15, 2022

hasezoey changed the title ~~Add SchemaType UUID~~ Add UUID Schema Type (BSON Buffer SubType 4) Sep 21, 2022

vkarpov15 changed the base branch from master to 6.7 October 24, 2022 15:36

vkarpov15 merged commit a20f7bf into Automattic:6.7 Oct 24, 2022

hasezoey deleted the addUUID branch October 24, 2022 20:16

OffensiveBias-08-145 mentioned this pull request Oct 25, 2022

UUID is missing from Types in 6.7.0 release #12593

Closed

2 tasks

adarshmadrecha mentioned this pull request Oct 25, 2022

Make UUID a formal Schema.Type #3208

Closed

vkarpov15 added a commit that referenced this pull request Nov 2, 2022

fix(types): add UUID to types

0b78e82

Fix #12593 Re: #12268 Re: #3208

vkarpov15 mentioned this pull request Nov 2, 2022

fix(types): add UUID to types #12650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UUID Schema Type (BSON Buffer SubType 4) #12268

Add UUID Schema Type (BSON Buffer SubType 4) #12268

hasezoey commented Aug 12, 2022

Uzlopak commented Aug 12, 2022

hasezoey commented Aug 12, 2022

Uzlopak commented Aug 12, 2022

hasezoey commented Aug 12, 2022

hasezoey commented Aug 13, 2022

hasezoey commented Aug 23, 2022

hasezoey commented Aug 23, 2022

Uzlopak commented Aug 23, 2022

vkarpov15 left a comment

vkarpov15 Aug 23, 2022

hasezoey Aug 23, 2022

hasezoey Aug 24, 2022

AbdelrahmanHafez Aug 24, 2022

hasezoey Aug 24, 2022

vkarpov15 Aug 24, 2022

Uzlopak Aug 24, 2022

hasezoey Aug 25, 2022

Uzlopak Aug 25, 2022

hasezoey Aug 25, 2022

hasezoey commented Aug 23, 2022

hasezoey commented Aug 24, 2022

hasezoey commented Aug 24, 2022

vkarpov15 left a comment

hasezoey commented Sep 15, 2022

vkarpov15 commented Oct 24, 2022

OffensiveBias-08-145 commented Oct 25, 2022

Add UUID Schema Type (BSON Buffer SubType 4) #12268

Add UUID Schema Type (BSON Buffer SubType 4) #12268

Conversation

hasezoey commented Aug 12, 2022

Uzlopak commented Aug 12, 2022

hasezoey commented Aug 12, 2022

Uzlopak commented Aug 12, 2022

hasezoey commented Aug 12, 2022

hasezoey commented Aug 13, 2022

hasezoey commented Aug 23, 2022

hasezoey commented Aug 23, 2022

Uzlopak commented Aug 23, 2022

vkarpov15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasezoey commented Aug 23, 2022

hasezoey commented Aug 24, 2022

hasezoey commented Aug 24, 2022

vkarpov15 left a comment

Choose a reason for hiding this comment

hasezoey commented Sep 15, 2022

vkarpov15 commented Oct 24, 2022

OffensiveBias-08-145 commented Oct 25, 2022