From 26196388d7833b146d92faf31182531a151c0f7e Mon Sep 17 00:00:00 2001 From: Chi Fujii Date: Wed, 17 May 2023 04:42:43 +0900 Subject: [PATCH] Cleanup projection (#2641) Closes #2430. --- .../aggregate_documents_compat_test.go | 52 +++++++++++++ integration/aggregate_documents_test.go | 76 +++++++++++++++++++ .../common/aggregations/projection.go | 31 ++------ .../aggregations/projection_iterator.go | 9 +-- .../common/aggregations/stages/project.go | 12 --- internal/handlers/common/projection.go | 9 +-- .../handlers/common/projection_iterator.go | 6 -- internal/handlers/commonerrors/error.go | 3 + .../handlers/commonerrors/errorcode_string.go | 12 +-- 9 files changed, 149 insertions(+), 61 deletions(-) create mode 100644 integration/aggregate_documents_test.go diff --git a/integration/aggregate_documents_compat_test.go b/integration/aggregate_documents_compat_test.go index 9db41382a787..127d4de8c207 100644 --- a/integration/aggregate_documents_compat_test.go +++ b/integration/aggregate_documents_compat_test.go @@ -1569,6 +1569,58 @@ func TestAggregateCompatProject(t *testing.T) { bson.D{{"$project", bson.D{{"_id", false}, {"foo", primitive.Regex{Pattern: "^fo"}}}}}, }, }, + "DotNotationInclude": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{ + {"_id", true}, + {"v.foo", true}, + }}}, + }, + }, + "DotNotationIncludeTwo": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{ + {"_id", true}, + {"v.foo", true}, + {"v.array", true}, + }}}, + }, + }, + "DotNotationExclude": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{ + {"_id", true}, + {"v.foo", false}, + }}}, + }, + }, + "DotNotationExcludeTwo": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{ + {"_id", true}, + {"v.foo", false}, + {"v.array", false}, + }}}, + }, + }, + "DotNotationExcludeSecondLevel": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{ + {"_id", true}, + {"v.array.42", false}, + }}}, + }, + }, + "DotNotationIncludeExclude": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{ + {"_id", true}, + {"v.foo", true}, + {"v.array.42", false}, + }}}, + }, + resultType: emptyResult, + }, } testAggregateStagesCompat(t, testCases) diff --git a/integration/aggregate_documents_test.go b/integration/aggregate_documents_test.go new file mode 100644 index 000000000000..d5a55f7ccbdc --- /dev/null +++ b/integration/aggregate_documents_test.go @@ -0,0 +1,76 @@ +// Copyright 2021 FerretDB Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package integration + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/mongo" + + "github.com/FerretDB/FerretDB/integration/setup" +) + +func TestAggregateProjectErrors(t *testing.T) { + t.Parallel() + + for name, tc := range map[string]struct { + expectedErr *mongo.CommandError + altMessage string + skip string + pipeline bson.A + }{ + "EmptyPipeline": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{}}}, + }, + expectedErr: &mongo.CommandError{ + Code: 51272, + Name: "Location51272", + Message: "Invalid $project :: caused by :: projection specification must have at least one field", + }, + }, + "EmptyProjectionField": { + pipeline: bson.A{ + bson.D{{"$project", bson.D{{"v", bson.D{}}}}}, + }, + expectedErr: &mongo.CommandError{ + Code: 51270, + Name: "Location51270", + Message: "Invalid $project :: caused by :: An empty sub-projection is not a valid value. Found empty object at path", + }, + skip: "https://github.com/FerretDB/FerretDB/issues/2633", + }, + } { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + if tc.skip != "" { + t.Skip(tc.skip) + } + + t.Parallel() + ctx, collection := setup.Setup(t) + + _, err := collection.Aggregate(ctx, tc.pipeline) + + if tc.expectedErr != nil { + AssertEqualAltCommandError(t, *tc.expectedErr, tc.altMessage, err) + return + } + require.NoError(t, err) + }) + } +} diff --git a/internal/handlers/common/aggregations/projection.go b/internal/handlers/common/aggregations/projection.go index fb6a9a515b49..1faf613cdfed 100644 --- a/internal/handlers/common/aggregations/projection.go +++ b/internal/handlers/common/aggregations/projection.go @@ -27,24 +27,24 @@ import ( "github.com/FerretDB/FerretDB/internal/util/must" ) -// errProjectionEmpty indicates projection field is empty. -var errProjectionEmpty = errors.New("projection is empty") - // ValidateProjection check projection document. // Document fields could be either included or excluded but not both. // Exception is for the _id field that could be included or excluded. // // Errors: +// - `ErrEmptyProject` when projection document is empty; // - `ErrProjectionExIn` when there is exclusion in inclusion projection; // - `ErrProjectionInEx` when there is inclusion in exclusion projection; // - `ErrNotImplemented` when there is unimplemented projection operators and expressions; -// - `errProjectionEmpty` when projection document is empty. func ValidateProjection(projection *types.Document) (*types.Document, bool, error) { validated := types.MakeDocument(0) if projection.Len() == 0 { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 - return nil, false, errProjectionEmpty + return nil, false, commonerrors.NewCommandErrorMsgWithArgument( + commonerrors.ErrEmptyProject, + "Invalid $project :: caused by :: projection specification must have at least one field", + "$project (stage)", + ) } var projectionVal *bool @@ -62,7 +62,7 @@ func ValidateProjection(projection *types.Document) (*types.Document, bool, erro return nil, false, lazyerrors.Error(err) } - if strings.Contains(key, "$") { + if strings.HasPrefix(key, "$") { return nil, false, commonerrors.NewCommandErrorMsg( commonerrors.ErrNotImplemented, fmt.Sprintf("projection operator $ is not supported in %s", key), @@ -295,12 +295,10 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( // set doc if projected has field from other projection field. v := must.NotFail(projected.Get(key)) if d, ok := v.(*types.Document); ok { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 doc = d } if arr, ok := v.(*types.Array); ok { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 // use next prefix key with arr value, allowing array to parse existing // projection fields. doc = must.NotFail(types.NewDocument(path.TrimPrefix().Prefix(), arr)) @@ -318,7 +316,6 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( case *types.Document: setBySourceOrder(key, doc, source, projected) case *types.Array: - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 projected.Set(key, arr) } @@ -333,7 +330,6 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( if v, err := projected.Get(key); err == nil { projectedArr, ok := v.(*types.Array) if ok { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 arr = projectedArr inclusionExists = true } @@ -345,7 +341,6 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( _, arrElem, err := iter.Next() if err != nil { if errors.Is(err, iterator.ErrIteratorDone) { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 break } @@ -353,7 +348,6 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( } if _, ok := arrElem.(*types.Document); !ok { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 continue } @@ -380,7 +374,6 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( doc = docVal } else { // first inclusion field, insert it to the doc. - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 arr.Append(doc) } @@ -388,14 +381,12 @@ func includeProjection(path types.Path, source any, projected *types.Document) ( return nil, err } - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 arr.Set(i, doc) i++ } return arr, nil default: - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 // field is not a document or an array, nothing to set. return nil, nil } @@ -431,7 +422,6 @@ func excludeProjection(path types.Path, projected any) { return } - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 // recursively remove field from the embeddedSource. excludeProjection(path.TrimPrefix(), embeddedSource) @@ -442,18 +432,15 @@ func excludeProjection(path types.Path, projected any) { arrElem := must.NotFail(projected.Get(i)) if _, ok := arrElem.(*types.Document); !ok { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 // not a document, cannot possibly be part of path, do nothing. continue } - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 excludeProjection(path, arrElem) } return default: - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 // not a path, nothing to exclude. return } @@ -480,12 +467,10 @@ func setBySourceOrder(key string, val any, source, projected *types.Document) { } if newFieldIndex >= len(projectedKeys) { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 break } if sourceKey == projectedKeys[newFieldIndex] { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 newFieldIndex++ } } @@ -494,7 +479,6 @@ func setBySourceOrder(key string, val any, source, projected *types.Document) { // remove fields of projected from newFieldIndex to the end for i := newFieldIndex; i < len(projectedKeys); i++ { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 projected.Remove(projectedKeys[i]) } @@ -503,7 +487,6 @@ func setBySourceOrder(key string, val any, source, projected *types.Document) { // copy newFieldIndex-th to the end from tmp to projected i := newFieldIndex for _, key := range tmp.Keys()[newFieldIndex:] { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 projected.Set(key, must.NotFail(tmp.Get(tmp.Keys()[i]))) i++ } diff --git a/internal/handlers/common/aggregations/projection_iterator.go b/internal/handlers/common/aggregations/projection_iterator.go index 06aa6f74a903..05d886daf7b6 100644 --- a/internal/handlers/common/aggregations/projection_iterator.go +++ b/internal/handlers/common/aggregations/projection_iterator.go @@ -15,8 +15,6 @@ package aggregations import ( - "errors" - "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/iterator" "github.com/FerretDB/FerretDB/internal/util/lazyerrors" @@ -30,13 +28,8 @@ import ( // Close method closes the underlying iterator. func ProjectionIterator(iter types.DocumentsIterator, closer *iterator.MultiCloser, projection *types.Document) (types.DocumentsIterator, error) { //nolint:lll // for readability projectionValidated, inclusion, err := ValidateProjection(projection) - if errors.Is(err, errProjectionEmpty) { - // TODO: https://github.com/FerretDB/FerretDB/issues/2633 - return iter, nil - } - if err != nil { - return nil, lazyerrors.Error(err) + return nil, err } res := &projectionIterator{ diff --git a/internal/handlers/common/aggregations/stages/project.go b/internal/handlers/common/aggregations/stages/project.go index 1f0da9e93b01..a525674085c7 100644 --- a/internal/handlers/common/aggregations/stages/project.go +++ b/internal/handlers/common/aggregations/stages/project.go @@ -16,8 +16,6 @@ package stages import ( "context" - "errors" - "fmt" "github.com/FerretDB/FerretDB/internal/handlers/common" "github.com/FerretDB/FerretDB/internal/handlers/common/aggregations" @@ -50,17 +48,7 @@ func newProject(stage *types.Document) (aggregations.Stage, error) { ) } - var cmdErr *commonerrors.CommandError - validated, inclusion, err := aggregations.ValidateProjection(fields) - if errors.As(err, &cmdErr) { - return nil, commonerrors.NewCommandErrorMsgWithArgument( - cmdErr.Code(), - fmt.Sprintf("Invalid $project :: caused by :: %s", cmdErr.Unwrap()), - "$project (stage)", - ) - } - if err != nil { return nil, err } diff --git a/internal/handlers/common/projection.go b/internal/handlers/common/projection.go index 59da0635d189..fe6853bee2dc 100644 --- a/internal/handlers/common/projection.go +++ b/internal/handlers/common/projection.go @@ -27,9 +27,6 @@ import ( "github.com/FerretDB/FerretDB/internal/util/must" ) -// errProjectionEmpty indicates projection field is empty. -var errProjectionEmpty = errors.New("projection is empty") - // ValidateProjection check projection document. // Document fields could be either included or excluded but not both. // Exception is for the _id field that could be included or excluded. @@ -40,12 +37,12 @@ var errProjectionEmpty = errors.New("projection is empty") // - `ErrProjectionExIn` when there is exclusion in inclusion projection; // - `ErrProjectionInEx` when there is inclusion in exclusion projection; // - `ErrNotImplemented` when there is unimplemented projection operators and expressions; -// - `errProjectionEmpty` when projection document is empty. func ValidateProjection(projection *types.Document) (*types.Document, bool, error) { validated := types.MakeDocument(0) if projection.Len() == 0 { - return nil, false, errProjectionEmpty + // empty projection is exclusion project. + return types.MakeDocument(0), false, nil } var projectionVal *bool @@ -63,7 +60,7 @@ func ValidateProjection(projection *types.Document) (*types.Document, bool, erro return nil, false, lazyerrors.Error(err) } - if strings.Contains(key, "$") { + if strings.HasPrefix(key, "$") { return nil, false, commonerrors.NewCommandErrorMsg( commonerrors.ErrNotImplemented, fmt.Sprintf("projection operator $ is not supported in %s", key), diff --git a/internal/handlers/common/projection_iterator.go b/internal/handlers/common/projection_iterator.go index 933669b39d14..8008651b3b1f 100644 --- a/internal/handlers/common/projection_iterator.go +++ b/internal/handlers/common/projection_iterator.go @@ -15,8 +15,6 @@ package common import ( - "errors" - "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/iterator" "github.com/FerretDB/FerretDB/internal/util/lazyerrors" @@ -30,10 +28,6 @@ import ( // Close method closes the underlying iterator. func ProjectionIterator(iter types.DocumentsIterator, closer *iterator.MultiCloser, projection *types.Document) (types.DocumentsIterator, error) { //nolint:lll // for readability projectionValidated, inclusion, err := ValidateProjection(projection) - if errors.Is(err, errProjectionEmpty) { - return iter, nil - } - if err != nil { return nil, lazyerrors.Error(err) } diff --git a/internal/handlers/commonerrors/error.go b/internal/handlers/commonerrors/error.go index 2c7656b2b6ca..97b720f39f92 100644 --- a/internal/handlers/commonerrors/error.go +++ b/internal/handlers/commonerrors/error.go @@ -227,6 +227,9 @@ const ( // ErrBadRegexOption indicates bad regex option value passed. ErrBadRegexOption = ErrorCode(51108) // Location51108 + // ErrEmptyProject indicates that projection specification must have at least one field. + ErrEmptyProject = ErrorCode(51272) // Location51272 + // ErrDuplicateField indicates duplicate field is specified. ErrDuplicateField = ErrorCode(4822819) // Location4822819 diff --git a/internal/handlers/commonerrors/errorcode_string.go b/internal/handlers/commonerrors/errorcode_string.go index c7d3b4d8b485..2fc1bbead802 100644 --- a/internal/handlers/commonerrors/errorcode_string.go +++ b/internal/handlers/commonerrors/errorcode_string.go @@ -73,13 +73,14 @@ func _() { _ = x[ErrRegexOptions-51075] _ = x[ErrRegexMissingParen-51091] _ = x[ErrBadRegexOption-51108] + _ = x[ErrEmptyProject-51272] _ = x[ErrDuplicateField-4822819] _ = x[ErrStageSkipBadValue-5107200] _ = x[ErrStageLimitInvalidArg-5107201] _ = x[ErrStageCollStatsInvalidArg-5447000] } -const _ErrorCode_name = "UnsetInternalErrorBadValueFailedToParseTypeMismatchIllegalOperationNamespaceNotFoundIndexNotFoundUnsuitableValueTypeConflictingUpdateOperatorsCursorNotFoundNamespaceExistsDollarPrefixedFieldNameInvalidIDEmptyNameCommandNotFoundImmutableFieldCannotCreateIndexInvalidOptionsInvalidNamespaceIndexOptionsConflictIndexKeySpecsConflictOperationFailedDocumentValidationFailureNotImplementedMechanismUnavailableLocation11000Location15947Location15948Location15955Location15958Location15959Location15969Location15973Location15974Location15975Location15976Location15981Location15998Location16410Location16872Location17276Location28667Location28724Location28812Location28818Location31253Location31254Location40156Location40157Location40158Location40160Location40234Location40237Location40238Location40323Location40352Location40414Location40415Location50840Location51024Location51075Location51091Location51108Location4822819Location5107200Location5107201Location5447000" +const _ErrorCode_name = "UnsetInternalErrorBadValueFailedToParseTypeMismatchIllegalOperationNamespaceNotFoundIndexNotFoundUnsuitableValueTypeConflictingUpdateOperatorsCursorNotFoundNamespaceExistsDollarPrefixedFieldNameInvalidIDEmptyNameCommandNotFoundImmutableFieldCannotCreateIndexInvalidOptionsInvalidNamespaceIndexOptionsConflictIndexKeySpecsConflictOperationFailedDocumentValidationFailureNotImplementedMechanismUnavailableLocation11000Location15947Location15948Location15955Location15958Location15959Location15969Location15973Location15974Location15975Location15976Location15981Location15998Location16410Location16872Location17276Location28667Location28724Location28812Location28818Location31253Location31254Location40156Location40157Location40158Location40160Location40234Location40237Location40238Location40323Location40352Location40414Location40415Location50840Location51024Location51075Location51091Location51108Location51272Location4822819Location5107200Location5107201Location5447000" var _ErrorCode_map = map[ErrorCode]string{ 0: _ErrorCode_name[0:5], @@ -146,10 +147,11 @@ var _ErrorCode_map = map[ErrorCode]string{ 51075: _ErrorCode_name[858:871], 51091: _ErrorCode_name[871:884], 51108: _ErrorCode_name[884:897], - 4822819: _ErrorCode_name[897:912], - 5107200: _ErrorCode_name[912:927], - 5107201: _ErrorCode_name[927:942], - 5447000: _ErrorCode_name[942:957], + 51272: _ErrorCode_name[897:910], + 4822819: _ErrorCode_name[910:925], + 5107200: _ErrorCode_name[925:940], + 5107201: _ErrorCode_name[940:955], + 5447000: _ErrorCode_name[955:970], } func (i ErrorCode) String() string {