Skip to content
This repository was archived by the owner on Jul 9, 2024. It is now read-only.

reverse Timestamp() param order COMPASS-5733 #109

Merged
merged 4 commits into from
Apr 29, 2022
+1,732 −1,871
Merged

reverse Timestamp() param order COMPASS-5733 #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
454f0de
reverse Timestamp() param order
lerouxb Apr 28, 2022
32457c4
only swap if the parameters are two numbers
lerouxb Apr 28, 2022
09dba0c
no need to bump, mongodb-query-parser yet
lerouxb Apr 28, 2022
339b817
use unambiguous bson timestamp
lerouxb Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,24 @@
"bson": "^4.2.3"
},
"devDependencies": {
"@babel/core": "^7.14.3",
"@babel/core": "^7.16.0",
"@babel/plugin-proposal-class-properties": "^7.13.0",
"@babel/plugin-proposal-object-rest-spread": "^7.11.0",
"@babel/preset-env": "^7.13.12",
"@babel/preset-typescript": "^7.13.0",
"@babel/preset-env": "^7.15.6",
"@babel/preset-typescript": "^7.15.6",
"@preconstruct/cli": "^2.1.0",
"@types/bson": "^4.0.3",
"@types/estree": "^0.0.41",
"@types/jest": "^26.0.21",
"benchmark": "^2.1.4",
"bson": "^4.2.3",
"bson": "^4.6.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to bump bson so we can gain support for {t, i } in tests. See mongodb/js-bson#449

"husky": ">=5",
"jest": "^26.6.3",
"lint-staged": ">=11",
"mongodb-query-parser": "2.1.1",
"np": "^7.4.0",
"prettier": "^1.19.1",
"ts-jest": "^26.5.4",
"typescript": "^3.9.9"
"typescript": "^4.3.5"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to bump typescript because bson became typescript and it was still expecting a @types/bson which kept giving me this error:

    test/index.spec.ts:1:23 - error TS7016: Could not find a declaration file for module 'bson'. '/Users/leroux.bodenstein/mongo/ejson-shell-parser/node_modules/bson/lib/bson.js' implicitly has an 'any' type.
      Try `npm install @types/bson` if it exists or add a new declaration (.d.ts) file containing `declare module 'bson';`

    1 import * as bson from 'bson';

},
"preconstruct": {
"umdName": "EJSONShellParser",
Expand Down
6 changes: 6 additions & 0 deletions src/scope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ const SCOPE: { [x: string]: Function } = {
return new (bson as any).BSONSymbol(i);
},
Timestamp: function(low: any, high: any) {
if (typeof low === 'number' && typeof high === 'number') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only nit is possibly we need to change the parameters from low to high to something easier to read/understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on that. My current thinking (and really this is still very easy to change) is that the function gets the 32-bit words in that order. BUT Timestamp is overloaded and can take either one or two params. So in a sense it is just an args array that we have to inspect and either pass through unchanged or do something specific with in this one case.

Which is kinda the long way of saying I can't think of anything better than maybe ...args: any[].

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe that's what we should do? I agree that it probably doesn't matter much either way, we're just passing stuff in and praying it's valid anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some reference https://github.com/mongodb/js-bson/blob/dfd14e41479309d403c44dfd2234644b7c2cb786/src/timestamp.ts#L34-L45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong preferences, I think it’s fine to keep them as they are or to switch them, the important thing is that it’s visible to readers that something is going on here and I think that is being achieved :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was about to suggest calling the params a and b, but that felt about as ridiculous, so I'm going to just merge :)

// https://www.mongodb.com/docs/manual/reference/bson-types/#timestamps
// reverse the order to match the legacy shell
return new bson.Timestamp({ t: low, i: high });
}

return new bson.Timestamp(low, high);
},
ISODate: function(...args: any[]) {
Expand Down
10 changes: 7 additions & 3 deletions test/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ it('should accept a complex query', function() {
ObjectID: ObjectID("5e159ba7eac34211f2252aaa"),
ObjectId: ObjectId("5e159ba7eac34211f2252aaa"),
Symbol: Symbol('symbol'),
Timestamp: Timestamp(123, 456),
Timestamp: Timestamp(100, 0),
Timestamp_object: Timestamp({ t: 1, i: 2 }),
Timestamp_long: Timestamp(new Long(1, 2)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just trying to come up with a test case that shows that Timestamp()'s params aren't always two numbers.

ISODate: ISODate("2020-01-01 12:00:00"),
Date: Date("2020-01-01 12:00:00")
}`)
Expand Down Expand Up @@ -80,7 +82,9 @@ it('should accept a complex query', function() {
ObjectID: new bson.ObjectID('5e159ba7eac34211f2252aaa'),
ObjectId: new bson.ObjectId('5e159ba7eac34211f2252aaa'),
Symbol: new (bson as any).BSONSymbol('symbol'),
Timestamp: new bson.Timestamp(123, 456),
Timestamp: new bson.Timestamp({ t: 100, i: 0 }),
Timestamp_object: new bson.Timestamp({ t: 1, i: 2 }),
Timestamp_long: new bson.Timestamp(bson.Long.fromNumber(8589934593)),
ISODate: new Date('2020-01-01 12:00:00'),
Date: new Date('2020-01-01 12:00:00'),
});
Expand All @@ -95,7 +99,7 @@ it('should support binary operators (like plus / minus)', function() {
}`)
).toEqual({
_id: new bson.ObjectId('5e159ba7eac34211f2252aaa'),
created: new bson.Timestamp(20, 10),
created: new bson.Timestamp(10, 20),
filter: { year: { $gte: 2020 } },
});
});
Expand Down
Loading