From d6750bbce113bc7fdde3ba9e895910a65cbdc530 Mon Sep 17 00:00:00 2001 From: tiffanycheng <8397787+tiffanycheng@users.noreply.github.com> Date: Fri, 8 Dec 2017 13:23:15 -0600 Subject: [PATCH 1/5] Do not read from fallback for not found errors (#256) --- connectors/cache/fallback.go | 8 ++++++ connectors/cache/fallback_test.go | 42 +++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/connectors/cache/fallback.go b/connectors/cache/fallback.go index 106fff93..54658456 100644 --- a/connectors/cache/fallback.go +++ b/connectors/cache/fallback.go @@ -135,6 +135,10 @@ func (c *Connector) Read(ctx context.Context, ei *dosa.EntityInfo, keys map[stri return source, sourceErr } + if dosa.ErrorIsNotFound(sourceErr) { + return source, sourceErr + } + // if source of truth fails, try the fallback. If the fallback fails, // return the original error value, err := c.getValueFromFallback(ctx, adaptedEi, cacheKey) @@ -188,6 +192,10 @@ func (c *Connector) Range(ctx context.Context, ei *dosa.EntityInfo, columnCondit return sourceRows, sourceToken, sourceErr } + if dosa.ErrorIsNotFound(sourceErr) { + return sourceRows, sourceToken, sourceErr + } + value, err := c.getValueFromFallback(ctx, adaptedEi, cacheKey) c.logFallback("RANGE", ei.Def.Name, err) if err != nil { diff --git a/connectors/cache/fallback_test.go b/connectors/cache/fallback_test.go index fc678168..6ca1b862 100644 --- a/connectors/cache/fallback_test.go +++ b/connectors/cache/fallback_test.go @@ -202,6 +202,7 @@ func TestReadCases(t *testing.T) { createReadSuccessTestCase(), createReadUncachedEntityTestCase(), createReadFailTestCase(), + createReadNotFoundTestCase(), createReadEncodeErrorTestCase(), createReadDecodeErrorTestCase(), createReadFallbackFailTestCase(), @@ -265,6 +266,22 @@ func createReadFailTestCase() testCase { } } +func createReadNotFoundTestCase() testCase { + originResponse := map[string]dosa.FieldValue{"a": "b"} + originErr := &dosa.ErrNotFound{} + return testCase{ + encoder: NewJSONEncoder(), + cachedEntities: cacheableEntities, + originRead: &expectArgs{ + err: originErr, + resp: originResponse, + }, + expectedResp: originResponse, + expectedErr: originErr, + description: "Test that when read origin has err not found, do not read from fallback, return origin response and error", + } +} + func createReadEncodeErrorTestCase() testCase { originResponse := map[string]dosa.FieldValue{"a": "b"} @@ -425,6 +442,7 @@ func TestRangeCases(t *testing.T) { createRangeSuccessTestCase(), createRangeUncachedEntityTestCase(), createRangeFailTestCase(), + createRangeNotFoundTestCase(), createRangeEncodeErrorTestCase(), createRangeDecodeErrorTestCase(), createRangeFallbackFailTestCase(), @@ -510,6 +528,30 @@ func createRangeFailTestCase() testCase { } } +func createRangeNotFoundTestCase() testCase { + conditions := map[string][]*dosa.Condition{"column": {{Op: dosa.GtOrEq, Value: "columnVal"}}} + rangeResponse := []map[string]dosa.FieldValue{{"a": "b"}} + rangeTok := "nextToken" + rangeErr := &dosa.ErrNotFound{} + + return testCase{ + encoder: &BadEncoder{}, + cachedEntities: cacheableEntities, + originRange: &rangeArgs{ + columnConditions: conditions, + token: "token", + limit: 2, + resp: rangeResponse, + nextToken: rangeTok, + err: rangeErr, + }, + expectedErr: rangeErr, + expectedManyResp: rangeResponse, + expectedTok: rangeTok, + description: "Test that when origin has err not found, do not read from fallback, return origin response and error", + } +} + func createRangeEncodeErrorTestCase() testCase { rangeResponse := []map[string]dosa.FieldValue{{"a": "b"}} rangeTok := "nextToken" From 6ff0a29f7c699fa5b0c8470687460c82a73a5e84 Mon Sep 17 00:00:00 2001 From: junchaowu Date: Wed, 13 Dec 2017 13:50:46 -0800 Subject: [PATCH 2/5] detect whether error is thrift error or network error in yarpc connector (#258) --- connectors/cache/fallback_test.go | 2 +- connectors/yarpc/yarpc.go | 97 +++++++++++++++++++++++++------ mocks/client.go | 3 +- mocks/connector.go | 3 +- 4 files changed, 84 insertions(+), 21 deletions(-) diff --git a/connectors/cache/fallback_test.go b/connectors/cache/fallback_test.go index 6ca1b862..f9a42b85 100644 --- a/connectors/cache/fallback_test.go +++ b/connectors/cache/fallback_test.go @@ -273,7 +273,7 @@ func createReadNotFoundTestCase() testCase { encoder: NewJSONEncoder(), cachedEntities: cacheableEntities, originRead: &expectArgs{ - err: originErr, + err: originErr, resp: originResponse, }, expectedResp: originResponse, diff --git a/connectors/yarpc/yarpc.go b/connectors/yarpc/yarpc.go index 0bcbbffc..065951c5 100644 --- a/connectors/yarpc/yarpc.go +++ b/connectors/yarpc/yarpc.go @@ -228,8 +228,12 @@ func (c *Connector) CreateIfNotExists(ctx context.Context, ei *dosa.EntityInfo, return errors.Wrap(&dosa.ErrAlreadyExists{}, "failed to create") } } + + if !dosarpc.Dosa_CreateIfNotExists_Helper.IsException(err) { + return errors.Wrap(err, "failed to CreateIfNotExists due to network issue") + } } - return errors.Wrap(err, "failed to create") + return errors.Wrap(err, "failed to CreateIfNotExists") } // Upsert inserts or updates your data @@ -242,7 +246,14 @@ func (c *Connector) Upsert(ctx context.Context, ei *dosa.EntityInfo, values map[ Ref: entityInfoToSchemaRef(ei), EntityValues: ev, } - return c.Client.Upsert(ctx, &upsertRequest, VersionHeader()) + + err = c.Client.Upsert(ctx, &upsertRequest, VersionHeader()) + + if !dosarpc.Dosa_Upsert_Helper.IsException(err) { + return errors.Wrap(err, "failed to Upsert due to network issue") + } + + return errors.Wrap(err, "failed to Upsert") } // Read reads a single entity @@ -284,7 +295,12 @@ func (c *Connector) Read(ctx context.Context, ei *dosa.EntityInfo, keys map[stri return nil, errors.Wrap(&dosa.ErrNotFound{}, "Read failed: not found") } } - return nil, errors.Wrap(err, "Read failed") + + if !dosarpc.Dosa_Read_Helper.IsException(err) { + return nil, errors.Wrap(err, "failed to Read due to network issue") + } + + return nil, errors.Wrap(err, "failed to Read") } // no error, so for each column, transform it into the map of (col->value) items @@ -323,7 +339,11 @@ func (c *Connector) MultiRead(ctx context.Context, ei *dosa.EntityInfo, keys []m response, err := c.Client.MultiRead(ctx, request, VersionHeader()) if err != nil { - return nil, errors.Wrap(err, "MultiRead failed") + if !dosarpc.Dosa_MultiRead_Helper.IsException(err) { + return nil, errors.Wrap(err, "failed to MultiRead due to network issue") + } + + return nil, errors.Wrap(err, "failed to MultiRead") } rpcResults := response.Results @@ -376,7 +396,11 @@ func (c *Connector) Remove(ctx context.Context, ei *dosa.EntityInfo, keys map[st err := c.Client.Remove(ctx, removeRequest, VersionHeader()) if err != nil { - return errors.Wrap(err, "Remove failed") + if !dosarpc.Dosa_Remove_Helper.IsException(err) { + return errors.Wrap(err, "failed to Remove due to network issue") + } + + return errors.Wrap(err, "failed to Remove") } return nil } @@ -394,7 +418,10 @@ func (c *Connector) RemoveRange(ctx context.Context, ei *dosa.EntityInfo, column } if err := c.Client.RemoveRange(ctx, request, VersionHeader()); err != nil { - return errors.Wrap(err, "RemoveRange failed") + if !dosarpc.Dosa_RemoveRange_Helper.IsException(err) { + return errors.Wrap(err, "failed to RemoveRange due to network issue") + } + return errors.Wrap(err, "failed to RemoveRange") } return nil } @@ -410,7 +437,7 @@ func (c *Connector) Range(ctx context.Context, ei *dosa.EntityInfo, columnCondit rpcMinimumFields := makeRPCminimumFields(minimumFields) rpcConditions, err := createRPCConditions(columnConditions) if err != nil { - return nil, "", errors.Wrap(err, "Range failed: invalid column conditions") + return nil, "", errors.Wrap(err, "failed to Range: invalid column conditions") } rangeRequest := dosarpc.RangeRequest{ Ref: entityInfoToSchemaRef(ei), @@ -421,7 +448,11 @@ func (c *Connector) Range(ctx context.Context, ei *dosa.EntityInfo, columnCondit } response, err := c.Client.Range(ctx, &rangeRequest, VersionHeader()) if err != nil { - return nil, "", errors.Wrap(err, "Range failed") + if !dosarpc.Dosa_Range_Helper.IsException(err) { + return nil, "", errors.Wrap(err, "failed to Range due to network issue") + } + + return nil, "", errors.Wrap(err, "failed to Range") } results := []map[string]dosa.FieldValue{} for _, entity := range response.Entities { @@ -466,7 +497,11 @@ func (c *Connector) Scan(ctx context.Context, ei *dosa.EntityInfo, minimumFields } response, err := c.Client.Scan(ctx, &scanRequest, VersionHeader()) if err != nil { - return nil, "", errors.Wrap(err, "Scan failed") + if !dosarpc.Dosa_Scan_Helper.IsException(err) { + return nil, "", errors.Wrap(err, "failed to Scan due to network issue") + } + + return nil, "", errors.Wrap(err, "failed to Scan") } results := []map[string]dosa.FieldValue{} for _, entity := range response.Entities { @@ -487,7 +522,11 @@ func (c *Connector) CheckSchema(ctx context.Context, scope, namePrefix string, e } response, err := c.Client.CheckSchema(ctx, &csr, VersionHeader()) if err != nil { - return dosa.InvalidVersion, wrapError(err, "CheckSchema failed", scope, c.Config.ServiceName) + if !dosarpc.Dosa_CheckSchema_Helper.IsException(err) { + return dosa.InvalidVersion, errors.Wrap(err, "failed to CheckSchema due to network issue") + } + + return dosa.InvalidVersion, wrapError(err, "failed to CheckSchema", scope, c.Config.ServiceName) } return *response.Version, nil @@ -506,7 +545,10 @@ func (c *Connector) CanUpsertSchema(ctx context.Context, scope, namePrefix strin } response, err := c.Client.CanUpsertSchema(ctx, &csr, VersionHeader()) if err != nil { - return dosa.InvalidVersion, wrapError(err, "Check schema compatibility failed", scope, c.Config.ServiceName) + if !dosarpc.Dosa_CanUpsertSchema_Helper.IsException(err) { + return dosa.InvalidVersion, errors.Wrap(err, "failed to CanUpsertSchema due to network issue") + } + return dosa.InvalidVersion, wrapError(err, "failed to CanUpsertSchema", scope, c.Config.ServiceName) } return *response.Version, nil @@ -523,7 +565,10 @@ func (c *Connector) UpsertSchema(ctx context.Context, scope, namePrefix string, response, err := c.Client.UpsertSchema(ctx, request, VersionHeader()) if err != nil { - return nil, wrapError(err, "UpsertSchema failed", scope, c.Config.ServiceName) + if !dosarpc.Dosa_UpsertSchema_Helper.IsException(err) { + return nil, errors.Wrap(err, "failed to UpsertSchema due to network issue") + } + return nil, wrapError(err, "failed to UpsertSchema", scope, c.Config.ServiceName) } status := "" @@ -532,7 +577,7 @@ func (c *Connector) UpsertSchema(ctx context.Context, scope, namePrefix string, } if response.Version == nil { - return nil, errors.New("UpsertSchema failed: server returns version nil") + return nil, errors.New("failed to UpsertSchema: server returns version nil") } return &dosa.SchemaStatus{ @@ -547,7 +592,11 @@ func (c *Connector) CheckSchemaStatus(ctx context.Context, scope, namePrefix str response, err := c.Client.CheckSchemaStatus(ctx, &request, VersionHeader()) if err != nil { - return nil, wrapError(err, "ChecksShemaStatus failed", scope, c.Config.ServiceName) + if !dosarpc.Dosa_CheckSchemaStatus_Helper.IsException(err) { + return nil, errors.Wrap(err, "failed to CheckSchemaStatus due to network issue") + } + + return nil, wrapError(err, "failed to CheckSchemaStatus", scope, c.Config.ServiceName) } status := "" @@ -556,7 +605,7 @@ func (c *Connector) CheckSchemaStatus(ctx context.Context, scope, namePrefix str } if response.Version == nil { - return nil, errors.New("ChecksShemaStatus failed: server returns version nil") + return nil, errors.New("failed to ChecksShemaStatus: server returns version nil") } return &dosa.SchemaStatus{ @@ -572,7 +621,11 @@ func (c *Connector) CreateScope(ctx context.Context, scope string) error { } if err := c.Client.CreateScope(ctx, request, VersionHeader()); err != nil { - return errors.Wrap(err, "CreateScope failed") + if !dosarpc.Dosa_CreateScope_Helper.IsException(err) { + return errors.Wrap(err, "failed to CreateScope due to network issue") + } + + return errors.Wrap(err, "failed to CreateScope") } return nil @@ -585,7 +638,11 @@ func (c *Connector) TruncateScope(ctx context.Context, scope string) error { } if err := c.Client.TruncateScope(ctx, request, VersionHeader()); err != nil { - return errors.Wrap(err, "TruncateScope failed") + if !dosarpc.Dosa_TruncateScope_Helper.IsException(err) { + return errors.Wrap(err, "failed to TruncateScope due to network issue") + } + + return errors.Wrap(err, "failed to TruncateScope") } return nil @@ -598,7 +655,11 @@ func (c *Connector) DropScope(ctx context.Context, scope string) error { } if err := c.Client.DropScope(ctx, request, VersionHeader()); err != nil { - return errors.Wrap(err, "DropScope failed") + if !dosarpc.Dosa_DropScope_Helper.IsException(err) { + return errors.Wrap(err, "failed to DropScope due to network issue") + } + + return errors.Wrap(err, "failed to DropScope") } return nil diff --git a/mocks/client.go b/mocks/client.go index 3bd25f2e..55e3f2f5 100644 --- a/mocks/client.go +++ b/mocks/client.go @@ -26,9 +26,10 @@ package mocks import ( context "context" + reflect "reflect" + gomock "github.com/golang/mock/gomock" dosa "github.com/uber-go/dosa" - reflect "reflect" ) // MockClient is a mock of Client interface diff --git a/mocks/connector.go b/mocks/connector.go index b873228e..f1ee7e00 100644 --- a/mocks/connector.go +++ b/mocks/connector.go @@ -26,9 +26,10 @@ package mocks import ( context "context" + reflect "reflect" + gomock "github.com/golang/mock/gomock" dosa "github.com/uber-go/dosa" - reflect "reflect" ) // MockConnector is a mock of Connector interface From 42c9c5c73382c81cba23e017f3113af8e327f245 Mon Sep 17 00:00:00 2001 From: junchaowu Date: Wed, 13 Dec 2017 14:49:49 -0800 Subject: [PATCH 3/5] return the actual error from the routing connector (#259) --- connectors/routing/connector.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/connectors/routing/connector.go b/connectors/routing/connector.go index c8a3fc04..6f09b34c 100644 --- a/connectors/routing/connector.go +++ b/connectors/routing/connector.go @@ -179,7 +179,7 @@ func (rc *Connector) Scan(ctx context.Context, ei *dosa.EntityInfo, minimumField func (rc *Connector) CheckSchema(ctx context.Context, scope, namePrefix string, ed []*dosa.EntityDefinition) (int32, error) { connector, err := rc.getConnector(scope, namePrefix, "CheckSchema") if err != nil { - return dosa.InvalidVersion, base.ErrNoMoreConnector{} + return dosa.InvalidVersion, err } return connector.CheckSchema(ctx, scope, namePrefix, ed) } @@ -188,7 +188,7 @@ func (rc *Connector) CheckSchema(ctx context.Context, scope, namePrefix string, func (rc *Connector) UpsertSchema(ctx context.Context, scope, namePrefix string, ed []*dosa.EntityDefinition) (*dosa.SchemaStatus, error) { connector, err := rc.getConnector(scope, namePrefix, "UpsertSchema") if err != nil { - return nil, base.ErrNoMoreConnector{} + return nil, err } return connector.UpsertSchema(ctx, scope, namePrefix, ed) } @@ -197,7 +197,7 @@ func (rc *Connector) UpsertSchema(ctx context.Context, scope, namePrefix string, func (rc *Connector) CheckSchemaStatus(ctx context.Context, scope string, namePrefix string, version int32) (*dosa.SchemaStatus, error) { connector, err := rc.getConnector(scope, namePrefix, "CheckSchemaStatus") if err != nil { - return nil, base.ErrNoMoreConnector{} + return nil, err } return connector.CheckSchemaStatus(ctx, scope, namePrefix, version) } @@ -207,7 +207,7 @@ func (rc *Connector) CreateScope(ctx context.Context, scope string) error { // will fall to default connector connector, err := rc.getConnector(scope, "", "CreateScope") if err != nil { - return base.ErrNoMoreConnector{} + return err } return connector.CreateScope(ctx, scope) } @@ -217,7 +217,7 @@ func (rc *Connector) TruncateScope(ctx context.Context, scope string) error { // will fall to default connector connector, err := rc.getConnector(scope, "", "TruncateScope") if err != nil { - return base.ErrNoMoreConnector{} + return err } return connector.TruncateScope(ctx, scope) } @@ -227,7 +227,7 @@ func (rc *Connector) DropScope(ctx context.Context, scope string) error { // will fall to default connector connector, err := rc.getConnector(scope, "", "DropScope") if err != nil { - return base.ErrNoMoreConnector{} + return err } return connector.DropScope(ctx, scope) } @@ -237,7 +237,7 @@ func (rc *Connector) ScopeExists(ctx context.Context, scope string) (bool, error // will fall to default connector connector, err := rc.getConnector(scope, "", "ScopeExists") if err != nil { - return false, base.ErrNoMoreConnector{} + return false, err } return connector.ScopeExists(ctx, scope) } From 39532e90c6d13e6a1fbf839285e9d43a051b52ae Mon Sep 17 00:00:00 2001 From: Shamim Mohamed Date: Thu, 14 Dec 2017 14:08:27 -0800 Subject: [PATCH 4/5] Better error message for malformed entity tag --- entity_parser.go | 24 ++++++++++++++++++++++++ entity_parser_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/entity_parser.go b/entity_parser.go index 975ac3db..e32bdd50 100644 --- a/entity_parser.go +++ b/entity_parser.go @@ -100,6 +100,10 @@ func parsePartitionKey(pkStr string) []string { // parsePrimaryKey func parses the primary key of DOSA object func parsePrimaryKey(tableName, pkStr string) (*PrimaryKey, error) { + // parens must be matched + if !parensBalanced(pkStr) { + return nil, fmt.Errorf("unmatched parentheses: %q", pkStr) + } // filter out "trailing comma and space" pkStr = strings.TrimRight(pkStr, ", ") pkStr = strings.TrimSpace(pkStr) @@ -384,6 +388,26 @@ func parseField(typ Type, isPointer bool, name string, tag string) (*ColumnDefin return &ColumnDefinition{Name: name, IsPointer: isPointer, Type: typ}, nil } +func parensBalanced(s string) bool { + // This is effectively pushing left parens on the stack, and popping them when + // a right paren is seen. Since the stack only ever contains the same character, + // we don't actually need the stack -- only its size. + var ssize uint + for i := 0; i < len(s); i++ { + if s[i] == '(' { + ssize += 1 + } else if s[i] == ')' { + if ssize == 0 { + // Extra right paren + return false + } + ssize -= 1 + } + } + // Stack must be empty + return ssize == 0 +} + var ( uuidType = reflect.TypeOf(UUID("")) blobType = reflect.TypeOf([]byte{}) diff --git a/entity_parser_test.go b/entity_parser_test.go index 0effd9af..58a07e22 100644 --- a/entity_parser_test.go +++ b/entity_parser_test.go @@ -492,6 +492,35 @@ func TestInvalidFieldInTag(t *testing.T) { assert.Contains(t, err.Error(), "invalid") } +func TestInvalidSyntaxInTag(t *testing.T) { + type HasInvalidTagSyntax struct { + Entity `dosa:"primaryKey=((Val, Key), TS DESC"` + Val string + Key string + TS time.Time + } + table, err := TableFromInstance(&HasInvalidTagSyntax{}) + assert.Nil(t, table) + assert.Error(t, err) + assert.Contains(t, err.Error(), "unmatched parentheses") +} + +func TestParensBalanced(t *testing.T) { + assert.True(t, parensBalanced("()")) + assert.True(t, parensBalanced("()()")) + assert.True(t, parensBalanced("(()())")) + assert.True(t, parensBalanced("()")) + assert.True(t, parensBalanced("")) + assert.True(t, parensBalanced("()(()(()(()()))())()()")) + + assert.True(t, !parensBalanced("(")) + assert.True(t, !parensBalanced(")")) + assert.True(t, !parensBalanced("(()")) + assert.True(t, !parensBalanced(")(")) + assert.True(t, !parensBalanced("(()))")) + assert.True(t, !parensBalanced("((()())")) +} + /* These tests do not currently pass, but I think they should */ From 483d6c9186431782262466dcec5e0ba0d909d243 Mon Sep 17 00:00:00 2001 From: Shamim Mohamed Date: Thu, 14 Dec 2017 14:39:16 -0800 Subject: [PATCH 5/5] Cleanup, and fix failing test --- entity_parser.go | 4 ++-- entity_parser_test.go | 12 ++++++------ finder_test.go | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/entity_parser.go b/entity_parser.go index e32bdd50..20a712de 100644 --- a/entity_parser.go +++ b/entity_parser.go @@ -395,13 +395,13 @@ func parensBalanced(s string) bool { var ssize uint for i := 0; i < len(s); i++ { if s[i] == '(' { - ssize += 1 + ssize++ } else if s[i] == ')' { if ssize == 0 { // Extra right paren return false } - ssize -= 1 + ssize-- } } // Stack must be empty diff --git a/entity_parser_test.go b/entity_parser_test.go index 58a07e22..9f3f562e 100644 --- a/entity_parser_test.go +++ b/entity_parser_test.go @@ -513,12 +513,12 @@ func TestParensBalanced(t *testing.T) { assert.True(t, parensBalanced("")) assert.True(t, parensBalanced("()(()(()(()()))())()()")) - assert.True(t, !parensBalanced("(")) - assert.True(t, !parensBalanced(")")) - assert.True(t, !parensBalanced("(()")) - assert.True(t, !parensBalanced(")(")) - assert.True(t, !parensBalanced("(()))")) - assert.True(t, !parensBalanced("((()())")) + assert.False(t, parensBalanced("(")) + assert.False(t, parensBalanced(")")) + assert.False(t, parensBalanced("(()")) + assert.False(t, parensBalanced(")(")) + assert.False(t, parensBalanced("(()))")) + assert.False(t, parensBalanced("((()())")) } /* diff --git a/finder_test.go b/finder_test.go index 76a9e42b..60d66fae 100644 --- a/finder_test.go +++ b/finder_test.go @@ -80,7 +80,7 @@ func TestParser(t *testing.T) { assert.Equal(t, len(expectedEntities)+len(entitiesExcludedForTest), len(entities), fmt.Sprintf("%s", entities)) // TODO(jzhan): remove the hard-coded number of errors. - assert.Equal(t, 21, len(errs), fmt.Sprintf("%v", errs)) + assert.Equal(t, 22, len(errs), fmt.Sprintf("%v", errs)) assert.Nil(t, err) for _, entity := range entities {