rubenada commented on code in PR #5266: URL: https://github.com/apache/calcite/pull/5266#discussion_r4024484644
########## splunk/src/test/java/org/apache/calcite/adapter/splunk/SplunkPushDownRuleTest.java: ########## @@ -0,0 +1,122 @@ +/* + * 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.calcite.adapter.splunk; + +import org.apache.calcite.jdbc.JavaTypeFactoryImpl; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexInputRef; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlOperator; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeName; + +import com.google.common.collect.ImmutableList; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.List; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * Unit tests for the field-name allowlist inside + * {@link SplunkPushDownRule}. SPL has no generic identifier-quoting + * mechanism, so a field name that is copied verbatim into the generated + * search must match a strict allowlist; anything else could shift the + * boundaries of the emitted command, and the corresponding filter or + * rename is evaluated by Calcite instead. + * + * <p>The tests exercise the filter translation directly and do not + * require a running Splunk instance. + */ +class SplunkPushDownRuleTest { + + private static final RelDataTypeFactory TYPE_FACTORY = + new JavaTypeFactoryImpl(); + private static final RexBuilder REX_BUILDER = new RexBuilder(TYPE_FACTORY); + + /** Translates {@code <field> = 'foo'} via the rule's (private) + * {@code getFilter}, returning the SPL string, or null if the rule + * refuses the push down. */ + private static String filterString(String fieldName) throws Throwable { + RelDataType varchar = TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR); + RexInputRef ref = REX_BUILDER.makeInputRef(varchar, 0); + RexNode lit = REX_BUILDER.makeLiteral("foo"); + Method m = Review Comment: The required method was private, but I've changed it into package private to facilitate tests ########## geode/src/main/java/org/apache/calcite/adapter/geode/util/GeodeUtils.java: ########## @@ -317,6 +318,38 @@ public static RelDataType autodetectRelTypeFromRegion(Region<?, ?> region) { return createRelDataType(entry); } + /** + * OQL has no way to escape identifiers, so an identifier that is copied + * verbatim into the generated query must be a simple alphanumeric name (with + * optional dotted path or bracketed integer subscript for nested access). + * Anything else could shift the boundaries of the emitted statement. + */ + private static final Pattern SAFE_OQL_IDENTIFIER = + Pattern.compile( + "[A-Za-z_$][A-Za-z0-9_$]*+(?:\\.[A-Za-z_$][A-Za-z0-9_$]*+|\\[[0-9]++\\])*+"); + + /** + * Returns whether {@code name} may be emitted verbatim as an OQL identifier + * (a field name, alias, or dotted path). Since OQL provides no way to quote + * an identifier, callers should refuse to push down when this returns false. + */ + public static boolean isSafeOqlIdentifier(String name) { + return name != null && SAFE_OQL_IDENTIFIER.matcher(name).matches(); + } + + /** + * Validates that {@code name} may be emitted verbatim into a generated OQL + * statement, throwing a {@link RuntimeException} otherwise so the planner Review Comment: ok, changed. ########## geode/src/main/java/org/apache/calcite/adapter/geode/rel/GeodeAggregate.java: ########## @@ -100,7 +101,10 @@ public GeodeAggregate(RelOptCluster cluster, List<String> groupByFields = new ArrayList<>(); for (int group : groupSet) { - groupByFields.add(inputFields.get(group)); + // OQL provides no way to quote identifiers, so the group key must + // match a strict allowlist before it can be copied verbatim into the + // generated statement. + groupByFields.add(GeodeUtils.checkSafeOqlIdentifier(inputFields.get(group))); Review Comment: Correct, it shall throw when the identifier is not safe. -- 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]
