Expose JSON Patch operations as public API #1089

ikhoon · 2025-01-14T15:18:08Z

Motivation:

JSON Patch syntax is not easy to write in string. An API will prevent users from writing raw operations in string and help to build JSON Patch operations type-safely.

Modifications:

Move JSON Patch operations to common.jsonpatch from internal.jsonpatch.
- Fix Change.ofJsonPatch() to create JSON patch the JsonPatchOperations.
- Add factory methods to JsonPatchOperation.
Create JsonPatchConflictException and TextPatchConflitException to distinguish exceptions easily and provide a detailed message.
Fix GitRepository to allow an empty message JSON patch because test or testAbsent do not have changes.
- In addition, JSON patch operations have their own validation mechanism.

Result:

Closes Expose JsonPatch as public API #1088
You can now easily create a JSON patch with JsonPatchOperation.

// Add
AddOperation add = JsonPatchOperation.add("/b", new IntNode(2));
// Copy
CopyOperation copy = JsonPatchOperation.copy("/a", "/b");
// Move
MoveOperation move = JsonPatchOperation.move("/a","/b");
// Remove
RemoveOperation remove = JsonPatchOperation.remove("/a");
// Remove if exists 
RemoveIfExistsOperation removeIfExists = JsonPatchOperation.removeIfExists("/a");
// Replace
ReplaceOperation replace = JsonPatchOperation.replace("/a", new IntNode(2));
// Safe replace (aka. compare and set)
SafeReplaceOperation safeReplace =
        JsonPatchOperation.safeReplace("/a", new IntNode(1), new IntNode(2));
// Test if a value exists in a node
TestOperation test = JsonPatchOperation.test("/a", new IntNode(1));
// Test absent
TestAbsenceOperation testAbsence = JsonPatchOperation.testAbsence("/b");

The operations above can be used to create a change and push to a Central Dogma server.

// Create a change with JSON patch operations
Change<JsonNode> change = Change.ofJsonPatch("/a.json",
                                             List.of(add, move, remove, safeReplace, ...));
repository.commit("json patch operations", change)
          .push()
          .join();

ikhoon · 2025-01-17T09:47:40Z

client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaRepository.java

+    /**
+     * Returns the name of the repository.
+     */
+    public String repositoryName() {


The getters will be useful when logging.

jrhee17 · 2025-01-21T05:54:02Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+        if (Iterables.isEmpty(changes)) {
+            return false;
+        }
+        // JsonPatch operations its own validation for the changes so we don't need to validate them here.


allow an empty message JSON patch because test or testAbsent do not have changes

Question) Is this logic so that users can use json patches that only contain test type?

If so, was there a request to let users apply json patches solely comprising of test operations? I imagined that this operation exists only to be used in conjunction with other mutating patches

I didn't think of a case where users use only test. It was originally added for removeIfExists which removes a node only if it exists. If a node is absent, the change for removeIfExists will be empty.

I see.

If a node is absent, the change for removeIfExists will be empty.

So in this case, I understood your proposal is that an extra commit is added. Am I understanding correctly?

If JSON patch operations do not create a change, an empty commit is allowed and the request is returned as success. No extra commit will be added.

No extra commit will be added.

Tested at JsonPatchOperationIntegrationTest.removeIfExists and it seems like the extra commit is added.

before the second commit

user@AL02437565 bar % git cat-file --batch-check --batch-all-objects 0ffe3ffe89a256411408733924396c73749a0da2 commit 307 27730a65b30896185d4fd495790b301e657cb70c commit 309 3b734aff447e0db5994659b62e2c62bc9f69c7a8 tree 34 4b825dc642cb6eb9a060e54bf8d69288fbee4904 tree 0 560674f7d227411ca65c21329bbb9ce5b0fe8dd9 blob 7 73a5d70e33e32e09271e19a98a3326573331d88d blob 13 893c0f344db68915ae53c4fbce76529ebe445c28 commit 276 8efc8a3bfa377804232a6fd4dc8af7870d2623d9 tree 34 user@AL02437565 bar % git cat-file --batch-check --batch-all-objects

after the second commit

