feat: GraphQL upsert mutation #3075

nasdf · 2024-09-25T19:10:11Z

Relevant issue(s)

Resolves #783

Description

This PR adds a new upsert GraphQL mutation operation.

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 integration tests.

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

MacOS

codecov · 2024-09-25T19:40:03Z

Codecov Report

Attention: Patch coverage is 81.31868% with 34 lines in your changes missing coverage. Please review.

Project coverage is 79.44%. Comparing base (54c99e7) to head (2f94014).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
internal/planner/upsert.go	69.77%	15 Missing and 11 partials ⚠️
internal/request/graphql/parser/mutation.go	89.47%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3075      +/-   ##
===========================================
- Coverage    79.50%   79.44%   -0.06%     
===========================================
  Files          345      346       +1     
  Lines        26565    26705     +140     
===========================================
+ Hits         21118    21214      +96     
- Misses        3936     3963      +27     
- Partials      1511     1528      +17

Flag	Coverage Δ
all-tests	`79.44% <81.32%> (-0.06%)`	⬇️

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

Files with missing lines	Coverage Δ
client/request/mutation.go	`100.00% <ø> (ø)`
internal/planner/create.go	`81.16% <100.00%> (ø)`
internal/planner/errors.go	`25.00% <ø> (ø)`
internal/planner/explain.go	`60.87% <ø> (ø)`
internal/planner/mapper/mapper.go	`88.83% <100.00%> (+0.01%)`	⬆️
internal/planner/planner.go	`83.39% <100.00%> (+0.23%)`	⬆️
internal/planner/update.go	`78.48% <100.00%> (-0.27%)`	⬇️
internal/request/graphql/schema/generate.go	`88.11% <100.00%> (+0.14%)`	⬆️
internal/request/graphql/parser/mutation.go	`91.07% <89.47%> (+5.16%)`	⬆️
internal/planner/upsert.go	`69.77% <69.77%> (ø)`

... and 9 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 54c99e7...2f94014. Read the comment docs.

shahzadlone

Wowza, super awesome stuff! Left some comments in first pass :)

tests/integration/mutation/upsert/simple_test.go

internal/request/graphql/schema/descriptions.go

tests/integration/mutation/upsert/simple_test.go

tests/integration/explain/debug/upsert_test.go

tests/integration/mutation/upsert/simple_test.go

internal/planner/upsert.go

islamaliev

Looks nice for far! Very useful feature. I have most suggestions.

islamaliev · 2024-09-26T08:19:01Z

internal/planner/upsert.go

@@ -0,0 +1,185 @@
+// Copyright 2022 Democratized Data Foundation


nitpick: 2024

islamaliev · 2024-09-26T08:23:54Z

internal/planner/upsert.go

+	updateInput map[string]any
+	doc         *client.Document
+	isUpserting bool
+	results     planNode


suggestion: I find the name results a bit confusing for planNode. When reading methods' bodies I though it was a results objects of storage iterator.
I think a more common name would be source or plan

changed to source

islamaliev · 2024-09-26T08:27:10Z

internal/planner/upsert.go

+	createInput map[string]any
+	updateInput map[string]any
+	doc         *client.Document
+	isUpserting bool


suggestion: was also a bit puzzled why upsertNode has isUpserting prop. Maybe call it something like isInitialized? As a bonus you wouldn't need to initialize it to true

I copied it from the update node which has an isUpdating prop.

islamaliev · 2024-09-26T08:35:35Z

internal/planner/upsert.go

