feat: Add SchemaPatch CLI command #1250

jsimnz · 2023-03-29T07:56:44Z

Relevant issue(s)

Resolves #1209

Description

A fairly straightforward implementation of the PatchSchema CLI command. Defined as defradb client schema patch. Has almost identical structure and semantics as schema add, including support for file, stdin, and raw string inputs.

Updated the API handlers as well to expose db.PatchSchema, again mostly a carbon copy of the existing load schema endpoint.

Made a judgment call on the name schema patch instead of something like schema update. I have no strong opinion here.

I don't think its worth it yet, but since this is the first API endpoint to have a common prefix, there is an argument to be made for making a sub router, but things are still pretty simple, so didn't bother going down that route 🙃 (pun intended).

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?

Manually, no CLI tests at the moment.

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

Ubuntu WSL2 on Windows

shahzadlone

Mostly looks good. Some questions/comments:

nitpick: Slight preference for schema update over schema patch.
nitpick: Slight preference for sub-router.
If no schema is loaded and you try to do an update, here is the output to the user:
Command: $ defradb client schema patch -f=examples/schema/patch/user-add-address.patch.json

Result:

"errors": [
    {
      "message": "add operation does not apply: doc is missing path: \"/user/Schema/Fields/-\": missing value",
      "extensions": {
        "status": 500,
        "httpError": "Internal Server Error"
      }
    }
  ]

question: Is it really an internal error? seems more like invalid usage / user error? perhaps StatusBadRequest.

Giving LGTM as these are somewhat non-blocking.

islamaliev

LGTM

orpheuslummis · 2023-03-29T13:04:05Z

note that this command wil also be taken up in #928 to be changed slightly and tested.

orpheuslummis · 2023-03-29T13:21:52Z

examples/schema/patch/user-add-address.patch.json

