From b297b71b7da0d2fe4a094baa250a9ac93a9622ce Mon Sep 17 00:00:00 2001 From: Shamim Mohamed Date: Mon, 22 Jul 2019 15:12:20 -0700 Subject: [PATCH] Routing connector fixes (#393) * Add String() methods to all components of the routing connector * better test * Tests for the routing connector * Initial * more.... * cleanup, and another test * reorg comments * cleanup parsing code * test to ensure a.b.c > a.* * no need for routing.Rule to be accessible outside the package * No need for DefaultName to be public * type Routers also does not need to be public * oops, forgot a logging print * remove dead code * update changelog * update version.go --- CHANGELOG.md | 6 +- client_test.go | 2 +- connectors/routing/config.go | 112 +++++---- connectors/routing/config_test.go | 290 +++++++++++++++++----- connectors/routing/connector.go | 17 +- connectors/routing/connector_test.go | 32 ++- connectors/routing/routing_config.go | 186 +++++++++++--- connectors/routing/routing_config_test.go | 93 ++++++- entity_parser_key_parser_test.go | 4 +- entity_parser_test.go | 2 +- entity_test.go | 4 +- names.go | 60 ++--- names_test.go | 63 ++++- version.go | 2 +- 14 files changed, 652 insertions(+), 221 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74d6573a..d40dbc8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,11 @@ # Changelog -## v3.4.7 (unreleased) +## v3.4.8 (unreleased) + - Nothing changed yet + +## v3.4.7 (2019-07-22) - Add String() method to all components of the routing connector + - Fix many bugs in the routing connector ## v3.4.6 (2019-06-11) - use append but not copy to clone diff --git a/client_test.go b/client_test.go index b290a914..a71824a5 100644 --- a/client_test.go +++ b/client_test.go @@ -1061,7 +1061,7 @@ type TestEntityC struct { { dirs: []string{tmpdir}, scope: scope, - errContains: "cannot be empty", + errContains: "invalid", }, } diff --git a/connectors/routing/config.go b/connectors/routing/config.go index c6938937..27417237 100644 --- a/connectors/routing/config.go +++ b/connectors/routing/config.go @@ -28,34 +28,71 @@ import ( "github.com/pkg/errors" ) -// DefaultScope represents the default scope -const DefaultScope = "default" +// defaultName is an alias for the glob "*" (regexp .*) +const defaultName = "default" -// Routers represents a list of routing config -type Routers []*Rule +//////////////////////////////////////////////////////////////////////////////////////////////////// +// NOTE: "Router" is a synonym for "Rule". +//////////////////////////////////////////////////////////////////////////////////////////////////// -func (r Routers) Len() int { +// Config for the routing connector is a "case statement" of scope names, and each entry is a list +// of assigments "pattern" -> engine-name. +// +// Example: +// +// routers: +// - "*": +// sless_*: schemaless +// "*": dosa_dev +// - production: +// serviceA: cassandra +// serviceX: schemaless +// *: dosa +// - development: +// *: dosa_dev +// serviceB: cassandra +// serviceX: schemaless +// - engineB_*: +// *: engine-b +// - ebook: +// '*': ebook +// apple.*: ebook +// apple.foo.bar: other-one +// ebook_store: ebook +// +// A pattern is not a regular expression: only prefixes may be specified (i.e. trailing "*"). +// The string "default" is a synonym for "*". +// Rules are tried in order. Literal strings (no "*") sort before patterns, i.e. "footer" < "foo*" +// +type Config struct { + Routers routers `yaml:"routers"` +} + +// routers represents a list of routing rules. +type routers []*rule + +// Sort methods so that rules are ordered according to the spec. +func (r routers) Len() int { return len(r) } -func (r Routers) Swap(i, j int) { +func (r routers) Swap(i, j int) { r[i], r[j] = r[j], r[i] } -func (r Routers) Less(i, j int) bool { - if r[i].Scope == r[j].Scope { - return r[i].NamePrefix > r[j].NamePrefix +func (r routers) Less(i, j int) bool { + if r[i].canonScope == r[j].canonScope { + return r[i].canonPfx < r[j].canonPfx } - return r[i].Scope > r[j].Scope + return r[i].canonScope < r[j].canonScope } // UnmarshalYAML unmarshals the config into gocql cluster config -func (r *Routers) UnmarshalYAML(unmarshal func(interface{}) error) error { - routers := make(Routers, 0) +func (r *routers) UnmarshalYAML(unmarshal func(interface{}) error) error { + routers := make(routers, 0) scopes := make([]map[string]interface{}, 0) if err := unmarshal(&scopes); err != nil { return err } - defaultRouterExist := false for _, scopeMap := range scopes { for scope, namePrefixes := range scopeMap { namePrefixesMap, ok := namePrefixes.(map[interface{}]interface{}) @@ -74,54 +111,35 @@ func (r *Routers) UnmarshalYAML(unmarshal func(interface{}) error) error { return errors.Wrap(err, "failed to parse routing config") } routers = append(routers, router) - if scope == DefaultScope { - defaultRouterExist = true - } } } } - - if !defaultRouterExist { - return errors.New("there should be a default scope defined in routing config yaml file") + sort.Sort(routers) + lastRule := routers[len(routers)-1] + if lastRule.Scope() != "*" || lastRule.NamePrefix() != "*" { + return errors.New("no default rule defined in the 'routers' config") } - sort.Sort(routers) *r = routers return nil } -// Config is a struct contains fields from yaml -// scope should be an exact string in any case, -// namePrefix could be in 3 different format: -// 1. exact string like "service" that matches namePrefix that is exactly "service" -// 2. partial glob match like "service.*" that matches all namePrefix that has a prefix of "service." -// 3. full glob match like "*" that matches all namePrefix -type Config struct { - Routers Routers `yaml:"routers"` -} - -// FindRouter finds the router information based on scope and namePrefix. -func (c *Config) FindRouter(scope, namePrefix string) *Rule { - for _, router := range c.Routers { - if router.RouteTo(scope, namePrefix) { - return router +// getEngineName returns the name of the engine to use for a given (scope, name-prefix). The "Routers" list +// MUST be sorted in priority order. +func (c *Config) getEngineName(scope, namePrefix string) string { + // At some point we should replace this sequential search with something like a trie.... + for _, rule := range c.Routers { + if rule.canHandle(scope, namePrefix) { + return rule.Destination() } } - return c.findDefaultRouter() -} - -// findDefaultRouter finds the default router information. -func (c *Config) findDefaultRouter() *Rule { - for _, router := range c.Routers { - if router.Scope == DefaultScope { - return router - } - } - return nil + // The last rule in the list is the default rule, which always exists; we should never + // reach this point. + return "an unknown error has occurred" } -func (r *Routers) String() string { +func (r *routers) String() string { s := []string{} for _, rule := range *r { s = append(s, rule.String()) diff --git a/connectors/routing/config_test.go b/connectors/routing/config_test.go index 1bba0397..bf788fe2 100644 --- a/connectors/routing/config_test.go +++ b/connectors/routing/config_test.go @@ -28,104 +28,266 @@ import ( "gopkg.in/yaml.v2" ) -// TestBasicConfig test the basic yaml file conversion -func TestBasicConfig(t *testing.T) { - yamlFile := ` +var yamlFile = ` routers: -# routers structure looks like: -# - [scope] -# [namePrefix_1]: connectorName -# [namePrefix_2]: connectorName - production: - default: cassandra - serviceA: cassandra + "*": cassandra1 + serviceA: cassandra2 - development: - default: cassandra - serviceB: cassandra + default: cassandra3 + serviceB: cassandra4 + a.b.c.d.*: external - ebook: - '*': ebook - apple.*: ebook - default: ebook - ebook-store: ebook + app: ebook0 + apple.*: ebook2 + apple.foo.bar: ebook5 + '*': ebook1 + ebook_store: ebook4 +- ebook*: + foo: ebook-foo + foo*: ebook-foo-2 + '*': ebook.42 - default: - default: dosa + foo_*: dosa1 + default: dosa2 ` + +// TestBasicConfig test the basic yaml file conversion +func TestBasicConfig(t *testing.T) { testCfg := &Config{} err := yaml.Unmarshal([]byte(yamlFile), testCfg) assert.NoError(t, err) - assert.Len(t, testCfg.Routers, 9) - rs := Routers{ - buildRouter("production", "serviceA", "cassandra"), - buildRouter("production", "default", "cassandra"), - buildRouter("ebook", "ebook-store", "ebook"), - buildRouter("ebook", "default", "ebook"), - buildRouter("ebook", "apple.*", "ebook"), - buildRouter("ebook", "*", "ebook"), - buildRouter("development", "serviceB", "cassandra"), - buildRouter("development", "default", "cassandra"), - buildRouter("default", "default", "dosa"), + rs := routers{ + buildRule("development", "a.b.c.d.*", "external"), + buildRule("development", "serviceB", "cassandra4"), + buildRule("development", "default", "cassandra3"), + buildRule("ebook", "app", "ebook0"), + buildRule("ebook", "apple.foo.bar", "ebook5"), + buildRule("ebook", "apple.*", "ebook2"), + buildRule("ebook", "ebook_store", "ebook4"), + buildRule("ebook", "*", "ebook1"), + buildRule("ebook*", "foo", "ebook-foo"), + buildRule("ebook*", "foo*", "ebook-foo-2"), + buildRule("ebook*", "*", "ebook.42"), + buildRule("production", "serviceA", "cassandra2"), + buildRule("production", "*", "cassandra1"), + buildRule("default", "foo_*", "dosa1"), + buildRule("default", "default", "dosa2"), } assert.Equal(t, testCfg.Routers, rs) + err = yaml.Unmarshal([]byte(`bad yaml file`), testCfg) assert.Error(t, err) s := []string{ - "{production.serviceA -> cassandra}", - "{production.default -> cassandra}", - "{ebook.ebook-store -> ebook}", - "{ebook.default -> ebook}", - "{ebook.apple.* -> ebook}", - "{ebook.* -> ebook}", - "{development.serviceB -> cassandra}", - "{development.default -> cassandra}", - "{default.default -> dosa}", + "{development.a.b.c.d.* -> external}", + "{development.serviceB -> cassandra4}", + "{development.* -> cassandra3}", + "{ebook.app -> ebook0}", + "{ebook.apple.foo.bar -> ebook5}", + "{ebook.apple.* -> ebook2}", + "{ebook.ebook_store -> ebook4}", + "{ebook.* -> ebook1}", + "{ebook*.foo -> ebook-foo}", + "{ebook*.foo* -> ebook-foo-2}", + "{ebook*.* -> ebook.42}", + "{production.serviceA -> cassandra2}", + "{production.* -> cassandra1}", + "{*.foo_* -> dosa1}", + "{*.* -> dosa2}", } assert.Equal(t, "["+strings.Join(s, ",")+"]", rs.String()) } -func buildRouter(scope, namePrefix, connector string) *Rule { +func buildRule(scope, namePrefix, connector string) *rule { rc, _ := NewRule(scope, namePrefix, connector) return rc } -func TestRouter(t *testing.T) { - yamlFile := ` +func TestMissingDefault(t *testing.T) { + badFile := ` routers: -# routers structure looks like: -# - [scope] -# [namePrefix_1]: connectorName -# [namePrefix_2]: connectorName - production: - default: cassandra - serviceA: cassandra + "*": cassandra1 + serviceA: cassandra2 - development: - default: cassandra - serviceB: cassandra -- ebook: - '*': ebook - apple.*: ebook - default: ebook - ebook-store: ebook -- default: - default: dosa + default: cassandra3 + serviceB: cassandra4 +- "*": + foo_*: dosa1 ` + var cfg Config + err := yaml.Unmarshal([]byte(badFile), &cfg) + assert.Error(t, err) + assert.Contains(t, err.Error(), "no default rule") +} + +func TestRouting(t *testing.T) { testCfg := &Config{} err := yaml.Unmarshal([]byte(yamlFile), testCfg) assert.NoError(t, err) - cfg := testCfg.findDefaultRouter() - assert.Equal(t, cfg.Scope, "default") + var eng string + + eng = testCfg.getEngineName("production", "serviceA") + assert.Equal(t, eng, "cassandra2") + + eng = testCfg.getEngineName("production", "something_else") + assert.Equal(t, eng, "cassandra1") + + eng = testCfg.getEngineName("development", "serviceA") + assert.Equal(t, eng, "cassandra3") + + eng = testCfg.getEngineName("development", "a.b.c.d.") + assert.Equal(t, eng, "external") + + eng = testCfg.getEngineName("development", "a.b.c.d.42") + assert.Equal(t, eng, "external") + + eng = testCfg.getEngineName("development", "a.b.c.d,42") + assert.Equal(t, eng, "cassandra3") + + eng = testCfg.getEngineName("development", "a.b.c.d42") + assert.Equal(t, eng, "cassandra3") + + eng = testCfg.getEngineName("ebook", "app") + assert.Equal(t, eng, "ebook0") + + eng = testCfg.getEngineName("ebook", "apple.k") + assert.Equal(t, eng, "ebook2") + + eng = testCfg.getEngineName("ebook", "apple.foo.bar") + assert.Equal(t, eng, "ebook5") + + eng = testCfg.getEngineName("ebook", "apple.foo.bar.") + assert.Equal(t, eng, "ebook2") + + eng = testCfg.getEngineName("ebook", "apple2") + assert.Equal(t, eng, "ebook1") + + eng = testCfg.getEngineName("ebook", "d.k") + assert.Equal(t, eng, "ebook1") + + eng = testCfg.getEngineName("ebook", "foo") + assert.Equal(t, eng, "ebook1") + + eng = testCfg.getEngineName("ebook2", "foo") + assert.Equal(t, eng, "ebook-foo") + + eng = testCfg.getEngineName("ebook2", "app") + assert.Equal(t, eng, "ebook.42") + + eng = testCfg.getEngineName("ebook2", "foo_bar") + assert.Equal(t, eng, "ebook-foo-2") + + eng = testCfg.getEngineName("ebook_bar", "baz") + assert.Equal(t, eng, "ebook.42") + + eng = testCfg.getEngineName("dev_user2", "foo_bar") + assert.Equal(t, eng, "dosa1") + + eng = testCfg.getEngineName("a", "d.k") + assert.Equal(t, eng, "dosa2") +} + +var prodConfig = ` +routers: + - production: + "dosa3test*": dosa_prod_a + "eternal2a": dosa_prod_a + "other_client*": dosa_prod_a + "*": dosa + - service: + "*": cl_service + - service_tier1: + "*": cl_service_tier1 + - dosa_test: + "*": dosa_staging + - default: + "*": dosa_dev +` + +func TestProdConfig(t *testing.T) { + // Make sure the production config works as expected. + + prodCfg := &Config{} + err := yaml.Unmarshal([]byte(prodConfig), prodCfg) + assert.NoError(t, err) + + rs := routers{ + buildRule("dosa_test", "*", "dosa_staging"), + buildRule("production", "dosa3test*", "dosa_prod_a"), + buildRule("production", "eternal2a", "dosa_prod_a"), + buildRule("production", "other_client*", "dosa_prod_a"), + buildRule("production", "*", "dosa"), + buildRule("service", "*", "cl_service"), + buildRule("service_tier1", "*", "cl_service_tier1"), + buildRule("default", "*", "dosa_dev"), + } + assert.Equal(t, prodCfg.Routers, rs) - cfg = testCfg.FindRouter("production", "serviceA") - assert.Equal(t, cfg, buildRouter("production", "serviceA", "cassandra")) + assert.Equal(t, prodCfg.getEngineName("production", "other_client"), "dosa_prod_a") + assert.Equal(t, prodCfg.getEngineName("production", "other_client_b"), "dosa_prod_a") + assert.Equal(t, prodCfg.getEngineName("production", "dosa3test.bar"), "dosa_prod_a") + assert.Equal(t, prodCfg.getEngineName("production", "eternal2a"), "dosa_prod_a") + assert.Equal(t, prodCfg.getEngineName("production", "prog1"), "dosa") + assert.Equal(t, prodCfg.getEngineName("service", "all_users"), "cl_service") + assert.Equal(t, prodCfg.getEngineName("service_tier1", "all_users"), "cl_service_tier1") + assert.Equal(t, prodCfg.getEngineName("dosa_test", "indexer"), "dosa_staging") + assert.Equal(t, prodCfg.getEngineName("myDevScope", "myService"), "dosa_dev") +} - cfg = testCfg.FindRouter("ebook", "apple.k") - assert.Equal(t, cfg, buildRouter("ebook", "apple.*", "ebook")) +var stConfig = ` +routers: + - production: + "testsvc2a*": svc_prod_a + "turtle2a": svc_prod_a + "anaconda*": svc_prod_a + "krait_*": krait_dev + "*": dosa + - kestrel: + "*": kestrel + - kestrel_level1: + "*": kestrel_level1 + - svc_test: + "*": svc_staging + - krait_*: + "*": krait_prod + - default: + "krait_*": krait_dev + "*": svc_dev +` - cfg = testCfg.FindRouter("ebook", "d.k") - assert.Equal(t, cfg, buildRouter("ebook", "*", "ebook")) +func TestOtherRouting(t *testing.T) { + tsCfg := &Config{} + err := yaml.Unmarshal([]byte(stConfig), tsCfg) + assert.NoError(t, err) + + rs := routers{ + buildRule("kestrel", "*", "kestrel"), + buildRule("kestrel_level1", "*", "kestrel_level1"), + buildRule("krait_*", "*", "krait_prod"), + buildRule("production", "anaconda*", "svc_prod_a"), + buildRule("production", "krait_*", "krait_dev"), + buildRule("production", "testsvc2a*", "svc_prod_a"), + buildRule("production", "turtle2a", "svc_prod_a"), + buildRule("production", "*", "dosa"), + buildRule("svc_test", "*", "svc_staging"), + buildRule("default", "krait_*", "krait_dev"), + buildRule("default", "*", "svc_dev"), + } + assert.Equal(t, tsCfg.Routers, rs) - cfg = testCfg.FindRouter("a", "d.k") - assert.Equal(t, cfg, buildRouter("default", "default", "dosa")) + assert.Equal(t, tsCfg.getEngineName("production", "krait_trips"), "krait_dev") + assert.Equal(t, tsCfg.getEngineName("kestrel", "svc"), "kestrel") + assert.Equal(t, tsCfg.getEngineName("kestrel_level1", "helper"), "kestrel_level1") + assert.Equal(t, tsCfg.getEngineName("krait_store", "krait_trips"), "krait_prod") + assert.Equal(t, tsCfg.getEngineName("krait_meta", "krait_accts"), "krait_prod") + assert.Equal(t, tsCfg.getEngineName("krajt_store", "other_trips"), "svc_dev") + assert.Equal(t, tsCfg.getEngineName("krajt_meta", "other_accts"), "svc_dev") + assert.Equal(t, tsCfg.getEngineName("my_dev", "krait_trips"), "krait_dev") + assert.Equal(t, tsCfg.getEngineName("team4", "krait_accts"), "krait_dev") + assert.Equal(t, tsCfg.getEngineName("my_dev", "service"), "svc_dev") + assert.Equal(t, tsCfg.getEngineName("team4", "users"), "svc_dev") } diff --git a/connectors/routing/connector.go b/connectors/routing/connector.go index 8aeeaecd..d45f8e1d 100644 --- a/connectors/routing/connector.go +++ b/connectors/routing/connector.go @@ -28,12 +28,10 @@ import ( "github.com/uber-go/dosa" ) -// Connector holds a slice of configured connectors to route to +// The routing connector maps a (scope, namePrefix) pair into a storage engine connector. + +// Connector is a routing connector. type Connector struct { - // config connector slice is sorted in a manner: - // for the value of Config name prefix, strict string without "*" always comes first, - // and then string with "*" suffix (glob match) and pure "*". - // There shouldn't be any scope with a prefix "*" like "*.service.v1" config Config connectors map[string]dosa.Connector } @@ -51,13 +49,12 @@ func (rc *Connector) String() string { return fmt.Sprintf("[Routing %s]", rc.config.String()) } -// get connector by scope an namePrefix +// get connector by scope and namePrefix func (rc *Connector) getConnector(scope, namePrefix string) (dosa.Connector, error) { - router := rc.config.FindRouter(scope, namePrefix) - - c, ok := rc.connectors[router.Connector] + connName := rc.config.getEngineName(scope, namePrefix) + c, ok := rc.connectors[connName] if !ok { - return nil, fmt.Errorf("can't find %q connector", router.Connector) + return nil, fmt.Errorf("can't find %q connector", connName) } return c, nil diff --git a/connectors/routing/connector_test.go b/connectors/routing/connector_test.go index 44f2b04b..01e5f383 100644 --- a/connectors/routing/connector_test.go +++ b/connectors/routing/connector_test.go @@ -38,28 +38,26 @@ const idcount = 10 var ( cfg = Config{ - Routers: Routers{ - buildRouter("production", "map", "memory"), - buildRouter("production", "default", "random"), - buildRouter("ebook", "ebook-store", "memory"), - buildRouter("ebook", "default", "random"), - buildRouter("ebook", "apple.*", "memory"), - buildRouter("ebook", "*", "devnull"), - buildRouter("development", "map", "memory"), - buildRouter("development", "default", "random"), - buildRouter("default", "default", "memory"), + Routers: routers{ + buildRule("ebook", "apple.*", "memory"), + buildRule("ebook", "ebook_store", "memory"), + buildRule("ebook", "*", "devnull"), + buildRule("development", "map", "memory"), + buildRule("development", "default", "random"), + buildRule("production", "map", "memory"), + buildRule("production", "default", "random"), + buildRule("default", "default", "memory"), }, } routersStr = strings.Join([]string{ - "{production.map -> memory}", - "{production.default -> random}", - "{ebook.ebook-store -> memory}", - "{ebook.default -> random}", "{ebook.apple.* -> memory}", + "{ebook.ebook_store -> memory}", "{ebook.* -> devnull}", "{development.map -> memory}", - "{development.default -> random}", - "{default.default -> memory}", + "{development.* -> random}", + "{production.map -> memory}", + "{production.* -> random}", + "{*.* -> memory}", }, ",") testInfo = &dosa.EntityInfo{ Ref: &dosa.SchemaRef{ @@ -191,7 +189,7 @@ func TestGetConnector(t *testing.T) { assert.NotNil(t, conn) // exact match - ei.Ref.NamePrefix = "ebook-store" + ei.Ref.NamePrefix = "ebook_store" conn, err = rc.getConnector(ei.Ref.Scope, ei.Ref.NamePrefix) assert.Nil(t, err) assert.NotNil(t, conn) diff --git a/connectors/routing/routing_config.go b/connectors/routing/routing_config.go index ddb4495b..6a2f8efd 100644 --- a/connectors/routing/routing_config.go +++ b/connectors/routing/routing_config.go @@ -22,61 +22,185 @@ package routing import ( "fmt" + "regexp" "strings" - "github.com/gobwas/glob" "github.com/pkg/errors" + "github.com/uber-go/dosa" ) -// Rule stores routing rule -// to decide which connector the Connector talks to -type Rule struct { - Scope string - NamePrefix string - Connector string - GlobMatch glob.Glob +// rule is an assignment from scope.prefixPattern to a connector name. +type rule struct { + scope string + namePrefix string + connector string + scopePattern *regexp.Regexp + prefixPattern *regexp.Regexp + // The canonical representations of scope and prefix, chosen so that they sort in the required orrder. + canonScope string + canonPfx string } -// NewRule initializes 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 == "" { + return nil, errors.New("could not parse routing rule: scope cannot be empty") + } + if scope == defaultName { + scope = "*" + } if namePrefix == "" { - return nil, errors.New("namePrefix could not be empty, should be defined in yaml file") + return nil, errors.New("could not parse routing rule: namePrefix cannot be empty") + } + if namePrefix == defaultName { + namePrefix = "*" } - if scope == "" { - return nil, errors.New("scope could not be empty, should be defined in yaml file") + scope, canonScope, scopePat, err := parseName(scope, true) + if err != nil { + return nil, errors.Wrap(err, fmt.Sprintf("could not parse routing rule: invalid scope %s", scope)) + } + + namePrefix, canonPrefix, prefixPat, err := parseName(namePrefix, false) + if err != nil { + return nil, errors.Wrap(err, fmt.Sprintf("could not parse routing rule: invalid namePrefix %s", namePrefix)) + } + + return &rule{ + namePrefix: namePrefix, + scope: scope, + connector: connector, + scopePattern: scopePat, + prefixPattern: prefixPat, + canonScope: canonScope, + canonPfx: canonPrefix, + }, nil +} + +// Parse a name (scope or prefix) optionaly with a "*" suffix. Return the underlying name, a canonical representation +// of the name, and if the name is a glob, a regular expression recognizing it. +func parseName(name string, isScope bool) (string, string, *regexp.Regexp, error) { + var regex *regexp.Regexp + isGlob := false + + if strings.HasSuffix(name, "*") { + name = strings.TrimSuffix(name, "*") + isGlob = true + } + // Sanity check the name. No need to check "", that's the pattern "*". + if name != "" { + var err error + if isScope { + _, err = dosa.NormalizeName(name) + } else { + _, err = dosa.NormalizeNamePrefix(name) + } + if err != nil { + return "", "", nil, err + } + } + canon, err := canonicalize(name, isGlob, isScope) + if err != nil { + return "", "", nil, err + } + if isGlob { + regex = makePrefixRegexp(name) } + return name, canon, regex, nil +} - if strings.HasPrefix(namePrefix, "*") && len(namePrefix) != 1 { - // we don't support name like '*.service.v1' - return nil, errors.Errorf( - "namePrefix like %v is not supported, cannot put * at the beginning of namePrefix.", namePrefix) +// Scope returns the rule's scope. +func (r *rule) Scope() string { + if r.scopePattern == nil { + return r.scope } + return r.scope + "*" +} - globMatch := glob.MustCompile(namePrefix) +// NamePrefix returns the rule's name-prefix. +func (r *rule) NamePrefix() string { + if r.prefixPattern == nil { + return r.namePrefix + } + return r.namePrefix + "*" +} - return &Rule{NamePrefix: namePrefix, Scope: scope, Connector: connector, GlobMatch: globMatch}, nil +// Destination returns the rule's destination. +func (r *rule) Destination() string { + return r.connector } -// RouteTo implements the method to choose the matched connector -func (r *Rule) RouteTo(scope string, namePrefix string) bool { - // scope should be an exact match - if r.Scope != scope { - return false +func (r *rule) String() string { + var prefixPat, scopePat string + if r.scopePattern != nil { + scopePat = "*" } + if r.prefixPattern != nil { + prefixPat = "*" + } + return fmt.Sprintf("{%s%s.%s%s -> %s}", r.scope, scopePat, r.namePrefix, prefixPat, r.connector) +} - // namePrefix could be glob match, but first check if there's an exact match - if r.NamePrefix == namePrefix { - return true +// canHandle says whether or not this rule can handle the given scope:prefix. +func (r *rule) canHandle(scope string, namePrefix string) bool { + if r.scope == scope { + if r.namePrefix == namePrefix { + // Exact match for both. + return true + } + if r.prefixPattern != nil && r.prefixPattern.MatchString(namePrefix) { + // Exact match for scope, RE match for prefix. + return true + } } - if r.GlobMatch.Match(namePrefix) { - return true + if r.scopePattern != nil && r.scopePattern.MatchString(scope) { + if r.namePrefix == namePrefix { + // RE match for scope, exact match for prefix. + return true + } + if r.prefixPattern != nil && r.prefixPattern.MatchString(namePrefix) { + // RE match for both. + return true + } } return false } -func (r *Rule) String() string { - return fmt.Sprintf("{%s.%s -> %s}", r.Scope, r.NamePrefix, r.Connector) +// Make a regexp from the "glob" pattern used in routing rules. +func makePrefixRegexp(pat string) *regexp.Regexp { + if pat == "" { + return regexp.MustCompile(".") + } + // Quote dots and add begin-of-line. The string is known to be a valid scope/prefix name i.e. + // alphanumeric possibly with "." characters, no other special chars need to be quoted. + return regexp.MustCompile("^" + strings.Join(strings.Split(pat, "."), "\\.")) +} + +// This character is used in canonical representations to make sure that "foo*" will sort after "foot". +const lastASCIIChar = "~" + +// Convert a string to canonical form that allows lexicographic sorting according to the routing rule semantics. +// The returned value is not necessarily a valid regexp. +func canonicalize(s string, isPattern bool, isScope bool) (string, error) { + if s == "" && isPattern { + return lastASCIIChar, nil + } + + var err error + if isScope { + s, err = dosa.NormalizeName(s) + } else { + s, err = dosa.NormalizeNamePrefix(s) + } + if err != nil { + return "", err + } + if !isPattern { + return s, nil + } + return s + lastASCIIChar, nil } diff --git a/connectors/routing/routing_config_test.go b/connectors/routing/routing_config_test.go index 98f11e91..65d561e3 100644 --- a/connectors/routing/routing_config_test.go +++ b/connectors/routing/routing_config_test.go @@ -21,6 +21,7 @@ package routing import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -29,23 +30,23 @@ import ( func TestNewRoutingConfig(t *testing.T) { rConfig, err := NewRule("production", "test", "memory") assert.Nil(t, err) - assert.Equal(t, rConfig.NamePrefix, "test") + assert.Equal(t, rConfig.namePrefix, "test") } func TestNewRoutingConfigWithStarPrefix(t *testing.T) { rConfig, err := NewRule("production", "*.v1", "memory") assert.Nil(t, rConfig) - assert.Contains(t, err.Error(), "is not supported") + assert.Contains(t, err.Error(), "invalid") } func TestTestNewRoutingConfigError(t *testing.T) { rConfig, err := NewRule("production", "", "memory") assert.Nil(t, rConfig) - assert.Contains(t, err.Error(), "could not be empty") + assert.Contains(t, err.Error(), "cannot be empty") rConfig, err = NewRule("", "test", "memory") assert.Nil(t, rConfig) - assert.Contains(t, err.Error(), "scope could not be empty") + assert.Contains(t, err.Error(), "scope cannot be empty") } func TestString(t *testing.T) { @@ -53,3 +54,87 @@ func TestString(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "{production.test -> memory}", r.String()) } + +func TestCanonicalize(t *testing.T) { + cases := []struct { + pat string + cpat string + isScope bool + err bool + }{ + { + pat: "foobar", + cpat: "foobar", + }, + { + pat: "fooBar", + cpat: "foobar", + }, + { + pat: "foobar", + cpat: "foobar", + isScope: true, + }, + { + pat: "fooBar", + cpat: "foobar", + isScope: true, + }, + { + pat: "9foobar", + err: true, + }, + { + pat: "9fooBar", + err: true, + }, + { + pat: "9foobar", + isScope: true, + err: true, + }, + { + pat: "9fooBar", + isScope: true, + err: true, + }, + { + pat: "foo.Bar", + cpat: "foo.bar", + }, + { + pat: "foo.bar", + isScope: true, + err: true, + }, + { + pat: "foo!bar", + err: true, + }, + { + pat: "foo!bar", + isScope: true, + err: true, + }, + } + for _, tc := range cases { + // literal string + cpLit, e1 := canonicalize(tc.pat, false, tc.isScope) + if tc.err { + assert.Error(t, e1) + } else { + assert.NoError(t, e1) + } + // * at the end + cpGlob, e2 := canonicalize(tc.pat, true, tc.isScope) + if tc.err { + assert.Error(t, e2) + } else { + assert.NoError(t, e2) + assert.Equal(t, tc.cpat, cpLit) + // Check that globs sort after literals and are prefixes + assert.True(t, cpLit < cpGlob) + assert.True(t, strings.HasPrefix(cpGlob, cpLit)) + } + } +} diff --git a/entity_parser_key_parser_test.go b/entity_parser_key_parser_test.go index fec35cb1..e6aab837 100644 --- a/entity_parser_key_parser_test.go +++ b/entity_parser_key_parser_test.go @@ -309,7 +309,7 @@ func TestNameTag(t *testing.T) { }, { Tag: "name=ji^&*", - Error: errors.New("failed to normalize to a valid name for ji^&*"), + Error: errors.New("invalid"), Name: "", FullName: "", }, @@ -680,7 +680,7 @@ func TestEntityParse(t *testing.T) { PartitionKeys: []string{"ok"}, ClusteringKeys: nil, }, - Error: errors.New("cannot be empty"), + Error: errors.New("invalid"), ETL: EtlOff, }, { diff --git a/entity_parser_test.go b/entity_parser_test.go index ab494e41..2c27c775 100644 --- a/entity_parser_test.go +++ b/entity_parser_test.go @@ -565,7 +565,7 @@ func TestInvalidStructName(t *testing.T) { table, err := TableFromInstance(&ăBădNăme{}) assert.Nil(t, table) assert.Error(t, err) - assert.Contains(t, err.Error(), "ormalize") + assert.Contains(t, err.Error(), "invalid") } func TestInvalidFieldInTag(t *testing.T) { type HasInvalidCharInTag struct { diff --git a/entity_test.go b/entity_test.go index afc6da01..6c512b5c 100644 --- a/entity_test.go +++ b/entity_test.go @@ -82,7 +82,7 @@ func TestEntityDefinitionEnsureValid(t *testing.T) { { e: invalidName, valid: false, - msg: "name must contain only", + msg: "invalid", }, { e: nilColumn, @@ -229,7 +229,7 @@ func TestEntityDefinitionEnsureValidForIndex(t *testing.T) { { e: invalidName, valid: false, - msg: "name must contain only", + msg: "invalid", }, { e: nilPK, diff --git a/names.go b/names.go index f8b2d678..5f6d50ab 100644 --- a/names.go +++ b/names.go @@ -27,66 +27,52 @@ import ( "github.com/pkg/errors" ) +// This module does some sanity checking of names used in DOSA. A name must have a leading letter, +// followed by a string of letters and digits. A name can have up to 32 chars. +// +// A special kind of name is the name-prefix. A name-prefix has the same restrictions as a name, +// except that a name-prefix can also contain the "." character. + const ( maxNameLen = 32 ) var ( namePrefixRegex = regexp.MustCompile("^[a-z_][a-z0-9_.]{0,31}$") + nameRegex = regexp.MustCompile("^[a-z_][a-z0-9_]{0,31}$") ) -// IsValidNamePrefix checks if a name prefix (a namespace prefixed to all DOSA entity type names) -// conforms the following rules: -// 1. name prefix starts with [a-z_] -// 2. the rest of name prefix can contain only [a-z0-9_.] -// 3. the length of name prefix must be greater than 0 and less than or equal to 32 +// IsValidNamePrefix checks if a name prefix is valid. func IsValidNamePrefix(namePrefix string) error { normalized := strings.ToLower(strings.TrimSpace(namePrefix)) if !namePrefixRegex.MatchString(normalized) { - return errors.Errorf("Name Prefix %s is invalid. It was normalized to "+ - "%s which means it does not pass the regex %s", - namePrefix, - normalized, - namePrefixRegex.String(), - ) + return errors.Errorf("invalid name-prefix '%s'", namePrefix) } return nil } -func isInvalidFirstRune(r rune) bool { - return !((r >= 'a' && r <= 'z') || r == '_') -} - -func isInvalidOtherRune(r rune) bool { - return !(r >= '0' && r <= '9') && isInvalidFirstRune(r) -} - -// IsValidName checks if a name conforms the following rules: -// 1. name starts with [a-z_] -// 2. the rest of name can contain only [a-z0-9_] -// 3. the length of name must be greater than 0 and less than or equal to maxNameLen +// IsValidName checks if a string corresponds to DOSA naming rules. func IsValidName(name string) error { - if len(name) == 0 { - return errors.Errorf("cannot be empty") - } - if len(name) > maxNameLen { - return errors.Errorf("too long: %v has length %d, max allowed is %d", name, len(name), maxNameLen) - } - if strings.IndexFunc(name[:1], isInvalidFirstRune) != -1 { - return errors.Errorf("name must start with [a-z_]. Actual='%s'", name) - } - if strings.IndexFunc(name[1:], isInvalidOtherRune) != -1 { - return errors.Errorf("name must contain only [a-z0-9_], Actual='%s'", name) + if !nameRegex.MatchString(name) { + return errors.Errorf("invalid name '%s'", name) } return nil } -// NormalizeName normalizes names to a canonical representation by lowercase everything. -// It returns error if the resultant canonical name is invalid. +// NormalizeName normalizes a name to a canonical representation. func NormalizeName(name string) (string, error) { lowercaseName := strings.ToLower(strings.TrimSpace(name)) if err := IsValidName(lowercaseName); err != nil { - return "", errors.Wrapf(err, "failed to normalize to a valid name for %s", name) + return "", err + } + return lowercaseName, nil +} + +// NormalizeNamePrefix normalizes a name-prefix to a canonical representation. +func NormalizeNamePrefix(name string) (string, error) { + lowercaseName := strings.ToLower(strings.TrimSpace(name)) + if err := IsValidNamePrefix(lowercaseName); err != nil { + return "", err } return lowercaseName, nil } diff --git a/names_test.go b/names_test.go index 966858d7..9423e672 100644 --- a/names_test.go +++ b/names_test.go @@ -110,10 +110,12 @@ func TestNormalizeName(t *testing.T) { allowed: true, expected: "_alreadynormalized9", }, + // Invalid { - arg: "an apple", - allowed: false, - expected: "", + arg: "an apple", + }, + { + arg: "9Monkeys", }, } @@ -148,3 +150,58 @@ func TestIsValidNamePrefix(t *testing.T) { err = IsValidNamePrefix("this.prefix.has.more.than.thrity.two.characters.in.it") assert.Error(t, err) } + +func TestNormalizeNamePrefix(t *testing.T) { + cases := []struct { + arg string + bogus bool + expected string + }{ + { + arg: "lOwerEVeryTHiNG", + expected: "lowereverything", + }, + { + arg: "_MyName", + expected: "_myname", + }, + { + arg: "_alreadynormalized9", + expected: "_alreadynormalized9", + }, + { + arg: "_My.Name", + expected: "_my.name", + }, + { + arg: "_already.normalized.9", + expected: "_already.normalized.9", + }, + { + arg: "an apple", + bogus: true, + }, + { + arg: "apple!", + bogus: true, + }, + { + arg: "a.b.c.d!", + bogus: true, + }, + { + arg: "9Monkeys", + bogus: true, + }, + } + + for _, tc := range cases { + name, err := NormalizeNamePrefix(tc.arg) + if tc.bogus { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expected, name) + } + } +} diff --git a/version.go b/version.go index 5198063c..fb319b49 100644 --- a/version.go +++ b/version.go @@ -21,4 +21,4 @@ package dosa // VERSION indicates the dosa client version -const VERSION = "3.4.6" +const VERSION = "3.4.7"