Skip to content

Commit 1ceb42e

Browse files
bhshkhbhshkh
authored
feat(firestore): refine Pipelines API, error handling, and tests (#14429)
This PR introduces several refinements to the Firestore Pipelines experimental feature, aimed at improving API consistency, error reporting, and the reliability of integration tests. Key Changes: - API Refinement: - Renamed ArraySliceWithLength to ArraySliceToEnd to better reflect its behavior similar to [Java](https://github.com/googleapis/java-firestore/blob/9065134fd70da22250f038d7e964f26aebfeb3cf/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/Expression.java#L2644). - Enabled the ArrayFilter expression (previously commented out). - Updated TimestampTruncateWithTimezone to accept any for the timezone parameter, allowing for dynamic timezone expressions. TimestampTruncateWithTimezone in Java accepts string or expression [src](https://github.com/googleapis/java-firestore/blob/9065134fd70da22250f038d7e964f26aebfeb3cf/google-cloud-firestore/src/main/java/com/google/cloud/firestore/pipeline/expressions/Expression.java#L3763-L3908). - Error Handling: - Introduced typed errors ErrPipelineWithoutDatabase and ErrUnionNotSupportRelativeScope to replace generic fmt.Errorf calls #14365 (comment). - Added internal nil checks across various pipeline stages (inputStageDocuments, findNearestStage, removeFieldsStage, etc.) to provide more descriptive error messages during proto conversion. - Integration Test Enhancements: - Replaced skipIfEnterprise with a more flexible skipIfEdition helper to handle feature support across different Firestore editions. - Improved resource cleanup in tests by moving t.Cleanup calls earlier, ensuring documents are deleted even if a test fails prematurely. - Updated deleteCollection to use Select().Documents(ctx) for more efficient document deletion during test setup/teardown. - Bug Fixes & Robustness: - Added nil checks when processing results in TestIntegration_Query_Pipeline. - Standardized test skipping logic across all pipeline-related integration tests.
1 parent 45d75e5 commit 1ceb42e

9 files changed

Lines changed: 346 additions & 293 deletions

‎firestore/integration_test.go‎

Lines changed: 106 additions & 119 deletions
Large diffs are not rendered by default.

‎firestore/pipeline.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,20 @@ package firestore
1616

1717
import (
1818
"context"
19+
"errors"
1920
"fmt"
2021
"reflect"
2122

2223
pb "cloud.google.com/go/firestore/apiv1/firestorepb"
2324
)
2425

26+
var (
27+
// ErrPipelineWithoutDatabase is returned when a pipeline is executed without a database such as a subcollection pipeline.
28+
ErrPipelineWithoutDatabase = errors.New("firestore: pipeline without a database cannot be executed directly, only as part of another pipeline")
29+
// ErrRelativeScopeUnionUnsupported is returned when a union is used with a relative scope pipeline.
30+
ErrRelativeScopeUnionUnsupported = errors.New("firestore: union only supports combining root pipelines; relative scope pipelines (like subcollection pipelines) are not supported")
31+
)
32+
2533
// Pipeline class provides a flexible and expressive framework for building complex data
2634
// transformation and query pipelines for Firestore.
2735
//
@@ -256,7 +264,7 @@ func (p *Pipeline) Execute(ctx context.Context, opts ...ExecuteOption) *Pipeline
256264
}
257265

258266
if newP.c == nil {
259-
newP.err = fmt.Errorf("pipeline created without a database (e.g., as a subcollection pipeline) cannot be executed directly; it can only be used as part of another pipeline")
267+
newP.err = ErrPipelineWithoutDatabase
260268
return &PipelineSnapshot{
261269
iter: &PipelineResultIterator{
262270
err: newP.err,
@@ -900,7 +908,7 @@ func (p *Pipeline) Union(other *Pipeline, opts ...UnionOption) *Pipeline {
900908
return p
901909
}
902910
if other.c == nil {
903-
p.err = fmt.Errorf("union only supports combining root pipelines; relative scope pipelines (like subcollection pipelines) are not supported")
911+
p.err = ErrRelativeScopeUnionUnsupported
904912
return p
905913
}
906914
options := make(map[string]any)

‎firestore/pipeline_expression.go‎

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -170,15 +170,15 @@ type Expression interface {
170170
//
171171
// The parameter 'n' can be an integer constant or an [Expression] that evaluates to an integer.
172172
ArrayLastN(n any) Expression
173-
// ArraySlice creates an expression that returns a slice of an array starting from the specified offset.
173+
// ArraySliceToEnd creates an expression that returns a slice of an array starting from the specified offset.
174174
//
175175
// The parameter 'offset' is the 0-based index of the first element to include. It can be an int, int32, int64 or [Expression].
176-
ArraySlice(offset any) Expression
177-
// ArraySliceWithLength creates an expression that returns a slice of an array starting from the specified offset with a given length.
176+
ArraySliceToEnd(offset any) Expression
177+
// ArraySlice creates an expression that returns a slice of an array starting from the specified offset with a given length.
178178
//
179179
// The parameter 'offset' is the 0-based index of the first element to include. It can be an int, int32, int64 or [Expression].
180180
// The parameter 'length' is the number of elements to include. It can be an int, int32, int64 or [Expression].
181-
ArraySliceWithLength(offset, length any) Expression
181+
ArraySlice(offset, length any) Expression
182182
// ArrayIndexOf creates an expression that returns the first index of a search value in an array.
183183
//
184184
// The parameter 'search' is the value to search for. It can be a constant or [Expression].
@@ -205,12 +205,11 @@ type Expression interface {
205205
// If the expression resolves to an absent value, it is converted to NULL.
206206
// The order of elements in the output array is not stable and shouldn't be relied upon.
207207
ArrayAggDistinct() AggregateFunction
208-
// TODO: Uncomment this after fixing the proto representation of this function.
209208
// ArrayFilter creates an expression for array_filter(array, param, body).
210209
//
211210
// The parameter 'param' is the name of the parameter to use in the body expression.
212211
// The parameter 'body' is the expression to evaluate for each element of the array.
213-
// ArrayFilter(param string, body BooleanExpression) Expression
212+
ArrayFilter(param string, body BooleanExpression) Expression
214213
// LogicalMaximum returns the maximum value of the expression and the specified values.
215214
LogicalMaximum(others ...any) Expression
216215
// LogicalMinimum returns the minimum value of the expression and the specified values.
@@ -242,7 +241,7 @@ type Expression interface {
242241
// "week(wednesday)", "week(thursday)", "week(friday)", "week(saturday)", "week(sunday)", "isoweek", "month", "quarter", "year", and "isoyear".
243242
// The parameter 'timezone' can be a string constant (e.g., "America/Los_Angeles") or an [Expression] that evaluates to a valid timezone string.
244243
// Valid values are from the TZ database or in the format "Etc/GMT-1".
245-
TimestampTruncateWithTimezone(granularity any, timezone string) Expression
244+
TimestampTruncateWithTimezone(granularity any, timezone any) Expression
246245
// TimestampToUnixMicros creates an expression that converts a timestamp expression to the number of microseconds since
247246
// the Unix epoch (1970-01-01 00:00:00 UTC).
248247
TimestampToUnixMicros() Expression
@@ -578,18 +577,18 @@ func (b *baseExpression) ArrayReverse() Expression { return Arra
578577
func (b *baseExpression) ArrayConcat(otherArrays ...any) Expression {
579578
return ArrayConcat(b, otherArrays...)
580579
}
581-
func (b *baseExpression) ArraySum() Expression { return ArraySum(b) }
582-
func (b *baseExpression) ArrayMaximum() Expression { return ArrayMaximum(b) }
583-
func (b *baseExpression) ArrayMaximumN(n any) Expression { return ArrayMaximumN(b, n) }
584-
func (b *baseExpression) ArrayMinimum() Expression { return ArrayMinimum(b) }
585-
func (b *baseExpression) ArrayMinimumN(n any) Expression { return ArrayMinimumN(b, n) }
586-
func (b *baseExpression) ArrayFirst() Expression { return ArrayFirst(b) }
587-
func (b *baseExpression) ArrayFirstN(n any) Expression { return ArrayFirstN(b, n) }
588-
func (b *baseExpression) ArrayLast() Expression { return ArrayLast(b) }
589-
func (b *baseExpression) ArrayLastN(n any) Expression { return ArrayLastN(b, n) }
590-
func (b *baseExpression) ArraySlice(offset any) Expression { return ArraySlice(b, offset) }
591-
func (b *baseExpression) ArraySliceWithLength(offset, length any) Expression {
592-
return ArraySliceLength(b, offset, length)
580+
func (b *baseExpression) ArraySum() Expression { return ArraySum(b) }
581+
func (b *baseExpression) ArrayMaximum() Expression { return ArrayMaximum(b) }
582+
func (b *baseExpression) ArrayMaximumN(n any) Expression { return ArrayMaximumN(b, n) }
583+
func (b *baseExpression) ArrayMinimum() Expression { return ArrayMinimum(b) }
584+
func (b *baseExpression) ArrayMinimumN(n any) Expression { return ArrayMinimumN(b, n) }
585+
func (b *baseExpression) ArrayFirst() Expression { return ArrayFirst(b) }
586+
func (b *baseExpression) ArrayFirstN(n any) Expression { return ArrayFirstN(b, n) }
587+
func (b *baseExpression) ArrayLast() Expression { return ArrayLast(b) }
588+
func (b *baseExpression) ArrayLastN(n any) Expression { return ArrayLastN(b, n) }
589+
func (b *baseExpression) ArraySliceToEnd(offset any) Expression { return ArraySliceToEnd(b, offset) }
590+
func (b *baseExpression) ArraySlice(offset, length any) Expression {
591+
return ArraySlice(b, offset, length)
593592
}
594593
func (b *baseExpression) ArrayIndexOf(search any) Expression {
595594
return ArrayIndexOf(b, search)
@@ -605,11 +604,9 @@ func (b *baseExpression) Last() AggregateFunction { return Last(b) }
605604
func (b *baseExpression) ArrayAgg() AggregateFunction { return ArrayAgg(b) }
606605
func (b *baseExpression) ArrayAggDistinct() AggregateFunction { return ArrayAggDistinct(b) }
607606

608-
// TODO: Uncomment this after fixing the proto representation of this function.
609-
//
610-
// func (b *baseExpression) ArrayFilter(param string, body BooleanExpression) Expression {
611-
// return ArrayFilter(b, param, body)
612-
// }
607+
func (b *baseExpression) ArrayFilter(param string, body BooleanExpression) Expression {
608+
return ArrayFilter(b, param, body)
609+
}
613610
func (b *baseExpression) LogicalMaximum(others ...any) Expression {
614611
return LogicalMaximum(b, others...)
615612
}
@@ -627,7 +624,7 @@ func (b *baseExpression) TimestampSubtract(unit, amount any) Expression {
627624
func (b *baseExpression) TimestampTruncate(granularity any) Expression {
628625
return TimestampTruncate(b, granularity)
629626
}
630-
func (b *baseExpression) TimestampTruncateWithTimezone(granularity any, timezone string) Expression {
627+
func (b *baseExpression) TimestampTruncateWithTimezone(granularity any, timezone any) Expression {
631628
return TimestampTruncateWithTimezone(b, granularity, timezone)
632629
}
633630
func (b *baseExpression) TimestampToUnixMicros() Expression { return TimestampToUnixMicros(b) }

‎firestore/pipeline_function.go‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ func TimestampTruncate(timestamp, granularity any) Expression {
322322
//
323323
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
324324
// regardless of any other documented package stability guarantees.
325-
func TimestampTruncateWithTimezone(timestamp, granularity any, timezone string) Expression {
325+
func TimestampTruncateWithTimezone(timestamp, granularity any, timezone any) Expression {
326326
return newBaseFunction("timestamp_trunc", []Expression{asFieldExpr(timestamp), asStringExpr(granularity), asStringExpr(timezone)})
327327
}
328328

@@ -569,24 +569,24 @@ func ArrayLastN(exprOrFieldPath any, n any) Expression {
569569
return newBaseFunction("array_last_n", []Expression{asFieldExpr(exprOrFieldPath), asInt64Expr(n)})
570570
}
571571

572-
// ArraySlice creates an expression that returns a slice of an array starting from the specified offset.
572+
// ArraySliceToEnd creates an expression that returns a slice of an array starting from the specified offset.
573573
// - exprOrFieldPath can be a field path string, [FieldPath] or an [Expression] that evaluates to an array.
574574
// - offset is the 0-based index of the first element to include. It can be an int, int32, int64 or [Expression].
575575
//
576576
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
577577
// regardless of any other documented package stability guarantees.
578-
func ArraySlice(exprOrFieldPath any, offset any) Expression {
578+
func ArraySliceToEnd(exprOrFieldPath any, offset any) Expression {
579579
return newBaseFunction("array_slice", []Expression{asFieldExpr(exprOrFieldPath), asInt64Expr(offset)})
580580
}
581581

582-
// ArraySliceLength creates an expression that returns a slice of an array starting from the specified offset with a given length.
582+
// ArraySlice creates an expression that returns a slice of an array starting from the specified offset with a given length.
583583
// - exprOrFieldPath can be a field path string, [FieldPath] or an [Expression] that evaluates to an array.
584584
// - offset is the 0-based index of the first element to include. It can be an int, int32, int64 or [Expression].
585585
// - length is the number of elements to include. It can be an int, int32, int64 or [Expression].
586586
//
587587
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
588588
// regardless of any other documented package stability guarantees.
589-
func ArraySliceLength(exprOrFieldPath any, offset any, length any) Expression {
589+
func ArraySlice(exprOrFieldPath any, offset any, length any) Expression {
590590
return newBaseFunction("array_slice", []Expression{asFieldExpr(exprOrFieldPath), asInt64Expr(offset), asInt64Expr(length)})
591591
}
592592

@@ -597,10 +597,9 @@ func ArraySliceLength(exprOrFieldPath any, offset any, length any) Expression {
597597
//
598598
// Experimental: Firestore Pipelines is currently in preview and is subject to potential breaking changes in future versions,
599599
// regardless of any other documented package stability guarantees.
600-
// TODO: Uncomment this after fixing the proto representation of this function.
601-
// func ArrayFilter(array any, param string, body BooleanExpression) Expression {
602-
// return newBaseFunction("array_filter", []Expression{asFieldExpr(array), ConstantOf(param), body})
603-
// }
600+
func ArrayFilter(array any, param string, body BooleanExpression) Expression {
601+
return newBaseFunction("array_filter", []Expression{asFieldExpr(array), ConstantOf(param), body})
602+
}
604603

605604
// ArrayIndexOf creates an expression that returns the first index of a search value in an array.
606605
// - exprOrFieldPath can be a field path string, [FieldPath] or an [Expression] that evaluates to an array.

‎firestore/pipeline_function_test.go‎

Lines changed: 47 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -969,8 +969,8 @@ func TestArrayFunctions(t *testing.T) {
969969
}},
970970
},
971971
{
972-
desc: "ArraySlice",
973-
expr: ArraySlice("field", 1),
972+
desc: "ArraySliceToEnd",
973+
expr: ArraySliceToEnd("field", 1),
974974
want: &pb.Value{ValueType: &pb.Value_FunctionValue{
975975
FunctionValue: &pb.Function{
976976
Name: "array_slice",
@@ -983,7 +983,7 @@ func TestArrayFunctions(t *testing.T) {
983983
},
984984
{
985985
desc: "ArraySliceWithLength",
986-
expr: ArraySliceLength("field", 1, 2),
986+
expr: ArraySlice("field", 1, 2),
987987
want: &pb.Value{ValueType: &pb.Value_FunctionValue{
988988
FunctionValue: &pb.Function{
989989
Name: "array_slice",
@@ -995,51 +995,50 @@ func TestArrayFunctions(t *testing.T) {
995995
},
996996
}},
997997
},
998-
// TODO: Uncomment this after fixing the proto representation of this function.
999-
// {
1000-
// desc: "ArrayFilter",
1001-
// expr: ArrayFilter("field", "item", FieldOf("item").GreaterThan(5)),
1002-
// want: &pb.Value{ValueType: &pb.Value_FunctionValue{
1003-
// FunctionValue: &pb.Function{
1004-
// Name: "array_filter",
1005-
// Args: []*pb.Value{
1006-
// {ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "field"}},
1007-
// {ValueType: &pb.Value_StringValue{StringValue: "item"}},
1008-
// {ValueType: &pb.Value_FunctionValue{
1009-
// FunctionValue: &pb.Function{
1010-
// Name: "greater_than",
1011-
// Args: []*pb.Value{
1012-
// {ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "item"}},
1013-
// {ValueType: &pb.Value_IntegerValue{IntegerValue: 5}},
1014-
// },
1015-
// },
1016-
// }},
1017-
// },
1018-
// },
1019-
// }},
1020-
// },
1021-
// {
1022-
// desc: "baseExpression ArrayFilter",
1023-
// expr: FieldOf("field").ArrayFilter("item", FieldOf("item").GreaterThan(5)),
1024-
// want: &pb.Value{ValueType: &pb.Value_FunctionValue{
1025-
// FunctionValue: &pb.Function{
1026-
// Name: "array_filter",
1027-
// Args: []*pb.Value{
1028-
// {ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "field"}},
1029-
// {ValueType: &pb.Value_StringValue{StringValue: "item"}},
1030-
// {ValueType: &pb.Value_FunctionValue{
1031-
// FunctionValue: &pb.Function{
1032-
// Name: "greater_than",
1033-
// Args: []*pb.Value{
1034-
// {ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "item"}},
1035-
// {ValueType: &pb.Value_IntegerValue{IntegerValue: 5}},
1036-
// },
1037-
// },
1038-
// }},
1039-
// },
1040-
// },
1041-
// }},
1042-
// },
998+
{
999+
desc: "ArrayFilter",
1000+
expr: ArrayFilter("field", "item", FieldOf("item").GreaterThan(5)),
1001+
want: &pb.Value{ValueType: &pb.Value_FunctionValue{
1002+
FunctionValue: &pb.Function{
1003+
Name: "array_filter",
1004+
Args: []*pb.Value{
1005+
{ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "field"}},
1006+
{ValueType: &pb.Value_StringValue{StringValue: "item"}},
1007+
{ValueType: &pb.Value_FunctionValue{
1008+
FunctionValue: &pb.Function{
1009+
Name: "greater_than",
1010+
Args: []*pb.Value{
1011+
{ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "item"}},
1012+
{ValueType: &pb.Value_IntegerValue{IntegerValue: 5}},
1013+
},
1014+
},
1015+
}},
1016+
},
1017+
},
1018+
}},
1019+
},
1020+
{
1021+
desc: "baseExpression ArrayFilter",
1022+
expr: FieldOf("field").ArrayFilter("item", FieldOf("item").GreaterThan(5)),
1023+
want: &pb.Value{ValueType: &pb.Value_FunctionValue{
1024+
FunctionValue: &pb.Function{
1025+
Name: "array_filter",
1026+
Args: []*pb.Value{
1027+
{ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "field"}},
1028+
{ValueType: &pb.Value_StringValue{StringValue: "item"}},
1029+
{ValueType: &pb.Value_FunctionValue{
1030+
FunctionValue: &pb.Function{
1031+
Name: "greater_than",
1032+
Args: []*pb.Value{
1033+
{ValueType: &pb.Value_FieldReferenceValue{FieldReferenceValue: "item"}},
1034+
{ValueType: &pb.Value_IntegerValue{IntegerValue: 5}},
1035+
},
1036+
},
1037+
}},
1038+
},
1039+
},
1040+
}},
1041+
},
10431042
{
10441043
desc: "ArrayIndexOf",
10451044
expr: ArrayIndexOf("field", "search"),

0 commit comments

Comments
 (0)