Croway commented on PR #1900:
URL:
https://github.com/apache/camel-spring-boot/pull/1900#issuecomment-5441526018
@oscerd I think there is room for improvement here, in particular wrt code
duplication, I think we might create an abstract
`EarlyResolutionPropertiesParser` (or something similar, naming things is hard)
in `core/camel-spring-boot` and refactor the PropertiesParser, can be done in
another PR.
Moreover I have some agents findings (I didn't verify them though)
```
- Precedence is inverted. getPropertySources() iterates highest-precedence
first, and all seven put into one flat Properties. Last write wins, so the
lowest-precedence source's value survives, and then the merged result is
addFirsted above everything. If the same key holds a vault placeholder in both
application.properties and an application-prod.properties, the wrong one wins.
Fix is to iterate in reverse.
- replace() should be substring(). .replace("{{aws:", "").replace("}}", "")
strips every occurrence, not just the delimiters. A remainder containing }}
gets silently mangled.
- First failure aborts. Introduced by this PR. An operator with five broken
placeholders restarts five times to find them all. Collecting the failures and
throwing one aggregate is barely more code and much kinder.
- Boolean.parseBoolean(getProperty(...)) vs getProperty(key, Boolean.class,
false). With parseBoolean, ignore-resolution-failures=yes silently means false.
That fails closed, so it's the safe direction, but for a flag whose whole
purpose is opting out of a security behaviour I'd rather a typo produced a
conversion error than silent strictness. Your call, worth being deliberate
about it.
- The flag is read before the early-resolve guard in all seven, so it's
evaluated even when the feature is off. Harmless, reads oddly, disappears under
the abstraction.
- "Fail closed" is narrower than the PR body implies. The match is
startsWith(prefix) && endsWith("}}"), so only values that are entirely a
placeholder are handled. url=https://host/{{aws:path}}/x is never touched here
and falls through to normal Camel resolution. Worth stating precisely in the
upgrade guide, since that's what people will read to understand the breaking
change.
- Google's parser wraps IOException in plain RuntimeException, not
RuntimeCamelException, unlike its own sibling branch two lines down.
- Worth checking: PropertiesFunction has an optional(remainder) default
method for "missing is acceptable." The early resolver ignores it, so a
placeholder Camel would treat as optional at normal resolution time now aborts
startup at early resolution time. I haven't traced whether any vault function
overrides it, but the semantics should probably line up.
```
--
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]
