Cleanup String methods, and do not use reflect.DeepEqual to compare e…

…ntities (#395)

* Cleanup String() methods, and do not use reflect to compare entities

* review comments

* bump up version to 3.4.8

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

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")
 				}

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
 }
 

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 == "" {

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())
 }

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,38 +159,13 @@ 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, ", ") + ")"
 	}
 	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, ", ") + "}"
+}

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())
+}

version.go

@@ -21,4 +21,4 @@
 package dosa
 
 // VERSION indicates the dosa client version
-const VERSION = "3.4.7"
+const VERSION = "3.4.8"