feat(firestore): BSON types#10099
Conversation
* add decimal128 * add new tests * format * resolve comments * update comments * refactor compareNumbers * copy paste Quadruple code * resolve comments 1 * make Quadruple value in decimal128 private
🦋 Changeset detectedLatest commit: a643098 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for BSON types (including BsonBinaryData, BsonObjectId, BsonTimestamp, Decimal128Value, Int32Value, MaxKey, MinKey, and RegexValue) across both the standard and Lite Firestore SDKs. The changes span API definitions, serialization, indexing, query validation, and local store comparisons, supported by new utility classes for 128-bit decimal operations and extensive integration tests. Feedback on the implementation highlights opportunities to improve safety and compatibility, such as using optional chaining to prevent runtime errors when writing BSON timestamp index maps, validating integer inputs for BSON types, using SDK-consistent FirestoreError classes, and replacing ES2022 Uint8Array.prototype.at() with standard index access for broader environment compatibility.
| const fields = mapValue.fields || {}; | ||
| if (fields) { | ||
| // The JS SDK encodes BSON timestamps differently than the backend. | ||
| // This is due to the limitation of `number` in JS which handles up to 53-bit precision. | ||
| this.writeIndexMap( | ||
| fields[RESERVED_BSON_TIMESTAMP_KEY].mapValue!, | ||
| encoder | ||
| ); | ||
| } |
There was a problem hiding this comment.
The if (fields) check is redundant because fields is initialized to mapValue.fields || {} which is always truthy. More importantly, accessing fields[RESERVED_BSON_TIMESTAMP_KEY].mapValue! directly can cause a runtime TypeError if the key is missing. Using optional chaining and checking if the map exists before writing is much safer.
const fields = mapValue.fields || {};
const timestampMap = fields[RESERVED_BSON_TIMESTAMP_KEY]?.mapValue;
if (timestampMap) {
// The JS SDK encodes BSON timestamps differently than the backend.
// This is due to the limitation of `number` in JS which handles up to 53-bit precision.
this.writeIndexMap(timestampMap, encoder);
}| constructor(readonly subtype: number, data: Uint8Array) { | ||
| if (subtype < 0 || subtype > 255) { | ||
| throw new FirestoreError( | ||
| Code.INVALID_ARGUMENT, | ||
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | ||
| ); | ||
| } | ||
| this.subtype = subtype; |
There was a problem hiding this comment.
The assignment this.subtype = subtype; is redundant because readonly subtype: number in the constructor parameter list automatically declares and initializes it as a class property. Additionally, we should validate that subtype is an integer to prevent floating-point values or NaN from being silently accepted.
| constructor(readonly subtype: number, data: Uint8Array) { | |
| if (subtype < 0 || subtype > 255) { | |
| throw new FirestoreError( | |
| Code.INVALID_ARGUMENT, | |
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | |
| ); | |
| } | |
| this.subtype = subtype; | |
| constructor(readonly subtype: number, data: Uint8Array) { | |
| if (subtype < 0 || subtype > 255 || !Number.isInteger(subtype)) { | |
| throw new FirestoreError( | |
| Code.INVALID_ARGUMENT, | |
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | |
| ); | |
| } |
| constructor(readonly seconds: number, readonly increment: number) { | ||
| // Make sure 'seconds' and 'increment' are in the range of a 32-bit unsigned integer. | ||
| if (seconds < 0 || seconds > 4294967295) { | ||
| throw new Error( | ||
| "BsonTimestamp 'seconds' must be in the range of a 32-bit unsigned integer." | ||
| ); | ||
| } | ||
| if (increment < 0 || increment > 4294967295) { | ||
| throw new Error( | ||
| "BsonTimestamp 'increment' must be in the range of a 32-bit unsigned integer." | ||
| ); | ||
| } |
There was a problem hiding this comment.
We should validate that seconds and increment are integers to prevent floating-point values or NaN from being silently accepted. Also, using FirestoreError with Code.INVALID_ARGUMENT is much more consistent with the rest of the Firestore SDK API than a generic Error. Note: You will need to import Code and FirestoreError from ../util/error at the top of the file.
constructor(readonly seconds: number, readonly increment: number) {
// Make sure 'seconds' and 'increment' are in the range of a 32-bit unsigned integer.
if (seconds < 0 || seconds > 4294967295 || !Number.isInteger(seconds)) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
"BsonTimestamp 'seconds' must be in the range of a 32-bit unsigned integer."
);
}
if (increment < 0 || increment > 4294967295 || !Number.isInteger(increment)) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
"BsonTimestamp 'increment' must be in the range of a 32-bit unsigned integer."
);
}| if (bytes.length === 0) { | ||
| throw new Error('Received empty bytesValue for BsonBinaryData'); | ||
| } | ||
| const subtype = bytes.at(0); |
There was a problem hiding this comment.
Uint8Array.prototype.at() is an ES2022 feature and may not be supported in all target environments (older browsers or Node.js versions) without a polyfill. Using standard index access bytes[0] is fully compatible, safer, and more performant.
| const subtype = bytes.at(0); | |
| const subtype = bytes[0]; |
* feat(firestore): Merge `BsonBinaryData` into `Bytes` - Removed the BsonBinaryData class and merged its functionality into the Bytes class. - The Bytes class now supports an optional subtype parameter (defaults to 0, range 0 to 255). - Added a readonly subtype property and a data getter on the Bytes class. - Updated Bytes serialization (toJSON and fromJSON) to support subtypes, omitting it when subtype is 0. - Updated the local index writer to encode Bytes with subtype 0 identically to standard blobs to ensure consistent sorting. - Updated internal readers, writers, and enums to map BSON binary data to Bytes. * formatting * lint and format * fix lint * chore(firestore): remove unrelated BSON Int32Value numeric increment test case from dl/bsonbinary * fix format
3 participants
Draft