refactor: GQL responses #2872

nasdf · 2024-07-26T18:22:05Z

Relevant issue(s)

Resolves #2869

Description

This PR makes our GQL response types spec compliant. It also enables running multiple selections within queries and mutations.

Update readme examples

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?

make test

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

MacOS

islamaliev

Briefly looked over. Looks good so far.

islamaliev · 2024-07-26T21:05:45Z

internal/planner/mapper/mapper.go

+			operation.Mutations = append(operation.Mutations, m)
+			operation.DocumentMapping.Add(i, m.Name)
+			operation.DocumentMapping.SetChildAt(i, m.DocumentMapping)
+			operation.DocumentMapping.RenderKeys = append(operation.DocumentMapping.RenderKeys, renderKey)


nitpick: the bodies of all 3 cases are very similar. Is there a way to extract common functionality?

I moved some of this to a function and reduced most of the duplicate lines

islamaliev · 2024-07-26T21:07:59Z

internal/planner/operation.go

+		return map[string]any{}, nil
+
+	case request.ExecuteExplain:
+		return map[string]any{}, nil


question: is it supposed to be empty?

Did you add explain tests that assert this?

I'm not super familiar with the planner package. Is this supposed to return the operations that this node executes? It does appear in the existing explain tests correctly.

I believe you can omit the Explain method entirely, since its an optional interface. I don't know if there is any context that this planNode can add to the existing plan, so simply not including it in the explain system is an option.

As @jsimnz pointed out, making a node explainable is optional. Moreover, MultiNode types are not / don't need to be explainable but we use them to wrap the children under them so the explain graph builder I believe handles that case. But when/if you make operationNode unexplainable don't forget to leave the added line in the tests/integration/explain.go file because the debug explain graph should still need it to assert it properly in the testing framework.

An example of this is parallelNode has no Explain method, yet still shows in the explain graph to wrap the children under it.

islamaliev · 2024-07-26T21:12:08Z

internal/planner/operation.go

+		}
+	}
+
+	n.isDone = true


question: do you really need this flag? It's being set only in Init(). Next() is not supposed to be called anyway before Init() and it's not multi-threaded.

I tried removing it, but the return value was always incorrect. I believe we need it so that the operation is able to run at least once.

