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

closes #293: allow empty options object in all commands #310

Merged
merged 2 commits into from
Mar 30, 2023
Merged

closes #293: allow empty options object in all commands #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0202362
closes #293: allow empty options object in all commands
Mar 29, 2023
226475f
added test in all int tests, new round of refactoring of int tests
Mar 29, 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
7 changes: 7 additions & 0 deletions src/main/java/io/stargate/sgv2/jsonapi/api/configuration/ObjectMapperConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,21 @@ ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) {

private ObjectMapper createMapper() {
return JsonMapper.builder()

// important for retaining number accuracy!
.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)

// case insensitive enums, so "before" will match to "BEFORE" in an enum
.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS)

// Verify uniqueness of JSON Object properties
.enable(StreamReadFeature.STRICT_DUPLICATE_DETECTION)

// Prevent use of Engineering Notation with trailing zeroes:
.enable(StreamWriteFeature.WRITE_BIGDECIMAL_AS_PLAIN)

// don't fail on unknown props, mainly for empty options object in commands
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
Comment on lines +46 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tatu-at-datastax any better idea?

Copy link
Contributor

@tatu-at-datastax tatu-at-datastax Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could (and probably should) use

 @JsonIgnoreProperties({ "options" })
 public interface Command {}

in a base class/interface. It can be applied either with annotations or using "configOverrides" (let me dig up reference), or using mix-in annotations.
This type of ignoral is only used in case there is no explicit property (unlike @JsonIgnore that forcibly prevents use) so it should not prevent use of non-empty "ignores" cases.

It'd probably be better to do that so we don't accidentally hide properties with typos etc.

Also: we could enable "fail all unknown" just on Command types with

 @JsonIgnoreProperties(ignoreUnknown = true)

and not globally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so "Config overrides" work like so:

        ObjectMapper mapper = JsonMapper.builder()
            .withConfigOverride(Point.class,
                cfg -> cfg.setIgnorals(JsonIgnoreProperties.Value.forIgnoredProperties("y")))
            .build();

and let one apply equivalent of @JsonIgnoreProperties on any type. But one big limitation is that it only works on exact target class, but not base class.

So I think what we can use is:

@JsonIgnoreProperties({ "options" })
public interface Command {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 50a276f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proposal does not work, it completely omits options even in the commands that should have them.. See https://github.com/stargate/jsonapi/actions/runs/4562316051/jobs/8049363613

Reverting to previous solution..

Copy link
Contributor

Choose a reason for hiding this comment

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

Alas. Sorry about suggesting that; you are right.

.build();
}
}
95 changes: 47 additions & 48 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/CountIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ private void insert(String json) {
}

@Test
@Order(2)
public void countNoFilter() {
public void noFilter() {
String json =
"""
{
Expand All @@ -139,8 +138,30 @@ public void countNoFilter() {
}

@Test
@Order(2)
public void countByColumn() {
public void emptyOptionsAllowed() {
String json =
"""
{
"countDocuments": {
"options": {}
}
}
""";

given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
.contentType(ContentType.JSON)
.body(json)
.when()
.post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName)
.then()
.statusCode(200)
.body("status.count", is(5))
.body("errors", is(nullValue()));
}

@Test
public void byColumn() {
String json =
"""
{
Expand All @@ -164,8 +185,7 @@ public void countByColumn() {
}

@Test
@Order(2)
public void countWithEqComparisonOperator() {
public void withEqComparisonOperator() {
String json =
"""
{
Expand All @@ -189,8 +209,7 @@ public void countWithEqComparisonOperator() {
}

@Test
@Order(2)
public void countWithEqSubDoc() {
public void withEqSubDoc() {
String json =
"""
{
Expand All @@ -214,8 +233,7 @@ public void countWithEqSubDoc() {
}

@Test
@Order(2)
public void countWithEqSubDocWithIndex() {
public void withEqSubDocWithIndex() {
String json =
"""
{
Expand All @@ -239,8 +257,7 @@ public void countWithEqSubDocWithIndex() {
}

@Test
@Order(2)
public void countWithEqArrayElement() {
public void withEqArrayElement() {
String json =
"""
{
Expand All @@ -264,8 +281,7 @@ public void countWithEqArrayElement() {
}

@Test
@Order(2)
public void countWithExistFalseOperator() {
public void withExistFalseOperator() {
String json =
"""
{
Expand All @@ -287,8 +303,7 @@ public void countWithExistFalseOperator() {
}

@Test
@Order(2)
public void countWithExistOperator() {
public void withExistOperator() {
String json =
"""
{
Expand All @@ -312,8 +327,7 @@ public void countWithExistOperator() {
}

@Test
@Order(2)
public void countWithAllOperator() {
public void withAllOperator() {
String json =
"""
{
Expand All @@ -337,8 +351,7 @@ public void countWithAllOperator() {
}

@Test
@Order(2)
public void countWithAllOperatorLongerString() {
public void withAllOperatorLongerString() {
String json =
"""
{
Expand All @@ -362,8 +375,7 @@ public void countWithAllOperatorLongerString() {
}

@Test
@Order(2)
public void countWithAllOperatorMixedAFormatArray() {
public void withAllOperatorMixedAFormatArray() {
String json =
"""
{
Expand All @@ -387,8 +399,7 @@ public void countWithAllOperatorMixedAFormatArray() {
}

@Test
@Order(2)
public void countWithAllOperatorNoMatch() {
public void withAllOperatorNoMatch() {
String json =
"""
{
Expand All @@ -412,8 +423,7 @@ public void countWithAllOperatorNoMatch() {
}

@Test
@Order(2)
public void countWithEqSubDocumentShortcut() {
public void withEqSubDocumentShortcut() {
String json =
"""
{
Expand All @@ -437,8 +447,7 @@ public void countWithEqSubDocumentShortcut() {
}

@Test
@Order(2)
public void countWithEqSubDocument() {
public void withEqSubDocument() {
String json =
"""
{
Expand All @@ -462,8 +471,7 @@ public void countWithEqSubDocument() {
}

@Test
@Order(2)
public void countWithEqSubDocumentOrderChangeNoMatch() {
public void withEqSubDocumentOrderChangeNoMatch() {
String json =
"""
{
Expand All @@ -487,8 +495,7 @@ public void countWithEqSubDocumentOrderChangeNoMatch() {
}

@Test
@Order(2)
public void countWithEqSubDocumentNoMatch() {
public void withEqSubDocumentNoMatch() {
String json =
"""
{
Expand All @@ -512,8 +519,7 @@ public void countWithEqSubDocumentNoMatch() {
}

@Test
@Order(2)
public void countWithSizeOperator() {
public void withSizeOperator() {
String json =
"""
{
Expand All @@ -537,8 +543,7 @@ public void countWithSizeOperator() {
}

@Test
@Order(2)
public void countWithSizeOperatorNoMatch() {
public void withSizeOperatorNoMatch() {
String json =
"""
{
Expand All @@ -562,8 +567,7 @@ public void countWithSizeOperatorNoMatch() {
}

@Test
@Order(2)
public void countWithEqOperatorArray() {
public void withEqOperatorArray() {
String json =
"""
{
Expand All @@ -587,8 +591,7 @@ public void countWithEqOperatorArray() {
}

@Test
@Order(2)
public void countWithEqOperatorNestedArray() {
public void withEqOperatorNestedArray() {
String json =
"""
{
Expand All @@ -612,8 +615,7 @@ public void countWithEqOperatorNestedArray() {
}

@Test
@Order(2)
public void countWithEqOperatorArrayNoMatch() {
public void withEqOperatorArrayNoMatch() {
String json =
"""
{
Expand All @@ -637,8 +639,7 @@ public void countWithEqOperatorArrayNoMatch() {
}

@Test
@Order(2)
public void countWithEqOperatorNestedArrayNoMatch() {
public void withEqOperatorNestedArrayNoMatch() {
String json =
"""
{
Expand All @@ -662,8 +663,7 @@ public void countWithEqOperatorNestedArrayNoMatch() {
}

@Test
@Order(2)
public void countWithNEComparisonOperator() {
public void withNEComparisonOperator() {
String json =
"""
{
Expand All @@ -685,8 +685,7 @@ public void countWithNEComparisonOperator() {
}

@Test
@Order(2)
public void countByBooleanColumn() {
public void byBooleanColumn() {
String json =
"""
{
Expand Down
41 changes: 34 additions & 7 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/DeleteManyIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private void insert(int countOfDocument) {
}

@Test
public void deleteManyById() {
public void byId() {
insert(2);
String json =
"""
Expand Down Expand Up @@ -129,7 +129,34 @@ public void deleteManyById() {
}

@Test
public void deleteManyByColumn() {
public void emptyOptionsAllowed() {
insert(2);
String json =
"""
{
"deleteMany": {
"filter" : {"_id" : "doc1"},
"options": {}
}
}
""";

given()
.header(HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME, getAuthToken())
.contentType(ContentType.JSON)
.body(json)
.when()
.post(CollectionResource.BASE_PATH, keyspaceId.asInternal(), collectionName)
.then()
.statusCode(200)
.body("status.deletedCount", is(1))
.body("status.moreData", is(nullValue()))
.body("data", is(nullValue()))
.body("errors", is(nullValue()));
}

@Test
public void byColumn() {
insert(5);
String json =
"""
Expand Down Expand Up @@ -177,7 +204,7 @@ public void deleteManyByColumn() {
}

@Test
public void deleteManyNoFilter() {
public void noFilter() {
insert(20);
String json =
"""
Expand Down Expand Up @@ -222,7 +249,7 @@ public void deleteManyNoFilter() {
}

@Test
public void deleteManyNoFilterMoreDataFlag() {
public void noFilterMoreDataFlag() {
insert(25);
String json =
"""
Expand Down Expand Up @@ -278,9 +305,9 @@ public void concurrentDeletes() throws Exception {
String document =
"""
{
"_id": "concurrent-%s"
}
""";
"_id": "concurrent-%s"
}
""";
for (int i = 0; i < totalDocuments; i++) {
insertDoc(document.formatted(i));
}
Expand Down
Loading