diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/FilterException.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/FilterException.java index 291c327bc..4aaa0a035 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/FilterException.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/FilterException.java @@ -20,6 +20,7 @@ public enum Code implements ErrorCode { 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, diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/UpdateAttemptBuilder.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/UpdateAttemptBuilder.java index 3981f0059..435c0ccf5 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/UpdateAttemptBuilder.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/UpdateAttemptBuilder.java @@ -24,11 +24,12 @@ public class UpdateAttemptBuilder { private final SchemaT tableBasedSchema; private final WhereCQLClauseAnalyzer whereCQLClauseAnalyzer; - public UpdateAttemptBuilder(SchemaT tableBasedSchema) { + public UpdateAttemptBuilder(SchemaT tableBasedSchema, boolean updateOne) { this.tableBasedSchema = tableBasedSchema; this.whereCQLClauseAnalyzer = - new WhereCQLClauseAnalyzer(tableBasedSchema, WhereCQLClauseAnalyzer.StatementType.UPDATE); + new WhereCQLClauseAnalyzer( + tableBasedSchema, WhereCQLClauseAnalyzer.StatementType.UPDATE_ONE); } public UpdateAttempt build( diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/WhereCQLClauseAnalyzer.java b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/WhereCQLClauseAnalyzer.java index d42ba0b00..7a61ff617 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/WhereCQLClauseAnalyzer.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/WhereCQLClauseAnalyzer.java @@ -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; @@ -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()); @@ -237,6 +239,31 @@ private void checkFilteringOnComplexColumns(Map iden } } + /** + * Check if any $in filter is used. + * + *

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 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. * diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateOneCommandResolver.java b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateOneCommandResolver.java index 9938f6613..7a4ddec79 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateOneCommandResolver.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/service/resolver/UpdateOneCommandResolver.java @@ -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 = diff --git a/src/main/resources/errors.yaml b/src/main/resources/errors.yaml index c63fe118c..9edd9466e 100644 --- a/src/main/resources/errors.yaml +++ b/src/main/resources/errors.yaml @@ -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. + + Update One and Delete One commands can only modify a single rows, and so cannot use $in or $nin. + + 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 diff --git a/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/LogicalExpressionTestData.java b/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/LogicalExpressionTestData.java index 3ab18805a..d757d9557 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/LogicalExpressionTestData.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/LogicalExpressionTestData.java @@ -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() diff --git a/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/TestDataNames.java b/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/TestDataNames.java index b466aabaa..3a7080cc1 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/TestDataNames.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/TestDataNames.java @@ -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 = diff --git a/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/WhereAnalyzerTestData.java b/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/WhereAnalyzerTestData.java index f6a6c9dc6..a8d9a6068 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/WhereAnalyzerTestData.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/fixtures/testdata/WhereAnalyzerTestData.java @@ -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) diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/tables/DeleteUpdateWhereAnalyzerTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/tables/DeleteUpdateWhereAnalyzerTest.java index c544b42d9..106c37aeb 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/tables/DeleteUpdateWhereAnalyzerTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/tables/DeleteUpdateWhereAnalyzerTest.java @@ -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; @@ -36,7 +38,8 @@ private static Stream 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 @@ -54,7 +57,7 @@ private static Stream 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 @@ -81,7 +84,8 @@ private static Stream 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 @@ -118,7 +122,7 @@ private static Stream 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)); } @@ -145,7 +149,7 @@ private static Stream 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)); } @@ -172,7 +176,7 @@ private static Stream 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)); } @@ -199,7 +203,7 @@ private static Stream 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)); } @@ -226,7 +230,7 @@ private static Stream 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)); } @@ -254,7 +258,7 @@ private static Stream 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)); } @@ -282,7 +286,7 @@ private static Stream 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)); } @@ -301,4 +305,31 @@ public void skip1and3of3ClusteringKey( .eqOnlyOneClusteringKey(1) .analyzeMaybeFilterError(expectedCode); } + + private static Stream 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()); + } }