gnodet-bot commented on code in PR #28377:
URL: https://github.com/apache/camel/pull/28377#discussion_r4366699368
##########
components/camel-undertow/src/main/java/org/apache/camel/component/undertow/handlers/CamelPathTemplateHandler.java:
##########
@@ -36,7 +36,18 @@ public class CamelPathTemplateHandler implements HttpHandler
{
private String handlerString;
public CamelPathTemplateHandler(CamelMethodHandler defaultHandler) {
+ this(defaultHandler, null);
+ }
+
+ /**
+ * @param defaultHandler the handler of the static base path, for a
request that no template matches
+ * @param fallbackHandler the handler for a request that no template
matches while no static base path is set (the
+ * handler of the requests that no path of the
server matches), or null to let the empty
+ * default handler answer 405
+ */
+ public CamelPathTemplateHandler(CamelMethodHandler defaultHandler,
HttpHandler fallbackHandler) {
defaultHandlerWrapper.set(defaultHandler);
+ defaultHandlerWrapper.fallback = fallbackHandler;
Review Comment:
📝 **Nit (still open from previous review):** `defaultHandlerWrapper.fallback
= fallbackHandler` is a direct field write, while `handler` goes through
`set()`. The author replied that `fallback` is write-once at construction — if
that's the design intent, a `setFallback(HttpHandler)` method on `Wrapper`
(even just delegating to a plain field assignment) would make the access
pattern consistent and prevent future callers from bypassing it with another
direct write.
##########
components/camel-undertow/src/main/java/org/apache/camel/component/undertow/handlers/CamelMethodHandler.java:
##########
@@ -67,6 +67,18 @@ public void handleRequest(HttpServerExchange exchange)
throws Exception {
}
}
+ /**
+ * Whether no handler is set for any method, such as the default handler
of a template path that has no static path.
+ */
+ public boolean isEmpty() {
Review Comment:
📝 **Nit (still open from previous review):** The Javadoc does not mention
that `isEmpty()` iterates `methodMap` without holding `methodMapLock`. The
author confirmed this is intentional (same rationale as `handleRequest()`). A
note mirroring the comment there would help future readers.
```suggestion
/**
* Whether no handler is set for any method, such as the default handler
of a template path that has no static path.
* <p>
* Iterates {@code methodMap} without holding {@code methodMapLock};
reads are weakly consistent
* (same rationale as {@link #handleRequest}).
*/
public boolean isEmpty() {
```
##########
components/camel-undertow/src/main/java/org/apache/camel/component/undertow/handlers/CamelPathTemplateHandler.java:
##########
@@ -106,6 +117,7 @@ public String toString() {
static class Wrapper implements HttpHandler {
private CamelMethodHandler handler;
+ private HttpHandler fallback;
Review Comment:
📝 **Nit (still open from previous review):** `fallback` has no Javadoc.
Given that the author confirmed it is write-once at construction (never changed
afterwards), a brief comment explaining the contract would prevent future
readers from wondering why there is no setter.
```suggestion
/** Set once at construction by the enclosing class; null means fall
back to the default 405 behaviour. */
private HttpHandler fallback;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
