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

Fix #395: verify that "no options" Commands are not given any options #412

Merged
merged 5 commits into from
May 3, 2023
Merged

Fix #395: verify that "no options" Commands are not given any options #412

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
3d45b97
Fix #395: verify that "no options" Commands are not given any options
tatu-at-datastax May 2, 2023
836272c
Add ITs for couple more commands where relevant
tatu-at-datastax May 2, 2023
27c18b9
Add `@RegisterForReflection` as it seems to be needed
tatu-at-datastax May 2, 2023
e8e8df6
Refactor unit tests as per code review suggestions
tatu-at-datastax May 3, 2023
677c71f
Add happy path test for DeleteOne too
tatu-at-datastax May 3, 2023
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
25 changes: 21 additions & 4 deletions src/main/java/io/stargate/sgv2/jsonapi/api/model/command/NoOptionsCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,29 @@
package io.stargate.sgv2.jsonapi.api.model.command;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.JsonNode;
import io.quarkus.runtime.annotations.RegisterForReflection;
import io.stargate.sgv2.jsonapi.exception.ErrorCode;
import io.stargate.sgv2.jsonapi.exception.JsonApiException;

/**
* Interface that Commands that only take "empty" Command (that is, do not expose options but should
* allow empty JSON Object nonetheless) should implement. <br>
* NOTE: if {@code Command} starts accepting options, it should NO LONGER implement this interface
* as combination will not work (options field will not be deserialized).
* as combination will probably not work.
*/
@JsonIgnoreProperties({"options"})
public interface NoOptionsCommand {}
@RegisterForReflection
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, Native build CI/ITs failed; adding made them pass. Not 100% sure why but suspecting that default method implementation (annotation included, in particular) would not be retained.
I am glad we have tests for that set up to catch these problems.

public interface NoOptionsCommand {
@JsonProperty("options")
default void setOptions(JsonNode value) throws JsonApiException {
// Empty JSON Object and null are accepted; anything else failure
if (value.isNull() || (value.isObject() && value.isEmpty())) {
return;
}
final String msg =
String.format(
"%s: %s",
ErrorCode.COMMAND_ACCEPTS_NO_OPTIONS.getMessage(), getClass().getSimpleName());
throw new JsonApiException(ErrorCode.COMMAND_ACCEPTS_NO_OPTIONS, msg);
}
}
2 changes: 2 additions & 0 deletions src/main/java/io/stargate/sgv2/jsonapi/exception/ErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ public enum ErrorCode {
/** Command error codes. */
COMMAND_NOT_IMPLEMENTED("The provided command is not implemented."),

COMMAND_ACCEPTS_NO_OPTIONS("Command accepts no options"),

CONCURRENCY_FAILURE("Unable to complete transaction due to concurrent transactions"),

DATASET_TOO_BIG("Response data set too big to be sorted, add more filters"),
Expand Down
109 changes: 109 additions & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/configuration/ObjectMapperConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package io.stargate.sgv2.jsonapi.api.configuration;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.catchException;

import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.quarkus.test.junit.QuarkusTest;
Expand All @@ -18,10 +20,12 @@
import io.stargate.sgv2.jsonapi.api.model.command.clause.update.UpdateClause;
import io.stargate.sgv2.jsonapi.api.model.command.impl.CountDocumentsCommands;
import io.stargate.sgv2.jsonapi.api.model.command.impl.CreateCollectionCommand;
import io.stargate.sgv2.jsonapi.api.model.command.impl.DeleteOneCommand;
import io.stargate.sgv2.jsonapi.api.model.command.impl.FindOneAndUpdateCommand;
import io.stargate.sgv2.jsonapi.api.model.command.impl.FindOneCommand;
import io.stargate.sgv2.jsonapi.api.model.command.impl.InsertManyCommand;
import io.stargate.sgv2.jsonapi.api.model.command.impl.InsertOneCommand;
import io.stargate.sgv2.jsonapi.exception.ErrorCode;
import java.util.List;
import javax.inject.Inject;
import org.assertj.core.api.Assertions;
Expand Down Expand Up @@ -118,6 +122,27 @@ public void filterClauseOptional() throws Exception {
FindOneCommand.class,
findOne -> Assertions.assertThat(findOne.filterClause()).isNull());
}

// Only "empty" Options allowed, nothing else
@Test
public void failForNonEmptyOptions() throws Exception {
String json =
"""
{
"findOne": {
"options": {
"noSuchOption": "value"
}
}
}
""";

Exception e = catchException(() -> objectMapper.readValue(json, Command.class));
assertThat(e)
.isInstanceOf(JsonMappingException.class)
.hasMessageStartingWith(
ErrorCode.COMMAND_ACCEPTS_NO_OPTIONS.getMessage() + ": FindOneCommand");
}
}

@Nested
Expand Down Expand Up @@ -148,6 +173,90 @@ public void happyPath() throws Exception {
assertThat(document.required("some").required("data").asBoolean()).isTrue();
});
}

// Only "empty" Options allowed, nothing else
@Test
public void failForNonEmptyOptions() throws Exception {
String json =
"""
{
"insertOne": {
"document": {
"some": {
"data": true
}
},
"options": {
"noSuchOption": "value"
}
}
}
""";

Exception e = catchException(() -> objectMapper.readValue(json, Command.class));
assertThat(e)
.isInstanceOf(JsonMappingException.class)
.hasMessageStartingWith(
ErrorCode.COMMAND_ACCEPTS_NO_OPTIONS.getMessage() + ": InsertOneCommand");
}
}

