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

Extend error response with additional information #1325

Merged
merged 22 commits into from
Aug 13, 2024

Conversation

Hazel-Datastax
Copy link
Contributor

What this PR does:
Extend error response with additional information
The matching family and scope for each error code are here

Which issue(s) this PR fixes:
Fixes NA

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@Hazel-Datastax Hazel-Datastax requested a review from a team as a code owner August 1, 2024 16:26
@Hazel-Datastax
Copy link
Contributor Author

Hazel-Datastax commented Aug 1, 2024

If the extendError flag is enabled, the new error response will be like:

{
    "errors": [
        {
            "message": "\"insertOne1\" not one of \"CollectionCommand\"s: known commands are [countDocuments, deleteMany, deleteOne, estimatedDocumentCount, find, findOne, findOneAndDelete, findOneAndReplace, findOneAndUpdate, insertMany, insertOne, updateMany, updateOne]",
            "errorCode": "COMMAND_UNKNOWN",
            "scope": "",
            "id": "18819834-b12d-4395-b484-f6795c2467cc",
            "title": "Provided command unknown",
            "family": "REQUEST"
        }
    ]
}

By default, the flag is disabled, and the error response will not be changed:

{
    "errors": [
        {
            "message": "Provided command unknown: \"insertOne1\" not one of \"CollectionCommand\"s: known commands are [countDocuments, deleteMany, deleteOne, estimatedDocumentCount, find, findOne, findOneAndDelete, findOneAndReplace, findOneAndUpdate, insertMany, insertOne, updateMany, updateOne]",
            "errorCode": "COMMAND_UNKNOWN"
        }
    ]
}

@@ -803,7 +802,7 @@ public void happyPathWithEmptyVector() {
.body("errors", hasSize(1))
.body("errors[0].exceptionClass", is("JsonApiException"))
.body("errors[0].errorCode", is("SHRED_BAD_VECTOR_SIZE"))
.body("errors[0].message", is(ErrorCode.SHRED_BAD_VECTOR_SIZE.getMessage()));
.body("errors[0].message", is("$vector value can't be empty"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this was changed? Earlier code more robust wrt changes to error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I added extendError - the configuration in the ErrorCode class. It will cause the class initialization error in this case - because it cannot find the config

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. We will probably change things to ensure that no loading occurs from within static initialization of ErrorCode (or its replacement) in future.
But is fine for now.

Copy link
Contributor

@amorton amorton left a comment

Choose a reason for hiding this comment

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

couple of changes ot the way we are creating the family / scope - but I am still happy with the general approach rather than changing all of the existing ErrorCode enum

@Hazel-Datastax Hazel-Datastax merged commit b5402aa into main Aug 13, 2024
3 checks passed
@Hazel-Datastax Hazel-Datastax deleted the hazel/refactor_exception branch August 13, 2024 19:00
PROJECTION("PROJECTION"),
SCHEMA("SCHEMA"),
SORT("SORT"),
UPDATE("UPDATE");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there also be an error scope for OPTIONS if you pass in an invalid option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants