fix(firestore): Change stages to match Java and Node#14253
fix(firestore): Change stages to match Java and Node#14253bhshkh merged 3 commits intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors several Firestore pipeline methods, including Select, AddFields, RemoveFields, Unnest, Sample, and RawStage, to improve their API design. Specifically, methods that accept variadic arguments now require at least one argument explicitly. The Unnest and Sample stages were updated to use an options pattern with interfaces and helper functions, replacing direct struct usage. The RawStage builder pattern was simplified to direct arguments. The errInvalidArg helper function was enhanced to include the stage name for clearer error messages. A critical issue was identified where the Select call in firestore/query.go could panic if the fields slice is empty, requiring a guard. Additionally, there is an opportunity to refactor duplicated code for processing UnnestOptions in the Unnest and UnnestWithAlias methods.
daniel-sanche
left a comment
There was a problem hiding this comment.
This gemini comment might be worth looking at. Otherwise LGTM
| } | ||
|
|
||
| // RemoveFields removes fields from outputs from previous stages. | ||
| // fieldpaths can be a string or a [FieldPath] or an expression obtained by calling [FieldOf]. |
There was a problem hiding this comment.
make the argument name match the comment
| // fieldpaths can be a string or a [FieldPath] or an expression obtained by calling [FieldOf]. | |
| // field can be a string or a [FieldPath] or an expression obtained by calling [FieldOf]. |
Java stages for reference: https://github.com/googleapis/java-firestore/blob/6e30a6c11efe5d428607bfd78f82ba7b49497bd9/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Pipeline.java
Node stages for reference: https://github.com/googleapis/google-cloud-node/blob/c26967ca277fdab196ea94f5561e4f451ac1ec8a/handwritten/firestore/dev/src/pipelines/pipelines.ts
.removeFields(field("published"), field("genre"), field("nestedField"))but in Go user cannot doRemoveFields(FieldOf("published"), FieldOf("genre"), FieldOf("nestedField")). This PR fixes this by allowing FieldOf in RemoveFields argsThe changes to the sample stage from
SampleSpectoSampler, andSampleByDocumentstoByDocumentswere driven by idiomatic Go design principles regarding readability and removing structural redundancy:Reducing Stuttering in Code
In the previous API, calling a sample operation resulted in a redundant "stutter" in the code:
By removing the redundant "Sample" prefix in the builder function name, the code now reads much more cleanly like a sentence
Conciseness and Clarity
Renaming
SampleSpectoSamplerrepresents idiomatic Go naming conventions. Interfaces or types that perform a single conceptual action or "sample" elements are standardly suffixed with-er(likeReader,Writer,Sampler) rather than-Spec.Exposing
ByPercentageconsistentlyIn addition to rewriting
SampleByDocumentstoByDocuments, the new API now cleanly supportsByPercentagedirectly too:This is much more expressive than forced instantiation for percentage configurations.
The
RawStagerefactoring represents a significant shift from a builder pattern (which is Java-idiomatic) to a direct method invocation with variadic arguments (which is Go-idiomatic). Here is a detailed breakdown of the rationale:Removing Public API Bloat
In the previous implementation,
RawStagewas a publicly exported struct (type RawStage struct). This forced the SDK to export a constructorNewRawStage(), and builder methods likeWithArguments()andWithOptions().The new API makes
rawStageinternal (lowercaser). The user now simply calls a direct method on the pipeline (pipeline.RawStage(...)). There are no superfluous constructors or builder methods exposed on the documentation surface.Idiomatic Go (Direct Method Calls)
Go prefers passing configuring arguments directly into a method rather than instantiating builder objects. Compare the old chained instantiation against the new design:
Old Way (Java Builder style):
New Way (Idiomatic Go):
The new syntax is much shorter, easier to read, and accurately describes the intent without boilerplate.
Streamlining Options with Variadics
The new method
RawStage(name string, args []any, opts ...RawStageOptions)safely takes variadic pointers for options. If no options are needed, the caller simply omits them. If they are needed, they can supply any amount ofRawStageOptionswithout calling unique chain builders.Architectural Alignment across the SDK
This aligns
RawStagewith the rest of the module's recent refactors (shiftingSampleSpectoSampler, and*UnnestOptionsto...UnnestOption). It unifies the builder's syntax so developers maintain a standard mental model when chained expressions are invoked.