Repository navigation
fix(firestore, pipelines): send Timestamp and Date constants as timestamps - #9416
Mastersam07 wants to merge 3 commits into
Conversation
…tamps
A Timestamp passed to a pipeline as a constant was serialized as a plain
`{ seconds, nanoseconds }` map, and a Date as `{}`, so the backend
received map constants and comparisons against them never matched.
Serialize both as `{ __kind: 'timestamp', seconds, nanoseconds }`,
truncating nanoseconds to microseconds like firebase-js-sdk, and reject
out-of-range values and invalid Dates. The iOS and Android pipeline
parsers and the web builder rebuild the SDK Timestamp, including for
rawStage params, and validate the same bounds.
The Android SDK has no Timestamp Stage.withOption overload, so a
Timestamp rawStage option is rejected on Android with a validation
error; this is recorded in the platform parity registry.
…py instanceof trap
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9416 +/- ##
============================================
+ Coverage 75.07% 76.75% +1.69%
============================================
Files 620 465 -155
Lines 41243 30164 -11079
Branches 5242 3617 -1625
============================================
- Hits 30958 23149 -7809
+ Misses 8703 6141 -2562
+ Partials 1582 874 -708
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
russellwheatley
left a comment
russellwheatley
left a comment
There was a problem hiding this comment.
Thanks @Mastersam07, looks good overall and CI is green everywhere. A few small things inline, mostly cleanup.
| ), | ||
| 'options.selections[0].expr.value to be a timestamp', | ||
| ); | ||
| await expectAsyncError( |
There was a problem hiding this comment.
This runs the same execute twice to check two halves of the message. One call with the full substring would cover both, e.g. 'options.selections[0].expr.value to be a timestamp with integer seconds between -62135596800 and 253402300799 and integer nanoseconds between 0 and 999999999.', so the second call can go.
There was a problem hiding this comment.
Merged into a single expectAsyncError with the full shared substring (the path prefix differs between iOS and Android/web, but options.selections[0].expr.value to be a timestamp with … is common to all three). Fixed in 1b21050.
| } | ||
|
|
||
| export function revivePipelineValue(value: unknown): unknown { | ||
| export function revivePipelineValue(value: unknown, fieldName = 'stage.options'): unknown { |
There was a problem hiding this comment.
fieldName isn't passed by the only caller (pipeline_bridge_factory.ts), so I think this param can go.
There was a problem hiding this comment.
Removed the param; revivePipelineValue now passes 'stage.options' straight to the validator. Fixed in 1b21050.
| return value !== null && typeof value === 'object' && !Array.isArray(value); | ||
| } | ||
|
|
||
| const MIN_TIMESTAMP_SECONDS = -62135596800; |
There was a problem hiding this comment.
Same bounds are defined in pipeline_runtime.ts, and FirestoreTimestamp.ts already has MIN_SECONDS. Might be worth exporting one set so they don't drift.
There was a problem hiding this comment.
Agreed. FirestoreTimestamp.ts now exports the min/max seconds bounds and both the pipeline serializer and the web builder import them, so there's one definition. Fixed in 1b21050.
| guard let rawValue = value else { | ||
| return [] | ||
| } | ||
| let value = try RNFBFirestorePipelineParser.reviveSerializedTimestamps(rawValue, fieldName: fieldName) |
There was a problem hiding this comment.
coerceRawParamValue already walks this tree iteratively, so this adds a second full traversal plus its own frame enum and box class in the parser. Might be worth checking for the timestamp tag in coerceRawParamValue's map branch instead. coerceRawOptions would need the same check in coerceExpressionTree though, so not sure it's worth it. Up to you.
There was a problem hiding this comment.
I looked at folding it into coerceRawParamValue. I'd prefer to keep the separate walk: the same reviveSerializedTimestamps helper also handles rawStage options, which go through coerceExpressionTree, so folding it in would mean adding the tag check in two coercion paths instead of one shared helper. The extra pass is only over rawStage params/options, which are small. Happy to revisit if you feel strongly.
…he malformed-tag e2e Export the Timestamp seconds bounds from FirestoreTimestamp and use them in the pipeline serializer and the web builder instead of local copies. Drop the unused fieldName parameter from revivePipelineValue, and assert the malformed timestamp error with a single execute.
Description
A
Timestamppassed to a Firestore pipeline as a constant was serialized as a plain{ seconds, nanoseconds }map, and a JSDateas{}. Nothing on iOS, Android or web turned either back into a timestamp, so the backend received map constants: comparisons against them never matched andconstant(ts)read back as a map.Changes:
serializeValuesendsTimestampandDateas{ __kind: 'timestamp', seconds, nanoseconds }, with nanoseconds truncated to microseconds to match firebase-js-sdkconstant(). Out-of-range or non-integer values and invalidDates are rejected before execute. Timestamps are detected structurally as well as withinstanceof, because the e2e harness loadsfirestore/pipelinesfromliband the main entry fromdist.Timestampfrom the tag, includingrawStageparams, and validate the same bounds, so a malformed value is a validation error rather than a native exception.Timestampwith the same validation.Stage.withOptionhas no Timestamp overload (firebase-firestore27.0.1), so a TimestamprawStageoption is rejected on Android with a validation error. No documented pipeline stage takes a timestamp-valued option. Recorded in the platform parity registry.instanceofgotcha, inokf-bundle/packages/firestore/pipelines.md.BytesandVectorValueconstants have the same root cause and are tracked separately.Related issues
Release Summary
fix(firestore, pipelines):
TimestampandDatepipeline constants (andrawStageparams) are now sent as timestamps instead of mapsChecklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
yarn tests:jest packages/firestore/__tests__(373/373). Covers the tagged shape,Dateconversion, microsecond truncation, pre-1970 and invalidDates, range validation, nested maps/arrays,where/select/addFields/ comparison operands,rawStageparams and options, and web revive.yarn tsc:compile,yarn tsc:compile:consumer,yarn lint:js,yarn lint:deps,yarn compare:types;type-test.tscoversconstant(Timestamp)andconstant(Date).yarn tests:ios:build,yarn tests:android:build,yarn tests:android:unit.Pipeline.e2e.js): new tests read backTimestamp/Dateconstants, compare a stored timestamp againstTimestampandDateconstants (equal,greaterThan,lessThan), read backTimestamp/Datevalues passed asrawStageparams, and expect a malformed timestamp-shaped map to be rejected. Run locally on macOS and the iOS simulator; Android runs in CI.Maintainer note: Fixes internal CPRN-610