Skip to content

Commit b384e07

Browse files
ricardozaniniricardozanini
committed
bug: OpenAPIExecutor was swallowing server URL path; Caching same server + op;
Signed-off-by: Ricardo Zanini <[email protected]>
1 parent c8100c0 commit b384e07

File tree

5 files changed

+260
-64
lines changed

5 files changed

+260
-64
lines changed

‎impl/http/pom.xml‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
1+
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
2+
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
23
<modelVersion>4.0.0</modelVersion>
34
<parent>
45
<groupId>io.serverlessworkflow</groupId>
@@ -8,9 +9,13 @@
89
<artifactId>serverlessworkflow-impl-http</artifactId>
910
<name>Serverless Workflow :: Impl :: HTTP</name>
1011
<dependencies>
11-
<dependency>
12-
<groupId>io.serverlessworkflow</groupId>
13-
<artifactId>serverlessworkflow-impl-template-resolver</artifactId>
14-
</dependency>
12+
<dependency>
13+
<groupId>io.serverlessworkflow</groupId>
14+
<artifactId>serverlessworkflow-impl-template-resolver</artifactId>
15+
</dependency>
16+
<dependency>
17+
<groupId>org.junit.jupiter</groupId>
18+
<artifactId>junit-jupiter-engine</artifactId>
19+
</dependency>
1520
</dependencies>
1621
</project>

‎impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutor.java‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,41 @@ private static WorkflowValueResolver<WebTarget> getTargetSupplier(
254254

255255
private static WorkflowValueResolver<WebTarget> getTargetSupplier(
256256
WorkflowValueResolver<URI> uriSupplier, WorkflowValueResolver<URI> pathSupplier) {
257-
return (w, t, n) ->
258-
HttpClientResolver.client(w, t)
259-
.target(uriSupplier.apply(w, t, n).resolve(pathSupplier.apply(w, t, n)));
257+
return (w, t, n) -> {
258+
URI base = uriSupplier.apply(w, t, n);
259+
URI path = pathSupplier.apply(w, t, n);
260+
URI combined = buildTargetUri(base, path);
261+
return HttpClientResolver.client(w, t).target(combined);
262+
};
263+
}
264+
265+
static URI buildTargetUri(URI base, URI path) {
266+
if (path.isAbsolute()) {
267+
return path;
268+
}
269+
270+
String basePath = base.getPath();
271+
if (basePath == null || basePath.isEmpty()) {
272+
basePath = "/";
273+
} else if (!basePath.endsWith("/")) {
274+
basePath = basePath + "/";
275+
}
276+
277+
String relPath = path.getPath() == null ? "" : path.getPath();
278+
while (relPath.startsWith("/")) {
279+
relPath = relPath.substring(1);
280+
}
281+
282+
try {
283+
return new URI(
284+
base.getScheme(),
285+
base.getAuthority(),
286+
basePath + relPath,
287+
path.getQuery(),
288+
path.getFragment());
289+
} catch (Exception e) {
290+
throw new IllegalArgumentException(
291+
"Failed to build combined URI from base=" + base + " and path=" + path, e);
292+
}
260293
}
261294
}

