Support for YAML #1229
Support for YAML #1229
Conversation
Motivation: As a preliminary work for line#1215, add code in advance to maintain compatibility when JSON is provided as a raw content in `Change`. If a new version of the client sends `rawContent` in `Change` but the server hasn't been updated yet, the `rawContent` will be nomalized in the same way as the current `content` then stored. Modifications: - Parse `rawContent` into `JsonNode` if exists and set it to `content` for forward compatibility. Result: Implemented preliminary compatility work for JSON 5 support
📝 WalkthroughWalkthroughAdds first-class YAML support across Core, Client, Server, Storage, Mirror, and Web UI: new EntryType/ChangeType/QueryType for YAML, YAML parsing/serialization utilities, structured-change model, path-aware validation, storage/mirror YAML writes, and editor/UI structured-file handling. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(245,250,255)
participant UI as Web UI
participant Client as Armeria client
participant Server as CentralDogma server
participant Applier as ChangesApplier
participant Git as GitRepository
note right of Server: DtoConverter handles YAML/rawContent/viewRaw
end
UI->>Client: submit change (content + inferred type)
Client->>Server: HTTP request (path, content, type, viewRaw)
Server->>Server: DtoConverter.newEntryDto / validate path
Server->>Applier: apply(StructuredChange / UPSERT_YAML)
Applier->>Git: write bytes (pretty JSON or raw YAML) / commit
Git-->>Applier: commit result
Applier-->>Server: apply result
Server-->>Client: EntryDto (content/json or rawContent)
Client-->>UI: render response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. ✨ Finishing touches
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/linecorp/centraldogma/common/Change.java (1)
390-397: MissingYAMLcase will causefromFileto fail for YAML files.The
fromFilemethod usesEntryType.guessFromPathto determine the file type, but the switch statement only handlesJSONandTEXT. Since this PR addsEntryType.YAML, importing YAML files from the filesystem will throw"unexpected entry type: YAML".Apply this diff:
switch (entryType) { case JSON: return ofJsonUpsert(targetPath, content); + case YAML: + return ofYamlUpsert(targetPath, content); case TEXT: return ofTextUpsert(targetPath, content); default: throw new Error("unexpected entry type: " + entryType); }
🧹 Nitpick comments (3)
it/server/src/test/java/com/linecorp/centraldogma/it/FileManagementTest.java (1)
84-87: LGTM! Changes correctly validate trailing-slash normalization for directories.The updates properly test that directory entries are now returned with trailing slashes, aligning with the PR's path normalization changes. The added comment on line 84 helpfully clarifies the expected behavior.
For slightly improved clarity, consider renaming
testRootWithoutSlashto something liketestRootPatternor adding a comment explaining it's intentionally used to test path normalization:- final String testRootWithoutSlash = TEST_ROOT.substring(0, TEST_ROOT.length() - 1); + // Request directory without trailing slash to verify server normalizes it + final String testRootWithoutSlash = TEST_ROOT.substring(0, TEST_ROOT.length() - 1);This would make it clearer why the variable is created without a trailing slash but the result is expected with one.
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
62-64: Consider case-insensitive extension matching.The
isYamlmethod only matches lowercase extensions (.yaml,.yml). While this is consistent with the webapp implementation, files with uppercase extensions like.YAMLor.Yamlwon't be recognized as YAML.If case-insensitive matching is desired:
public static boolean isYaml(String filename) { - return filename.endsWith(".yaml") || filename.endsWith(".yml"); + final String lower = filename.toLowerCase(java.util.Locale.ROOT); + return lower.endsWith(".yaml") || lower.endsWith(".yml"); }Alternatively, if case-sensitivity is intentional (e.g., following filesystem conventions), consider adding a brief comment to document this decision.
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
126-150: Consider grouping related methods together for consistency.The YAML validation methods are split:
validateYamlFilePathis at lines 126-131, butisValidYamlFilePathis at lines 146-150, with structured file methods in between. For consistency with the JSON pattern (wherevalidateJsonFilePathandisValidJsonFilePathare adjacent), consider reordering:public static String validateYamlFilePath(String path, String paramName) { ... } +public static boolean isValidYamlFilePath(String path) { + requireNonNull(path, "path"); + return !path.isEmpty() && path.charAt(0) == '/' && + YAML_FILE_PATH_PATTERN.matcher(path).matches(); +} + public static boolean isValidStructuredFilePath(String path) { ... } public static String validateStructuredFilePath(String path, String paramName) { ... } -public static boolean isValidYamlFilePath(String path) { - requireNonNull(path, "path"); - return !path.isEmpty() && path.charAt(0) == '/' && - YAML_FILE_PATH_PATTERN.matcher(path).matches(); -}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java(3 hunks)common-legacy/src/main/java/com/linecorp/centraldogma/internal/thrift/EntryConverter.java(1 hunks)common/build.gradle(1 hunks)common/src/main/java/com/linecorp/centraldogma/common/Change.java(7 hunks)common/src/main/java/com/linecorp/centraldogma/common/ChangeDeserializer.java(1 hunks)common/src/main/java/com/linecorp/centraldogma/common/ChangeType.java(1 hunks)common/src/main/java/com/linecorp/centraldogma/common/ContentHolder.java(4 hunks)common/src/main/java/com/linecorp/centraldogma/common/Entry.java(4 hunks)common/src/main/java/com/linecorp/centraldogma/common/EntryType.java(3 hunks)common/src/main/java/com/linecorp/centraldogma/common/JsonPathQuery.java(2 hunks)common/src/main/java/com/linecorp/centraldogma/common/Query.java(2 hunks)common/src/main/java/com/linecorp/centraldogma/common/QueryType.java(2 hunks)common/src/main/java/com/linecorp/centraldogma/common/StructuredChange.java(5 hunks)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java(3 hunks)common/src/main/java/com/linecorp/centraldogma/internal/Util.java(2 hunks)common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java(1 hunks)common/src/main/java/com/linecorp/centraldogma/internal/api/v1/EntryDto.java(2 hunks)common/src/test/java/com/linecorp/centraldogma/internal/YamlTest.java(1 hunks)it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/GitMirrorIntegrationTest.java(4 hunks)it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/LocalToRemoteGitMirrorTest.java(2 hunks)it/server/src/test/java/com/linecorp/centraldogma/it/FileManagementTest.java(1 hunks)server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/api/DtoConverter.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/AbstractChangesApplier.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.java(5 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java(2 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/PathPatternFilter.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/TransformingChangesApplier.java(1 hunks)server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryUtil.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java(1 hunks)server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java(5 hunks)webapp/package.json(1 hunks)webapp/src/dogma/common/components/CommitForm.tsx(3 hunks)webapp/src/dogma/common/components/editor/FileEditor.tsx(2 hunks)webapp/src/dogma/common/components/editor/JsonPath.tsx(1 hunks)webapp/src/dogma/features/file/FileList.tsx(1 hunks)webapp/src/dogma/features/file/NewFile.tsx(4 hunks)webapp/src/dogma/features/file/StructuredFileSupport.ts(1 hunks)webapp/src/dogma/util/path-util.ts(1 hunks)webapp/tests/dogma/feature/file/FileList.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
webapp/src/dogma/common/components/CommitForm.tsx (1)
webapp/src/dogma/features/file/StructuredFileSupport.ts (1)
detectChangeType(5-19)
it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/GitMirrorIntegrationTest.java (1)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-291)
common/src/test/java/com/linecorp/centraldogma/internal/YamlTest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
Yaml(29-67)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/AbstractChangesApplier.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(70-415)
webapp/src/dogma/features/file/NewFile.tsx (2)
webapp/src/dogma/features/file/StructuredFileSupport.ts (1)
detectChangeType(5-19)webapp/src/dogma/features/notification/notificationSlice.ts (1)
newNotification(43-50)
common/src/main/java/com/linecorp/centraldogma/common/JsonPathQuery.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-543)
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
webapp/src/dogma/util/path-util.ts (1)
isYaml(56-58)
webapp/src/dogma/common/components/editor/FileEditor.tsx (3)
webapp/src/dogma/features/file/StructuredFileSupport.ts (1)
isStructuredFile(49-51)webapp/src/dogma/common/components/editor/JsonPathLegend.tsx (1)
JsonPathLegend(40-78)webapp/src/dogma/common/components/editor/JsonPath.tsx (1)
JsonPath(6-33)
webapp/src/dogma/features/file/StructuredFileSupport.ts (1)
webapp/src/dogma/util/path-util.ts (3)
isJson(48-50)isJson5(52-54)isYaml(56-58)
webapp/src/dogma/features/file/FileList.tsx (2)
webapp/src/dogma/features/file/FileDto.ts (1)
FileDto(3-11)webapp/src/dogma/common/components/ChakraLink.tsx (1)
ChakraLink(9-22)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (2)
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
Yaml(29-67)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(70-415)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (2)
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
Yaml(29-67)common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-291)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (10)
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java (1)
CentralDogma(202-1399)client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaRepository.java (1)
CentralDogmaRepository(53-386)common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-291)common/src/main/java/com/linecorp/centraldogma/common/PushResult.java (1)
PushResult(31-103)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/AddOperation.java (1)
AddOperation(80-138)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/JsonPatchConflictException.java (1)
JsonPatchConflictException(25-42)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/RemoveIfExistsOperation.java (1)
RemoveIfExistsOperation(40-94)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/RemoveOperation.java (1)
RemoveOperation(58-108)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/ReplaceOperation.java (1)
ReplaceOperation(55-82)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/TestOperation.java (1)
TestOperation(56-73)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (3)
common/src/main/java/com/linecorp/centraldogma/internal/Json5.java (1)
Json5(57-104)webapp/src/dogma/util/path-util.ts (3)
isJson(48-50)isJson5(52-54)isYaml(56-58)common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
Yaml(29-67)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java (1)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-291)
webapp/src/dogma/common/components/editor/JsonPath.tsx (1)
webapp/src/dogma/features/file/StructuredFileSupport.ts (2)
parseContent(22-33)stringifyContent(36-47)
common/src/main/java/com/linecorp/centraldogma/common/StructuredChange.java (2)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-543)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(70-415)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.java (3)
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (1)
Yaml(29-67)webapp/src/dogma/util/path-util.ts (1)
isYaml(56-58)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(70-415)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: flaky-tests
- GitHub Check: docker-build
- GitHub Check: lint
🔇 Additional comments (66)
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryUtil.java (2)
23-23: LGTM!The import addition is consistent with existing identity query type imports and necessary for the YAML identity query support.
142-143: LGTM! The extension correctly handles YAML identity queries by returning the entry unchanged, consistent with the existing pattern for JSON and TEXT identity queries.QueryType.IDENTITY_YAMLis properly configured withEntryType.YAMLin itssupportedEntryTypes(), ensuring queries pass the validation at line 137.server-mirror-git/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractGitMirror.java (2)
344-346: LGTM! YAML mirroring added consistently.The YAML case correctly follows the established pattern for JSON and TEXT, using
Change.ofYamlUpsertto handle YAML content when mirroring from remote Git to local Central Dogma repository.
635-645: LGTM! Unified JSON and YAML handling is appropriate.The fall-through case structure correctly handles both JSON and YAML with the same logic—both are structured formats that can use identical string comparison and git storage. The variable renaming from
oldJson/newJsontooldData/newDataappropriately reflects the dual-format handling.common/build.gradle (1)
10-11: LGTM!The Jackson YAML dataformat dependency is correctly added alongside existing Jackson libraries, following the established pattern. This enables YAML parsing functionality for the new YAML support feature.
webapp/src/dogma/features/file/FileList.tsx (1)
58-80: LGTM!The path column logic correctly:
- Normalizes the path by removing trailing slashes for display
- Determines directory vs file type based on the original path's trailing slash
- Constructs appropriate hrefs for navigation
- Shows the correct icon based on entry type
server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java (1)
478-480: LGTM!The test expectations are correctly updated to reflect the new trailing-slash semantics for directory paths. The method rename from
listADirectoryWithoutSlashtolistADirectoryWithSlashaccurately describes the verified behavior.Also applies to: 504-506, 558-560, 791-823
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java (1)
406-437: LGTM!The YAML entry handling is correctly integrated into both the fetch-content and no-fetch-content branches:
- Content bytes are converted to a string once and reused, improving code clarity
- YAML entries are created using
Entry.ofYaml()consistently with JSON and TEXT patterns- Break statements are properly placed to prevent fall-through
The implementation aligns with the
Entry.ofYaml(Revision, String, String)method signature from the relevant code snippets.common/src/main/java/com/linecorp/centraldogma/common/StructuredChange.java (5)
20-21: LGTM! Class renamed to support structured formats.The import additions and class rename from
JsonChangetoStructuredChangeappropriately generalize the implementation to handle both JSON and YAML formats.Also applies to: 34-34
38-38: LGTM! Field and constructor signatures updated.The field rename from
jsonTexttostructuredTextand updated constructor signatures correctly reflect the broader scope of structured formats (JSON, JSON5, YAML).Also applies to: 43-49
51-71: LGTM! Solid validation and path-aware parsing.The validation logic correctly accepts both JSON and YAML file paths (lines 55-56), and the use of
Jackson.readTree(path, structuredText)(line 65) enables path-based dispatch to the appropriate parser. The improved error message including the file path (line 67) aids debugging.
78-98: LGTM! Content retrieval logic correctly prioritizes raw text.The
rawContent()andcontentAsText()methods correctly preferstructuredTextwhen available and use path-aware serialization (Jackson.writeValueAsString(path(), jsonNode)at line 94) to ensure proper format output.
100-137: LGTM! Object methods updated consistently.The
equals(),hashCode(), andtoString()methods are correctly updated to useStructuredChangetype and preferstructuredTextfor display purposes while comparing canonicaljsonNoderepresentations for equality.common/src/main/java/com/linecorp/centraldogma/common/ChangeDeserializer.java (1)
111-116: LGTM! YAML deserialization follows established pattern.The
UPSERT_YAMLcase correctly requiresrawContentand delegates toChange.ofYamlUpsert(path, rawContent), consistent with the handling ofUPSERT_TEXTabove.common/src/main/java/com/linecorp/centraldogma/common/Query.java (2)
65-72: LGTM! YAML query API consistent with JSON.The new
ofYaml(String path)factory method correctly returns anIdentityQuerywithQueryType.IDENTITY_YAMLand follows the same pattern asofJson().
114-115: LGTM! Switch case properly handles IDENTITY_YAML.The
IDENTITY_YAMLcase is correctly implemented, consistent with the other identity type cases (IDENTITY_TEXT,IDENTITY_JSON).webapp/src/dogma/util/path-util.ts (1)
56-58: LGTM! YAML detection covers both common extensions.The
isYaml()function correctly checks for both.yamland.ymlextensions, consistent with the pattern ofisJson()andisJson5().server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/TransformingChangesApplier.java (1)
64-64: LGTM! InsertJson call updated for new constructor signature.The
InsertJsonconstructor call correctly removes the trailingnullparameter, aligning with the updated constructor signature that no longer accepts ajsonTextparameter.common/src/main/java/com/linecorp/centraldogma/common/EntryType.java (2)
20-20: LGTM! YAML entry type correctly uses JsonNode representation.The new
YAMLenum value withJsonNode.classtype is correct, as YAML content is deserialized toJsonNode(similar to JSON), enabling JSON Path and JSON Patch operations on YAML content.Also applies to: 33-36
61-63: LGTM! YAML path detection correctly positioned.The YAML detection in
guessFromPath()is correctly placed after JSON checks and before the TEXT fallback, ensuring proper format prioritization.webapp/package.json (1)
44-45: Verify the YAML package version is current and secure.The
yamldependency at version^2.5.0should be verified for latest stability and known security advisories. Please confirm this matches the current stable release and check npm security registry or runnpm auditto ensure no vulnerabilities are present in this dependency.common/src/main/java/com/linecorp/centraldogma/common/JsonPathQuery.java (1)
19-19: LGTM! Path validation correctly generalized to structured files.The change from
validateJsonFilePathtovalidateStructuredFilePathproperly extends JSON Path query support to YAML files, aligning with the PR's objective to provide first-class YAML support.Also applies to: 46-46
common/src/test/java/com/linecorp/centraldogma/internal/YamlTest.java (1)
25-76: LGTM! Good basic YAML parsing test coverage.The tests cover fundamental YAML structures (simple key-value, multiple keys, nested objects, and arrays) and correctly verify the YAML → JsonNode conversion using the
Yaml.readTreeutility.common/src/main/java/com/linecorp/centraldogma/common/ContentHolder.java (1)
108-112: LGTM! YAML content correctly parsed to JsonNode.The
contentAsJson()method properly handles YAML by routing toYaml.readTree(), maintaining consistency with the design goal of treating YAML as a structured format that can be represented as JsonNode.common/src/main/java/com/linecorp/centraldogma/common/ChangeType.java (1)
35-39: LGTM! UPSERT_YAML properly defined.The new
UPSERT_YAMLenum constant correctly usesJsonNode.classas its content type, maintaining consistency withUPSERT_JSONand enabling YAML content to be treated as structured data throughout the system.webapp/src/dogma/common/components/CommitForm.tsx (1)
44-50: Good refactoring to centralize change type detection.Replacing inline validation with
detectChangeType()reduces code duplication and ensures consistent handling across the webapp. The error handling correctly prevents invalid content from being committed.webapp/tests/dogma/feature/file/FileList.test.tsx (1)
33-35: LGTM! Test correctly handles trailing slash for directory paths.The updated test data and assertion logic properly accommodate the new directory path convention (trailing slash), ensuring that the UI correctly displays directory names without the trailing slash.
Also applies to: 61-65
webapp/src/dogma/common/components/editor/JsonPath.tsx (1)
6-14: LGTM! JsonPath properly generalized for structured files.The component now accepts
languageandoriginalContentprops and usesparseContent/stringifyContentto handle JSON, JSON5, and YAML uniformly. This enables JSON Path queries to work seamlessly across all structured file types.Also applies to: 18-21
it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/LocalToRemoteGitMirrorTest.java (3)
171-173: LGTM! YAML test content is well-structured.The YAML content is minimal yet complete for testing mirroring functionality, paralleling the JSON5 test approach.
181-181: LGTM! Proper YAML upsert integration.The YAML change is correctly integrated into the commit alongside other content types.
200-201: LGTM! YAML mirroring validation is correct.The assertion properly validates that YAML content is mirrored with a trailing newline, consistent with JSON5 behavior.
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/AbstractChangesApplier.java (2)
84-88: LGTM! Simplified constructor signature.Removing the
jsonTextparameter simplifies the InsertJson API. All JSON content will now be consistently serialized through Jackson, ensuring uniform formatting.
90-99: LGTM! Consistent pretty-printing improves readability.Always serializing JSON with pretty-printing ensures uniform, readable formatting in Git storage and the web UI. This aligns with the PR objective to improve readability.
it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/GitMirrorIntegrationTest.java (5)
34-34: LGTM! Required import for type-safe values handling.
200-203: LGTM! YAML test file follows established patterns.The YAML content is simple and appropriate for testing remote-to-local mirroring, paralleling the JSON5 test structure.
235-237: LGTM! YAML entry expectations are correctly structured.The directory and YAML entry assertions properly validate the mirrored content structure.
243-246: LGTM! Comprehensive YAML content validation.The assertions properly verify both raw YAML content preservation and JSON representation parsing, validating a key feature of YAML support.
299-301: LGTM! Improved type safety with explicit Collection variable.common/src/main/java/com/linecorp/centraldogma/common/Entry.java (3)
50-52: LGTM! Directory path normalization improves consistency.Adding a trailing slash to directory paths ensures unambiguous directory identification and simplifies EntryType determination, as noted in the PR objectives.
87-108: LGTM! YAML factory methods follow established JSON patterns.The two
ofYamlmethods mirror theofJsonAPI design, enabling both pre-parsed and string-based YAML entry creation. Storing YAML as JsonNode enables JSON operations on YAML content.
254-256: LGTM! Path-aware parsing enables multi-format support.Using
Jackson.readTree(path, rawContent)enables automatic format detection (JSON/JSON5/YAML) based on file extension, which is essential for the new YAML support.server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (1)
1-497: LGTM! Comprehensive YAML CRUD test coverage.This new test class provides excellent coverage of YAML functionality:
- Basic CRUD operations with and without raw view
- JSON Path queries on YAML content
- Watch functionality for YAML files
- JSON Patch operations (add, remove, replace, test)
- Error cases (viewRaw with JSON Path, redundant changes, conflicts)
- Support for both .yaml and .yml extensions
The test structure is clear, well-organized, and follows JUnit 5 best practices.
webapp/src/dogma/common/components/editor/FileEditor.tsx (3)
30-30: LGTM! Centralized structured file detection.The
isStructuredFileutility properly abstracts structured file detection, supporting JSON, JSON5, and YAML formats.
174-174: LGTM! JSON Path legend now shown for all structured files.Extending the JSON Path legend to all structured file types (JSON, JSON5, YAML) is correct since JSON Path operations work on all these formats after parsing.
176-181: LGTM! JsonPath component API generalized for multi-format support.Passing
languageandoriginalContentinstead of pre-parsedjsonContentallows the JsonPath component to handle parsing for all structured formats (JSON, JSON5, YAML) internally.common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (3)
21-23: LGTM! Required imports for format detection.
185-208: LGTM! Path-based format dispatching enables multi-format support.The two overloads properly dispatch to format-specific parsers based on file extension:
- String version falls back to JSON parser for unknown types (lenient)
- Byte version throws
IllegalArgumentExceptionfor unknown types (strict)The different error handling strategies are reasonable: String inputs are more likely to come from flexible sources where fallback is acceptable, while byte inputs suggest more controlled scenarios where explicit format is expected.
238-244: LGTM! Path-based serialization completes multi-format support.The new
writeValueAsStringoverload correctly selects JSON or YAML serialization based on file extension, completing the read/write multi-format infrastructure.common/src/main/java/com/linecorp/centraldogma/common/QueryType.java (3)
30-30: LGTM! IDENTITY query type now includes YAML.Adding YAML to the IDENTITY query type is correct since it retrieves content as-is for all supported entry types.
42-45: LGTM! New IDENTITY_YAML query type follows established patterns.The new YAML-specific identity query type mirrors IDENTITY_TEXT and IDENTITY_JSON, providing consistent API design.
51-51: LGTM! JSON_PATH extended to YAML enables querying.Extending JSON_PATH to support YAML is correct since YAML content can be parsed to JSON, enabling JSON Path queries on YAML files. This implements a key PR objective: "Allowing querying of YAML".
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (2)
1076-1108: LGTM!The dispatch logic correctly routes YAML entries through the new
entryAsYamlhandler while maintaining JSON handling for JSON entries. The addition ofIDENTITY_YAMLcase and YAML support withinJSON_PATH/IDENTITY_JSONenables the key feature of querying YAML with JSON Path.
1066-1074: LGTM!The entry type inference correctly handles backward compatibility with older servers. By using
EntryType.guessFromPath(entryPath)when the server doesn't explicitly return DIRECTORY, the client can properly identify YAML files based on their extension even when older servers return TEXT.common/src/main/java/com/linecorp/centraldogma/internal/api/v1/EntryDto.java (1)
41-48: LGTM!The deprecation of the
typefield and accessor is well-documented. The JavaDoc clearly explains the migration path (useEntryType.guessFromPath(String)) and the rationale (adding new types without breaking clients). This enables graceful evolution of the API as new entry types like YAML are introduced.Also applies to: 77-81
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.java (2)
198-206: LGTM!The format-aware content writing correctly preserves the file format when applying JSON patches. The comment appropriately warns that JSON5/YAML features like comments will be lost, which is inherent to JSON Patch semantics.
256-262: LGTM!Good simplification. The
Jackson.readTree(path, content)method centralizes format detection based on file extension, reducing code duplication and ensuring consistent handling across the codebase.server/src/main/java/com/linecorp/centraldogma/server/internal/api/DtoConverter.java (1)
80-104: LGTM!The YAML handling logic is well-designed for backward compatibility:
- When
rawContentis null (JSON_PATH queries), returns proper YAML type since this is a new feature only used by new clients.- When
rawContentis present (full file reads), returns TEXT type to maintain compatibility with older clients that don't recognize YAML.The TODO comment appropriately tracks the technical debt for future cleanup.
common/src/main/java/com/linecorp/centraldogma/internal/Yaml.java (2)
29-36: LGTM!Good configuration choices:
WRITE_DOC_START_MARKERdisabled produces cleaner output without the---document separator.FAIL_ON_READING_DUP_TREE_KEYenabled prevents silent data loss from duplicate keys, which is a common source of YAML configuration bugs.
38-56: LGTM!The error handling pattern correctly distinguishes between
JsonParseException(which callers may want to handle specifically) and otherIOExceptiontypes (wrapped asIOErrorsince they indicate unexpected I/O failures).common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
57-60: LGTM! Patterns correctly extend existing validation infrastructure.The regex patterns follow the established convention and properly use case-insensitive matching
(?i)for file extensions, consistent with the JSON pattern.webapp/src/dogma/features/file/StructuredFileSupport.ts (2)
5-19: LGTM!The function correctly delegates extension detection to the path utilities and validates content by parsing. The exception propagation for invalid content is appropriate for validation purposes.
49-51: LGTM!This is the correct source of truth for structured file types.
common/src/main/java/com/linecorp/centraldogma/common/Change.java (4)
88-113: LGTM! Clean API extension for YAML support.The new
ofYamlUpsertmethod follows the established pattern of the JSON counterpart, and the migration toStructuredChangeis consistent across both methods.
199-234: LGTM! Good improvements to JSON patch methods.The use of path-aware
Jackson.readTree(path, ...)improves error messages, and the consistent migration toStructuredChangeis well-executed.
236-283: LGTM!The updated Javadoc clearly communicates that JSON patch operations work across JSON, JSON5, and YAML files, and the important caveat about comment normalization is helpful.
310-324: LGTM!Consistent with the other JSON patch method updates.
...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
Show resolved
Hide resolved
common-legacy/src/main/java/com/linecorp/centraldogma/internal/thrift/EntryConverter.java
Show resolved
Hide resolved
common/src/main/java/com/linecorp/centraldogma/common/ContentHolder.java
Show resolved
Hide resolved
.../com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.java
Show resolved
Hide resolved
...java/com/linecorp/centraldogma/server/internal/storage/repository/git/PathPatternFilter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
common/src/main/java/com/linecorp/centraldogma/common/Change.java (3)
390-397: Add YAML case to handle YAML imports.The
switchstatement only handlesJSONandTEXTentry types. Since this PR addsEntryType.YAML, the switch will throw anErrorwhenfromFileorfromDirectoryencounters a.yamlor.ymlfile.Apply this diff to add YAML support:
switch (entryType) { case JSON: return ofJsonUpsert(targetPath, content); + case YAML: + return ofYamlUpsert(targetPath, content); case TEXT: return ofTextUpsert(targetPath, content); default: throw new Error("unexpected entry type: " + entryType); }
62-78: Consider preventing YAML files in ofTextUpsert.Now that YAML is a structured format,
ofTextUpsertshould prevent YAML files (like it does for JSON at line 73) to ensure they're handled viaofYamlUpsert. The javadoc at lines 62-68 should also mention YAML alongside JSON/JSON5./** * Returns a newly-created {@link Change} whose type is {@link ChangeType#UPSERT_TEXT}. * - * <p>Note that you should use {@link #ofJsonUpsert(String, String)} if the specified {@code path} ends with - * {@code ".json"} or {@code ".json5"}. The {@link #ofJsonUpsert(String, String)} will check that the given - * {@code text} is a valid JSON. + * <p>Note that you should use {@link #ofJsonUpsert(String, String)} or {@link #ofYamlUpsert(String, String)} + * if the specified {@code path} ends with {@code ".json"}, {@code ".json5"}, {@code ".yaml"}, or {@code ".yml"}. * * @param path the path of the file * @param text the content of the file - * @throws ChangeFormatException if the path ends with {@code ".json"} or {@code ".json5"} + * @throws ChangeFormatException if the path ends with a structured file extension */ static Change<String> ofTextUpsert(String path, String text) { requireNonNull(text, "text"); validateFilePath(path, "path"); - if (EntryType.guessFromPath(path) == EntryType.JSON) { + final EntryType entryType = EntryType.guessFromPath(path); + if (entryType == EntryType.JSON || entryType == EntryType.YAML) { throw new ChangeFormatException("invalid file type: " + path + - " (expected: a non-JSON file). Use Change.ofJsonUpsert() instead"); + " (expected: a non-structured file). Use appropriate structured methods"); } return new TextChange(path, ChangeType.UPSERT_TEXT, text); }
136-187: Consider preventing YAML files in ofTextPatch methods.Similar to
ofTextUpsert, theofTextPatchmethods check for JSON at lines 151 and 181 but not YAML. Ensure YAML files are directed to use JSON patch operations (which work on YAML per the updated javadocs).Apply this pattern to both
ofTextPatchoverloads:static Change<String> ofTextPatch(String path, @Nullable String oldText, String newText) { validateFilePath(path, "path"); requireNonNull(newText, "newText"); - if (EntryType.guessFromPath(path) == EntryType.JSON) { + final EntryType entryType = EntryType.guessFromPath(path); + if (entryType == EntryType.JSON || entryType == EntryType.YAML) { throw new ChangeFormatException("invalid file type: " + path + - " (expected: a non-JSON file). Use Change.ofJsonPatch() instead"); + " (expected: a non-structured file). Use Change.ofJsonPatch() instead"); } // ... rest of method }Update javadoc at lines 139-146 to mention YAML/JSON5 alongside JSON.
♻️ Duplicate comments (2)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (1)
1150-1177: Issues from previous review remain unaddressed.The two issues flagged in the past review are still present:
- Line 1158: The comment incorrectly references "raw JSON text" instead of "raw YAML text"
- Lines 1162-1164: The error handling is inconsistent with
entryAsJson(lines 1143-1145), which logs a warning and falls back gracefully whenrawContentis nullApply this diff to fix both issues:
} catch (JsonParseException e) { - // Should never reach here as the raw JSON text was already validated by the server. + // Should never reach here as the raw YAML text was already validated by the server. throw new IllegalStateException(e); } } - // Should never reach here as the server already supports YAML entries. - throw new IllegalStateException("The server does not provide rawContent for YAML entry: " + - entryPath); + logger.warn("The server does not support raw content for YAML. Using Entry#content() instead. path: {}", + entryPath); + // Fall through to content-based handling below. }common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
198-209: Same concerns as the String overload.This method has the same dispatch logic and potential issues as
readTree(String path, String data)(lines 185-196):
- Missing null validation for the
pathparameter.- Fallback to JSON parsing when path format is unrecognized could hide errors.
Ensure these concerns are addressed consistently across both overloads.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java(3 hunks)common/src/main/java/com/linecorp/centraldogma/common/Change.java(7 hunks)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java(3 hunks)webapp/src/dogma/features/file/StructuredFileSupport.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/src/dogma/features/file/StructuredFileSupport.ts
🔇 Additional comments (9)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (4)
120-120: LGTM!The import for
Yamlutility is correctly added to support the new YAML entry handling.
1076-1108: LGTM!The query type routing correctly handles YAML alongside existing types. The addition of
IDENTITY_YAMLcase and YAML handling withinIDENTITY_JSON/JSON_PATHcases aligns with the PR objective to support JSON Path operations on YAML content.
1167-1175: Good backward compatibility handling for textual YAML content.The logic correctly handles older servers that may return YAML content as a string rather than a pre-parsed
JsonNode. The fallback parsing withYaml.readTree()ensures compatibility.
1066-1074: Entry type inference correctly handles backward compatibility with older servers.The implementation properly falls back to extension-based type inference when the server doesn't provide explicit type information.
EntryType.guessFromPath()correctly identifies.json,.json5,.yaml, and.ymlfiles, and defaults to TEXT for unknown extensions.common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
185-196: Null path parameter will cause NPE in format detection helpers.The
isJson(),isJson5(), andYaml.isYaml()helper methods call string methods on thepathparameter without null checks. Ifpathis null, the first conditionisJson(path)will throw NPE when trying to callAscii.toLowerCase(path).endsWith(). Consider validating the path parameter or ensuring callers always provide a non-null path.Additionally, the fallback path (lines 192-194) silently treats unknown file extensions as JSON. If
pathhas an unexpected extension, YAML or JSON5 content will be parsed as JSON, which could mask format detection issues or produce parsing errors. Confirm whether this behavior is intentional.common/src/main/java/com/linecorp/centraldogma/common/Change.java (4)
88-102: LGTM: StructuredChange refactoring enables multi-format support.The refactoring from
JsonChangetoStructuredChangeappropriately generalizes these factory methods to handle JSON, JSON5, and YAML formats.
199-215: LGTM: Path-aware parsing improves error diagnostics.The switch to
Jackson.readTree(path, ...)enables better error messages with path context. TheStructuredChangereturn type is consistent with the refactoring.
224-234: LGTM: Consistent refactoring to StructuredChange.
236-324: LGTM: Javadoc clarifies JSON patch applicability to structured formats.The documentation updates at lines 238-241, 254-257, 269-272, 287-290, and 312-315 appropriately clarify that JSON patch operations work across JSON, JSON5, and YAML. The normalization caveat about losing comments is valuable guidance.
| } else { | ||
| jsonBytes = Jackson.writeValueAsBytes(jsonNode); | ||
| } | ||
| final byte[] jsonBytes = Jackson.writeValueAsPrettyString(jsonNode).getBytes(UTF_8); |
There was a problem hiding this comment.
Note) Understood that the intention of this change is that usage of ContentTransformer is intended for internal writes only - hence formatting is discarded.
There was a problem hiding this comment.
Right. In addition, the original content is not preserved when using ContentTransformer
There was a problem hiding this comment.
Then, can we just use writeValueAsBytes?
...java/com/linecorp/centraldogma/server/internal/storage/repository/git/PathPatternFilter.java
Show resolved
Hide resolved
...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
Show resolved
Hide resolved
| * @param path the path of the YAML file | ||
| * @param content the content of the YAML file | ||
| */ | ||
| public static Entry<JsonNode> ofYaml(Revision revision, String path, JsonNode content) { |
There was a problem hiding this comment.
Question) It isn't easy to track whether rawContent is null or non-null. What do you think of removing this and just always populating rawContent for yaml types?
There was a problem hiding this comment.
rawContent is not provided if viewRaw is false. This factory method is used for that case. The rawContent always exists at the server side and it is nullable at the client side depending on the viewRaw option.
Do you prefer filling the serialized YAML content even for viewRaw=false option for simplicity?
server/src/main/java/com/linecorp/centraldogma/server/internal/api/DtoConverter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (2)
78-80: Remove unnecessarythrows InterruptedExceptiondeclaration.The method uses
.join()which doesn't throwInterruptedException(unlike.get()fromFuture). Any exceptions are wrapped inCompletionException.🔎 Proposed fix
@CsvSource({ "config.yaml", "config.yml" }) @ParameterizedTest -void pushAndReadRawYamlFile(String fileName) throws InterruptedException { +void pushAndReadRawYamlFile(String fileName) {
366-366: Consider removing unnecessarythrows JsonParseExceptiondeclarations.The methods at lines 366, 397, 448, and 477 declare
throws JsonParseException, but the operations used (Change.ofYamlUpsert,contentAsJson(), etc.) don't propagate this checked exception to callers. The actual parsing exceptions are handled internally or wrapped in runtime exceptions.🔎 Proposed fix for all affected methods
-void jsonPatchAddOnYamlFile() throws JsonParseException { +void jsonPatchAddOnYamlFile() {-void jsonPatchRemoveIfExistsOnYamlFile() throws JsonParseException { +void jsonPatchRemoveIfExistsOnYamlFile() {-void jsonPatchMultipleOperationsOnYamlFile() throws JsonParseException { +void jsonPatchMultipleOperationsOnYamlFile() {-void jsonPatchOnYmlExtensionFile() throws JsonParseException { +void jsonPatchOnYmlExtensionFile() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/java/com/linecorp/centraldogma/common/Query.javaserver/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (7)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-291)common/src/main/java/com/linecorp/centraldogma/common/RedundantChangeException.java (1)
RedundantChangeException(26-66)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/JsonPatchConflictException.java (1)
JsonPatchConflictException(25-42)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/RemoveIfExistsOperation.java (1)
RemoveIfExistsOperation(40-94)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/RemoveOperation.java (1)
RemoveOperation(58-108)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/ReplaceOperation.java (1)
ReplaceOperation(55-82)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/TestOperation.java (1)
TestOperation(56-73)
common/src/main/java/com/linecorp/centraldogma/common/Query.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-543)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: flaky-tests
- GitHub Check: lint
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: docker-build
🔇 Additional comments (14)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (10)
1-54: LGTM!License header and imports are well-organized. All imported classes are utilized across the test methods.
55-76: LGTM!Test scaffolding with
runForEachTest()returning true ensures proper test isolation. The setup follows JUnit 5 extension patterns correctly.
81-112: LGTM!Comprehensive test covering YAML with comments, various data types, nested structures, and arrays. Assertions properly validate both raw content preservation and parsed JSON structure.
114-173: LGTM!Good coverage of the three retrieval modes: default (no viewRaw), YAML query, and text query. Each test validates the appropriate content representation.
175-222: LGTM!Excellent coverage of JSON Path queries on YAML files, testing both array extraction (
$.users) and scalar extraction ($.settings.theme). This validates a key feature from the PR objectives.
224-237: LGTM!Properly tests the constraint that JSON Path queries cannot be combined with raw view mode, validating the exception type and message.
239-296: LGTM!Multi-file retrieval tests with path patterns are well-structured, validating both viewRaw and non-viewRaw modes with proper assertions on rawContent presence.
298-346: LGTM!Watch functionality tests properly validate that changes are detected and returned correctly, with appropriate handling of viewRaw in the watch context.
396-445: LGTM!Excellent coverage of JSON Patch exception scenarios. The tests properly validate:
RedundantChangeExceptionwhen removing an already-removed fieldRedundantChangeExceptionfor test operations with matching values (no-op)JsonPatchConflictExceptionfor test operations with non-matching values
447-496: LGTM!Good coverage of sequential JSON Patch operations and
.ymlextension support. The multiple operations test validates that changes are applied correctly in sequence, and the.ymltest ensures both YAML file extensions work identically.common/src/main/java/com/linecorp/centraldogma/common/Query.java (4)
19-20: LGTM!The imports are necessary for the new path validation logic.
68-76: LGTM!The new
ofYamlmethod follows the same pattern asofJson, with appropriate path validation and clear documentation about returning YAML content asJsonNode.
64-64: Remove this comment—validation occurs in IdentityQuery, not Query.java line 64, and all existing callers already pass valid JSON paths.The path validation actually happens in
IdentityQuery's constructor (not Query.java), and verification shows all existing call sites already conform to the JSON file path pattern (starting with/and ending with.jsonor.json5). This is not a breaking change for any existing code.
109-126: This review comment is incorrect. BothQuery.ofJson(path)andQuery.of(QueryType.IDENTITY_JSON, path)call the identicalIdentityQueryconstructor with the sameQueryType, which means they perform identical validation. There is no inconsistency:
Query.ofJson("/foo.txt")→ throwsIllegalArgumentException(path doesn't match JSON file pattern)Query.of(QueryType.IDENTITY_JSON, "/foo.txt")→ also throwsIllegalArgumentException(same validation in constructor)Both methods follow the same code path through
IdentityQuery(String path, QueryType queryType), which validates paths based on theirQueryTypeusing the appropriate validator method. The validation is applied consistently.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (1)
97-101: Consider using a typed method to avoid unchecked cast.The unchecked cast is functional but reduces type safety. If the client API supports it, consider using a typed method like
repo.fileAsJson()or similar to avoid the suppression.Alternative approach if available
If the API supports typed file retrieval:
- @SuppressWarnings("unchecked") - final Entry<JsonNode> rawEntry = (Entry<JsonNode>) repo.file('/' + fileName) - .viewRaw(true) - .get() - .join(); + final Entry<JsonNode> rawEntry = repo.fileAsJson('/' + fileName) + .viewRaw(true) + .get() + .join();common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.java (1)
36-51: Simplify the default case to avoid redundant validation.For
IDENTITY_TEXTandIDENTITYtypes,validateFilePathis invoked in the default case (lines 46-50) and then again on line 52. The second call on line 52 is necessary to assign the (potentially normalized) path tothis.path, making the call within the switch redundant for these types.🔎 Suggested refactor to remove redundant validation
switch (queryType) { case IDENTITY_JSON: validateJsonFilePath(path, "path"); break; case IDENTITY_YAML: validateYamlFilePath(path, "path"); break; case JSON_PATH: validateStructuredFilePath(path, "path"); break; - case IDENTITY_TEXT: - case IDENTITY: - default: - validateFilePath(path, "path"); - break; + case IDENTITY_TEXT: + case IDENTITY: + default: + // No additional validation needed; validateFilePath is called below + break; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.javaserver/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (8)
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java (1)
CentralDogma(202-1399)common/src/main/java/com/linecorp/centraldogma/common/PushResult.java (1)
PushResult(31-103)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/AddOperation.java (1)
AddOperation(80-138)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/JsonPatchConflictException.java (1)
JsonPatchConflictException(25-42)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/RemoveIfExistsOperation.java (1)
RemoveIfExistsOperation(40-94)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/RemoveOperation.java (1)
RemoveOperation(58-108)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/ReplaceOperation.java (1)
ReplaceOperation(55-82)common/src/main/java/com/linecorp/centraldogma/common/jsonpatch/TestOperation.java (1)
TestOperation(56-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: docker-build
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: flaky-tests
- GitHub Check: lint
🔇 Additional comments (17)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (16)
55-76: LGTM!The test setup is well-structured with proper test isolation via
runForEachTest()returningtrue, ensuring each test runs with a fresh project/repository.
114-133: LGTM!The test correctly verifies that when
viewRawis not set, only the parsed content is available (rawContent is null).
135-157: LGTM!Good test coverage for
Query.ofYaml()demonstrating YAML-to-JSON conversion.
159-173: LGTM!Straightforward test verifying YAML can be retrieved as plain text using
Query.ofText().
175-222: LGTM!Excellent test demonstrating JSON Path queries on YAML files, including both array and scalar value extraction. The normalization of scalar values to quoted strings in the expected output is the correct behavior.
224-237: LGTM!Proper negative test verifying that JSON_PATH queries cannot be combined with
viewRaw=true, with clear error message validation.
239-271: LGTM!Good coverage of multi-file operations using
PathPatternwith YAML files, verifying both raw and parsed content.
273-296: LGTM!Complements the previous test by verifying multi-file retrieval without
viewRaw, correctly showing null rawContent.
298-324: LGTM!Well-structured watch test verifying both raw and parsed content changes, correctly using revision-based watching.
326-346: LGTM!Complements the previous watch test by verifying behavior without
viewRaw.
348-361: LGTM!Good negative test for watch operations. Note that this throws
IllegalArgumentExceptionatwatch()time (Line 359), whereas thefile().get()version throwsIllegalStateExceptionatget()time (Line 235). This difference appears intentional based on when validation occurs.
365-394: LGTM!Excellent test demonstrating JSON Patch operations on YAML files. The normalization behavior (Line 387:
name: test→name: "test") is correctly documented in the comment and verified.
396-420: LGTM!Well-structured test verifying both successful removal and the
RedundantChangeExceptionon redundant operations, correctly handlingCompletionExceptionwrapping.
422-445: LGTM!Comprehensive test of
TestOperationsemantics: matching value (redundant change) and mismatching value (conflict), with proper exception validation.
447-474: LGTM!Excellent test demonstrating sequential JSON Patch operations (replace, add, remove) on YAML files, with comprehensive final state verification.
476-496: LGTM!Important test verifying JSON Patch operations work correctly with
.ymlextension, ensuring consistency across both YAML file extensions.common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.java (1)
36-52: The return values from type-specific path validators can be safely discarded. All validators (validateJsonFilePath,validateYamlFilePath,validateStructuredFilePath,validateFilePath) return the input path unchanged—they perform validation only via pattern matching without any transformation or normalization. Therefore, discarding their return values on lines 38, 41, and 44 has no functional impact.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.java:
- Around line 36-52: The constructor for IdentityQuery currently runs
type-specific validators in the switch but then always calls
validateFilePath(path, "path") again and assigns that result, causing double
validation and ignored specialized results; refactor the switch in the
IdentityQuery constructor to set a single validatedPath (e.g., validatedPath =
validateJsonFilePath(...)/validateYamlFilePath(...)/validateStructuredFilePath(...)/validateFilePath(...))
for each queryType branch and after the switch assign this.path = validatedPath
so each path is validated exactly once using the appropriate validator based on
queryType.
🧹 Nitpick comments (1)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (1)
365-394: LGTM! Validates JSON Patch add operation on YAML.Properly tests that JSON Patch operations work on YAML files, including validation of YAML normalization (unquoted to quoted strings). The comment at line 385 is helpful for understanding expected behavior.
Optional: Remove unused throws declaration
Line 366 declares
throws JsonParseExceptionbut the method doesn't explicitly throw it. Consider removing if not needed:-void jsonPatchAddOnYamlFile() throws JsonParseException { +void jsonPatchAddOnYamlFile() {(Similarly for other JSON Patch test methods at lines 397, 448, 477)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.javaserver/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-543)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: flaky-tests
- GitHub Check: lint
🔇 Additional comments (16)
server/src/test/java/com/linecorp/centraldogma/server/YamlCrudTest.java (16)
55-76: LGTM! Clean test setup with proper isolation.The test class uses
CentralDogmaExtensionwithrunForEachTest()returning true, ensuring each test gets a fresh project/repository. This provides proper test isolation.
78-112: LGTM! Comprehensive validation of YAML round-trip with raw content.This parameterized test properly validates that YAML files (both .yaml and .yml extensions) can be pushed and retrieved with both raw text and parsed JSON content intact. Good coverage of various YAML data types.
114-133: LGTM! Correctly validates default behavior.Properly tests that without
viewRaw(true), only parsed content is returned (rawContent is null). This aligns with the PR's backward compatibility requirement.
135-157: LGTM! Validates YAML query functionality.Properly tests
Query.ofYaml()method and confirms that YAML content can be retrieved and parsed as JSON with nested property access.
159-173: LGTM! Validates text query on YAML.Confirms that YAML files can be read as plain text using
Query.ofText(), which is useful for clients that want raw content without parsing.
175-222: LGTM! Excellent JSON Path integration test.This test comprehensively validates JSON Path queries over YAML content, including array extraction and nested property access. The contentAsText() validation at lines 201-209 nicely confirms YAML formatting is preserved in query results.
Note: Line 221's assertion compares a JsonNode to a quoted string literal (
"\"dark\""), which appears intentional for testing scalar JSON Path results.
224-237: LGTM! Proper validation of incompatible operations.Correctly tests that JSON Path queries cannot be combined with
viewRaw(true), which makes sense since JSON Path operates on parsed structures. Good error message validation.
239-271: LGTM! Validates bulk YAML file retrieval.Properly tests that multiple YAML files can be retrieved using path patterns with
viewRaw(true), confirming both raw and parsed content are available for each file.
273-296: LGTM! Validates default behavior for bulk operations.Confirms that bulk file retrieval without
viewRawreturns only parsed content (rawContent is null), consistent with single-file behavior.
298-324: LGTM! Validates watch semantics for YAML.Properly tests that file watching works with YAML files and
viewRaw(true), confirming that changes are detected and both raw and parsed content are available.
326-346: LGTM! Validates default watch behavior.Confirms that watching YAML files without
viewRawreturns only parsed content, consistent with the get() API behavior.
396-420: LGTM! Validates removeIfExists idempotency.Properly tests that
removeIfExistssuccessfully removes a field and that attempting the same operation again throwsRedundantChangeException. Good use of cause chain validation for async exceptions.
422-445: LGTM! Validates JSON Patch test operation semantics.Correctly tests both scenarios for the
testoperation: matching values throwRedundantChangeException(no-op), and mismatches throwJsonPatchConflictException. This validates proper conflict detection.
447-474: LGTM! Validates sequential JSON Patch operations.Excellent test of applying multiple JSON Patch operations (replace, add, remove) sequentially and validating the cumulative effect. This confirms YAML files support the full JSON Patch workflow.
476-496: LGTM! Validates .yml extension support.Confirms that the
.ymlextension is treated equivalently to.yamlfor JSON Patch operations, ensuring feature parity across both common YAML file extensions.
348-361: No action needed—exception types are intentionally different and semantically correct.The difference between
IllegalArgumentException(thrown bywatch().viewRaw(true)) andIllegalStateException(thrown byfile().viewRaw(true).get()) is intentional and follows Java exception semantics:
IllegalArgumentExceptionis thrown when an invalid argument is passed to theviewRaw(boolean)method itselfIllegalStateExceptionis thrown when the object is in an invalid state during theget()executionEach exception is thrown at the appropriate point in the call chain where the incompatibility is detected.
| switch (queryType) { | ||
| case IDENTITY_JSON: | ||
| validateJsonFilePath(path, "path"); | ||
| break; | ||
| case IDENTITY_YAML: | ||
| validateYamlFilePath(path, "path"); | ||
| break; | ||
| case JSON_PATH: | ||
| validateStructuredFilePath(path, "path"); | ||
| break; | ||
| case IDENTITY_TEXT: | ||
| case IDENTITY: | ||
| default: | ||
| validateFilePath(path, "path"); | ||
| break; | ||
| } | ||
| this.path = validateFilePath(path, "path"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate redundant path validation.
The constructor performs type-specific validation in the switch statement (lines 36-51) but discards the return values, then calls validateFilePath(path, "path") again on line 52. This results in:
- Double validation for all query types
validateFilePathis called twice forIDENTITY_TEXT,IDENTITY, and default cases- Confusing code flow where the more specific validation result is ignored
🔎 Proposed refactor to use a single validation call per type
IdentityQuery(String path, QueryType queryType) {
+ final String validatedPath;
switch (queryType) {
case IDENTITY_JSON:
- validateJsonFilePath(path, "path");
+ validatedPath = validateJsonFilePath(path, "path");
break;
case IDENTITY_YAML:
- validateYamlFilePath(path, "path");
+ validatedPath = validateYamlFilePath(path, "path");
break;
case JSON_PATH:
- validateStructuredFilePath(path, "path");
+ validatedPath = validateStructuredFilePath(path, "path");
break;
case IDENTITY_TEXT:
case IDENTITY:
default:
- validateFilePath(path, "path");
+ validatedPath = validateFilePath(path, "path");
break;
}
- this.path = validateFilePath(path, "path");
+ this.path = validatedPath;
this.queryType = requireNonNull(queryType, "queryType");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (queryType) { | |
| case IDENTITY_JSON: | |
| validateJsonFilePath(path, "path"); | |
| break; | |
| case IDENTITY_YAML: | |
| validateYamlFilePath(path, "path"); | |
| break; | |
| case JSON_PATH: | |
| validateStructuredFilePath(path, "path"); | |
| break; | |
| case IDENTITY_TEXT: | |
| case IDENTITY: | |
| default: | |
| validateFilePath(path, "path"); | |
| break; | |
| } | |
| this.path = validateFilePath(path, "path"); | |
| IdentityQuery(String path, QueryType queryType) { | |
| final String validatedPath; | |
| switch (queryType) { | |
| case IDENTITY_JSON: | |
| validatedPath = validateJsonFilePath(path, "path"); | |
| break; | |
| case IDENTITY_YAML: | |
| validatedPath = validateYamlFilePath(path, "path"); | |
| break; | |
| case JSON_PATH: | |
| validatedPath = validateStructuredFilePath(path, "path"); | |
| break; | |
| case IDENTITY_TEXT: | |
| case IDENTITY: | |
| default: | |
| validatedPath = validateFilePath(path, "path"); | |
| break; | |
| } | |
| this.path = validatedPath; | |
| this.queryType = requireNonNull(queryType, "queryType"); | |
| } |
🤖 Prompt for AI Agents
In @common/src/main/java/com/linecorp/centraldogma/common/IdentityQuery.java
around lines 36-52, The constructor for IdentityQuery currently runs
type-specific validators in the switch but then always calls
validateFilePath(path, "path") again and assigns that result, causing double
validation and ignored specialized results; refactor the switch in the
IdentityQuery constructor to set a single validatedPath (e.g., validatedPath =
validateJsonFilePath(...)/validateYamlFilePath(...)/validateStructuredFilePath(...)/validateFilePath(...))
for each queryType branch and after the switch assign this.path = validatedPath
so each path is validated exactly once using the appropriate validator based on
queryType.
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
Looks great all in all. Left some questions and suggestions.
...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
Outdated
Show resolved
Hide resolved
...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
Outdated
Show resolved
Hide resolved
...ria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
Outdated
Show resolved
Hide resolved
| } else { | ||
| jsonBytes = Jackson.writeValueAsBytes(jsonNode); | ||
| } | ||
| final byte[] jsonBytes = Jackson.writeValueAsPrettyString(jsonNode).getBytes(UTF_8); |
There was a problem hiding this comment.
Then, can we just use writeValueAsBytes?
| new InsertJson(changePath, inserter, newJsonNode, change.rawContent())); | ||
| String newJson = rawContent; | ||
| if (newJson == null) { | ||
| // Use pretty format for readability in the web UI. |
There was a problem hiding this comment.
So now, we always store a JSON file with the pretty format. Is that correct? If so, do you think it's okay to store a big file (e.g. 50000 lines of JSON)?
There was a problem hiding this comment.
So now, we always store a JSON file with the pretty format. Is that correct?
I guess a formatted JSON content is usually passed as an input for ofJsonUpsert by git mirroring or push API. If ofJsonPatch() collapses it, the diff becomes noisy and harder to read when using the viewRaw option.
do you think it's okay to store a big file (e.g. 50000 lines of JSON)?
I don't think storing itself is a problem. Instead, the web UI could be unresponsive and frozen when rendering a large single line.
There was a problem hiding this comment.
Thanks for the explanation. I was just curious about it. 😉
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/linecorp/centraldogma/common/Change.java (1)
376-404: Add YAML support to thefromFilemethod.The
fromFilemethod creates aChangefrom a file based on itsEntryType, but the switch statement at lines 396-403 only handlesJSONandTEXTcases. With YAML now being a first-class type, attempting to import YAML files using this method will hit the default case and throw an "unexpected entry type" error.🔎 Proposed fix to add YAML case
switch (entryType) { case JSON: return ofJsonUpsert(targetPath, content); + case YAML: + return ofYamlUpsert(targetPath, content); case TEXT: return ofTextUpsert(targetPath, content); default: throw new Error("unexpected entry type: " + entryType); }
🤖 Fix all issues with AI agents
In @common/src/main/java/com/linecorp/centraldogma/common/Change.java:
- Around line 104-113: The text-based API methods ofTextUpsert and ofTextPatch
currently only prevent JSON files but still allow YAML; update both methods to
reject YAML files the same way as JSON by checking the path extension (e.g.,
.yaml and .yml) and throwing an IllegalArgumentException with a clear message
directing callers to use ofYamlUpsert for YAML content; ensure you mirror the
existing JSON validation logic and error wording used elsewhere (use the same
pattern as the JSON check and reference ChangeType.TEXT for these methods,
leaving ofYamlUpsert/ChangeType.UPSERT_YAML unchanged).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.javacommon/src/main/java/com/linecorp/centraldogma/common/Change.javawebapp/package.jsonwebapp/src/dogma/common/components/editor/FileEditor.tsxwebapp/src/dogma/features/file/NewFile.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
- webapp/src/dogma/features/file/NewFile.tsx
- webapp/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-05T08:17:09.929Z
Learnt from: ikhoon
Repo: line/centraldogma PR: 1229
File: webapp/src/dogma/features/file/StructuredFileSupport.ts:22-33
Timestamp: 2026-01-05T08:17:09.929Z
Learning: In Central Dogma's web app, file extensions are normalized to Monaco editor language identifiers before being passed to content parsing functions. Specifically, the `.yml` extension is normalized to the `yaml` language identifier in FileEditor.tsx before reaching functions like `parseContent` in StructuredFileSupport.ts. Monaco editor uses 'yaml' as the standard language identifier for both `.yaml` and `.yml` files.
Applied to files:
webapp/src/dogma/common/components/editor/FileEditor.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: docker-build
- GitHub Check: flaky-tests
- GitHub Check: lint
🔇 Additional comments (3)
webapp/src/dogma/common/components/editor/FileEditor.tsx (2)
30-30: LGTM! Clean abstraction for structured file detection.The import of
isStructuredFilereplaces hardcoded language checks and provides a centralized, maintainable way to identify structured formats (JSON, JSON5, YAML).
182-189: LGTM! Structured file integration is correct and well-designed.The changes correctly extend JSON Path functionality to all structured formats (JSON, JSON5, YAML):
- Line 182:
isStructuredFile(language)cleanly replaces the hardcoded JSON check for renderingJsonPathLegend.- Lines 184-189: The updated
JsonPathprops are more descriptive (originalContentinstead ofjsonContent) and the addedlanguageprop enables format-aware parsing.The integration aligns well with the PR's goal of first-class YAML support.
common/src/main/java/com/linecorp/centraldogma/common/Change.java (1)
242-256: LGTM: Enhanced documentation and StructuredChange migration.The documentation now clearly states that JSON patch operations apply to JSON, JSON5, and YAML files, and warns about normalization behavior. The StructuredChange migration is consistent across all overloads.
| /** | ||
| * Returns a newly-created {@link Change} whose type is {@link ChangeType#UPSERT_YAML}. | ||
| * | ||
| * @param path the path of the file | ||
| * @param yamlText the content of the file | ||
| */ | ||
| static Change<JsonNode> ofYamlUpsert(String path, String yamlText) { | ||
| requireNonNull(yamlText, "yamlText"); | ||
| return new StructuredChange(path, ChangeType.UPSERT_YAML, yamlText); | ||
| } |
There was a problem hiding this comment.
Past review concern remains unaddressed: Prevent YAML files in text-based APIs.
A previous review comment flagged that ofTextUpsert and ofTextPatch should also prevent YAML files (lines 70-78, 148-154, 178-187), just as they currently prevent JSON files. This ensures YAML content is handled as structured data through ofYamlUpsert instead of text operations, maintaining consistency with the existing JSON validation flow.
🤖 Prompt for AI Agents
In @common/src/main/java/com/linecorp/centraldogma/common/Change.java around
lines 104 - 113, The text-based API methods ofTextUpsert and ofTextPatch
currently only prevent JSON files but still allow YAML; update both methods to
reject YAML files the same way as JSON by checking the path extension (e.g.,
.yaml and .yml) and throwing an IllegalArgumentException with a clear message
directing callers to use ofYamlUpsert for YAML content; ensure you mirror the
existing JSON validation logic and error wording used elsewhere (use the same
pattern as the JSON check and reference ChangeType.TEXT for these methods,
leaving ofYamlUpsert/ChangeType.UPSERT_YAML unchanged).
3 participants
Motivation:
This PR adds YAML support in a similar way to JSON5 and enables the use of advances features such as JSON Path and JSON Patch.
Modifications:
ChangeType.UPSERT_YAMLandEntryType.YAMLfor representing YAML files.YAMLMapperof Jackson is used to (de)serialize the content. The default configuration is copied from Add better YAML supports #616JsonChangeis renamed toStructuredChangeto include structured formats such as JSON, JSON5, YAML.JsonNode, after which JSON Path and Patch operations can be applied.typefield inEntryDtoand fixed the client logic to guess theEntryTypefrom the file extension.viewRawis not set, the server returnsEntryType.TEXTinstead ofEntryType.YAML.EntryDto.type, we aim to reduce the migration effort required when additional entry types are added later./) is added to directory paths to make it easier to determine theEntryTypefrom the path.JsonNodeto Git to improve readability in web UI.Result:
Co-Authored-By: chacham [email protected]