feat: GQL variables and operation name #2993

nasdf · 2024-09-10T20:13:02Z

Relevant issue(s)

Resolves #1441
Resolves #1395

Description

This PR adds support for GraphQL variables and operation name.

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).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Added tests

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-09-10T20:41:19Z

Codecov Report

Attention: Patch coverage is 91.26984% with 22 lines in your changes missing coverage. Please review.

Project coverage is 79.41%. Comparing base (36fb5b5) to head (0aa3a76).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
internal/request/graphql/parser/request.go	72.09%	7 Missing and 5 partials ⚠️
internal/request/graphql/parser/filter.go	77.78%	3 Missing and 1 partial ⚠️
internal/request/graphql/parser/query.go	94.81%	3 Missing and 1 partial ⚠️
cli/request.go	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2993      +/-   ##
===========================================
+ Coverage    79.24%   79.41%   +0.17%     
===========================================
  Files          328      329       +1     
  Lines        25209    25120      -89     
===========================================
- Hits         19976    19949      -27     
+ Misses        3795     3757      -38     
+ Partials      1438     1414      -24

Flag	Coverage Δ
all-tests	`79.41% <91.27%> (+0.17%)`	⬆️

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

Files with missing lines	Coverage Δ
client/db.go	`100.00% <100.00%> (ø)`
http/client.go	`57.20% <100.00%> (+1.23%)`	⬆️
http/handler_store.go	`85.25% <100.00%> (+0.65%)`	⬆️
internal/db/request.go	`82.76% <100.00%> (ø)`
internal/db/store.go	`72.58% <100.00%> (+0.68%)`	⬆️
internal/db/view.go	`64.71% <100.00%> (ø)`
internal/request/graphql/parser.go	`89.83% <100.00%> (ø)`
internal/request/graphql/parser/commit.go	`87.23% <100.00%> (+5.14%)`	⬆️
internal/request/graphql/parser/mutation.go	`87.50% <100.00%> (+5.32%)`	⬆️
internal/request/graphql/parser/subscription.go	`86.21% <100.00%> (+9.06%)`	⬆️
... and 6 more

... and 18 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 36fb5b5...0aa3a76. Read the comment docs.

fredcarle · 2024-09-13T19:10:32Z

internal/request/graphql/parser/request.go

 	r := &request.Request{
 		Queries:      make([]*request.OperationDefinition, 0),
 		Mutations:    make([]*request.OperationDefinition, 0),
 		Subscription: make([]*request.OperationDefinition, 0),
 	}


thought (out of scope): Since we are now following the specs and explicitly allowing only one operation even if multiple are defined, it probably makes sense to refactor the request.Request type at some point.

The spec allows multiple, but if multiple operations are defined, technically you're supposed to specify which named op to run within in the document

6.1Executing Requests
To execute a request, the executor must have a parsed Document and a selected operation name to run if the document defines multiple operations, otherwise the document is expected to only contain a single operation.

Additionally, I would like to support multiple operations in the future, to support something akin to sub queries.

So you can use the result of one query into the doc set of another query.

The spec allows multiple, but if multiple operations are defined, technically you're supposed to specify which named op to run within in the document

My thought still stands here as although the gql can have multiple ops, the execution will only use one.

Additionally, I would like to support multiple operations in the future, to support something akin to sub queries.

We'll have to think carefully about the design of this one and that might also impact what request.Request looks like.

Additionally, I would like to support multiple operations in the future, to support something akin to sub queries.

So you can use the result of one query into the doc set of another query.

Can this be achieved with fragments?

Additionally, I would like to support multiple operations in the future, to support something akin to sub queries.

So you can use the result of one query into the doc set of another query.

Can this be achieved with fragments?

I don't see how off the top of my head, but open to seeing what if anything you have in mind.

fredcarle · 2024-09-13T19:57:11Z

tests/integration/query/simple/with_variables_test.go

+	executeTestCase(t, test)
+}
+
+func TestQuerySimpleWithVariableDefaultValue(t *testing.T) {


todo: Please add a test that shows that defaults can be overwritten.

fredcarle

LGTM. I like this change. There is some nice simplification in here that is much appreciated.
Just one todo to resolve before merge.

islamaliev · 2024-10-17T20:32:53Z

bug bash result: used different clients (Altair and Postman) to test variables and operation name. Worked as expected.

nasdf added 3 commits September 10, 2024 13:09

add support for gql variables

fb92ac4

add variable and operation tests

7f3bf0d

make docs

9e69cea

nasdf added the area/query Related to the query component label Sep 10, 2024

nasdf added this to the DefraDB v0.14 milestone Sep 10, 2024

nasdf self-assigned this Sep 10, 2024

nasdf added 3 commits September 10, 2024 13:23

make mocks

4eac0a3

make tidy

3a7edd7

add missing file headers

f7052f4

nasdf requested a review from a team September 10, 2024 20:24

nasdf mentioned this pull request Sep 11, 2024

feat: Operation selection in gql requests #2882

Closed

4 tasks

fredcarle reviewed Sep 13, 2024

View reviewed changes

fredcarle approved these changes Sep 13, 2024

View reviewed changes

nasdf and others added 2 commits September 13, 2024 14:55

add default variable override test

2bd8746

Merge branch 'develop' into nasdf/feat/gql-variables

0aa3a76

nasdf merged commit ccf4d93 into sourcenetwork:develop Sep 13, 2024
40 of 42 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: GQL variables and operation name #2993

feat: GQL variables and operation name #2993

nasdf commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

fredcarle Sep 13, 2024

jsimnz Sep 13, 2024

jsimnz Sep 13, 2024

jsimnz Sep 13, 2024

fredcarle Sep 13, 2024

nasdf Sep 13, 2024

jsimnz Sep 13, 2024

fredcarle Sep 13, 2024

fredcarle left a comment

islamaliev commented Oct 17, 2024

feat: GQL variables and operation name #2993

feat: GQL variables and operation name #2993

Conversation

nasdf commented Sep 10, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

islamaliev commented Oct 17, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading