fix: Add support for operationName and variables in HTTP GET #3292

ChrisBQu · 2024-12-04T21:35:26Z

Relevant issue(s)

Resolves #3153

Description

I have made it so that operationName and variables are supported by HTTP GET requests. I did this by modifying the ExecRequest function inside http/handler_store.go such that operationName and variables parameters are extracted if they are present.

I added two new tests to http/handler_store_test.go which test this functionality. See:

TestExecRequest_WithValidQuery_HttpGet_WithOperationName_OmitsErrors
and
TestExecRequest_HttpGet_WithVariables_OmitsErrors

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).

How has this been tested?

The platform(s) on which this was tested:

Windows

fredcarle

Some initial comments :)

fredcarle · 2024-12-04T21:41:58Z

http/handler_store.go

+		operationNameFromQuery := req.URL.Query().Get("operationName")
+		if operationNameFromQuery != "" {
+			request.OperationName = operationNameFromQuery
+		}


suggestion: You could assign directly to request.OperationName. Doing so would have the exact same outcome as what you're doing here.

fredcarle · 2024-12-04T21:49:39Z

http/handler_store_test.go

@@ -93,3 +94,79 @@ func TestExecRequest_WithInvalidQuery_HasSpecCompliantErrors(t *testing.T) {
 		"message": "Cannot query field \"invalid\" on type \"User\".",
 	}})
 }
+
+func TestExecRequest_WithValidQuery_HttpGet_WithOperationName_OmitsErrors(t *testing.T) {


suggestion: The test above that has WithValidQuery_OmitsErrors, has this pattern because it specifically checks that with a valid query, the API returns no error field in the payload (we previously had an empty list for errors). I would suggestion removing WithValidQuery as it's not that relevant to the test and instead of OmitsErrors, maybe have something like ShouldSucceed or something more meaningful. Same for the test bellow.

http/handler_store_test.go

playground/package-lock.json

codecov · 2024-12-04T22:22:26Z

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.09%. Comparing base (e4599e8) to head (616ddc4).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
http/handler_store.go	66.67%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3292   +/-   ##
========================================
  Coverage    78.09%   78.09%           
========================================
  Files          382      382           
  Lines        35371    35383   +12     
========================================
+ Hits         27621    27632   +11     
  Misses        6117     6117           
- Partials      1633     1634    +1

Flag	Coverage Δ
all-tests	`78.09% <66.67%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
http/handler_store.go	`81.16% <66.67%> (-0.46%)`	⬇️

... and 13 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4599e8...616ddc4. Read the comment docs.

fredcarle · 2024-12-05T19:38:44Z

http/handler_store_test.go

+	// errors should be omitted
+	_, ok := gqlResponse["errors"]
+	assert.False(t, ok)


suggestion: This could be removed as having it in the test suggests that it's important for this test to check that the errors are omitted (it's not). We have a dedicated test for this above.

fredcarle · 2024-12-05T19:38:59Z

http/handler_store_test.go

+	// errors should be omitted
+	_, ok := gqlResponse["errors"]
+	assert.False(t, ok)


suggestion: Same as above.

fredcarle · 2024-12-05T19:42:21Z

http/handler_store_test.go

+	_, ok := gqlResponse["errors"]
+	assert.False(t, ok)
+
+	// Compare the response data to the expected output


suggestion: This comment is describing what is already evident just by looking at the code. Was is less evident though is that you are ensuring that the returned user list doesn't include the _docID field from the other operation. The comment should probably convey that.

fredcarle · 2024-12-05T19:45:15Z

http/handler_store_test.go

+	_, ok := gqlResponse["errors"]
+	assert.False(t, ok)
+
+	// Compare the response data to the expected output


suggestion: Similar to above. In this case, something like Ensure only bob is returned based on the provided filter variable. would be more descriptive.

fredcarle

LGTM! Thanks for taking care of this, Chris :)

…etwork#3292) ## Relevant issue(s) Resolves sourcenetwork#3153 ## Description I have made it so that operationName and variables are supported by HTTP GET requests. I did this by modifying the `ExecRequest` function inside `http/handler_store.go` such that `operationName` and `variables` parameters are extracted if they are present. I added two new tests to `http/handler_store_test.go` which test this functionality. See: `TestExecRequest_WithValidQuery_HttpGet_WithOperationName_OmitsErrors` and `TestExecRequest_HttpGet_WithVariables_OmitsErrors` ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). ## How has this been tested? The platform(s) on which this was tested: - Windows

Issue 3153 first commit

227365a

ChrisBQu added the bug Something isn't working label Dec 4, 2024

ChrisBQu added this to the DefraDB v0.15 milestone Dec 4, 2024

ChrisBQu self-assigned this Dec 4, 2024

ChrisBQu changed the title ~~Issue 3153 first commit~~ bug: Add support for operationName and variables in HTTP GET Dec 4, 2024

ChrisBQu requested a review from a team December 4, 2024 21:36

ChrisBQu changed the title ~~bug: Add support for operationName and variables in HTTP GET~~ fix: Add support for operationName and variables in HTTP GET Dec 4, 2024

ChrisBQu removed the request for review from a team December 4, 2024 21:52

fredcarle requested changes Dec 4, 2024

View reviewed changes

Fixed one unit test

80f0661

ChrisBQu added 4 commits December 5, 2024 10:24

Feedback changes

76273ae

Feedback changes

c8f70a1

Roll back package-lock.json in playground

462ee5f

Adjust white space

942ae7f

ChrisBQu requested a review from fredcarle December 5, 2024 15:55

ChrisBQu added 2 commits December 5, 2024 11:23

Removed extraneous line of code

169fbd1

Major fixes; Changed tests

f6a0eb8

fredcarle reviewed Dec 5, 2024

View reviewed changes

ChrisBQu added 2 commits December 5, 2024 15:14

Improve comments, remove error checks

5afefe6

Delint

5a6078b

ChrisBQu force-pushed the issue-3153 branch from 1047589 to 5a6078b Compare December 5, 2024 20:22

Merge branch 'develop' into issue-3153

6190011

fredcarle approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'develop' into issue-3153

616ddc4

ChrisBQu merged commit 89f9f41 into sourcenetwork:develop Dec 5, 2024
41 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add support for operationName and variables in HTTP GET #3292

fix: Add support for operationName and variables in HTTP GET #3292

ChrisBQu commented Dec 4, 2024

fredcarle left a comment

fredcarle Dec 4, 2024

fredcarle Dec 4, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

fredcarle Dec 5, 2024

fredcarle Dec 5, 2024

fredcarle Dec 5, 2024

fredcarle Dec 5, 2024

fredcarle left a comment

fix: Add support for operationName and variables in HTTP GET #3292

fix: Add support for operationName and variables in HTTP GET #3292

Conversation

ChrisBQu commented Dec 4, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle Dec 4, 2024

Choose a reason for hiding this comment

fredcarle Dec 4, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

fredcarle Dec 5, 2024

Choose a reason for hiding this comment

fredcarle Dec 5, 2024

Choose a reason for hiding this comment

fredcarle Dec 5, 2024

Choose a reason for hiding this comment

fredcarle Dec 5, 2024

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 •

edited

Loading