From d94bc734a06a972bb8e9b038683c3cbcfc788df5 Mon Sep 17 00:00:00 2001 From: Shamim Mohamed Date: Mon, 29 Jul 2019 14:02:09 -0700 Subject: [PATCH] Cleanup String methods, and do not use reflect.DeepEqual to compare entities (#395) * Cleanup String() methods, and do not use reflect to compare entities * review comments * bump up version to 3.4.8 --- CHANGELOG.md | 6 +- connectors/routing/config.go | 2 +- connectors/routing/config_test.go | 2 +- connectors/routing/routing_config.go | 4 +- connectors/routing/routing_config_test.go | 10 +- entity.go | 162 +++++++++++++++------- entity_test.go | 23 +++ version.go | 2 +- 8 files changed, 153 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59c16af4..def62129 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,11 @@ # Changelog -## v3.4.8 (unreleased) +## v3.4.9 (unreleased) + - Nothing changed yet + +## v3.4.8 (2019-07-26) - Add transport parameter to yarpc connector and cli + - CheckSchema needs to compare schemas carefully, and not just use reflect.DeepEqual ## v3.4.7 (2019-07-22) - Add String() method to all components of the routing connector diff --git a/connectors/routing/config.go b/connectors/routing/config.go index 27417237..7161d9cc 100644 --- a/connectors/routing/config.go +++ b/connectors/routing/config.go @@ -106,7 +106,7 @@ func (r *routers) UnmarshalYAML(unmarshal func(interface{}) error) error { if !ok { return fmt.Errorf("failed to parse the config: %v", namePrefixesMap) } - router, err := NewRule(scope, namePrefixStr, connectorName) + router, err := newRule(scope, namePrefixStr, connectorName) if err != nil { return errors.Wrap(err, "failed to parse routing config") } diff --git a/connectors/routing/config_test.go b/connectors/routing/config_test.go index bf788fe2..363f2862 100644 --- a/connectors/routing/config_test.go +++ b/connectors/routing/config_test.go @@ -101,7 +101,7 @@ func TestBasicConfig(t *testing.T) { } func buildRule(scope, namePrefix, connector string) *rule { - rc, _ := NewRule(scope, namePrefix, connector) + rc, _ := newRule(scope, namePrefix, connector) return rc } diff --git a/connectors/routing/routing_config.go b/connectors/routing/routing_config.go index 6a2f8efd..f5d8c301 100644 --- a/connectors/routing/routing_config.go +++ b/connectors/routing/routing_config.go @@ -41,8 +41,8 @@ type rule struct { canonPfx string } -// NewRule creates a rule. -func NewRule(scope, namePrefix, connector string) (*rule, error) { +// newRule creates a rule. +func newRule(scope, namePrefix, connector string) (*rule, error) { // scope must be a valid name, optionally with a suffix *. namePrefix must be a valid prefix name, // optionally with a suffix *. if scope == "" { diff --git a/connectors/routing/routing_config_test.go b/connectors/routing/routing_config_test.go index 65d561e3..a1dd74be 100644 --- a/connectors/routing/routing_config_test.go +++ b/connectors/routing/routing_config_test.go @@ -28,29 +28,29 @@ import ( ) func TestNewRoutingConfig(t *testing.T) { - rConfig, err := NewRule("production", "test", "memory") + rConfig, err := newRule("production", "test", "memory") assert.Nil(t, err) assert.Equal(t, rConfig.namePrefix, "test") } func TestNewRoutingConfigWithStarPrefix(t *testing.T) { - rConfig, err := NewRule("production", "*.v1", "memory") + rConfig, err := newRule("production", "*.v1", "memory") assert.Nil(t, rConfig) assert.Contains(t, err.Error(), "invalid") } func TestTestNewRoutingConfigError(t *testing.T) { - rConfig, err := NewRule("production", "", "memory") + rConfig, err := newRule("production", "", "memory") assert.Nil(t, rConfig) assert.Contains(t, err.Error(), "cannot be empty") - rConfig, err = NewRule("", "test", "memory") + rConfig, err = newRule("", "test", "memory") assert.Nil(t, rConfig) assert.Contains(t, err.Error(), "scope cannot be empty") } func TestString(t *testing.T) { - r, err := NewRule("production", "test", "memory") + r, err := newRule("production", "test", "memory") assert.Nil(t, err) assert.Equal(t, "{production.test -> memory}", r.String()) } diff --git a/entity.go b/entity.go index beba121b..c3712f70 100644 --- a/entity.go +++ b/entity.go @@ -22,10 +22,9 @@ package dosa import ( "bytes" + "fmt" + "sort" "strings" - - "reflect" - "time" "github.com/pkg/errors" @@ -55,6 +54,15 @@ func (ck ClusteringKey) String() string { return ck.Name + " ASC" } +// Format a key's ClusteringKeys. +func formatClusteringKeys(keys []*ClusteringKey) string { + pieces := make([]string, len(keys)) + for index, ck := range keys { + pieces[index] = ck.String() + } + return strings.Join(pieces, ", ") +} + // PrimaryKey stores information about partition keys and clustering keys type PrimaryKey struct { PartitionKeys []string @@ -70,7 +78,6 @@ func (pk PrimaryKey) Clone() *PrimaryKey { for i, k := range pk.PartitionKeys { npk.PartitionKeys[i] = k } - } if pk.ClusteringKeys != nil { @@ -86,6 +93,45 @@ func (pk PrimaryKey) Clone() *PrimaryKey { return npk } +func (pk PrimaryKey) String() string { + var b bytes.Buffer + b.WriteByte('(') + b.WriteString(formatPartitionKeys(pk.PartitionKeys)) + b.WriteString(pk.clusteringKeysString()) + b.WriteByte(')') + return b.String() +} + +func (pk PrimaryKey) clusteringKeysString() string { + if pk.ClusteringKeys == nil || len(pk.ClusteringKeys) == 0 { + return "" + } + return ", " + formatClusteringKeys(pk.ClusteringKeys) +} + +func (pk PrimaryKey) equal(other *PrimaryKey) error { + if !stringSliceEqual(pk.PartitionKeys, other.PartitionKeys) { + return errors.Errorf("partition key mismatch: (%v vs %v)", pk.PartitionKeys, other.PartitionKeys) + } + if err := clusteringKeysEqual(pk.ClusteringKeys, other.ClusteringKeys); err != nil { + return err + } + return nil +} + +func clusteringKeysEqual(ck1, ck2 []*ClusteringKey) error { + if len(ck1) != len(ck2) { + return errors.Errorf("clustering keys mismatch: (%v vs %v)", ck1, ck2) + } + for i, k1 := range ck1 { + k2 := ck2[i] + if *k1 != *k2 { + return errors.Errorf("clustering key mismatch: (%v vs %v)", k1, k2) + } + } + return nil +} + // ClusteringKeySet returns a set of all clustering keys. func (pk PrimaryKey) ClusteringKeySet() map[string]struct{} { m := make(map[string]struct{}) @@ -113,16 +159,6 @@ func (pk PrimaryKey) PrimaryKeySet() map[string]struct{} { return m } -// formatClusteringKeys takes an array of ClusteringKeys and returns -// a string that shows all of them, separated by commas -func formatClusteringKeys(keys []*ClusteringKey) string { - pieces := make([]string, len(keys)) - for index, ck := range keys { - pieces[index] = ck.String() - } - return strings.Join(pieces, ", ") -} - func formatPartitionKeys(keys []string) string { if len(keys) > 1 { return "(" + strings.Join(keys, ", ") + ")" @@ -130,21 +166,6 @@ func formatPartitionKeys(keys []string) string { return keys[0] } -// String method produces the following output: -// for multiple partition keys: ((partition-key, ...), clustering-key ASC/DESC, ...) -// for one partition key: (partition-key, clustering-key ASC/DESC, ...) -func (pk PrimaryKey) String() string { - var b bytes.Buffer - b.WriteByte('(') - b.WriteString(formatPartitionKeys(pk.PartitionKeys)) - if pk.ClusteringKeys != nil && len(pk.ClusteringKeys) > 0 { - b.WriteString(", ") - b.WriteString(formatClusteringKeys(pk.ClusteringKeys)) - } - b.WriteByte(')') - return b.String() -} - // ColumnDefinition stores information about a column type ColumnDefinition struct { Name string // normalized column name @@ -157,13 +178,31 @@ type ColumnDefinition struct { // Clone returns a deep copy of ColumnDefinition func (cd *ColumnDefinition) Clone() *ColumnDefinition { - // TODO: clone tag return &ColumnDefinition{ - Name: cd.Name, - Type: cd.Type, + Name: cd.Name, + Type: cd.Type, + IsPointer: cd.IsPointer, + Tags: cd.cloneTags(), } } +func (cd *ColumnDefinition) cloneTags() map[string]string { + if cd.Tags == nil { + return nil + } + tags := map[string]string{} + for k, v := range cd.Tags { + tags[k] = v + } + return tags +} + +func (cd *ColumnDefinition) String() string { + // We want this to be deterministic, and cd.Tags is a map.... + return fmt.Sprintf("{Name: %s, Type: %v, IsPointer: %v, Tags: %s}", cd.Name, cd.Type, cd.IsPointer, + deterministicPrintMap(cd.Tags)) +} + // IndexDefinition stores information about a DOSA entity's index type IndexDefinition struct { Key *PrimaryKey @@ -182,6 +221,20 @@ func (id *IndexDefinition) Clone() *IndexDefinition { } } +func (id *IndexDefinition) String() string { + return fmt.Sprintf("%+v", *id) +} + +func (id *IndexDefinition) equal(other *IndexDefinition) error { + if err := id.Key.equal(other.Key); err != nil { + return errors.Errorf("partitionKey mismatch: (%v)", err) + } + if !stringSliceEqual(id.Columns, other.Columns) { + return errors.Errorf("columns mismatch: (%v vs %v)", id.Columns, other.Columns) + } + return nil +} + // EntityDefinition stores information about a DOSA entity type EntityDefinition struct { Name string // normalized entity name @@ -412,21 +465,10 @@ func (e *EntityDefinition) CanBeUpsertedOn(older *EntityDefinition) error { } // primary key should be exactly same - pksNewer := newer.Key.PartitionKeys - pksOlder := older.Key.PartitionKeys - - b := reflect.DeepEqual(pksNewer, pksOlder) - if !b { - return errors.Errorf("partition key mismatch: (%v vs %v)", pksNewer, pksOlder) + if err := newer.Key.equal(older.Key); err != nil { + return err } - cksNewer := newer.Key.ClusteringKeys - cksOlder := older.Key.ClusteringKeys - if len(cksOlder) != 0 || len(cksNewer) != 0 { - if !reflect.DeepEqual(cksNewer, cksOlder) { - return errors.Errorf("clustering key mismatch: (%v vs %v)", cksNewer, cksOlder) - } - } // only allow to add new columns colsMapNewer := newer.ColumnTypes() colsMapOlder := older.ColumnTypes() @@ -453,8 +495,9 @@ func (e *EntityDefinition) CanBeUpsertedOn(older *EntityDefinition) error { return errors.Errorf("Index %s in the old entity %s are missing in the new entity", name, older.Name) } - if !reflect.DeepEqual(indexNewer, indexOlder) { - return errors.Errorf("index %q mismatch: (%v vs %v)", name, indexNewer, indexOlder) + // Type here is *IndexDefinition + if err := indexNewer.equal(indexOlder); err != nil { + return errors.Errorf("index %q mismatch: %v", name, err) } } } @@ -503,3 +546,28 @@ func (e *EntityDefinition) UniqueKey(oldKey *PrimaryKey) *PrimaryKey { return &result } + +func stringSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i, s := range a { + if s != b[i] { + return false + } + } + return true +} + +func deterministicPrintMap(m map[string]string) string { + keys := make([]string, 0, len(m)) + for k, _ := range m { + keys = append(keys, k) + } + sort.Strings(keys) + pairs := make([]string, 0, len(m)) + for _, k := range keys { + pairs = append(pairs, fmt.Sprintf("%s: %s", k, m[k])) + } + return "{" + strings.Join(pairs, ", ") + "}" +} diff --git a/entity_test.go b/entity_test.go index 6c512b5c..87c9e6b5 100644 --- a/entity_test.go +++ b/entity_test.go @@ -553,3 +553,26 @@ func TestClone(t *testing.T) { ed1 := ed.Clone() assert.Equal(t, ed, ed1) } + +func TestColumnDefinitionPrint(t *testing.T) { + cd := dosa.ColumnDefinition{ + Name: "foo", + Type: dosa.TUUID, + IsPointer: true, + Tags: map[string]string{ + "e": "bar1", + "c": "bar2", + "a": "bar3", + "b": "bar4", + "d": "bar5", + }, + } + assert.Equal(t, "{Name: foo, Type: TUUID, IsPointer: true, Tags: {a: bar3, b: bar4, c: bar2, d: bar5, e: bar1}}", + cd.String()) + + cd = dosa.ColumnDefinition{ + Name: "bar", + Type: dosa.Int64, + } + assert.Equal(t, "{Name: bar, Type: Int64, IsPointer: false, Tags: {}}", cd.String()) +} diff --git a/version.go b/version.go index fb319b49..098a2fbd 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package dosa // VERSION indicates the dosa client version -const VERSION = "3.4.7" +const VERSION = "3.4.8"