Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite the json-patch CLI tool to be more robust #154

Closed
wants to merge 11 commits into from

Conversation

JeffFaer
Copy link

  • It can now generate RFC7396 merge patches with the diff command
  • It can now apply RFC7396 merge patches in addition to RFC6902 patches with the patch command.

I did some light manual testing (see below). The only particularly
things to note:

  • The error messages could maybe be a little better (for instance
    "cannot unmarshal object into Go value of type jsonpatch.Patch" could
    mention that the user might want to tweak the --patch-format flag
    somehow)
  • RFC6902 patches can be applied with RFC7396 logic. I'm not sure if
    this is intended or not, but it mildly surprised me.
$ cat foo.json
{
    "foo": [1, 2, 3],
    "baz": 1,
    "obj": {
        "1": null,
        "2": 4,
        "3": "foo"
    }
}
$ cat bar.json
{
    "foo": [1, 2, 4, 5],
    "baz": 1,
    "obj": {
        "1": null,
        "2": 3
    }
}
$ json-patch diff foo.json bar.json
{"foo":[1,2,4,5],"obj":{"2":3,"3":null}}
$ json-patch patch -p <(json-patch diff foo.json bar.json) <<< '{"obj": {"1": 2, "3": 99}}'
Error: could not decode patch #0: json: cannot unmarshal object into Go value of type jsonpatch.Patch
$ json-patch patch -p <(json-patch diff foo.json bar.json) -f RFC7396
<<< '{"obj": {"1": 2, "3": 99}}'
{"foo":[1,2,4,5],"obj":{"1":2,"2":3}}
$ cat orig.json
{
    "name": "John",
    "age": 24,
    "height": 3.21
}
$ cat patch.json
[
    {"op": "replace", "path": "/name", "value": "Jane"},
    {"op": "remove", "path": "/height"}
]
$ json-patch patch -p patch.json orig.json
{"age":24,"name":"Jane"}
$ json-patch patch -p patch.json orig.json -f RFC7396
[{"op":"replace","path":"/name","value":"Jane"},{"op":"remove","path":"/height"}]

@evanphx
Copy link
Owner

evanphx commented Dec 13, 2021

Hi @JeffreyFalgout,

So my big concern is that this changes how the tool can be used, so any existing users will have errors thrown. Since you have a bunch of new functionality, perhaps it should split out to it's own repo entirely?

@JeffFaer
Copy link
Author

Yeah, this patch as it is isn't reverse compatible :/

My big hesitation with splitting this into a separate repository is that now we'd have two competing tools doing the same thing. What if I updated my changes so that it was reverse compatible? i.e. invoking json-patch without create or apply would do the same thing that the tool does today

Then we can have these improvements to the CLI, without breaking pre-existing uses of the tool. We could maybe even add a deprecation warning encouraging users to use the new apply sub-command

@JeffFaer
Copy link
Author

JeffFaer commented Jan 6, 2022

Oops, looks like I forgot to comment after my most recent changes: I made the command reverse compatible. You should be able to call the existing tool and the new tool the same way. The new tool has the additional functionality I mentioned at the top

@JeffFaer
Copy link
Author

JeffFaer commented Jan 6, 2022

Although I'm not sure if I touched the right files. Should I have updated the v5/ files instead?
#150 (comment)

I saw the v5 directory already had module files, but I'm not sure why
there weren't any others.

I separated cmd/json-patch into its own module since the CLI application
has dependencies that the library doesn't need.
  - It can now generate RFC7396 merge patches with the diff command
  - It can now apply RFC7396 merge patches in addition to RFC6902
patches with the patch command.

I did some light manual testing (see below). The only particularly
things to note:
  - The error messages could maybe be a little better (for instance
"cannot unmarshal object into Go value of type jsonpatch.Patch" could
mention that the user might want to tweak the --patch-format flag
somehow)
  - RFC6902 patches can be applied with RFC7396 logic. I'm not sure if
this is intended or not, but it mildly surprised me.

