feat: Mutation typed input #2167

nasdf · 2024-01-05T17:43:28Z

Relevant issue(s)

Resolves #2143

Description

This PR adds a typed input object for create and update mutations.

~~As a side effect of this change the relationship alias has been replaced with the <type>_id input field.~~

Relational sub-documents cannot be created from mutation input in this implementation.

Related SIP https://github.com/sourcenetwork/SIPs/discussions/10

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

codecov · 2024-01-05T18:04:41Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (858e6a6) 74.14% compared to head (4bd619f) 74.22%.

❗ Current head 4bd619f differs from pull request most recent head 876a693. Consider uploading reports for the commit 876a693 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2167      +/-   ##
===========================================
+ Coverage    74.14%   74.22%   +0.07%     
===========================================
  Files          256      256              
  Lines        25399    25465      +66     
===========================================
+ Hits         18831    18899      +68     
+ Misses        5285     5277       -8     
- Partials      1283     1289       +6

Flag	Coverage Δ
all-tests	`74.22% <80.33%> (+0.07%)`	⬆️

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

Files	Coverage Δ
client/document.go	`67.60% <100.00%> (+3.53%)`	⬆️
client/request/mutation.go	`100.00% <ø> (ø)`
planner/create.go	`64.13% <100.00%> (-3.55%)`	⬇️
planner/explain.go	`58.99% <ø> (ø)`
planner/mapper/mapper.go	`87.42% <100.00%> (ø)`
planner/mapper/mutation.go	`0.00% <0.00%> (ø)`
request/graphql/parser/mutation.go	`79.82% <93.55%> (+0.05%)`	⬆️
planner/update.go	`76.24% <57.14%> (-0.23%)`	⬇️
request/graphql/schema/errors.go	`29.82% <0.00%> (-2.87%)`	⬇️
request/graphql/schema/generate.go	`86.01% <80.88%> (+0.28%)`	⬆️

... 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 858e6a6...876a693. Read the comment docs.

fredcarle · 2024-01-09T16:45:33Z

request/graphql/schema/generate.go

