From 6869e536e7d1f1de6db34af679c864f01cea09ce Mon Sep 17 00:00:00 2001 From: Sam Lai Date: Tue, 28 Mar 2023 13:28:18 +0100 Subject: [PATCH 1/4] mock: validate empty partition keys for all operations The mock currently only validates empty partition keys for the Set operation, leading to unexpected success when empty partition keys are mistakenly used for Delete/Update/Read operations in tests. When testing against a real C* or Keyspaces instance, a `Key may not be empty` error will appear, code `0x2200`. This PR copies the logic used for the Set operation over to the other operations. I kept the same error message, even though it doesn't match the C* one. For clarity, note that - * a single partition key cannot be an empty string * a composite partition key can have one or all empty string values * a single or composite clustering columns can have one or all empty string values I've added tests for these for the Map and MultiMap table types, but couldn't for the MultiMapMultiKey table type because there's a bug with the implementation there that eliminates empty key values even when they're allowed by C* and Keyspaces. Changing that would change app behaviour, so I've left that as-is, with a few added comments. --- mock.go | 37 +++++++++++++++++++++++++++++++++++++ mock_test.go | 32 ++++++++++++++++++++++++++++++++ multimap_multikey_table.go | 2 ++ 3 files changed, 71 insertions(+) diff --git a/mock.go b/mock.go index f6092eb..f8503e4 100644 --- a/mock.go +++ b/mock.go @@ -544,6 +544,10 @@ func (f *MockFilter) UpdateWithOptions(m map[string]interface{}, options Options return err } + if err := validatePartitionKeys(rowKeys); err != nil { + return err + } + for _, rowKey := range rowKeys { superColumnKeys, err := f.fieldsFromRelations(f.table.keys.ClusteringColumns) if err != nil { @@ -583,6 +587,10 @@ func (f *MockFilter) Delete() Op { return err } + if err := validatePartitionKeys(rowKeys); err != nil { + return err + } + f.table.mtx.Lock() defer f.table.mtx.Unlock() for _, rowKey := range rowKeys { @@ -651,6 +659,10 @@ func (q *MockFilter) readSomeRows() ([]map[string]interface{}, error) { return nil, err } + if err := validatePartitionKeys(rowKeys); err != nil { + return nil, err + } + var result []map[string]interface{} for _, rowKey := range rowKeys { row := q.table.rows[rowKey.RowKey()] @@ -694,6 +706,31 @@ func (q *MockFilter) ReadOne(out interface{}) Op { }) } +func validatePartitionKeys(keys []key) error { + if len(keys) == 0 { + return fmt.Errorf("Missing all mandatory PRIMARY KEYs") + } + + // no validation needed for composite partition keys + if len(keys) > 1 { + return nil + } + + // For a single partition key of type string, check that it is not + // empty, this is same as this error from a real C* cluster- + // InvalidRequest: Error from server: code=2200 [Invalid query] + // message="Key may not be empty" + for _, k := range keys[0] { + value := k.Value + stringVal, isString := value.(string) + if isString && stringVal == "" { + return fmt.Errorf("Missing mandatory PRIMARY KEY part %s", k.Key) + } + } + + return nil +} + // mockIterator takes in a slice of maps and implements a Scannable iterator // which goes row by row within the slice. type mockIterator struct { diff --git a/mock_test.go b/mock_test.go index 594876e..ac1cdfb 100644 --- a/mock_test.go +++ b/mock_test.go @@ -122,6 +122,25 @@ func (s *MockSuite) TestEmptyPrimaryKey() { s.NoError(s.embMapTbl.Set(add).Run()) s.NoError(s.addressByCountyMmTbl.Set(add).Run()) + // SELECTs + selectAdd := &address{} + s.Error(s.embMapTbl.Read("", selectAdd).Run()) + s.Error(s.addressByCountyMmTbl.Read("", add.Id, selectAdd).Run()) + // empty clustering columns are ok though + s.NoError(s.addressByCountyMmTbl.Read(add.County, "", selectAdd).Run()) + + // UPDATEs + s.Error(s.embMapTbl.Update("", map[string]interface{}{}).Run()) + s.Error(s.addressByCountyMmTbl.Update("", add.Id, map[string]interface{}{}).Run()) + // empty clustering columns are ok though + s.NoError(s.addressByCountyMmTbl.Update(add.County, "", map[string]interface{}{}).Run()) + + // DELETEs + s.Error(s.embMapTbl.Delete("").Run()) + s.Error(s.addressByCountyMmTbl.Delete("", add.Id).Run()) + // empty clustering columns are ok though + s.NoError(s.addressByCountyMmTbl.Delete(add.County, "").Run()) + add = address{ Id: "", TownID: "", @@ -134,6 +153,19 @@ func (s *MockSuite) TestEmptyPrimaryKey() { // no error in writing all empty values to a table with a composite // primary key s.NoError(s.mmMkTable.Set(add).Run()) + + // can't test SELECT/UPDATE/DELETEs for mmMkTable because it doesn't support + // empty composite partition keys or clustering columns, even though C* + // does, see + // https://github.com/monzo/gocassa/blob/ac4547a19b85b0fefbc2e004288dc6f4cbe46aaa/multimap_multikey_table.go#L84 + // + // TODO: uncomment the following lines if support for empty composite keys + // is added + // partitionKeys := map[string]interface{}{"Id": "", "TownID": ""} + // clusteringColumns := map[string]interface{}{"County": ""} + // s.NoError(s.mmMkTable.Read(partitionKeys, clusteringColumns, selectAdd).Run()) + // s.NoError(s.mmMkTable.Update(partitionKeys, clusteringColumns, map[string]interface{}{"PostCode": "DEF"}).Run()) + // s.NoError(s.mmMkTable.Delete(partitionKeys, clusteringColumns).Run()) } func (s *MockSuite) TestTableRead() { diff --git a/multimap_multikey_table.go b/multimap_multikey_table.go index 9e7a4d8..ec37757 100644 --- a/multimap_multikey_table.go +++ b/multimap_multikey_table.go @@ -80,6 +80,7 @@ func (mm *multimapMkT) WithOptions(o Options) MultimapMkTable { func (mm *multimapMkT) ListOfEqualRelations(fieldsToIndex, ids map[string]interface{}) []Relation { relations := make([]Relation, 0) + // TODO: support empty values in composite partition keys for _, field := range mm.fieldsToIndexBy { if value := fieldsToIndex[field]; value != nil && value != "" { relation := Eq(field, value) @@ -87,6 +88,7 @@ func (mm *multimapMkT) ListOfEqualRelations(fieldsToIndex, ids map[string]interf } } + // TODO: support empty values in composite clustering keys for _, field := range mm.idField { if value := ids[field]; value != nil && value != "" { relation := Eq(field, value) From 71858aa9713f23e6471a98ab2c8d03b952afadf9 Mon Sep 17 00:00:00 2001 From: Sam Lai Date: Tue, 28 Mar 2023 13:50:27 +0100 Subject: [PATCH 2/4] clean up comment --- multimap_multikey_table.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/multimap_multikey_table.go b/multimap_multikey_table.go index ec37757..5c35386 100644 --- a/multimap_multikey_table.go +++ b/multimap_multikey_table.go @@ -88,7 +88,7 @@ func (mm *multimapMkT) ListOfEqualRelations(fieldsToIndex, ids map[string]interf } } - // TODO: support empty values in composite clustering keys + // TODO: support empty values in clustering columns for _, field := range mm.idField { if value := ids[field]; value != nil && value != "" { relation := Eq(field, value) From 8ebcaefef87496be54693c6e2d8ab111a1e606dd Mon Sep 17 00:00:00 2001 From: Sam Lai Date: Tue, 28 Mar 2023 13:51:38 +0100 Subject: [PATCH 3/4] more comment tweaks --- multimap_multikey_table.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/multimap_multikey_table.go b/multimap_multikey_table.go index 5c35386..e4cd72c 100644 --- a/multimap_multikey_table.go +++ b/multimap_multikey_table.go @@ -80,7 +80,7 @@ func (mm *multimapMkT) WithOptions(o Options) MultimapMkTable { func (mm *multimapMkT) ListOfEqualRelations(fieldsToIndex, ids map[string]interface{}) []Relation { relations := make([]Relation, 0) - // TODO: support empty values in composite partition keys + // TODO: support empty string values in composite partition keys for _, field := range mm.fieldsToIndexBy { if value := fieldsToIndex[field]; value != nil && value != "" { relation := Eq(field, value) @@ -88,7 +88,7 @@ func (mm *multimapMkT) ListOfEqualRelations(fieldsToIndex, ids map[string]interf } } - // TODO: support empty values in clustering columns + // TODO: support empty string values in clustering columns for _, field := range mm.idField { if value := ids[field]; value != nil && value != "" { relation := Eq(field, value) From b5dc3a41595e639a56359d7cf328a48758046bf3 Mon Sep 17 00:00:00 2001 From: Sam Lai Date: Wed, 29 Mar 2023 16:41:53 +0100 Subject: [PATCH 4/4] allow no partition keys --- mock.go | 4 +++- mock_test.go | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/mock.go b/mock.go index f8503e4..b1fd9fe 100644 --- a/mock.go +++ b/mock.go @@ -707,8 +707,10 @@ func (q *MockFilter) ReadOne(out interface{}) Op { } func validatePartitionKeys(keys []key) error { + // this is not allowed for DELETE/UPDATEs, but is allowed for SELECTs, so + // we don't validate here if len(keys) == 0 { - return fmt.Errorf("Missing all mandatory PRIMARY KEYs") + return nil } // no validation needed for composite partition keys diff --git a/mock_test.go b/mock_test.go index ac1cdfb..a895513 100644 --- a/mock_test.go +++ b/mock_test.go @@ -270,6 +270,10 @@ func (s *MockSuite) TestMapTableMultiRead() { s.Len(users, 2) s.Equal("Jane", users[0].Name) s.Equal("Jill", users[1].Name) + + var users2 []user + s.NoError(s.mapTbl.MultiRead([]interface{}{}, &users2).Run()) + s.Len(users2, 0) } func (s *MockSuite) TestMapTableUpdate() {