Conversation
chickenchickenlove
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
I would like to receive feedback on whether this PR is heading in the right direction, as it is currently a draft. After fix direction, I will write unit tests and Java docs based on the BaseContextPathsTest code. When you have time, please take a look 🙇♂️ |
|
I didn't see that a fluent style is good for nested context paths in terms of API design. Users may find it difficult to know the current depth and parent trees and children trees. How about taking a lambda expression as a second parameter to leverage Kotlin trailing lamdas? I imagined the following style API. We used Server
.builder()
.contextPath("/rest") {
contextPath("/catalog") {
service("/product", new GetProductService())
service("/products", new ProductsHandler())
}
}
.contextPath("/cart") {
contextPath("/foo") {
contextPath("/bar") {
service("/checkout", new CheckoutService())
}
}
}
.contextPath("/gql") {
service("/catalog", new GraphQLService())
}Related work: https://ktor.io/docs/server-routing.html#nested_routes |
|
@ikhoon nim, Thanks for your guidelines. |
|
@ikhoon nim, i change code style from fluent style to nested lambda expression. // Without virtual host
sb.baseContextPath("/api")
.contextPath("/context-path/a1", "/context-path/a2")
.nestedContext()
.contextPaths(Set.of("/b1", "/b2"), ctx1 -> ctx1 // ctx1 scope start (lambda function)
.annotatedService(new Object() { ... })
.service("/my-service", new HttpService() { ... })
.contextPaths(Set.of("/c1", "/c2"), ctx2 -> ctx2 // ctx2 scope start (lambda function)
.service("/my-service1", new HttpService() { ... })
.contextPaths(Set.of(...), ctx3 -> ctx3 // ctx3 scope start (lambda function)
.service("/...", new HttpService() { ... })
) // ctx3 scope end.
) // ctx2 scope end.
) // ctx1 scope end.
.contextPaths(Set.of("/b3", "/b4"), ctx11 -> ctx11
.service("/my-service", new HttpService() { ... });
// With virtual host.
sb.virtualHost("foo.com")
.contextPath("/virtual-foo")
.nestedContext()
.contextPaths(Set.of("/a1", "/a2"), ctx -> ctx
.service("/my-service1", new HttpService() { ... }));To get detail code, you can refer to this java code.
It may not be exactly the same as the API you imagined, but since we can represent the scope within a lambda function, I believe it will be more user-friendly than my initial commit. also, java cannot perfectly match to When you have time, please take another look. 🙇♂️ |
|
@ikhoon nim, gently ping. |
Is it possible to build nested context paths without using |
chickenchickenlove
force-pushed
the
2407428-GH-5758
branch
from
0d8adb5 to
fd182c8
Compare
|
@ikhoon nim, sure. I updated example codes and API test codes.. Style1It starts with sb.baseContextPath("/api")
.contextPath("/context-path/a1", "/context-path/a2")
.contextPaths(Set.of("/b1", "/b2"), ctx1 -> ctx1 // ctx1 scope start (lambda function)
.annotatedService(new Object() { ... })
.service("/my-service", new HttpService() { ... })
.contextPaths(Set.of("/c1", "/c2"), ctx2 -> ctx2 // ctx2 scope start (lambda function)
.service("/my-service1", new HttpService() { ... })
.contextPaths(Set.of(...), ctx3 -> ctx3 // ctx3 scope start (lambda function)
.service("/...", new HttpService() { ... })
) // ctx3 scope end.
) // ctx2 scope end.
) // ctx1 scope end.
.contextPaths(Set.of("/b3", "/b4"), ctx11 -> ctx11
.service("/my-service", new HttpService() { ... });Style2It starts with sb.baseContextPath("/api")
.contextPath(Set.of("/b1", "/b2"), ctx1 -> ctx1 // ctx1 scope start (lambda function)
.annotatedService(new Object() { ... })
.service("/my-service", new HttpService() { ... })
.contextPaths(Set.of("/c1", "/c2"), ctx2 -> ctx2 // ctx2 scope start (lambda function)
.service("/my-service1", new HttpService() { ... })
.contextPaths(Set.of(...), ctx3 -> ctx3 // ctx3 scope start (lambda function)
.service("/...", new HttpService() { ... })
) // ctx3 scope end.
) // ctx2 scope end.
) // ctx1 scope end.
.contextPaths(Set.of("/b3", "/b4"), ctx11 -> ctx11
.service("/my-service", new HttpService() { ... });When you have time, PTAL 🙇♂️ |
|
Could you explain the difference between |
chickenchickenlove
force-pushed
the
2407428-GH-5758
branch
from
8cb306d to
6a1b4ee
Compare
|
@ikhoon nim, sorry to late. For
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5846 +/- ##
============================================
- Coverage 74.46% 74.30% -0.16%
- Complexity 22234 23466 +1232
============================================
Files 1963 2105 +142
Lines 82437 87926 +5489
Branches 10764 11519 +755
============================================
+ Hits 61385 65335 +3950
- Misses 15918 17107 +1189
- Partials 5134 5484 +350 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@ikhoon nim |
| * @return | ||
| */ | ||
| @UnstableApi | ||
| public ContextPathServicesBuilder toContextBuilder() { |
There was a problem hiding this comment.
Question) Could we add a new API that directly builds services under a context path with a customizer instead of toContextBuilder()?
public ServerBuilder contextPath(Iterable<String> contextPaths,
Consumer<ContextPathServicesBuilder> customizer) {
requireNonNull(contextPaths, "contextPaths");
customizer.accept(contextPath(contextPaths));
return this;
}There was a problem hiding this comment.
It works. applied.
|
Hi, @ikhoon nim. |
|
@ikhoon , HI! If #6499 is merged, I can write test code for testing duplicated routing rules in nested context path. When you have time, please take another look. 🙇♂️ |
| final Set<String> mergedContextPaths = new HashSet<>(); | ||
| for (String currentContextPath : contextPaths()) { | ||
| for (String childContextPath : paths) { | ||
| RouteUtil.ensureAbsolutePath(childContextPath, "contextPath"); |
There was a problem hiding this comment.
Should we validate the paths when they are set in the builder methods?
There was a problem hiding this comment.
@ikhoon Thanks for your comments!
IMHO, It is not enough, so I think we must validate the paths before they are merged.
If we don't validate the context path before they are merged,
the codes below will not be failed.
Builder will see /hellorelative and it don't violate absolute path.
@Test
void relativeNestedContextPathShouldBeFailed() {
final ServerBuilder serverBuilder = new ServerBuilder().baseContextPath("/api");
assertThatThrownBy(() -> {
serverBuilder.contextPath(ImmutableSet.of("/hello"), ctx1 -> {
ctx1.contextPath(ImmutableSet.of("relative"), ctx2 -> {
ctx2.serviceUnder("/hello", (ctx, req) -> HttpResponse.of(HttpStatus.OK));
});
});
}).isInstanceOf(IllegalArgumentException.class);
}So, I think keeping RouteUtil.ensureAbsolutePath(...) in mergeContextPaths(...) is reasonable.
What do you think?
There was a problem hiding this comment.
My intent was to promptly validate relative if the path is absolute when ctx1.contextPath(ImmutableSet.of("relative") is called.
See
https://armeria.dev/community/developer-guide#validate to learn Armeria code convention.
There was a problem hiding this comment.
Sorry, I missed this comments 🙇♂️
I addressed it.
| }); | ||
| }); | ||
| }) | ||
| .contextPath("/single-path1", ctx1 -> { |
There was a problem hiding this comment.
Optional) It would be nice if we had some Kotlin tests.
There was a problem hiding this comment.
OK, Let me try!
If #6499 is merged, I can write test code for testing duplicated routing rules in nested context path.
I will create bunch of test codes to validate duplicate nested context path.
At that time, I will create some kotlin tests as well!
|
@ikhoon nim |
ikhoon
left a comment
ikhoon
left a comment
There was a problem hiding this comment.
Thanks, @chickenchickenlove! 👍🚀
| final Set<String> mergedContextPaths = new HashSet<>(); | ||
| for (String currentContextPath : contextPaths()) { | ||
| for (String childContextPath : paths) { | ||
| RouteUtil.ensureAbsolutePath(childContextPath, "contextPath"); |
There was a problem hiding this comment.
My intent was to promptly validate relative if the path is absolute when ctx1.contextPath(ImmutableSet.of("relative") is called.
See
https://armeria.dev/community/developer-guide#validate to learn Armeria code convention.
| contextPath(ImmutableSet.of(path), customizer); | ||
| return this; |
There was a problem hiding this comment.
nit
| contextPath(ImmutableSet.of(path), customizer); | |
| return this; | |
| return contextPath(ImmutableSet.of(path), customizer); |
| /** | ||
| * Applies the specified {@code customizer} to a {@link ContextPathServicesBuilder} rooted at the | ||
| * given {@code contextPaths} of the default {@link VirtualHost}. | ||
| * @param contextPaths the context paths to group services under; must be non-empty and absolute |
There was a problem hiding this comment.
must be non-empty
This constrain doesn't seem to be applied.
There was a problem hiding this comment.
I make it constrain.
Please take a look!
| contextPath(ImmutableSet.of(path), customizer); | ||
| return this; |
There was a problem hiding this comment.
| contextPath(ImmutableSet.of(path), customizer); | |
| return this; | |
| return contextPath(ImmutableSet.of(path), customizer); |
WalkthroughAdds nested context-path configuration overloads that accept a Consumer to create and customize child builders rooted at combined context paths; includes absolute/non-empty path validation, new abstract APIs, concrete implementations on server/virtual-host builders, and comprehensive Java/Kotlin tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/src/test/java/com/linecorp/armeria/server/HttpServerNestedContextPathTest.java (2)
347-362: Consider adding descriptive assertion messages for easier debugging.When assertions fail within the loop, identifying the failing URL requires additional investigation. Adding context to assertions would improve test maintainability.
- assertThat(res.status()).isEqualTo(expectedRes.status); + assertThat(res.status()) + .as("Status mismatch for URL: %s", url.getKey()) + .isEqualTo(expectedRes.status); if (res.status() == HttpStatus.OK) { - assertThat(res.content(StandardCharsets.UTF_8)).isEqualTo(expectedRes.body); + assertThat(res.content(StandardCharsets.UTF_8)) + .as("Body mismatch for URL: %s", url.getKey()) + .isEqualTo(expectedRes.body); }
484-527: Naming convention inconsistency with other test methods.These test methods use
snake_casewhile other methods in this file usecamelCase. Consider aligning the naming convention for consistency.- void duplicate_path_should_be_failed_in_2depth_nested_context_with_server_builder() { + void duplicatePathShouldBeFailedIn2DepthNestedContextWithServerBuilder() {- void duplicate_path_should_be_failed_in_1depth_nested_context_with_server_builder() { + void duplicatePathShouldBeFailedIn1DepthNestedContextWithServerBuilder() {core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServicesBuilder.java (1)
23-25: Nested contextPath API and helpers look sound; fix minor Javadoc nitThe new helpers (
virtualHostBuilder(),parent(),mergedContextPaths(...)) plus the abstractcontextPath(Iterable<String>, Consumer<SELF>)/contextPath(String, Consumer<SELF>)give a clean foundation for the concrete builders to build the context-path tree, and the Cartesian-product merge viaHashSetis appropriate.One small follow-up:
- In the Javadoc of the
contextPath(String path, Consumer<SELF> customizer)overload, the@throwsclause still refers topathsinstead ofpath, which can be confusing when reading the API docs.Optionally, you could also consider relaxing the consumer type to
Consumer<? super SELF>in the abstract signature to allow slightly more flexible callers, but that’s not required for correctness here.Also applies to: 415-432, 459-469
core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java (1)
465-503: VirtualHost-level contextPath customizer overloads are consistent; explicit empty-check is optionalThe new
contextPath(Iterable<String>, Consumer<VirtualHostContextPathServicesBuilder>)and single-path overload:
- Null-check both parameters.
- Reuse the existing
contextPath(Iterable<String>)factory to obtain aVirtualHostContextPathServicesBuilder, so non-empty and absolute-path constraints are enforced by the underlying builder construction.- Return
thisfor fluent chaining, matching the rest of the API.If you want error reporting to mirror
ServerBuilder.contextPath(Iterable, Consumer), you could add an explicitIterables.isEmpty-style check here to fail early on empty iterables, but current behavior is already correct and throws anIllegalArgumentExceptionvia the child builder constructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServicesBuilder.java(2 hunks)core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java(2 hunks)core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java(3 hunks)core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java(1 hunks)core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java(2 hunks)core/src/test/java/com/linecorp/armeria/server/HttpServerNestedContextPathTest.java(1 hunks)kotlin/src/test/kotlin/com/linecorp/armeria/server/kotlin/HttpServerNestedContextPathTest.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java (2)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (1)
UnstableApi(43-132)core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
UnstableApi(44-136)
kotlin/src/test/kotlin/com/linecorp/armeria/server/kotlin/HttpServerNestedContextPathTest.kt (1)
core/src/test/java/com/linecorp/armeria/server/HttpServerNestedContextPathTest.java (1)
StatusCodeAndBody(577-590)
core/src/test/java/com/linecorp/armeria/server/HttpServerNestedContextPathTest.java (2)
junit5/src/main/java/com/linecorp/armeria/testing/junit5/server/ServerExtension.java (1)
ServerExtension(49-405)kotlin/src/test/kotlin/com/linecorp/armeria/server/kotlin/HttpServerNestedContextPathTest.kt (3)
VIRTUAL_HOSTNAME(45-669)configure(53-187)status(381-384)
core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (1)
UnstableApi(43-132)
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (3)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (1)
UnstableApi(43-132)core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
UnstableApi(44-136)core/src/main/java/com/linecorp/armeria/server/ContextPathServiceBindingBuilder.java (1)
UnstableApi(36-44)
⏰ 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). (12)
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: flaky-tests
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: Kubernetes Chaos test
🔇 Additional comments (8)
core/src/test/java/com/linecorp/armeria/server/HttpServerNestedContextPathTest.java (2)
577-590: LGTM!The helper class is clean and serves its purpose well for pairing expected status codes with response bodies.
52-154: Solid test configuration covering the nested context path API.The server configuration exercises a comprehensive range of scenarios: multi-path context sets, deeply nested contexts (4 levels with
/single-path1/.../single-path4), virtual hosts, exact vs. prefix matching, and parameterized routes. This provides good coverage for the new feature.core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (1)
25-29: Concrete nested contextPath implementation validates inputs and composes paths correctlyThe
contextPath(Iterable<String>, Consumer<ContextPathServicesBuilder>)and single-path overload both:
- Enforce non-null arguments.
- Validate each path as absolute via
RouteUtil.ensureAbsolutePath.- Create a child builder using
parent(),virtualHostBuilder(), andmergedContextPaths(paths), then scope the customizer to that child while returningthisfor fluent chaining.This matches the abstract contract and Armeria’s “validate on setter” guideline, and nicely centralizes the multi-path-to-single-path delegation via
ImmutableSet.of(path).Also applies to: 106-131
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (1)
69-69: Server-level contextPath customizer overloads correctly enforce non-empty, absolute pathsThe new
contextPath(Iterable<String>, Consumer<ContextPathServicesBuilder>)and single-path overload:
- Enforce non-null
contextPathsandcustomizer.- Explicitly reject empty iterables with a clear
IllegalArgumentException, satisfying the “must be non-empty” contract.- Validate each context path as absolute via
RouteUtil.ensureAbsolutePath.- Reuse the existing
contextPath(Iterable<String>)factory so builder creation stays centralized, then scope thecustomizerto that scoped builder.This design lines up well with the dev-guide recommendation to validate arguments at the point they’re set and gives a clean, Java-friendly nested DSL for context paths.
Also applies to: 103-103, 1350-1395
core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
25-29: Virtual-host nested contextPath APIs mirror server-side behavior and validate paths eagerlyThe new
contextPath(Iterable<String>, Consumer<VirtualHostContextPathServicesBuilder>)and single-path overload:
- Null-check both arguments.
- Ensure each child path is absolute via
RouteUtil.ensureAbsolutePathbefore computing merged context paths.- Build a child
VirtualHostContextPathServicesBuilderwithparent(),virtualHostBuilder(), andmergedContextPaths(paths), then apply the customizer and returnthis.This matches the semantics of the server-level
ContextPathServicesBuilderand keeps validation and path composition consistent across entry points.Also applies to: 108-135
kotlin/src/test/kotlin/com/linecorp/armeria/server/kotlin/HttpServerNestedContextPathTest.kt (3)
49-187: Comprehensive server and virtual-host setup exercises the full nested context-path surfaceThe
ServerExtensionconfiguration plus theTEST_URLS/VIRTUAL_HOST_TEST_URLStables do a nice job of:
- Covering multi-depth Cartesian products of context paths under a base context path.
- Validating both annotated services and plain
service/serviceUnderroutes, including prefix and exact-match behavior.- Exercising both the default virtual host and a named virtual host (
foo.com), which is important for the VirtualHostBuilder-side APIs.This gives strong end-to-end coverage for the new nested contextPath DSL.
Also applies to: 190-378
386-560: Validation and normalization tests align tightly with the new contextPath contractThe tests for:
- Empty and relative (top-level and nested) context paths.
- Duplicate context paths being merged without generating extra distinct routes.
- No double-slash in exact routes for both base and nested context paths.
all directly exercise the new Java-side validation and merging logic and will catch regressions in path validation or normalization. The use of
RoutePathType.EXACTand route enumeration to assert on actual registered paths is particularly helpful for guarding against subtle path-joining bugs.
562-668: Duplicate-route tests ensure nested context paths respect RejectedRouteHandler semanticsThe four duplicate-route tests (for both ServerBuilder and VirtualHostBuilder, at 1- and 2-depth nesting):
- Configure
RejectedRouteHandler.FAIL.- Intentionally register identical effective routes via different nested-context combinations.
- Assert that
DuplicateRouteExceptionis thrown onbuild().These are important to verify that the nested
contextPathAPIs integrate correctly with the existing duplicate-route detection logic and will help prevent subtle regressions in future refactors.
| @Test | ||
| void duplicatedContextPathShouldbeMerged() { |
There was a problem hiding this comment.
Minor typo in method name.
The method name has inconsistent capitalization: Shouldbe should be ShouldBe.
- void duplicatedContextPathShouldbeMerged() {
+ void duplicatedContextPathShouldBeMerged() {📝 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.
| @Test | |
| void duplicatedContextPathShouldbeMerged() { | |
| @Test | |
| void duplicatedContextPathShouldBeMerged() { |
🤖 Prompt for AI Agents
In
core/src/test/java/com/linecorp/armeria/server/HttpServerNestedContextPathTest.java
around lines 426 to 427, the test method name has a capitalization typo:
`duplicatedContextPathShouldbeMerged` should be
`duplicatedContextPathShouldBeMerged`; rename the method to correct the
camel-casing (update declaration and any references) so `Shouldbe` becomes
`ShouldBe`.
|
@ikhoon nim, |
| requireNonNull(paths, "contextPaths"); | ||
| requireNonNull(customizer, "customizer"); | ||
|
|
||
| for (String path : paths) { |
There was a problem hiding this comment.
Question) Should the empty check be also done here? (if paths.isEmpty())
| requireNonNull(paths, "contextPaths"); | ||
| requireNonNull(customizer, "customizer"); | ||
|
|
||
| for (String path : paths) { |
There was a problem hiding this comment.
Question) Is there no need to check if paths is empty here?
There was a problem hiding this comment.
Thanks for your comments!
I added empty check logic in both ContextPathServercesBuilder and VirutlaHostContextPathServiceBuilder.!
| } | ||
|
|
||
| VirtualHostBuilder virtualHostBuilder() { | ||
| return this.virtualHostBuilder; |
There was a problem hiding this comment.
nit:
| return this.virtualHostBuilder; | |
| return virtualHostBuilder; |
There was a problem hiding this comment.
Addressed!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServicesBuilder.java (1)
423-432: Consider validating thatpathsis not empty inmergedContextPaths.The
mergedContextPathsmethod silently returns an empty set ifpathsis empty, which could lead to confusing behavior. While the calling code in subclasses validates this, defensive validation here would catch misuse from future callers.However, since the abstract
contextPathmethods document thatpathsmust be non-empty and the concrete implementations validate this before callingmergedContextPaths, this is acceptable.final Set<String> mergedContextPaths(Iterable<String> paths) { + requireNonNull(paths, "paths"); final Set<String> mergedContextPaths = new HashSet<>(); for (String currentContextPath : contextPaths()) { for (String childContextPath : paths) { final String mergedContextPath = currentContextPath + childContextPath; mergedContextPaths.add(mergedContextPath); } } return ImmutableSet.copyOf(mergedContextPaths); }core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
133-141: Consider extracting the common validation logic to reduce duplication.Both
ContextPathServicesBuilderandVirtualHostContextPathServicesBuilderhave identical validation logic in theircontextPath(Iterable, Consumer)implementations. While the child builder instantiation must differ, the validation could potentially be extracted to a helper inAbstractContextPathServicesBuilder.For example, add a protected helper in
AbstractContextPathServicesBuilder:protected static void validateContextPaths(Iterable<String> paths) { requireNonNull(paths, "contextPaths"); boolean isEmpty = true; for (String path : paths) { isEmpty = false; RouteUtil.ensureAbsolutePath(path, "contextPath"); } if (isEmpty) { throw new IllegalArgumentException("contextPaths must not be empty."); } }Then in subclasses:
public VirtualHostContextPathServicesBuilder contextPath( Iterable<String> paths, Consumer<VirtualHostContextPathServicesBuilder> customizer) { - requireNonNull(paths, "contextPaths"); requireNonNull(customizer, "customizer"); - - boolean isEmpty = true; - for (String path : paths) { - isEmpty = false; - RouteUtil.ensureAbsolutePath(path, "contextPath"); - } - - if (isEmpty) { - throw new IllegalArgumentException("contextPaths must not be empty."); - } + validateContextPaths(paths); final VirtualHostContextPathServicesBuilder child = ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServicesBuilder.java(2 hunks)core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java(2 hunks)core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.javacore/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.javacore/src/main/java/com/linecorp/armeria/server/AbstractContextPathServicesBuilder.java
🧬 Code graph analysis (1)
core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (1)
UnstableApi(43-139)
⏰ 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). (13)
- GitHub Check: Kubernetes Chaos test
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: flaky-tests
🔇 Additional comments (5)
core/src/main/java/com/linecorp/armeria/server/AbstractContextPathServicesBuilder.java (2)
414-421: LGTM! Package-private accessors for child builder construction.These accessors enable subclasses to create child builders while maintaining encapsulation. The visibility is appropriately restricted.
434-469: Well-documented abstract API for nested context paths.The JavaDoc clearly explains the Cartesian product semantics, the absolute path requirement, and provides a helpful example. The
@throwsand@seeannotations improve discoverability.core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (2)
107-130: Implementation correctly validates inputs and creates nested context.The validation logic properly:
- Checks non-null for both arguments
- Validates each path is absolute via
RouteUtil.ensureAbsolutePath- Tracks and throws on empty paths
This addresses the past review comment from jrhee17 about empty check.
132-138: LGTM! Clean delegation to the Iterable overload.The single-path convenience method correctly delegates to the multi-path variant using
ImmutableSet.of(path).core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
108-131: Implementation mirrorsContextPathServicesBuildercorrectly.The validation and child-builder creation logic is identical to
ContextPathServicesBuilder, which is expected given the abstract API contract. The duplication is unavoidable due to Java's generics requiring concrete types for child builder instantiation.One minor observation: this addresses the past review comment from jrhee17 regarding the empty check.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
108-131: Implementation is correct and consistent with ContextPathServicesBuilder.The validation and child builder creation logic mirrors the implementation in
ContextPathServicesBuilder, addressing jrhee17's previous question about empty path validation.Consider extracting the shared validation logic (null checks, empty check, absolute path validation) into a helper method in
AbstractContextPathServicesBuilderto reduce duplication between this class andContextPathServicesBuilder.// In AbstractContextPathServicesBuilder, add a protected helper: protected static void validateContextPaths(Iterable<String> paths) { boolean isEmpty = true; for (String path : paths) { isEmpty = false; RouteUtil.ensureAbsolutePath(path, "contextPath"); } if (isEmpty) { throw new IllegalArgumentException("contextPaths must not be empty."); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java(2 hunks)core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.javacore/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java
🧬 Code graph analysis (1)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (1)
core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (1)
UnstableApi(44-142)
⏰ 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). (13)
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: flaky-tests
- GitHub Check: Kubernetes Chaos test
🔇 Additional comments (5)
core/src/main/java/com/linecorp/armeria/server/ContextPathServicesBuilder.java (3)
25-28: LGTM!The imports for
ImmutableSetandRouteUtilare appropriate for the new nested context path functionality.
107-130: Implementation correctly addresses previous review concerns.The validation logic properly handles:
- Null checks for both
pathsandcustomizer- Empty iterable detection via the
isEmptyflag pattern (sinceIterablelacksisEmpty())- Absolute path validation via
RouteUtil.ensureAbsolutePathThis addresses jrhee17's previous question about empty path validation.
132-138: LGTM!The single-path overload correctly delegates to the iterable variant using
ImmutableSet.of(path), as suggested in previous reviews.core/src/main/java/com/linecorp/armeria/server/VirtualHostContextPathServicesBuilder.java (2)
25-28: LGTM!Imports are consistent with
ContextPathServicesBuilder.
133-141: LGTM!The single-path overload correctly delegates to the iterable variant, consistent with the pattern in
ContextPathServicesBuilder.
|
Thanks, @chickenchickenlove! |
4 participants
Motivation:
armeriasupports only1-depthcontext paths. Sometimes, user want deeper context paths than 1-depth when they usecontextPaths().Modifications:
ContextPathServiceBuildertree to support nested context paths.before()is for going back previous node.contextPath()is for adding context paths and making childContextPathServiceBuilderparent(): To give ChildcontextPathServiceBuilderparent object. because of this, child can return parent object whenand()is called.virtualHostbuilder(): to relay their context viaContextPathServiceBuildertree.mergeContextPaths(): to merge previous context paths and current context paths.Example
Result:
/rest/catalog/product=>getProductService/rest/catalog/products=>productsHandler/rest/cart/foo/bar/checkout=>checkoutService/gql/catalog=>GraphQLService