0ffe3ffe89a256411408733924396c73749a0da2 commit 307 27730a65b30896185d4fd495790b301e657cb70c commit 309 3b734aff447e0db5994659b62e2c62bc9f69c7a8 tree 34 4502bce9e89208c2d88834d590e67a31cdedff78 commit 315 4b825dc642cb6eb9a060e54bf8d69288fbee4904 tree 0 560674f7d227411ca65c21329bbb9ce5b0fe8dd9 blob 7 73a5d70e33e32e09271e19a98a3326573331d88d blob 13 893c0f344db68915ae53c4fbce76529ebe445c28 commit 276 8efc8a3bfa377804232a6fd4dc8af7870d2623d9 tree 34 user@AL02437565 bar % git cat-file -p 4502bce9e89208c2d88834d590e67a31cdedff78 tree 3b734aff447e0db5994659b62e2c62bc9f69c7a8 parent 27730a65b30896185d4fd495790b301e657cb70c author admin <[email protected]> 1737441026 +0000 committer admin <[email protected]> 1737441026 +0000 { "summary" : "remove a again", "detail" : "", "markup" : "plaintext", "revision" : "4" }% user@AL02437565 bar %

Or did you mean something else by 'no extra commit is added'?

An empty commit means pushing a commit without any object.
We shouldn't add an empty commit when removeIfExists is solely used.

Tested at JsonPatchOperationIntegrationTest.removeIfExists and it seems like the extra commit is added.

Good point. Let me fix it not to commit an empty object.

minwoox

Looks good all in all. 👍

minwoox · 2025-01-21T13:15:54Z

common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/DualPathOperation.java

+    /**
+     * Returns the source path.
+     */
+    public JsonPointer from() {


Suggested change

public JsonPointer from() {

JsonPointer from() {

because DualPathOperation is package-private

I intentionally made it public so users can read the value from subclasses.

minwoox · 2025-01-21T13:19:55Z

common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/JsonPatchOperation.java

+ * Patch</a> operation. A {@link JsonPatchOperation} can be converted into a JSON patch by calling
+ * {@link #toJsonNode()}.
+ *
+ * <p><a href="https://tools.ietf.org/html/draft-ietf-appsawg-json-patch-10">JSON


rfc?
https://datatracker.ietf.org/doc/html/rfc6902

minwoox · 2025-01-21T13:26:43Z

.../com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.java

@@ -158,7 +160,11 @@ int doApply(Revision unused, DirCache dirCache,
                    try {
                        newJsonNode = JsonPatch.fromJson((JsonNode) change.content()).apply(oldJsonNode);
                    } catch (Exception e) {


} catch (JsonPatchConflictException e) { throw e; } catch (Exception e) { throw new JsonPatchConflictException("failed to apply JSON patch: " + change, e); }

Good suggestion.

minwoox · 2025-01-21T13:30:18Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+        if (Iterables.isEmpty(changes)) {
+            return false;
+        }
+        // JsonPatch operations its own validation for the changes so we don't need to validate them here.


An empty commit means pushing a commit without any object.
We shouldn't add an empty commit when removeIfExists is solely used.

Expose JSON Patch operations as public API

fa878be

ikhoon added the new feature label Jan 14, 2025

polish

820316f

ikhoon force-pushed the json-patch-api branch from 4ce8617 to 820316f Compare January 17, 2025 08:30

ikhoon added 2 commits January 17, 2025 18:41

fix tests

c709af4

lint

7c28e78

ikhoon commented Jan 17, 2025

View reviewed changes

copyright

f50f492

ikhoon marked this pull request as ready for review January 17, 2025 10:46

ikhoon requested review from jrhee17, minwoox and trustin as code owners January 17, 2025 10:46

ikhoon added this to the 0.74.0 milestone Jan 17, 2025

jrhee17 approved these changes Jan 21, 2025

View reviewed changes

add shortcut factory methods

1d2c74e

minwoox reviewed Jan 21, 2025

View reviewed changes

Address comments

5630ad2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose JSON Patch operations as public API #1089

Expose JSON Patch operations as public API #1089

ikhoon commented Jan 14, 2025 •

edited

Loading

ikhoon Jan 17, 2025

jrhee17 Jan 21, 2025

ikhoon Jan 21, 2025

jrhee17 Jan 21, 2025

ikhoon Jan 21, 2025

jrhee17 Jan 21, 2025

minwoox Jan 21, 2025

ikhoon Jan 23, 2025 •

edited

Loading

minwoox left a comment

minwoox Jan 21, 2025

ikhoon Jan 23, 2025

minwoox Jan 21, 2025

minwoox Jan 21, 2025

ikhoon Jan 23, 2025

minwoox Jan 21, 2025

Expose JSON Patch operations as public API #1089

Are you sure you want to change the base?

Expose JSON Patch operations as public API #1089

Conversation

ikhoon commented Jan 14, 2025 • 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

ikhoon Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

minwoox 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

ikhoon commented Jan 14, 2025 •

edited

Loading

ikhoon Jan 23, 2025 •

edited

Loading