```
$ cat foo.json
{
    "foo": [1, 2, 3],
    "baz": 1,
    "obj": {
        "1": null,
        "2": 4,
        "3": "foo"
    }
}
$ cat bar.json
{
    "foo": [1, 2, 4, 5],
    "baz": 1,
    "obj": {
        "1": null,
        "2": 3
    }
}
$ json-patch diff foo.json bar.json
{"foo":[1,2,4,5],"obj":{"2":3,"3":null}}
$ json-patch patch -p <(json-patch diff foo.json bar.json) <<< '{"obj": {"1": 2, "3": 99}}'
Error: could not decode patch #0: json: cannot unmarshal object into Go value of type jsonpatch.Patch
$ json-patch patch -p <(json-patch diff foo.json bar.json) -f RFC7396
<<< '{"obj": {"1": 2, "3": 99}}'
{"foo":[1,2,4,5],"obj":{"1":2,"2":3}}
```

```
$ cat orig.json
{
    "name": "John",
    "age": 24,
    "height": 3.21
}
$ cat patch.json
[
    {"op": "replace", "path": "/name", "value": "Jane"},
    {"op": "remove", "path": "/height"}
]
$ json-patch patch -p patch.json orig.json
{"age":24,"name":"Jane"}
$ json-patch patch -p patch.json orig.json -f RFC7396
[{"op":"replace","path":"/name","value":"Jane"},{"op":"remove","path":"/height"}]
```

fixes evanphx#153
These new verbs seem to read better to me. "json-patch create" and
"json-patch apply" vs "json-patch diff" and "json-patch patch".
The root json-patch command should now behave the same as it used to.
The only difference between the tools was the json-patch import path:
$ diff cmd/json-patch/main.go v5/cmd/json-patch/
file_flag.go  main.go
$ diff cmd/json-patch/main.go v5/cmd/json-patch/main.go
9c9
< 	jsonpatch "github.com/evanphx/json-patch"
---
> 	jsonpatch "github.com/evanphx/json-patch/v5"
$ diff cmd/json-patch/file_flag.go v5/cmd/json-patch/file_flag.go

So I just copied the files over as-is then post-edited to change the
import paths again.
@JeffFaer
Copy link
Author

JeffFaer commented Jan 6, 2022

Updated the PR to touch both 🤷

@evanphx
Copy link
Owner

evanphx commented Jan 6, 2022

Hi @JeffreyFalgout, I think we should probably just delete the v5/cmd directory entirely now that cmd is it's own go module. I highly doubt anyone was using that, and they can easily just switch to the non-v5 prefix.

After that, it looks good!

Instead, use the top-level cmd/ directory.

Point cmd/json-patch to the v5/ version of the library instead of the
top-level version. It looks like updates are being applied only to v5/
instead of both v5/ and the top-level version. We should use the most
up-to-date version of the library in the command line tool to prevent
user confusion (this should fix evanphx#150).
@JeffFaer
Copy link
Author

JeffFaer commented Jan 6, 2022

Done! That should also end up fixing #150 since I changed the cmd/ directory to use the v5/ version of json-patch now, as well.

@evanphx
Copy link
Owner

evanphx commented Jan 7, 2022

One last thing, in the past the presence of go.mod in the top-level dir made Go versions confused. I don't recall the exact reason, but since we're not using them anymore (since the cmd's go.mod pulls in v5) let's go ahead and just delete them.

cmd/json-patch is referencing v5/ now, so we don't strictly need these
files anymore.
@JeffFaer
Copy link
Author

JeffFaer commented Jan 8, 2022

Sure. I removed those files. I'm curious what problems you ran into, though

@JeffFaer
Copy link
Author

Friendly ping :)

@evanphx
Copy link
Owner

evanphx commented Sep 11, 2023

Hi @JeffreyFalgout,

I'm sorry about the extremely long delay here. I've decided your code is best as it's own package. I'm strongly considering deleting the existing cmd/ entirely, so consider it fully unmaintained (the rest of the package is fully maintained).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants