feat(NODE-6503): Add Int32 as a SchemaType #1

baileympearson · 2024-11-07T18:36:00Z

Note: @aditi-khare-mongoDB is the implementer, just a clarification in case it seems confusing why @baileympearson is leaving review comments.

Summary

Motivation
Support a 32-bit integer type in mongoose for compatibility with CSFLE/QE. The current Number schema type can result in either Int32, Int64, or Doubles that are inserted into the database using type inference but CSFLE and QE require exact BSON types in schemas.

Summary of Changes

Add SchemaType Int32, which has the following behavior:
- its default cast method throws when provided a
  - a non-numeric string
  - a non-integer value
  - a NaN
  - a number beyond the bounds of INT32_MAX or INT32_MIN
- it is queryable as $type: int or $type: number, but NOT $type: double
Add API Docs for new query option and new schema type

Example

const vehicleSchema = new Schema({ numberOfTires: mongoose.Int32 });

lib/schema.js

lib/browser.js

lib/schema/int32.js

docs/schematypes.md

lib/cast/int32.js

lib/schema/int32.js

lib/cast/int32.js

test/int32.test.js

baileympearson

besides removing the code from lib/types, only minor comments. Lint is failing too, btw

(also can't request changes)

docs/schematypes.md

baileympearson · 2024-11-11T22:15:18Z

lib/schema/int32.js

+  const INT32_MIN = -0x80000000;
+
+  if (v != null) {
+    if (typeof v !== 'number') {


do you think we should also confirm that it is an integer here?

I'm not sure - I wonder if that would make it too similar to the other cast function

We can leave a comment open for when we open this for Val

Yes, this function should check for integer.

The idea behind _defaultCaster() is that _defaultCaster() is the function Mongoose will use if casting is disabled using cast: false, so it should skip any type coercions (like converting strings or objects to int32).

baileympearson · 2024-11-11T22:25:01Z

lib/types/index.js

 exports.DocumentArray = require('./documentArray');
 exports.Decimal128 = require('./decimal128');
 exports.ObjectId = require('./objectid');
+exports.Int32 = require('./int32');


No - but mongoose has specific wrappers for them in lib/types. see Automattic#1671

test/int32.test.js

types/inferschematype.d.ts

lib/schema/int32.js

docs/schematypes.md

baileympearson

just one minor comment

baileympearson · 2024-11-13T16:10:46Z

lib/mongoose.js

+ *
+ * @property Int32
+ * @api public
+ */


Do we need to store a reference to our schema on the Mongoose object? My thought is no becase the majority of schema types do not. Can we remove this? (and the TS changes you made as well)?

Removed them - will add back in depending on discussion with Val

baileympearson · 2024-11-13T16:12:22Z

index.js

 module.exports.Mixed = mongoose.Mixed;
 module.exports.Date = mongoose.Date;
 module.exports.Number = mongoose.Number;
+module.exports.Int32 = mongoose.Int32;


Along with my comment in mongoose.js, i think this should be removed

Why do you think this should be removed?

Not all schema types are exported as top-level exports, so I assumed that the exported schema types are exported for a reason. Is that incorrect?

I've removed the schema type top level export for Int32 but i can add it back in case this is incorrect behavior

lib/cast/int32.js

nbbeeken · 2024-11-18T17:14:47Z

lib/cast/int32.js

+  }
+
+  let coercedVal;
+  if (val instanceof BSON.Int32 || val instanceof BSON.Double) {


we check for _bsontype because that lines up with how the serializer interprets our BSON types. instanceof is a different contract than our code expects so I think it shouldn't be introduced here so we don't create two versions of what is a true "type check"

I agree with using _bsontype rather than instanceof. Mongoose has an isBsonType() helper for this purpose.

The reason why we use _bsontype over instanceof is that it is common for Node.js apps to have multiple versions of bson floating around, and relying on instanceof can lead to hard-to-debug behavior when we get an Int32 instance from a different version of bson module

made the change!

vkarpov15 · 2024-11-18T19:55:32Z

test/int32.test.js

+  });
+
+  describe('supports the required property', function() {
+    it('when vaglue is null', async function() {


Minor typo value -> vaglue

Made the change, another good catch!

vkarpov15 · 2024-11-18T19:57:23Z

test/int32.test.js

+      const Test = mongoose.model('Test', schema);
+
+      const doc = new Test({
+        myInt: -42


If I'm reading this test correctly, myInt should be a string here

Good catch!

vkarpov15

I had a couple of minor comments, but overall this looks good, no major issues

nbbeeken · 2024-11-19T22:05:31Z

lib/cast/int32.js

+    return null;
+  }
+
+  const coercedVal = val instanceof BSON.Long ? val.toNumber() : Number(val);


#1 (comment)

Shouldn't be using instanceof here per convo ^

My bad! Fixed now!

Per Val's comment, I think it is recommended to use the isBsonType() helper in mongoose

Oh I see! I'll change it again.

nbbeeken · 2024-11-20T15:37:21Z

lib/cast/int32.js

+    return null;
+  }
+
+  const coercedVal = val instanceof BSON.Long ? val.toNumber() : Number(val);


Per Val's comment, I think it is recommended to use the isBsonType() helper in mongoose

baileympearson

I can't approve because I opened the PR but LGTM

initial commit

05ce588

aditi-khare-mongoDB changed the title ~~initial commit~~ feat(NODE-6503): Add Int32 to SchemaType Nov 7, 2024

aditi-khare-mongoDB added 2 commits November 8, 2024 02:24

ready for review

df7c715

API docs for promoteValues

68f2ebd

aditi-khare-mongoDB changed the title ~~feat(NODE-6503): Add Int32 to SchemaType~~ feat(NODE-6503): Add Int32 as a SchemaType Nov 8, 2024

aditi-khare-mongoDB reviewed Nov 8, 2024

View reviewed changes

lib/schema.js Outdated Show resolved Hide resolved

lib/browser.js Outdated Show resolved Hide resolved

lib/schema/int32.js Show resolved Hide resolved

aditi-khare-mongoDB added 2 commits November 8, 2024 15:07

requested changes

021f84b

fix failing tests

538629c

aditi-khare-mongoDB assigned baileympearson and aditi-khare-mongoDB and unassigned baileympearson Nov 8, 2024

baileympearson commented Nov 8, 2024

View reviewed changes

requested changes 2

4492a3e

baileympearson commented Nov 11, 2024

View reviewed changes

requested changes 3

8226905

baileympearson commented Nov 12, 2024

View reviewed changes

types/inferschematype.d.ts Show resolved Hide resolved

lib/schema/int32.js Show resolved Hide resolved

aditi-khare-mongoDB added 5 commits November 12, 2024 16:36

requested changes 4

7c6ff5c

requested changes 4 cont.

c5dc226

remove extranous changes

9537974

fix failing tests

49804ed

remove extranous file

c6bbafb

baileympearson commented Nov 13, 2024

View reviewed changes

docs/schematypes.md Show resolved Hide resolved

baileympearson commented Nov 15, 2024

View reviewed changes

nbbeeken reviewed Nov 18, 2024

View reviewed changes

vkarpov15 reviewed Nov 18, 2024

View reviewed changes

suggested changes

18d18a7

aditi-khare-mongoDB force-pushed the NODE-6503/int32-schemaType branch from 885d0ee to 18d18a7 Compare November 19, 2024 21:43

lint

0e4b257

aditi-khare-mongoDB requested review from nbbeeken and vkarpov15 November 19, 2024 21:51

nbbeeken requested changes Nov 19, 2024

View reviewed changes

misc change

010d1e2

aditi-khare-mongoDB requested a review from nbbeeken November 20, 2024 00:16

misc change

0f78bf1

nbbeeken requested changes Nov 20, 2024

View reviewed changes

use isBsonType

9dcada3

aditi-khare-mongoDB requested a review from nbbeeken November 20, 2024 21:37

nbbeeken approved these changes Nov 21, 2024

View reviewed changes

baileympearson commented Nov 21, 2024

View reviewed changes

aditi-khare-mongoDB closed this Nov 22, 2024

aditi-khare-mongoDB mentioned this pull request Nov 22, 2024

feat(NODE-6503): Add Int32 to SchemaType Automattic/mongoose#15054

Merged

feat(NODE-6503): Add Int32 as a SchemaType #1

feat(NODE-6503): Add Int32 as a SchemaType #1

Uh oh!

Conversation

baileympearson commented Nov 7, 2024 • edited by aditi-khare-mongoDB Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Example

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkarpov15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baileympearson commented Nov 7, 2024 •

edited by aditi-khare-mongoDB

Loading