xiangfu0 commented on code in PR #19664:
URL: https://github.com/apache/pinot/pull/19664#discussion_r4237851748
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java:
##########
@@ -76,16 +86,63 @@ private static String getControllerBaseUrl(HelixManager
helixManager) {
return controllerBaseUrl;
}
- /// Execute DML Statement
+ /// Parses and executes a DML statement.
+ ///
+ /// A `DELETE` is refused by [#executeStatement] with a
[QueryErrorCode#ACCESS_DENIED] error, since this method
+ /// cannot authorize the caller: it is executed once its table is resolved
and the caller authorized to delete rows
+ /// from it, as the query endpoints of the broker and the controller do.
///
/// @param sqlNodeAndOptions Parsed DML object
/// @param headers extra headers map for minion task submission
/// @return BrokerResponse is the DML executed response
public BrokerResponse executeDMLStatement(SqlNodeAndOptions
sqlNodeAndOptions,
@Nullable Map<String, String> headers) {
- DataManipulationStatement statement =
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DataManipulationStatement statement;
+ try {
+ statement = DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ } catch (QueryException e) {
+ // e.g. a DELETE without a WHERE clause
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ return executeStatement(statement, headers);
+ }
+
+ /// Executes a parsed DML statement, e.g. from
[DataManipulationStatementParser#parse], as its
+ /// [DataManipulationStatement#getExecutionType()] tells: in-process, as a
minion task, or through the hook of this
+ /// executor for the statement
([DataManipulationStatement.ExecutionType#EXECUTOR]), e.g. [#executeDelete] for
a
+ /// [DeleteStatement].
+ ///
+ /// It does not authorize the caller. The table of a [DeleteStatement] must
be resolved with
+ /// [DeleteStatement#resolveTableName] and the caller authorized to delete
rows from it before it is executed, as the
+ /// query endpoints of the broker and the controller do: an unresolved
`DELETE` is refused with a
+ /// [QueryErrorCode#ACCESS_DENIED] error.
+ ///
+ /// Execution failures are returned in the response, never thrown: a
[QueryException] thrown by the executing hook
+ /// is returned with its error code and message, any other exception as a
[QueryErrorCode#QUERY_EXECUTION] error.
+ ///
+ /// @param statement parsed statement
+ /// @param headers headers of the original request, e.g. for minion task
submission
+ /// @return the response of the statement
+ public BrokerResponse executeStatement(DataManipulationStatement statement,
@Nullable Map<String, String> headers) {
BrokerResponseNative result = new BrokerResponseNative();
switch (statement.getExecutionType()) {
+ case EXECUTOR:
+ if (statement instanceof DeleteStatement) {
+ DeleteStatement deleteStatement = (DeleteStatement) statement;
+ if (!deleteStatement.isResolved()) {
+ return new BrokerResponseNative(QueryErrorCode.ACCESS_DENIED,
UNAUTHORIZED_DELETE_MESSAGE);
+ }
+ try {
+ return executeDelete(deleteStatement, headers);
+ } catch (QueryException e) {
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ } catch (Exception e) {
+ return new BrokerResponseNative(QueryErrorCode.QUERY_EXECUTION,
e.getMessage());
Review Comment:
Fixed in b5d0ecb260: `SqlQueryExecutor` has a logger now; a `QueryException`
from `executeDelete` is logged at WARN with the table and its code, any other
exception at ERROR with the stack trace, and the response falls back to
`e.toString()` when the message is null. `SqlQueryExecutorTest` has a case for
an exception without a message.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +442,77 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, the caller authorized to
delete rows from it, and only then is
+ /// the table checked to exist, so that the existence of a table is not
leaked to a caller who is not authorized for
+ /// it. The executor deletes rows from that exact table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement statement = ((DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions))
+
.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ authorizeDelete(accessControl, statement.getTableName(), httpHeaders);
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + statement.getTableName());
+ }
+ LOGGER.info("Executing DELETE on table: {}, predicate: {}, options: {},
client: {}", statement.getTableName(),
+ statement.getPredicate(), statement.getOptions().keySet(),
getClientIp(httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Client of the request for the log, from the proxy headers as
`HttpRequesterIdentity#getClientIp` reads them.
+ private static String getClientIp(HttpHeaders httpHeaders) {
+ String forwardedFor = httpHeaders.getHeaderString("X-Forwarded-For");
+ if (forwardedFor != null) {
+ return forwardedFor;
+ }
+ String realIp = httpHeaders.getHeaderString("X-Real-IP");
+ return realIp != null ? realIp : CommonConstants.UNKNOWN;
+ }
+
+ /// Authorizes the caller, who passed the caller-level check of
[#executeDelete], to delete rows from the table.
+ ///
+ /// It checks, on the raw table name and in this order, the `READ` access
type with the `/sql` endpoint (the request
+ /// path, as the authentication filter passes for the other endpoints), the
fine-grained [Actions.Table#QUERY]
+ /// action, the `DELETE` access type with the `/sql` endpoint and the
fine-grained [Actions.Table#DELETE_ROWS]
+ /// action. The `READ` and `QUERY` checks are there because the WHERE clause
reads the table, the `DELETE` and
+ /// `DELETE_ROWS` ones because rows are deleted, as the other deletions of
the controller (e.g. of segments) check
+ /// `DELETE` on the raw table name. This is stricter than the controller's
query path, which only checks the `READ`
+ /// access type with `"Query"` as the endpoint and no fine-grained action,
and it does not apply the row-level
+ /// security of the brokers. Unlike the broker, which has no access type to
tell a deletion from a query and denies
+ /// it by default, an access control that does not tell access types apart
and allows every fine-grained action
+ /// lets every reader of the table delete rows, as it lets them delete
segments.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, as for queries
+ private void authorizeDelete(AccessControl accessControl, String tableName,
HttpHeaders httpHeaders) {
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders,
SQL_ENDPOINT)
+ || !accessControl.hasAccess(httpHeaders, TargetType.TABLE,
rawTableName, Actions.Table.QUERY)
+ || !accessControl.hasAccess(rawTableName, AccessType.DELETE,
httpHeaders, SQL_ENDPOINT)
+ || !accessControl.hasAccess(httpHeaders, TargetType.TABLE,
rawTableName, Actions.Table.DELETE_ROWS)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows from table: " + tableName);
Review Comment:
Changed in b5d0ecb260: the checks still run on the resolved name, but every
error names the table as the caller wrote it, and the logical-table rejection
now happens after the table checks pass (`resolveTableName` records
`isLogicalTable()` instead of throwing). The broker does the same, and its 403
only carries the access control's reason when that reason does not mention the
resolved table name (or its raw name), so the message is identical for an
existing and a non-existing table; the full reason stays in the broker log.
Tests on both roles cover an unauthorized caller on a resolved-case name and on
a logical table.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -713,6 +739,137 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table.
+ ///
+ /// The first-step access control runs first, as for queries, followed by
the table check without a table, so that
+ /// an access control that only authenticates the caller in that check
rejects an unauthenticated caller before the
+ /// table is looked up. The table is then resolved with the database of the
+ /// request and in the case it is defined with, the caller is authorized to
delete rows from it (see
+ /// [#authorizeDelete]), and the executor deletes rows from that exact
table. A table the table cache does not know
+ /// fails with [QueryErrorCode#TABLE_DOES_NOT_EXIST], as a query does, but
only once the caller is authorized for
+ /// it, so that the existence of a table is not leaked to a caller who is
not authorized for it.
+ private BrokerResponse executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
Map<String, String> headers,
+ HttpRequesterIdentity requesterIdentity, @Nullable HttpHeaders
httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ // The first-step access control runs before the table is looked up, as
for queries
+ AuthorizationResult authorizationResult =
accessControl.authorize(requesterIdentity);
+ if (authorizationResult.hasAccess()) {
+ // Access controls that authenticate the caller in their table check
rather than in the first step (e.g. the
+ // ZooKeeper basic auth) reject an unauthenticated caller here, before
the table is looked up
+ authorizationResult = accessControl.authorize(requesterIdentity,
Set.of());
Review Comment:
Both handled in b5d0ecb260. An `UnsupportedOperationException` from the
table check (the probe or the per-table call) is now treated as a denial
through `deleteAccessDenied`, so a `BrokerRequest`-only access control gets a
403 and the dropped-request meter rather than a 500; a test with an access
control that overrides only the `BrokerRequest` overloads covers it. And a
logical table is handled like an unknown one: `resolveTableName` records it,
`authorizeDelete` runs, and only then does the broker answer QUERY_VALIDATION,
so the probe's result no longer decides what an unauthenticated caller learns.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -165,8 +169,8 @@ private static SqlNodeAndOptions
extractSqlNodeAndOptions(SqlNodeList sqlNodeLis
SqlNode statementNode = null;
Map<String, String> options = new HashMap<>();
for (SqlNode sqlNode : sqlNodeList) {
- if (sqlNode instanceof SqlInsertFromFile) {
- // extract insert statement (execution statement)
+ if (sqlNode instanceof SqlInsertFromFile || sqlNode instanceof
SqlDelete) {
Review Comment:
Restored in b5d0ecb260: `extractSqlNodeAndOptions` throws
`SqlCompilationException("EXPLAIN is not supported for DELETE")` when an
`EXPLAIN`'s explicandum is a `SqlDelete`, with
`CalciteSqlCompilerTest.testExplainDeleteIsRejected`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/dml/DeleteStatement.java:
##########
@@ -0,0 +1,351 @@
+/**
+ * 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.pinot.sql.parsers.dml;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlDelete;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSyntax;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.sql.validate.SqlValidatorUtil;
+import org.apache.pinot.common.config.provider.TableCache;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.ExpressionType;
+import org.apache.pinot.common.request.Function;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DatabaseUtils;
+import org.apache.pinot.spi.config.task.AdhocTaskConfig;
+import org.apache.pinot.spi.exception.QueryErrorCode;
+import org.apache.pinot.spi.exception.QueryException;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+
+/// A SQL `DELETE FROM <table> WHERE <predicate>` statement.
+///
+/// Pinot parses `DELETE` but does not delete rows itself: the default SQL
executor answers that it is not supported,
+/// and a deployment that can delete rows, e.g. by purging the matching rows
from the segments with a minion task,
+/// executes the parsed statement by overriding
`SqlQueryExecutor#executeDelete`. Its execution type is
+/// [ExecutionType#EXECUTOR]: the generic [#execute()], [#getResultSchema()]
and [#generateAdhocTaskConfig()] do not
+/// apply to it.
+///
+/// Before handing the statement to the executor, the broker and the
controller resolve its table with
+/// [#resolveTableName], authorize the caller to delete rows from that table,
and check that the table exists with
+/// [#tableExists()].
+///
+/// Its options are the `SET` statements, the legacy `OPTION(...)` suffix and
the request `queryOptions` of the
+/// statement (`SET` takes precedence). The `database` option only qualifies
the table name, see [#resolveTableName].
+/// Every other option, including query options such as `timeoutMs`, is passed
to the executor through
+/// [#getOptions()], so that an option the executor relies on (e.g. a dry run)
cannot be reclassified as a query option
+/// and silently dropped.
+///
+/// Instances are immutable and thread-safe.
+public class DeleteStatement implements DataManipulationStatement {
+ public static final String NOT_SUPPORTED_MESSAGE =
+ "DELETE is not supported by this endpoint, it requires a SQL executor
that implements row deletion";
+
+ /// Serializes the WHERE clause into SQL that the Pinot parser reads back
into the same expression. Combined with
+ /// `quoteAllIdentifiers = false`, identifiers are only quoted (with double
quotes) when they were quoted.
+ private static final SqlDialect PINOT_SQL_DIALECT = new
SqlDialect(SqlDialect.EMPTY_CONTEXT
+ .withIdentifierQuoteString("\"")
+ .withLiteralQuoteString("'")
+ .withLiteralEscapedQuoteString("''")
+ .withUnquotedCasing(Casing.UNCHANGED)
+ .withQuotedCasing(Casing.UNCHANGED)
+ .withCaseSensitive(true));
+
+ /// Quotes the unquoted identifiers named like a SQL function without
arguments, e.g. `user`, `pi` or `current_date`:
+ /// Calcite unparses them as upper-cased keywords, while Pinot reads them as
columns, so they would name another
+ /// column. Mirrors the check of `SqlUtil.unparseSqlIdentifierSyntax`.
+ private static final SqlShuttle KEYWORD_IDENTIFIER_QUOTER = new SqlShuttle()
{
+ @Override
+ public SqlNode visit(SqlIdentifier identifier) {
+ if (identifier.isSimple() && !identifier.getParserPosition().isQuoted())
{
+ SqlOperator operator =
+
SqlValidatorUtil.lookupSqlFunctionByID(SqlStdOperatorTable.instance(),
identifier, null);
+ if (operator != null && (operator.getSyntax() == SqlSyntax.FUNCTION_ID
+ || operator.getSyntax() == SqlSyntax.FUNCTION_ID_CONSTANT)) {
+ return new SqlIdentifier(identifier.names, null,
SqlParserPos.QUOTED_ZERO,
+ List.of(SqlParserPos.QUOTED_ZERO));
+ }
+ }
+ return identifier;
+ }
+ };
+
+ /// Canonical names (lower case, without underscores) of the functions that
read another table than the one the
+ /// statement deletes from, which the caller is not authorized to read.
+ private static final Set<String> CROSS_TABLE_FUNCTIONS = Set.of("lookup",
"insubquery", "inpartitionedsubquery");
+
+ /// Canonical names of the functions that run a script. Rejected because the
broker's groovy policy
+ /// (`pinot.broker.disable.query.groovy`, the per-table
`queryConfig.disableGroovy` override and the static
+ /// analyzer) only applies to queries, not to a `DELETE`, and the executor
may run the predicate on another role.
+ private static final Set<String> SCRIPT_FUNCTIONS = Set.of("groovy");
+
+ private final String _tableName;
+ private final String _predicate;
+ @Nullable
+ private final String _database;
+ private final Map<String, String> _options;
+ private final boolean _resolved;
+ private final boolean _tableExists;
+
+ private DeleteStatement(String tableName, String predicate, @Nullable String
database, Map<String, String> options) {
+ this(tableName, predicate, database, options, false, false);
+ }
+
+ private DeleteStatement(String tableName, String predicate, @Nullable String
database, Map<String, String> options,
+ boolean resolved, boolean tableExists) {
+ _tableName = tableName;
+ _predicate = predicate;
+ _database = database;
+ _options = Collections.unmodifiableMap(new HashMap<>(options));
+ _resolved = resolved;
+ _tableExists = tableExists;
+ }
+
+ /// Parses a `DELETE` statement.
+ ///
+ /// @throws IllegalArgumentException if the statement is not a supported
`DELETE`: it must have a WHERE clause, no
+ /// table alias, a WHERE clause that Pinot
can parse as an expression, that does
+ /// not read another table (e.g. with
`lookUp` or `IN_SUBQUERY`) and that does not
+ /// run a script (`groovy`), and set the
database with the `database` option only
+ public static DeleteStatement parse(SqlNodeAndOptions sqlNodeAndOptions) {
+ SqlNode sqlNode = sqlNodeAndOptions.getSqlNode();
+ checkArgument(sqlNode instanceof SqlDelete, "Not a DELETE statement: %s",
sqlNode.getKind());
+ SqlDelete sqlDelete = (SqlDelete) sqlNode;
+ String tableName = getTableName(sqlDelete.getTargetTable());
+ checkArgument(sqlDelete.getAlias() == null,
+ "DELETE does not support a table alias, reference the columns
directly");
+ SqlNode condition = sqlDelete.getCondition();
+ checkArgument(condition != null, "DELETE requires a WHERE clause to
prevent an accidental full-table delete; use "
+ + "an explicit always-true predicate or delete the table segments to
remove all rows");
+ String predicate = toPinotSql(condition);
+
+ String database = null;
+ Map<String, String> options = new HashMap<>();
+ for (Map.Entry<String, String> option :
sqlNodeAndOptions.getOptions().entrySet()) {
+ String key = option.getKey();
+ if (key.equals(CommonConstants.DATABASE)) {
+ database = option.getValue();
+ } else if (key.equalsIgnoreCase(CommonConstants.DATABASE)) {
+ // Queries only read the exact `database` option (where they read it
at all, see resolveTableName): fail
+ // rather than delete from another table
+ throw new IllegalArgumentException(
+ "Unsupported option: " + key + ", set the database with the '" +
CommonConstants.DATABASE + "' option");
+ } else {
+ options.put(key, option.getValue());
+ }
+ }
+ return new DeleteStatement(tableName, predicate, database, options);
+ }
+
+ private static String getTableName(SqlNode targetTable) {
+ // Table hints and EXTEND clauses parse into other node types
+ checkArgument(targetTable instanceof SqlIdentifier, "DELETE only supports
a plain table name, got: %s",
+ targetTable);
+ // A quoted name part may contain a dot, which splits it like the table
name of a query
+ String tableName = String.join(".", ((SqlIdentifier) targetTable).names);
+ // Empty parts, e.g. in "db."."t", are rejected rather than dropped, so
that the table name is the one authorized
+ String[] parts = tableName.split("\\.", -1);
+ checkArgument(parts.length <= 2 &&
Arrays.stream(parts).noneMatch(String::isEmpty),
+ "Invalid table name: %s, expected [database.]table", tableName);
+ return tableName;
+ }
+
+ /// Serializes a WHERE clause back into SQL, and verifies that Pinot parses
it into the same expression.
+ private static String toPinotSql(SqlNode condition) {
+ Expression expression;
+ Expression serializedExpression;
+ String predicate;
+ try {
+ expression = CalciteSqlParser.compileToExpression(condition);
+ predicate = condition.accept(KEYWORD_IDENTIFIER_QUOTER)
+ .toSqlString(config -> config.withDialect(PINOT_SQL_DIALECT)
+ .withQuoteAllIdentifiers(false)
+ .withIndentation(0))
+ .getSql();
+ serializedExpression = CalciteSqlParser.compileToExpression(predicate);
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Unsupported WHERE clause in DELETE:
" + describe(condition), e);
+ }
+ // Fail rather than hand over a predicate that selects other rows than the
statement
+ checkArgument(serializedExpression.equals(expression),
+ "Unsupported WHERE clause in DELETE, it cannot be serialized back into
the same expression: %s", condition);
+ checkFunctions(expression);
+ return predicate;
+ }
+
+ /// Renders a node for an error message without failing: rendering it is
what may have failed, e.g. for an operator
+ /// without unparse support such as `AT TIME ZONE`, in which case the cause
of the error carries the detail.
+ private static String describe(SqlNode node) {
+ try {
+ return node.toString();
+ } catch (RuntimeException e) {
+ return "<" + node.getKind() + " expression that cannot be rendered>";
+ }
+ }
+
+ /// Rejects the functions that read another table, as the caller is only
authorized for the table it deletes from,
+ /// and the functions that run a script, see [#SCRIPT_FUNCTIONS].
+ private static void checkFunctions(Expression expression) {
Review Comment:
Done in b5d0ecb260: the same WHERE-node visitor rejects `OVER` calls and
`SqlWindow` nodes before compilation ("window functions are not supported in a
DELETE predicate"), and `checkFunctions` also rejects an `over` operator at the
expression level. Tests cover `RANK() OVER (ORDER BY lookUp(...))` and a
`groovy(...)` inside `PARTITION BY`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -424,6 +442,77 @@ private StreamingOutput executeSqlQuery(@Context
HttpHeaders httpHeaders, String
}
}
+ /// Executes a `DELETE` once the caller is authorized to delete rows from
its table, see [#authorizeDelete].
+ ///
+ /// The caller is first checked without a table (the `READ` access type with
the `/sql` endpoint, as for a
+ /// multi-stage query), before the statement is parsed and its table looked
up: with basic auth, an unauthenticated
+ /// caller gets HTTP 401 here, before the table cache is consulted. The
table is then resolved, with the database of
+ /// the request and in the case it is defined with, the caller authorized to
delete rows from it, and only then is
+ /// the table checked to exist, so that the existence of a table is not
leaked to a caller who is not authorized for
+ /// it. The executor deletes rows from that exact table.
+ ///
+ /// The executor runs before the response streams, so that an exception it
lets escape is mapped to an error
+ /// response by [#executeSqlQueryCatching] rather than failing the request
with HTTP 500.
+ ///
+ /// @throws QueryException with [QueryErrorCode#ACCESS_DENIED] if the caller
is not authorized, with
+ /// [QueryErrorCode#TABLE_DOES_NOT_EXIST] if the
table does not exist, and with the errors of
+ /// [DataManipulationStatementParser#parse] and
[DeleteStatement#resolveTableName]
+ private StreamingOutput executeDelete(SqlNodeAndOptions sqlNodeAndOptions,
HttpHeaders httpHeaders) {
+ AccessControl accessControl = _accessControlFactory.create();
+ if (!accessControl.hasAccess(AccessType.READ, httpHeaders, SQL_ENDPOINT)) {
+ throw QueryErrorCode.ACCESS_DENIED.asException("Permission denied to
delete rows");
+ }
+ DeleteStatement statement = ((DeleteStatement)
DataManipulationStatementParser.parse(sqlNodeAndOptions))
+
.resolveTableName(httpHeaders.getHeaderString(CommonConstants.DATABASE),
+ _pinotHelixResourceManager.getTableCache());
+ authorizeDelete(accessControl, statement.getTableName(), httpHeaders);
+ if (!statement.tableExists()) {
+ throw QueryErrorCode.TABLE_DOES_NOT_EXIST.asException("Table does not
exist: " + statement.getTableName());
+ }
+ LOGGER.info("Executing DELETE on table: {}, predicate: {}, options: {},
client: {}", statement.getTableName(),
+ statement.getPredicate(), statement.getOptions().keySet(),
getClientIp(httpHeaders));
+ BrokerResponse response = _sqlQueryExecutor.executeStatement(statement,
extractHeaders(httpHeaders));
+ return output -> {
+ try (OutputStream os = output) {
+ response.toOutputStream(os);
+ }
+ };
+ }
+
+ /// Client of the request for the log, from the proxy headers as
`HttpRequesterIdentity#getClientIp` reads them.
Review Comment:
Changed in b5d0ecb260: the controller helper prefers `X-Forwarded-For` with
`,` replaced by `;`, falls back to `X-Real-IP`, and its Javadoc only claims the
shared `;` format. I left `HttpRequesterIdentity` alone rather than introduce a
shared helper here, since changing how the broker glues both headers together
affects the query log format and belongs in its own change.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/sql/SqlQueryExecutor.java:
##########
@@ -76,16 +86,63 @@ private static String getControllerBaseUrl(HelixManager
helixManager) {
return controllerBaseUrl;
}
- /// Execute DML Statement
+ /// Parses and executes a DML statement.
+ ///
+ /// A `DELETE` is refused by [#executeStatement] with a
[QueryErrorCode#ACCESS_DENIED] error, since this method
+ /// cannot authorize the caller: it is executed once its table is resolved
and the caller authorized to delete rows
+ /// from it, as the query endpoints of the broker and the controller do.
///
/// @param sqlNodeAndOptions Parsed DML object
/// @param headers extra headers map for minion task submission
/// @return BrokerResponse is the DML executed response
public BrokerResponse executeDMLStatement(SqlNodeAndOptions
sqlNodeAndOptions,
@Nullable Map<String, String> headers) {
- DataManipulationStatement statement =
DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ DataManipulationStatement statement;
+ try {
+ statement = DataManipulationStatementParser.parse(sqlNodeAndOptions);
+ } catch (QueryException e) {
+ // e.g. a DELETE without a WHERE clause
+ return new BrokerResponseNative(e.getErrorCode(), e.getMessage());
+ }
+ return executeStatement(statement, headers);
+ }
+
+ /// Executes a parsed DML statement, e.g. from
[DataManipulationStatementParser#parse], as its
+ /// [DataManipulationStatement#getExecutionType()] tells: in-process, as a
minion task, or through the hook of this
+ /// executor for the statement
([DataManipulationStatement.ExecutionType#EXECUTOR]), e.g. [#executeDelete] for
a
+ /// [DeleteStatement].
+ ///
+ /// It does not authorize the caller. The table of a [DeleteStatement] must
be resolved with
+ /// [DeleteStatement#resolveTableName] and the caller authorized to delete
rows from it before it is executed, as the
+ /// query endpoints of the broker and the controller do: an unresolved
`DELETE` is refused with a
+ /// [QueryErrorCode#ACCESS_DENIED] error.
+ ///
+ /// Execution failures are returned in the response, never thrown: a
[QueryException] thrown by the executing hook
+ /// is returned with its error code and message, any other exception as a
[QueryErrorCode#QUERY_EXECUTION] error.
+ ///
+ /// @param statement parsed statement
+ /// @param headers headers of the original request, e.g. for minion task
submission
+ /// @return the response of the statement
+ public BrokerResponse executeStatement(DataManipulationStatement statement,
@Nullable Map<String, String> headers) {
BrokerResponseNative result = new BrokerResponseNative();
switch (statement.getExecutionType()) {
+ case EXECUTOR:
+ if (statement instanceof DeleteStatement) {
+ DeleteStatement deleteStatement = (DeleteStatement) statement;
+ if (!deleteStatement.isResolved()) {
Review Comment:
Reworded in b5d0ecb260: the constant keeps its name and code, but its text
and the `executeStatement`/`executeDelete`/`isResolved` Javadoc now describe
the guard as a wiring check that rejects a statement that did not come through
a query endpoint, explicitly not an authorization check, and say that any other
entry point must authorize the caller itself. I did not extract the per-role
authorization chains into a shared helper in this PR: the two roles use
different `AccessControl` types and check different things, so a shared helper
would mostly be an interface with two implementations; happy to do that as a
follow-up if a second entry point shows up.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
