gnodet commented on code in PR #23263: URL: https://github.com/apache/camel/pull/23263#discussion_r3267360943
########## core/camel-core-languages/src/main/java/org/apache/camel/language/simple/functions/SkipFunctionFactory.java: ########## @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.language.simple.functions; + +import org.apache.camel.CamelContext; +import org.apache.camel.Expression; +import org.apache.camel.language.simple.SimpleExpressionBuilder; +import org.apache.camel.language.simple.types.SimpleParserException; +import org.apache.camel.spi.SimpleLanguageFunctionFactory; +import org.apache.camel.util.ObjectHelper; +import org.apache.camel.util.StringHelper; + +import static org.apache.camel.language.simple.ast.SimpleFunctionExpression.ifStartsWithReturnRemainder; + +/** + * Built-in Simple function: {@code ${skip(number)}}. + */ +public final class SkipFunctionFactory implements SimpleLanguageFunctionFactory { + + @Override + public Expression createFunction(CamelContext camelContext, String function, int index) { + String remainder = ifStartsWithReturnRemainder("skip(", function); + if (remainder == null) { + return null; + } + String values = StringHelper.before(remainder, ")"); Review Comment: **Bug (pre-existing, but should fix now):** This uses `StringHelper.before(remainder, ")")` while `createCode` below (line 53) uses `StringHelper.beforeLast(remainder, ")")`. The difference matters if the argument ever contains a literal `)` — `before` stops at the first one, `beforeLast` stops at the last. This inconsistency was faithfully ported from the original code (runtime line 1196 vs code-gen line 3266), but since we're extracting this into its own class, this is the right time to fix it. All other factories consistently use `beforeLast`. ```suggestion String values = StringHelper.beforeLast(remainder, ")"); ``` ########## core/camel-core-languages/src/main/java/org/apache/camel/language/simple/ast/SimpleFunctionExpression.java: ########## @@ -401,33 +400,10 @@ private Expression doCreateSimpleExpression(CamelContext camelContext, String fu return math; } - // attachments - if ("attachments".equals(function) || "clearAttachments".equals(function) - || ifStartsWithReturnRemainder("setAttachment", function) != null - || ifStartsWithReturnRemainder("attachment", function) != null) { - Expression exp = createSimpleAttachments(camelContext, function); - if (exp != null) { - return exp; - } - } - // base64 - if ("base64Encode".equals(function) || "base64Decode".equals(function) - || ifStartsWithReturnRemainder("base64Encode", function) != null - || ifStartsWithReturnRemainder("base64Decode", function) != null) { - Expression exp = createSimpleBase64(camelContext, function); - if (exp != null) { - return exp; - } - } - // html - if ("htmlClean".equals(function) || "htmlParse".equals(function) || "htmlDecode".equals(function) - || ifStartsWithReturnRemainder("htmlClean", function) != null - || ifStartsWithReturnRemainder("htmlParse", function) != null - || ifStartsWithReturnRemainder("htmlDecode", function) != null) { - Expression exp = createSimpleHtml(camelContext, function); - if (exp != null) { - return exp; - } + // functions from external components (attachments, base64, html, ...) Review Comment: **Ordering change:** In the original code, `random`, `skip`, `collate`, `join` were evaluated inside `createSimpleExpressionMisc()` (called ~10 lines above, before `createSimpleExpressionMath`). Now they're evaluated here via the dispatcher, which runs *after* math. In practice this is safe because `random(`, `skip(`, `collate(`, `join(` don't collide with math expression patterns. But the PR description says "behaviour preserved exactly" — this subtle shift in evaluation order should at least be acknowledged. Alternatively, the dispatcher call for built-ins could be placed before the `createSimpleExpressionMisc` call to truly preserve the original ordering. The same applies to the `doCreateCode` path below. ########## core/camel-core-languages/src/main/java/org/apache/camel/language/simple/SimpleFunctionDispatcher.java: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.language.simple; + +import java.util.List; +import java.util.function.Predicate; + +import org.apache.camel.CamelContext; +import org.apache.camel.Expression; +import org.apache.camel.language.simple.functions.CollateFunctionFactory; +import org.apache.camel.language.simple.functions.JoinFunctionFactory; +import org.apache.camel.language.simple.functions.RandomFunctionFactory; +import org.apache.camel.language.simple.functions.SkipFunctionFactory; +import org.apache.camel.spi.SimpleLanguageFunctionFactory; +import org.apache.camel.support.ResolverHelper; + +import static org.apache.camel.language.simple.ast.SimpleFunctionExpression.ifStartsWithReturnRemainder; + +/** + * Resolves {@link SimpleLanguageFunctionFactory} implementations shipped by external Camel components (currently Review Comment: The Javadoc says this class "resolves `SimpleLanguageFunctionFactory` implementations shipped by external Camel components" — but it now also manages the built-in factories (`BUILT_INS`). Consider updating to reflect both roles, e.g.: > Dispatches Simple/CSimple function lookup to built-in function factories and to `SimpleLanguageFunctionFactory` implementations shipped by external Camel components (currently camel-attachments, camel-base64, camel-jsoup). ########## core/camel-core-languages/src/main/java/org/apache/camel/language/simple/SimpleFunctionDispatcher.java: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.language.simple; + +import java.util.List; +import java.util.function.Predicate; + +import org.apache.camel.CamelContext; +import org.apache.camel.Expression; +import org.apache.camel.language.simple.functions.CollateFunctionFactory; +import org.apache.camel.language.simple.functions.JoinFunctionFactory; +import org.apache.camel.language.simple.functions.RandomFunctionFactory; +import org.apache.camel.language.simple.functions.SkipFunctionFactory; +import org.apache.camel.spi.SimpleLanguageFunctionFactory; +import org.apache.camel.support.ResolverHelper; + +import static org.apache.camel.language.simple.ast.SimpleFunctionExpression.ifStartsWithReturnRemainder; + +/** + * Resolves {@link SimpleLanguageFunctionFactory} implementations shipped by external Camel components (currently + * camel-attachments, camel-base64, camel-jsoup) and delegates Simple/CSimple function lookup to whichever one claims + * the function string. + * <p> + * Each entry carries a gate that decides whether its factory is consulted for a given function string; gates mirror the + * inline checks that previously lived in {@code SimpleFunctionExpression}, so a function belonging to a missing + * component still surfaces the "add the JAR to your classpath" error from + * {@link ResolverHelper#resolveMandatoryBootstrapService}. Factory resolution is cached by the bootstrap factory + * finder, so repeated dispatch is cheap. + */ +public final class SimpleFunctionDispatcher { + + /** + * Built-in factories shipped by camel-core-languages itself. Iterated before {@link #EXPRESSION_ENTRIES}, matching + * the original priority of these functions inside {@code SimpleFunctionExpression}. Each factory returns + * {@code null} for inputs it does not recognise, so no gating predicate is needed. + */ + private static final List<SimpleLanguageFunctionFactory> BUILT_INS = List.of( + new RandomFunctionFactory(), + new SkipFunctionFactory(), + new CollateFunctionFactory(), + new JoinFunctionFactory()); + + private static final List<Entry> EXPRESSION_ENTRIES = List.of( + new Entry("camel-attachments", SimpleFunctionDispatcher::isAttachmentFunction), + new Entry("camel-base64", SimpleFunctionDispatcher::isBase64Function), + new Entry("camel-jsoup", SimpleFunctionDispatcher::isHtmlFunction)); + + /** + * Code-generation entries exclude camel-jsoup deliberately: it has no csimple support and its {@code createCode} + * throws. + */ + private static final List<Entry> CODE_ENTRIES = List.of( + new Entry("camel-attachments", SimpleFunctionDispatcher::isAttachmentFunction), + new Entry("camel-base64", SimpleFunctionDispatcher::isBase64Function)); + + private SimpleFunctionDispatcher() { + } + + public static Expression tryCreate(CamelContext camelContext, String function, int index) { + for (SimpleLanguageFunctionFactory factory : BUILT_INS) { + Expression answer = factory.createFunction(camelContext, function, index); + if (answer != null) { + return answer; + } + } + for (Entry entry : EXPRESSION_ENTRIES) { + if (!entry.claims.test(function)) { + continue; + } + SimpleLanguageFunctionFactory factory = resolve(camelContext, entry.jarName); + Expression answer = factory.createFunction(camelContext, function, index); + if (answer != null) { + return answer; + } + } + return null; + } + + public static String tryCreateCode(CamelContext camelContext, String function, int index) { + for (SimpleLanguageFunctionFactory factory : BUILT_INS) { + @SuppressWarnings("deprecation") + String code = factory.createCode(camelContext, function, index); + if (code != null) { + return code; + } + } + for (Entry entry : CODE_ENTRIES) { + if (!entry.claims.test(function)) { + continue; + } + SimpleLanguageFunctionFactory factory = resolve(camelContext, entry.jarName); + @SuppressWarnings("deprecation") + String code = factory.createCode(camelContext, function, index); + if (code != null) { + return code; + } + } + return null; + } + + private static SimpleLanguageFunctionFactory resolve(CamelContext camelContext, String jarName) { + return ResolverHelper.resolveMandatoryBootstrapService( + camelContext, + SimpleLanguageFunctionFactory.FACTORY + "/" + jarName, + SimpleLanguageFunctionFactory.class, + jarName); + } + + private static boolean isAttachmentFunction(String function) { + return "attachments".equals(function) + || "clearAttachments".equals(function) + || ifStartsWithReturnRemainder("setAttachment", function) != null + || ifStartsWithReturnRemainder("attachment", function) != null; + } + + private static boolean isBase64Function(String function) { + return "base64Encode".equals(function) + || "base64Decode".equals(function) + || ifStartsWithReturnRemainder("base64Encode", function) != null + || ifStartsWithReturnRemainder("base64Decode", function) != null; + } + + private static boolean isHtmlFunction(String function) { + return "htmlClean".equals(function) + || "htmlParse".equals(function) + || "htmlDecode".equals(function) + || ifStartsWithReturnRemainder("htmlClean", function) != null + || ifStartsWithReturnRemainder("htmlParse", function) != null + || ifStartsWithReturnRemainder("htmlDecode", function) != null; + } + + private record Entry(String jarName, Predicate<String> claims) { + } +} Review Comment: Nit: Records are used sparingly in Camel core. The project guidelines say "do NOT use Records unless already present in the file." Since this is a new file, it's a judgment call — a simple `private static final class Entry` with two fields would be more conventional for this codebase. Not blocking, but other committers may flag it. ########## core/camel-core-languages/src/main/java/org/apache/camel/language/simple/functions/JoinFunctionFactory.java: ########## @@ -0,0 +1,107 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.language.simple.functions; + +import org.apache.camel.CamelContext; +import org.apache.camel.Expression; +import org.apache.camel.language.simple.SimpleExpressionBuilder; +import org.apache.camel.language.simple.ast.SimpleFunctionExpression; Review Comment: **Circular dependency:** This import creates a dependency from the extracted factory back to `SimpleFunctionExpression` — the very class we're decomposing. `codeSplitSafe` is a utility method that doesn't belong on `SimpleFunctionExpression`. Consider moving `codeSplitSafe` to a utility class (e.g., `SimpleParserHelper` or `StringQuoteHelper`) in a preparatory commit. That would make the extracted factories truly standalone and avoid the circular reference: `SimpleFunctionExpression` → `SimpleFunctionDispatcher` → `JoinFunctionFactory` → `SimpleFunctionExpression`. ########## core/camel-core-languages/src/main/java/org/apache/camel/language/simple/ast/SimpleFunctionExpression.java: ########## @@ -401,33 +400,10 @@ private Expression doCreateSimpleExpression(CamelContext camelContext, String fu return math; } Review Comment: Nit: The comment says "external components" but the dispatcher now also handles built-in functions (`random`, `skip`, `collate`, `join`). Consider: ```suggestion // functions from built-in factories and external components (attachments, base64, html, ...) ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