@@ -0,0 +1,3 @@
+[
+    { "op": "add", "path": "/user/Schema/Fields/-", "value": {"Name": "address", "Kind": "String"} }


suggestion: upper case the type

This is to match the existing example folder types. I can update, just requires updating in both locations.

orpheuslummis · 2023-03-29T13:24:34Z

note: the endpoint expects a list of objects, e.g.
[{ "op": "add", "path": "/User/Schema/Fields/-", "value": {"Name": "newfield", "Kind": "String"} }]
when given a single object e.g.
{ "op": "add", "path": "/User/Schema/Fields/-", "value": {"Name": "newfield", "Kind": "String"} }
it errors
with error 500 json: cannot unmarshal object into Go value of type jsonpatch.Patch
this is not a problem with this PR but rather with the endpoint

fredcarle · 2023-03-29T16:15:48Z

this is not a problem with this PR but rather with the endpoint

@orpheuslummis, JSON Patch expects an array of operations so it has to be an array and it's only being decode in the db.patchSchema function. I'm not sure we want to deal with that at the moment.

nitpick: Slight preference for schema update over schema patch.

@shahzadlone, I prefer schema patch as this is more representative of what is happening and what the payload needs to be.

nitpick: Slight preference for sub-router.

@shahzadlone, we have 8 routes, two of which have the same prefix. Let's wait until we have a good reason for sur-routers before adding complexity. 2 matching prefixes is definitely not a good reason 😅.

question: Is it really an internal error? seems more like invalid usage / user error? perhaps StatusBadRequest

It is an invalid usage but to get an appropriate error type we would need a way to flag a user error. @jsimnz probably by wrapping the error of patch.Apply? Most of the possible errors in that function would be internal errors. So we would need to check the error type and if it's from patch.Apply, we could return StatusBadRequest instead.

AndrewSisley · 2023-03-29T16:46:19Z

Made a judgment call on the name schema patch instead of something like schema update.

I have a preference for patch, as JSON patches do not necessarily update the schema even currently (e.g. test, empty arrays, operation series with a net-zero outcome). And in the future they could also be used to add brand new schema, which doesn't quite fit within update.

AndrewSisley

Looks good, got a couple of minor suggestions that would be good to get addressed before merge, but nothing complicated so don't wait on me once you either done them or pushed back against them :)

AndrewSisley · 2023-03-29T16:49:10Z

cli/schema_patch.go

+	Short: "Update an existing schema type",
+	Long: `Update an existing schema.
+
+Uses JSON PATCH formatting as an update DDL.


suggestion: I think update is slightly inappropriate here (is not always an update), and on this line it could simply be omitted.

Similar thoughts on the use of the word update below, there may be better words to use (maybe just use patch?

AndrewSisley · 2023-03-29T16:52:22Z

cli/schema_patch.go

+			if err = cmd.Usage(); err != nil {
+				return err
+			}
+			return errors.New("too many arguments")


todo: These errors should be defined in errors.go. I think the CLI package might still only be partially migrated, but new stuff should be done in the way we want longer-term.

AndrewSisley · 2023-03-29T16:57:24Z

examples/schema/patch/user-add-address.patch.json

@@ -0,0 +1,3 @@
+[
+    { "op": "add", "path": "/user/Schema/Fields/-", "value": {"Name": "address", "Kind": "String"} }
+]


nitpick: (as this is a 'public' example), should really end the file with a line-break 😅

jsimnz · 2023-03-29T20:27:51Z

@orpheuslummis, JSON Patch expects an array of operations so it has to be an array and it's only being decode in the db.patchSchema function. I'm not sure we want to deal with that at the moment.

Indeed this is a discussion that needs to be had about the Patch system in general. If we want to be able to support single ops as a JSON object, instead of always requiring an array of ops, even for a single. My intuituin says it should always be an array for simplicity, but it might be nicer DX to allow objects for single ops.

RE: @shahzadlone @fredcarle BadRequest

I overall agree with you Shahzad about the use of InternalServerError here, but as Fred noted, the way the errors are structured we don't really know if it is a bad request or a server error. Its not ideal but I would prefer to leave as is and we can address the nature of errors bubbling up to user (cli or http) in a diff PR.

codecov · 2023-03-29T20:39:23Z

Codecov Report

Merging #1250 (7fea2c1) into develop (9e090b2) will decrease coverage by 0.39%.
The diff coverage is 3.27%.

❗ Current head 7fea2c1 differs from pull request most recent head cfd9a45. Consider uploading reports for the commit cfd9a45 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1250      +/-   ##
===========================================
- Coverage    70.56%   70.17%   -0.39%     
===========================================
  Files          182      183       +1     
  Lines        17251    17343      +92     
===========================================
- Hits         12173    12171       -2     
- Misses        4140     4241     +101     
+ Partials       938      931       -7

Impacted Files	Coverage Δ
api/http/handlerfuncs.go	`77.44% <0.00%> (-8.00%)`	⬇️
cli/errors.go	`0.00% <0.00%> (ø)`
cli/schema_patch.go	`3.22% <3.22%> (ø)`
api/http/router.go	`100.00% <100.00%> (ø)`

... and 8 files with indirect coverage changes

islamaliev · 2023-04-05T13:09:11Z

tested this one by applying different patches using inline json strings and json files.

* Added initial cli command for schema patch * Added PatchSchema API handler and route, updated CLI to use new endpoint * Added an example PATCH to the 'examples' folder * Updated errors * Updated examples to be more consistent * Added nolint err checks to useless err checks that were removed

jsimnz added feature New feature or request action/no-benchmark Skips the action that runs the benchmark. labels Mar 29, 2023

jsimnz added this to the DefraDB v0.5 milestone Mar 29, 2023

jsimnz self-assigned this Mar 29, 2023

jsimnz requested review from orpheuslummis and AndrewSisley March 29, 2023 07:56

jsimnz added the area/cli Related to the CLI binary label Mar 29, 2023

shahzadlone approved these changes Mar 29, 2023

View reviewed changes

islamaliev approved these changes Mar 29, 2023

View reviewed changes

orpheuslummis reviewed Mar 29, 2023

View reviewed changes

AndrewSisley approved these changes Mar 29, 2023

View reviewed changes

jsimnz added 6 commits March 29, 2023 17:28

Added initial cli command for schema patch

3bcf884

Added PatchSchema API handler and route, updated CLI to use new endpoint

24afbee

Added an example PATCH to the 'examples' folder

9f5b4b9

Updated errors

d7fbea4

Updated examples to be more consistent

31200a4

Added nolint err checks to useless err checks that were removed

cfd9a45

jsimnz force-pushed the jsimnz/feat/I1209-patch-schema-cli branch from 7fea2c1 to cfd9a45 Compare March 29, 2023 21:28

jsimnz merged commit 706e55d into develop Mar 30, 2023

jsimnz deleted the jsimnz/feat/I1209-patch-schema-cli branch March 30, 2023 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SchemaPatch CLI command #1250

feat: Add SchemaPatch CLI command #1250

jsimnz commented Mar 29, 2023

shahzadlone left a comment

islamaliev left a comment

orpheuslummis commented Mar 29, 2023

orpheuslummis Mar 29, 2023

jsimnz Mar 29, 2023

orpheuslummis commented Mar 29, 2023

fredcarle commented Mar 29, 2023

AndrewSisley commented Mar 29, 2023

AndrewSisley left a comment

AndrewSisley Mar 29, 2023

jsimnz Mar 29, 2023

AndrewSisley Mar 29, 2023 •

edited

Loading

jsimnz Mar 29, 2023

AndrewSisley Mar 29, 2023

jsimnz Mar 29, 2023

jsimnz commented Mar 29, 2023

codecov bot commented Mar 29, 2023 •

edited

Loading

islamaliev commented Apr 5, 2023

		@@ -0,0 +1,3 @@
		[
		{ "op": "add", "path": "/user/Schema/Fields/-", "value": {"Name": "address", "Kind": "String"} }

feat: Add SchemaPatch CLI command #1250

feat: Add SchemaPatch CLI command #1250

Conversation

jsimnz commented Mar 29, 2023

Relevant issue(s)

Description

Tasks

How has this been tested?

shahzadlone left a comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

orpheuslummis commented Mar 29, 2023

orpheuslummis Mar 29, 2023

Choose a reason for hiding this comment

jsimnz Mar 29, 2023

Choose a reason for hiding this comment

orpheuslummis commented Mar 29, 2023

fredcarle commented Mar 29, 2023

AndrewSisley commented Mar 29, 2023

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley Mar 29, 2023

Choose a reason for hiding this comment

jsimnz Mar 29, 2023

Choose a reason for hiding this comment

AndrewSisley Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

jsimnz Mar 29, 2023

Choose a reason for hiding this comment

AndrewSisley Mar 29, 2023

Choose a reason for hiding this comment

jsimnz Mar 29, 2023

Choose a reason for hiding this comment

jsimnz commented Mar 29, 2023

codecov bot commented Mar 29, 2023 • edited Loading

Codecov Report

islamaliev commented Apr 5, 2023

AndrewSisley Mar 29, 2023 •

edited

Loading

codecov bot commented Mar 29, 2023 •

edited

Loading