Render template files using variables#1244
Conversation
Motivation:
If template files are supported, specific values in a configuration file
can be overridden, and sensitive information such as database passwords
or tokens can be mirrored without being exposed to the upstream Git
repository. For example:
```json
{
"env": "${vars.phase}"
"db": {
"host": "${vars.db.host}",
"pw": "${secrets.db.password}",
}
}
```
As a first step, this PR implements REST API to store variables in
Central Dogma. Follow-up work includes template processing, a UI
for variable UI, and secret support.
Modifications:
- Implemented `VariableServiceV1` to store variables at the project
level and repository level.
- The prefix of each API is as follows:
- `/api/projects/{projectName}/variables
- `/api/projects/{projectName}/repos/{repoName}/variables
- `JSON` and `STRING` are supported for variables.
- Introduced `CrudOperation` to perform CRUD operations on arbitrary
repository and paths.
- `GitCrudRepository` now delegates the logic to `CrudOperation`
- Moved `HasRevision` to
`com.linecorp.centraldogma.server.storage.repository` to expose it as
a public API
Result:
Variables can be stored and retrieved via REST API at the project and
repository level.
📝 WalkthroughWalkthroughThreads template parameters through client APIs and watchers, adds server-side FreeMarker-based templating (Templater), extends Entry/DTO with templateRevision, introduces transformer-aware repository/watch APIs, converters, tests, dependency, and exception handling to support variable-driven template rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerAPI
participant WatchService
participant Repository
participant Templater
participant FreeMarker
Client->>ServerAPI: GET /files?applyTemplate=true&variableFile=...&templateRevision=...
ServerAPI->>ServerAPI: convert query -> TemplateParams
ServerAPI->>Repository: get(revision, query, transformer)
Repository-->>ServerAPI: Entry (may include templateRevision)
alt TemplateParams.applyTemplate == true
ServerAPI->>WatchService: request get/watch with TemplateParams
WatchService->>Templater: render(repo, entry, variableFile, templateRevision)
Templater->>Repository: load variable files (project/repo/file)
Templater->>Templater: merge variables (project < repo < file/client)
Templater->>FreeMarker: process template with merged vars
FreeMarker-->>Templater: rendered content
Templater-->>WatchService: transformed Entry (with templateRevision)
WatchService-->>ServerAPI: transformed Entry
end
ServerAPI->>ServerAPI: convert Entry -> EntryDto (include templateRevision)
ServerAPI-->>Client: HTTP 200 with EntryDto
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested labels
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
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 (5)
client/java-armeria-legacy/src/main/java/com/linecorp/centraldogma/client/armeria/legacy/LegacyCentralDogma.java (3)
225-234: Inconsistent handling of unsupported features.The
applyTemplateandvariableFileparameters are silently ignored, but elsewhere in this class (lines 342, 470, 496), unsupported features throw exceptions viacheckArgument(). For consistency, consider validating these parameters to fail fast:Suggested validation
public <T> CompletableFuture<Entry<T>> getFile(String projectName, String repositoryName, Revision revision, Query<T> query, boolean viewRaw, boolean applyTemplate, `@Nullable` String variableFile) { requireNonNull(query, "query"); + checkArgument(!applyTemplate && variableFile == null, + "applyTemplate and variableFile are not supported in LegacyCentralDogma."); return maybeNormalizeRevision(projectName, repositoryName, revision).thenCompose(normRev -> {
285-295: Same inconsistency applies here.The
applyTemplateandvariableFileparameters are silently ignored. Apply the same validation pattern as suggested forgetFile.Suggested validation
public CompletableFuture<Map<String, Entry<?>>> getFiles(String projectName, String repositoryName, Revision revision, PathPattern pathPattern, boolean viewRaw, boolean applyTemplate, `@Nullable` String variableFile) { requireNonNull(pathPattern, "pathPattern"); + checkArgument(!applyTemplate && variableFile == null, + "applyTemplate and variableFile are not supported in LegacyCentralDogma."); return maybeNormalizeRevision(projectName, repositoryName, revision).thenCompose(normRev -> {
493-506: Validate unsupported template parameters and update comment.The comment mentions
viewRawandapplyTemplatebut omitsvariableFileandvariableRevision. Also, these should be validated likeerrorOnEntryNotFoundon line 496.Suggested fix
public <T> CompletableFuture<Entry<T>> watchFile(String projectName, String repositoryName, Revision lastKnownRevision, Query<T> query, long timeoutMillis, boolean errorOnEntryNotFound, boolean viewRaw, boolean applyTemplate, `@Nullable` String variableFile, `@Nullable` Revision variableRevision) { checkArgument(!errorOnEntryNotFound, "errorOnEntryNotFound is not supported in LegacyCentralDogma."); + checkArgument(!applyTemplate && variableFile == null && variableRevision == null, + "applyTemplate, variableFile and variableRevision are not supported in LegacyCentralDogma."); validateProjectAndRepositoryName(projectName, repositoryName); requireNonNull(lastKnownRevision, "lastKnownRevision"); requireNonNull(query, "query"); final CompletableFuture<WatchFileResult> future = run(callback -> { - // viewRaw and applyTemplate are not supported in LegacyCentralDogma. + // viewRaw, applyTemplate, variableFile and variableRevision are not supported in LegacyCentralDogma. client.watchFile(projectName, repositoryName,server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.java (2)
83-101: Double sanitization and asymmetric comparison in UPSERT_JSON.Two issues in this block:
Comparison asymmetry (lines 84-87):
rawContentis sanitized before comparison, butoldRawContentis not. This causes false-positive change detection if the old content differs only by characters thatsanitizeTextnormalizes (e.g., line endings, trailing whitespace).Redundant sanitization (line 101):
sanitizeText(newJson)is called unconditionally, but:
- When
rawContent != null: already sanitized at line 84- When
rawContent == null: already sanitized at line 99Proposed fix
if (rawContent != null) { rawContent = sanitizeText(rawContent); // If rawContent is provided, compare the raw JSON text. - final String oldRawContent = oldContent != null ? new String(oldContent, UTF_8) : null; + final String oldRawContent = oldContent != null ? sanitizeText(new String(oldContent, UTF_8)) : null; hasChanges = !rawContent.equals(oldRawContent); } else { // Otherwise, compare the parsed JSON nodes. final JsonNode oldJsonNode = toJsonNode(changePath, oldContent); hasChanges = !Objects.equals(newJsonNode, oldJsonNode); } if (hasChanges) { String newJson = rawContent; if (newJson == null) { // Use pretty format for readability in the web UI. newJson = Jackson.writeValueAsPrettyString(newJsonNode); newJson = sanitizeText(newJson); } - newJson = sanitizeText(newJson); applyPathEdit(dirCache, new InsertText(changePath, inserter, newJson)); numEdits++; }
107-125: Double sanitization and asymmetric comparison in UPSERT_YAML.Same issues as the JSON case:
Comparison asymmetry (lines 111 vs 115):
newYamlis sanitized at line 111, butoldYamlis not sanitized. This causes false-positive change detection.Redundant sanitization (line 121):
newYamlis already sanitized at line 111, making line 121 redundant.Proposed fix
case UPSERT_YAML: String newYaml = change.rawContent(); // rawContent must not be null for YAML upsert. assert newYaml != null; newYaml = sanitizeText(newYaml); final String oldYaml; if (oldContent != null) { - oldYaml = new String(oldContent, UTF_8); + oldYaml = sanitizeText(new String(oldContent, UTF_8)); } else { oldYaml = null; } if (!newYaml.equals(oldYaml)) { - newYaml = sanitizeText(newYaml); applyPathEdit(dirCache, new InsertText(changePath, inserter, newYaml)); numEdits++; } break;
🤖 Fix all issues with AI agents
In
`@client/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.java`:
- Around line 264-268: The toString() of the anonymous/getFile request omits the
applyTemplate flag; update the toString() in ReplicationLagTolerantCentralDogma
(the overridden toString() that builds "getFile(...)" using projectName,
repositoryName, revision, query, viewRaw, variableFile) to include the
applyTemplate parameter in the same position/format as getFiles' toString so the
string representation reflects applyTemplate as well.
In `@dependencies.toml`:
- Around line 181-184: Add FreeMarker to the project NOTICE and include its
Apache-2.0 license file: update NOTICE.txt to add an entry mentioning the new
dependency "org.freemarker:freemarker" (the libraries.freemarker
module/version.ref addition) with its version and attribution, and add a new
LICENSE file containing the Apache License 2.0 text (e.g.,
LICENSE.apache-2.0.txt) corresponding to FreeMarker; ensure the NOTICE entry and
the LICENSE filename clearly reference FreeMarker and the module coordinates so
compliance reviewers can map the dependency to its license.
In
`@it/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/GitMirrorIntegrationTest.java`:
- Around line 224-226: No change required: the getFiles call has been correctly
adapted to the expanded signature—ensure the call to getFiles(projName,
REPO_FOO, rev3, PathPattern.all(), true, false, null) remains with viewRaw=true
and applyTemplate=false (variableFile null) so raw content is preserved for this
mirroring test; keep the same invocation and parameter order around getFiles,
PathPattern.all, projName, REPO_FOO, and rev3.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java`:
- Around line 30-37: The class/method Javadoc incorrectly refer to Query (likely
copy-pasted); update the documentation for TemplateParamsConverter and its
conversion method to state that it converts incoming requests to TemplateParams
(not Query) when the request contains a valid template path, and return null
otherwise; ensure references mention TemplateParams and
RequestConverterFunction/TemplateParamsConverter so readers can find the
converter and its behavior.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java`:
- Around line 146-152: The code calls repo.getOrNull(...).thenApply(entry0 -> {
... }) without checking for a null entry0, which can cause an NPE when the
variable file is missing; update the lambda in Templater to first check if
entry0 == null and if so throw a TemplateProcessingException (including
variableFile and revision in the message) before inspecting entry0.type(), then
proceed to call parseVariableFile(entry0) when non-null.
In
`@server/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java`:
- Around line 157-159: The assertion uses an inconsistent leading space in the
expected JSON string; update the expectation in the assertThatJson call
(assertThatJson(entry.content()).isEqualTo(...)) to remove the leading space so
it matches other tests and normalized output (use "{\"environment\":
\"staging\"}" as the expected string), or alternatively normalize/trim
entry.content() before asserting if the space is intentional across other tests.
- Around line 1820-1825: The deleteVariable implementation calls
httpClient.prepare().delete(path).execute() but doesn't check the HTTP response;
capture the response (e.g., assign the result of execute() to a response
variable) and assert its status code matches the expected success code (mirror
the checks used in createVariable/updateVariable, e.g., assertEquals(200 or 204,
response.getStatusCode()) or equivalent), ensuring deleteVariable fails the test
when the server returns a non-success status.
- Around line 1797-1812: The updateVariable method declares repoName as a
non-null String but treats and is invoked with null; annotate the repoName
parameter with `@Nullable` to reflect its actual nullability and update imports
(e.g., javax.annotation.Nullable or the project's preferred Nullable) so the
test code and IDE/compiler understand the intent; keep the method signature
public/private void updateVariable(String projectName, `@Nullable` String
repoName, String id, VariableType type, String value) and run tests to ensure no
other nullability warnings remain.
🧹 Nitpick comments (12)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (3)
83-95: Cache key uses Entry which includes mutable-like revision semantics.The cache uses
Entry<?>as the key. SinceEntryimplementsequals()andhashCode()based on revision, path, content, type, and variableRevision, entries with different revisions but same content will create duplicate template objects. This is likely intentional for correctness, but consider if caching by(path, content)tuple would be more efficient.
210-231: Consider using TemplateProcessingException instead of IllegalStateException for parse failures.Lines 218 and 224 throw
IllegalStateExceptionfor JSON/YAML parse errors after template processing. Since this is user-facing content (template output may be invalid),TemplateProcessingExceptionwould be more appropriate and consistent with the rest of the class.♻️ Proposed fix
case JSON: try { return Entry.ofJson(entry.revision(), entry.path(), content); } catch (JsonProcessingException e) { - throw new IllegalStateException(e); + throw new TemplateProcessingException( + "Template output is not valid JSON for " + entry.path() + ".\n" + e.getMessage(), e); } case YAML: try { return Entry.ofYaml(entry.revision(), entry.path(), content); } catch (JsonParseException e) { - throw new IllegalStateException(e); + throw new TemplateProcessingException( + "Template output is not valid YAML for " + entry.path() + ".\n" + e.getMessage(), e); }
226-230: Avoid throwing Error for unreachable code paths.
throw new Error()is unconventional for unreachable branches. ConsiderAssertionErrorwith a descriptive message, or simply omit the case (let the compiler warn if a new EntryType is added).♻️ Proposed fix
case DIRECTORY: default: // Should not reach here. - throw new Error(); + throw new AssertionError("Unexpected entry type: " + entry.type());common/src/test/java/com/linecorp/centraldogma/common/EntryTest.java (1)
164-171: LGTM!The new
testVariableVersion()method provides good coverage for the new variable revision functionality:
- Verifies that a newly created entry has
nullvariableRevision- Verifies that
withVariableRevision()creates a new entry with the specified variable revision while preserving the original revisionConsider adding an assertion to verify that the original
entryremains unchanged after callingwithVariableRevision()to ensure immutability.💡 Optional: Verify immutability of original entry
void testVariableVersion() { final Entry<String> entry = Entry.ofText(new Revision(10), "/a.txt", "a"); assertThat(entry.variableRevision()).isNull(); final Entry<String> entryWithVarRev = entry.withVariableRevision(new Revision(20)); assertThat(entryWithVarRev.revision()).isEqualTo(new Revision(10)); assertThat(entryWithVarRev.variableRevision()).isEqualTo(new Revision(20)); + // Verify original entry remains unchanged (immutability) + assertThat(entry.variableRevision()).isNull(); }client/java/src/main/java/com/linecorp/centraldogma/client/AbstractFileRequest.java (1)
58-77: Consider resettingvariableFilewhenapplyTemplate(false)is called.When
applyTemplate(false)is called, thevariableFilefield is not reset. This could lead to unexpected behavior if a user previously set a variable file and then disables template processing:request.applyTemplate("/vars/prod.json") // sets applyTemplate=true, variableFile="/vars/prod.json" .applyTemplate(false); // sets applyTemplate=false, variableFile still "/vars/prod.json"If
applyTemplate()is later called withtrueagain, it would unexpectedly use the previously set variable file.♻️ Proposed fix
public SELF applyTemplate(boolean applyTemplate) { this.applyTemplate = applyTemplate; + if (!applyTemplate) { + this.variableFile = null; + } return self(); }server/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java (1)
1258-1270: Potential test flakiness due to timing-based assertions.Using
Thread.sleep(1000)to assert that a future is not done can lead to flaky tests if the server responds faster than expected under certain conditions. Consider using a shorter sleep or using Awaitility with a negative condition.♻️ Suggested improvement using Awaitility
- Thread.sleep(1000); - // The future should not be done yet because the JSON path query result has not changed. - assertThat(future).isNotDone(); + // The future should not be done yet because the JSON path query result has not changed. + // Wait briefly and verify it's still pending + await().pollDelay(500, java.util.concurrent.TimeUnit.MILLISECONDS) + .atMost(1, java.util.concurrent.TimeUnit.SECONDS) + .untilAsserted(() -> assertThat(future).isNotDone());client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java (1)
823-828: Consider adding Javadoc for the new template-related parameters.The
watchFilemethod now accepts several new parameters (applyTemplate,variableFile,variableRevision) but the Javadoc doesn't explain their purpose. This is important as this is a public API method.📖 Suggested documentation addition
/** * Waits for the file matched by the specified {`@link` Query} to be changed since the specified * {`@code` lastKnownRevision}. If the file does not exist and {`@code` errorOnEntryNotFound} is {`@code` true}, * the returned {`@link` CompletableFuture} will be completed exceptionally with * {`@link` EntryNotFoundException}. If no changes were made within the specified {`@code` timeoutMillis}, * the returned {`@link` CompletableFuture} will be completed with {`@code` null}. * It is recommended to specify the largest {`@code` timeoutMillis} allowed by the server. - * This method is equivalent to calling: - * <pre>{`@code` - * CentralDogma dogma = ... - * dogma.forRepo(projectName, repositoryName) - * .watch(query) - * .timeoutMillis(timeoutMillis) - * .errorOnEntryNotFound(errorOnEntryNotFound) - * .start(lastKnownRevision); - * }</pre> * + * `@param` viewRaw whether to view the raw content of the file + * `@param` applyTemplate whether to apply template processing using variables + * `@param` variableFile the path to a custom variable file, or {`@code` null} to use default variable files + * `@param` variableRevision the last known revision of the variable file, or {`@code` null} * `@return` the {`@link` Entry} which contains the latest known {`@link` Query} result. * {`@code` null} if the file was not changed for {`@code` timeoutMillis} milliseconds * since the invocation of this method. Even before the timeout, the watch may return null * earlier due to issues such as server restart. * {`@link` EntryNotFoundException} is raised if the target does not exist. */server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java (1)
26-26: Unused import.The
Queryimport is not used in this file and appears to be a leftover from the copy-pasted Javadoc.🧹 Suggested fix
-import com.linecorp.centraldogma.common.Query;server/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.java (1)
71-76: Consider adding a guard inwithVariableRevision.If
withVariableRevisionis called on a disabledTemplateParamsinstance, it would create a new instance withapplyTemplate=falsebut a non-nullvariableRevision, which may be semantically inconsistent. However, this is likely only called on enabled instances, so this is a minor defensive coding suggestion.🛡️ Optional defensive guard
public TemplateParams withVariableRevision(Revision variableRevision) { + if (!applyTemplate) { + return this; + } if (Objects.equals(variableRevision, this.variableRevision)) { return this; } return new TemplateParams(applyTemplate, variableFile, variableRevision); }server/src/main/java/com/linecorp/centraldogma/server/internal/api/WatchService.java (1)
118-139: Approve with minor suggestion: Consider using a local variable instead of reassigning the parameter.The logic is correct—disabling the transformer when
applyTemplate()is false. However, reassigning method parameters can reduce readability.♻️ Optional: Use a local variable
public <T> CompletableFuture<Entry<T>> watchFile( Repository repo, Revision lastKnownRevision, Query<T> query, long timeoutMillis, boolean errorOnEntryNotFound, TemplateParams templateParams, `@Nullable` Function<Revision, EntryTransformer<T>> transformerFactory) { final ServiceRequestContext ctx = RequestContext.current(); updateRequestTimeout(ctx, timeoutMillis); - if (!templateParams.applyTemplate()) { - transformerFactory = null; - } + final Function<Revision, EntryTransformer<T>> effectiveTransformerFactory = + templateParams.applyTemplate() ? transformerFactory : null; final CompletableFuture<Entry<T>> result = repo.watch(lastKnownRevision, query, errorOnEntryNotFound, templateParams.variableFile(), templateParams.variableRevision(), - transformerFactory); + effectiveTransformerFactory);server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryWatcher.java (1)
140-177: Dual-watch race semantics are correct but could benefit from a clarifying comment.The design where both
repoFutureanddogmaFuturerace to complete the samefutureis intentional—whichever source changes first triggers the update. SinceCompletableFuture.complete()is idempotent (returnsfalseif already completed), this is safe.♻️ Optional: Add clarifying comment
+ // Race between repo and dogma watches - first change wins. + // CompletableFuture.complete() is idempotent, so the second completion is ignored. repoFuture.handle((newRev, cause) -> {server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java (1)
285-301: Consider potential memory/concurrency impact with large result sets.All entries are transformed in parallel via
CompletableFutures.allAsList. Iffindreturns a large number of entries, this could create many concurrent transformation futures.For typical use cases with limited file counts this should be fine, and the
Templater's Caffeine cache will help. However, if there's a possibility of very large result sets, consider adding bounded parallelism or documenting this behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
client/java-armeria-legacy/src/main/java/com/linecorp/centraldogma/client/armeria/legacy/LegacyCentralDogma.javaclient/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.javaclient/java/src/main/java/com/linecorp/centraldogma/client/AbstractFileRequest.javaclient/java/src/main/java/com/linecorp/centraldogma/client/AbstractWatcher.javaclient/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.javaclient/java/src/main/java/com/linecorp/centraldogma/client/FileRequest.javaclient/java/src/main/java/com/linecorp/centraldogma/client/FileWatcher.javaclient/java/src/main/java/com/linecorp/centraldogma/client/FilesRequest.javaclient/java/src/main/java/com/linecorp/centraldogma/client/FilesWatcher.javaclient/java/src/main/java/com/linecorp/centraldogma/client/Latest.javaclient/java/src/main/java/com/linecorp/centraldogma/client/WatchRequest.javaclient/java/src/main/java/com/linecorp/centraldogma/client/WatcherRequest.javaclient/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.javaclient/java/src/test/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogmaTest.javacommon/src/main/java/com/linecorp/centraldogma/common/Entry.javacommon/src/main/java/com/linecorp/centraldogma/common/TemplateProcessingException.javacommon/src/main/java/com/linecorp/centraldogma/internal/api/v1/EntryDto.javacommon/src/test/java/com/linecorp/centraldogma/common/EntryTest.javadependencies.tomlit/mirror/src/test/java/com/linecorp/centraldogma/it/mirror/git/GitMirrorIntegrationTest.javaserver/build.gradleserver/src/main/java/com/linecorp/centraldogma/server/CentralDogma.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/DtoConverter.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/HttpApiExceptionHandler.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/WatchService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryWrapper.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultChangesApplier.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/EntryTransformer.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryWatcher.javaserver/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java
🧰 Additional context used
🧬 Code graph analysis (12)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.java (1)
TemplateParams(27-103)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/HttpApiExceptionHandler.java (1)
common/src/main/java/com/linecorp/centraldogma/common/TemplateProcessingException.java (1)
TemplateProcessingException(22-39)
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryWatcher.java (2)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-338)common/src/main/java/com/linecorp/centraldogma/common/EntryNotFoundException.java (1)
EntryNotFoundException(22-76)
common/src/test/java/com/linecorp/centraldogma/common/EntryTest.java (1)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-338)
client/java/src/main/java/com/linecorp/centraldogma/client/WatcherRequest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-543)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java (4)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java (1)
TemplateParamsConverter(33-50)server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/WatchRequestConverter.java (2)
WatchRequestConverter(44-188)WatchRequest(157-187)server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (1)
Templater(61-287)server/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.java (1)
TemplateParams(27-103)
server/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java (4)
testing-internal/src/main/java/com/linecorp/centraldogma/testing/internal/auth/TestAuthMessageUtil.java (1)
TestAuthMessageUtil(44-146)common/src/main/java/com/linecorp/centraldogma/common/PushResult.java (1)
PushResult(31-103)common/src/main/java/com/linecorp/centraldogma/common/TemplateProcessingException.java (1)
TemplateProcessingException(22-39)testing/junit/src/main/java/com/linecorp/centraldogma/testing/junit/CentralDogmaExtension.java (1)
CentralDogmaExtension(60-278)
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java (1)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-338)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (2)
common/src/main/java/com/linecorp/centraldogma/common/MergeSource.java (1)
MergeSource(27-95)common/src/main/java/com/linecorp/centraldogma/common/TemplateProcessingException.java (1)
TemplateProcessingException(22-39)
server/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.java (1)
TemplateParams(27-103)
client/java/src/main/java/com/linecorp/centraldogma/client/AbstractFileRequest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Util.java (1)
Util(38-543)
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryUtil.java (1)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-338)
⏰ 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-macos-latest-jdk-25
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-ubuntu-latest-jdk-25-snapshot
- GitHub Check: build-ubuntu-latest-jdk-21
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: flaky-tests
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: docker-build
- GitHub Check: lint
🔇 Additional comments (72)
client/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.java (4)
272-292: LGTM!The new template parameters are correctly propagated to the delegate, and the
toString()representation includes all parameters for debugging visibility.
300-300: LGTM!Diamond operator modernization is appropriate for Java 9+ anonymous class instantiation.
481-513: LGTM!The
watchFilemethod correctly propagates all new template-related parameters (viewRaw,applyTemplate,variableFile,variableRevision) to the delegate and includes them in thetoString()for debugging.
529-530: LGTM!Diamond operator usage is consistent with other modernizations in this file.
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (11)
72-73: LGTM!The new imports for
QueryParamsandQueryParamsBuildersupport the refactoring from manual query string concatenation to a cleaner, builder-based approach that handles URL encoding automatically.
160-160: LGTM!Adding
TemplateProcessingExceptionto the exception factories map enables proper deserialization of template-related errors from the server.
473-507: LGTM!The refactoring to use
QueryParamsBuilderimproves readability and handles URL encoding automatically. The conditional addition ofvariableFileonly whenapplyTemplateis true correctly reflects that variable files are only meaningful during template processing.
519-557: LGTM!Consistent implementation with
getFile, using the sameQueryParamsBuilderpattern and conditional template parameter handling.
591-607: LGTM!Clean refactoring that leverages
QueryParamsBuilder.addInt()for the revision parameter and properly iterates over merge sources to add path parameters.
669-688: LGTM!Clean refactoring to
QueryParamsBuilderwhile preserving the conditional logic formaxCommits.
718-735: LGTM!Consistent refactoring to
QueryParamsBuilder, properly integrating with the updatedappendJsonPathshelper.
755-786: LGTM!Consistent
QueryParamsBuilderrefactoring for thePathPatternvariant ofgetDiff.
909-962: LGTM!Well-implemented template support for file watching. The conditional handling of
variableFileandvariableRevision(only whenapplyTemplateis true) is correct. The empty query params check (lines 950-955) properly avoids appending an unnecessary?to the path.
1056-1063: LGTM!Clean refactoring of the helper method to work with
QueryParamsBuilder, following the in-place mutation pattern consistent with builder APIs.
1116-1128: LGTM!Good implementation with defensive
isInt()check before parsing thevariableRevisionfield. The separation betweentoEntry(handling variable revision) andtoEntry0(core entry parsing) keeps the logic clean.dependencies.toml (1)
25-25: FreeMarker 2.3.34 is the current stable version with no known security advisories.The version specified is the latest stable release (released Dec 22, 2024) and has no CVEs in the core library. While template injection is a general concern when implementing template rendering, it is not a vulnerability in FreeMarker 2.3.34 itself but rather a responsibility of the consuming code. This version choice is sound.
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryTest.java (1)
226-230: LGTM!The test correctly passes
nullfor the newvariableRevisionparameter inEntry.of()calls. This aligns with the updated Entry factory API and is appropriate since these tests verify basic repository operations without template processing.server/src/main/java/com/linecorp/centraldogma/server/storage/repository/EntryTransformer.java (1)
31-45: LGTM!Clean functional interface design. The use of
UnmodifiableFuture::completedFuturefor the identity transformer is an efficient choice as it returns a pre-completed immutable future.common/src/main/java/com/linecorp/centraldogma/common/TemplateProcessingException.java (1)
22-39: LGTM!Well-structured exception class following standard Java exception patterns. Both constructors (message-only and message+cause) cover the common exception chaining use cases.
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryUtil.java (1)
137-139: LGTM!Correctly propagates
variableRevisionthrough JSON_PATH query application. This ensures that template-rendered entries maintain their variable revision metadata after query transformation, which is essential for the watch API to properly track changes.server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (2)
192-208: Mutation of ImmutableMap will throw UnsupportedOperationException.On line 195,
variables.remove("revision")is called on the map returned bymergeVariables(). However, line 263-264 wraps the result in aHashMapwith onlyvarsandrevisionkeys, so this should work. But looking more closely,variableshere is the outer map (the HashMap), not the inner ImmutableMap—this is fine.However, the cast
(int) variables.remove("revision")will throwClassCastExceptionif no variables were found (returnsInteger.MAX_VALUEwhich is fine), but the issue is that ifvariablesdoesn't contain "revision" key,remove()returnsnulland unboxing will throw NPE.Actually, reviewing lines 263-266, the "revision" key is always added, so this is safe. No issue here.
70-81: FreeMarker security configuration looks good.The configuration properly disables potentially dangerous features:
setNewBuiltinClassResolver(TemplateClassResolver.ALLOWS_NOTHING_RESOLVER)prevents?newfrom instantiating arbitrary classessetAPIBuiltinEnabled(false)disables the?apibuilt-inThis mitigates Server-Side Template Injection (SSTI) risks.
client/java/src/main/java/com/linecorp/centraldogma/client/Latest.java (2)
36-56: Well-structured addition of variableRevision field.The implementation maintains backward compatibility by having the original constructor delegate to the new one. The nullable annotation and proper initialization are correct.
84-101: Equality and hashCode correctly updated.The
equalsmethod properly usesObjects.equalsfor nullablevariableRevisioncomparison, andhashCodeincludes it viaObjects.hash.client/java/src/main/java/com/linecorp/centraldogma/client/WatcherRequest.java (2)
158-185: Template API methods are well-designed with proper validation.The two
applyTemplateoverloads have clear semantics:
applyTemplate(boolean)toggles template processing and clearsvariableFileapplyTemplate(String)enables templating with a specific variable file and validates the pathThe state management correctly clears
variableFilewhen using the boolean overload.
219-226: Good guard against invalid configuration.The
checkState(!applyTemplate, ...)correctly prevents users from enabling template processing when watching multiple files viaPathPattern, which is documented in the Javadoc as unsupported.common/src/main/java/com/linecorp/centraldogma/common/Entry.java (3)
133-151: New factory overload correctly adds variableRevision support.The new
Entry.of(Revision, String, EntryType, T, Revision)overload follows the existing pattern and the original method delegates to it withnullfor backward compatibility.
217-225: withVariableRevision preserves rawContent correctly.The
withVariableRevisionmethod creates a new Entry while preserving all existing fields includingrawContent, which is important for maintaining template content fidelity.
304-326: Equality and hash code properly incorporate variableRevision.Both
hashCodeandequalsnow includevariableRevision, ensuring that entries with different variable revisions are considered distinct. The use ofObjects.equalscorrectly handles nullable comparison.common/src/main/java/com/linecorp/centraldogma/internal/api/v1/EntryDto.java (1)
38-39: DTO correctly mirrors the domain model changes.The
variableRevisionfield is properly added with:
@Nullableannotation- Constructor parameter in the right position
@JsonPropertygetter for serialization- Class-level
@JsonInclude(Include.NON_NULL)ensures it's omitted from JSON when nullAlso applies to: 59-68, 76-80
common/src/test/java/com/linecorp/centraldogma/common/EntryTest.java (1)
131-148: LGTM!The test calls have been correctly updated to use the new 5-parameter
Entry.of()signature, passingnullfor thevariableRevisionparameter. This maintains test coverage for the existing null-checks and type validation logic.client/java/src/main/java/com/linecorp/centraldogma/client/AbstractFileRequest.java (1)
72-77: LGTM!The
applyTemplate(String variableFile)method correctly:
- Validates the path using
validateStructuredFilePath- Sets
applyTemplate = trueimplicitly when a variable file is specified- Stores the variable file path
The path validation ensures only structured file paths are accepted.
client/java/src/main/java/com/linecorp/centraldogma/client/FileWatcher.java (2)
43-65: LGTM!The template processing fields and constructor are correctly implemented:
- Fields
applyTemplateandvariableFileare properly declared with appropriate nullability- Constructor correctly initializes all new fields
- Parameters are threaded through from the caller (WatcherRequest)
68-87: LGTM!The
doWatchmethod is correctly updated to:
- Accept the new
variableRevisionparameter (nullable)- Pass all template-related parameters (
applyTemplate,variableFile,variableRevision) tocentralDogma.watchFile()- Propagate
entry.variableRevision()through theLatestobject in both mapper and non-mapper code pathsThis ensures that variable revision tracking works correctly through the watch flow.
client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java (2)
198-220: LGTM!The
getFilemethod overloads are correctly updated:
- The default method properly delegates with default values (
false, false, null)- The abstract method signature is extended with
applyTemplateandvariableFileparameters- Parameter ordering is logical:
viewRaw→applyTemplate→variableFile
246-268: LGTM!The
getFilesmethod overloads follow the same pattern asgetFile, consistently extending the interface with template processing support.server/src/main/java/com/linecorp/centraldogma/server/internal/api/HttpApiExceptionHandler.java (1)
139-140: LGTM!The exception handling for
TemplateProcessingExceptionfollows the established pattern and correctly maps toHttpStatus.INTERNAL_SERVER_ERROR, which is appropriate for server-side template rendering failures.server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryWrapper.java (1)
218-225: LGTM!The wrapper correctly delegates the extended
watchmethod with template-related parameters (variableFile,variableRevision, andtransformerFactory) to the underlying repository implementation, maintaining the delegation pattern used throughout this class.client/java/src/main/java/com/linecorp/centraldogma/client/FilesWatcher.java (1)
64-65: LGTM!The unused
variableRevisionparameter is appropriately documented. SinceFilesWatchermonitors repository revisions rather than rendered template content, this parameter is correctly not used here while satisfying the abstract method contract.client/java/src/main/java/com/linecorp/centraldogma/client/FileRequest.java (1)
60-64: Template processing parameters correctly propagated to underlying API.The
viewRaw(),applyTemplate(), andvariableFile()parameters are properly inherited fromAbstractFileRequestand correctly passed toCentralDogma.getFile()with matching method signatures.server/build.gradle (1)
33-34: LGTM. FreeMarker 2.3.34 is the current stable version with no known vulnerabilities in the library itself. The dependency is appropriately pinned independencies.toml.server/src/main/java/com/linecorp/centraldogma/server/internal/thrift/CentralDogmaServiceImpl.java (2)
67-67: LGTM!Import added for the new
TemplateParamsclass to support the template parameter plumbing.
424-425: LGTM!The Thrift API correctly uses
TemplateParams.disabled()to disable template processing for legacy Thrift clients, maintaining backward compatibility. ThenullforvariableRevisionis appropriate since template rendering is not supported in this legacy path.server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java (1)
39-49: LGTM on the conversion logic.The implementation correctly:
- Uses
Boolean.parseBooleanwhich safely returnsfalsefor null/missing values- Handles nullable
variableFileappropriately- Only creates a
RevisionwhenvariableRevStris non-nullInvalid revision strings will throw an
IllegalArgumentExceptionfrom theRevisionconstructor, which should be handled by the framework's exception handling.server/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.java (2)
27-42: LGTM!Well-designed immutable value class with:
- Singleton pattern for the disabled state to avoid unnecessary allocations
- Clean factory method that enforces the disabled singleton when
applyTemplateis false
78-102: LGTM!Standard
equals,hashCode, andtoStringimplementations following best practices.server/src/main/java/com/linecorp/centraldogma/server/internal/api/DtoConverter.java (2)
80-107: LGTM!The
variableRevisionis consistently propagated through all code paths:
- Raw content path (line 87)
- YAML with JSON_PATH path (line 93)
- YAML for backward compatibility path (line 100)
- Generic content path (line 104)
- Non-content path correctly uses
null(line 111)This ensures template variable tracking is preserved in the DTOs.
114-125: LGTM!Method signature correctly extended to accept the nullable
variableRevisionparameter, and it's properly passed to theEntryDtoconstructor.client/java/src/main/java/com/linecorp/centraldogma/client/AbstractWatcher.java (2)
293-296: LGTM!The watch loop correctly extracts
variableRevisionfrom the currentlatestvalue (ornullfor initial state) and passes it todoWatch. This enables the server to detect changes in variable files used for template rendering.
364-365: LGTM!The abstract method signature is correctly updated to include the nullable
variableRevisionparameter. All subclasses (FileWatcherandFilesWatcher) have been properly updated with the new signature.server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java (1)
998-998: LGTM!The addition of
pm(ProjectManager) to theContentServiceV1constructor is correct. This enables template rendering support by providing access to project/repository resources needed by the Templater component.client/java/src/test/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogmaTest.java (4)
150-162: LGTM!The mock stubs and verifications are correctly updated to accommodate the extended
getFilesignature with template-related parameters (false, false, null).
348-358: LGTM!The test correctly invokes
getFilewith the new template parameters and verifies the delegate call with matching arguments.
369-384: LGTM!The
watchFiletest correctly handles the extended signature with template parameters (false, false, null, null).
392-403: LGTM!The final
getFiletest segment is correctly updated to match the extended API signature.server/src/main/java/com/linecorp/centraldogma/server/internal/api/WatchService.java (1)
29-31: LGTM!New imports are correctly added to support the template functionality.
client/java/src/main/java/com/linecorp/centraldogma/client/WatchRequest.java (3)
41-43: LGTM!The new fields for template support are properly declared with appropriate nullability annotation.
83-111: LGTM!The two
applyTemplateoverloads provide a clean fluent API:
applyTemplate(boolean)for simple enable/disable with variableFile resetapplyTemplate(String)for specifying a custom variable file with path validationThe documentation clearly explains the interaction with
viewRaw.
137-144: LGTM!The
startmethod correctly passes the template parameters to the underlyingwatchFilecall.server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryWatcher.java (5)
1-15: LGTM!The copyright header and license are correctly specified for a new file.
55-76: LGTM!The constructor correctly initializes the watcher with proper conditional logic:
- When
transformerFactoryis null, uses simple path pattern and skips dogma repo setup- When
transformerFactoryis provided, constructs extended path pattern including variable files and sets up dogma repo reference
78-102: LGTM!The initial
watch()method correctly:
- Defaults to
Revision.HEADfor variable revision on first watch- Applies the transformer factory appropriately
- Uses
getOrNullto fetch the initial entry state before starting the watch loop
104-138: Consider adding explicit null-check documentation for content comparison.At line 127,
Objects.equals(oldResult.content(), newResult.content())is safe becauseoldResultis checked for null, butnewResult.content()could potentially be null for directory entries. SinceObjects.equalshandles nulls correctly, this works, but a brief comment could clarify intent.The re-watch logic at lines 129-132 correctly handles the case where content hasn't changed by initiating another watch cycle.
179-188: LGTM!Simple and appropriate holder class for the revision pair.
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java (5)
113-121: LGTM! Constructor correctly initializes templater with required dependencies.The
Templateris properly instantiated withCommandExecutorandProjectManager, which aligns with theTemplaterconstructor signature shown in the relevant snippets.
141-169: LGTM! Template-aware findFiles implementation.The method correctly:
- Creates a transformer based on
TemplateParams- Applies it to the
repository.find()call- Preserves
templateParamsin recursive directory expansion (lines 159-160)
296-308: LGTM! Template parameters correctly propagated through all code paths.Both the single-file query path (line 299) and the multi-file find path (line 306) properly apply the transformer based on
TemplateParams.
310-327: LGTM! Watch path properly handles variable revision updates.The transformer factory lambda correctly creates a new templater with the updated
variableRevisionfor each watch cycle, enabling the watch API to detect changes in both the file content and the variables.
329-338: LGTM! Clean transformer factory implementation.The method correctly returns an identity transformer when templating is disabled, avoiding unnecessary processing overhead.
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java (3)
169-198: LGTM! Clean delegation pattern for get methods.Both overloads properly delegate to their respective
getOrNullvariants and consistently throwEntryNotFoundExceptionwhen the result is null.
235-261: LGTM! Null-safe transformer-aware query execution.The implementation correctly:
- Short-circuits on null entries (line 244)
- Applies transformation before query execution as documented (lines 247, 254)
- Preserves
CentralDogmaExceptionwhile wrapping other exceptions inQueryExecutionException(lines 255-259)
582-596: LGTM! Backward-compatible extension for template-aware watching.The default
watchmethod maintains backward compatibility by passing nulls for the new parameters, while the new overload enables template-aware watching with dynamic transformer creation based on variable revision.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/VariableServiceV1.java (1)
240-248: Typo:vale()should bevalue().This will cause a compilation error. The method name
vale()does not exist on theVariableclass.🐛 Proposed fix
private static void validateVariable(Variable variable) { if (variable.type() == VariableType.JSON) { try { - Jackson.readTree(variable.vale()); + Jackson.readTree(variable.value()); } catch (JsonParseException e) { throw new IllegalArgumentException("Invalid JSON value for variable: " + variable.id(), e); } } }
🤖 Fix all issues with AI agents
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java`:
- Around line 147-158: Currently the thenApply block that handles
repo.getOrNull(revision, variableFile) silently returns ImmutableMap.of() when
entry0 is null; instead, when a user explicitly provided a variableFile (i.e.,
variableFile is non-null/non-empty or flagged as user-specified), throw a
TemplateProcessingException indicating the variable file was not found (use
TemplateProcessingException and include variableFile in the message) rather than
returning an empty map; keep the existing behavior (return ImmutableMap.of())
only for the implicit/default-no-variable-file case and continue to validate
type and call parseVariableFile(entry0) for non-null entries.
🧹 Nitpick comments (4)
server/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java (1)
1245-1270: Consider using Awaitility for the negative assertion.The
Thread.sleep(1000)at line 1262 followed byassertThat(future).isNotDone()is a timing-based negative assertion that could be flaky under load. While this pattern is sometimes necessary for "should NOT happen" scenarios, consider if Awaitility'sduring()method could make this more robust:await().during(Duration.ofSeconds(1)).untilAsserted(() -> { assertThat(future).isNotDone(); });However, I understand this is a test for template watch behavior where proving the negative (no notification) requires a reasonable wait time.
server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (3)
196-212: Mutating the input map and implicit contract forrevisionkey.Line 199 modifies the
variablesmap by removing therevisionkey. This creates a hidden contract betweenmergeVariablesandprocess. Consider passing the revision separately or using a wrapper object to make this explicit.Additionally, if the
revisionkey is ever missing or not anInteger, this will throw an NPE orClassCastException.♻️ Suggested approach
- private <T> Entry<T> process(Entry<T> entry, Map<String, Object> variables) { + private <T> Entry<T> process(Entry<T> entry, Map<String, Object> variables, int variableRevision) { final StringWriter out = new StringWriter(); final Template template = cache.get(entry); - final int revision = (int) variables.remove("revision"); try { template.process(variables, out); //noinspection unchecked Entry<T> newEntry = (Entry<T>) newEntryWithContent(entry, out.toString()); - if (revision != Integer.MAX_VALUE) { - newEntry = newEntry.withVariableRevision(new Revision(revision)); + if (variableRevision != Integer.MAX_VALUE) { + newEntry = newEntry.withVariableRevision(new Revision(variableRevision)); } return newEntry;
214-235: Unused type parameter and error handling concerns.
- Line 214: The type parameter
<T>is unused in this method signature.- Lines 222 and 228: Wrapping parsing exceptions in
IllegalStateExceptionloses the context that template rendering produced invalid output. ConsiderTemplateProcessingExceptionfor consistency.- Line 233:
throw new Error()is unconventional;AssertionErroris more idiomatic for "should never happen" cases.♻️ Suggested fix
- private static <T> Entry<?> newEntryWithContent(Entry<?> entry, String content) { + private static Entry<?> newEntryWithContent(Entry<?> entry, String content) { switch (entry.type()) { case TEXT: return Entry.ofText(entry.revision(), entry.path(), content); case JSON: try { return Entry.ofJson(entry.revision(), entry.path(), content); } catch (JsonProcessingException e) { - throw new IllegalStateException(e); + throw new TemplateProcessingException( + "Template rendered invalid JSON for " + entry.path(), e); } case YAML: try { return Entry.ofYaml(entry.revision(), entry.path(), content); } catch (JsonParseException e) { - throw new IllegalStateException(e); + throw new TemplateProcessingException( + "Template rendered invalid YAML for " + entry.path(), e); } case DIRECTORY: default: // Should not reach here. - throw new Error(); + throw new AssertionError("Unexpected entry type: " + entry.type()); } }
275-290: UseAssertionErrorinstead ofErrorfor unreachable code.Same issue as in
newEntryWithContent-throw new Error()at line 288 should bethrow new AssertionError("Unexpected variable type: " + variable.type())for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
NOTICE.txtclient/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.javagradle.propertieslicenses/LICENSE.freemarker.al20.txtserver/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/VariableServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.javaserver/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java
✅ Files skipped from review due to trivial changes (1)
- NOTICE.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java
🧰 Additional context used
🧬 Code graph analysis (4)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (4)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-338)common/src/main/java/com/linecorp/centraldogma/common/TemplateProcessingException.java (1)
TemplateProcessingException(22-39)common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(70-416)server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/DefaultCrudOperation.java (1)
DefaultCrudOperation(49-139)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java (4)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java (1)
TemplateParamsConverter(33-50)server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/WatchRequestConverter.java (2)
WatchRequestConverter(44-188)WatchRequest(157-187)server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (1)
Templater(61-291)server/src/main/java/com/linecorp/centraldogma/server/internal/api/TemplateParams.java (1)
TemplateParams(27-103)
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java (1)
common/src/main/java/com/linecorp/centraldogma/common/Entry.java (1)
Entry(41-338)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/VariableServiceV1.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(70-416)
🪛 LanguageTool
licenses/LICENSE.freemarker.al20.txt
[style] ~162-~162: ‘any and all’ might be wordy. Consider a shorter alternative.
Context: ...ge, computer failure or malfunction, or any and all other commercial damages or losse...
(EN_WORDINESS_PREMIUM_ANY_AND_ALL)
⏰ 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). (4)
- 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
🔇 Additional comments (30)
licenses/LICENSE.freemarker.al20.txt (1)
1-218: LGTM! Proper license compliance.The addition of the Apache License 2.0 for FreeMarker is appropriate given the PR's integration of FreeMarker for template rendering functionality.
client/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.java (5)
251-271: LGTM!The method correctly propagates the new template parameters (
applyTemplate,variableFile) to the delegate, and thetoString()representation now includes all parameters for complete debugging information.
273-293: LGTM!Consistent with
getFilechanges—template parameters are correctly propagated and included in thetoString()representation.
295-334: Clean syntax modernization.The diamond operator usage (
new Function<>(),new BiFunction<>()) is consistent with Java best practices and matches other methods in this file.
482-514: LGTM!The
watchFilemethod correctly propagates all three new template-related parameters (applyTemplate,variableFile,variableRevision) to the delegate. The retry logic appropriately continues to trackentry.revision()for replication lag handling, which is the content revision distinct fromvariableRevision.
526-544: Consistent syntax improvement.Diamond operator usage aligns with the rest of the file.
gradle.properties (1)
23-23: This exclusion is appropriate for a Spring ConfigurationProperties POJO.
CentralDogmaSettingsis a Spring@ConfigurationPropertiesclass used for externalized configuration binding. It contains only getters, setters, and a utilitytoString()method. Spring ConfigurationProperties classes are conventionally excluded from code coverage because they are Spring-managed POJOs with no business logic, and their methods are invoked through reflection during configuration binding rather than direct unit test instantiation. No action needed.server/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.java (12)
59-105: LGTM! Well-structured test setup.The test setup follows good practices:
- Clean state isolation via project removal/recreation in
@BeforeEach- Multiple repositories (
testRepo1,testRepo2) enable cross-repo behavior testing- Auth configuration with
TestAuthProviderFactoryis appropriate for integration testing
107-195: LGTM! Good coverage of basic template scenarios.These tests effectively validate:
- String and JSON variable substitution
- Repository-level variable scoping and isolation
- Variable precedence (repo overrides project)
- Cross-repo access prevention (lines 162-168)
253-310: LGTM! Comprehensive variable lifecycle testing.The tests thoroughly cover:
- Variable deletion with fallback to parent scope
- Dynamic variable override creation
- Variable update propagation
- Multi-variable template rendering
382-410: LGTM! Good FreeMarker directive coverage.This test validates FreeMarker's conditional (
<#if>) and iteration (<#list>) directives work correctly with the templating system. The test demonstrates both boolean conditionals and array iteration.
466-477: LGTM! Proper error handling validation.The test correctly validates that undefined variables result in
TemplateProcessingException, which is the expected behavior for template rendering failures.
633-702: LGTM! Thorough variable file priority testing.The priority tests effectively validate the documented precedence:
.variables.json>.variables.json5>.variables.yaml>.variables.yml- Same-directory files take precedence over root-level files
This ensures the variable resolution order is correct.
946-983: LGTM! Practical multi-environment testing.This test demonstrates a realistic use case where different variable files (
/vars/dev.json,/vars/prod.json) are used to render the same template for different environments. Both parsed JSON and raw YAML output assertions validate the feature works correctly.
1227-1243: LGTM! Important constraint validation.This test correctly validates that
JSON_PATHqueries cannot be used withviewRaw(true), throwingIllegalStateException. This is an important API constraint that users need to be aware of.
1527-1558: LGTM! Important behavior validation.This test verifies a critical optimization: watchers are only notified when variables actually used in the template change. Updating
unusedVarshould not trigger a notification, while updatingusedVarshould. This is important for performance in systems with many variables.
1627-1651: LGTM! Well-structured long-running watcher test.Good practices demonstrated:
- Uses try-with-resources for proper
Watchercleanup- Uses
await().untilAsserted()for async assertions (from Awaitility)- Tests multiple sequential updates to ensure consistent behavior
1780-1826: LGTM! Clean helper methods with proper response validation.The helper methods correctly:
- Use
@Nullableannotation onrepoNameparameters- Assert HTTP response status codes (
CREATED,OK,NO_CONTENT)- Handle both project-level and repo-level variable paths
Previous review feedback has been properly addressed.
1-58: Excellent comprehensive test coverage.This test class provides thorough end-to-end validation of the Variable Template feature:
- Variable scoping (project, repo, file levels)
- Variable precedence and override behavior
- Multiple file formats (JSON, JSON5, YAML, YML)
- FreeMarker directive support
- JSON path queries with templates
- Watch/notification functionality
- Error handling for missing variables
The test structure follows good practices with clear method names, appropriate assertions, and proper resource cleanup.
server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java (2)
71-97: FreeMarker configuration looks secure.Good security posture with
TemplateClassResolver.ALLOWS_NOTHING_RESOLVERto prevent class instantiation andsetAPIBuiltinEnabled(false)to disable API built-ins. Cache settings are reasonable.
84-96: Template parsing errors are properly caught downstream.The cache loader can throw when FreeMarker fails to parse a template, but the
processmethod's catch block at line 208 handles this appropriately by wrapping inTemplateProcessingException.server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java (5)
115-121: Constructor change looks good.The
Templateris properly initialized with theCommandExecutorandProjectManagerdependencies.
141-169: Template parameter propagation looks correct.The
TemplateParamsare properly threaded through the recursivefindFilescalls and the transformer is applied viarepository.find().
310-327: Watch file templating integration is well designed.The transformer factory pattern at line 316 allows the watch mechanism to re-render templates with updated variable revisions when changes occur.
329-336: Clean helper method for creating templater.The conditional logic properly returns an identity transformer when templating is disabled, avoiding unnecessary processing.
269-308: Template parameters correctly integrated into all code paths.The
TemplateParamsare properly propagated through watch, single file get, and multi-file find operations.server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java (4)
171-198: Transformer-enabled get methods are well structured.The delegation pattern from
get()togetOrNull()with identity transformer, and the new transformer-aware overloads provide clean API extensibility. The documentation clearly states that the transformer is applied first, then the query.
235-261: Transformer composition implementation is correct.The two-stage async pipeline properly:
- Fetches and transforms the entry (lines 241-248)
- Applies the query to the transformed result (lines 249-260)
Null propagation and exception handling are appropriate.
285-306: Transformer-aware find implementation is correct.Good optimization to short-circuit when identity transformer is used. The parallel transformation of entries using
CompletableFutures.allAsListis appropriate for this use case.
587-601: Watch API extension for templating is backward compatible.The original
watch(Revision, Query)overload (line 587-589) delegates to the new extended version with null parameters, preserving backward compatibility. ThetransformerFactoryparameter enables dynamic transformer creation based on variable revision changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
The current implementation select the highest-priority variable file and merges it with the variables stored in the Dogma repository. Upon further consideration, I propose merging all variable files, where values from higher-priority files override those from lower-priority ones. |
| * <p>{@code null} if this {@link Entry} was not created by applying variables to a template. | ||
| */ | ||
| @Nullable | ||
| public Revision variableRevision() { |
There was a problem hiding this comment.
Naming) If secrets are also implemented, I understood that this will also use "variable"Revision as the representative revision.
There was a problem hiding this comment.
Considering secrets, a more neutral name like templateRevision might be a better choice.
| // Should not reach here as the value has been validated before storing. | ||
| throw new IllegalStateException(e); | ||
| } | ||
| case STRING: |
There was a problem hiding this comment.
Note) Understood that the STRING type is kept going forward
There was a problem hiding this comment.
I kept STRING type because it is more convenient when users enter values via an input box in the UI.
| } | ||
|
|
||
| if (newResult == null || | ||
| oldResult != null && Objects.equals(oldResult.content(), newResult.content())) { |
There was a problem hiding this comment.
Note) Understood that watch terminates if the template-applied contents differ
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java`:
- Around line 671-680: The query param builder is receiving an already-encoded
path pattern which causes double-encoding (e.g. %20 -> %2520); change uses of
pathPattern.encoded() passed into QueryParams.builder().add(...) (e.g. the
add("path", ...) and add("pathPattern", ...) calls in ArmeriaCentralDogma) to
pass the raw pattern string via pathPattern.patternString() so
QueryParamsBuilder can perform proper URL-encoding when build() is called;
update both occurrences mentioned (the QueryParamsBuilder around the to/path
handling and the separate QueryParams.builder() that adds "pathPattern") to use
patternString() instead of encoded().
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/variable/Templater.java`:
- Around line 202-213: The templateRevision is currently computed as the minimum
(oldest) variable revision which breaks watch logic; update the logic inside
process(Entry<T> entry, Map<String,Object> variables) and the similar block at
lines ~239-279 to compute the latest revision (or use the requested template
revision when present) instead of Math.min, then set templateRevision via
newEntry.withTemplateRevision(new Revision(latestRevisionOrRequested)) and
adjust any sentinel constants/initial values (e.g., replace Integer.MAX_VALUE
use-case) so they represent “no revision yet” correctly; locate and change
usages in methods process and newEntryWithContent/withTemplateRevision to ensure
the stored revision reflects the newest variable change.
- Around line 141-149: The code currently uses chooseVariableFile to pick a
single Entry in findVariableFile; instead, iterate over all matching entries,
parse each via parseVariableFile, sort them by the required precedence (.json >
.json5 > .yaml > .yml) and merge their maps so that higher‑precedence files
override lower‑precedence keys; remove or stop using chooseVariableFile and
apply the same merge logic to the other similar block referenced (lines 166–184)
so all applicable .variables.* files at a level are merged rather than a single
file chosen.
♻️ Duplicate comments (4)
client/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.java (3)
296-302: Anonymous-class diamond here is covered by the Java-version check above.
316-323: Anonymous-class diamond here is covered by the Java-version check above.
526-533: Anonymous-class diamond here is covered by the Java-version check above.server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/TemplateParamsConverter.java (1)
30-32: Incorrect Javadoc: referencesQueryinstead ofTemplateParams.The class-level Javadoc still mentions
Querybut this converter producesTemplateParams.📝 Suggested fix
-/** - * A request converter that converts to {`@link` Query} when the request has a valid file path. - */ +/** + * A request converter that extracts template parameters from the request context. + */
🧹 Nitpick comments (6)
client/java-armeria-legacy/src/main/java/com/linecorp/centraldogma/client/armeria/legacy/LegacyCentralDogma.java (3)
225-234: Consider validating unsupported parameters and fix grammar.Two observations:
Grammar: Line 230 uses "is not supported" but should be "are not supported" since it references two features (viewRaw and applyTemplate).
Silent failure: Unlike
errorOnEntryNotFoundwhich throws viacheckArgument()when used (see line 470), passingapplyTemplate=trueorviewRaw=trueis silently ignored. Callers might expect template processing but receive raw content instead.Suggested fix
public <T> CompletableFuture<Entry<T>> getFile(String projectName, String repositoryName, Revision revision, Query<T> query, boolean viewRaw, boolean applyTemplate, `@Nullable` String variableFile) { requireNonNull(query, "query"); + checkArgument(!viewRaw, "viewRaw is not supported in LegacyCentralDogma."); + checkArgument(!applyTemplate, "applyTemplate is not supported in LegacyCentralDogma."); return maybeNormalizeRevision(projectName, repositoryName, revision).thenCompose(normRev -> { final CompletableFuture<GetFileResult> future = run(callback -> { - // viewRaw and applyTemplate is not supported in LegacyCentralDogma. client.getFile(projectName, repositoryName, RevisionConverter.TO_DATA.convert(normRev), QueryConverter.TO_DATA.convert(query), callback); });
283-298: Same validation concern asgetFile.For consistency with the suggested fix in
getFile, consider adding validation to throw whenapplyTemplate=trueorviewRaw=trueis passed.Suggested fix
public CompletableFuture<Map<String, Entry<?>>> getFiles(String projectName, String repositoryName, Revision revision, PathPattern pathPattern, boolean viewRaw, boolean applyTemplate, `@Nullable` String variableFile) { requireNonNull(pathPattern, "pathPattern"); + checkArgument(!viewRaw, "viewRaw is not supported in LegacyCentralDogma."); + checkArgument(!applyTemplate, "applyTemplate is not supported in LegacyCentralDogma."); return maybeNormalizeRevision(projectName, repositoryName, revision).thenCompose(normRev -> { final CompletableFuture<List<com.linecorp.centraldogma.internal.thrift.Entry>> future = run(callback -> { - // viewRaw and applyTemplate are not supported in LegacyCentralDogma. client.getFiles(projectName, repositoryName, RevisionConverter.TO_DATA.convert(normRev), pathPattern.patternString(), callback); });
489-506: Same validation concern applies here.For consistency, consider adding validation for
viewRaw,applyTemplate, and potentiallytemplateRevisionwhen non-null.Suggested fix
public <T> CompletableFuture<Entry<T>> watchFile(String projectName, String repositoryName, Revision lastKnownRevision, Query<T> query, long timeoutMillis, boolean errorOnEntryNotFound, boolean viewRaw, boolean applyTemplate, `@Nullable` String variableFile, `@Nullable` Revision templateRevision) { checkArgument(!errorOnEntryNotFound, "errorOnEntryNotFound is not supported in LegacyCentralDogma."); + checkArgument(!viewRaw, "viewRaw is not supported in LegacyCentralDogma."); + checkArgument(!applyTemplate, "applyTemplate is not supported in LegacyCentralDogma."); validateProjectAndRepositoryName(projectName, repositoryName); requireNonNull(lastKnownRevision, "lastKnownRevision"); requireNonNull(query, "query"); final CompletableFuture<WatchFileResult> future = run(callback -> { - // viewRaw and applyTemplate are not supported in LegacyCentralDogma. client.watchFile(projectName, repositoryName,server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryWatcher.java (1)
154-167: Potential NPE:dogmaRepomay be dereferenced without null check.At line 156,
dogmaRepo.watch(...)is called whenlastKnownTempRev != null. While the constructor logic ensuresdogmaRepois only set whentransformerFactory != null, the null safety relies on an implicit invariant: iflastKnownTempRevis non-null, thendogmaRepomust also be non-null.This invariant holds because:
- Constructor sets
dogmaRepoonly whentransformerFactory != nulllastKnownTemplateRevis only set in that same branchlastKnownTempRevinwatchReposcomes fromoldVariableRev, which derives from eitherlastKnownTemplateRevoroldResult.templateRevision()(which requires templating)However, the coupling is subtle and could break during future refactoring. Consider adding an explicit null check or assertion for clarity.
💡 Optional: Add defensive null check
final CompletableFuture<Revision> dogmaFuture; if (lastKnownTempRev != null) { + assert dogmaRepo != null : "dogmaRepo must be set when lastKnownTempRev is non-null"; dogmaFuture = dogmaRepo.watch(lastKnownTempRev, "/**/variables/**/*.json");client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java (1)
800-828: Document the new template-related parameters.The Javadoc for this overload is copied from the simpler version and doesn't describe the new parameters (
viewRaw,applyTemplate,variableFile,templateRevision). Consider adding@paramtags or updating the description to explain their purpose.common/src/test/java/com/linecorp/centraldogma/common/EntryTest.java (1)
163-171: Consider renaming test method to align with the field name.The commit message indicates
variableRevisionwas renamed totemplateRevision. The test method nametestVariableVersionuses the old terminology. Consider renaming totestTemplateRevisionfor consistency.Suggested rename
- void testVariableVersion() { + void testTemplateRevision() {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@it/server/src/test/java/com/linecorp/centraldogma/it/FileHistoryAndDiffTest.java`:
- Around line 76-81: The assertion in FileHistoryAndDiffTest is missing commit
"0" from the combined history; update the expectation for the summaries
(produced from commits via extractSummaryAsInt) to include 0 so it asserts all
10 commits are present (e.g. change the containsExactly call on summaries in the
test to include 0 at the end), locating the code around the commits =
client.forRepo(...).history(...).get(...) block and the subsequent
summaries/assertThat(summaries) line.
2 participants
Motivation:
As a follow-up to #1243, this PR introduces a templating feature to generate a configuration with variables.
Modifications:
Templater..variables.json.variables.json5.variables.yaml.variables.yml.variables.xxxfiles at the same level, they take precedence in the order listed above.vars.prefix when accessing them.EntryTransformerto render template files with variables.Repository.ContentServiceV1usesEntryTransformerto wrapTemplaterand render templates.applyTemplate,variableFileandvariableRevisionparameters to thegetFilesAPI to indicate whether and how to render a template file.Entry.variableRevisionwas added to propagate the variable version used when rendering a template.applyTemplate(boolean)andapplyTemplate(variableFile)to the fluent APIs.ArmeriaCentralDogmato useQueryParamsinstead of raw query encoding.variableRevisionalong withlastKnownRevisionto detect new changes triggered by variable updates.Result:
Template rendering with variables is now supported.