Skip to content

Commit

Permalink
Fixes #873: improve Bean Validation failure messages (#874)
Browse files Browse the repository at this point in the history
  • Loading branch information
@tatu-at-datastax
tatu-at-datastax authored Feb 14, 2024
1 parent a103fbf commit 7e11cbb
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 40 deletions.
34 changes: 17 additions & 17 deletions docs/jsonapi-spec.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -532,11 +532,11 @@ See [Multi-Document Failure Considerations](#multi-document-failure-consideratio

#### countDocuments Command Response

| Response Elements | Description |
| ----------------- | ------------------------------------------------------- |
| `data` | Not present |
| `status` | Preset with fields: `count: <zero-or-positive-integer>` |
| `errors` | Present if errors occur. |
| Response Elements | Description |
| ----------------- |----------------------------------------------------------|
| `data` | Not present |
| `status` | Present with fields: `count: <zero-or-positive-integer>` |
| `errors` | Present if errors occur. |

If an error occurs the command will not return `status`.

Expand Down Expand Up @@ -683,11 +683,11 @@ The maximum amount of documents that can be deleted in a single operation is 20.

#### deleteMany Command Response

| Response Elements | Description |
| ----------------- | ------------------------------------------------------------------------------------ |
| `data` | Not present |
| `status` | Preset with fields: `deletedCount: <zero-or-positive-integer>`, `moreData` with value `true` if there are more documents that match the delete `filter`, but were not deleted since the limit of documents to delete in the single operation was hit. |
| `errors` | Present if errors occur. |
| Response Elements | Description |
| ----------------- |--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `data` | Not present |
| `status` | Present with fields: `deletedCount: <zero-or-positive-integer>`, `moreData` with value `true` if there are more documents that match the delete `filter`, but were not deleted since the limit of documents to delete in the single operation was hit. |
| `errors` | Present if errors occur. |

If an error occurs the command will still return `status` as some documents may have been deleted.

Expand All @@ -714,11 +714,11 @@ See [Multi-Document Failure Considerations](#multi-document-failure-consideratio

#### deleteOne Command Response

| Response Elements | Description |
| ----------------- | --------------------------------------------------------- |
| `data` | Not present |
| `status` | Preset with fields: `deletedCount: <zero-or-one-integer>` |
| `errors` | Present if errors occur. |
| Response Elements | Description |
| ----------------- |------------------------------------------------------------|
| `data` | Not present |
| `status` | Present with fields: `deletedCount: <zero-or-one-integer>` |
| `errors` | Present if errors occur. |

If an error occurs the command will not return `status`.

Expand Down Expand Up @@ -905,7 +905,7 @@ If `<sort-clause>` is present, the maximum amount of documents that could be sor
| Response Elements | Description |
|-------------------|----------------------------------------------------------------------------------------------------------------------------------------------------|
| `data` | Present with fields : `document` only, see [findOneAndReplace Command Options](#findOneAndReplace-command-options) for controlling the projection. |
| `status` | Preset with fields: `upsertedId: <id of document upserted>`, if a document was upserted. |
| `status` | Present with fields: `upsertedId: <id of document upserted>`, if a document was upserted. |
| `errors` | Present if errors occur. |

If an error occurs the command will not return `data` or `status`.
Expand Down Expand Up @@ -980,7 +980,7 @@ If `<sort-clause>` is present, the maximum amount of documents that could be sor
| Response Elements | Description |
|-------------------|--------------------------------------------------------------------------------------------------------------------------------------------------|
| `data` | Present with fields : `document` only, see [findOneAndUpdate Command Options](#findOneAndUpdate-command-options) for controlling the projection. |
| `status` | Preset with fields: `upsertedId: <json-value>` only if a document was upserted. |
| `status` | Present with fields: `upsertedId: <json-value>` only if a document was upserted. |
| `errors` | Present if errors occur. |


Expand Down
16 changes: 13 additions & 3 deletions ...n/java/io/stargate/sgv2/jsonapi/exception/mappers/ConstraintViolationExceptionMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import io.stargate.sgv2.jsonapi.api.model.command.CommandResult;
import jakarta.validation.ConstraintViolation;
import jakarta.validation.ConstraintViolationException;
import jakarta.validation.Path;
import jakarta.ws.rs.core.Response;
import java.util.List;
import java.util.Map;
Expand All @@ -13,6 +12,7 @@

/** Simple exception mapper for the {@link ConstraintViolationException}. */
public class ConstraintViolationExceptionMapper {
private static final String PREFIX_POST_COMMAND = "postCommand.";

// constant error fields
public static final Map<String, Object> ERROR_FIELDS =
Expand All @@ -36,8 +36,18 @@ public RestResponse<CommandResult> constraintViolationException(

private static CommandResult.Error getError(ConstraintViolation<?> violation) {
String message = violation.getMessage();
Path propertyPath = violation.getPropertyPath();
String msg = "Request invalid, the field %s not valid: %s.".formatted(propertyPath, message);
String propertyPath = violation.getPropertyPath().toString();
// Let's remove useless "postCommand." prefix if seen
if (propertyPath.startsWith(PREFIX_POST_COMMAND)) {
propertyPath = propertyPath.substring(PREFIX_POST_COMMAND.length());
}

Object propertyValue = violation.getInvalidValue();
String propertyValueDesc =
(propertyValue == null) ? "`null`" : String.format("\"%s\"", propertyValue);
String msg =
"Request invalid: field '%s' value %s not valid. Problem: %s."
.formatted(propertyPath, propertyValueDesc, message);
return new CommandResult.Error(msg, ERROR_FIELDS, ERROR_FIELDS_METRICS_TAG, Response.Status.OK);
}
}
22 changes: 12 additions & 10 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/CollectionResourceIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ public void malformedBody() {
.post(CollectionResource.BASE_PATH, namespaceName, collectionName)
.then()
.statusCode(200)
.body("errors[0].message", is(not(blankString())))
.body("errors[0].exceptionClass", is("JsonParseException"));
.body("errors[0].exceptionClass", is("JsonParseException"))
.body("errors[0].message", is(not(blankString())));
}

@Test
Expand All @@ -69,10 +69,10 @@ public void unknownCommand() {
.post(CollectionResource.BASE_PATH, namespaceName, collectionName)
.then()
.statusCode(200)
.body("errors[0].errorCode", is("NO_COMMAND_MATCHED"))
.body(
"errors[0].message",
startsWith("No \"unknownCommand\" command found as \"CollectionCommand\""))
.body("errors[0].errorCode", is("NO_COMMAND_MATCHED"));
startsWith("No \"unknownCommand\" command found as \"CollectionCommand\""));
}

@Test
Expand All @@ -94,10 +94,11 @@ public void invalidNamespaceName() {
.post(CollectionResource.BASE_PATH, "7_no_leading_number", collectionName)
.then()
.statusCode(200)
.body("errors[0].exceptionClass", is("ConstraintViolationException"))
.body(
"errors[0].message",
startsWith("Request invalid, the field postCommand.namespace not valid"))
.body("errors[0].exceptionClass", is("ConstraintViolationException"));
startsWith(
"Request invalid: field 'namespace' value \"7_no_leading_number\" not valid. Problem: must match "));
}

@Test
Expand All @@ -119,10 +120,11 @@ public void invalidCollectionName() {
.post(CollectionResource.BASE_PATH, namespaceName, "7_no_leading_number")
.then()
.statusCode(200)
.body("errors[0].exceptionClass", is("ConstraintViolationException"))
.body(
"errors[0].message",
startsWith("Request invalid, the field postCommand.collection not valid"))
.body("errors[0].exceptionClass", is("ConstraintViolationException"));
startsWith(
"Request invalid: field 'collection' value \"7_no_leading_number\" not valid. Problem: must match "));
}

@Test
Expand All @@ -134,8 +136,8 @@ public void emptyBody() {
.post(CollectionResource.BASE_PATH, namespaceName, collectionName)
.then()
.statusCode(200)
.body("errors[0].message", is(not(blankString())))
.body("errors[0].exceptionClass", is("ConstraintViolationException"));
.body("errors[0].exceptionClass", is("ConstraintViolationException"))
.body("errors[0].message", is(not(blankString())));
}
}
}
17 changes: 9 additions & 8 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/NamespaceResourceIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ public void malformedBody() {
.post(NamespaceResource.BASE_PATH, namespaceName)
.then()
.statusCode(200)
.body("errors[0].message", is(not(blankString())))
.body("errors[0].exceptionClass", is("JsonParseException"));
.body("errors[0].exceptionClass", is("JsonParseException"))
.body("errors[0].message", is(not(blankString())));
}