‎impl/http/src/test/java/io/serverlessworkflow/impl/executors/http/HttpExecutorTargetSupplierTest.java‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/*
2+
* Copyright 2020-Present The Serverless Workflow Specification Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package io.serverlessworkflow.impl.executors.http;
17+
18+
import static org.junit.jupiter.api.Assertions.assertEquals;
19+
20+
import java.net.URI;
21+
import org.junit.jupiter.api.Test;
22+
23+
class HttpExecutorTargetSupplierTest {
24+
25+
@Test
26+
void openApiServerWithTrailingSlashAndRootPath() {
27+
URI base = URI.create("https://petstore3.swagger.io/api/v3/");
28+
URI path = URI.create("/pet/findByStatus");
29+
30+
URI result = HttpExecutor.buildTargetUri(base, path);
31+
32+
assertEquals("https://petstore3.swagger.io/api/v3/pet/findByStatus", result.toString());
33+
}
34+
35+
@Test
36+
void openApiServerWithoutTrailingSlashAndRootPath() {
37+
URI base = URI.create("https://petstore3.swagger.io/api/v3");
38+
URI path = URI.create("/pet/findByStatus");
39+
40+
URI result = HttpExecutor.buildTargetUri(base, path);
41+
42+
assertEquals("https://petstore3.swagger.io/api/v3/pet/findByStatus", result.toString());
43+
}
44+
45+
@Test
46+
void baseWithSlashAndRelativePath() {
47+
URI base = URI.create("https://example.com/api/v1/");
48+
URI path = URI.create("pets");
49+
50+
URI result = HttpExecutor.buildTargetUri(base, path);
51+
52+
assertEquals("https://example.com/api/v1/pets", result.toString());
53+
}
54+
55+
@Test
56+
void baseWithoutPathAndRootPath() {
57+
URI base = URI.create("https://example.com");
58+
URI path = URI.create("/pets");
59+
60+
URI result = HttpExecutor.buildTargetUri(base, path);
61+
62+
assertEquals("https://example.com/pets", result.toString());
63+
}
64+
65+
@Test
66+
void absolutePathOverridesBase() {
67+
URI base = URI.create("https://example.com/api");
68+
URI path = URI.create("https://other.example.com/foo");
69+
70+
URI result = HttpExecutor.buildTargetUri(base, path);
71+
72+
assertEquals("https://other.example.com/foo", result.toString());
73+
}
74+
75+
@Test
76+
void queryAndFragmentAreTakenFromPath() {
77+
URI base = URI.create("https://example.com/api/v1");
78+
URI path = URI.create("/pets?status=available#top");
79+
80+
URI result = HttpExecutor.buildTargetUri(base, path);
81+
82+
assertEquals("https://example.com/api/v1/pets?status=available#top", result.toString());
83+
}
84+
}

‎impl/openapi/src/main/java/io/serverlessworkflow/impl/executors/openapi/OpenAPIExecutor.java‎

Lines changed: 91 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import io.serverlessworkflow.impl.executors.http.HttpExecutor;
2929
import io.serverlessworkflow.impl.executors.http.HttpExecutor.HttpExecutorBuilder;
3030
import io.serverlessworkflow.impl.resources.ResourceLoaderUtils;
31+
import io.swagger.v3.oas.models.media.Schema;
3132
import io.swagger.v3.oas.models.parameters.Parameter;
3233
import java.util.Collection;
3334
import java.util.HashMap;
@@ -52,7 +53,10 @@ public void init(CallOpenAPI task, WorkflowDefinition definition) {
5253
OpenAPIArguments with = task.getWith();
5354
this.processor = new OpenAPIProcessor(with.getOperationId());
5455
this.resource = with.getDocument();
55-
this.parameters = with.getParameters().getAdditionalProperties();
56+
this.parameters =
57+
with.getParameters() != null && with.getParameters().getAdditionalProperties() != null
58+
? with.getParameters().getAdditionalProperties()
59+
: Map.of();
5660
this.builder =
5761
HttpExecutor.builder(definition)
5862
.withAuth(with.getAuthentication())
@@ -62,20 +66,27 @@ public void init(CallOpenAPI task, WorkflowDefinition definition) {
6266
@Override
6367
public CompletableFuture<WorkflowModel> apply(
6468
WorkflowContext workflowContext, TaskContext taskContext, WorkflowModel input) {
69+
70+
// In the same workflow, access to an already cached document
71+
final OperationDefinition operationDefinition =
72+
processor.parse(
73+
workflowContext
74+
.definition()
75+
.resourceLoader()
76+
.load(
77+
resource,
78+
ResourceLoaderUtils::readString,
79+
workflowContext,
80+
taskContext,
81+
input));
82+
83+
fillHttpBuilder(workflowContext.definition().application(), operationDefinition);
84+
// One executor per operation, even if the document is the same
85+
// Me may refactor this even further to reuse the same executor (since the base URI is the same,
86+
// but the path differs, although some use cases may require different client configurations for
87+
// different paths...)
6588
Collection<HttpExecutor> executors =
66-
workflowContext
67-
.definition()
68-
.resourceLoader()
69-
.<Collection<HttpExecutor>>load(
70-
resource,
71-
r -> {
72-
OperationDefinition o = processor.parse(ResourceLoaderUtils.readString(r));
73-
fillHttpBuilder(workflowContext.definition().application(), o);
74-
return o.getServers().stream().map(s -> builder.build(s)).toList();
75-
},
76-
workflowContext,
77-
taskContext,
78-
input);
89+
operationDefinition.getServers().stream().map(s -> builder.build(s)).toList();
7990

8091
Iterator<HttpExecutor> iter = executors.iterator();
8192
if (!iter.hasNext()) {
@@ -91,9 +102,9 @@ public CompletableFuture<WorkflowModel> apply(
91102
}
92103

93104
private void fillHttpBuilder(WorkflowApplication application, OperationDefinition operation) {
94-
Map<String, Object> headersMap = new HashMap<String, Object>();
95-
Map<String, Object> queryMap = new HashMap<String, Object>();
96-
Map<String, Object> pathParameters = new HashMap<String, Object>();
105+
Map<String, Object> headersMap = new HashMap<>();
106+
Map<String, Object> queryMap = new HashMap<>();
107+
Map<String, Object> pathParameters = new HashMap<>();
97108

98109
Map<String, Object> bodyParameters = new HashMap<>(parameters);
99110
for (Parameter parameter : operation.getParameters()) {
@@ -110,6 +121,8 @@ private void fillHttpBuilder(WorkflowApplication application, OperationDefinitio
110121
}
111122
}
112123

124+
validateRequiredParameters(operation, headersMap, queryMap, pathParameters);
125+
113126
builder
114127
.withMethod(operation.getMethod())
115128
.withPath(new OperationPathResolver(operation.getPath(), application, pathParameters))
@@ -124,4 +137,65 @@ private void param(String name, Map<String, Object> origMap, Map<String, Object>
124137
collectorMap.put(name, value);
125138
}
126139
}
140+
141+
private void validateRequiredParameters(
142+
OperationDefinition operation,
143+
Map<String, Object> headersMap,
144+
Map<String, Object> queryMap,
145+
Map<String, Object> pathParameters) {
146+
147+
StringBuilder missing = new StringBuilder();
148+
149+
for (Parameter parameter : operation.getParameters()) {
150+
if (!Boolean.TRUE.equals(parameter.getRequired())) {
151+
continue;
152+
}
153+
154+
String in = parameter.getIn();
155+
String name = parameter.getName();
156+
157+
Map<String, Object> targetMap =
158+
switch (in) {
159+
case "header" -> headersMap;
160+
case "path" -> pathParameters;
161+
case "query" -> queryMap;
162+
default -> null;
163+
};
164+
165+
if (targetMap == null) {
166+
// We don't currently handle other "in" locations here (e.g., cookie).
167+
// Treat as "not validated" instead of failing.
168+
continue;
169+
}
170+
171+
boolean present = targetMap.containsKey(name);
172+
173+
if (!present) {
174+
// Try to satisfy the requirement using the OpenAPI default, if any
175+
Schema<?> schema = parameter.getSchema();
176+
Object defaultValue = schema != null ? schema.getDefault() : null;
177+
178+
if (defaultValue != null) {
179+
targetMap.put(name, defaultValue);
180+
present = true;
181+
}
182+
}
183+
184+
if (!present) {
185+
if (!missing.isEmpty()) {
186+
missing.append(", ");
187+
}
188+
missing.append(in).append(" parameter '").append(name).append("'");
189+
}
190+
}
191+
192+
if (!missing.isEmpty()) {
193+
String operationId =
194+
operation.getOperation().getOperationId() != null
195+
? operation.getOperation().getOperationId()
196+
: "<unknown>";
197+
throw new IllegalArgumentException(
198+
"Missing required OpenAPI parameters for operation '" + operationId + "': " + missing);
199+
}
200+
}
127201
}

0 commit comments

Comments
 (0)