Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

forbid inFilter for table updateOne and deleteOne command #1693

Merged
merged 5 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public enum Code implements ErrorCode<FilterException> {
FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE,
INCOMPLETE_PRIMARY_KEY_FILTER,
INVALID_FILTER,
INVALID_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE,
NON_PRIMARY_KEY_FILTER_FOR_UPDATE_DELETE,
UNKNOWN_TABLE_COLUMNS,
UNSUPPORTED_COLUMN_TYPES,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ public class UpdateAttemptBuilder<SchemaT extends TableSchemaObject> {
private final SchemaT tableBasedSchema;
private final WhereCQLClauseAnalyzer whereCQLClauseAnalyzer;

public UpdateAttemptBuilder(SchemaT tableBasedSchema) {
public UpdateAttemptBuilder(SchemaT tableBasedSchema, boolean updateOne) {
Yuqi-Du marked this conversation as resolved.
Show resolved Hide resolved

this.tableBasedSchema = tableBasedSchema;
this.whereCQLClauseAnalyzer =
new WhereCQLClauseAnalyzer(tableBasedSchema, WhereCQLClauseAnalyzer.StatementType.UPDATE);
new WhereCQLClauseAnalyzer(
tableBasedSchema, WhereCQLClauseAnalyzer.StatementType.UPDATE_ONE);
}

public UpdateAttempt<SchemaT> build(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ public class WhereCQLClauseAnalyzer {
/** Different statementTypes for analyzer to work on, thus different strategies. */
public enum StatementType {
SELECT,
UPDATE,
// currently, table feature only supports updateOne
UPDATE_ONE,
DELETE_ONE,
DELETE_MANY;

Expand All @@ -63,12 +64,13 @@ AnalysisStrategy getStrategy(WhereCQLClauseAnalyzer analyzer) {
analyzer::warnNotInUnsupportedByIndexing,
analyzer::warnNoFilters,
analyzer::warnPkNotFullySpecified));
case DELETE_ONE, UPDATE ->
case DELETE_ONE, UPDATE_ONE ->
new AnalysisStrategy(
List.of(
analyzer::checkNoFilters,
analyzer::checkAllColumnsExist,
analyzer::checkNonPrimaryKeyFilters,
analyzer::checkNoInFilterUsage,
analyzer::checkFullPrimaryKey,
analyzer::checkFilteringOnComplexColumns),
List.of());
Expand Down Expand Up @@ -237,6 +239,31 @@ private void checkFilteringOnComplexColumns(Map<CqlIdentifier, TableFilter> iden
}
}

/**
* Check if any $in filter is used.
*
* <p>For UpdateOne and DeleteOne table commands, the use of $in/$nin can affect multiple rows, so
* add a check rule to ban the usage.
*/
private void checkNoInFilterUsage(Map<CqlIdentifier, TableFilter> identifierToFilter) {

var inFilterColumns =
identifierToFilter.entrySet().stream()
.filter(entry -> (entry.getValue() instanceof InTableFilter))
.map((Map.Entry::getKey))
.sorted(CQL_IDENTIFIER_COMPARATOR)
.toList();

if (!inFilterColumns.isEmpty()) {
throw FilterException.Code.INVALID_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE.get(
errVars(
tableSchemaObject,
map -> {
map.put("inFilterColumns", errFmtCqlIdentifier(inFilterColumns));
}));
}
}

/**
* Check if all primary keys components (partition keys, clustering keys) are specified in filter.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public Operation resolveTableCommand(
errVars(ctx.schemaObject(), map -> {}));
}

var builder = new UpdateAttemptBuilder<>(ctx.schemaObject());
var builder = new UpdateAttemptBuilder<>(ctx.schemaObject(), true);

// need to update so we use WithWarnings correctly
var where =
Expand Down
12 changes: 12 additions & 0 deletions src/main/resources/errors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,18 @@ request-errors:

Note, DeleteMany without a filter truncates the table.

- scope: FILTER
code: INVALID_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE
title: In filter is not allowed for UpdateOne and DeleteOne commands
body: |-
The command used a $in or $nin filter to select the rows the modify, however these filters can select more than one row.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sure there's an internal reason to disallow calling updateOne() with $in/$nin, but this sentence doesn't seem to justify why you shouldn't use $in or $nin with updateOne().

For example, a common pattern I've seen with updateOne() is updateOne({ _id: 42, status: { $in: ['pending', 'active'] } }, update): update the document with _id: 42 if it has status 'pending' or 'active'. This pattern can only update at most one row.

Similarly, updateOne({ isDeleted: false }) can select many rows.

Not critical, but it would be nice to make this error message more accurately summarize the reasoning for this behavior.


Update One and Delete One commands can only modify a single rows, and so cannot use $in or $nin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor typo: should be "modify a single row" not "modify a single rows"


The command used an invalid filter on the columns: ${inFilterColumns}.

Resend the request without using the $in or $nin filter.

- scope: FILTER
code: NON_PRIMARY_KEY_FILTER_FOR_UPDATE_DELETE
title: Only Primary Key columns can be filtered on for Update and Delete commands
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,17 @@ public FixtureT eqAllPrimaryKeys() {
return eqAllClusteringKeys();
}

public FixtureT inOnOnePartitionKey(
InTableFilter.Operator inFilterOperator, ColumnMetadata firstPartitionKey) {
if (inFilterOperator == InTableFilter.Operator.IN) {
expression.addFilter(in(firstPartitionKey));
}
if (inFilterOperator == InTableFilter.Operator.NIN) {
expression.addFilter(nin(firstPartitionKey));
}
return fixture;
}

public FixtureT eqAllPartitionKeys() {
tableMetadata
.getPartitionKey()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class TestDataNames {
CqlIdentifier.fromInternal("regular-2-" + System.currentTimeMillis());

public final CqlIdentifier COL_INDEXED_1 =
CqlIdentifier.fromInternal("indexed-1-" + System.currentTimeMillis());
CqlIdentifier.fromInternal("indexed_1_" + System.currentTimeMillis());

// DO NOT ADD TO A TABLE
public final CqlIdentifier COL_UNKNOWN_1 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,15 @@ public WhereAnalyzerFixture assertExceptionOnDurationColumns(CqlIdentifier... co
return assertExceptionContains(warning);
}

public WhereAnalyzerFixture assertExceptionOnInFilerForUpdateOneAndDeleteOne(
CqlIdentifier... columns) {
var identifiers = Arrays.stream(columns).sorted(CQL_IDENTIFIER_COMPARATOR).toList();
var warning =
"The command used an invalid filter on the columns: %s."
.formatted(errFmtCqlIdentifier(identifiers));
return assertExceptionContains(warning);
}

public WhereAnalyzerFixture assertExceptionContains(String contains) {

assertThat(exception)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package io.stargate.sgv2.jsonapi.service.operation.tables;

import com.datastax.oss.driver.api.core.metadata.schema.ColumnMetadata;
import io.stargate.sgv2.jsonapi.exception.FilterException;
import io.stargate.sgv2.jsonapi.fixtures.testdata.TestData;
import io.stargate.sgv2.jsonapi.fixtures.testdata.TestDataNames;
import io.stargate.sgv2.jsonapi.service.operation.filters.table.InTableFilter;
import io.stargate.sgv2.jsonapi.service.operation.tables.WhereCQLClauseAnalyzer.StatementType;
import java.util.stream.Stream;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -36,7 +38,8 @@ private static Stream<Arguments> emptyFilterTests() {
StatementType.DELETE_ONE, FilterException.Code.FILTER_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(
StatementType.DELETE_MANY, FilterException.Code.FILTER_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.UPDATE, FilterException.Code.FILTER_REQUIRED_FOR_UPDATE_DELETE));
Arguments.of(
StatementType.UPDATE_ONE, FilterException.Code.FILTER_REQUIRED_FOR_UPDATE_DELETE));
}

@ParameterizedTest
Expand All @@ -54,7 +57,7 @@ private static Stream<Arguments> eqAllPrimaryKeysTests() {
return Stream.of(
Arguments.of(StatementType.DELETE_ONE, null),
Arguments.of(StatementType.DELETE_MANY, null),
Arguments.of(StatementType.UPDATE, null));
Arguments.of(StatementType.UPDATE_ONE, null));
}

@ParameterizedTest
Expand All @@ -81,7 +84,8 @@ private static Stream<Arguments> eqOnNonPrimaryKeyOrIndexedTests() {
StatementType.DELETE_MANY,
FilterException.Code.NON_PRIMARY_KEY_FILTER_FOR_UPDATE_DELETE),
Arguments.of(
StatementType.UPDATE, FilterException.Code.NON_PRIMARY_KEY_FILTER_FOR_UPDATE_DELETE));
StatementType.UPDATE_ONE,
FilterException.Code.NON_PRIMARY_KEY_FILTER_FOR_UPDATE_DELETE));
}

@ParameterizedTest
Expand Down Expand Up @@ -118,7 +122,7 @@ private static Stream<Arguments> eqMissingPartitionKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, FilterException.Code.INCOMPLETE_PRIMARY_KEY_FILTER),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand All @@ -145,7 +149,7 @@ private static Stream<Arguments> skip1of3ClusteringKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, FilterException.Code.INCOMPLETE_PRIMARY_KEY_FILTER),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand All @@ -172,7 +176,7 @@ private static Stream<Arguments> skip2of3ClusteringKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, FilterException.Code.INCOMPLETE_PRIMARY_KEY_FILTER),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand All @@ -199,7 +203,7 @@ private static Stream<Arguments> skip3of3ClusteringKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, null),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand All @@ -226,7 +230,7 @@ private static Stream<Arguments> skip1and2of3ClusteringKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, FilterException.Code.INCOMPLETE_PRIMARY_KEY_FILTER),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand Down Expand Up @@ -254,7 +258,7 @@ private static Stream<Arguments> skip2and3of3ClusteringKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, null),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand Down Expand Up @@ -282,7 +286,7 @@ private static Stream<Arguments> skip1and3of3ClusteringKeyTests() {
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE),
Arguments.of(StatementType.DELETE_MANY, FilterException.Code.INCOMPLETE_PRIMARY_KEY_FILTER),
Arguments.of(
StatementType.UPDATE,
StatementType.UPDATE_ONE,
FilterException.Code.FULL_PRIMARY_KEY_REQUIRED_FOR_UPDATE_DELETE));
}

Expand All @@ -301,4 +305,31 @@ public void skip1and3of3ClusteringKey(
.eqOnlyOneClusteringKey(1)
.analyzeMaybeFilterError(expectedCode);
}

private static Stream<Arguments> inFilterUsage() {
return Stream.of(
Arguments.of(StatementType.DELETE_ONE, InTableFilter.Operator.IN),
Arguments.of(StatementType.DELETE_ONE, InTableFilter.Operator.NIN),
Arguments.of(StatementType.UPDATE_ONE, InTableFilter.Operator.IN),
Arguments.of(StatementType.UPDATE_ONE, InTableFilter.Operator.NIN));
}

@ParameterizedTest
@MethodSource("inFilterUsage")
public void forbidInFilterUsage(
StatementType statementType, InTableFilter.Operator inTableFilterOperator) {

var fixture =
TEST_DATA
.whereAnalyzer()
.table2PK3Clustering1Index("in_filter_on_" + statementType.name(), statementType);
final ColumnMetadata firstPartitionKey =
TEST_DATA.tableMetadata().table2PK3Clustering1Index().getPartitionKey().getFirst();

fixture
.expression()
.inOnOnePartitionKey(inTableFilterOperator, firstPartitionKey)
.analyzeMaybeFilterError(FilterException.Code.INVALID_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE)
.assertExceptionOnInFilerForUpdateOneAndDeleteOne(firstPartitionKey.getName());
}
}