CAMEL-22894: Refactor SimpleFunctionExpression: introduce function-factory dispatcher (part 1)#23263
Conversation
The createCode(...) method exists solely to support csimple, which is deprecated since 4.19 and slated for removal in 5.0. Mark the method @deprecated(since = "4.21") and make it a default returning null so implementations that no longer care about csimple don't need to override it. The three in-tree implementations (camel-attachments, camel-base64, camel-jsoup) keep their existing overrides and pick up the deprecation marker via inheritance. This is the first step toward refactoring the dual-responsibility SimpleFunctionExpression class. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…eFunctionDispatcher SimpleFunctionExpression had six near-identical methods plus six gating predicate blocks dedicated to dispatching function strings to the three SimpleLanguageFunctionFactory implementations shipped by external components (camel-attachments, camel-base64, camel-jsoup). Move the dispatch into a new SimpleFunctionDispatcher, which owns the list of known component factories and their gating predicates and exposes tryCreate(...) / tryCreateCode(...) entry points. Behavior preserved exactly: - Same factories consulted in the same order. - Same gating predicates (so a function string belonging to a missing component still surfaces the helpful "add this JAR" error via ResolverHelper.resolveMandatoryBootstrapService). - camel-jsoup remains excluded from the csimple createCode dispatch (it has no csimple support and would throw). Net: SimpleFunctionExpression shrinks by ~80 lines and adding a fourth external factory is now a one-line edit in SimpleFunctionDispatcher instead of a six-block change spread across SimpleFunctionExpression. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…unctionFactory classes Move four self-contained Simple functions out of SimpleFunctionExpression into their own SimpleLanguageFunctionFactory implementations under org.apache.camel.language.simple.functions, registered as built-ins on SimpleFunctionDispatcher. The dispatcher consults built-ins before external component factories, preserving today's priority. Both createFunction (Simple) and createCode (csimple) are moved together into each class, keeping the runtime and code-generation logic for the same function side by side instead of ~2,000 lines apart in SimpleFunctionExpression. Behavior preserved: each factory short-circuits on its function prefix and returns null on non-match, exactly mirroring the inline blocks they replace. All Simple and CSimple tests continue to pass. SimpleFunctionExpression shrinks by ~147 lines. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
SimpleFunctionExpression: introduce function-factory dispatcher (part 1)
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
Build reactor — dependencies compiled but only changed modules were tested (3 modules)
|
|
We are not keeping csimple until camel 5 in so many years. It will be removed sooner. Its not really in use by end users. And users who use it can easily use regular simple |
davsclaus
left a comment
There was a problem hiding this comment.
Review — CAMEL-22894: Refactor SimpleFunctionExpression: introduce function-factory dispatcher (part 1)
Well-structured refactoring PR with clear scope boundaries. The three-commit structure is easy to follow, and the PR description is exemplary. CI is green and no blocking issues found.
Observations
1. (Low) Evaluation order change for random, skip, collate, join
In the original code these four functions are evaluated inside createSimpleExpressionMisc() which runs before createSimpleExpressionMath(). After this PR they are evaluated via the dispatcher after math. Since these function names (random(, skip(, collate(, join() have no overlap with math function patterns, this is harmless in practice. But the PR description states "Behaviour preserved exactly" — this is a safe ordering change rather than exact preservation, and worth acknowledging.
2. (Low) JoinFunctionFactory couples back to SimpleFunctionExpression.codeSplitSafe()
For the follow-up PRs, codeSplitSafe should move to a shared utility (e.g. SimpleParserHelper) as more factories are extracted. Not blocking for this PR.
3. (Low) No dedicated unit tests for individual factory classes
The existing SimpleTest (227 tests), CSimpleTest, etc. cover behavioral correctness. As more factories are extracted in follow-up PRs, per-factory unit tests would help catch regressions faster.
Verified Clean
SkipFunctionFactorycorrectly usesStringHelper.before()increateFunctionandStringHelper.beforeLast()increateCode, matching the original.- The
@DeprecateddefaultcreateCodereturningnullis safe — existing overrides in camel-attachments/base64/jsoup continue working. @Deprecated(since = "4.21")withoutforRemovalis consistent with existing csimple deprecation convention.- CI green, formatting clean.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
Good first step toward decomposing the 4,500-line SimpleFunctionExpression. The commit structure is clean — deprecation, dispatcher, extraction are each independently reviewable. Two issues to address before merge (see inline comments), plus a few suggestions.
Summary of findings:
-
(blocking)
SkipFunctionFactory:beforevsbeforeLastinconsistency —createFunctionusesStringHelper.before()whilecreateCodeusesStringHelper.beforeLast(). This is a pre-existing bug faithfully ported from the original code, but since we're touching this code now, we should fix it. -
(blocking) Ordering change for extracted built-ins —
random,skip,collate,joinpreviously evaluated increateSimpleExpressionMisc()(beforecreateSimpleExpressionMath()). Now they evaluate via the dispatcher (after math). Safe in practice since these prefixes don't collide with math patterns, but the PR description claims "behaviour preserved exactly" — this should be acknowledged or addressed. -
(major) Circular dependency —
JoinFunctionFactorycallsSimpleFunctionExpression.codeSplitSafe(...), creating a dependency from the extracted factory back into the monolith being decomposed. -
(minor) Javadoc/comment mismatches — The dispatcher Javadoc and inline comments don't reflect that it now handles both built-in and external functions.
|
The first three commits were intentionally scoped to prove the extraction/dispatcher pattern and get reviewer feedback on the shape before applying it more broadly. Based on that review, this branch now also:
|
- restore built-in function dispatch ordering relative to misc/math parsing - move codeSplitSafe into SimpleFunctionHelper to decouple JoinFunctionFactory from SimpleFunctionExpression - align SkipFunctionFactory runtime parsing with createCode using beforeLast - move random/skip/collate/join coverage into dedicated factory unit tests rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
`AbstractSimpleFunctionFactoryTestSupport.evaluate()` previously called
the factory directly, bypassing `SimpleFunctionExpression` and the
dispatcher entirely. The four factory tests therefore did not exercise
the dispatch ordering (built-ins before misc/math) that the review
flagged.
Route `evaluate()` through `context.resolveLanguage("simple")` so each
factory test exercises the full tokenizer -> parser -> dispatcher ->
factory chain. `createCode()` continues to call the factory directly
since there is no equivalent full pipeline for csimple.
The two error-message assertions in `RandomFunctionFactoryTest` are
updated to check `getCause().getMessage()`: the full pipeline wraps
the factory's `SimpleParserException` inside a
`SimpleIllegalSyntaxException` that appends location context.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
gnodet
left a comment
There was a problem hiding this comment.
All review feedback has been addressed:
SkipFunctionFactorybefore/beforeLastinconsistency fixed- Evaluation ordering restored (built-ins dispatched before misc/math)
- Circular dependency removed (
codeSplitSafemoved toSimpleFunctionHelper) - Javadoc and comments updated to reflect both built-in and external dispatch
record Entryreplaced with conventionalstatic final class- Dedicated per-factory unit tests added, routed through the full language pipeline
Claude Code on behalf of Guillaume Nodet
|
thanks part 2 is welcome |
Description
First incremental step toward CAMEL-22894: break up the 4,500-line
SimpleFunctionExpressionthat today carries both runtime Simple language and compile-time CSimple code-generation responsibilities, with no behavior change.This PR establishes the pattern and ports the first batch of small built-in families. Larger families (
header,exchangeProperty, the rest) will follow in subsequent PRs once the dispatcher pattern lands.Three commits, each independently reviewable:
fc43e0d— DeprecateSimpleLanguageFunctionFactory#createCode. ThecreateCode(...)method on the publicSimpleLanguageFunctionFactorySPI (@since 4.10) exists solely to support csimple, which is already@Deprecated(since = \"4.19\")and slated for removal in 5.0. Marked the method@Deprecated(since = \"4.21\")and made it adefaultreturningnull, so implementations that don't care about csimple no longer have to override it. The three in-tree implementations (camel-attachments,camel-base64,camel-jsoup) keep their existing overrides and pick up the deprecation marker via inheritance.4beafc9— Centralize external-component function dispatch inSimpleFunctionDispatcher.SimpleFunctionExpressionpreviously had six near-identical methods plus six gating predicate blocks dedicated to dispatching to the three externalSimpleLanguageFunctionFactoryimplementations (camel-attachments,camel-base64,camel-jsoup). Moved the dispatch into a new internalSimpleFunctionDispatcherthat owns the list of known component factories and their gating predicates, exposingtryCreate(...)/tryCreateCode(...)entry points. Behaviour preserved exactly: same factories, same order, same gating (so a missing component still surfaces the helpful "add this JAR" error viaResolverHelper.resolveMandatoryBootstrapService), andcamel-jsoupremains excluded from the csimplecreateCodepath because itscreateCodethrowsUnsupportedOperationException.63d326e— Extractrandom,skip,collate,joinintoSimpleLanguageFunctionFactoryclasses. Moved four self-contained Simple functions out ofSimpleFunctionExpressioninto their own factory implementations underorg.apache.camel.language.simple.functions, registered as built-ins onSimpleFunctionDispatcher. The dispatcher consults built-ins before external component factories, preserving today's priority. BothcreateFunction(Simple) andcreateCode(csimple) live side by side in each class, instead of being ~2,000 lines apart inSimpleFunctionExpression.Net structural impact so far:
SimpleFunctionExpression.java: 4,526 → ~4,255 lines (−270 LOC), with the new dispatcher (~150 LOC) and four small factory classes (~300 LOC combined).SimpleFunctionDispatcherinstead of a six-block change spread acrossSimpleFunctionExpression.Out of scope for this PR (and the broader refactor, see plan locked in JIRA discussion):
addFunctionFactory(...)onSimpleFunctionRegistry).Verification:
mvnd clean install -Dquickly -Dci.env.name=localfrom repo root: ✅ BUILD SUCCESS.SimpleTest(227),CSimpleTest(1),SimpleCustomFunctionTest(5),CSimplePredicateParserTest(4),CSimpleExpressionParserTest(13),SimpleExpressionParserNodesTest(2),SimplePredicateParserNodesTest(3): all green.camel-attachmentsSimpleAttachmentTest(10),camel-base64SimpleBase64Test(2), fullcamel-jsouptest suite (3): all green.mvn formatter:format impsort:sortclean.Review follow-up after the initial proof-of-concept commits:
The first three commits were intentionally scoped to prove the extraction/dispatcher pattern and get reviewer feedback on the shape before applying it more broadly. Based on that review, this branch now also:
codeSplitSafe(...)intoSimpleFunctionHelperskip(...)runtime/code inconsistencyTarget
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.Claude Code on behalf of Adriano Machado