feat: Allow relation alias on create and update #1609

shahzadlone · 2023-07-04T19:03:17Z

Relevant issue(s)

Resolves #1279

Description

Here are the main tasks that were done in this PR:

Add some tests that are might not be directly related
Before implementation add more tests for non-alias behavior while creating document.
Add implementation for aliasing on related object link while creating document.
Add 1-1 tests for aliasing on related object while creating document.
Add 1-M tests for aliasing on related object while creating document.
Before implementation add tests for non-alias behavior while updating document.
Add implementation for aliasing on related object link while updating document.
Add 1-1 tests for aliasing on related object while updating document.
Add 1-M tests for aliasing on related object while updating document.

In addition, I had to resolve a circular dependency but making the core/cid.go stuff it's own modular package. Refactored manual uses of cid.Prefix and dockey generation so that it's consistent.

For Reviewers

This commit PR(FIX): Squash the dockey verification + diff bug undoes some work in the commit PR(CORE): Implement the alias logic for create to fix a bug. Other than that should be "cleanish" history.

How has this been tested?

Integration tests.

codecov · 2023-07-04T19:12:27Z

Codecov Report

Patch coverage: 79.07% and project coverage change: +0.09 🎉

Comparison is base (ce28685) 75.56% compared to head (48e3f45) 75.65%.

@@             Coverage Diff             @@
##           develop    #1609      +/-   ##
===========================================
+ Coverage    75.56%   75.65%   +0.09%     
===========================================
  Files          199      200       +1     
  Lines        20769    20808      +39     
===========================================
+ Hits         15693    15741      +48     
+ Misses        3997     3990       -7     
+ Partials      1079     1077       -2

