refactor: remove embedding of context.Context in resolve.Context (#522)

Instead, use an approach very similar to the one used by net/http.Request Also, update CI to use Go 1.19 and 1.20, enable race detector and run the linters as a separate job.
wundergraph · May 5, 2023 · 4998581 · 4998581
1 parent 503b7af
commit 4998581
Show file tree

Hide file tree

Showing 38 changed files with 585 additions and 2,760 deletions.
diff --git a/.github/workflows/ci-full.yml b/.github/workflows/ci-full.yml
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -5,24 +5,56 @@ on:
     branches:
       - master
 jobs:
-  build:
-    name: Build (go ${{ matrix.go }}/${{ matrix.os }})
+  test:
+    name: Build and test (go ${{ matrix.go }} / ${{ matrix.os }})
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        go: [ '1.18' ]
+        go: [ '1.19', '1.20' ]
         os: [ubuntu-latest, windows-latest]
     steps:
       - name: Set up Go ${{ matrix.go }}
-        uses: actions/setup-go@v1
+        uses: actions/setup-go@v4
         with:
-          go-version: ${{ matrix.go }}
+          go-version: ^${{ matrix.go }}
         id: go
       - name: Set git to use LF
         run: |
           git config --global core.autocrlf false
           git config --global core.eol lf
       - name: Check out code into the Go module directory
-        uses: actions/checkout@v1
+        uses: actions/checkout@v3
       - name: CI
         run: make ci
+      - name: Run tests under race detector
+        if: runner.os != 'Windows' # These are very slow on Windows, skip them
+        run: make test-race
+
+  lint:
+    name: Linters
+    runs-on: ubuntu-latest
+    steps:
+      - name: Set up Go 1.20
+        uses: actions/setup-go@v4
+        with:
+          go-version: ^1.20
+      - name: Check out code into the Go module directory
+        uses: actions/checkout@v3
+
+      - name: Run linters
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: v1.51.1
+          args: --timeout=3m
+  ci:
+    name: CI Success
+    if: ${{ always() }}
+    runs-on: ubuntu-latest
+    needs: [test, lint]
+    steps:
+      - run: exit 1
+        if: >-
+          ${{
+               contains(needs.*.result, 'failure')
+            || contains(needs.*.result, 'cancelled')
+          }}
diff --git a/Makefile b/Makefile
@@ -1,40 +1,32 @@
-GOLANG_CI_VERSION = "v1.51.1"
-GOLANG_CI_VERSION_SHORT = "1.51.1"
-HAS_GOLANG_CI_LINT := $(shell command -v /tmp/ci/golangci-lint;)
-INSTALLED_VERSION := $(shell command -v /tmp/ci/golangci-lint version;)
-HAS_CORRECT_VERSION := $(shell command -v if [[ $(INSTALLED_VERSION) == *$(GOLANG_CI_VERSION_SHORT)* ]]; echo "OK" fi)
-
-.PHONY: bootstrap
-
 .PHONY: test
 test:
-	go test --short -count=1 ./...
+	go test ./...
 
-.PHONY: test-full
-test-full:
+.PHONY: test-quick
+test-quick:
 	go test -count=1 ./...
 
+.PHONY: test-race
+test-race:
+	go test -race ./...
+
 # updateTestFixtures will update all! golden fixtures
 .PHONY: updateTestFixtures
 updateTestFixtures:
 	go test ./pkg/... -update
 
-.PHONY: lint
-lint:
-	/tmp/ci/golangci-lint run
-
 .PHONY: format
 format:
 	go fmt ./...
 
 .PHONY: prepare-merge
-prepare-merge: format test lint
+prepare-merge: format test
 
 .PHONY: ci
-ci: bootstrap test lint
+ci: test
 
-.PHONY: ci-full
-ci-full: bootstrap test-full lint
+.PHONY: ci-quick
+ci-full: test-quick
 
 .PHONY: generate
 generate: $(GOPATH)/bin/go-enum $(GOPATH)/bin/mockgen $(GOPATH)/bin/stringer
@@ -52,12 +44,3 @@ $(GOPATH)/bin/mockgen:
 $(GOPATH)/bin/stringer:
 	go get -u -a golang.org/x/tools/cmd/stringer
 	go install golang.org/x/tools/cmd/stringer
-
-bootstrap:
-ifndef HAS_GOLANG_CI_LINT
-	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b /tmp/ci ${GOLANG_CI_VERSION}
-endif
-
-updateci:
-	rm /tmp/ci/golangci-lint
-	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b /tmp/ci ${GOLANG_CI_VERSION}
diff --git a/pkg/engine/datasource/graphql_datasource/graphql_datasource.go b/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
@@ -1329,7 +1329,8 @@ type Source struct {
 	httpClient *http.Client
 }
 
-func (s *Source) compactAndUnNullVariables(input []byte, undefinedVariables []string) []byte {
+func (s *Source) compactAndUnNullVariables(input []byte) []byte {
+	undefinedVariables := httpclient.UndefinedVariables(input)
 	variables, _, _, err := jsonparser.Get(input, "body", "variables")
 	if err != nil {
 		return input
@@ -1339,7 +1340,9 @@ func (s *Source) compactAndUnNullVariables(input []byte, undefinedVariables []st
 	}
 	if bytes.ContainsAny(variables, " \t\n\r") {
 		buf := bytes.NewBuffer(make([]byte, 0, len(variables)))
-		_ = json.Compact(buf, variables)
+		if err := json.Compact(buf, variables); err != nil {
+			panic(fmt.Errorf("compacting variables: %w", err))
+		}
 		variables = buf.Bytes()
 	}
 
@@ -1409,9 +1412,7 @@ func (s *Source) replaceEmptyObject(variables []byte) ([]byte, bool) {
 }
 
 func (s *Source) Load(ctx context.Context, input []byte, writer io.Writer) (err error) {
-	undefinedVariables := httpclient.CtxGetUndefinedVariables(ctx)
-
-	input = s.compactAndUnNullVariables(input, undefinedVariables)
+	input = s.compactAndUnNullVariables(input)
 	return httpclient.Do(s.httpClient, ctx, input, writer)
 }
 

diff --git a/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go b/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
@@ -7801,23 +7801,23 @@ func TestSubscriptionSource_Start(t *testing.T) {
 
 	t.Run("invalid json: should stop before sending to upstream", func(t *testing.T) {
 		next := make(chan []byte)
-		ctx := context.Background()
-		defer ctx.Done()
+		ctx := resolve.NewContext(context.Background())
+		defer ctx.Context().Done()
 
-		source := newSubscriptionSource(ctx)
+		source := newSubscriptionSource(ctx.Context())
 		chatSubscriptionOptions := chatServerSubscriptionOptions(t, `{"variables": {}, "extensions": {}, "operationName": "LiveMessages", "query": "subscription LiveMessages { messageAdded(roomName: "#test") { text createdBy } }"}`)
-		err := source.Start(ctx, chatSubscriptionOptions, next)
+		err := source.Start(ctx.Context(), chatSubscriptionOptions, next)
 		require.ErrorIs(t, err, resolve.ErrUnableToResolve)
 	})
 
 	t.Run("invalid syntax (roomNam)", func(t *testing.T) {
 		next := make(chan []byte)
-		ctx := context.Background()
-		defer ctx.Done()
+		ctx := resolve.NewContext(context.Background())
+		defer ctx.Context().Done()
 
-		source := newSubscriptionSource(ctx)
+		source := newSubscriptionSource(ctx.Context())
 		chatSubscriptionOptions := chatServerSubscriptionOptions(t, `{"variables": {}, "extensions": {}, "operationName": "LiveMessages", "query": "subscription LiveMessages { messageAdded(roomNam: \"#test\") { text createdBy } }"}`)
-		err := source.Start(ctx, chatSubscriptionOptions, next)
+		err := source.Start(ctx.Context(), chatSubscriptionOptions, next)
 		require.NoError(t, err)
 
 		msg, ok := <-next
@@ -7851,12 +7851,12 @@ func TestSubscriptionSource_Start(t *testing.T) {
 
 	t.Run("should successfully subscribe with chat example", func(t *testing.T) {
 		next := make(chan []byte)
-		ctx := context.Background()
-		defer ctx.Done()
+		ctx := resolve.NewContext(context.Background())
+		defer ctx.Context().Done()
 
-		source := newSubscriptionSource(ctx)
+		source := newSubscriptionSource(ctx.Context())
 		chatSubscriptionOptions := chatServerSubscriptionOptions(t, `{"variables": {}, "extensions": {}, "operationName": "LiveMessages", "query": "subscription LiveMessages { messageAdded(roomName: \"#test\") { text createdBy } }"}`)
-		err := source.Start(ctx, chatSubscriptionOptions, next)
+		err := source.Start(ctx.Context(), chatSubscriptionOptions, next)
 		require.NoError(t, err)
 
 		username := "myuser"
@@ -7913,12 +7913,12 @@ func TestSubscription_GTWS_SubProtocol(t *testing.T) {
 
 	t.Run("invalid syntax (roomNam)", func(t *testing.T) {
 		next := make(chan []byte)
-		ctx := context.Background()
-		defer ctx.Done()
+		ctx := resolve.NewContext(context.Background())
+		defer ctx.Context().Done()
 
-		source := newSubscriptionSource(ctx)
+		source := newSubscriptionSource(ctx.Context())
 		chatSubscriptionOptions := chatServerSubscriptionOptions(t, `{"variables": {}, "extensions": {}, "operationName": "LiveMessages", "query": "subscription LiveMessages { messageAdded(roomNam: \"#test\") { text createdBy } }"}`)
-		err := source.Start(ctx, chatSubscriptionOptions, next)
+		err := source.Start(ctx.Context(), chatSubscriptionOptions, next)
 		require.NoError(t, err)
 
 		msg, ok := <-next
@@ -7952,12 +7952,12 @@ func TestSubscription_GTWS_SubProtocol(t *testing.T) {
 
 	t.Run("should successfully subscribe with chat example", func(t *testing.T) {
 		next := make(chan []byte)
-		ctx := context.Background()
-		defer ctx.Done()
+		ctx := resolve.NewContext(context.Background())
+		defer ctx.Context().Done()
 
-		source := newSubscriptionSource(ctx)
+		source := newSubscriptionSource(ctx.Context())
 		chatSubscriptionOptions := chatServerSubscriptionOptions(t, `{"variables": {}, "extensions": {}, "operationName": "LiveMessages", "query": "subscription LiveMessages { messageAdded(roomName: \"#test\") { text createdBy } }"}`)
-		err := source.Start(ctx, chatSubscriptionOptions, next)
+		err := source.Start(ctx.Context(), chatSubscriptionOptions, next)
 		require.NoError(t, err)
 
 		username := "myuser"
@@ -8159,7 +8159,8 @@ func TestSource_Load(t *testing.T) {
 			buf := bytes.NewBuffer(nil)
 
 			undefinedVariables := []string{"a", "c"}
-			ctx := httpclient.CtxSetUndefinedVariables(context.Background(), undefinedVariables)
+			ctx := context.Background()
+			input = httpclient.SetUndefinedVariables(input, undefinedVariables)
 
 			require.NoError(t, src.Load(ctx, input, buf))
 			assert.Equal(t, `{"variables":{"b":null}}`, buf.String())
@@ -8171,85 +8172,85 @@ func TestUnNullVariables(t *testing.T) {
 	t.Run("should not unnull variables if not enabled", func(t *testing.T) {
 		t.Run("two variables, one null", func(t *testing.T) {
 			s := &Source{}
-			out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":null,"b":true}}}`), []string{})
+			out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":null,"b":true}}}`))
 			expected := `{"body":{"variables":{"a":null,"b":true}}}`
 			assert.Equal(t, expected, string(out))
 		})
 	})
 
 	t.Run("variables with whitespace and empty objects", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"email":null,"firstName": "FirstTest",		"lastName":"LastTest","phone":123456,"preferences":{ "notifications":{}},"password":"password"}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"email":null,"firstName": "FirstTest",		"lastName":"LastTest","phone":123456,"preferences":{ "notifications":{}},"password":"password"}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{"firstName":"FirstTest","lastName":"LastTest","phone":123456,"password":"password"}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("empty variables", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("null inside an array", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"list":["a",null,"b"]}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"list":["a",null,"b"]}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{"list":["a",null,"b"]}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("complex null inside nested objects and arrays", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":null, "b": {"key":null, "nested": {"nestedkey": null}}, "arr": ["1", null, "3"], "d": {"nested_arr":["4",null,"6"]}}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":null, "b": {"key":null, "nested": {"nestedkey": null}}, "arr": ["1", null, "3"], "d": {"nested_arr":["4",null,"6"]}}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{"b":{"key":null,"nested":{"nestedkey":null}},"arr":["1",null,"3"],"d":{"nested_arr":["4",null,"6"]}}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("two variables, one null", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":null,"b":true}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":null,"b":true}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{"b":true}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("two variables, one null reverse", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":true,"b":null}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"a":true,"b":null}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{"a":true}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("null variables", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":null},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":null},"unnull_variables":true}`))
 		expected := `{"body":{"variables":null},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("ignore null inside non variables", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"foo":null},"body":"query {foo(bar: null){baz}}"},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"foo":null},"body":"query {foo(bar: null){baz}}"},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{},"body":"query {foo(bar: null){baz}}"},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("ignore null in variable name", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"not_null":1,"null":2,"not_null2":3}},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"variables":{"not_null":1,"null":2,"not_null2":3}},"unnull_variables":true}`))
 		expected := `{"body":{"variables":{"not_null":1,"null":2,"not_null2":3}},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("variables missing", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"query":"{foo}"},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"query":"{foo}"},"unnull_variables":true}`))
 		expected := `{"body":{"query":"{foo}"},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})
 
 	t.Run("variables null", func(t *testing.T) {
 		s := &Source{}
-		out := s.compactAndUnNullVariables([]byte(`{"body":{"query":"{foo}","variables":null},"unnull_variables":true}`), []string{})
+		out := s.compactAndUnNullVariables([]byte(`{"body":{"query":"{foo}","variables":null},"unnull_variables":true}`))
 		expected := `{"body":{"query":"{foo}","variables":null},"unnull_variables":true}`
 		assert.Equal(t, expected, string(out))
 	})