@Test
Expand All @@ -66,10 +66,10 @@ public void unknownCommand() {
.post(NamespaceResource.BASE_PATH, namespaceName)
.then()
.statusCode(200)
.body("errors[0].errorCode", is("NO_COMMAND_MATCHED"))
.body(
"errors[0].message",
startsWith("No \"unknownCommand\" command found as \"NamespaceCommand\""))
.body("errors[0].errorCode", is("NO_COMMAND_MATCHED"));
startsWith("No \"unknownCommand\" command found as \"NamespaceCommand\""));
}

@Test
Expand All @@ -91,10 +91,11 @@ public void invalidNamespaceName() {
.post(NamespaceResource.BASE_PATH, "7_no_leading_number")
.then()
.statusCode(200)
.body("errors[0].exceptionClass", is("ConstraintViolationException"))
.body(
"errors[0].message",
startsWith("Request invalid, the field postCommand.namespace not valid"))
.body("errors[0].exceptionClass", is("ConstraintViolationException"));
startsWith(
"Request invalid: field 'namespace' value \"7_no_leading_number\" not valid. Problem: must match "));
}

@Test
Expand All @@ -106,8 +107,8 @@ public void emptyBody() {
.post(NamespaceResource.BASE_PATH, namespaceName)
.then()
.statusCode(200)
.body("errors[0].message", is(not(blankString())))
.body("errors[0].exceptionClass", is("ConstraintViolationException"));
.body("errors[0].exceptionClass", is("ConstraintViolationException"))
.body("errors[0].message", is(not(blankString())));
}
}
}
2 changes: 1 addition & 1 deletion src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateManyIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ public void invalidCommand() {
.body(
"errors[0].message",
is(
"Request invalid, the field postCommand.command.updateClause not valid: must not be null."));
"Request invalid: field 'command.updateClause' value `null` not valid. Problem: must not be null."));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/io/stargate/sgv2/jsonapi/api/v1/UpdateOneIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2042,7 +2042,7 @@ public void invalidCommand() {
.body(
"errors[0].message",
is(
"Request invalid, the field postCommand.command.updateClause not valid: must not be null."));
"Request invalid: field 'command.updateClause' value `null` not valid. Problem: must not be null."));
}

@Test
Expand Down

0 comments on commit 7e11cbb

Please sign in to comment.