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.

Sign up for GitHub

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

Add validation of paths used for indexing "allow"/"deny" lists. #804

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Add validation of paths used for indexing "allow"/"deny" lists. #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c57481
Add validation of paths used for indexing "allow"/"deny" lists.
tatu-at-datastax Jan 17, 2024
da1b960
Add actual validation, fix messages
tatu-at-datastax Jan 17, 2024
a43b601
Merge branch 'main' into tatu/790-validate-indexing-paths
tatu-at-datastax Jan 17, 2024
728c627
Add check that brackets (array access) are not allowed in no-index paths
tatu-at-datastax Jan 17, 2024
c144774
Testing improvement: also, allow "$vector" in no-index allow/deny lists
tatu-at-datastax Jan 17, 2024
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
29 changes: 29 additions & 0 deletions src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Expand Down Expand Up @@ -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) {
Expand All @@ -133,7 +142,27 @@ 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<String> paths) {
for (String path : paths) {
if (!DocumentConstants.Fields.VALID_PATH_PATTERN.matcher(path).matches()) {
// One exception: $vector is allowed
if (!DocumentConstants.Fields.VECTOR_EMBEDDING_FIELD.equals(path)) {
return path;
}
}
}
return null;
}
}

Expand Down
84 changes: 83 additions & 1 deletion src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -94,7 +95,7 @@ class CreateCollection {
"function" : "cosine"
},
"indexing" : {
"allow" : ["field1", "field2", "address.city", "_id"]
"allow" : ["field1", "field2", "address.city", "_id", "$vector"]
}
}
}
Expand Down Expand Up @@ -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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split existing long test method in two parts since they were testing different aspects.

// create vector collection with indexing allow option
given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
Expand Down Expand Up @@ -396,6 +411,73 @@ 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)
// Brackets not allowed in field names
.body(
"""
{
"createCollection": {
"name": "collection_with_bad_allows",
"options" : {
"indexing" : {
"allow" : ["valid-field", "address[1].street"]
}
}
}
}
""")
.when()
.post(NamespaceResource.BASE_PATH, namespaceName)
.then()
.statusCode(200)
.body("status", is(nullValue()))
.body("data", is(nullValue()))
.body(
"errors[0].message",
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"));
}

@Test
public void failWithInvalidNameInIndexingDeny() {
// create a vector collection
given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
.contentType(ContentType.JSON)
.body(
// Dollars not allowed in regular field names (can only start operators)
"""
{
"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("Invalid indexing definition - `deny` contains invalid path: '$in'"))
.body("errors[0].errorCode", is("INVALID_INDEXING_DEFINITION"))
.body("errors[0].exceptionClass", is("JsonApiException"));
}
}

@Nested
Expand Down