CAMEL-24988: Add scalar Switch EIP - #27079
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
There was a problem hiding this comment.
Solid implementation of the Switch EIP. Thread-safe processor (immutable table + AtomicLongArray counters), comprehensive startup validation in SwitchReifier (duplicate detection, mode mixing, dynamic URI rejection), and type-strict composite matching with correct BigDecimal normalization. All DSL round-trips (Java, XML, YAML, JAXB) preserve types and namespaces. Test coverage is thorough — scalar/composite matching, error handling, stream cache rewinding, locale independence, redelivery semantics, nested switches, and JMX management.
No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 584 of 697 tested, 0 compile-only — current: 76 all testedMaveniverse Scalpel detected 584 affected modules (current approach: 76). Skip-tests mode would test 584 modules (20 direct + 48 downstream), skip tests for 0 (generated code, meta-modules)
|
| Module | Duration | Status |
|---|---|---|
| Camel :: Java DSL IO | 19.8s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 19.0s | SUCCESS |
| Camel :: XML IO | 15.5s | SUCCESS |
| Camel :: Maven Plugins :: Camel Maven Package | 14.9s | SUCCESS |
| Camel :: YAML DSL | 13.0s | SUCCESS |
| Camel :: Core Model | 8.9s | SUCCESS |
| Camel :: XML DSL with camel-xml-io | 8.5s | SUCCESS |
| Camel :: YAML DSL :: Validator | 7.7s | SUCCESS |
| Camel :: YAML IO | 7.5s | SUCCESS |
| Camel :: YAML DSL :: Deserializers | 4.2s | SUCCESS |
| Camel :: Core Processor | 4.0s | SUCCESS |
| Camel :: Core Reifier | 2.6s | SUCCESS |
| Camel :: Management API | 2.3s | SUCCESS |
| Camel :: Core Engine | 1.4s | SUCCESS |
| Camel :: YAML DSL :: Maven Plugins | 1.1s | SUCCESS |
| Camel :: XML JAXB | 0.8s | SUCCESS |
| Camel :: Core | n/a | |
| Camel :: JBang :: Core | n/a | |
| Camel :: Management | n/a | |
| Camel :: Spring XML | n/a |
Top 20 slowest modules:
Camel :: Java DSL IO(19.8s)Camel :: Catalog :: Camel Catalog(19.0s)Camel :: XML IO(15.5s)Camel :: Maven Plugins :: Camel Maven Package(14.9s)Camel :: YAML DSL(13.0s)Camel :: Core Model(8.9s)Camel :: XML DSL with camel-xml-io(8.5s)Camel :: YAML DSL :: Validator(7.7s)Camel :: YAML IO(7.5s)Camel :: YAML DSL :: Deserializers(4.2s)Camel :: Core Processor(4.0s)Camel :: Core Reifier(2.6s)Camel :: Management API(2.3s)Camel :: Core Engine(1.4s)Camel :: YAML DSL :: Maven Plugins(1.1s)Camel :: XML JAXB(0.8s)
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after efd4219: otherwise promoted from a plain String attribute to a first-class SwitchOtherwiseDefinition element. All call sites updated consistently (SwitchReifier, ManagedSwitch, JavaDslModelWriterSupport, ModelParser, ModelWriter, YamlModelWriter). Deep-copy via copyDefinition() is correct (test verifies independence). getOtherwiseDefinition() re-reads otherwise.getUri() on each call, so mutating the model element is picked up by endpoint discovery — intentional and tested. New SwitchSchemaTest validates the element-object constraint in both canonical and regular YAML schemas. Solid refactoring, no issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after efd4219: otherwise promoted from a String @XmlAttribute to a proper SwitchOtherwiseDefinition @XmlElement. This is a clean structural improvement:
- New
SwitchOtherwiseDefinitionimplementsEndpointRequiredDefinition, deep-copy safe viacopyDefinition() - Lazy
ToDefinitionsync ingetOtherwiseDefinition()re-reads the URI on every call — correct for mutable source-of-truth pattern, verified by the new endpoint discovery test SwitchReifier,ManagedSwitch,JavaDslModelWriterSupportall updated to.getOtherwise().getUri()- Generated parsers/writers (XML-IO, YAML-IO, YAML deserializer) consistent
- XSD/JSON schemas updated:
otherwisemoves from attribute to child element with requireduriattribute - Docs (AsciiDoc) updated across all three examples (scalar, composite, semantic) — XML uses
<otherwise uri="..."/>, YAML usesotherwise: {uri: ...} - New tests: fallback URI blank rejection, dynamic fallback URI rejection, schema validation, YAML round-trip assertion, deep-copy independence
No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Luigi, the scalar Switch is very well done and closely follows the CAMEL-24988 plan: a decision table with value/uri attributes, case-insensitive Locale.ENGLISH matching, duplicates rejected at startup, doSwitch/doCase, the EndpointRequiredDefinition route for YAML uri+parameters, static-URI enforcement, JMX/tracing ids per case, and good test coverage across all DSLs.
As decided on the JIRA, the first release is scalar only. Please remove the composite form (keys, values, SwitchValueDefinition, typed boolean/number literals), together with the hand-written YAML deserializer, the schema/writer special-cases it needs, and the camel-semantic test and doc additions. If composite comes back later it will be a separate proposal, and strings only (typed literals don't fit XML attributes).
One must-fix: ManagedSwitch.extendedInformation() exposes case and otherwise URIs without masking. Please mask them the same way ManagedSendProcessor does.
Open question: the fallback was planned as an otherwise attribute on <switch>. The element form does allow YAML parameters on the fallback, which is a fair reason to keep it. Let's decide explicitly.
Minor: the @Metadata description and the docs' :description: still say "scalar or composite".
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after df2a08b: all davsclaus CHANGES_REQUESTED findings addressed in this commit.
Composite form fully removed:
SwitchValueDefinitionclass + JSON metadata + JAXB index entry deletedkeysfield,keys()method,doCase(Map),CaseBuilder.value()removed fromSwitchDefinitionvaluesfield removed fromSwitchCaseDefinition;valueattribute nowrequired = true- Hand-written
SwitchValueDefinitionDeserializer+CustomResolverentry +pom.xmlbanned-definition entry all removed GenerateYamlSchemaMojoswitchValue special-case andmodel-yaml-writer.vmoverride removedSwitchProcessor.normalizeLiteral()/compositeKey()removed; table simplified toMap<String, Integer>SwitchReifiercomposite validation branches removed- Entire "Composite selectors" doc section + semantic batch example removed from switch-eip.adoc
SemanticEipTest.batchDecisionsSelectOneSwitchDestination+ Switch row in semantic-language.adoc removed
URI masking in ManagedSwitch (must-fix):
init(ManagementStrategy)reads the mask setting;URISupport.sanitizeUri()applied to both case and fallback URIs- New parameterized test
masksCaseAndFallbackUrisverifiesnull(default=masked),true, andfalse
getSelector() side-effect-free:
preCreateProcessor()removed fromgetSelector(); now called explicitly inSwitchReifier.createProcessor(),LwModelToXMLDumper,LwModelToYAMLDumper,JaxbHelper, andJavaDslModelWriterSupport- New test
fluentSelectorSurvivesDumpBeforeStartupverifies dump before startup works
All descriptions updated: "scalar or composite" → "scalar" across @Metadata, JSON models, XSD schemas, YAML schemas.
Tests updated consistently: composite tests replaced with scalar equivalents, structured-result rejection tests added (Map, List, array→IllegalArgumentException`), error-handling test for map selector result. CI green.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the rework addresses my earlier comments: the composite form is gone, JMX masks the case/otherwise URIs, getSelector() is a plain getter, the docs say scalar only, and Choice is untouched.
- Merge conflict in
core/camel-yaml-io/.../YamlPrinter.javaagainst CAMEL-25079 (#54836b9), so CI has not run on this head. Main's YAML 1.2NUMBER_PATTERNalready quotes leading-zero numbers like007, so drop the PR's regex change, take main's, and keep the newneedsQuoting("001")test. - The open question about
otherwisestaying an element (SwitchDefinition.java:51) still needs an answer. Keeping it as an element is fine by me if the reason is YAMLuri+parameters; please reply on the thread. - Minor:
getOtherwiseDefinition()still creates and mutates the transientToDefinitioninside a getter, the same pattern I objected to forgetSelector().
Claude Code on behalf of davsclaus
|
A few doc/discoverability additions so users find Switch next to Choice:
Claude Code on behalf of davsclaus |
|
Addressed the remaining points from this review in 7147494eb882:
Validation passed: 341 focused/exporter tests, 1,085 Java 17 catalog tests against the final regenerated catalog, and the full 695-module clean build with Comment written by Codex on behalf of @luigidemasi. |
|
Following up on the Choice ↔ Switch cross-linking: please add a
TIP: Switch has no inlined sub-routes: each case sends to a single endpoint. This fosters a
design where each branch is its own route, linked via xref:ROOT:direct-component.adoc[Direct]
or xref:ROOT:seda-component.adoc[SEDA], keeping the dispatch table flat and each branch
independently readable and testable. If you need predicates, ranges, or inlined processing
steps in the branches, use the xref:choice-eip.adoc[Choice] EIP instead.
TIP: If you route on the literal value of a single expression (a decision table), consider the
xref:switch-eip.adoc[Switch] EIP. Unlike Choice, Switch does not have inlined sub-routes; each
case sends to one endpoint, typically another route linked via
xref:ROOT:direct-component.adoc[Direct] or xref:ROOT:seda-component.adoc[SEDA].(Remember to regenerate the catalog doc mirrors afterwards.) Claude Code on behalf of davsclaus |
|
Two more areas before this is complete: round-trip test coverage across all the DSLs, and making the new EIP discoverable by AI tooling. Tests: DSL parsers and round tripAlready covered: core
AI discoverabilityThe generated
Claude Code on behalf of davsclaus |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 7147494: all three items from the previous davsclaus review are addressed.
1. Merge conflict with CAMEL-25079 (YAML quoting) — resolved. Main's NUMBER_PATTERN is kept; the needsQuoting("001") regression test is retained in YamlPrinterTest.
2. otherwise element question — confirmed on the thread. SwitchOtherwiseDefinition stays as a first-class @XmlElement so YAML uri plus parameters works on the fallback, matching the case endpoint pattern. A case literal named "otherwise" remains an ordinary literal.
3. getOtherwiseDefinition() / getToDefinition() side-effect-free — fixed. SwitchCaseDefinition.getToDefinition() now returns null until prepareToDefinition() is explicitly called by the reifier. SwitchOtherwiseDefinition.getToDefinition() returns a pre-allocated field that is initialized via setUri() or afterUnmarshal(), with no lazy mutation in the getter. SwitchDefinition.getOtherwiseDefinition() delegates through cleanly. New SwitchDefinitionTest.readingDestinationsDoesNotOverwritePreparedNodes() confirms the prepared-node stability. copiedFallbackHasIndependentDestinationAndParent() confirms deep-copy independence.
Code is solid. The SwitchReifier.createProcessor() flow is now explicit: validate → idOrCreate → prepareToDefinition/prepareOtherwiseDefinition → createSend. No hidden side effects in getters.
Note: davsclaus posted two follow-up doc/discoverability requests (EIP index row, AI patterns page, Choice↔Switch cross-linking, TIP blocks, @Metadata aliases) after this commit. Those are not yet addressed and will need a follow-up commit.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
To keep this PR moving: the doc additions (EIP index, AI patterns page, the TIPs on the Choice/Switch pages), the extra round-trip tests (xml-io, yaml-io, spring-xml, yaml-dsl) and the AI discoverability items (aliases, Claude Code on behalf of davsclaus |
davsclaus
left a comment
There was a problem hiding this comment.
LGTM, thanks @luigidemasi. The merge conflict is resolved, otherwise stays an element (for YAML uri + parameters), the getters no longer mutate the model, and selector namespaces survive the XML/YAML/JAXB dumps. Approving; please merge once CI is green.
Please follow up with a second PR right after this one (and open a JIRA for it so it is not forgotten), covering the items from the earlier comments:
- Docs (comment, TIP):
- A Switch row next to Choice in the EIP index.
- Switch on the AI patterns page (Router / Dispatch, plus a classify/intent-routing row with camel-semantic).
- TIP blocks on both the Choice and Switch pages: Switch has no inlined sub-routes and fosters branches as separate routes linked via
direct:/seda:.
- Round-trip tests (comment):
- camel-xml-io
switch.xmlfixture. - camel-yaml-io
yaml-route-switch.yaml+XmlToYamlTest/YamlPrinterRoundTripTest. - camel-spring-xml
SpringSwitchTest+ XML. - yaml-dsl loading from a
.camel.yamlfile.
- camel-xml-io
- AI discoverability (same comment):
aliasesonSwitchDefinition.CatalogSamplesINTENTS/PART_OF entries.switchin the MCP prompt/ToolRegistryEIP lists.- A first doc sample showing one
direct:route per case.
Claude Code on behalf of davsclaus
|
Thanks, Claus. I opened CAMEL-25178 to track the documentation, AI discoverability, and DSL round-trip follow-up. The semantic Switch example will use a single reference with a scalar result; refs:department,urgent stays in the batched semantic results documentation.\n\n_Codex on behalf of @luigidemasi_ |
Evaluate a Camel expression once and select one static endpoint using a literal lookup table. Support named, typed composite keys, including semantic batch results, without coupling core routing to a language. Add Java, XML and YAML DSL support, model round trips, case instrumentation, startup validation, documentation and generated metadata. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
Show header, map-header and semantic batch routes in Java, XML and YAML, including question registration, example inputs and expected destinations. Provide logging routes for trying the examples and regenerate the catalog documentation and JBang samples. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
Use the XML IO namespace for the standalone routes document so the catalog documentation schema checks validate it. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
Use an otherwise child with a required uri in XML and an otherwise URI object in YAML. Preserve the Java fluent API and literal matching rules, and regenerate DSL metadata, schemas, and documentation samples. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
Remove composite keys, typed values, and their DSL and semantic additions. Preserve the structured otherwise endpoint and existing fallback rules. Reject structured selector results through normal Camel error handling, honor management URI masking, and prepare fluent selectors explicitly during reification and export instead of mutating them in the getter. Preserve literal strings when exporting YAML, including leading zeros, and regenerate the model metadata, schemas, writers, and documentation. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
7147494 to
e590a5a
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after force-push (commit restructuring). The five commits are a clean squash of the previously reviewed progression.
Key delta from the last reviewed SHA (7147494): the explicit prepareToDefinition() / prepareOtherwiseDefinition() pattern was replaced with lazy initialization inside getToDefinition() and getOtherwiseDefinition(). This removes SwitchDefinitionTest.java (which tested the explicit-prepare contract) and drops the prepareToDefinition / prepareOtherwiseDefinition calls from SwitchReifier. The lazy-init approach is simpler and functionally equivalent — the reifier immediately overwrites the URI with the resolved value anyway, so the getter's sync is a no-op in that path. ManagedSwitch.extendedInformation() reads c.getUri() / c.getId() directly, so it's unaffected.
Note: getOtherwiseDefinition() and getToDefinition() now set URI/ID/metadata on every call (side-effectful getters). davsclaus's previous review asked for side-effect-free getters, and the 7147494 commit addressed that. This force-push reverts to lazy mutation. Given davsclaus's final APPROVE was based on the side-effect-free version, he may want to confirm this simplification is acceptable — but the code is functionally correct either way.
No new issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Keep the model getters side-effect-free and prepare endpoint nodes during reification. Restore regression coverage for stable prepared nodes and independent copies. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
|
Thanks for catching this, Gnodet. The rebase had dropped the side-effect-free getter changes that were carried in the original merge commit. Commit d5deafb restores explicit destination preparation during reification, read-only getters, and the SwitchDefinitionTest regression coverage. The focused model tests pass; CI is rerunning on this corrected head.\n\n_Codex on behalf of @luigidemasi_ |
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after d5deafb: this commit restores the explicit prepareToDefinition() / prepareOtherwiseDefinition() pattern that davsclaus requested. The lazy-mutation getters from the previous force-push are gone.
SwitchCaseDefinition.getToDefinition()is now a plain field getter; mutation is inprepareToDefinition()SwitchOtherwiseDefinitionpre-allocatestoDefinitionas afinalfield, syncs URI insetUri(), and usesafterUnmarshal()for JAXBSwitchDefinition.getOtherwiseDefinition()delegates tootherwise.getToDefinition()— no side effectsSwitchReifier.createProcessor()callsprepareToDefinition()/prepareOtherwiseDefinition()explicitly before getting the nodes- New
SwitchDefinitionTestverifies prepared-node stability and deep-copy independence
This aligns with the 7147494 version that davsclaus approved. No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after d5deafb: explicit prepareToDefinition() / prepareOtherwiseDefinition() pattern restored, addressing davsclaus's feedback about side-effectful getters.
getToDefinition()andgetOtherwiseDefinition()are now pure getters — no state mutation- Preparation is explicit:
SwitchReifier.createProcessor()callsprepareToDefinition()/prepareOtherwiseDefinition()before reading the nodes SwitchOtherwiseDefinition.toDefinitionisfinal— initialized at construction, synced viasetUri()andafterUnmarshal()setOtherwise(),setId(),setGeneratedId()propagate preparation to keep the otherwise ID in sync with the switch ID- Copy constructor routes through
setOtherwise()to trigger preparation on the copy - Tests verify: prepared-node stability across
getChildren()calls, and deep-copy independence
No issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Prepare literal destination nodes before JMX performance-counter traversal. This keeps model getters side-effect-free while making each Switch destination available to management instrumentation. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Luigi De Masi <ldemasi@redhat.com>
|
Follow-up: the first correction exposed one more required call site. JMX performance-counter traversal also prepares the case and fallback destinations before reading them. Commit 449a6a9 includes that fix; all four ManagedSwitchTest cases pass locally. CI is rerunning on this head. Codex on behalf of @luigidemasi |
Description
Add a scalar Switch EIP that evaluates a selector once per entry and maps its value to one fixed endpoint. Cases are literal strings matched case-insensitively with
Locale.ENGLISH; duplicate values are rejected at startup. Null or unmatched results use the optional fallback, while selector failures follow normal Camel error handling. Destinations support property placeholders and reject Simple expressions.Java, XML and YAML DSLs, endpoint discovery, tracing/JMX, documentation, and generated metadata are included. The selector can use any Camel expression language. Map, collection and array results fail explicitly through Camel error handling. Composite matching is outside this PR's scope, and Choice is unchanged.
The fallback is an explicit endpoint element: XML uses
<otherwise uri="direct:review"/>, YAML usesotherwise: {uri: direct:review}, and Java uses.otherwise("direct:review"). Keeping the element lets both cases and the fallback use standard YAMLuriplusparameters. A case literal namedotherwiseremains an ordinary literal.The documentation example includes equivalent Java, XML and YAML routes, sample inputs, expected destinations, and destination routes.
Validation
Before rebasing onto the latest main, 341 focused/exporter tests passed across the Switch model, routing, management, JAXB, XML/YAML DSLs, schemas, and the complete Java IO and YAML IO module suites. The 1,085 catalog tests passed on Java 17, including XML documentation schema validation. The full 695-module root build passed with tests skipped; focused tests were run separately.
The branch is rebased onto upstream/main at
c3632125; all seven PR commits retain their GPG signatures and Signed-off-by trailers. The rebase range-diff confirms the original patches are preserved, with main's YAML number-pattern implementation retained. Signed follow-up commits restore side-effect-free destination getters, explicitly prepare destinations during reification and before JMX performance-counter traversal, and include regression coverage for both paths. CI is running against the corrected head.Target
apache/camel:mainand incorporates current main.Apache Camel coding standards and style
git diff --checkpassed.AI-assisted contributions
Generated by Codex on behalf of @luigidemasi.