Allow text operations on any file type and fall back to text diff for non-semantic changes#1292
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughText-change factories now accept JSON/JSON5/YAML paths and parse text with Jackson; GitRepository caches raw bytes and emits text diffs when structured patches are empty but raw text differs; server & repo plumbing support IDENTITY_TEXT; multiple tests added or adjusted to validate raw-preservation and diff behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as GitRepository
participant Reader as ObjectReader
participant Parser as Jackson/Json5/Yaml
participant ChangeMap as ChangeMap
Repo->>Reader: open(oldPath).getBytes() / open(newPath).getBytes()
Reader-->>Repo: rawOldBytes / rawNewBytes
Repo->>Parser: readTree(path, rawOldBytes) / readTree(path, rawNewBytes)
Parser-->>Repo: oldNode / newNode
Repo->>Parser: computePatch(oldNode, newNode)
Parser-->>Repo: patch
alt patch non-empty
Repo->>ChangeMap: putStructuredDiff(patch)
else patch empty but raw differs (JSON5/YAML)
Repo->>ChangeMap: sanitizeText(rawOldBytes/rawNewBytes) -> putTextDiff(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/src/main/java/com/linecorp/centraldogma/common/Change.java`:
- Around line 163-166: ofTextUpsert currently rejects JSON/JSON5 based on
EntryType.guessFromPath(path) while ofTextPatch uses Json5.isJson(path); update
ofTextUpsert (both overloads around lines near methods named ofTextUpsert) to
check Json5.isJson(path) and throw ChangeFormatException with the same message
guidance ("invalid file type: " + path + " (expected: a non-JSON file). Use
Change.ofJsonUpsert() instead") instead of relying on EntryType.guessFromPath,
so PATCH_TO_UPSERT flows from GitRepository.toChangeMap() and putTextDiff() will
accept .json5 files consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf627bf3-352e-4e9f-a44f-da7554edcced
📒 Files selected for processing (3)
common/src/main/java/com/linecorp/centraldogma/common/Change.javait/server/src/test/java/com/linecorp/centraldogma/it/GetDiffTest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
common/src/test/java/com/linecorp/centraldogma/common/ChangeTest.java (1)
123-141: Consider simplifying this test with parameterization/helper extraction.The current assertions are correct, but the repetition and variable naming make the test harder to scan than necessary.
♻️ Example refactor
- void shouldAllowJsonFileWith_OfText() { - // Text operations should accept any file type including JSON and JSON5. - final Change<String> textUpsertJson = Change.ofTextUpsert("/foo.json", "abc"); - assertThat(textUpsertJson.type()).isEqualTo(ChangeType.UPSERT_TEXT); - - final Change<String> textUpsertJson5 = Change.ofTextUpsert("/foo.json5", "abc"); - assertThat(textUpsertJson5.type()).isEqualTo(ChangeType.UPSERT_TEXT); - - final Change<String> textPatchJson = Change.ofTextPatch("/foo.json", "abc"); - assertThat(textPatchJson.type()).isEqualTo(ChangeType.APPLY_TEXT_PATCH); - - final Change<String> textPatchJson5 = Change.ofTextPatch("/foo.json5", "abc"); - assertThat(textPatchJson5.type()).isEqualTo(ChangeType.APPLY_TEXT_PATCH); - - final Change<String> textPatchJson2 = Change.ofTextPatch("/foo.json", "foo", "bar"); - assertThat(textPatchJson2.type()).isEqualTo(ChangeType.APPLY_TEXT_PATCH); - - final Change<String> textPatchJson52 = Change.ofTextPatch("/foo.json5", "foo", "bar"); - assertThat(textPatchJson52.type()).isEqualTo(ChangeType.APPLY_TEXT_PATCH); - } + void shouldAllowTextOperationsOnJsonAndJson5Files() { + assertTextUpsertType("/foo.json"); + assertTextUpsertType("/foo.json5"); + assertTextPatchType("/foo.json"); + assertTextPatchType("/foo.json5"); + } + + private static void assertTextUpsertType(String path) { + assertThat(Change.ofTextUpsert(path, "abc").type()).isEqualTo(ChangeType.UPSERT_TEXT); + } + + private static void assertTextPatchType(String path) { + assertThat(Change.ofTextPatch(path, "abc").type()).isEqualTo(ChangeType.APPLY_TEXT_PATCH); + assertThat(Change.ofTextPatch(path, "foo", "bar").type()).isEqualTo(ChangeType.APPLY_TEXT_PATCH); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/test/java/com/linecorp/centraldogma/common/ChangeTest.java` around lines 123 - 141, The test shouldAllowJsonFileWith_OfText in ChangeTest repeats the same assertions; refactor by extracting a small helper or parameterizing the test to iterate filenames ["/foo.json","/foo.json5"] and the two operations Change.ofTextUpsert and Change.ofTextPatch (and the multi-arg overload of ofTextPatch), asserting the resulting Change.type() equals the expected ChangeType (ChangeType.UPSERT_TEXT or ChangeType.APPLY_TEXT_PATCH); update the test to call that helper or use a `@ParameterizedTest/`@ValueSource to remove duplicated variables and assertions while keeping the same coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@common/src/test/java/com/linecorp/centraldogma/common/ChangeTest.java`:
- Around line 123-141: The test shouldAllowJsonFileWith_OfText in ChangeTest
repeats the same assertions; refactor by extracting a small helper or
parameterizing the test to iterate filenames ["/foo.json","/foo.json5"] and the
two operations Change.ofTextUpsert and Change.ofTextPatch (and the multi-arg
overload of ofTextPatch), asserting the resulting Change.type() equals the
expected ChangeType (ChangeType.UPSERT_TEXT or ChangeType.APPLY_TEXT_PATCH);
update the test to call that helper or use a `@ParameterizedTest/`@ValueSource to
remove duplicated variables and assertions while keeping the same coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd0e8bd5-c520-446a-9b10-8ca3fd5b45ef
📒 Files selected for processing (5)
common/src/main/java/com/linecorp/centraldogma/common/Change.javacommon/src/test/java/com/linecorp/centraldogma/common/ChangeTest.javait/server/src/test/java/com/linecorp/centraldogma/it/GetDiffTest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.javaserver/src/test/java/com/linecorp/centraldogma/server/TextUpsertTest.java
💤 Files with no reviewable changes (1)
- common/src/main/java/com/linecorp/centraldogma/common/Change.java
✅ Files skipped from review due to trivial changes (1)
- server/src/test/java/com/linecorp/centraldogma/server/TextUpsertTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- it/server/src/test/java/com/linecorp/centraldogma/it/GetDiffTest.java
- server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java
ikhoon
reviewed
Apr 8, 2026
| static Change<String> ofTextUpsert(String path, String text) { | ||
| requireNonNull(text, "text"); | ||
| validateFilePath(path, "path"); | ||
| if (EntryType.guessFromPath(path) == EntryType.JSON) { |
Contributor
There was a problem hiding this comment.
Shouldn't we validate text if it is a structured data?
Contributor
There was a problem hiding this comment.
Ignore this review if this PR is still being worked on.
Contributor
Author
There was a problem hiding this comment.
Ignore this review if this PR is still being worked on.
Yeah, I'm still working on it, but I forgot to validate the data. 😓
non-semantic changes Motivation: - When a JSON5 file had text-level changes that were semantically identical (e.g., trailing commas), the diff was silently dropped because the parsed JSON nodes were equal. The same issue existed for YAML files with comment changes. Modifications: - In `GitRepository.toChangeMap()`, added a text diff fallback for JSON5 and YAML files when the JSON patch is empty but the raw text differs. - Removed the file type restriction from `ofTextUpsert` that rejected JSON and JSON5 files, and from both `ofTextPatch` overloads that rejected JSON files. Result: - JSON5 and YAML files now correctly produce text diffs when only non-semantic text changes are made (e.g., trailing commas, comments, formatting). - Text operations (`ofTextUpsert`, `ofTextPatch`) now accept any file type.
minwoox
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
(e.g., trailing commas), the diff was silently dropped because the parsed
JSON nodes were equal. The same issue existed for YAML files with comment
changes.
Modifications:
GitRepository.toChangeMap(), added a text diff fallback for JSON5and YAML files when the JSON patch is empty but the raw text differs.
ofTextUpsertthat rejected JSONand JSON5 files, and from both
ofTextPatchoverloads that rejected JSONfiles.
Result:
non-semantic text changes are made (e.g., trailing commas, comments,
formatting).
ofTextUpsert,ofTextPatch) now accept any file type.