gnodet-bot commented on code in PR #28377:
URL: https://github.com/apache/camel/pull/28377#discussion_r4365260303
##########
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:
🔒 **Concurrency note:** `isEmpty()` iterates `methodMap.values()` without
holding `methodMapLock`. This is consistent with `handleRequest()`, which also
reads the map lock-free and relies on `ConcurrentHashMap`'s iteration safety
guarantee. It's correct, but worth a Javadoc note mirroring the comment in
`handleRequest()` — e.g. *"lock-free: relies on ConcurrentHashMap's consistent
iterator under concurrent modifications"* — so the intent is clear to the next
reader.
##########
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:
💡 **Design nit:** `defaultHandlerWrapper.fallback = fallbackHandler` is a
direct field write on a package-private inner class, bypassing the
encapsulation pattern used for the `handler` field (which goes through
`set()`). This works here because both the outer class and the inner class are
in the same file, but it also means `setDefault()` in `Wrapper` (if one is ever
added) won't keep `fallback` in sync. Consider adding a
`setFallback(HttpHandler)` method to `Wrapper` to keep the access pattern
consistent.
##########
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:
📝 **Docs:** `fallback` has no setter (access is through direct field
assignment in the outer class) and is effectively write-once after
construction. A brief Javadoc comment explaining the contract — e.g. *"set once
at construction by the enclosing class; null means fall back to the default 405
behaviour"* — would help future readers understand why there's no setter and
prevent accidental direct-write copies elsewhere.
--
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]
