feat: Add PatchCollection #2402

AndrewSisley · 2024-03-11T19:41:48Z

Relevant issue(s)

Resolves #2389

Description

Adds the PatchCollection command.

Mutating anything but the collection name is currently disabled, we can expand this as we see fit, but for now I'd prefer to keep the initial PR small.

This change means that the Collection Name is no longer always going to be the same as the Schema Name.

I've manually tested the OpenApi stuff via playground.

This was in the wrong place, the folders are named after patch actions, and 'index' is not a patch action

codecov · 2024-03-11T20:02:34Z

Codecov Report

Attention: Patch coverage is 81.13208% with 60 lines in your changes are missing coverage. Please review.

Project coverage is 75.13%. Comparing base (d524320) to head (6336395).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2402      +/-   ##
===========================================
+ Coverage    74.99%   75.13%   +0.14%     
===========================================
  Files          268      269       +1     
  Lines        26017    26334     +317     
===========================================
+ Hits         19511    19785     +274     
- Misses        5181     5213      +32     
- Partials      1325     1336      +11

Flag	Coverage Δ
all-tests	`75.13% <81.13%> (+0.14%)`	⬆️

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

Files	Coverage Δ
cli/cli.go	`100.00% <100.00%> (ø)`
cli/schema_patch.go	`66.18% <100.00%> (ø)`
db/errors.go	`67.53% <100.00%> (+6.44%)`	⬆️
http/handler_store.go	`84.25% <85.19%> (+0.05%)`	⬆️
db/txn_db.go	`63.37% <64.29%> (+0.07%)`	⬆️
http/client.go	`52.88% <53.85%> (+0.04%)`	⬆️
cli/collection_patch.go	`68.18% <68.18%> (ø)`
db/description/collection.go	`51.11% <44.44%> (-1.18%)`	⬇️
db/collection.go	`73.23% <88.57%> (+2.16%)`	⬆️

... 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 d524320...6336395. Read the comment docs.

shahzadlone · 2024-03-13T05:15:29Z

cli/collection_patch.go

@@ -0,0 +1,68 @@
+// Copyright 2023 Democratized Data Foundation


todo:

Suggested change

// Copyright 2023 Democratized Data Foundation

// Copyright 2024 Democratized Data Foundation

😁 Will do

2024

shahzadlone · 2024-03-13T05:21:44Z

cli/collection_patch.go

+			case len(args) >= 1:
+				patch = args[0]


question: In what case can len(args) > 1?

I don't really care :) But I've added a param limit, arg length is now 0 or 1.

shahzadlone · 2024-03-13T05:23:39Z

client/db.go

+	//
+	// It will also update the GQL types used by the query system. It will error and not apply any of the
+	// requested, valid updates should the net result of the patch result in an invalid state.  The
+	// individual operations defined in the patch do not need to result in a valid state, only the net result


question: what is meant by valid state here.

Suggested change

// individual operations defined in the patch do not need to result in a valid state, only the net result

// individual operations defined in the patch do not need to result in a `valid state`, only the net result

A valid CollectionDescription mutation, at the moment that means one that only changes the name (or tests, or does nothing).

shahzadlone · 2024-03-13T05:27:11Z

client/db.go

+	// It will also update the GQL types used by the query system. It will error and not apply any of the
+	// requested, valid updates should the net result of the patch result in an invalid state.  The


todo: This sentence reads very odd to me "It will error and not apply any of the requested, valid updates should the net result of the patch result in an invalid state."

Please reword.

What is odd about it? Do you have a rough alternative in mind?

I think it is important to highlight that the full patch will be rolled back if an given patch item fails - especially given that SQLs do not do this and require manual rollback when given multiple DDL statements.

This sounds more clear when you said: full patch will be rolled back if an given patch item fails

This is very wordy: ... apply any of the requested, valid updates should the net result...

rolled back is incorrect/lazy lol - the patches are aggregated before being applied - hence the use of net result of the patch

The individual operations defined in the patch do not need to result in a valid state, only the net result of the full patch.

I though I understood the bahaviour from the prior sentence but this one gets me confused. How can the net result be valid if an individual operation creates an invalid state?

the patches are aggregated before being applied

This will be nice to document somewhere too

I though I understood the bahaviour from the prior sentence but this one gets me confused. How can the net result be valid if an individual operation creates an invalid state?

There are many reasons why we want this to be, and many of the tests cover this, for example:

{ "op": "copy", "from": "/Users/Fields/1", "path": "/Users/Fields/2" },
{ "op": "replace", "path": "/Users/Fields/2/Name", "value": "age" },

The first field is copied (invalid as it is now a duplicate), and then renamed (net is now valid). This allows existing stuff (including full schema/collections) to be used as templates. There are other use cases too.

This will be nice to document somewhere too

It is, we are talking about the documentation that currently documents it.

