CAMEL-22894: Extract direct and misc function factories from SimpleFunctionExpression#23445
CAMEL-22894: Extract direct and misc function factories from SimpleFunctionExpression#23445ammachado wants to merge 4 commits into
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:
Build reactor — dependencies compiled but only changed modules were tested (2 modules)
|
|
[05:04:22.454] WARN (asciidoctor): skipping reference to missing attribute: body |
gnodet
left a comment
There was a problem hiding this comment.
Note: This diff exceeds 2000 lines. This review focuses on high-risk areas (core API, expression evaluation logic, test coverage).
Summary
This is a clean, well-structured continuation of the CAMEL-22894 refactoring. The extraction of DirectFunctionFactory and MiscFunctionFactory from SimpleFunctionExpression is mechanically correct -- I verified each function in both createFunction and createCode maps faithfully to the original logic.
What looks good
- No behavioral changes in the refactored code. The if-else chains, token-length checks, and error messages are preserved verbatim.
not()code-gen correctly kept inline inSimpleFunctionExpressionbecause it depends onskipFileFunctionsfrom the parser context. Good call.routeGrouphandled correctly: it was only in the expression path (not increateCode) in the original, and the PR preserves this.iifvalidation difference preserved:createFunctionusestokens.length > 3whilecreateCodeusestokens.length != 3-- both matching the original.- Good test coverage: 28 tests for
DirectFunctionFactory(expression evaluation + code generation) and 35 tests forMiscFunctionFactory. - Documentation fixes are all correct -- the typos (data->date, avg->average, ceil->floor, JSon->JSON, missing parens, wrong operators) are genuine fixes.
Issue: AsciiDoc build failure
The docs build is failing because of unescaped {body} attribute references in the pad function example. As @davsclaus pointed out, AsciiDoc interprets {body} as an attribute reference even inside backticks. The existing sum example on the line just above uses the proper $\{body} escape -- the pad example needs the same treatment. See the inline comment below.
Claude Code on behalf of Guillaume Nodet
| If width is a positive number, then the string is padded to the right; if negative, it is padded to the left. | ||
| The optional separator specifies the padding character(s) to use. If not specified, it defaults to the space character. | ||
| If the message body contains `foo` then `${pad(5)}` return `"foo "` and `${pad(-5)}` returns `" foo"`, and `${pad(-5,'@')}` returns `@@foo`. | ||
| If the message body contains `foo` then `${pad(${body},5)}` returns `"foo "` and `${pad(${body},-5)}` returns `" foo"`, and `${pad(${body},-5,'@')}` returns `@@foo`. |
There was a problem hiding this comment.
This line introduces unescaped ${body} references that AsciiDoc interprets as attribute lookups, causing the build failure @davsclaus flagged. The sum example just above (line 472) uses $\{body} to escape the braces -- the same pattern is needed here:
| If the message body contains `foo` then `${pad(${body},5)}` returns `"foo "` and `${pad(${body},-5)}` returns `" foo"`, and `${pad(${body},-5,'@')}` returns `@@foo`. | |
| If the message body contains `foo` then `${pad($\{body},5)}` returns `"foo "` and `${pad($\{body},-5)}` returns `" foo"`, and `${pad($\{body},-5,'@')}` returns `@@foo`. |
Claude Code on behalf of Guillaume Nodet
Add DirectFunctionFactory for exact-match keyword functions (id, exchangeId, exception, null, etc.) that must be dispatched before OGNL prefix checks, and MiscFunctionFactory for the remaining utility functions (isEmpty, isNumeric, not, convertTo, uuid, hash, iif, load, etc.). SimpleFunctionExpression shrinks from ~1700 to ~1000 lines. The not() CSimple code-generation path is kept inline because it requires the skipFileFunctions flag from the enclosing parser context. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
28 tests for DirectFunctionFactory (all keyword functions + createCode) 35 tests for MiscFunctionFactory (isEmpty, isAlpha, isNumeric, not, convertTo, uuid, hash, empty, iif, load + createCode variants). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- sysenv.key: corrected description from "JVM system property" to "OS
environment variable" (table entry and prose)
- date:command:pattern table: fix typo "data:command" -> "date:command"
- isAlpha examples: add missing closing parentheses
- average prose: fix wrong function name avg() -> average()
- floor prose: replace ceil() examples with floor() examples
- sum prose: fix missing opening brace $sum -> ${sum}
- toPrettyJson: replace TODO placeholder with actual pretty-printed output
- XPath section: replace copy-pasted jsonpath() examples with xpath() ones
- lowercase/uppercase examples: replace backtick with closing single quote
- pad examples: replace invalid single-arg form with explicit two-arg form
- Chain operator: fix -> to ~> in multi-chain example
- AND/OR examples: remove spurious single quote in ${header.type'}
- Init block: add missing closing ) in uppercase() call
- @BindToRegistry: close unclosed string literal
- Normalize "JSon" -> "JSON" throughout the document
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.
The AsciiDoc escaping issue is fixed -- pad() examples now use $\{body} correctly (commit d0d8372).
One minor observation on the docs/local-build.sh change: using "$*" joins all positional parameters into a single string, which changes behavior compared to the original $*. If the intent is to pass through multiple arguments, "$@" would be the standard choice. Not a blocker though; the script is rarely used and the rest of the PR is solid.
The overall refactoring and doc fixes look good. LGTM.
Claude Code on behalf of Guillaume Nodet
Escape {body} as $\{body} in the pad function prose to prevent
AsciiDoc from interpreting it as an attribute reference, matching
the pattern already used in the sum() example on the preceding line.
Also fix docs/local-build.sh: derive LOCAL from the actual checkout
directory name instead of the hardcoded "camel", so the script works
with worktrees and forks checked out under a different name. Also fix
$CW not expanding in the error message (was in single quotes) and
quote all variables to handle paths with spaces.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Adriano Machado <60320+ammachado@users.noreply.github.com>
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Description
Continues the refactoring started in CAMEL-22894 by extracting two more
groups of functions out of
SimpleFunctionExpressioninto dedicated factory classes.Refactoring
DirectFunctionFactoryhandles exact-match keyword functions that must be dispatched before OGNL prefix checks (id,exchangeId,exception,null,body,originalBody, etc.).MiscFunctionFactoryhandles the remaining utility functions (isEmpty,isNumeric,isAlpha,not,convertTo,uuid,hash,iif,load, etc.).SimpleFunctionExpressionshrinks from ~1700 to ~1000 lines. Thenot()CSimple code-generation path is kept inline because it requires theskipFileFunctionsflag from the enclosing parser context.Tests
DirectFunctionFactory(all keyword functions +createCodepaths)MiscFunctionFactory(isEmpty,isAlpha,isNumeric,not,convertTo,uuid,hash,empty,iif,load+createCodevariants)Documentation fixes
Fix 15 errors found during an audit of
simple-language.adoc:sysenv.keytable entry: corrected "JVM system property" to "OS environment variable"data:command->date:command(two rows)isAlphaexamples: add missing closing parenthesesaverageprose: fix wrong function name${avg()}->${average()}floorprose: replace copy-pasted${ceil(...)}examples with${floor(...)}sumprose: fix missing opening brace$sum(...)->${sum(...)}toPrettyJson: replace literalTODO:placeholder with actual output examplejsonpath()examples withxpath()expressionslowercase/uppercaseexamples: replace backtick with closing single quotepadexamples: replace invalid single-argument form with correct two-argument form->to~>on the last link'inside${header.type'})in${uppercase('Hello ${body}')}@BindToRegistrytip: close unclosed string literalJSon->JSONthroughout the documentTarget
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