Repository navigation
Conversation
Coverage Report✅ Coverage 79.80% meets 40% threshold Total Coverage: 79.80% Package Breakdown
Minimum threshold: 40% |
12a6616 to
c007ec2
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive updates to the Firestore Pipeline API, including support for FieldPath objects across various methods, enhanced validation for document references, and expanded E2E test coverage. I have reviewed the comments and identified several critical issues regarding missing FieldPath support in ascending, descending, findNearest, and removeFields, as well as a validation gap in the documents method. I have kept all these actionable comments, as they point to functional bugs or API inconsistencies that need to be addressed.
| PipelineOrdering ascending(Object expression) { | ||
| return PipelineOrdering._('ascending', expression); | ||
| return PipelineOrdering._( | ||
| 'ascending', | ||
| expression is String ? field(expression) : expression, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The ascending helper currently only checks if expression is a String to convert it to a PipelineField via field(). If a FieldPath is passed directly, it will bypass this check and be treated as a constant expression (constant(FieldPath)), which is incorrect and will fail during serialization or backend execution. We should update the check to also support FieldPath.
PipelineOrdering ascending(Object expression) {
return PipelineOrdering._(
'ascending',
expression is String || expression is FieldPath
? field(expression)
: expression,
);
}| PipelineOrdering descending(Object expression) { | ||
| return PipelineOrdering._('descending', expression); | ||
| return PipelineOrdering._( | ||
| 'descending', | ||
| expression is String ? field(expression) : expression, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The descending helper currently only checks if expression is a String to convert it to a PipelineField via field(). If a FieldPath is passed directly, it will bypass this check and be treated as a constant expression (constant(FieldPath)), which is incorrect and will fail during serialization or backend execution. We should update the check to also support FieldPath.
PipelineOrdering descending(Object expression) {
return PipelineOrdering._(
'descending',
expression is String || expression is FieldPath
? field(expression)
: expression,
);
}| final refs = [ | ||
| for (final document in documents) | ||
| switch (document) { | ||
| String() => _firestore.doc(document), | ||
| DocumentReference() => document, | ||
| _ => throw ArgumentError.value( | ||
| document, | ||
| 'documents', | ||
| 'Expected a DocumentReference or a document path.', | ||
| ), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
The documents method's doc comment states that it throws an ArgumentError when any of the provided document references target a different database than this Pipeline. However, the implementation does not perform this validation for DocumentReference elements. We should add a call to _validateSameDatabase to enforce this constraint as documented.
final refs = <DocumentReference<dynamic>>[];
for (final document in documents) {
if (document is String) {
refs.add(_firestore.doc(document));
} else if (document is DocumentReference) {
_validateSameDatabase(_firestore, document.firestore, 'documents');
refs.add(document);
} else {
throw ArgumentError.value(
document,
'documents',
'Expected a DocumentReference or a document path.',
);
}
}| return _findNearest( | ||
| vectorField: vectorField is String ? field(vectorField) : vectorField, | ||
| queryVector: queryVector, | ||
| distanceMeasure: distanceMeasure, | ||
| limit: limit, | ||
| distanceField: distanceResultField == null | ||
| ? null | ||
| : field(distanceResultField), | ||
| distanceThreshold: distanceThreshold, | ||
| rawOptions: rawOptions, | ||
| ); |
There was a problem hiding this comment.
In findNearest, vectorField is only converted to a PipelineField via field() if it is a String. If a FieldPath is passed directly, it will bypass this check and be passed directly to _findNearest and _stage, which will fail to serialize it correctly. We should update the check to also support FieldPath.
return _findNearest(
vectorField: vectorField is String || vectorField is FieldPath
? field(vectorField)
: vectorField,
queryVector: queryVector,
distanceMeasure: distanceMeasure,
limit: limit,
distanceField: distanceResultField == null
? null
: field(distanceResultField),
distanceThreshold: distanceThreshold,
rawOptions: rawOptions,
);| return _stage('remove_fields', [ | ||
| for (final value in fields) | ||
| if (value is String) field(value) else value, | ||
| ]); | ||
| ], rawOptions: rawOptions); |
There was a problem hiding this comment.
In removeFields, if value is a FieldPath, it is not converted to a PipelineField via field(), which will cause serialization to fail since FieldPath is not a PipelineExpression or primitive value. We should update the check to also support FieldPath.
return _stage('remove_fields', [
for (final value in fields)
if (value is String || value is FieldPath) field(value) else value,
], rawOptions: rawOptions);| (Object expression, PipelineField target) _selectableParts(Object selectable) { | ||
| return switch (selectable) { | ||
| String() => (field(selectable), selectable), | ||
| PipelineField() => (selectable, selectable.path), | ||
| PipelineAliasedExpression() => (selectable.expression, selectable.name), | ||
| String() => (field(selectable), field(selectable)), | ||
| PipelineField() => (selectable, selectable), | ||
| PipelineAliasedExpression() => ( | ||
| selectable.expression, | ||
| field(selectable.name), | ||
| ), |
There was a problem hiding this comment.
In _selectableParts, adding support for FieldPath directly will allow users to pass FieldPath objects to select, unnest, distinct, and aggregate groups, making the API much more consistent and robust across all Pipeline methods that accept field paths.
(Object expression, PipelineField target) _selectableParts(Object selectable) {
return switch (selectable) {
String() => (field(selectable), field(selectable)),
FieldPath() => (field(selectable), field(selectable)),
PipelineField() => (selectable, selectable),
PipelineAliasedExpression() => (
selectable.expression,
field(selectable.name),
),…erage Split the live suite into test/e2e/pipeline/ by area, with a shared harness that seeds one run per file, documents the seed data and runs one test per case. Add test/pipeline_e2e_coverage_test.dart, a credential-free guard that resolves the E2E sources with package:analyzer and fails unless every public Pipeline member, optional parameter, value position, required argument shape and enum value is exercised live.
…nt Pipeline functions Several Pipeline functions were built separately by each of their forms, which is how the static and fluent forms drifted apart before (different backend names, argument conversions and optional arguments). Make every form of a function forward, in one call and with its parameters in order, to a single implementation: - the top-level comparisons, and, or and not now call their PipelineFunctions counterparts instead of building the function themselves; the private _comparison helper is gone; - the top-level field, constant and variable forward to Expression, which holds them because a bare name inside Expression resolves to its own member; - Expression.raw forwards to PipelineFunctions.raw, which gains the options parameter it lacked; - PipelineExpression.arraySlice always calls PipelineFunctions.arraySlice, whose length is now optional like the fluent form and the Node SDK, instead of building a raw array_slice when length was omitted; - PipelineExpression.arrayContainsAll and arrayContainsAny take an Object? like their static forms and the Node SDK, so they also accept an array expression; - PipelineField no longer overrides ascending and descending with copies of the inherited methods. The encoded requests are unchanged for every valid input.
…TransformWithIndex PipelineFunctions.arrayTransform took an optional trailing index alias that its fluent form did not, so the fluent arrayTransformWithIndex had to call it with its arguments reordered. Follow the Node SDK instead: arrayTransform takes (array, elementAlias, transform) and the new PipelineFunctions.arrayTransformWithIndex takes (array, elementAlias, indexAlias, transform), which PipelineExpression.arrayTransformWithIndex now forwards to. Both send the same array_transform function as before. BREAKING CHANGE: PipelineFunctions.arrayTransform no longer accepts a fourth index-alias argument. Call PipelineFunctions.arrayTransformWithIndex(array, elementAlias, indexAlias, transform) instead.
…g as a field
ascending('rating') sent the string constant 'rating' as the sort
expression instead of the field, unlike field('rating').ascending(),
every other Pipeline helper that takes a field name, and the Node SDK's
ascending(fieldName). A String now names a field.
Add pipeline_single_implementation_test.dart, which resolves lib/src/pipeline.dart with package:analyzer and groups its public members by name across the forms a Pipeline function can take (PipelineFunctions statics, Expression statics, top-level functions and PipelineExpression methods). For every name written in more than one form, every form but the first must be a single call forwarding its parameters, in order and with the same optionality and defaults, to that first form; fluent members with no same-named counterpart, such as toLowerCase, must forward to the function they alias. A documented exceptions map covers concat, ascending and descending, and stale entries fail. The same file encodes every pair through each of its forms, with field names, expressions, literals and optional arguments, and checks the captured ExecutePipelineRequest JSON is identical. A pair without a behavioural entry, or an entry that skips one of its forms, fails. The analyzer dev dependency allows 10.x through 14.x; the checker avoids the argument AST types that changed in analyzer 13.
Add a golden wire-format harness for Firestore Pipelines. The expected ExecutePipelineRequests come from the Node.js SDK, never from this package, so a test can no longer pass by asserting our own output. tool/pipeline_golden builds 840 named Pipelines with a pinned @google-cloud/firestore (9.3.1) and records the request it sends, by stubbing Firestore.requestStream, as proto3 JSON fixtures. The corpus covers every source, createFrom of collection, collection-group and vector queries, every stage, every function in both its top-level and Expression method forms, every aggregate and the execute options; the generator fails if a Node function or method has no case. test/pipeline_golden_test.dart builds the same Pipelines with the Dart SDK and requires an identical request. Intended differences, pending fixes and Node APIs Dart cannot express are listed explicitly, and each entry is checked so it cannot silently go stale. A path-filtered workflow regenerates the fixtures and fails when the committed ones are stale.
Unit tests only exercise the signature they were written against, so they missed Pipeline API mismatches such as an optional join() delimiter or a mapRemove() that took an Iterable of keys. tool/pipeline_api_parity extracts the Pipelines declarations of the pinned @google-cloud/firestore 9.3.1 into test/fixtures/node_pipeline_api.json with the TypeScript compiler API. test/pipeline_api_parity_test.dart reads the Dart API with package:analyzer and compares the two: missing and Dart-only members, argument counts and optional positions, single values versus lists, accepted kinds of values, options objects versus named parameters, return types, the isType values, and barrel exports. Intentional differences are listed with their reasons, and the gaps that still need a fix in lib/ are listed separately; stale entries fail the test. A workflow regenerates the snapshot on google_cloud_firestore changes and fails when the committed file is out of date.
… refactor The refactor fixed arraySlice, arrayTransform(WithIndex) and the fluent arrayContains* signatures, and top-level ascending/descending, so their pending entries are gone; the golden cases now call arrayTransformWithIndex.
Fill the E2E suite until the coverage guard reports no gap: every stage, source, result accessor, execute option, enum value and function (static and fluent) now runs against the live database, with optional arguments both supplied and omitted, expressions and plain values in value positions, collections holding expressions and numeric lists for vectors. Exempt PipelineFunctions.raw(options) like its Expression.raw counterpart.
… Node
Mirror the Node SDK's OptionsUtil and rawStage, found by the golden
harness:
- rawOptions keys are dot-separated paths. `execute(rawOptions:)`,
`Transaction.executePipeline(rawOptions:)` and `rawStage(options:)`
expand `'a.b'` into nested maps and overlay them on the typed options
key by key, so `explain:` plus `{'explain_options.output_format': ...}`
sends one explain_options map. Keys with an empty segment throw an
ArgumentError, as Node does.
- rawStage sends a Map param as a literal map, but converts the
collections nested in it to map(...)/array(...) functions; prod rejects
expressions inside a nested literal map. Other params, lists included,
are still sent as is, like Node.
- Every stage and source takes an optional `rawOptions`, as every Node
stage does, overlaid on the stage's typed options.
Golden cases cover rawStage params (maps and lists, with and without
expressions, nested), dotted option keys and their ordering rules for
stages and execute, and rawOptions on every stage and source.
FieldPath's canonical form replaced a backtick inside a quoted segment
with a lone backslash, so where('a`b', ...) sent `a\b`, a different
field. It now escapes it as \`, like the Node SDK's
FieldPath.formattedName. This affects every request that sends a field
path: queries, field masks, updates and transforms.
field() sent its argument verbatim, so field('first-name') reached the
backend as first-name instead of `first-name`. It now sends the
canonical field path, built with FieldPath's existing formatting: a
String is split on dots into segments, and each segment that is not a
simple identifier is backtick-quoted. Every String field name a stage
or function takes goes through field(), so removeFields, sort, unnest,
findNearest, function targets and the select, distinct and aggregate
group values are quoted too.
As in the Node SDK, select, distinct and aggregate groups key a String
by the string itself and a PipelineField by its quoted path, and
PipelineField.path (Node's fieldName) is the quoted path.
field() and Expression.field() also take a FieldPath now, whose segments
stay whole: field(FieldPath(['a.b'])) is `a.b`. createFrom passes the
query's FieldPaths through instead of their formatted strings, which
would otherwise be split and quoted a second time.
Node quotes twice in two places, which Dart does not copy: the target of
unnest(field('my tags')) and the projection of createFrom(query.select())
with special characters. Both are golden cases listed as known
divergences.
…iply The Node SDK's add and multiply are variadic: add(first, second, ...others) and expr.add(second, ...others). PipelineFunctions.add and multiply now take an optional trailing Iterable of further operands, like concat and logicalMaximum, and the fluent methods take (second, [others]). Every operand is sent in a single add/multiply call, as Node's method form sends them. Two-operand calls are unchanged. Node's top-level add/multiply are typed variadic but drop ...others at runtime; the golden corpus now records that as a known divergence.
The Node SDK's documents() takes Array<string | DocumentReference>. documents() now takes an Iterable<Object> of DocumentReferences and slash-separated document paths, which are read like Firestore.doc reads them, as Node does with db.doc(path): a leading slash is ignored, and a path that does not point to a document, or that is empty or contains "//", throws an ArgumentError. Existing calls with references are unchanged.
Like the Node SDK's Ordering, a PipelineOrdering now exposes the expression it sorts by (expr; a field name given to ascending() or descending() reads back as a PipelineField) and its direction, 'ascending' or 'descending'.
The Node SDK's map() takes a Record<string, unknown>, which Dart callers
could not pass: PipelineFunctions.map only took an Iterable alternating
keys and values. It now also takes a Map, as in
map({'title': field('title')}), sent exactly like the alternating form;
the golden cases check both forms against Node's request. Keys are sent
as strings, and any other argument throws an ArgumentError. Nested Maps
in value positions are built through the same path.
…ings isInequalityFilter only covered <, <=, > and >=, so a query filtering with != or not-in was sorted by the document key alone: createFrom(query) built a Pipeline sorted by __name__ where the backend (and Node) sort by the filtered field first, and startAt/startAfter/endAt/endBefore with a DocumentSnapshot built a cursor on the wrong orderings. The two paths now share one implicit ordering, mirroring Node's createImplicitOrderBy: the explicit orderBy clauses, then every inequality field not already ordered, then __name__, all in the direction of the last explicit orderBy. This also fixes two differences the snapshot cursors had from Node: they only used the first inequality field, and only when there was no explicit orderBy. Inequality fields sort segment by segment as UTF-8, like Node's FieldPath.compareTo, instead of by their quoted form, which put `price-tier` before price.amount. New golden cases cover mixed, repeated and composite inequalities, quoted segments, explicit orderings, document key inequalities, limitToLast and cursors with inequalities.
createFrom(vectorQuery) sent the threshold as a distance_threshold option of find_nearest. The stage documents no such option (only limit and distance_field, in the Firestore docs, the Node SDK and the system tests), so the backend may ignore or reject it. Node drops the threshold, which returns documents beyond it. The threshold is now a where stage after find_nearest that compares the distance with it: at most the threshold for euclidean and cosine, at least for dot product, as VectorQuery documents. It reads the distance result field when the query has one, and otherwise computes the distance with euclidean_distance, cosine_distance or dot_product. The documents within the threshold are the nearest ones, so filtering the limit nearest neighbors returns the documents the VectorQuery returns. The distance result field is still sent as find_nearest's distance_field, which Node drops. Both are now known divergences, with golden cases for every distance measure and for a threshold with a result field.
A cursor is a position in the query's order, but createFrom(query) always
compared cursor values as if every ordering were ascending, as Node's
whereConditionsFromCursor does. On orderBy('rating', descending: true),
startAt([4]) kept ratings of 4 and more instead of 4 and less, and
endBefore kept the wrong side too.
Each bound now compares in its ordering's direction. The new golden cases
for descending, mixed-direction and limitToLast cursors are known
divergences from Node.
…ne results A Pipeline result reference that names no document (the database root returned by parent() of a top-level document, or a collection) decoded to its resource name only when it was a top-level field. Nested inside a map or array it went through Serializer.decodeValue, whose Firestore.doc call threw, failing the whole execute(). Thread a reference decoder through the serializer's recursion so Pipeline results apply the same rule at every depth: document references decode to a DocumentReference and any other reference to its resource name as a String. Regular document decoding is unchanged. The Node SDK throws for such references when the field is read; eager decoding cannot, so the existing top-level String behaviour is kept.
Close the coverage guard's gaps for the API merged since the live suite was written, and add live checks for the createFrom fixes: - sources: documents() with paths (and a leading slash) and references; createFrom against the regular Query for != and not-in (same documents, Pipeline sorted by the filtered field), for startAt, startAfter, endBefore and endAt with limitToLast on a descending ordering, and for a DocumentSnapshot cursor on a != query; createFrom of a VectorQuery with a distance threshold under each distance measure, and with a result field, against the VectorQuery itself. - stages: select with field names that are not identifiers, read through the key the backend returns; PipelineOrdering.expr and direction; findNearest and unnest rawOptions overriding their typed options. - functions: add and multiply with further operands, field and Expression.field with a FieldPath, and map from a Map. The seed gains `first-name` and `last name` fields. Stages and sources with no backend option to set (or only force_index, which needs an index the E2E database lacks) exempt their rawOptions in the guard.
The backend's add and multiply take exactly two operands. A live run rejected the single N-operand call PipelineFunctions.add/multiply and their fluent forms sent for further operands: The function 'add[T <: number](x: T, y: T): T' takes [2..2] argument(s), but 4 were provided. Further operands are now folded into left-nested two-operand calls, add(add(add(a, b), c), d). A two-operand call is unchanged. The Node SDK sends the rejected N-operand call from its method form, so the add/multiply variadic golden fixtures become known divergences, checked for the nested shape.
A live run rejected the distance_threshold option Pipeline.findNearest sent: Stage 'find_nearest(field: FieldReference, vector: Vector, distance_measure: DistanceMeasure, limit: Int64?, distance_field: FieldPath?)' does not support option(s) [distance_threshold]. findNearest(distanceThreshold:) now shares createFrom(vectorQuery)'s implementation: a where stage after find_nearest keeps distances at most the threshold for euclidean and cosine, and at least it for dot product, reading the distance result field when there is one and recomputing the distance otherwise. Node has no such option, so the parity entry becomes a known difference.
A live run rejected the index_mode option Pipeline.execute and Transaction.executePipeline sent for indexMode: Unsupported option: index_mode The Node SDK sends it too. Its only value, recommended, lets the backend choose its indexes, which it already does by default, so indexMode is now ignored. The indexMode parameters and PipelineIndexMode are deprecated rather than removed, keeping the change non-breaking. rawOptions can still send an index_mode explicitly. The golden fixtures where Node sends index_mode from indexMode become known divergences, checked for the absence of index_mode.
A live run rejected select(['last name']):
Invalid property path "last name". Unquoted property paths must match
regex ([a-zA-Z_][a-zA-Z_0-9]*), and quoted property paths must match
regex (`(?:[^`\\]|(?:\\.))+`)
The backend reads projection keys as field paths. A String selection
was keyed by the raw string, as Node's selectablesToObject does, while
a PipelineField is keyed by its quoted path, which the backend
accepted. The shared projection helper behind select, distinct,
aggregate groups and addFields now keys a String by the same quoted
path, so select(['x-y', field('x-y')]) is reported as a duplicate.
Aliases stay as written, as in Node; their dartdoc now says they are
read as field paths.
The String special-character stage fixtures become known divergences,
and the live case still accepts either key back, since the failed run
never returned results.
nested.level1 is {level2: {value: 42}}, so mapValues returns its single
value, the level2 map {value: 42}, as the live run did. The case
expected the value of nested itself. Its mapKeys, mapValues and
mapEntries siblings expect the right values.
…name field() and Expression.field() take a String or a FieldPath, but the other field-name positions only checked for a String, so a FieldPath fell through to constant(FieldPath) and failed to encode. Every field name is now read through _fieldOrExpression, which takes either: the target of every PipelineFunctions function, ascending() and descending(), the entries of select, distinct, aggregate groups and removeFields, unnest, replaceWith, and findNearest's vectorField and distanceResultField, which now takes a FieldPath like Query.findNearest's. The golden unary and binary function cases, and the stage cases naming fields, gain FieldPath forms, and the coverage guard requires a live FieldPath case for each stage position.
PipelineSource.documents() already validates each DocumentReference against the Pipeline's database (since #318), as its dartdoc promises, but the test only covered a single foreign reference. Cover a foreign reference after a path and a same-database reference, and a mixed list from the same database.
c007ec2 to
20e489c
Compare
Summary
This PR stacks on the review-fixes PR. That review found 14 wire-format and parity bugs that our tests never caught, for two reasons:
This PR adds four automated checks so that kind of bug fails CI. It also fixes the new bugs those checks found.
Checks against the Node SDK
test/pipeline_golden_test.dart+tool/pipeline_golden/@google-cloud/firestore9.3.1 builds about 900 pipelines covering every source, stage and function form and argument type, and records the requests it would send. Dart's requests must match exactly. Intentional differences are listed with a reason and re-checked. A new workflow fails if the recorded requests are out of date.test/pipeline_api_parity_test.dart+tool/pipeline_api_parity/.d.tsinto a snapshot, which is compared with the Dart API (resolved withpackage:analyzer): names, arity, which parameters are optional, single value vs list, exports.test/pipeline_single_implementation_test.darttest/pipeline_e2e_coverage_test.dartAll four are untagged unit tests, so they run in the normal
dart testwithout credentials.E2E overhaul
test/e2e/pipeline_e2e_test.dartis nowtest/e2e/pipeline/: a shared harness plus 12 files by area. Each file seeds its own documents once, and each case is its own test.Fixes found by the new checks
Single implementation:
ascending('x')/descending('x')sorted by the string constant'x'instead of the field.Node comparison:
createFrom()andDocumentSnapshotquery cursors left!=andnot-infields out of the implicit ordering.createFrom()kept the wrong side of a cursor bound on descending orderings. Node has the same bug.createFrom()of aVectorQuerysentdistanceThresholdas an undocumented option. It is now a filter on the distance.rawOptionskeys weren't expanded into nested maps, andrawStagesent nested maps that contained expressions as literal maps.Signature check:
field(FieldPath),documents()with path strings,addandmultiplywith any number of operands,map(Map),PipelineOrdering.expr/direction, andrawOptionson every stage.PipelineFunctions.arraySlicenow takes an optional length.arrayTransformno longer takes a trailing index alias; usearrayTransformWithIndexinstead.E2E, first live run (748 of 761 passed). These are deliberate differences from Node, which has the same bugs:
addandmultiplywith more than two operands: the backend accepts exactly two (takes [2..2] argument(s)). Extra operands are now sent as nested two-operand calls.findNearest(distanceThreshold:): the backend has no such option (does not support option(s) [distance_threshold]). The threshold is now applied as awherefilter on the distance, the same waycreateFrom(vectorQuery)does it.indexMode: the backend rejects it (Unsupported option: index_mode). It is no longer sent and is deprecated. Its only value,recommended, is already the backend's default.selectwith a String field name that isn't a plain identifier: the backend reads projection keys as field paths, so a String is now keyed by its quoted path, asfield()already was. Aliases are still sent as written.Gemini review:
FieldPath: function targets,ascending/descending,select,distinct,aggregategroups,addFields,removeFields,unnest,replaceWithandfindNearest.Breaking changes
PipelineFunctions.arrayTransform(array, alias, transform, [indexAlias])is nowarrayTransform(array, alias, transform)plusarrayTransformWithIndex(array, alias, indexAlias, transform). This matches Node and the fluent form.PipelineIndexModeand theindexModeparameters are deprecated.select(['x-y', field('x-y')])is reported as a duplicate.PipelineField.pathreturns the quoted path.field('')andfield('a..b')throw.rawOptionskeys with an empty segment throw.Testing
dart analyzeis clean.dart testruns 1,519 tests; the only failures are the 7 knownbundle_test.dartones, which try to reachmetadata.google.internal.dart test -P prod test/e2e/pipeline/loads and skips all 764 tests without credentials. Running them needs theE2E Pipelineworkflow.E2E Pipelineworkflow re-checks every push.