shahzadlone · 2024-03-13T05:28:47Z

db/collection.go

 	"strconv"
 	"strings"

+	jsonpatch "github.com/evanphx/json-patch/v5"


question: I forgot if we cared about camel casing package import alias names. If we do then jsonPatch?

I don't know either, my IDE did this, and I don't see it as being unreadable

shahzadlone · 2024-03-13T05:40:28Z

db/collection.go

@@ -526,6 +529,283 @@ func validateUpdateSchemaFields(
 	return hasChanged, nil
 }

+func (db *db) patchCollection(


suggestion: I would much rather than patch stuff be moved to a new file, like: db/collection_patch.go

All of core functions to mutating schema and collections currently live in this file, I agree that it should be reorganized, but not in this PR.

I've opened a ticket: #2407

shahzadlone · 2024-03-13T05:47:38Z

db/collection.go

+			continue
+		}
+
+		// DeepEqual is temporary, as this validation is temporary


question: Why is this temporary? Will the use of reflection package be removed?

The validation (function) is temporary, which means that if this function is removed, the call to DeepEqual will be removed with it.

The validation (function) is temporary, which means that if this function is removed, the call to DeepEqual will be removed with it.

Why is it temporary?

Because the function is validating that indexes have not been modified, and we very much want them to be mutable in the future.

Thanks that makes so much sense.

Please document this

Please document this

It is.

shahzadlone · 2024-03-13T05:49:28Z

db/collection.go

