-
Notifications
You must be signed in to change notification settings - Fork 16
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
Failure modes for update and delete operations #237
Conversation
Aggregate errors by type as per spec
Aggregate errors by type for Delete and Update.
Aggregate errors by type for Delete and Update. Test case for multiple document failure
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/DeleteOperationPage.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/UpdateOperationPage.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/UpdateOperationPage.java
Outdated
Show resolved
Hide resolved
String message = | ||
"Failed to update document with _id %s: %s".formatted(documentId, throwable.getMessage()); | ||
"Failed to update documents with _id %s: %s".formatted(documentIds, throwable.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that similar to exception key, formatting of documentIds probably should be a shared utility method since we may want to add quoting/escaping at some point -- start with just List.toString()
for now, but I have found that it is good to distinguish f.ex between number 1
and String "1"
... especially as String keys can have all kind of content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think of this, since the only value where simple "value.toString()" may not be sufficient is String
(where we'd want to surround value in quotes), we could easily change toString()
method for DocumentId.StringId
and the message would be changed?
Downside is that there are lots of tests that compare exact exception message.
I can do a PR after merging this one so as not to extend scope here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What this PR does:
Delete and Update operation - fail silently
Which issue(s) this PR fixes:
Fixes #214
Checklist