davsclaus commented on code in PR #28560:
URL: https://github.com/apache/camel/pull/28560#discussion_r4388418910
##########
components/camel-lra/src/main/java/org/apache/camel/service/lra/LRASagaService.java:
##########
@@ -90,7 +91,17 @@ public LRASagaService() {
@Override
public CompletableFuture<CamelSagaCoordinator> newSaga(Exchange exchange) {
return client.newLRA(exchange)
- .thenApply(url -> new LRASagaCoordinator(LRASagaService.this,
url));
+ .thenApply(url -> {
+ URI lra = toUri(url);
+ if (lra == null || !isHostedByAllowedCoordinator(lra)) {
Review Comment:
When the URL is rejected here, the LRA has already been created by the `POST
.../start`, and nothing closes it. `newLRA` sends no `TimeLimit` (the time
limit is only added when a step joins), so the LRA stays active at the
coordinator indefinitely. With a misconfigured `allowedCoordinatorUrls`, every
incoming exchange leaves one more active LRA behind.
Cancelling it through the returned URL would defeat the check. Some options:
- cancel it through the **configured** coordinator, using the last path
segment of the returned URL (`coordinatorUrl + coordinatorContextPath + "/" +
id + "/cancel"`), on a best-effort basis
- or at least add a code comment saying the LRA is deliberately left open,
and mention it in the exception message, so an operator who finds stray active
LRAs knows where they came from
Non-blocking. Your call which one fits better.
##########
components/camel-lra/src/test/java/org/apache/camel/service/lra/LRASagaServiceTest.java:
##########
@@ -75,6 +78,86 @@ void testCallsClientWithExchange() throws Exception {
Mockito.verify(client).newLRA(exchange);
}
+ @DisplayName("Tests whether newSaga(Exchange) accepts an LRA hosted by the
configured coordinator")
+ @Test
+ void testNewSagaAcceptsAllowedCoordinatorUrl() throws Exception {
Review Comment:
Suggestion: the four new tests repeat the same setup block (mock client,
coordinator URL, context path, participant URL, CamelContext, `doStart()`).
`LRASagaServiceAllowedCoordinatorTest` already tests the allow-list for
`getSaga()` with `assertAccepted` / `assertRejected` helpers. `newSaga()`
variants of those helpers there, with a mocked `LRAClient`, would keep all the
allow-list tests in one class and remove the duplication. Alternatively, a
small `startedService(client)` helper in this class would do.
--
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]