+// for collection create and update mutation operations.
+func (g *Generator) buildMutationInputTypes(collections []client.CollectionDefinition) error {
+	for _, c := range collections {
+		// Copy the loop variable before usage within the loop or it


thought: Note that this will no longer be needed starting with Go 1.22

nitpick: Would appreciate a comment here mentioning 1.22 for regex search in the future, so can easily find some of these.

shahzadlone

As a side effect of this change the relationship alias has been replaced with the _id input field.

I don't understand what is meant by 'replaced' with the type_id, because before we had both the 'type' alias and 'type_id'.

Why were the alias tests deleted? did we just drop support for aliases (i.e. a whole feature seems to have been dropped)?

IIRC this was a feature that was requested by one of the partners, and was implemented in:

More context:
https://discord.com/channels/427944769851752448/1061831110956421150/1113950104517365900

nasdf · 2024-01-09T18:34:08Z

As a side effect of this change the relationship alias has been replaced with the _id input field.

I don't understand what is meant by 'replaced' with the type_id, because before we had both the 'type' alias and 'type_id'.

Why were the alias tests deleted? did we just drop support for aliases (i.e. a whole feature seems to have been dropped)?

IIRC this was a feature that was requested by one of the partners, and was implemented in:

feat: Add alias to groupBy related object #1579

feat: Allow relation alias on create and update #1609

More context: https://discord.com/channels/427944769851752448/1061831110956421150/1113950104517365900

We talked about this briefly in the standup last week. Since the mutation input fields are now typed it is not possible to support relation field aliases (unless we add gql union types).

fredcarle · 2024-01-09T18:47:44Z

it is not possible to support relation field aliases

It is possible if we set the mutation type for the foreign object as a gql.ID. But we may want to be able create the foreign document from the on mutation which would require the field to remain an object. We have to decide what we want to do here.

fredcarle

In order to not get this PR hung up by the reverting of the aliasing feature, lets keep the aliasing for now and lets try to reach consensus separately on what we want to do about the possibility of allowing embedded foreign objects.

Without the removal of aliasing, I think this PR can me approved and merged.

fredcarle

LGTM! Awesome work! I love that we now have this implemented.

If you don't mind just adding the 1.22 reference as mentionned by Shahzad before merging.

shahzadlone

Sorry just reviewing, thanks for adding alias stuff back. I am mid review will appreciate if possible to wait for me to finish.

shahzadlone · 2024-01-10T21:23:23Z

planner/create.go

 // Explain method returns a map containing all attributes of this node that
 // are to be explained, subscribes / opts-in this node to be an explainablePlanNode.
 func (n *createNode) Explain(explainType request.ExplainType) (map[string]any, error) {
 	switch explainType {
 	case request.SimpleExplain:
-		return n.simpleExplain()
+		return map[string]any{
+			dataLabel: n.input,


todo: the name dataLabeldoesn't make sense anymore. Please change to inputLabel

shahzadlone

Cheers for this PR! I like it a lot! Left some var renaming todos and in addition have this todo:

In planner/explain.go file we have on line 56:

dataLabel           = "data"

After renaming the variable please don't forget to change the value from"data" to "input".

OR if you want to use request.Input in all those instances and just remove the dataLabel/inputLabel , I am okay with that too :)

shahzadlone · 2024-01-10T22:41:15Z

client/request/consts.go

@@ -21,6 +21,7 @@ const (

 	Cid         = "cid"
 	Data        = "data"


todo: Please delete this Data variable now, it is now not used anywhere.

shahzadlone · 2024-01-10T22:46:25Z

planner/update.go

-		return nil, err
-	}
-	simpleExplainMap[dataLabel] = data
+	simpleExplainMap[dataLabel] = n.input


todo: Similar to other suggestions please change the variable name to be more relevant.

Suggested change

simpleExplainMap[dataLabel] = n.input

simpleExplainMap[inputLabel] = n.input

shahzadlone

Cheers for doing all the todos!

## Relevant issue(s) Resolves sourcenetwork#2143 ## Description This PR adds a typed input object for create and update mutations. ~~As a side effect of this change the relationship alias has been replaced with the `<type>_id` input field.~~ Relational sub-documents cannot be created from mutation input in this implementation. Related SIP sourcenetwork/SIPs#10 ## 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)). - [x] 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

nasdf added 9 commits December 20, 2023 11:30

introspection working for mutation input arg

f8ab109

update planner with mutation input args

0462b75

update tests with mutation input args

0ae5b5a

update tests to use id field

b49ddb8

add gql test functions

3b473d0

Merge branch 'develop' into nasdf/feat/gql-mutation-input-arg

fafb111

fix docID in test

266b197

allow int32 for document integer field

3ed7679

remove data field from mutations

63d9f7c

nasdf added feature New feature or request area/query Related to the query component area/parser Related to the parser components labels Jan 5, 2024

nasdf self-assigned this Jan 5, 2024

nasdf added 6 commits January 5, 2024 10:18

improve document float type parsing. replace _key in gql test utils

808a3b0

return empty array when parsing mutation input list

44cbfb3

remove relation alias from backup tests

8838487

limit mutation types for failing tests

0080965

delete relation alias tests

7853ade

fix linter warnings

1f1ede4

nasdf requested a review from a team January 5, 2024 19:35

fredcarle reviewed Jan 9, 2024

View reviewed changes

shahzadlone requested changes Jan 9, 2024

View reviewed changes

fredcarle requested changes Jan 10, 2024

View reviewed changes

nasdf added 2 commits January 10, 2024 12:44

relation input alias field

cbf948d

restore alias tests

0d6e6c0

fredcarle approved these changes Jan 10, 2024

View reviewed changes

shahzadlone requested changes Jan 10, 2024

View reviewed changes

shahzadlone reviewed Jan 10, 2024

View reviewed changes

nasdf and others added 4 commits January 10, 2024 13:30

fix failing tests

36adf4c

add go 1.22 loop comment

6993345

Merge branch 'develop' into nasdf/feat/gql-mutation-input-arg

35f895f

fix pncounter test

f0687c9

shahzadlone reviewed Jan 10, 2024

View reviewed changes

fix mutation input explain label

245ceb5

shahzadlone approved these changes Jan 10, 2024

View reviewed changes

nasdf and others added 2 commits January 10, 2024 16:04

Merge branch 'develop' into nasdf/feat/gql-mutation-input-arg

4bd619f

fix linter warning

876a693

nasdf merged commit 72732f3 into sourcenetwork:develop Jan 11, 2024
26 of 27 checks passed

fredcarle added this to the DefraDB v0.9 milestone Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Mutation typed input #2167

feat: Mutation typed input #2167

nasdf commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

fredcarle Jan 9, 2024

shahzadlone Jan 9, 2024 •

edited

Loading

shahzadlone left a comment •

edited

Loading

nasdf commented Jan 9, 2024

fredcarle commented Jan 9, 2024

fredcarle left a comment

fredcarle left a comment

shahzadlone left a comment

shahzadlone Jan 10, 2024

shahzadlone left a comment •

edited

Loading

shahzadlone Jan 10, 2024

shahzadlone Jan 10, 2024

shahzadlone left a comment

	simpleExplainMap[dataLabel] = n.input
	simpleExplainMap[inputLabel] = n.input

feat: Mutation typed input #2167

feat: Mutation typed input #2167

Conversation

nasdf commented Jan 5, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

fredcarle Jan 9, 2024

Choose a reason for hiding this comment

shahzadlone Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

nasdf commented Jan 9, 2024

fredcarle commented Jan 9, 2024

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

shahzadlone Jan 10, 2024

Choose a reason for hiding this comment

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

shahzadlone Jan 10, 2024

Choose a reason for hiding this comment

shahzadlone Jan 10, 2024

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented Jan 5, 2024 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

shahzadlone Jan 9, 2024 •

edited

Loading

shahzadlone left a comment •

edited

Loading

shahzadlone left a comment •

edited

Loading