allthingssecurity opened a new pull request, #27802: URL: https://github.com/apache/camel/pull/27802
# Description [CAMEL-25615](https://issues.apache.org/jira/browse/CAMEL-25615) `bonita:startCase` sends three requests per exchange when the body contains a `FileInput`: the process lookup, the upload of the file, and the case creation. This PR fixes two defects of the upload step. Both are in `BonitaAPIUtil`: the first in how the helper is obtained (`getInstance`, called from `BonitaAPI.startCase`), the second in `uploadFile`. **The upload used the server and the user of the bonita endpoint that was used first in the JVM.** The lookup and the case creation use the client that `BonitaAPIBuilder` builds for the configuration of the current endpoint. The upload went through `BonitaAPIUtil.getInstance(config)`, a lazily created static singleton that kept the web target and the login filter of the configuration it was first called with and ignored the argument afterwards. With two bonita endpoints that differ in hostname, port, username or password, an exchange with a file input on the second endpoint looked the process up on its own server, uploaded the file to the server of the first endpoint with the user of the first endpoint, and then created the case on its own server with the `tempPath` of a file that this server never received. With a single bonita endpoint, or several with the same server and user, nothing was wrong. The singleton was also assigned before its web target was set, without synchronisation, so a second thread entering `getInstance` during the very first call could get a helper without a web target. I read this from the code and a small model shows it; there is no test for it. **A temporary file per upload was never deleted.** `BonitaAPIUtil.uploadFile` creates a file with `Files.createTempFile("tempFile", ".tmp")`, writes the content of the file input to it and uses only its name, as the `filename` of the multipart part (the content that is sent comes from the byte array). Nothing deleted it, so every uploaded file input left a copy of its content in the temporary directory. This does not need a second endpoint. The change: - `BonitaAPIBuilder.build` creates the upload helper from the client of the `BonitaAPI` that it builds, so the three requests of an exchange go to the same server with the same user. There is no static state any more. `BonitaStartProducer` already builds one `BonitaAPI` per exchange, so nothing is shared between concurrent exchanges, and no additional client is created. - New public method `BonitaAPIUtil.forClient(Client, BonitaAPIConfig)` and a new protected constructor `BonitaAPI(BonitaAPIConfig, WebTarget, BonitaAPIUtil)`. `BonitaAPIUtil.getInstance(BonitaAPIConfig)` and the protected constructor `BonitaAPI(BonitaAPIConfig, WebTarget)` are kept; `getInstance` now returns a new helper (with a client of its own) for the given configuration on every call instead of the shared one. - `JsonClientFilter`, which makes the `text/plain` answer of the upload readable as JSON, is registered on the upload target only. On main it was registered on the separate client of the helper, so the lookup and the case creation are read as before. I checked this with a throw-away probe (a lookup answered as `text/plain` is still rejected with "No message body reader has been found"); the probe is not part of the PR. - `uploadFile` no longer writes the content of the file input to the temporary file, because nothing reads it back, and deletes the file in a `finally` block with `File.delete()`, which does not throw, so a failing clean-up cannot replace the exception of the upload. The (now empty) file is still created with `Files.createTempFile`, because its generated name is what the server receives as the file name of the part. The multipart request is the same as on main: one part, `Content-Disposition: form-data;filename=tempFile<digits>.tmp;name=file`, with the bytes of the file input as content. I compared the request that the stub server receives with the production code of main and with this change; they differ only in the random boundary and the random digits of the name. No upgrade-guide entry: a route with one bonita endpoint sends the same requests as before. The one change of a public method is that `BonitaAPIUtil.getInstance` no longer returns a shared instance; Camel itself calls it only from the kept `BonitaAPI` constructor. Not changed, and worth a follow-up: `BonitaStartProducer` builds a new JAX-RS client for every exchange and never closes it, and `BonitaAuthFilter` logs in again before every request with another new client (the request never carries a `JSESSIONID` cookie when the filter runs). Both are as on main. Tests: - New `BonitaStartCaseFileUploadTest` (4 tests). A CamelContext with two `bonita:startCase` endpoints that point to two WireMock servers and have different users. Each stub answers the login with its own session cookie and API token, returns its own `tempPath` for an upload, and records its requests. Two tests send a file input through one endpoint and then through the other (both orders) and assert, for each server, that it received exactly the lookup, the upload and the case creation of its own exchanges, each with a login of its own user, its own session cookie and API token, and that the case refers to the `tempPath` this server returned. Two tests assert that the upload is one multipart part named `tempFile<digits>.tmp` with the content of the file input, and that the temporary file of that name is gone after the exchange, when the upload succeeds and when the server answers it with 500 (`text/plain`); in the second case the exchange must fail with `InternalServerErrorExcepti on`. - Without the change, each test in a JVM of its own: the two endpoint tests fail with `file uploads among the requests received by the Bonita server on port ... ==> expected: <1> but was: <0>` for the endpoint that was used second, and the two temporary-file tests with `the temporary file of the upload was left behind: ...tempFile....tmp ==> expected: <false> but was: <true>` (their assertions on the multipart part and on the exception pass on main; only the last one, on the file, fails). Run together in one JVM on main, only the first test sees the helper being created; the later ones fail with `jakarta.ws.rs.ProcessingException: java.net.ConnectException`, because their uploads go to the stopped stub server of the first test. In a run of the whole module on main the helper may already have been created by `BonitaAPIUtilPrepareInputsTest`, so there the failure messages depend on the order of the test classes as well as on the order of the tests; all four tests fail in either case . - With the change: camel-bonita 22 tests run, 0 failures, 0 skipped (18 existing, 4 new). What was not run: nothing was run against a Bonita server (`BonitaProducerManualIT` is disabled and needs a local Bonita). I do not know what a Bonita server does with a case whose file input names a `tempPath` it does not have, or with an upload made by another user of the same server; the tests show which server and which user every request is sent with. I also do not know how common two differently configured bonita endpoints in one application are; the temporary files were left for every file input. How it was found: a review of static mutable state that is initialised from endpoint configuration, and a small TLA+ model of two endpoints with interleaved `startCase` calls. The model is only a sketch of the code. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [ ] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). (The ticket is drafted and not filed yet; CAMEL-25615 is a placeholder until it is.) # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built and tested camel-bonita, including the formatter and import-sort plugins. I did not run the full root build; the change has no generated files.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
