From 2c57481dcd7db9a5c703ba74c09034317470ebe8 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Tue, 16 Jan 2024 17:07:33 -0800 Subject: [PATCH 1/4] Add validation of paths used for indexing "allow"/"deny" lists. --- .../v1/CreateCollectionIntegrationTest.java | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java index 880e93c740..1be4cf4507 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java @@ -4,6 +4,7 @@ import static io.stargate.sgv2.common.IntegrationTestUtils.getAuthToken; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.startsWith; import io.quarkus.test.common.QuarkusTestResource; import io.quarkus.test.junit.QuarkusIntegrationTest; @@ -329,6 +330,20 @@ public void duplicateVectorCollectionNameWithDiffSetting() { .body("errors[0].errorCode", is("INVALID_COLLECTION_NAME")) .body("errors[0].exceptionClass", is("JsonApiException")); + // delete the collection + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body(deleteCollectionJson.formatted("simple_collection")) + .when() + .post(NamespaceResource.BASE_PATH, namespaceName) + .then() + .statusCode(200) + .body("status.ok", is(1)); + } + + @Test + public void createCollectionWithIndexing() { // create vector collection with indexing allow option given() .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) @@ -396,6 +411,70 @@ public void duplicateVectorCollectionNameWithDiffSetting() { .statusCode(200) .body("status.ok", is(1)); } + + @Test + public void failWithInvalidNameInIndexingAllows() { + // create a vector collection + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body( + """ + { + "createCollection": { + "name": "collection_with_bad_allows", + "options" : { + "indexing" : { + "allow" : ["field", "$eq"] + } + } + } + } + """) + .when() + .post(NamespaceResource.BASE_PATH, namespaceName) + .then() + .statusCode(200) + .body("status", is(nullValue())) + .body("data", is(nullValue())) + .body( + "errors[0].message", + startsWith("The provided collection name 'collection_with_bad_allows'")) + .body("errors[0].errorCode", is("INVALID_COLLECTION_NAME")) + .body("errors[0].exceptionClass", is("JsonApiException")); + } + + @Test + public void failWithInvalidNameInIndexingDeny() { + // create a vector collection + given() + .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) + .contentType(ContentType.JSON) + .body( + """ + { + "createCollection": { + "name": "collection_with_bad_deny", + "options" : { + "indexing" : { + "deny" : ["field", "$in"] + } + } + } + } + """) + .when() + .post(NamespaceResource.BASE_PATH, namespaceName) + .then() + .statusCode(200) + .body("status", is(nullValue())) + .body("data", is(nullValue())) + .body( + "errors[0].message", + startsWith("The provided collection name 'collection_with_bad_deny'")) + .body("errors[0].errorCode", is("INVALID_COLLECTION_NAME")) + .body("errors[0].exceptionClass", is("JsonApiException")); + } } @Nested From da1b960ab718fe490b52e6acaf332fecd5ad0ad4 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Tue, 16 Jan 2024 17:29:40 -0800 Subject: [PATCH 2/4] Add actual validation, fix messages --- .../command/impl/CreateCollectionCommand.java | 26 +++++++++++++++++++ .../v1/CreateCollectionIntegrationTest.java | 8 +++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java index 3fa96184dc..ee9a20fce3 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonTypeName; import io.stargate.sgv2.jsonapi.api.model.command.NamespaceCommand; +import io.stargate.sgv2.jsonapi.config.constants.DocumentConstants; import io.stargate.sgv2.jsonapi.exception.ErrorCode; import io.stargate.sgv2.jsonapi.exception.JsonApiException; import jakarta.validation.constraints.*; @@ -123,6 +124,14 @@ public void validate() { ErrorCode.INVALID_INDEXING_DEFINITION.getMessage() + " - `allow` cannot contain duplicates"); } + String invalid = findInvalidPath(allow()); + if (invalid != null) { + throw new JsonApiException( + ErrorCode.INVALID_INDEXING_DEFINITION, + String.format( + "%s - `allow` contains invalid path: '%s'", + ErrorCode.INVALID_INDEXING_DEFINITION.getMessage(), invalid)); + } } if (deny() != null) { @@ -133,7 +142,24 @@ public void validate() { ErrorCode.INVALID_INDEXING_DEFINITION.getMessage() + " - `deny` cannot contain duplicates"); } + String invalid = findInvalidPath(deny()); + if (invalid != null) { + throw new JsonApiException( + ErrorCode.INVALID_INDEXING_DEFINITION, + String.format( + "%s - `deny` contains invalid path: '%s'", + ErrorCode.INVALID_INDEXING_DEFINITION.getMessage(), invalid)); + } + } + } + + public String findInvalidPath(List paths) { + for (String path : paths) { + if (!DocumentConstants.Fields.VALID_PATH_PATTERN.matcher(path).matches()) { + return path; + } } + return null; } } diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java index 1be4cf4507..4836fa2e95 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java @@ -439,8 +439,8 @@ public void failWithInvalidNameInIndexingAllows() { .body("data", is(nullValue())) .body( "errors[0].message", - startsWith("The provided collection name 'collection_with_bad_allows'")) - .body("errors[0].errorCode", is("INVALID_COLLECTION_NAME")) + startsWith("Invalid indexing definition - `allow` contains invalid path: '$eq'")) + .body("errors[0].errorCode", is("INVALID_INDEXING_DEFINITION")) .body("errors[0].exceptionClass", is("JsonApiException")); } @@ -471,8 +471,8 @@ public void failWithInvalidNameInIndexingDeny() { .body("data", is(nullValue())) .body( "errors[0].message", - startsWith("The provided collection name 'collection_with_bad_deny'")) - .body("errors[0].errorCode", is("INVALID_COLLECTION_NAME")) + startsWith("Invalid indexing definition - `deny` contains invalid path: '$in'")) + .body("errors[0].errorCode", is("INVALID_INDEXING_DEFINITION")) .body("errors[0].exceptionClass", is("JsonApiException")); } } From 728c62748e564b14fac6517fd507feaa6ef5a8fd Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Wed, 17 Jan 2024 08:44:31 -0800 Subject: [PATCH 3/4] Add check that brackets (array access) are not allowed in no-index paths --- .../jsonapi/api/v1/CreateCollectionIntegrationTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java index 4836fa2e95..5981ef4913 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java @@ -418,6 +418,7 @@ public void failWithInvalidNameInIndexingAllows() { given() .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) .contentType(ContentType.JSON) + // Brackets not allowed in field names .body( """ { @@ -425,7 +426,7 @@ public void failWithInvalidNameInIndexingAllows() { "name": "collection_with_bad_allows", "options" : { "indexing" : { - "allow" : ["field", "$eq"] + "allow" : ["valid-field", "address[1].street"] } } } @@ -439,7 +440,7 @@ public void failWithInvalidNameInIndexingAllows() { .body("data", is(nullValue())) .body( "errors[0].message", - startsWith("Invalid indexing definition - `allow` contains invalid path: '$eq'")) + startsWith("Invalid indexing definition - `allow` contains invalid path: 'address[1].street'")) .body("errors[0].errorCode", is("INVALID_INDEXING_DEFINITION")) .body("errors[0].exceptionClass", is("JsonApiException")); } @@ -451,6 +452,7 @@ public void failWithInvalidNameInIndexingDeny() { .header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken()) .contentType(ContentType.JSON) .body( + // Dollars not allowed in regular field names (can only start operators) """ { "createCollection": { From c144774ed80e93944ae896332c667385599eb02e Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Wed, 17 Jan 2024 09:00:37 -0800 Subject: [PATCH 4/4] Testing improvement: also, allow "$vector" in no-index allow/deny lists --- .../api/model/command/impl/CreateCollectionCommand.java | 5 ++++- .../sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java | 5 +++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java index ee9a20fce3..9184d6298c 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java @@ -156,7 +156,10 @@ public void validate() { public String findInvalidPath(List paths) { for (String path : paths) { if (!DocumentConstants.Fields.VALID_PATH_PATTERN.matcher(path).matches()) { - return path; + // One exception: $vector is allowed + if (!DocumentConstants.Fields.VECTOR_EMBEDDING_FIELD.equals(path)) { + return path; + } } } return null; diff --git a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java index 5981ef4913..8ce98790d8 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java @@ -95,7 +95,7 @@ class CreateCollection { "function" : "cosine" }, "indexing" : { - "allow" : ["field1", "field2", "address.city", "_id"] + "allow" : ["field1", "field2", "address.city", "_id", "$vector"] } } } @@ -440,7 +440,8 @@ public void failWithInvalidNameInIndexingAllows() { .body("data", is(nullValue())) .body( "errors[0].message", - startsWith("Invalid indexing definition - `allow` contains invalid path: 'address[1].street'")) + startsWith( + "Invalid indexing definition - `allow` contains invalid path: 'address[1].street'")) .body("errors[0].errorCode", is("INVALID_INDEXING_DEFINITION")) .body("errors[0].exceptionClass", is("JsonApiException")); }