Ajm/fix 2318 collections driver exception handler #2333
Conversation
| var builder = CommandErrorV2.builder(); | ||
|
|
||
| if (debugEnabled) { | ||
| builder.errorClass(jsonApiException.getClass().getSimpleName()); |
There was a problem hiding this comment.
nit: could move logic to builder, which would take Throwable, access debugEnabled on its own, add e.getClass().getSimpleName() if debug enabled.
| .build(); | ||
| } | ||
|
|
||
| public CommandErrorV2 create(APIException apiException) { |
There was a problem hiding this comment.
Seems redundant as we already have:
public CommandErrorV2 create(Throwable throwable);
which calls the main
public CommandErrorV2 create(Throwable throwable, List<? extends DocRowIdentifer> documentIds)
which calls Yet Another create() method...
|
|
||
| public String getTitle() { | ||
| return title; | ||
| // XXX TO COMMENT |
There was a problem hiding this comment.
Add the comment? :)
Esp. since I am not quite sure what the logic is wrt "identity" in this case: was about to suggest implementation on top of equals() but that's not what is being done here.
| this.documentIds = | ||
| documentIds == null | ||
| ? Collections.emptyList() | ||
| : documentIds.stream().map(d -> (DocRowIdentifer) d).toList(); |
There was a problem hiding this comment.
Is this just for creating a copy of List? I guess it is to "convert" from List<? extends DocRowIdentifer> to List< DocRowIdentifer>.
tatu-at-datastax
left a comment
tatu-at-datastax
left a comment
There was a problem hiding this comment.
LGTM: a few nits, typos, questions.
| // another builder, because if we add a warning we want to use the V2 error object | ||
| // but may not be returning V2 errors in the result | ||
| private final APIExceptionCommandErrorBuilder apiWarningToError; | ||
| // amorton - we could probably use the same factory for errors and warning, keeping seperate |
There was a problem hiding this comment.
typo: seperate -> separate
| } | ||
|
|
||
| /** | ||
| * Enables debug for the duration of a try-with-resources block. |
There was a problem hiding this comment.
Why is this needed?
| return commandResult.toRestResponse(); | ||
| } | ||
| } | ||
| // public class GenericExceptionMapper { |
| return builder.build(); | ||
| } | ||
| } | ||
| // public record ThrowableCommandResultSupplier(Throwable t) implements Supplier<CommandResult> { |
| * | ||
| * <p>Changed the role to ONLY be remaping Java exception to exception. Previously it would also | ||
| * build the Command error (CommandResult.Error) that is returned to the client. Made the change to | ||
| * try simplifying as I work to remove this class completly. |
There was a problem hiding this comment.
typo: completly -> completely
| import org.jboss.resteasy.reactive.server.ServerExceptionMapper; | ||
|
|
||
| /** Tries to omit the `WebApplicationException` and just report the cause. */ | ||
| /** Tries to omit the `WebApplicationException` and just report the cause. <p/ */ |
| // Cannot get a session from the sessionCacheSupplier in the constructor because | ||
| // it will create a circular call. So need to wait until now to create the QueryExecutor | ||
| // this is OK, only happens when the table is not in the cache | ||
| // HACK - fix before merge |
| map -> | ||
| map.put( | ||
| "errorMessage", | ||
| "Collection creation failure (unable to create table).")))); |
There was a problem hiding this comment.
nit: usually trailing period part of template, not substitution part.
| // is error | ||
| tags.add(ExceptionMetrics.TAG_ERROR_TRUE); | ||
| // let error bubble, there must be a first error | ||
| // XXX AARON TODO - old code use a single error, can we use more than one tag value ? Cannot |
There was a problem hiding this comment.
typo: old code use -> old code uses
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist