centraldogma icon indicating copy to clipboard operation
centraldogma copied to clipboard

Provide an easy way to import a directory into Central Dogma programmatically

Open hyunw9 opened this issue 7 months ago • 2 comments

Motivation

  • Related Issus : #963
  • Previously, the user had to call each client API to import files ( createProject(), createRepository(), commit(), push())
  • With this feature, users can import files or create projects more easily

Modification

Interface Addition

  • Added importDir(Path), importResourceDir(String), and importResourceDir(String, ClassLoader) methods to the CentralDogma interface.

  • Delegation in AbstractCentralDogma

    • Parses the given Path or String to determine the target <project>/<repository> and delegates the actual import logic to the corresponding CentralDogmaRepository. https://github.com/hyunw9/centraldogma/blob/48a97cdb816cfe6499293b677c580750b8ca762d/client/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogma.java#L225-L233
  • Core Implementation in CentralDogmaRepository

    • Implements actual file resolution, and change computation.
    • Automatically creates the project/repository if they do not exist, using createProject() and createRepository() .
  • Business Logic (change building)

    • Local file changes are converted via toChange(), with placeholder files ignored.
    • For importResourceDir(), resource paths are resolved using ClassLoader.getResource() with appropriate fallbacks.
    • Exclude predefined placeholder files (e.g. .keep) during import
  • Testing

    • Added unit tests for CentralDogmaRepository to verify import logic with mock CentralDogma.
    • Added integration tests (ArmeriaCentralDogmaTest) to confirm actual repository creation, content import, and correct diff results.
  • Design Note

    • Considered using join() vs thenCompose() when handling project/repo creation.
detail

There were cases where the commit() operation was attempted before the project or repository had been fully created, resulting in ProjectNotFoundException or RepositoryNotFoundException.

Initially, we handled this by chaining createProject() and createRepository() with thenCompose() and fallback logic to tolerate already-existing resources.
Here is the original implementation:

private CompletableFuture<Void> ensureProjectAndRepoExist() {
    final Function<Throwable, Throwable> unwrap = (t) -> {
        if (t instanceof CompletionException) {
            return t.getCause();
        }
        return t;
    };

    return centralDogma.createProject(projectName()).handle((r, ex) -> {
        if (ex != null && !(unwrap.apply(ex) instanceof ProjectExistsException)) {
            throw new CompletionException(unwrap.apply(ex));
        }
        return null;
    }).thenCompose(unused ->
        centralDogma.createRepository(projectName(), repositoryName())
                    .handle((r, ex) -> {
                        if (ex != null && !(unwrap.apply(ex) instanceof RepositoryExistsException)) {
                            throw new CompletionException(unwrap.apply(ex));
                        }
                        return null;
                    })
    ).thenApply(unused -> null);
}

While this ensures safe creation of the project and repository, it introduces multiple handle() blocks and deep exception wrapping, which may degrade readability and traceability. For now, import can be canceled if there is repository already exist.

Result

Without manually creating projects or repositories, users can now import files from both local file systems and classpath resources into Central Dogma with a single method call.

Summary by CodeRabbit

  • New Features

    • Added the ability to import files and directories into repositories from filesystem paths and classpath resources, with support for custom class loaders.
    • Introduced an import result summary showing revision, timestamp, and import status.
  • Tests

    • Added comprehensive tests verifying import functionality, including handling of placeholder files and resource-based imports.

hyunw9 avatar May 10 '25 15:05 hyunw9

Walkthrough

Adds a directory-import feature: new API method importDir(...) on CentralDogma, implementation in AbstractCentralDogma that resolves/creates project and repository, repository-level importDir to traverse and upsert files (JSON → JSON upsert, others → text upsert), and new Armeria tests covering import scenarios.

Changes

Cohort / File(s) Summary of changes
Public API: Client interface
client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java
Declares new method CompletableFuture<PushResult> importDir(@NullableString projectName,@Nullable String repositoryName, Path dir, boolean createIfMissing) with Javadoc (signature appears twice in the file).
Client: Base implementation
client/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogma.java
Adds public CompletableFuture<PushResult> importDir(...): validates dir, resolves project/repo names (params or path), ensures project/repo exist (optionally creating them), then delegates to repository importDir. Adds private helpers: ensureProjectAndRepo, resolveProjectAndRepoNames, extractProjectAndRepoFromPath, extractRepoNameFromPath.
Client: Repository operations
client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogmaRepository.java
Adds public CompletableFuture<PushResult> importDir(Path dir) plus helpers collectImportFiles and toChange: recursively collects regular, non-dot files; treats .json as JSON upsert and other files as UTF-8 text upsert; commits changes or returns completed future with null when no files.
Tests: Armeria client
client/java-armeria/src/test/java/com/linecorp/centraldogma/client/armeria/CentralDogmaImportTest.java
New JUnit 5 test class exercising importDir: empty directories, placeholder files ignored, file import behavior, non-existent directory error, null repository handling (creation), and deriving project/repo from path components.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant API as CentralDogma (client API)
  participant Impl as AbstractCentralDogma
  participant Repo as CentralDogmaRepository
  participant Server as Central Dogma server

  User->>API: importDir(project?, repo?, dir, createIfMissing)
  API->>Impl: importDir(...)
  Impl->>Impl: validate dir (non-null, absolute, exists)
  Impl->>Impl: resolve project/repo (params or dir path)
  alt project or repo missing
    Impl->>Server: ensure project/repo (createIfMissing?)
    Server-->>Impl: exists or created / error
  end
  Impl->>Repo: importDir(dir)
  Repo->>Repo: collectImportFiles(dir) (skip dotfiles)
  Repo->>Repo: toChange(file) (JSON→ofJsonUpsert, else→ofTextUpsert)
  alt no changes
    Repo-->>Impl: completedFuture(null)
  else has changes
    Repo->>Server: commit(HEAD, summary, changes)
    Server-->>Repo: PushResult
    Repo-->>Impl: CompletableFuture<PushResult>
  end
  Impl-->>API: CompletableFuture<PushResult>
  API-->>User: future completes (result or error)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25–30 minutes

Potential review attention:

  • Correctness of project/repo name resolution logic and edge cases when deriving names from path.
  • Error handling and exception wrapping (especially CompletableFuture exceptional completion).
  • File traversal: correctness of dotfile filtering, path normalization, and JSON detection/parsing.
  • Duplicate method declaration in the interface file.

Poem

I nibble through folders, carrot-bright,
JSON leaves and text in moonlight.
I skip the dots, make nests anew,
Create projects when flags say true.
A gentle thump—your changes pushed! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a programmatic way to import directories into Central Dogma. It directly relates to the core functionality introduced across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3fca95a6ac86de086fa7e9c141374815a36d2370 and e295274885e6ea454e47d1804f1465659c0d0e65.

📒 Files selected for processing (1)
  • client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java (2 hunks)
🔇 Additional comments (3)
client/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.java (3)

21-21: LGTM!

The new imports support the importDir method signature correctly.

Also applies to: 29-29


450-451: Method signature looks good.

The parameter types and nullability annotations are appropriate. The CompletableFuture<PushResult> return type is consistent with other push operations in the interface.


425-451: The review comment is based on incorrect assumptions about past decisions.

The implementation in AbstractCentralDogma.java explicitly requires absolute paths (line 205: if (!dir.isAbsolute()) throw new IllegalArgumentException("dir must be an absolute path...")), and the Javadoc examples correctly reflect this with absolute paths like /home/user/myproject/myrepo. The test suite also uses only absolute paths.

Legitimate issue: The Javadoc lacks @throws documentation for the exceptions thrown by the implementation (IllegalArgumentException for invalid paths, insufficient path components, etc.; IllegalStateException when projects/repos don't exist and createIfMissing is false). However, this is a documentation gap, not a code inconsistency.

The claim of past review agreement for "relative-only paths with validation" does not align with the actual implementation or current Javadoc.

Likely an incorrect or invalid review comment.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar May 10 '25 15:05 coderabbitai[bot]

Thanks for the review! I've left some replies to your comments.

Once things are clarified and we're on the same page, I'll go ahead and apply the suggested changes ! .

hyunw9 avatar May 29 '25 13:05 hyunw9

@hyunw9 👍 👍 👍

minwoox avatar Nov 14 '25 13:11 minwoox