From e42205889e6edaa9618e7083b2c14f601d59f8b9 Mon Sep 17 00:00:00 2001 From: Keenan Nemetz Date: Fri, 26 Jul 2024 13:31:04 -0700 Subject: [PATCH 1/2] refactor(i): Collection tests (#2861) ## Relevant issue(s) Resolves #2860 ## Description This PR refactors the collection tests to use the test integration test framework. It also adds a new `UpdateWithFilter` test action. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? (*replace*) Describe the tests performed to verify the changes. Provide instructions to reproduce them. Specify the platform(s) on which this was tested: - MacOS --- cli/collection_update.go | 10 +- .../collection/update/simple/utils.go | 34 ++- .../update/simple/with_filter_test.go | 257 ++++++++---------- tests/integration/collection/utils.go | 116 -------- tests/integration/events.go | 43 +++ tests/integration/test_case.go | 37 +++ tests/integration/utils2.go | 31 +++ 7 files changed, 242 insertions(+), 286 deletions(-) delete mode 100644 tests/integration/collection/utils.go diff --git a/cli/collection_update.go b/cli/collection_update.go index fb7e352249..228bff0053 100644 --- a/cli/collection_update.go +++ b/cli/collection_update.go @@ -11,6 +11,8 @@ package cli import ( + "encoding/json" + "github.com/spf13/cobra" "github.com/sourcenetwork/defradb/client" @@ -48,8 +50,12 @@ Example: update private docID, with identity: } switch { - case filter != "" && updater != "": - res, err := col.UpdateWithFilter(cmd.Context(), filter, updater) + case filter != "" || updater != "": + var filterValue any + if err := json.Unmarshal([]byte(filter), &filterValue); err != nil { + return err + } + res, err := col.UpdateWithFilter(cmd.Context(), filterValue, updater) if err != nil { return err } diff --git a/tests/integration/collection/update/simple/utils.go b/tests/integration/collection/update/simple/utils.go index 91cbe700f7..224c3b9ce5 100644 --- a/tests/integration/collection/update/simple/utils.go +++ b/tests/integration/collection/update/simple/utils.go @@ -13,12 +13,10 @@ package update import ( "testing" - "github.com/sourcenetwork/defradb/client" testUtils "github.com/sourcenetwork/defradb/tests/integration" - testUtilsCol "github.com/sourcenetwork/defradb/tests/integration/collection" ) -var userCollectionGQLSchema = ` +var schema = ` type Users { name: String age: Int @@ -27,19 +25,19 @@ var userCollectionGQLSchema = ` } ` -var colDefMap = make(map[string]client.CollectionDefinition) - -func init() { - c, err := testUtils.ParseSDL(userCollectionGQLSchema) - if err != nil { - panic(err) - } - u := c["Users"] - u.Schema.Root = "bafkreiclkqkxhq3xu3sz5fqcixykk2qfpva5asj3elcaqyxscax66ok4za" - c["Users"] = u - colDefMap = c -} - -func executeTestCase(t *testing.T, test testUtilsCol.TestCase) { - testUtilsCol.ExecuteRequestTestCase(t, userCollectionGQLSchema, test) +func executeTestCase(t *testing.T, test testUtils.TestCase) { + testUtils.ExecuteTestCase( + t, + testUtils.TestCase{ + Description: test.Description, + Actions: append( + []any{ + testUtils.SchemaUpdate{ + Schema: schema, + }, + }, + test.Actions..., + ), + }, + ) } diff --git a/tests/integration/collection/update/simple/with_filter_test.go b/tests/integration/collection/update/simple/with_filter_test.go index ebe45e0b4f..a99f340753 100644 --- a/tests/integration/collection/update/simple/with_filter_test.go +++ b/tests/integration/collection/update/simple/with_filter_test.go @@ -11,193 +11,150 @@ package update import ( - "context" "testing" - "github.com/stretchr/testify/assert" - - "github.com/sourcenetwork/defradb/client" - testUtils "github.com/sourcenetwork/defradb/tests/integration/collection" + testUtils "github.com/sourcenetwork/defradb/tests/integration" ) -func TestUpdateWithInvalidFilterType(t *testing.T) { +func TestUpdateWithInvalidFilterType_ReturnsError(t *testing.T) { test := testUtils.TestCase{ Description: "Test update users with invalid filter type", - Docs: map[string][]string{}, - CollectionCalls: map[string][]func(client.Collection) error{ - "Users": []func(c client.Collection) error{ - func(c client.Collection) error { - ctx := context.Background() - // test with an invalid filter type - _, err := c.UpdateWithFilter( - ctx, - t, - `{"name": "Eric"}`, - ) - return err - }, + Actions: []any{ + testUtils.UpdateWithFilter{ + CollectionID: 0, + Filter: t, + Updater: `{"name": "Eric"}`, + ExpectedError: "invalid filter", }, }, - ExpectedError: "invalid filter", } executeTestCase(t, test) } -func TestUpdateWithEmptyFilter(t *testing.T) { +func TestUpdateWithEmptyFilter_ReturnsError(t *testing.T) { test := testUtils.TestCase{ Description: "Test update users with empty filter", - Docs: map[string][]string{}, - CollectionCalls: map[string][]func(client.Collection) error{ - "Users": []func(c client.Collection) error{ - func(c client.Collection) error { - ctx := context.Background() - // test with an empty filter - _, err := c.UpdateWithFilter( - ctx, - "", - `{"name": "Eric"}`, - ) - return err - }, + Actions: []any{ + testUtils.UpdateWithFilter{ + CollectionID: 0, + Filter: "", + Updater: `{"name": "Eric"}`, + ExpectedError: "invalid filter", }, }, - ExpectedError: "invalid filter", } executeTestCase(t, test) } -func TestUpdateWithFilter(t *testing.T) { - docStr := `{ - "name": "John", - "age": 21 - }` - - doc, err := client.NewDocFromJSON([]byte(docStr), colDefMap["Users"]) - if err != nil { - assert.Fail(t, err.Error()) +func TestUpdateWithInvalidJSON_ReturnsError(t *testing.T) { + test := testUtils.TestCase{ + Description: "Test update users with filter and invalid JSON", + Actions: []any{ + testUtils.CreateDoc{ + CollectionID: 0, + Doc: `{ + "name": "John", + "age": 21 + }`, + }, + testUtils.UpdateWithFilter{ + CollectionID: 0, + Filter: `{name: {_eq: "John"}}`, + Updater: `{name: "Eric"}`, + ExpectedError: "cannot parse JSON: cannot parse object", + }, + }, } - filter := `{name: {_eq: "John"}}` + executeTestCase(t, test) +} - tests := []testUtils.TestCase{ - { - Description: "Test update users with filter and invalid JSON", - Docs: map[string][]string{ - "Users": {docStr}, +func TestUpdateWithInvalidUpdater_ReturnsError(t *testing.T) { + test := testUtils.TestCase{ + Description: "Test update users with filter and invalid updator", + Actions: []any{ + testUtils.CreateDoc{ + CollectionID: 0, + Doc: `{ + "name": "John", + "age": 21 + }`, }, - CollectionCalls: map[string][]func(client.Collection) error{ - "Users": []func(c client.Collection) error{ - func(c client.Collection) error { - ctx := context.Background() - _, err := c.UpdateWithFilter( - ctx, - filter, - `{name: "Eric"}`, - ) - return err - }, - }, + testUtils.UpdateWithFilter{ + CollectionID: 0, + Filter: `{name: {_eq: "John"}}`, + Updater: `"name: Eric"`, + ExpectedError: "the updater of a document is of invalid type", }, - ExpectedError: "cannot parse JSON: cannot parse object", - }, { - Description: "Test update users with filter and invalid updator", - Docs: map[string][]string{ - "Users": {docStr}, + }, + } + + executeTestCase(t, test) +} + +func TestUpdateWithPatch_DoesNothing(t *testing.T) { + test := testUtils.TestCase{ + Description: "Test update users with filter and patch updator (not implemented so no change)", + Actions: []any{ + testUtils.CreateDoc{ + CollectionID: 0, + Doc: `{ + "name": "John", + "age": 21 + }`, }, - CollectionCalls: map[string][]func(client.Collection) error{ - "Users": []func(c client.Collection) error{ - func(c client.Collection) error { - ctx := context.Background() - _, err := c.UpdateWithFilter( - ctx, - filter, - `"name: Eric"`, - ) - return err - }, - }, + testUtils.UpdateWithFilter{ + CollectionID: 0, + Filter: `{name: {_eq: "John"}}`, + Updater: `[{"name": "Eric"}, {"name": "Sam"}]`, + SkipLocalUpdateEvent: true, }, - ExpectedError: "the updater of a document is of invalid type", - }, { - Description: "Test update users with filter and patch updator (not implemented so no change)", - Docs: map[string][]string{ - "Users": {docStr}, + testUtils.Request{ + Request: `query{ + Users { + name + } + }`, + Results: []map[string]any{ + {"name": "John"}, + }, }, - CollectionCalls: map[string][]func(client.Collection) error{ - "Users": []func(c client.Collection) error{ - func(c client.Collection) error { - ctx := context.Background() - _, err := c.UpdateWithFilter( - ctx, - filter, - `[ - { - "name": "Eric" - }, { - "name": "Sam" - } - ]`, - ) - if err != nil { - return err - } - - d, err := c.Get(ctx, doc.ID(), false) - if err != nil { - return err - } - - name, err := d.Get("name") - if err != nil { - return err - } + }, + } - assert.Equal(t, "John", name) + executeTestCase(t, test) +} - return nil - }, - }, +func TestUpdateWithFilter_Succeeds(t *testing.T) { + test := testUtils.TestCase{ + Description: "Test update users with filter", + Actions: []any{ + testUtils.CreateDoc{ + CollectionID: 0, + Doc: `{ + "name": "John", + "age": 21 + }`, }, - }, { - Description: "Test update users with filter", - Docs: map[string][]string{ - "Users": {docStr}, + testUtils.UpdateWithFilter{ + CollectionID: 0, + Filter: `{name: {_eq: "John"}}`, + Updater: `{"name": "Eric"}`, }, - CollectionCalls: map[string][]func(client.Collection) error{ - "Users": []func(c client.Collection) error{ - func(c client.Collection) error { - ctx := context.Background() - _, err := c.UpdateWithFilter( - ctx, - filter, - `{"name": "Eric"}`, - ) - if err != nil { - return err - } - - d, err := c.Get(ctx, doc.ID(), false) - if err != nil { - return err - } - - name, err := d.Get("name") - if err != nil { - return err - } - - assert.Equal(t, "Eric", name) - - return nil - }, + testUtils.Request{ + Request: `query{ + Users { + name + } + }`, + Results: []map[string]any{ + {"name": "Eric"}, }, }, }, } - for _, test := range tests { - executeTestCase(t, test) - } + executeTestCase(t, test) } diff --git a/tests/integration/collection/utils.go b/tests/integration/collection/utils.go deleted file mode 100644 index 497637a5c3..0000000000 --- a/tests/integration/collection/utils.go +++ /dev/null @@ -1,116 +0,0 @@ -// Copyright 2022 Democratized Data Foundation -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package collection - -import ( - "context" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/sourcenetwork/defradb/client" - "github.com/sourcenetwork/defradb/errors" - testUtils "github.com/sourcenetwork/defradb/tests/integration" -) - -type TestCase struct { - Description string - - // docs is a map from Collection name, to a list - // of docs in stringified JSON format - Docs map[string][]string - - CollectionCalls map[string][]func(client.Collection) error - - // Any error expected to be returned by collection calls. - ExpectedError string -} - -func ExecuteRequestTestCase( - t *testing.T, - schema string, - testCase TestCase, -) { - ctx := context.Background() - - db, err := testUtils.NewBadgerMemoryDB(ctx) - if err != nil { - t.Fatal(err) - } - - _, err = db.AddSchema(ctx, schema) - if assertError(t, testCase.Description, err, testCase.ExpectedError) { - return - } - - setupDatabase(ctx, t, db, testCase) - - for collectionName, collectionCallSet := range testCase.CollectionCalls { - col, err := db.GetCollectionByName(ctx, collectionName) - if assertError(t, testCase.Description, err, testCase.ExpectedError) { - return - } - - for _, collectionCall := range collectionCallSet { - err := collectionCall(col) - if assertError(t, testCase.Description, err, testCase.ExpectedError) { - return - } - } - } - - if testCase.ExpectedError != "" { - assert.Fail(t, "Expected an error however none was raised.", testCase.Description) - } -} - -func setupDatabase( - ctx context.Context, - t *testing.T, - db client.DB, - testCase TestCase, -) { - for collectionName, docs := range testCase.Docs { - col, err := db.GetCollectionByName(ctx, collectionName) - if assertError(t, testCase.Description, err, testCase.ExpectedError) { - return - } - - for _, docStr := range docs { - doc, err := client.NewDocFromJSON([]byte(docStr), col.Definition()) - if assertError(t, testCase.Description, err, testCase.ExpectedError) { - return - } - err = col.Save(ctx, doc) - if assertError(t, testCase.Description, err, testCase.ExpectedError) { - return - } - } - } -} - -func assertError(t *testing.T, description string, err error, expectedError string) bool { - if err == nil { - return false - } - - if expectedError == "" { - assert.NoError(t, err, description) - return false - } else { - if !strings.Contains(err.Error(), expectedError) { - assert.ErrorIs(t, err, errors.New(expectedError)) - return false - } - return true - } -} diff --git a/tests/integration/events.go b/tests/integration/events.go index c2db144d93..87b157a662 100644 --- a/tests/integration/events.go +++ b/tests/integration/events.go @@ -348,3 +348,46 @@ func getEventsForCreateDoc(s *state, action CreateDoc) map[string]struct{} { return expect } + +// getEventsForUpdateWithFilter returns a map of docIDs that should be +// published to the local event bus after a UpdateWithFilter action. +// +// This will take into account any primary documents that are patched as a result +// of the create or update. +func getEventsForUpdateWithFilter( + s *state, + action UpdateWithFilter, + result *client.UpdateResult, +) map[string]struct{} { + var collection client.Collection + if action.NodeID.HasValue() { + collection = s.collections[action.NodeID.Value()][action.CollectionID] + } else { + collection = s.collections[0][action.CollectionID] + } + + var docPatch map[string]any + err := json.Unmarshal([]byte(action.Updater), &docPatch) + require.NoError(s.t, err) + + def := collection.Definition() + expect := make(map[string]struct{}) + + for _, docID := range result.DocIDs { + expect[docID] = struct{}{} + + // check for any secondary relation fields that could publish an event + for name, value := range docPatch { + field, ok := def.GetFieldByName(name) + if !ok { + continue // ignore unknown field + } + _, ok = field.GetSecondaryRelationField(def) + if ok { + expect[value.(string)] = struct{}{} + } + } + } + + return expect +} diff --git a/tests/integration/test_case.go b/tests/integration/test_case.go index 1ebf396253..930146c8bb 100644 --- a/tests/integration/test_case.go +++ b/tests/integration/test_case.go @@ -360,6 +360,43 @@ type UpdateDoc struct { SkipLocalUpdateEvent bool } +// UpdateWithFilter will update the set of documents that match the given filter. +type UpdateWithFilter struct { + // NodeID may hold the ID (index) of a node to apply this update to. + // + // If a value is not provided the update will be applied to all nodes. + NodeID immutable.Option[int] + + // The identity of this request. Optional. + // + // If an Identity is not provided then can only update public document(s). + // + // If an Identity is provided and the collection has a policy, then + // can also update private document(s) that are owned by this Identity. + Identity immutable.Option[int] + + // The collection in which this document exists. + CollectionID int + + // The filter to match documents against. + Filter any + + // The update to apply to matched documents. + Updater string + + // Any error expected from the action. Optional. + // + // String can be a partial, and the test will pass if an error is returned that + // contains this string. + ExpectedError string + + // Skip waiting for an update event on the local event bus. + // + // This should only be used for tests that do not correctly + // publish an update event to the local event bus. + SkipLocalUpdateEvent bool +} + // IndexField describes a field to be indexed. type IndexedField struct { // Name contains the name of the field. diff --git a/tests/integration/utils2.go b/tests/integration/utils2.go index 139b53652b..31a93a0281 100644 --- a/tests/integration/utils2.go +++ b/tests/integration/utils2.go @@ -308,6 +308,9 @@ func performAction( case UpdateDoc: updateDoc(s, action) + case UpdateWithFilter: + updateWithFilter(s, action) + case CreateIndex: createIndex(s, action) @@ -1469,6 +1472,34 @@ func updateDocViaGQL( return nil } +// updateWithFilter updates the set of matched documents. +func updateWithFilter(s *state, action UpdateWithFilter) { + var res *client.UpdateResult + var expectedErrorRaised bool + actionNodes := getNodes(action.NodeID, s.nodes) + for nodeID, collections := range getNodeCollections(action.NodeID, s.collections) { + identity := getIdentity(s, nodeID, action.Identity) + ctx := db.SetContextIdentity(s.ctx, identity) + + err := withRetry( + actionNodes, + nodeID, + func() error { + var err error + res, err = collections[action.CollectionID].UpdateWithFilter(ctx, action.Filter, action.Updater) + return err + }, + ) + expectedErrorRaised = AssertError(s.t, s.testCase.Description, err, action.ExpectedError) + } + + assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised) + + if action.ExpectedError == "" && !action.SkipLocalUpdateEvent { + waitForUpdateEvents(s, action.NodeID, getEventsForUpdateWithFilter(s, action, res)) + } +} + // createIndex creates a secondary index using the collection api. func createIndex( s *state, From 82659f8cd55f291d091bff0597d0084ef61a5b51 Mon Sep 17 00:00:00 2001 From: Keenan Nemetz Date: Fri, 26 Jul 2024 14:55:55 -0700 Subject: [PATCH 2/2] refactor(i): Subscription test results (#2874) ## Relevant issue(s) Resolves #2873 ## Description This PR is small refactor of the subscription tests to make it easier to test ordering of results and eventually multiple selections within the results. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS --- .../subscription/subscription_test.go | 64 +++++++++++-------- tests/integration/test_case.go | 2 +- tests/integration/utils2.go | 56 ++++++---------- 3 files changed, 60 insertions(+), 62 deletions(-) diff --git a/tests/integration/subscription/subscription_test.go b/tests/integration/subscription/subscription_test.go index 2de42f2793..dc7a91d7c6 100644 --- a/tests/integration/subscription/subscription_test.go +++ b/tests/integration/subscription/subscription_test.go @@ -28,16 +28,20 @@ func TestSubscriptionWithCreateMutations(t *testing.T) { age } }`, - Results: []map[string]any{ + Results: [][]map[string]any{ { - "_docID": "bae-b3ce089b-f543-5984-be9f-ad7d08969f4e", - "age": int64(27), - "name": "John", + { + "_docID": "bae-b3ce089b-f543-5984-be9f-ad7d08969f4e", + "age": int64(27), + "name": "John", + }, }, { - "_docID": "bae-bc20b854-10b3-5408-b28c-f273ddda9434", - "age": int64(31), - "name": "Addo", + { + "_docID": "bae-bc20b854-10b3-5408-b28c-f273ddda9434", + "age": int64(31), + "name": "Addo", + }, }, }, }, @@ -82,10 +86,12 @@ func TestSubscriptionWithFilterAndOneCreateMutation(t *testing.T) { age } }`, - Results: []map[string]any{ + Results: [][]map[string]any{ { - "age": int64(27), - "name": "John", + { + "age": int64(27), + "name": "John", + }, }, }, }, @@ -119,7 +125,7 @@ func TestSubscriptionWithFilterAndOneCreateMutationOutsideFilter(t *testing.T) { age } }`, - Results: []map[string]any{}, + Results: [][]map[string]any{}, }, testUtils.Request{ Request: `mutation { @@ -150,10 +156,12 @@ func TestSubscriptionWithFilterAndCreateMutations(t *testing.T) { age } }`, - Results: []map[string]any{ + Results: [][]map[string]any{ { - "age": int64(27), - "name": "John", + { + "age": int64(27), + "name": "John", + }, }, }, }, @@ -217,11 +225,13 @@ func TestSubscriptionWithUpdateMutations(t *testing.T) { points } }`, - Results: []map[string]any{ + Results: [][]map[string]any{ { - "age": int64(27), - "name": "John", - "points": float64(45), + { + "age": int64(27), + "name": "John", + "points": float64(45), + }, }, }, }, @@ -273,16 +283,20 @@ func TestSubscriptionWithUpdateAllMutations(t *testing.T) { points } }`, - Results: []map[string]any{ + Results: [][]map[string]any{ { - "age": int64(31), - "name": "Addo", - "points": float64(55), + { + "age": int64(31), + "name": "Addo", + "points": float64(55), + }, }, { - "age": int64(27), - "name": "John", - "points": float64(55), + { + "age": int64(27), + "name": "John", + "points": float64(55), + }, }, }, }, diff --git a/tests/integration/test_case.go b/tests/integration/test_case.go index 930146c8bb..a384318dd2 100644 --- a/tests/integration/test_case.go +++ b/tests/integration/test_case.go @@ -616,7 +616,7 @@ type SubscriptionRequest struct { Request string // The expected (data) results yielded through the subscription across its lifetime. - Results []map[string]any + Results [][]map[string]any // Any error expected from the action. Optional. // diff --git a/tests/integration/utils2.go b/tests/integration/utils2.go index 31a93a0281..a9bd8aa4c3 100644 --- a/tests/integration/utils2.go +++ b/tests/integration/utils2.go @@ -1752,50 +1752,34 @@ func executeSubscriptionRequest( } go func() { - data := []map[string]any{} - errs := []error{} - + var results []*client.GQLResult allActionsAreDone := false - expectedDataRecieved := len(action.Results) == 0 - for { + for !allActionsAreDone || len(results) < len(action.Results) { select { case s := <-result.Subscription: - sData, _ := s.Data.([]map[string]any) - errs = append(errs, s.Errors...) - data = append(data, sData...) - - if len(data) >= len(action.Results) { - expectedDataRecieved = true - } + results = append(results, &s) case <-s.allActionsDone: allActionsAreDone = true } + } - if expectedDataRecieved && allActionsAreDone { - finalResult := &client.GQLResult{ - Data: data, - Errors: errs, - } - - subscriptionAssert <- func() { - // This assert should be executed from the main test routine - // so that failures will be properly handled. - expectedErrorRaised := assertRequestResults( - s, - finalResult, - action.Results, - action.ExpectedError, - nil, - // anyof is not yet supported by subscription requests - 0, - map[docFieldKey][]any{}, - ) - - assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised) - } - - return + subscriptionAssert <- func() { + for i, r := range action.Results { + // This assert should be executed from the main test routine + // so that failures will be properly handled. + expectedErrorRaised := assertRequestResults( + s, + results[i], + r, + action.ExpectedError, + nil, + // anyof is not yet supported by subscription requests + 0, + map[docFieldKey][]any{}, + ) + + assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised) } } }()