+	for _, oldCol := range oldColsByID {
+		for _, newCol := range newColsByID {
+			// It is not enough to just match by the map index, in case the index does not pair
+			// up with the ID (this can happen if a user moves it)


question: moves it moves what?

The collection relative to it's index in the map, I'll replace it

Replace it

shahzadlone · 2024-03-13T05:53:17Z

tests/integration/collection_description/updates/add/collections_test.go

+						{ "op": "add", "path": "/2", "value": {"Name": "Dogs"} }
+					]
+				`,
+				ExpectedError: "collection ID cannot be zero",


Would add another test where it is explicitly 0:
{ "op": "add", "path": "/2", "value": {"ID": 0, "Name": "Dogs"} }

Will add

Add explicit zero add test

shahzadlone · 2024-03-13T05:55:50Z

tests/integration/test_case.go

+	// If a value is not provided the patch will be applied to all nodes.
+	NodeID immutable.Option[int]
+
+	Patch string


todo: document please

My bad :) Of course :)

Doc test action Patch prop

nasdf

One minor fix from me.

nasdf · 2024-03-13T19:29:46Z

cli/collection_patch.go

+			return store.PatchCollection(cmd.Context(), patch)
+		},
+	}
+	cmd.Flags().StringVarP(&patchFile, "patch-file", "p", "", "File to load a patch from")


todo: This flag doesn't match the example above.

Nice catch, copy-pasted issue from PatchSchema, I've fixed both.

Don't all other file consumption commands use --file |-f why do we want to have a different flag name for this arg (does it clash with -f somewhere?)

PatchSchema uses p because it it takes two kinds of files. I think it is more important to be consistent with PatchSchema than anywhere else here.

How does it take two files? I see 1 so far with -p

PatchSchema takes two files, I assume you are looking at PatchCollection

Maybe we should change the flag for patch schema to also be -f and make it consistent throughout. The lens file can stay with -t or maybe -l would make more sense.

f and file is ambiguous for a command that takes two different files. I do not like that.

PatchSchema used to be f, but when the lens stuff got added it was changed to avoid this confusion.

@nasdf your opinion would be appreciated here when you have a chance.

I think its fine to keep it -p for consistency with the other patch method. Using -f when there are multiple files is worse in my opinion.

fredcarle

Just a few comments to resolve before approval :)

fredcarle · 2024-03-14T02:15:05Z

cli/collection_patch.go

+func MakeCollectionPatchCommand() *cobra.Command {
+	var patchFile string
+	var cmd = &cobra.Command{
+		Use:   "patch [patch]",


thought: I've notice that is some commands, the optional flags aren't shown in the Use field. I'm wondering if we should start showing all the optional flags here.

fredcarle · 2024-03-14T02:18:28Z

client/db.go

+	// It will also update the GQL types used by the query system. It will error and not apply any of the
+	// requested, valid updates should the net result of the patch result in an invalid state.  The


The individual operations defined in the patch do not need to result in a valid state, only the net result of the full patch.

I though I understood the bahaviour from the prior sentence but this one gets me confused. How can the net result be valid if an individual operation creates an invalid state?

fredcarle · 2024-03-14T02:52:26Z

db/collection.go

+			// It is not enough to just match by the map index, in case the index does not pair
+			// up with the ID (this can happen if a user moves the collection within the map)


question: Can you explain how this can be possible?

{ "op": "move", "from": "/1", "path": "/2" }

This moves the collection within the map, without mutating the ID.

Thanks for clarifying.

fredcarle · 2024-03-14T02:58:25Z

tests/integration/collection_description/updates/copy/name_test.go

+						{ "op": "copy", "from": "/1/Name", "path": "/2/Name" },
+						{ "op": "remove", "path": "/1/Name" }


todo: Can you please add a test showing that if only a copy is applied, we will get a duplicated name error or something like that.

I think the value is low, as duplicate names are tested elsewhere, but I will add

add copy name test

fredcarle · 2024-03-14T03:00:16Z

tests/integration/collection_description/updates/replace/id_test.go

+						{ "op": "replace", "path": "/2/ID", "value": 1 }
+					]
+				`,
+				ExpectedError: "collection sources cannot be mutated.",


thought: The error is a little weird here. It's talking about mutating sources when the patch is dealing with the collection ID.

Yes, unfortunately when mutating IDs it becomes impossible for us to tell what the original object was (and that the ID was mutated), so it will often get caught by other validation rules.

shahzadlone

LGTM assuming you will resolve other comments

AndrewSisley added 2 commits March 11, 2024 13:07

Move add field with existing index test

120a387

This was in the wrong place, the folders are named after patch actions, and 'index' is not a patch action

Add PatchCollection

3dcf0ab

AndrewSisley added feature New feature or request area/collections Related to the collections system labels Mar 11, 2024

AndrewSisley requested a review from a team March 11, 2024 19:41

AndrewSisley self-assigned this Mar 11, 2024

AndrewSisley added this to the DefraDB v0.11 milestone Mar 11, 2024

shahzadlone reviewed Mar 13, 2024

View reviewed changes

AndrewSisley added 5 commits March 13, 2024 11:09

PR FIXUP - Limit cli arg length

1005bab

PR FIXUP - Fix license header date

c551abc

PR FIXUP - Replace 'it' in comment

6307753

PR FIXUP - Add explicit zero add test

09327b1

PR FIXUP - Document test action prop

64a10f8

AndrewSisley requested a review from shahzadlone March 13, 2024 16:00

This was referenced Mar 13, 2024

Reorganize db/collection.go #2407

Closed

Move RelationName from schema to collection #2408

Closed

nasdf requested changes Mar 13, 2024

View reviewed changes

PR FIXUP - Fix patch-file CLI documentation

58ee4ef

AndrewSisley requested a review from nasdf March 13, 2024 20:21

fredcarle reviewed Mar 14, 2024

View reviewed changes

shahzadlone approved these changes Mar 14, 2024

View reviewed changes

PR FIXUP - Add copy name (duplicate) test

6336395

AndrewSisley requested a review from fredcarle March 14, 2024 15:42

nasdf approved these changes Mar 14, 2024

View reviewed changes

AndrewSisley merged commit c67bc56 into sourcenetwork:develop Mar 14, 2024
31 of 32 checks passed

AndrewSisley deleted the 2389-patch-col branch March 14, 2024 19:07

		@@ -0,0 +1,68 @@
		// Copyright 2023 Democratized Data Foundation

	// Copyright 2023 Democratized Data Foundation
	// Copyright 2024 Democratized Data Foundation

	// individual operations defined in the patch do not need to result in a valid state, only the net result
	// individual operations defined in the patch do not need to result in a `valid state`, only the net result

		// It will also update the GQL types used by the query system. It will error and not apply any of the
		// requested, valid updates should the net result of the patch result in an invalid state. The

		// It is not enough to just match by the map index, in case the index does not pair
		// up with the ID (this can happen if a user moves the collection within the map)

		{ "op": "copy", "from": "/1/Name", "path": "/2/Name" },
		{ "op": "remove", "path": "/1/Name" }

feat: Add PatchCollection #2402

feat: Add PatchCollection #2402

Conversation

AndrewSisley commented Mar 11, 2024

Relevant issue(s)

Description

codecov bot commented Mar 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

AndrewSisley Mar 13, 2024 • edited Loading

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

AndrewSisley Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Mar 13, 2024 • edited Loading

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

AndrewSisley Mar 13, 2024 • edited Loading

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

AndrewSisley Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

nasdf 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

AndrewSisley Mar 14, 2024 • edited Loading

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

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

AndrewSisley Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 11, 2024 •

edited

Loading

AndrewSisley Mar 13, 2024 •

edited

Loading

AndrewSisley Mar 13, 2024 •

edited

Loading

shahzadlone Mar 13, 2024 •

edited

Loading

AndrewSisley Mar 13, 2024 •

edited

Loading

AndrewSisley Mar 13, 2024 •

edited

Loading

AndrewSisley Mar 13, 2024 •

edited

Loading

AndrewSisley Mar 13, 2024 •

edited

Loading

AndrewSisley Mar 14, 2024 •

edited

Loading

AndrewSisley Mar 14, 2024 •

edited

Loading