You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "davsclaus (via GitHub)" <gi...@apache.org> on 2026/10/01 17:24:29 UTC
[PR] CAMEL-25238: a chosen branch silences the other branches of a pick-one construct [camel]
davsclaus opened a new pull request, #27231:
URL: https://github.com/apache/camel/pull/27231
**Stacked on #27227 (CAMEL-25194)** — both touch `YamlValidator`; merge that one first.
The branches of a pick-one `oneOf` such as `unmarshal` each require their own key, so a file that chose one collects "required property X not found" for every key it did **not** write. The key it *did* write is missing from that list, because its branch got past the required check and failed, if at all, further in. `filterOneOfNoise` then keeps one branch for all the places validated against the same construct, so a second place inherits the first one's winner:
```yaml
- choice:
when:
- simple: "${header.encoding} == 'JSON'"
steps: [ { unmarshal: { fhirJson: { fhirVersion: "{{fhirVersion}}" } } } ]
- simple: "${header.encoding} == 'XML'"
steps: [ { unmarshal: { fhirXml: { fhirVersion: "{{fhirVersion}}" } } } ]
```
```
Line 16: /0/.../when/1/steps/0/unmarshal: required property 'fhirJson' not found
```
The file is correct — `fhirXml` is a data format of `unmarshal` and `fhirVersion` is a placeholder the runtime resolves. `fhir-sink.kamelet.yaml` of camel-kamelets has exactly this shape, which is where it was found.
## The change
The `required` errors of a pick-one construct are dropped where the object at that place has **something in it**: a branch was chosen, so the others only say which keys were not used. An empty object, or a key no branch knows, still gets the construct's own error and the unknown-property error — which are the ones that tell the author what to do.
## Why it is this narrow
The general fix — a winner per place rather than one per construct — also works, and I measured it: it takes the fhir shape and all 250 kamelets to zero. But it costs `YamlValidatorTest.testTypeMismatchFiltersOneOfNoise`, which goes from 3 errors to 6. That assertion *is* the filter's purpose, and it fails because picking a winner per place stops a place borrowing evidence from another place — which is sometimes exactly what produces the clean report. Making that work wants the ranking improved first, so it is recorded on the issue as its own piece of work rather than attempted here.
## Verified
- **All 250 out-of-the-box kamelets of camel-kamelets validate clean**, where before `fhir-sink` did not.
- The documentation corpus, the EIP examples and the XML round-trip corpus of this module are unchanged — 180 tests green, including `testTypeMismatchFiltersOneOfNoise`.
- `OneOfAlternativesTest` adds five cases: the two-branch fhir shape; one branch chosen in several places; choosing nothing is still reported; choosing something that is not a data format is still reported; an unrelated typo is untouched.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Bp3538HRBPMQkb5ta9xRaj
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25238: a chosen branch silences the other branches of a pick-one construct [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27231:
URL: https://github.com/apache/camel/pull/27231#issuecomment-5936794010
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25238: a chosen branch silences the other branches of a pick-one construct [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #27231:
URL: https://github.com/apache/camel/pull/27231#discussion_r4158435444
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -1128,19 +1235,81 @@ private Map<String, OneOfGroup> loadOneOfGroups() {
* {@code BeanConstructorDefinition.index}, which is a map key and is not reachable from the schema - see
* CAMEL-24696 before exposing it.
*/
+ /**
Review Comment:
💡 **Orphaned Javadoc.** The large Javadoc block ending on the line above (originally documenting `isRuntimeAcceptedScalar`) is now separated from its method by `removeSatisfiedAlternatives`, `requiredPropertyOf`, and `constructOf`. In Java the second `/**` takes precedence, so the first block becomes dead documentation. Consider moving the original Javadoc down to sit directly above `isRuntimeAcceptedScalar` (line 1295).
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -1128,19 +1235,81 @@ private Map<String, OneOfGroup> loadOneOfGroups() {
* {@code BeanConstructorDefinition.index}, which is a map key and is not reachable from the schema - see
* CAMEL-24696 before exposing it.
*/
+ /**
+ * Drops "required property X not found" where one of the alternatives is in fact there.
+ * <p/>
+ * The branches of a {@code oneOf} each require their own key, and when all of them are reported the file is told it
+ * needs a key it does not: {@code unmarshal: {fhirXml: ...}} in one when branch of a choice was told "required
+ * property 'fhirJson' not found", because the winner of the other when branch was chosen for it as well. Collected
+ * per construct and per place, the required names of the branches are the alternatives; if the object at that place
+ * has any of them, one branch is satisfied and the rest are noise. When none of them is there, the file really does
+ * have to pick one and the errors stay (CAMEL-25238).
+ */
+ private static void removeSatisfiedAlternatives(List<Error> errors) {
+ Map<String, List<Error>> groups = new LinkedHashMap<>();
+ for (Error e : errors) {
+ if (!"required".equals(e.getKeyword())) {
+ continue;
+ }
+ String construct = constructOf(String.valueOf(e.getEvaluationPath()));
+ if (construct != null) {
+ groups.computeIfAbsent(construct + "@" + e.getInstanceLocation(), k -> new ArrayList<>()).add(e);
+ }
+ }
+ for (List<Error> group : groups.values()) {
+ JsonNode instance = group.get(0).getInstanceNode();
+ if (instance == null || !instance.isObject()) {
+ continue;
+ }
+ // the branches of a pick-one construct each require their own key, so one error per key the file did not
+ // write. The key it did write is the one missing from them: that branch got past its required check and
+ // failed, if at all, further in. So a non-empty object means a branch was chosen and the rest are noise.
+ // An object with nothing in it, or with a key no branch knows, still gets the construct's own error and
+ // the additionalProperties error, which is what actually tells the author what to do.
+ if (!instance.isEmpty() && group.size() > 1) {
+ errors.removeAll(group);
+ }
+ }
+ }
+
+ private static final Pattern REQUIRED_PROPERTY = Pattern.compile("required property '([^']+)' not found");
+
+ /** The property name a "required property 'X' not found" error names, or null. */
+ private static String requiredPropertyOf(Error error) {
+ Matcher m = REQUIRED_PROPERTY.matcher(String.valueOf(error.getMessage()));
+ return m.find() ? m.group(1) : null;
+ }
Review Comment:
💡 **Dead code.** `REQUIRED_PROPERTY` and `requiredPropertyOf` are declared but never called. If they're scaffolding for the per-place-winner improvement mentioned in the PR description, a `// TODO CAMEL-25238` would make that intent clear. Otherwise they can be removed.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25238: a chosen branch silences the other branches of a pick-one construct [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #27231:
URL: https://github.com/apache/camel/pull/27231
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25238: a chosen branch silences the other branches of a pick-one construct [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27231:
URL: https://github.com/apache/camel/pull/27231#issuecomment-5937228948
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `dsl/camel-yaml-dsl/camel-yaml-dsl-validator`
:repeat: **2 tests passed only after a retry on JDK 25** (2 retried attempts)
<details><summary>Recovered flaky tests on JDK 25 (2)</summary>
| Module | Test | Failed attempts | First failure |
| --- | --- | --- | --- |
| `dsl/camel-jbang/camel-jbang-plugin-tui` | `AiPanelTest.modelCommandPersistsSelectionAcrossRestarts(Path)` | 1 | the /model selection must be persisted so it survives a TUI restart ==> expected: <model-b> but was: <null> |
| `dsl/camel-jbang/camel-jbang-plugin-tui` | `TuiShellHistorySupportTest.positiveLimitUsesCamelHistoryFile(Path)` | 1 | Expecting value to be true but was false |
> :information_source: These tests did **not** fail the build. Surefire retried them and they passed.
> Retries are enabled project-wide by `surefire.rerunFailingTestsCount` in `parent/pom.xml`.
</details>
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 8 of 697 tested, 9 compile-only — current: 8 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **8 affected modules** (current approach: 8).
**Skip-tests mode would test 8 modules** (1 direct + 7 downstream), **skip tests for 9** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (8)</summary>
- `camel-jbang-mcp` ← downstream of org.apache.camel:camel-yaml-dsl-validator
- `camel-jbang-plugin-mcp` ← downstream of org.apache.camel:camel-jbang-core
- `camel-jbang-plugin-route-parser` ← downstream of org.apache.camel:camel-jbang-core
- `camel-jbang-plugin-tui` ← downstream of org.apache.camel:camel-yaml-dsl-validator
- `camel-jbang-plugin-validate` ← downstream of org.apache.camel:camel-yaml-dsl-validator
- `camel-launcher-container` ← downstream of org.apache.camel:camel-launcher
- `camel-yaml-dsl-validator` ← dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java, dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/test/java/org/apache/camel/dsl/yaml/validator/KameletValidationTest.java, dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/test/java/org/apache/camel/dsl/yaml/validator/OneOfAlternativesTest.java
- `camel-yaml-dsl-validator-maven-plugin` ← downstream of org.apache.camel:camel-yaml-dsl-validator
</details>
<details><summary>Modules with tests skipped (9)</summary>
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-launcher`
- `coverage`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
<details><summary>All tested modules (17 modules, 3m 55s total)</summary>
**Total reactor time:** 3m 55s
| Module | Duration | Status |
| --- | --- | --- |
| Camel :: JBang :: Plugin :: TUI | 58.7s | SUCCESS |
| Camel :: Launcher | 51.8s | SUCCESS |
| Camel :: JBang :: MCP | 43.2s | SUCCESS |
| Camel :: JBang :: Plugin :: Kubernetes | 22.6s | SUCCESS |
| Camel :: YAML DSL :: Validator | 20.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Testing | 10.9s | SUCCESS |
| Camel :: JBang :: Plugin :: Validate | 9.3s | SUCCESS |
| Camel :: Coverage | 4.9s | SUCCESS |
| Camel :: YAML DSL :: Validator Maven Plugin | 3.5s | SUCCESS |
| Camel :: JBang :: Plugin :: Generate | 2.3s | SUCCESS |
| Camel :: JBang :: Plugin :: MCP | 1.5s | SUCCESS |
| Camel :: JBang :: Plugin :: Edit | 1.4s | SUCCESS |
| Camel :: JBang :: Integration tests | 1.3s | SUCCESS |
| Camel :: JBang :: Plugin :: Route Parser | 1.1s | SUCCESS |
| Camel :: JBang :: Main | 0.9s | SUCCESS |
| Camel :: Launcher :: Container | 0.8s | SUCCESS |
| Camel :: JBang :: Core | n/a | |
**Top 20 slowest modules:**
- `Camel :: JBang :: Plugin :: TUI` (58.7s)
- `Camel :: Launcher` (51.8s)
- `Camel :: JBang :: MCP` (43.2s)
- `Camel :: JBang :: Plugin :: Kubernetes` (22.6s)
- `Camel :: YAML DSL :: Validator` (20.8s)
- `Camel :: JBang :: Plugin :: Testing` (10.9s)
- `Camel :: JBang :: Plugin :: Validate` (9.3s)
- `Camel :: Coverage` (4.9s)
- `Camel :: YAML DSL :: Validator Maven Plugin` (3.5s)
- `Camel :: JBang :: Plugin :: Generate` (2.3s)
- `Camel :: JBang :: Plugin :: MCP` (1.5s)
- `Camel :: JBang :: Plugin :: Edit` (1.4s)
- `Camel :: JBang :: Integration tests` (1.3s)
- `Camel :: JBang :: Plugin :: Route Parser` (1.1s)
- `Camel :: JBang :: Main` (0.9s)
- `Camel :: Launcher :: Container` (0.8s)
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/36899147970)
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-25238: a chosen branch silences the other branches of a pick-one construct [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #27231:
URL: https://github.com/apache/camel/pull/27231#discussion_r4159552732
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -1128,19 +1235,81 @@ private Map<String, OneOfGroup> loadOneOfGroups() {
* {@code BeanConstructorDefinition.index}, which is a map key and is not reachable from the schema - see
* CAMEL-24696 before exposing it.
*/
+ /**
Review Comment:
Fixed in 8036ea5a19a7: the new methods now sit above the original Javadoc, which is back directly on `isRuntimeAcceptedScalar`.
_Claude Code on behalf of davsclaus_
##########
dsl/camel-yaml-dsl/camel-yaml-dsl-validator/src/main/java/org/apache/camel/dsl/yaml/validator/YamlValidator.java:
##########
@@ -1128,19 +1235,81 @@ private Map<String, OneOfGroup> loadOneOfGroups() {
* {@code BeanConstructorDefinition.index}, which is a map key and is not reachable from the schema - see
* CAMEL-24696 before exposing it.
*/
+ /**
+ * Drops "required property X not found" where one of the alternatives is in fact there.
+ * <p/>
+ * The branches of a {@code oneOf} each require their own key, and when all of them are reported the file is told it
+ * needs a key it does not: {@code unmarshal: {fhirXml: ...}} in one when branch of a choice was told "required
+ * property 'fhirJson' not found", because the winner of the other when branch was chosen for it as well. Collected
+ * per construct and per place, the required names of the branches are the alternatives; if the object at that place
+ * has any of them, one branch is satisfied and the rest are noise. When none of them is there, the file really does
+ * have to pick one and the errors stay (CAMEL-25238).
+ */
+ private static void removeSatisfiedAlternatives(List<Error> errors) {
+ Map<String, List<Error>> groups = new LinkedHashMap<>();
+ for (Error e : errors) {
+ if (!"required".equals(e.getKeyword())) {
+ continue;
+ }
+ String construct = constructOf(String.valueOf(e.getEvaluationPath()));
+ if (construct != null) {
+ groups.computeIfAbsent(construct + "@" + e.getInstanceLocation(), k -> new ArrayList<>()).add(e);
+ }
+ }
+ for (List<Error> group : groups.values()) {
+ JsonNode instance = group.get(0).getInstanceNode();
+ if (instance == null || !instance.isObject()) {
+ continue;
+ }
+ // the branches of a pick-one construct each require their own key, so one error per key the file did not
+ // write. The key it did write is the one missing from them: that branch got past its required check and
+ // failed, if at all, further in. So a non-empty object means a branch was chosen and the rest are noise.
+ // An object with nothing in it, or with a key no branch knows, still gets the construct's own error and
+ // the additionalProperties error, which is what actually tells the author what to do.
+ if (!instance.isEmpty() && group.size() > 1) {
+ errors.removeAll(group);
+ }
+ }
+ }
+
+ private static final Pattern REQUIRED_PROPERTY = Pattern.compile("required property '([^']+)' not found");
+
+ /** The property name a "required property 'X' not found" error names, or null. */
+ private static String requiredPropertyOf(Error error) {
+ Matcher m = REQUIRED_PROPERTY.matcher(String.valueOf(error.getMessage()));
+ return m.find() ? m.group(1) : null;
+ }
Review Comment:
Removed in 8036ea5a19a7. They were left over from an approach that compared the required key names; the final check only looks at whether the object is non-empty. The Javadoc of `removeSatisfiedAlternatives` described the old approach too, so it now says what the code does.
_Claude Code on behalf of davsclaus_
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org