From f62e996c5ab41173de630046e9c83560017d75ce Mon Sep 17 00:00:00 2001 From: Rick Bijkerk Date: Fri, 13 Sep 2024 16:53:52 +0200 Subject: [PATCH] Feature/fix fragments not working inside operation (#126) Protect didnt correctly capture the operationName from operations which contained fragments inside. This MR fixes that --------- Co-authored-by: Rick Bijkerk --- go.mod | 1 - go.sum | 2 - .../persistedoperations/dir_loader.go | 2 +- .../persistedoperations/gcp_loader.go | 10 +++- .../business/persistedoperations/model.go | 44 +++++++--------- .../persistedoperations/model_test.go | 52 +++++++++---------- .../persisted_operations_test.go | 13 +++-- 7 files changed, 61 insertions(+), 63 deletions(-) diff --git a/go.mod b/go.mod index 294d678..018f473 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,6 @@ go 1.23.0 require ( cloud.google.com/go/storage v1.43.0 - github.com/ardanlabs/conf/v3 v3.1.8 github.com/jedib0t/go-pretty/v6 v6.5.9 github.com/prometheus/client_golang v1.20.3 github.com/stretchr/testify v1.9.0 diff --git a/go.sum b/go.sum index b6246c5..ffd2366 100644 --- a/go.sum +++ b/go.sum @@ -20,8 +20,6 @@ github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 h1:bvNMNQO63//z+xNg github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8= github.com/arbovm/levenshtein v0.0.0-20160628152529-48b4e1c0c4d0 h1:jfIu9sQUG6Ig+0+Ap1h4unLjW6YQJpKZVmUzxsD4E/Q= github.com/arbovm/levenshtein v0.0.0-20160628152529-48b4e1c0c4d0/go.mod h1:t2tdKJDJF9BV14lnkjHmOQgcvEKgtqs5a1N3LNdJhGE= -github.com/ardanlabs/conf/v3 v3.1.8 h1:r0KUV9/Hni5XdeWR2+A1BiedIDnry5CjezoqgJ0rnFQ= -github.com/ardanlabs/conf/v3 v3.1.8/go.mod h1:OIi6NK95fj8jKFPdZ/UmcPlY37JBg99hdP9o5XmNK9c= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/cenkalti/backoff/v4 v4.3.0 h1:MyRJ/UdXutAwSAT+s3wNd7MfTIcy71VQueUuFK343L8= diff --git a/internal/business/persistedoperations/dir_loader.go b/internal/business/persistedoperations/dir_loader.go index 3462c00..e35f53e 100644 --- a/internal/business/persistedoperations/dir_loader.go +++ b/internal/business/persistedoperations/dir_loader.go @@ -72,7 +72,7 @@ func (d *LocalLoader) Load(_ context.Context) (map[string]PersistedOperation, er filesProcessed++ - data, err := UnmarshallPersistedOperations(contents) + data, err := unmarshallPersistedOperations(contents) if err != nil { d.log.Warn("error unmarshalling operation file", "bytes", len(contents), "contents", string(contents), "filepath", filePath, "err", err) continue diff --git a/internal/business/persistedoperations/gcp_loader.go b/internal/business/persistedoperations/gcp_loader.go index deb7324..e43f14b 100644 --- a/internal/business/persistedoperations/gcp_loader.go +++ b/internal/business/persistedoperations/gcp_loader.go @@ -112,5 +112,13 @@ func (g *GcpLoader) processFile(ctx context.Context, attrs *storage.ObjectAttrs) return nil, fmt.Errorf("io.Copy: %w", err) } - return UnmarshallPersistedOperations(data) + operations, err := unmarshallPersistedOperations(data) + + for _, operation := range operations { + if operation.Name == "" { + g.log.Warn("Operation without operation name found!", "operation", operation) + } + } + + return operations, err } diff --git a/internal/business/persistedoperations/model.go b/internal/business/persistedoperations/model.go index 7be4f1f..20cffde 100644 --- a/internal/business/persistedoperations/model.go +++ b/internal/business/persistedoperations/model.go @@ -3,16 +3,20 @@ package persistedoperations import ( "encoding/json" "fmt" - "strings" + "regexp" ) +// find the first word after the 'query' or 'mutation' keyword +var operationNameRegexPattern = regexp.MustCompile(`\b(query|mutation)\s(\w+)`) + type PersistedOperation struct { Operation string - Name string + Name string `json:"name,omitempty"` } -func UnmarshallPersistedOperations(payload []byte) (map[string]PersistedOperation, error) { +func unmarshallPersistedOperations(payload []byte) (map[string]PersistedOperation, error) { var manifestHashes map[string]string + err := json.Unmarshal(payload, &manifestHashes) if err != nil { return nil, fmt.Errorf("error unmarshalling operation file, bytes: %d, contents: %s, error: %w", len(payload), string(payload), err) @@ -21,32 +25,22 @@ func UnmarshallPersistedOperations(payload []byte) (map[string]PersistedOperatio data := make(map[string]PersistedOperation) for hash, operation := range manifestHashes { - data[hash] = NewPersistedOperation(operation) + data[hash] = PersistedOperation{ + Operation: operation, + Name: extractOperationNameFromOperation(operation), + } } return data, nil } -func NewPersistedOperation(operation string) PersistedOperation { - name := extractOperationNameFromPersistedOperation(operation) - return PersistedOperation{ - Operation: operation, - Name: name, - } -} - -func extractOperationNameFromPersistedOperation(payload string) string { - firstSpace := strings.Index(payload, " ") - firstBracket := strings.Index(payload, "{") - firstParenthesis := strings.Index(payload, "(") - - until := firstBracket - if firstParenthesis < firstBracket { - until = firstParenthesis - } +func extractOperationNameFromOperation(payload string) string { + match := operationNameRegexPattern.FindStringSubmatch(payload) - if firstSpace > until || until == -1 { - return "" + // match[0] is the entire match + // match[1] is either mutation/query + // match[2] is the name of the operation + if len(match) == 3 { + return match[2] } - - return strings.TrimSpace(payload[firstSpace+1 : until]) + return "" } diff --git a/internal/business/persistedoperations/model_test.go b/internal/business/persistedoperations/model_test.go index b0b1919..e48abda 100644 --- a/internal/business/persistedoperations/model_test.go +++ b/internal/business/persistedoperations/model_test.go @@ -12,81 +12,77 @@ func TestNewPersistedOperation(t *testing.T) { tests := []struct { name string args args - want PersistedOperation + want string + err error }{ { name: "extracts operation from query", args: args{ operation: "query ProductQuery{ product(id: 1) { id title as } }", }, - want: PersistedOperation{ - Operation: "query ProductQuery{ product(id: 1) { id title as } }", - Name: "ProductQuery", - }, + want: "ProductQuery", }, { name: "extracts operation from mutation", args: args{ operation: "mutation ProductQuery{ product(id: 1) { id title as } }", }, - want: PersistedOperation{ - Operation: "mutation ProductQuery{ product(id: 1) { id title as } }", - Name: "ProductQuery", - }, + want: "ProductQuery", }, { name: "no operation name when not present", args: args{ operation: "mutation { product(id: 1) { id title as } }", }, - want: PersistedOperation{ - Operation: "mutation { product(id: 1) { id title as } }", - Name: "", - }, + want: "", }, { name: "no operation name when no space between type and bracket", args: args{ operation: "mutation{ product(id: 1) { id title as } }", }, - want: PersistedOperation{ - Operation: "mutation{ product(id: 1) { id title as } }", - }, + want: "", }, { name: "excludes operation arguments", args: args{ operation: "query Foobar($some: Int, $value: String){ product(id: 1) { id title as } }", }, - want: PersistedOperation{ - Operation: "query Foobar($some: Int, $value: String){ product(id: 1) { id title as } }", - Name: "Foobar", - }, + want: "Foobar", }, { name: "no weird stuff when getting a completely malformed string", args: args{ operation: "", }, - want: PersistedOperation{ - Operation: "", - Name: "", - }, + want: "", }, { name: "handles white space around operation name", args: args{ operation: "query Foobar ($some: Int, $value: String){ product(id: 1) { id title as } }", }, - want: PersistedOperation{ - Operation: "query Foobar ($some: Int, $value: String){ product(id: 1) { id title as } }", - Name: "Foobar", + want: "Foobar", + }, + { + name: "error is thrown on non parseable queries", + args: args{ + operation: "invalidQueryString", + }, + want: "", + }, + { + name: "Can deal with fragments inside a query", + args: args{ + operation: "fragment BaseItem on MenuItem { action { menuItemActionType url } id imageUrl level measurement { clickedMenuItem } title } query MenuItems($country: String!, $id: String, $language: String!, $levels: String) { menuItems(country: $country, language: $language, id: $id, levels: $levels) { children { children { ...BaseItem } ...BaseItem } imageHeaderUrl level ...BaseItem } }", }, + want: "MenuItems", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equalf(t, tt.want, NewPersistedOperation(tt.args.operation), "NewPersistedOperation(%v)", tt.args.operation) + operation := extractOperationNameFromOperation(tt.args.operation) + assert.Equalf(t, tt.want, operation, "newPersistedOperation(%v)", tt.args.operation) }) } } diff --git a/internal/business/persistedoperations/persisted_operations_test.go b/internal/business/persistedoperations/persisted_operations_test.go index c0e1a15..ee4d899 100644 --- a/internal/business/persistedoperations/persisted_operations_test.go +++ b/internal/business/persistedoperations/persisted_operations_test.go @@ -131,7 +131,7 @@ func TestNewPersistedOperations(t *testing.T) { return bts }(), cache: map[string]PersistedOperation{ - "foobar": NewPersistedOperation("query { foobar }"), + "foobar": newPersistedOperation("query { foobar }"), }, }, want: func(t *testing.T) http.Handler { @@ -183,8 +183,8 @@ func TestNewPersistedOperations(t *testing.T) { return bts }(), cache: map[string]PersistedOperation{ - "foobar": NewPersistedOperation("query { foobar }"), - "baz": NewPersistedOperation("query { baz }"), + "foobar": newPersistedOperation("query { foobar }"), + "baz": newPersistedOperation("query { baz }"), }, }, want: func(t *testing.T) http.Handler { @@ -229,8 +229,7 @@ func TestNewPersistedOperations(t *testing.T) { return bts }(), cache: map[string]PersistedOperation{ - "foobar": NewPersistedOperation("query { foobar }"), - }, + "foobar": newPersistedOperation("query { foobar }")}, }, want: func(t *testing.T) http.Handler { fn := func(_ http.ResponseWriter, _ *http.Request) { @@ -383,6 +382,10 @@ func (e *errorLoader) Type() string { return "error" } +func newPersistedOperation(query string) PersistedOperation { + return PersistedOperation{query, extractOperationNameFromOperation(query)} +} + func (e *errorLoader) Load(_ context.Context) (map[string]PersistedOperation, error) { if e.willReturnError { return nil, e.err