Flag	Coverage Δ
all-tests	`75.65% <79.07%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
client/errors.go	`57.14% <ø> (-2.86%)`	⬇️
planner/create.go	`65.66% <57.14%> (-2.05%)`	⬇️
db/collection.go	`70.62% <70.00%> (-0.29%)`	⬇️
client/document.go	`66.24% <73.91%> (+0.76%)`	⬆️
client/request/errors.go	`100.00% <100.00%> (ø)`
client/request/select.go	`100.00% <100.00%> (ø)`
core/cid/cid.go	`100.00% <100.00%> (ø)`
db/collection_update.go	`76.14% <100.00%> (+3.24%)`	⬆️
db/errors.go	`87.74% <100.00%> (+0.16%)`	⬆️

... and 8 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 ce28685...48e3f45. Read the comment docs.

shahzadlone · 2023-07-13T18:19:58Z

db/collection.go

+			fieldKey, isAliasKey, fieldExists := c.tryGetFieldKey(primaryKey, k)
+			// Overwrite the aliased key with the internal related object name.
+			if isAliasKey {
+				oldKey := k
+				k = oldKey + request.RelatedObjectID
+				doc.Fields()[k] = v
+				delete(doc.Fields(), oldKey)


tip-for-reviewer: This implementation and some other parts or this commit were removed and implemented elsewhere while fixing a bug in the commit [PR(FIX): Squash the dockey verification + diff bug]

fredcarle

Great work here Shahzad! Very nicely executed.

fredcarle · 2023-07-13T22:07:37Z

core/cid/cid.go

question: Should the date be changed to 2023?

It's fine IMO

fredcarle · 2023-07-14T16:34:34Z

db/collection.go

@@ -875,6 +866,7 @@ func (c *collection) save(

 		if val.IsDirty() {
 			fieldKey, fieldExists := c.tryGetFieldKey(primaryKey, k)
+


question: Was this extra line accidental?

It's when I undid the changes mentioned in the PR description lol.

fredcarle · 2023-07-14T16:47:21Z

tests/integration/mutation/one_to_many/update/related_object_link_test.go

+				Request: fmt.Sprintf(
+					`mutation {
+ 						update_Book(id: "%s", data: "{\"author_id\": \"%s\"}") {
+ 							name
+ 						}
+ 					}`,
+					bookKey,
+					invalidAuthorKey,


thought (Out of scope): This should probably return an error no document for the given key exists like it does on the 1-to-1.

Yes I see a few tests before this PR that note adding that in. Not sure if there is a ticket for that yet.

Undo some changes that aren't needed with this approach.

modularize generation of cid such that it can be used in a consistent manner. i.e. blockstore or in client package

islamaliev · 2023-07-20T12:06:30Z

bug bash results:
ran locally a defradb node with Author, Book, Article and others.
Using Altair GraphLQ client ran different create and update mutations with and without aliases. Worked fine.
Also ran queries with filters. Worked fine.

## Relevant issue(s) Resolves sourcenetwork#1279 ## Description Here are the main tasks that were done in this PR: - [x] Add some tests that are might not be directly related - [x] Before implementation add more tests for non-alias behavior while creating document. - [x] Add implementation for aliasing on related object link while creating document. - [x] Add 1-1 tests for aliasing on related object while creating document. - [x] Add 1-M tests for aliasing on related object while creating document. - [x] Before implementation add tests for non-alias behavior while updating document. - [x] Add implementation for aliasing on related object link while updating document. - [x] Add 1-1 tests for aliasing on related object while updating document. - [x] Add 1-M tests for aliasing on related object while updating document. In addition, I had to resolve a circular dependency but making the `core/cid.go` stuff it's own modular package. Refactored manual uses of `cid.Prefix` and dockey generation so that it's consistent.

shahzadlone added the feature New feature or request label Jul 4, 2023

shahzadlone added this to the DefraDB v0.6 milestone Jul 4, 2023

shahzadlone self-assigned this Jul 4, 2023

shahzadlone force-pushed the feat/alias-mutation-relation branch from ff11461 to 9551e71 Compare July 13, 2023 17:59

shahzadlone marked this pull request as ready for review July 13, 2023 18:04

shahzadlone requested a review from a team July 13, 2023 18:09

shahzadlone added the area/query Related to the query component label Jul 13, 2023

shahzadlone commented Jul 13, 2023

View reviewed changes

shahzadlone force-pushed the feat/alias-mutation-relation branch from 41c4738 to 6eb9499 Compare July 13, 2023 19:34

fredcarle approved these changes Jul 14, 2023

View reviewed changes

shahzadlone added 19 commits July 17, 2023 14:39

PR(MAKEFILE): Add a testing rule make test:quick

00c2afd

PR(UNRELATED): Test filtering on object id.

1a306ee

PR(CORE): Implement the alias logic for create

71c40ff

PR(TEST): Update the collection test according to alias change.

06fd941

PR(TEST): Add some missing tests.

64ef15a

PR(TEST): Add 1-1 tests for create with alias

f65d95c

PR(UNRELATED): Make a fixture for existing 1-to-m

5cf449d

PR(TEST): Add 1-M tests for create without alias

72740e9

PR(TEST): Add 1-M tests for create with alias

d0d3bc0

PR(FIX): Squash the dockey verification + diff bug

3c2331d

Undo some changes that aren't needed with this approach.

PR(FIX): Avoid circular dependency (import cycle).

7e5b3c6

PR(REFACTOR): private helper to regenerate dockey.

102aa4f

PR(REFAC): Remove circular dependency issue...

5913bae

modularize generation of cid such that it can be used in a consistent manner. i.e. blockstore or in client package

PR(REFAC): Circular dependency fixed so refactor.

cadeb99

PR(TEST): Add 1-1 tests for update without alias

caf4319

PR(TEST): Add 1-M tests for update without alias

4987730

PR(CORE): Implement the alias logic for update

46022a6

PR(TEST): Add 1-M tests for update with alias

1bf89be

PR(TEST): Add 1-1 tests for update with alias

516bad7

PR(LINT-FIX): Make linter happy.

48e3f45

shahzadlone force-pushed the feat/alias-mutation-relation branch from 6eb9499 to 48e3f45 Compare July 17, 2023 18:39

shahzadlone mentioned this pull request Jul 17, 2023

test: Remove CLI tests from make test #1643

Merged

shahzadlone merged commit f02e2cd into sourcenetwork:develop Jul 17, 2023
10 checks passed

shahzadlone deleted the feat/alias-mutation-relation branch July 17, 2023 19:04

shahzadlone mentioned this pull request Jan 9, 2024

feat: Mutation typed input #2167

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow relation alias on create and update #1609

feat: Allow relation alias on create and update #1609

shahzadlone commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading

shahzadlone Jul 13, 2023

fredcarle left a comment

fredcarle Jul 13, 2023

shahzadlone Jul 17, 2023

fredcarle Jul 14, 2023

shahzadlone Jul 17, 2023

fredcarle Jul 14, 2023

shahzadlone Jul 17, 2023

islamaliev commented Jul 20, 2023

		@@ -875,6 +866,7 @@ func (c *collection) save(

		if val.IsDirty() {
		fieldKey, fieldExists := c.tryGetFieldKey(primaryKey, k)

feat: Allow relation alias on create and update #1609

feat: Allow relation alias on create and update #1609

Conversation

shahzadlone commented Jul 4, 2023 • edited Loading

Relevant issue(s)

Description

For Reviewers

How has this been tested?

codecov bot commented Jul 4, 2023 • edited Loading

Codecov Report

shahzadlone Jul 13, 2023

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle Jul 13, 2023

Choose a reason for hiding this comment

shahzadlone Jul 17, 2023

Choose a reason for hiding this comment

fredcarle Jul 14, 2023

Choose a reason for hiding this comment

shahzadlone Jul 17, 2023

Choose a reason for hiding this comment

fredcarle Jul 14, 2023

Choose a reason for hiding this comment

shahzadlone Jul 17, 2023

Choose a reason for hiding this comment

islamaliev commented Jul 20, 2023

shahzadlone commented Jul 4, 2023 •

edited

Loading

codecov bot commented Jul 4, 2023 •

edited

Loading