Repository navigation
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 of 704 tested, 25 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (3 direct + 8 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (25)
All tested modules (36 modules, 5m 38s total)Total reactor time: 5m 38s
Top 20 slowest modules:
|
…r shares the secret engine between lookups The engine of a hashicorp: lookup was kept in a field of the shared properties function, so concurrent lookups could read from the wrong engine and a lookup without engine used the engine of the lookup before. The engine is now local to each lookup and defaults to the 'secret' engine. A missing secret with a field now resolves to no value (or the default value) instead of failing with a NullPointerException. Fixed the docs example of a default value with a version, which was missing the engine. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…se on CAMEL-25518 A lookup without engine now reads from the default secret engine instead of a null engine. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
7e3a5d5 to
92d3d41
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Good fix overall — the root cause (shared mutable engine field) is correctly identified and addressed.
What the PR gets right:
- Moving
enginefrom an instance field to a local variable inapply()is the right fix for the concurrency bug: the method is documented (and tested) as shared across concurrent lookups, so any per-call state must be local. - The
DEFAULT_ENGINE = "secret"constant makes the fallback explicit and readable. - The
instanceof Map<?,?> dataguard on the subkey path eliminates the raw cast + potential NPE in a clean, idiomatic way. - The concurrent stress test (
concurrentLookupsReadFromTheirOwnEngine) is exactly the right regression test and validates the fix properly. - The
@versionhandling in the non-subkey/non-#branch is a useful extension.
Minor observation — consistency of null-safety hardening:
The subkey path now has thorough null guards:
if (rawSecret != null && rawSecret.getData() != null
&& rawSecret.getData().get("data") instanceof Map<?, ?> data) {However, the non-subkey path (a few lines above, where there is no # in the key) retains an unguarded call:
if (ObjectHelper.isNotEmpty(rawSecret)) {
returnValue = rawSecret.getData().get("data").toString();
}This was already there before this PR and is therefore pre-existing, not a regression. However, since the PR already touched the surrounding code, it would be more consistent to apply the same guard here. rawSecret.getData() could return null if Vault returns a response without a data map, and .get("data") could return null even when the map exists, causing a NullPointerException on .toString().
Suggested hardening (optional, can be a follow-up):
if (rawSecret != null && rawSecret.getData() != null
&& rawSecret.getData().get("data") instanceof Map<?, ?> data) {
returnValue = data.toString();
}This review was generated by an AI agent, Hermès on behalf of @gnodet.
… to no value also without a field
Address review: the lookup without a field read rawSecret.getData().get("data") unguarded, so a response without data failed with a NullPointerException. Both paths now read the secret data once, null-safe.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after e9c57b3: the previous finding (inconsistent null-safety between the subkey and non-subkey paths of getSecretFromSource) is fully addressed. Both paths now use the same null-safe chain — rawSecret → responseData → secretData — so a missing secret or a response without a data map correctly resolves to no value instead of throwing a NullPointerException.
The new else if (remainder.contains("@")) branch correctly handles bare key@version lookups (no engine, no subkey) that previously fell through without version extraction. Test coverage is comprehensive: engine isolation under concurrency, the default engine, versions, missing secret/field with and without defaults, and responses without data. All good to merge.
Metadata: bug fix on main — consider backporting to camel-4.22.x (no port/camel-4.22.x label yet).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
2 participants
The
hashicorpproperties function is shared by all lookups, but kept the parsed secret engine in a field, so concurrent lookups for different engines could read from the wrong engine, and a lookup without engine used the engine of the previous lookup (ornull).hashicorp:mysecret,hashicorp:mysecret#field,hashicorp:mysecret@2) uses thesecretengine, as the secret refresh task already assumes.hashicorp:secret:missing#field), or a response without data (with or without a field), now resolves to no value / the default value instead of failing with aNullPointerException, so optional placeholders work.{{hashicorp:route:default@2}}example (parsed as engineroute) to{{hashicorp:secret:route:default@2}}, and documented the default engine.Tests:
HashicorpVaultPropertiesFunctionTest(fakeVaultTemplate, no Docker) covers engine isolation (sequential and concurrent), the default engine, versions, missing secret/field with and without default, and responses without data.Rebased on #27669 (CAMEL-25518, merged): the engine passed to
getSecretFromSourceis now also the one used for the name the secret refresh task tracks (trackedSecretName), and a stale test comment about the null engine is updated.HashicorpVaultPropertiesFunctionTestandHashicorpVaultReloadTriggerTaskTestpass (14 tests).Fixes CAMEL-25552
Claude Code on behalf of davsclaus
🤖 Generated with Claude Code