@Nested
class DeleteOne {
@Test
public void happyPath() throws Exception {
String json =
"""
{
"deleteOne": {
"filter" : {"username" : "Aaron"}
}
}
""";

Command result = objectMapper.readValue(json, Command.class);

assertThat(result)
.isInstanceOfSatisfying(
DeleteOneCommand.class,
cmd -> {
FilterClause filterClause = cmd.filterClause();
assertThat(filterClause).isNotNull();
assertThat(filterClause.comparisonExpressions()).hasSize(1);
assertThat(filterClause.comparisonExpressions())
.singleElement()
.satisfies(
expression -> {
ValueComparisonOperation<String> op =
new ValueComparisonOperation<>(
ValueComparisonOperator.EQ,
new JsonLiteral<>("Aaron", JsonType.STRING));

assertThat(expression.path()).isEqualTo("username");
assertThat(expression.filterOperations()).singleElement().isEqualTo(op);
});
});
}

// Only "empty" Options allowed, nothing else
@Test
public void failForNonEmptyOptions() throws Exception {
String json =
"""
{
"deleteOne": {
"filter" : {"_id" : "doc1"},
"options": {"setting":"abc"}
}
}
""";

Exception e = catchException(() -> objectMapper.readValue(json, Command.class));
assertThat(e)
.isInstanceOf(JsonMappingException.class)
.hasMessageStartingWith(
ErrorCode.COMMAND_ACCEPTS_NO_OPTIONS.getMessage() + ": DeleteOneCommand");
}
}

@Nested
Expand Down
32 changes: 32 additions & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/DeleteOneIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;

import io.quarkus.test.common.QuarkusTestResource;
Expand Down Expand Up @@ -119,6 +120,37 @@ public void emptyOptionsAllowed() {
.body("errors", is(nullValue()));
}

@Test
public void noOptionsAllowed() {
String json =
"""
{
"deleteOne": {
"filter" : {"_id" : "docWithOptions"},
"options": {"setting":"abc"}
}
}
""";

given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
.contentType(ContentType.JSON)
.body(json)
.when()
.post(CollectionResource.BASE_PATH, namespaceName, collectionName)
.then()
.statusCode(200)
.body("data.document", is(nullValue()))
.body("status", is(nullValue()))
.body("errors", is(notNullValue()))
.body("errors", hasSize(2))
.body("errors[0].message", is("Command accepts no options: DeleteOneCommand"))
.body("errors[0].exceptionClass", is("JsonMappingException"))
.body("errors[1].message", is("Command accepts no options: DeleteOneCommand"))
.body("errors[1].errorCode", is("COMMAND_ACCEPTS_NO_OPTIONS"))
.body("errors[1].exceptionClass", is("JsonApiException"));
}

@Test
public void byColumn() {
String json =
Expand Down
31 changes: 31 additions & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/FindOneIntegrationTest.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 net.javacrumbs.jsonunit.JsonMatchers.jsonEquals;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
Expand Down Expand Up @@ -153,6 +154,36 @@ public void emptyOptionsAllowed() {
.body("errors", is(nullValue()));
}

@Test
public void noOptionsAllowed() {
String json =
"""
{
"findOne": {
"options": { "unknown":"value"}
}
}
""";

given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
.contentType(ContentType.JSON)
.body(json)
.when()
.post(CollectionResource.BASE_PATH, namespaceName, collectionName)
.then()
.statusCode(200)
.body("data.document", is(nullValue()))
.body("status", is(nullValue()))
.body("errors", is(notNullValue()))
.body("errors", hasSize(2))
.body("errors[0].message", is("Command accepts no options: FindOneCommand"))
.body("errors[0].exceptionClass", is("JsonMappingException"))
.body("errors[1].message", is("Command accepts no options: FindOneCommand"))
.body("errors[1].errorCode", is("COMMAND_ACCEPTS_NO_OPTIONS"))
.body("errors[1].exceptionClass", is("JsonApiException"));
}

@Test
public void noFilterSortAscending() {
String json =
Expand Down
33 changes: 33 additions & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/InsertIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,39 @@ public void emptyOptionsAllowed() {
.body("errors", is(nullValue()));
}

@Test
public void noOptionsAllowed() {
String json =
"""
{
"insertOne": {
"document": {
"_id": "docWithOptions"
},
"options": {"setting":"abc"}
}
}
""";

given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
.contentType(ContentType.JSON)
.body(json)
.when()
.post(CollectionResource.BASE_PATH, namespaceName, collectionName)
.then()
.statusCode(200)
.body("data.document", is(nullValue()))
.body("status", is(nullValue()))
.body("errors", is(notNullValue()))
.body("errors", hasSize(2))
.body("errors[0].message", startsWith("Command accepts no options: InsertOneCommand"))
.body("errors[0].exceptionClass", is("JsonMappingException"))
.body("errors[1].message", startsWith("Command accepts no options: InsertOneCommand"))
.body("errors[1].errorCode", is("COMMAND_ACCEPTS_NO_OPTIONS"))
.body("errors[1].exceptionClass", is("JsonApiException"));
}

@Test
public void insertDuplicateDocument() {
String json =
Expand Down
Loading