nitpick/todo: The planNode interface and it's consuption dictates that Init can be called multiple times, and that it's state should be reset by the first call (before doing it's thing again). I can't think why this node would have an instance Init'ed multiple times, but it is kind of incorrect atm, and just last week we had a production issue caused by this.

Should be fixed now. I'm assuming you were referencing the currentValue not being reset.

islamaliev · 2024-07-26T21:15:55Z

internal/planner/planner.go

+func (p *Planner) MakePlan(req *request.Request) (planNode, error) {
+	var operation *request.OperationDefinition
+	if len(req.Mutations) > 0 {
+		operation = req.Mutations[0] // @todo: handle multiple mutation operation statements


todo: is this todo going to be resolved within this PR?

Otherwise please create a ticket and link it here.

I copied these over from another function. I'll double check if there is an existing issue or create one if needed.

Found it: #1395

islamaliev · 2024-07-26T21:18:43Z

tests/integration/acp/index/query_with_relation_test.go

-							{
-								"name": "Painted House",
+				Results: map[string]any{
+					"Author": []map[string]any{


praise: you did it all by hand? Wow. That must have been tedious.

I'm not gonna lie, It was a lot of work, but it had to be done.

islamaliev · 2024-07-26T21:25:36Z

tests/integration/utils2.go

@@ -1906,6 +1944,22 @@ func assertRequestResults(
 	return false
 }

+func convertToArrayOfMaps(t testing.TB, value any) []map[string]any {


question: these (and others) are changes from my PR that was merged. How did it end up here?

I think I copied these on accident when I was mid-refactor and didn't realize my local branch was behind. When I merge the develop branch these should appear as unchanged.

shahzadlone · 2024-07-29T10:08:21Z

tests/integration/explain.go

@@ -57,6 +57,7 @@ var (
 		"valuesNode":    {},
 		"viewNode":      {},
 		"lensNode":      {},
+		"operationNode": {},


praise: Thanks for not forgetting this :P (easy to miss if the operationNode was not explainable node).

tests/integration/explain/debug/fixture.go

shahzadlone · 2024-07-29T10:18:12Z

internal/planner/operation.go

+const operationNodeKind string = "operationNode"
+
+// operationNode is the top level node for operations with
+// one or more child selections, such as queries or mutations.


nitpick: I understand what you mean by operation but wonder if there is a better name for this node (I don't have any good ideas atm), the reason I say this is because every node is essentially an "instruction/operation step" so it seems a bit vague.

I agree the name could be improved. It should be fairly easy to rename if we find a better name.

shahzadlone · 2024-07-29T10:37:32Z

Huge praise for the work you did on this, it must have been a pain to make all these changes. I do have a preference to merge this ASAP if everyone is happy, otherwise maintaining this would not be so fun haha

codecov · 2024-07-29T16:38:14Z

Codecov Report

Attention: Patch coverage is 74.09326% with 50 lines in your changes missing coverage. Please review.

Project coverage is 79.44%. Comparing base (82659f8) to head (ca7bf81).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2872      +/-   ##
===========================================
- Coverage    79.49%   79.44%   -0.05%     
===========================================
  Files          323      325       +2     
  Lines        24695    24778      +83     
===========================================
+ Hits         19630    19683      +53     
- Misses        3660     3680      +20     
- Partials      1405     1415      +10

Flag	Coverage Δ
all-tests	`79.44% <74.09%> (-0.05%)`	⬇️

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

Files	Coverage Δ
http/handler_store.go	`84.60% <100.00%> (-0.32%)`	⬇️
internal/db/collection_update.go	`75.84% <100.00%> (-1.08%)`	⬇️
internal/db/request.go	`82.76% <ø> (-0.57%)`	⬇️
internal/db/subscriptions.go	`82.00% <100.00%> (-6.00%)`	⬇️
internal/planner/mapper/errors.go	`50.00% <ø> (ø)`
internal/planner/mapper/operation.go	`100.00% <100.00%> (ø)`
internal/planner/planner.go	`83.75% <86.49%> (+1.20%)`	⬆️
internal/planner/mapper/mapper.go	`88.79% <75.00%> (-0.76%)`	⬇️
internal/planner/operation.go	`79.76% <79.76%> (ø)`
internal/planner/explain.go	`60.87% <28.57%> (+0.31%)`	⬆️

... and 14 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 82659f8...ca7bf81. Read the comment docs.

…peration node

AndrewSisley

Looks good to me! Just a handful of small localized comments from me before merge.

AndrewSisley · 2024-07-30T00:12:33Z

internal/db/request.go

-
-	res.GQL.Data = results
+	if len(results) > 0 {
+		res.GQL.Data = results[0]


question/suggestion: Why is results still an array? Can we return just a map[string]any now instead?

Same question/suggestion for RunSelection.

Good catch! I've updated both to return maps.

AndrewSisley · 2024-07-30T00:15:47Z

internal/planner/mapper/mapper.go

+	}
+
+	for i, s := range operationRequest.Selections {
+		switch t := s.(type) {


suggestion: The default case is not handled, it might be better to return an error instead of quietly ignoring it - I believe it could only happen if we mess up in the future, but it could save some head-scratching even if it means a line or two of dead code.

I've added a default case with an error.

AndrewSisley · 2024-07-30T00:18:25Z

internal/planner/mapper/operation.go

+func (o *Operation) addSelection(i int, f request.Field, s Select) {
+	renderKey := core.RenderKey{Index: s.Index}
+	if f.Alias.HasValue() {
+		renderKey.Key = f.Alias.Value()


todo: Please add an operation alias test

Added tests/integration/query/simple/with_operation_alias_test.go

AndrewSisley · 2024-07-30T00:20:54Z

internal/planner/mapper/operation.go

+// The request.Field is used as the key for the core.RenderKey and the Select
+// document mapping is added as a child field.
+func (o *Operation) addSelection(i int, f request.Field, s Select) {
+	renderKey := core.RenderKey{Index: s.Index}


todo: There is a function getRenderKey that handles this, I would prefer that we don't duplicate that logic as it should remain consistent across all places.

AndrewSisley · 2024-07-30T00:23:49Z

internal/planner/operation.go

+// Operation creates a new operationNode using the given Selects.
+func (p *Planner) Operation(operation *mapper.Operation) (*operationNode, error) {
+	var children []planNode
+	var childIndexes []int


suggestion: I think a map[int]planNode would be safer and a little easier to read. Including on the operationNode type.

AndrewSisley · 2024-07-30T00:25:11Z

internal/planner/operation.go

+	}
+
+	if len(children) == 0 {
+		return nil, ErrOperationDefinitionMissingSelection


question: Is this spec-compliant? A selection set must always be provided?

I didn't see anything in the spec about empty selections. I've removed this error since it should be fine to return empty results.

AndrewSisley · 2024-07-30T00:26:24Z

internal/planner/operations.go

@@ -34,9 +34,11 @@ var (
 	_ planNode = (*valuesNode)(nil)
 	_ planNode = (*viewNode)(nil)
 	_ planNode = (*lensNode)(nil)
+	_ planNode = (*operationNode)(nil)


nitpick: MultiNode implements/extends planNode, so this line is not required.

AndrewSisley · 2024-07-30T00:29:13Z

internal/planner/planner.go

+// Note: Caller is responsible to call the `Close()` method to free the allocated
+// resources of the returned plan.
+//
+// @TODO {defradb/issues/368}: Test this exported function.


todo: This todo is incorrect - this is not a public function and does not require direct testing - if it is not used by us it can be deleted.

nasdf added 2 commits July 26, 2024 11:18

add operation node to planner

34173ea

update tests

e3ed55a

nasdf added the area/query Related to the query component label Jul 26, 2024

nasdf added this to the DefraDB v0.13 milestone Jul 26, 2024

nasdf self-assigned this Jul 26, 2024

fix typos and linter errors

9184773

islamaliev reviewed Jul 26, 2024

View reviewed changes

nasdf added 6 commits July 26, 2024 15:40

pr fixes

524fd54

fix ordering of operation children

4d847ac

Merge branch 'develop' into nasdf/refactor/gql-responses

8758c44

Merge branch 'develop' into nasdf/refactor/gql-responses

fcc576f

fix failing tests

b3469c6

check all map keys in assert results comparison

2ccc762

shahzadlone reviewed Jul 29, 2024

View reviewed changes

tests/integration/explain/debug/fixture.go Show resolved Hide resolved

shahzadlone reviewed Jul 29, 2024

View reviewed changes

nasdf added 3 commits July 29, 2024 08:48

fix http tests

bca807b

fix explain execute tests

819090e

fix result asserter tests

ead2482

nasdf marked this pull request as ready for review July 29, 2024 16:22

nasdf added 4 commits July 29, 2024 09:45

fix explain result asserter

681db99

fix gql mutation tests

585cc16

update docs

0935e4c

remove explain from operation node. properly reset state on init in o…

779959d

…peration node

nasdf requested a review from a team July 29, 2024 21:01

AndrewSisley approved these changes Jul 30, 2024

View reviewed changes

nasdf added 2 commits July 30, 2024 09:02

PR fixes

340d097

add operation alias test

ca7bf81

nasdf merged commit 8bd255f into sourcenetwork:develop Jul 30, 2024
40 of 42 checks passed

refactor: GQL responses #2872

refactor: GQL responses #2872

Conversation

nasdf commented Jul 26, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

islamaliev left a 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

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

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

shahzadlone commented Jul 29, 2024

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

AndrewSisley left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasdf commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading