Croway commented on code in PR #26054:
URL: https://github.com/apache/camel/pull/26054#discussion_r3923408139
##########
components/camel-platform-http/src/main/java/org/apache/camel/component/platform/http/HttpEndpointModel.java:
##########
@@ -25,7 +25,7 @@
/**
* Model of available http endpoints.
*/
-public class HttpEndpointModel implements Comparable<HttpEndpointModel> {
+public class HttpEndpointModel {
Review Comment:
Dropping `Comparable` here is a public API/behavior break: downstream code
that puts these in a `TreeSet` (which worked against 4.22) will now throw
`ClassCastException` at runtime on 4.23. Worth restoring a `compareTo`
consistent with the new identity-aware `equals`, or calling out the removal in
the upgrade guide.
##########
components/camel-platform-http/src/main/java/org/apache/camel/component/platform/http/PlatformHttpComponent.java:
##########
@@ -72,8 +72,8 @@ public class PlatformHttpComponent extends
HeaderFilterStrategyComponent
+ " or all requests must be handled by Camel.")
private boolean serverRequestValidation = true;
- private final Set<HttpEndpointModel> httpEndpoints = new TreeSet<>();
- private final Set<HttpEndpointModel> httpManagementEndpoints = new
TreeSet<>();
+ private final Set<HttpEndpointModel> httpEndpoints = new LinkedHashSet<>();
Review Comment:
With uri-only equality gone, two consumers on the same path (e.g. GET +
POST) no longer collapse into one entry with merged verbs — each now survives
as its own single-verb row. `HttpEndpointModel#addVerb()` is effectively dead
code as a result (a fresh model is always constructed in `addHttpEndpoint`, so
there's never an existing instance to merge into). Not necessarily wrong, but
it's an undocumented, untested presentation change for the dev console /
startup summary.
##########
components/camel-platform-http/src/main/java/org/apache/camel/component/platform/http/PlatformHttpComponent.java:
##########
@@ -167,13 +167,48 @@ public void removeHttpEndpoint(String uri) {
this.removeHttpEndpoint(this.httpEndpoints, uri);
}
+ /**
+ * Removes the http endpoint registered for the given consumer.
+ */
+ public void removeHttpEndpoint(Consumer consumer) {
Review Comment:
Overloading `removeHttpEndpoint` with both `String` and `Consumer` makes a
bare `removeHttpEndpoint(null)` ambiguous at compile time for any external
caller — this PR's own test needed an explicit `(Consumer) null` cast. Worth a
heads-up in the PR description for anyone calling this method with a literal
`null`.
##########
components/camel-platform-http/src/main/java/org/apache/camel/component/platform/http/DefaultPlatformHttpConsumer.java:
##########
@@ -116,8 +116,8 @@ protected void doStart() throws Exception {
@Override
protected void doStop() throws Exception {
super.doStop();
- if (register) {
- getComponent().removeHttpEndpoint(getEndpoint().getPath());
+ if (register && platformHttpConsumer != null) {
Review Comment:
This guard is only added here in `doStop()` — `doStart()` (unchanged by this
PR) still registers unconditionally. If a custom `PlatformHttpEngine` ever
returns `null` from `createConsumer`, the endpoint gets registered on start but
can never be removed here. Worth mirroring the same null-guard in `doStart()`
for symmetry (narrow case — no in-tree engine does this today).
##########
components/camel-platform-http/src/main/java/org/apache/camel/component/platform/http/PlatformHttpComponent.java:
##########
@@ -167,13 +167,48 @@ public void removeHttpEndpoint(String uri) {
this.removeHttpEndpoint(this.httpEndpoints, uri);
}
+ /**
+ * Removes the http endpoint registered for the given consumer.
+ */
+ public void removeHttpEndpoint(Consumer consumer) {
+ if (consumer == null) {
+ return;
+ }
+ this.removeHttpEndpoint(this.httpEndpoints, consumer);
+ }
+
/**
* Removes a known http endpoint managed by this component.
*/
public void removeHttpManagementEndpoint(String uri) {
this.removeHttpEndpoint(this.httpManagementEndpoints, uri);
}
+ /**
+ * Removes the http management endpoint registered for the given consumer.
+ */
+ public void removeHttpManagementEndpoint(Consumer consumer) {
+ if (consumer == null) {
+ return;
+ }
+ this.removeHttpEndpoint(this.httpManagementEndpoints, consumer);
+ }
+
+ private void removeHttpEndpoint(Set<HttpEndpointModel> endpoints, Consumer
consumer) {
Review Comment:
Minor: this and `removeHttpEndpoint(Set, String)` below are now
near-identical except for the filter predicate. Could collapse into one private
`removeHttpEndpoints(Set<HttpEndpointModel>, Predicate<HttpEndpointModel>)` to
drop the duplication.
--
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]
