gnodet-bot commented on code in PR #26766:
URL: https://github.com/apache/camel/pull/26766#discussion_r4081541019
##########
components/camel-rest-openapi/src/main/java/org/apache/camel/component/rest/openapi/RestOpenApiEndpoint.java:
##########
@@ -1005,16 +1004,21 @@ static OpenAPI loadSpecificationFrom(final CamelContext
camelContext, final Stri
if (openApi != null && openApi.getOpenAPI() != null) {
return openApi.getOpenAPI();
}
+ } catch (IllegalArgumentException e) {
+ if (e.getMessage() != null && e.getMessage().contains("URI is not
absolute")) {
+ throw new IllegalArgumentException(
+ "Failed to load OpenAPI specification from '" + uri +
"': URI is not absolute. "
+ + "Specify an absolute
location or define 'servers' in the OpenAPI contract.",
+ e);
+ }
+ throw e;
Review Comment:
⚠️ **Wrong location and fragile message match.**
This catch block intercepts `IllegalArgumentException` during *spec loading*
(`readLocation` / `readContents`) and rewraps it only when the message contains
`"URI is not absolute"`. Two problems:
1. **Wrong place.** For classpath/file resources, `loadSpecificationFrom`
reads the spec as text and never calls `new URI(...).toURL()` internally. The
`"URI is not absolute"` exception from `java.net.URI` is thrown later — when
the HTTP client actually tries to make a request using the relative server URL
in `determineHost` → `getURIs`. This catch will never fire for the described
scenario.
2. **Fragile.** `java.net.URI`'s error message is not part of its public
contract. Matching on `"URI is not absolute"` across JDK versions and locales
is a time-bomb.
The actionable error is already improved in `determineHost()` (the
`IllegalStateException` change). This catch block adds no value and should be
removed.
##########
components/camel-rest-openapi/src/test/java/org/apache/camel/component/rest/openapi/RestOpenApiEndpointV3Test.java:
##########
@@ -535,6 +557,18 @@ public void
shouldRaiseExceptionsForMissingSpecifications() {
() -> RestOpenApiEndpoint.loadSpecificationFrom(camelContext,
"non-existant.json"));
}
+ @Test
+ public void shouldLoadSpecificationWithoutServers() {
+ final CamelContext camelContext = new DefaultCamelContext();
+
+ OpenAPI openapi =
RestOpenApiEndpoint.loadSpecificationFrom(camelContext, "missing-servers.yaml");
+
+ assertThat(openapi).isNotNull();
+ assertThat(openapi.getServers()).isNotNull();
+ assertThat(openapi.getServers()).isNotEmpty();
+ assertThat(openapi.getServers().get(0).getUrl()).isEqualTo("/");
+ }
Review Comment:
⚠️ **This test validates pre-existing OpenAPI parser behavior, not anything
this PR changes.**
The OpenAPI 3.0 specification mandates that when no `servers` section is
present, parsers default to a single server with URL `"/"` (§4.7.5).
`swagger-parser` has always done this. This test was true before the PR and
will remain true regardless of any changes here.
Either:
- Remove this test (it adds no regression safety for the PR's stated
change), or
- Replace it with a test that verifies the PR's actual behavior: that
`determineHost()` throws a descriptive `IllegalStateException` when the only
server is relative (which `shouldFailDescriptivelyWhenServersSectionIsMissing`
already does — making this test fully redundant).
##########
components/camel-rest-openapi/src/test/resources/relative-server.yaml:
##########
@@ -0,0 +1,33 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+openapi: 3.0.0
+
+info:
+ title: Missing Servers Test
+ version: 1.0.0
+
+servers:
+ - url: /api/v1
+
+paths:
+ /test:
+ get:
+ operationId: test
+ responses:
+ '200':
+ description: OK
Review Comment:
❌ **Dead fixture — no test references this file.**
`relative-server.yaml` is added but never loaded by any test in this PR. If
it's meant to cover the case where a spec has a relative server URL (e.g.
`/api/v1`), add a test that uses it. Otherwise, remove it.
--
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]
