-
Notifications
You must be signed in to change notification settings - Fork 253
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
feat(NODE-4855): add hex and base64 ctor methods to Binary and ObjectId #569
Conversation
src/binary.ts
Outdated
@@ -292,7 +302,13 @@ export class Binary extends BSONValue { | |||
} | |||
|
|||
inspect(): string { | |||
return `new Binary(Buffer.from("${ByteUtils.toHex(this.buffer)}", "hex"), ${this.sub_type})`; | |||
if (this.position === 0) { |
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.
We had a bug here where the default new Binary()
printed:
Binary(Buffer.from("00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", "hex"), 0)
Which isn't the same as what would end up in BSON, b/c position determines how many bytes are serialized. The above constructs a Binary where position is set to 256, instead of set to zero.
// Throw an error if it's not a valid setup | ||
if (typeof hexString === 'undefined' || (hexString != null && hexString.length !== 24)) { | ||
throw new BSONError( | ||
'Argument passed in must be a single String of 12 bytes or a string of 24 hex characters' |
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.
Looks like a copy pasta mistake, the if stmt asserts for 24 characters, so a 12 byte string wouldn't work here.
|
||
describe('class Binary', () => { | ||
context('constructor()', () => { | ||
it('creates an 256 byte Binary with subtype 0 by default', () => { |
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.
Just testing the existing behavior here.
it(`${name} returns string that is runnable and has deep equality`, () => { | ||
const bsonValue = factory(); | ||
const ctx = { ...BSON, module: { exports: { result: null } } }; | ||
vm.runInNewContext(`module.exports.result = ${bsonValue.inspect()}`, ctx); |
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.
This will throw if we mistakenly introduce a syntax error or if we produce strings that when invoked lead to runtime errors
Description
What is changing?
Adds APIs to create BSON Binary / UUID / ObjectId types from hex and base64 strings.
Also corrects Binary.prototype.inspect to print a roundtrip empty instance. As well as use the new base64 helper for more concise usage.
What is the motivation for this change?
Printing out
Buffer.from()
as the first argument to Binary was clunky and didn't provide the best way to reproduce a Binary instance from the visible output.Double check the following
npm run lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript