Skip to content

CAMEL-22894: Extract direct and misc function factories from SimpleFunctionExpression#23445

Open
ammachado wants to merge 4 commits into
apache:mainfrom
ammachado:CAMEL-22894
Open

CAMEL-22894: Extract direct and misc function factories from SimpleFunctionExpression#23445
ammachado wants to merge 4 commits into
apache:mainfrom
ammachado:CAMEL-22894

Conversation

@ammachado
Copy link
Copy Markdown
Contributor

@ammachado ammachado commented May 22, 2026

Description

Continues the refactoring started in CAMEL-22894 by extracting two more
groups of functions out of SimpleFunctionExpression into dedicated factory classes.

Refactoring

DirectFunctionFactory handles exact-match keyword functions that must be dispatched before OGNL prefix checks (id, exchangeId, exception, null, body, originalBody, etc.).

MiscFunctionFactory handles the remaining utility functions (isEmpty, isNumeric, isAlpha, 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.

Tests

  • 28 unit tests for DirectFunctionFactory (all keyword functions + createCode paths)
  • 35 unit tests for MiscFunctionFactory (isEmpty, isAlpha, isNumeric, not, convertTo, uuid, hash, empty, iif, load + createCode variants)

Documentation fixes

Fix 15 errors found during an audit of simple-language.adoc:

  • sysenv.key table entry: corrected "JVM system property" to "OS environment variable"
  • Date function table: fix typo data:command -> date:command (two rows)
  • isAlpha examples: add missing closing parentheses
  • average prose: fix wrong function name ${avg()} -> ${average()}
  • floor prose: replace copy-pasted ${ceil(...)} examples with ${floor(...)}
  • sum prose: fix missing opening brace $sum(...) -> ${sum(...)}
  • toPrettyJson: replace literal TODO: placeholder with actual output example
  • XPath section: replace copy-pasted jsonpath() examples with xpath() expressions
  • lowercase/uppercase examples: replace backtick with closing single quote
  • pad examples: replace invalid single-argument form with correct two-argument form
  • Chain operator example: fix -> to ~> on the last link
  • AND/OR examples: remove spurious ' inside ${header.type'}
  • Init block example: add missing ) in ${uppercase('Hello ${body}')}
  • @BindToRegistry tip: close unclosed string literal
  • Normalize JSon -> JSON throughout the document

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

Claude Code on behalf of Adriano Machado

@ammachado ammachado changed the title CAMEL-22894: Fix documentation errors in simple-language.adoc CAMEL-22894: Extract direct and misc function factories from SimpleFunctionExpression May 22, 2026
@ammachado ammachado marked this pull request as ready for review May 22, 2026 03:32
@github-actions
Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 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 have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@github-actions
Copy link
Copy Markdown
Contributor

🧪 CI tested the following changed modules:

  • core/camel-core-languages
  • core/camel-core

ℹ️ Dependent modules were not tested because the total number of affected modules exceeded the threshold (50). Use the test-dependents label to force testing all dependents.

⚠️ Some tests are disabled on GitHub Actions (@DisabledIfSystemProperty(named = "ci.env.name")) and require manual verification:

  • core/camel-core: 2 test(s) disabled on GitHub Actions
Build reactor — dependencies compiled but only changed modules were tested (2 modules)
  • Camel :: Core
  • Camel :: Core Languages

⚙️ View full build and test results

@davsclaus
Copy link
Copy Markdown
Contributor

[05:04:22.454] WARN (asciidoctor): skipping reference to missing attribute: body
file: /home/runner/work/camel/camel/camel/docs/components/modules/languages/pages/simple-language.adoc
source: /home/runner/work/camel/camel/camel (branch: HEAD | start path: docs/components)
[05:04:22.457] WARN (asciidoctor): skipping reference to missing attribute: body
file: /home/runner/work/camel/camel/camel/docs/components/modules/languages/pages/simple-language.adoc
source: /home/runner/work/camel/camel/camel (branch: HEAD | start path: docs/components)
[05:04:22.458] WARN (asciidoctor): skipping reference to missing attribute: body
file: /home/runner/work/camel/camel/camel/docs/components/modules/languages/pages/simple-language.adoc
source: /home/runner/work/camel/camel/camel (branch: HEAD | start path: docs/components)

Copy link
Copy Markdown
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in SimpleFunctionExpression because it depends on skipFileFunctions from the parser context. Good call.
  • routeGroup handled correctly: it was only in the expression path (not in createCode) in the original, and the PR preserves this.
  • iif validation difference preserved: createFunction uses tokens.length > 3 while createCode uses tokens.length != 3 -- both matching the original.
  • Good test coverage: 28 tests for DirectFunctionFactory (expression evaluation + code generation) and 35 tests for MiscFunctionFactory.
  • 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`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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

ammachado added 3 commits May 22, 2026 16:02
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
Copy link
Copy Markdown
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants