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.

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 Number size limits (max 50 characters), enforcement #432

Merged
merged 20 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
63bd327
Add Jackson dep/version override to use 2.15.1
tatu-at-datastax May 17, 2023
a7d7b3b
Try with Jackson 2.15.2-SNAPSHOT
tatu-at-datastax May 19, 2023
213939e
Back to 2.15.1 from 2.15.2-SNAPSHOT, for some reason our CI can't acc…
tatu-at-datastax May 19, 2023
7ef3aef
Merge branch 'main' into tatu/172-number-len-limits
tatu-at-datastax May 22, 2023
da56aaf
Merge branch 'main' into tatu/172-number-len-limits
tatu-at-datastax May 23, 2023
7b1c9a2
Merge branch 'main' into tatu/172-number-len-limits
tatu-at-datastax May 31, 2023
3da83da
Update Jackson to 2.15.2 (to resolve record deser problem)
tatu-at-datastax May 31, 2023
7146f92
Merge branch 'main' into tatu/172-number-len-limits
tatu-at-datastax Jun 1, 2023
7cab08b
Add configuration of 50-chars-max-nunbers
tatu-at-datastax Jun 2, 2023
d775500
Add unit test to check low-level handling
tatu-at-datastax Jun 2, 2023
fe4a8e2
Add exception mapper to get proper API exception for constraints viol…
tatu-at-datastax Jun 2, 2023
48f1013
Add another unit test
tatu-at-datastax Jun 2, 2023
826ebc4
Merge branch 'main' into tatu/172-number-len-limits
tatu-at-datastax Jun 2, 2023
822ce5d
Update DSE dep to latest Stargate uses
tatu-at-datastax Jun 2, 2023
8bbdbe1
Udpate DseTestReosource wrt DSE 6.8.35 as well
tatu-at-datastax Jun 2, 2023
17b48a6
Minor streamlining of exception mapper
tatu-at-datastax Jun 2, 2023
cbaa3d2
Add one new IT
tatu-at-datastax Jun 2, 2023
6087134
Just cannot get Exception mapper to work, at all, no way no how. Drop…
tatu-at-datastax Jun 2, 2023
7171146
Merge branch 'main' into tatu/172-number-len-limits
tatu-at-datastax Jun 5, 2023
8a06aaa
Minor fix to ensure Metrics-validating tests run after other tests (h…
tatu-at-datastax Jun 5, 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
13 changes: 12 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
<version>1.0.0-ALPHA-10-SNAPSHOT</version>
<properties>
<stargate.version>${project.parent.version}</stargate.version>
<!-- 17-May-2023, tatu: [json-api#172] Need Jackson 2.15.x -->
<jackson.version>2.15.2</jackson.version>
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved
<failsafe.useModulePath>false</failsafe.useModulePath>
<!-- Please update github workflows that build docker images if changing image/additional tags -->
<quarkus.container-image.group>stargateio</quarkus.container-image.group>
Expand All @@ -20,7 +22,7 @@
<!-- Integration test props -->
<!-- When updating please change defaults in the DseTestResource class -->
<stargate.int-test.cassandra.image>datastax/dse-server</stargate.int-test.cassandra.image>
<stargate.int-test.cassandra.image-tag>6.8.34</stargate.int-test.cassandra.image-tag>
<stargate.int-test.cassandra.image-tag>6.8.35</stargate.int-test.cassandra.image-tag>
<stargate.int-test.coordinator.image>stargateio/coordinator-dse-68</stargate.int-test.coordinator.image>
<stargate.int-test.coordinator.image-tag>v${stargate.version}</stargate.int-test.coordinator.image-tag>
<stargate.int-test.cluster.name>dse-${stargate.int-test.cassandra.image-tag}-cluster</stargate.int-test.cluster.name>
Expand All @@ -30,6 +32,15 @@
</properties>
<dependencyManagement>
<dependencies>
<!-- 17-May-2023, tatu: [json-api#172] Need Jackson 2.15.x -->
<!-- NOTE: MUST come before Quarkus bom to have precedence -->
<dependency>
<groupId>com.fasterxml.jackson</groupId>
<artifactId>jackson-bom</artifactId>
<version>${jackson.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
<dependency>
<groupId>${quarkus.platform.group-id}</groupId>
<artifactId>${quarkus.platform.artifact-id}</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,27 @@
package io.stargate.sgv2.jsonapi.api.configuration;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.StreamReadConstraints;
import com.fasterxml.jackson.core.StreamReadFeature;
import com.fasterxml.jackson.core.StreamWriteFeature;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import io.quarkus.jackson.ObjectMapperCustomizer;
import io.stargate.sgv2.jsonapi.config.DocumentLimitsConfig;
import jakarta.enterprise.inject.Instance;
import jakarta.enterprise.inject.Produces;
import jakarta.inject.Singleton;

/** Configures the {@link ObjectMapper} instance that going to be injectable and used in the app. */
public class ObjectMapperConfiguration {

/** Replaces the CDI producer for ObjectMapper built into Quarkus. */
@Singleton
@Produces
ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) {
ObjectMapper mapper = createMapper();
ObjectMapper objectMapper(
DocumentLimitsConfig documentLimitsConfig, Instance<ObjectMapperCustomizer> customizers) {
ObjectMapper mapper = createMapper(documentLimitsConfig);

// apply all ObjectMapperCustomizer beans (incl. Quarkus)
for (ObjectMapperCustomizer customizer : customizers) {
Expand All @@ -28,13 +31,20 @@ ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) {
return mapper;
}

private ObjectMapper createMapper() {
return JsonMapper.builder()
private ObjectMapper createMapper(DocumentLimitsConfig documentLimitsConfig) {
int maxNumLen = documentLimitsConfig.maxNumberLength();

// Number token limit handled by lower-level parser factory, need to construct first:
JsonFactory jsonFactory =
JsonFactory.builder()
.streamReadConstraints(
StreamReadConstraints.builder().maxNumberLength(maxNumLen).build())
.build();
return JsonMapper.builder(jsonFactory)
// important for retaining number accuracy!
.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)

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

// Verify uniqueness of JSON Object properties
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@
*
* <p>Each command should validate itself using the <i>javax.validation</i> framework.
*/
@JsonTypeInfo(
use = JsonTypeInfo.Id.NAME,
include = JsonTypeInfo.As.WRAPPER_OBJECT,
property = "commandName")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"property" only used with As.PROPERTY, not with WRAPPER_OBJECT.

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.WRAPPER_OBJECT)
@JsonSubTypes({
@JsonSubTypes.Type(value = CountDocumentsCommands.class),
@JsonSubTypes.Type(value = CreateNamespaceCommand.class),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package io.stargate.sgv2.jsonapi.config;

import io.quarkus.runtime.annotations.StaticInitSafe;
import io.smallrye.config.ConfigMapping;
import io.smallrye.config.WithDefault;
import jakarta.validation.constraints.Positive;

/** Configuration Object that defines limits on Documents managed by JSON API. */
/**
* Configuration Object that defines limits on Documents managed by JSON API. Needed early for
* providers so has to be declared as {@link StaticInitSafe}.
*/
@StaticInitSafe
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved
@ConfigMapping(prefix = "stargate.jsonapi.document.limits")
public interface DocumentLimitsConfig {
/**
Expand Down Expand Up @@ -37,6 +42,11 @@ public interface DocumentLimitsConfig {
@WithDefault("64")
int maxObjectProperties();

/** @return Defines the maximum length of a single Number value (in characters). */
@Positive
@WithDefault("50")
int maxNumberLength();

/** @return Defines the maximum length of a single String value. */
@Positive
@WithDefault("16000")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
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.config.DocumentLimitsConfig;
import io.stargate.sgv2.jsonapi.exception.ErrorCode;
import jakarta.inject.Inject;
import java.util.List;
Expand All @@ -38,6 +39,8 @@ class ObjectMapperConfigurationTest {

@Inject ObjectMapper objectMapper;

@Inject DocumentLimitsConfig documentLimitsConfig;

@Nested
class FindOne {

Expand Down Expand Up @@ -199,6 +202,35 @@ public void failForNonEmptyOptions() throws Exception {
.hasMessageStartingWith(
ErrorCode.COMMAND_ACCEPTS_NO_OPTIONS.getMessage() + ": InsertOneCommand");
}

@Test
public void failForTooLongNumbers() {
String tooLongNumStr = "1234567890".repeat(6);
String json =
"""
{
"insertOne": {
"document": {
"_id" : 123,
"bigNumber" : %s
}
}
}
"""
.formatted(tooLongNumStr);

Exception e = catchException(() -> objectMapper.readValue(json, Command.class));
// Without exception mappers we just get default Jackson JsonMappingException wrapping the
// constraints violation exception
assertThat(e)
.isInstanceOf(JsonMappingException.class)
.hasMessageContaining(
"Number length ("
+ tooLongNumStr.length()
+ ") exceeds the maximum length ("
+ documentLimitsConfig.maxNumberLength()
+ ")");
}
}

@Nested
Expand Down
Loading