feat(firestore): support extended types in Node SDK#8357
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for non-native Firestore types, including MinKey, MaxKey, RegexValue, BsonObjectId, Int32Value, Decimal128Value, BsonTimestamp, and BsonBinaryData, by implementing their representation, serialization, deserialization, ordering, and validation. The review feedback highlights critical bugs such as incorrect digit indexing during mantissa parsing and a missing BigInt mask for subnormal numbers in Quadruple.fromNumber. Additionally, the reviewer recommends optimizing map representation detection and adding robust client-side validation across several type constructors to reject invalid formats, non-integers, or NaN values early.
| if (digits.length - firstDigit > QuadrupleBuilder.MAX_MANTISSA_LENGTH) { | ||
| const carry: boolean = digits[QuadrupleBuilder.MAX_MANTISSA_LENGTH] >= 5; // The highest digit to be truncated | ||
| const truncated: number[] = new Array( | ||
| QuadrupleBuilder.MAX_MANTISSA_LENGTH | ||
| ).fill(0); | ||
| for (let i = 0; i < QuadrupleBuilder.MAX_MANTISSA_LENGTH; i++) { | ||
| truncated[i] = digits[i + firstDigit]; | ||
| } |
There was a problem hiding this comment.
In parseMantissa, carry is calculated using digits[QuadrupleBuilder.MAX_MANTISSA_LENGTH]. However, if firstDigit > 0 (due to leading zeros), the kept digits are shifted, and the first truncated digit is actually at firstDigit + QuadrupleBuilder.MAX_MANTISSA_LENGTH. Using digits[QuadrupleBuilder.MAX_MANTISSA_LENGTH] results in rounding based on the wrong digit.
if (digits.length - firstDigit > QuadrupleBuilder.MAX_MANTISSA_LENGTH) {
const carry: boolean =
digits[firstDigit + QuadrupleBuilder.MAX_MANTISSA_LENGTH] >= 5; // The highest digit to be truncated
const truncated: number[] = new Array(
QuadrupleBuilder.MAX_MANTISSA_LENGTH
).fill(0);
for (let i = 0; i < QuadrupleBuilder.MAX_MANTISSA_LENGTH; i++) {
truncated[i] = digits[i + firstDigit];
}| constructor( | ||
| readonly seconds: number, | ||
| readonly increment: number | ||
| ) { | ||
| 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.
The BsonTimestamp constructor does not reject NaN or non-integer values because NaN < 0 is false and NaN > 4294967295 is false. We should use Number.isInteger to validate seconds and increment.
| constructor( | |
| readonly seconds: number, | |
| readonly increment: number | |
| ) { | |
| 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." | |
| ); | |
| } | |
| } | |
| constructor( | |
| readonly seconds: number, | |
| readonly increment: number | |
| ) { | |
| if (!Number.isInteger(seconds) || seconds < 0 || seconds > 4294967295) { | |
| throw new Error( | |
| "BsonTimestamp 'seconds' must be in the range of a 32-bit unsigned integer." | |
| ); | |
| } | |
| if (!Number.isInteger(increment) || increment < 0 || increment > 4294967295) { | |
| throw new Error( | |
| "BsonTimestamp 'increment' must be in the range of a 32-bit unsigned integer." | |
| ); | |
| } | |
| } |
| constructor( | ||
| readonly subtype: number, | ||
| readonly data: Uint8Array | ||
| ) { | ||
| // By definition the subtype should be 1 byte and should therefore | ||
| // have a value between 0 and 255 | ||
| if (subtype < 0 || subtype > 255) { | ||
| throw new Error( | ||
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The BsonBinaryData constructor does not reject NaN or non-integer values for subtype. We should use Number.isInteger to validate subtype.
| constructor( | |
| readonly subtype: number, | |
| readonly data: Uint8Array | |
| ) { | |
| // By definition the subtype should be 1 byte and should therefore | |
| // have a value between 0 and 255 | |
| if (subtype < 0 || subtype > 255) { | |
| throw new Error( | |
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | |
| ); | |
| } | |
| } | |
| constructor( | |
| readonly subtype: number, | |
| readonly data: Uint8Array | |
| ) { | |
| // By definition the subtype should be 1 byte and should therefore | |
| // have a value between 0 and 255 | |
| if (!Number.isInteger(subtype) || subtype < 0 || subtype > 255) { | |
| throw new Error( | |
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | |
| ); | |
| } | |
| } |
| if (exponent === 0) { | ||
| // subnormal - mantHi cannot be zero as that means value===+/-0 | ||
| const leadingZeros = QuadrupleBuilder.clz64(mantHi); | ||
| mantHi = leadingZeros < 63 ? mantHi << BigInt(leadingZeros + 1) : 0n; | ||
| exponent = -leadingZeros; | ||
| } |
There was a problem hiding this comment.
In Quadruple.fromNumber, when exponent === 0 (subnormal number), mantHi is shifted left by leadingZeros + 1 but is not masked with BigInt.asUintN(64, ...). Since BigInt has arbitrary precision, the shifted-out bit is kept at bit 64, making mantHi larger than 64 bits and violating the assumption that it is a 64-bit unsigned integer.
if (exponent === 0) {
// subnormal - mantHi cannot be zero as that means value===+/-0
const leadingZeros = QuadrupleBuilder.clz64(mantHi);
mantHi =
leadingZeros < 63
? BigInt.asUintN(64, mantHi << BigInt(leadingZeros + 1))
: 0n;
exponent = -leadingZeros;
}| const props = Object.keys(fields); | ||
| if ( | ||
| props.indexOf(RESERVED_MAP_KEY) !== -1 && | ||
| detectValueType(fields[RESERVED_MAP_KEY]) === 'stringValue' && | ||
| fields[RESERVED_MAP_KEY].stringValue === RESERVED_MAP_KEY_VECTOR_VALUE | ||
| ) { | ||
| return 'vectorValue'; | ||
| } else if (props.indexOf(RESERVED_MIN_KEY) !== -1) { | ||
| return 'minKeyValue'; | ||
| } else if (props.indexOf(RESERVED_MAX_KEY) !== -1) { | ||
| return 'maxKeyValue'; | ||
| } else if (props.indexOf(RESERVED_REGEX_KEY) !== -1) { | ||
| return 'regexValue'; | ||
| } else if (props.indexOf(RESERVED_BSON_OBJECT_ID_KEY) !== -1) { | ||
| return 'bsonObjectIdValue'; | ||
| } else if (props.indexOf(RESERVED_INT32_KEY) !== -1) { | ||
| return 'int32Value'; | ||
| } else if (props.indexOf(RESERVED_DECIMAL128_KEY) !== -1) { | ||
| return 'decimal128Value'; | ||
| } else if (props.indexOf(RESERVED_BSON_TIMESTAMP_KEY) !== -1) { | ||
| return 'bsonTimestampValue'; | ||
| } else if (props.indexOf(RESERVED_BSON_BINARY_KEY) !== -1) { | ||
| return 'bsonBinaryValue'; | ||
| } |
There was a problem hiding this comment.
Using Object.keys(fields) and multiple indexOf calls to probe the fields is inefficient. Since fields is a plain object, we can perform direct property checks which are
if (
fields[RESERVED_MAP_KEY] !== undefined &&
detectValueType(fields[RESERVED_MAP_KEY]) === 'stringValue' &&
fields[RESERVED_MAP_KEY].stringValue === RESERVED_MAP_KEY_VECTOR_VALUE
) {
return 'vectorValue';
} else if (fields[RESERVED_MIN_KEY] !== undefined) {
return 'minKeyValue';
} else if (fields[RESERVED_MAX_KEY] !== undefined) {
return 'maxKeyValue';
} else if (fields[RESERVED_REGEX_KEY] !== undefined) {
return 'regexValue';
} else if (fields[RESERVED_BSON_OBJECT_ID_KEY] !== undefined) {
return 'bsonObjectIdValue';
} else if (fields[RESERVED_INT32_KEY] !== undefined) {
return 'int32Value';
} else if (fields[RESERVED_DECIMAL128_KEY] !== undefined) {
return 'decimal128Value';
} else if (fields[RESERVED_BSON_TIMESTAMP_KEY] !== undefined) {
return 'bsonTimestampValue';
} else if (fields[RESERVED_BSON_BINARY_KEY] !== undefined) {
return 'bsonBinaryValue';
}| constructor( | ||
| readonly pattern: string, | ||
| readonly options: string | ||
| ) {} |
There was a problem hiding this comment.
Add client-side validation for RegexValue options to fail fast and prevent invalid options from being sent to the backend.
| constructor( | |
| readonly pattern: string, | |
| readonly options: string | |
| ) {} | |
| constructor( | |
| readonly pattern: string, | |
| readonly options: string | |
| ) { | |
| for (const char of options) { | |
| if (!['i', 'm', 's', 'u', 'x'].includes(char)) { | |
| throw new Error( | |
| `Invalid regex option '${char}'. Supported options are 'i', 'm', 's', 'u', and 'x'.` | |
| ); | |
| } | |
| } | |
| } |
| * @private | ||
| * @internal | ||
| */ | ||
| constructor(readonly value: string) {} |
There was a problem hiding this comment.
Add client-side validation for BsonObjectId length and hex format to fail fast.
| constructor(readonly value: string) {} | |
| constructor(readonly value: string) { | |
| if (value.length !== 24 || !/^[0-9a-fA-F]{24}$/.test(value)) { | |
| throw new Error('Object ID hex string has incorrect length.'); | |
| } | |
| } |
| * @private | ||
| * @internal | ||
| */ | ||
| constructor(readonly value: string) {} |
There was a problem hiding this comment.
Port of googleapis/nodejs-firestore#2337 to support extended non-native Firestore types (MinKey, MaxKey, RegexValue, BsonObjectId, Int32Value, Decimal128Value, BsonTimestamp, and BsonBinaryData) in the handwritten firestore package.