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. 👍

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

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

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

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.

minwoox

Looks great. 👍 👍 👍

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

minwoox · 2025-03-17T10:06:39Z

Please, fix the conflict when you get a chance. 😉

ikhoon · 2025-03-19T05:59:28Z

Done

trustin · 2025-03-19T06:10:23Z

common/src/main/java/com/linecorp/centraldogma/common/Change.java

+    static Change<JsonNode> ofJsonPatch(String path, Iterable<? extends JsonPatchOperation> jsonPatches) {
+        requireNonNull(path, "path");
+        requireNonNull(jsonPatches, "jsonPatches");
+        return new DefaultChange<>(path, ChangeType.APPLY_JSON_PATCH,
+                                   JsonPatchOperation.asJsonArray(jsonPatches));
+    }


Reject when jsonPatches is empty?

trustin · 2025-03-19T06:17:08Z

.../test/java/com/linecorp/centraldogma/common/jsonpatch/JsonPatchOperationIntegrationTest.java

+        final PushResult result1 = repository.commit("remove a again", change)
+                                             .push()
+                                             .join();
+        // Should not increase the revision if the path is absent and the history must be the same.
+        assertThat(result1.revision()).isEqualTo(result0.revision());


Shouldn't we expect a RedundantChangeException instead?

RedundantChangeException is a good approach, but since it is a conditional operator, I thought handling it as a no-op when the path is absent would align better with the existing contract rather than throwing an exception.

centraldogma/common/src/main/java/com/linecorp/centraldogma/internal/jsonpatch/RemoveIfExistsOperation.java

Lines 35 to 36 in 1cafcd1

* <p>This operation only takes one pointer ({@code path}) as an argument. Unlike, {@link RemoveOperation}, it

* does not throw an error if no JSON value exists at that pointer.</p>

I discussed this internally with @trustin and agreed that raising RedundantChangeException on the empty commit for all commits is more consistent behavior. Some users might get confused if RedundantChangeException is not raised when no commit is made.

If allowing an empty commit is necessary, we can consider adding it as a separate option.

Thanks, @ikhoon!

codecov · 2025-03-20T08:49:46Z

Codecov Report

Attention: Patch coverage is 75.64767% with 47 lines in your changes missing coverage. Please review.

Project coverage is 70.40%. Comparing base (767eff3) to head (889b4f5).
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
...traldogma/common/jsonpatch/JsonPatchOperation.java	74.57%	13 Missing and 2 partials ⚠️
...aldogma/common/jsonpatch/SafeReplaceOperation.java	47.05%	6 Missing and 3 partials ⚠️
...ntraldogma/common/jsonpatch/DualPathOperation.java	64.28%	3 Missing and 2 partials ⚠️
...traldogma/common/jsonpatch/PathValueOperation.java	61.53%	3 Missing and 2 partials ⚠️
.../java/com/linecorp/centraldogma/common/Change.java	70.00%	2 Missing and 1 partial ⚠️
...entraldogma/common/TextPatchConflictException.java	50.00%	2 Missing ⚠️
...a/common/jsonpatch/JsonPatchConflictException.java	50.00%	2 Missing ⚠️
.../storage/repository/git/DefaultChangesApplier.java	71.42%	1 Missing and 1 partial ⚠️
...ogma/common/jsonpatch/RemoveIfExistsOperation.java	83.33%	1 Missing ⚠️
...aldogma/common/jsonpatch/TestAbsenceOperation.java	83.33%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1089      +/-   ##
============================================
+ Coverage     70.07%   70.40%   +0.32%     
- Complexity     4486     4550      +64     
============================================
  Files           453      455       +2     
  Lines         18161    18544     +383     
  Branches       2008     2037      +29     
============================================
+ Hits          12727    13056     +329     
- Misses         4345     4392      +47     
- Partials       1089     1096       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

trustin

👍

minwoox · 2025-03-20T09:37:07Z

👍

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

ikhoon added 4 commits January 23, 2025 15:43

Address comments

5630ad2

lint

8ee6051

Merge branch 'main' into json-patch-api

ec4053f

checkstyle

5c6f681

minwoox approved these changes Mar 5, 2025

View reviewed changes

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java Outdated Show resolved Hide resolved

minwoox modified the milestones: 0.74.0, 0.75.0 Mar 5, 2025

Merge branch 'main' into json-patch-api

b4cb0c4

Merge branch 'main' into json-patch-api

f42b2b8

trustin reviewed Mar 19, 2025

View reviewed changes

ikhoon added 2 commits March 20, 2025 14:47

check empty

3a18853

revert to raise RedundantChangeException for empty commit

d799af6

fix tests

889b4f5

trustin approved these changes Mar 20, 2025

View reviewed changes

minwoox merged commit 3f53a53 into line:main Mar 20, 2025
12 checks passed

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

minwoox left a comment

minwoox commented Mar 17, 2025

ikhoon commented Mar 19, 2025

trustin Mar 19, 2025

trustin Mar 19, 2025

ikhoon Mar 20, 2025

ikhoon Mar 20, 2025

trustin Mar 20, 2025

codecov bot commented Mar 20, 2025 •

edited

Loading

trustin left a comment

minwoox commented Mar 20, 2025

	* <p>This operation only takes one pointer ({@code path}) as an argument. Unlike, {@link RemoveOperation}, it
	* does not throw an error if no JSON value exists at that pointer.</p>

Expose JSON Patch operations as public API #1089

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

minwoox left a comment

Choose a reason for hiding this comment

minwoox commented Mar 17, 2025

ikhoon commented Mar 19, 2025

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

codecov bot commented Mar 20, 2025 • edited Loading

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Mar 20, 2025

ikhoon commented Jan 14, 2025 •

edited

Loading

ikhoon Jan 23, 2025 •

edited

Loading

codecov bot commented Mar 20, 2025 •

edited

Loading