+		switch next {
+		case true:


suggestion: first time in my life I see this kind of switch :D
Maybe a classical if?

lol +1 for making into if

islamaliev · 2024-09-26T08:43:15Z

internal/planner/upsert.go

+	doc, err := client.NewDocFromMap(n.createInput, n.collection.Definition())
+	if err != nil {
+		return err
+	}
+	n.doc = doc


suggestion: this part is not needed when upserting. Maybe move it to "create" branch?

islamaliev · 2024-09-26T08:49:11Z

internal/planner/upsert.go

+}
+
+// Next only returns once.
+func (n *upsertNode) Next() (bool, error) {


suggestion: this upsertNode looks (to some extend) as copy&paste of 2 nodes updateNode and createNode.
Did you consider making upsertNode detect if it's update or create op and (on the fly?) use one of those?

I did have that thought, but I think its worth the extra LOC to keep it separate incase we decide to extend or modify it.

islamaliev · 2024-09-26T09:05:12Z

tests/integration/mutation/upsert/simple_test.go

+	testUtils.ExecuteTestCase(t, test)
+}
+
+func TestMutationUpsertSimple_WithUniqueCompositeIndexAndDuplicateUpdate_ReturnsError(t *testing.T) {


praise: thanks for this test and for making clear test names.

islamaliev · 2024-09-26T09:06:20Z

tests/integration/mutation/upsert/simple_test.go

+			testUtils.Request{
+				Request: `mutation {
+					upsert_Users(
+						filter: {name: {_eq: "Bob"}},
+						create: {name: "Bob", age: 40},
+						update: {age: 40}
+					) {
+						name
+						age
+					}
+				}`,
+				Results: map[string]any{
+					"upsert_Users": []map[string]any{
+						{
+							"name": "Bob",
+							"age":  int64(40),
+						},
+					},
+				},
+			},


suggestion: I think it also makes sense to run a follow up query request to make it really fetches them from storage and not just as a response.

islamaliev · 2024-09-26T09:07:29Z

client/request/consts.go

 	Input       = "input"
+	CreateInput = "create"
+	UpdateInput = "update"


question: shouldn't "create" and "update" replace "input"?

I think keeping the input label for create and update mutations is fine.

Changing it to create_User(create: {name: "Bob"}) doesn't read as nice imo.

islamaliev · 2024-09-26T09:09:00Z

internal/planner/upsert.go

+				return false, err
+			}
+			if next {
+				return false, ErrUpsertMultipleDocuments


question: is there an inherent problemt with upserting multiple docs?

There's nothing stopping us from supporting it in the future. It's there to stop users from doing something unexpected like updating all documents when they just wanted to upsert a single document.

If we do support it in the future we will want to add a variable to control the behavior: unique or all

islamaliev

Great job, Keenan!

fredcarle

LGTM!

shahzadlone

Thanks for doing the requested changes and answering all the comments =)

nasdf added 3 commits September 25, 2024 08:51

add upsert mutation definition

02f8672

add upsert planner node. add integration tests

f84cd96

add explain tests

a3a8d70

nasdf added feature New feature or request area/query Related to the query component labels Sep 25, 2024

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

nasdf self-assigned this Sep 25, 2024

fix doc typo

730b1ed

nasdf requested a review from a team September 25, 2024 20:27

shahzadlone reviewed Sep 26, 2024

View reviewed changes

islamaliev reviewed Sep 26, 2024

View reviewed changes

pr fixes

a75c62f

nasdf requested review from shahzadlone and islamaliev September 26, 2024 17:29

islamaliev approved these changes Sep 26, 2024

View reviewed changes

fredcarle approved these changes Sep 27, 2024

View reviewed changes

shahzadlone approved these changes Sep 27, 2024

View reviewed changes

Merge branch 'develop' into nasdf/feat/gql-upsert-mutation

2f94014

nasdf merged commit 88972df into sourcenetwork:develop Sep 27, 2024
41 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: GraphQL upsert mutation #3075

feat: GraphQL upsert mutation #3075

nasdf commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

shahzadlone left a comment •

edited

Loading

islamaliev left a comment

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

shahzadlone Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024

islamaliev Sep 26, 2024

nasdf Sep 26, 2024 •

edited

Loading

islamaliev left a comment

fredcarle left a comment

shahzadlone left a comment

		@@ -0,0 +1,185 @@
		// Copyright 2022 Democratized Data Foundation

feat: GraphQL upsert mutation #3075

feat: GraphQL upsert mutation #3075

Conversation

nasdf commented Sep 25, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

shahzadlone left a comment • edited Loading

Choose a reason for hiding this comment

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

nasdf Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

islamaliev 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

nasdf commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

shahzadlone left a comment •

edited

Loading

nasdf Sep 26, 2024 •

edited

Loading