7134 refactor errorhandlers

[poikilotherm]

What this PR does / why we need it:

• This refactors the API error handlers to be more relaxed about non-special-treatment Jersey exceptions.
• It also removes boilerplate and duplicated code from the package.
• It takes care of a standardized response format for errors (which should be adapted in other places to be consistent throughout our beloved API).
• It streamlines the logging of exceptions by enabling debugging and proper parsing in log analysis.

Which issue(s) this PR closes:

Closes #7134

Special notes for your reviewer:

I do hope the scope is not to big. I did not change the remaining two exception handlers for JSON and constraints to keep it smaller. Happy to change those if you think that makes sense.

One might also argue that special messages for Error 403 and 405 are not of much use. Happy to delete those.

I could also switch the Jersey Error 500 message to be the same as in ThrowableHandler.

The 302 and 307 were added because sometimes you might want to see a proper message via API, for example when you don't auto-follow redirects with curl et al.

I'm not sure if we should add a comment about the logging to the docs.

Suggestions on how to test this:

After deployment:

• Try debugging verbosity: asadmin set-log-levels edu.harvard.iq.dataverse.api.errorhandlers.ThrowableHandler=FINEST and go for a 404 error
• Not sure how to generate a 500 error...
• A 404, 405 and 406 are easy to test
• A 302/307 is not commonly used in Dataverse. The only place is for ZIP downloads. So to test this, you'll have to enable those. I created a unit test for this during development, but deleted it as it wasn't of that much use...
• No idea how to trigger a 503 error in Jersey...

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Ah API isn't classic UI, so let's say no.

Is there a release notes update needed for this change?:

Nope.

Additional documentation:

None.

[coveralls]

Coverage increased (+0.03%) to 19.64% when pulling a08b290 on poikilotherm:7134-refactor-errorhandlers into baa521d on IQSS:develop.

[qqmyers]

@poikilotherm - FWIW: redirects happen when you use direct downloads with S3. They are a normal part of operation so it seems odd to respond to the user with any form of incident id, etc.

[poikilotherm]

@qqmyers I could create a simple filter that removes the JSON entity for such cases like 302/307 redirects, although it should be configurable. When you try to debug things with curl, it might be beneficial to see a more verbose output. On the other hand: there are the logs... I see valid scenarios for both cases.

More people post their opinions where to go from here?

Another idea: incidentId, method, url and errorType in the JSON response could be limited to the 400 and 500 scopes of errors (client and server errors)

[qqmyers]

The underlying issue (w.r.t. 5.0) here is that something that should be a 406 response is getting returned as a 500 error.

This PR, as is, will change the error responses for many of the API calls - in a good way/to give more info to humans/standardize a json error body to work with, but that seems like a significant API change that could impact external tools (hopefully not - I know the DVUploader mostly deals with the response status and doesn't do much more than print any message, and with no standardization across the API, perhaps other tools haven't done much to parse the error messages either). I don't know how big a deal this is in practice but if people are concerned, it might not be something to push into 5.0.

Conversely, the underlying refactoring, to catch all WebApplicationExceptions before they get to the ThrowableHandler could avoid errors we haven't yet discovered (other error codes that shouldn't be converted to 500 errors). (And removes a lot of duplicate code - also good.)

How much work would it be to avoid changing the external API responses for now / split the PR into a 5.0 fix and a post-5.0 API change? Is that a possibility?

[poikilotherm]

OK let's look at the details of responses:

Feature	Before PR	With PR
All handlers except RedirectionException return JSON	👍	👍
All handlers have proper error code in Response + HTTP header	⛔ (broken with catchall)	👍
status == ERROR	👍	👍
code == XXX, XXX = int	👍	👍
HTTP codes correspond to intention in code	⛔	👍
Responses with 500 have JSON field incidentId	👍	👍
message field has some special messages for some cases like 403 etc	👍	🤏🏼 (same logic, but now without URI included)

So the most important fields like status, code and incidentId stayed stable. (It's debatable if it makes sense to extend the usage of incidentId, but that doesn't make it unstable.) The returned HTTP status codes stayed stable and are now aligned with the codes intention (like having a 406, not a 500).

The new fields for method, url, errorType are debatable, but extensions, thus not making things unstable. Happy to filter those to be included in cases of HTTP status codes > 400. (< 400 is not really an error)

I'm very confident no one will rely on parsing a message to have machine actionable activities in their client code. If so, 5.0 is a major release allowing for breaking changes.

I'm with you in terms of this needs polishing to be perfect, but I wouldn't consider it unstable. And even if - it's a release with breaking changes anyway. Happy to add stuff to API docs if you think this is missing. I also agree that streamlining the API responses becomes more important. There are a lot of places that could use a dust-off in the API code in terms of responses...

[qqmyers]

Thanks for the detail - less concerning given that the changes are mostly to add fields instead of changing them (minor note: I'm not sure things like 406 were json prior to ThrowableHandler though, but that's not a run-time issue - if you get that you fix your code).
I think it would be worthwhile to filter for <400 but it might be better to wait for other opinions on that.

As for docs, I haven't checked what's there now but it would probably be good to have info about the standardized response format you're implementing.

[qqmyers]

@poikilotherm - had some discussion - can you go ahead and filter for <400 and document the standard json format for responses to finish this off?

[poikilotherm]

Sure. But not today 😄 It's late over here 😉

Should I refactor the two other handlers, too to use the new common structure? Thinking of pushing this to a utility class, too.

Lemme know what you think

[poikilotherm]

@qqmyers I just pushed my latest code change. I created a new factory/decorator pattern based response formatter, much like the javax.ws.rs.core.Response.ResponseBuilder, but enhanced with fancy JSON. (I didn't want to inherit from the abstract base class for a complete custom ResponseBuilder, which would be a lot overhead for this.)

If you think this is feasible, I would provide developer guide updates on this. There are a million places inside the API code where things would need to be changed to this new builder (or just let the exception make its way) to streamline things, but this is definitely beyond scope for this issue/pull request.

EDIT:
I just learned about edu.harvard.iq.dataverse.api.AbstractApiBean methods to generate responses and AbstractApiBean.WrappedResponse. I truely believe those should be moved and refactored to use JAX-RS core exceptions and let those make their way to handlers.

Lots of this code was done by @michbarsinai between 2014 and 2016. Should we ping him and/or someone else about what's happening here?

[qqmyers]

Looks good. Some info for the API guide on the json structure would be useful. W.r.t. lots of places to change the code - changing the AbstractApiBean.error() method to use you JSON builder (iff error() only handles status 400+) would catch lots of cases. For those using exceptions now - the only reason to have your new code is to be able to catch those, so I'm not sure it makes sense to think about having other code avoid throwing them.

[qqmyers]

Since this is only for errors, could/should it be JSONErrorResponseBuilder?

[poikilotherm]

I would like to keep this open minded. For now its errors only, but it would be great not only to streamline error responses, but success stories, too. Like seen in ok(), ... at

dataverse/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java

Lines 652 to 747 in 352b17f

	protected Response ok( JsonArrayBuilder bld ) {
	return Response.ok(Json.createObjectBuilder()
	.add("status", STATUS_OK)
	.add("data", bld).build()).build();
	}
	protected Response ok( JsonObjectBuilder bld ) {
	return Response.ok( Json.createObjectBuilder()
	.add("status", STATUS_OK)
	.add("data", bld).build() )
	.type(MediaType.APPLICATION_JSON)
	.build();
	}
	protected Response ok( String msg ) {
	return Response.ok().entity(Json.createObjectBuilder()
	.add("status", STATUS_OK)
	.add("data", Json.createObjectBuilder().add("message",msg)).build() )
	.type(MediaType.APPLICATION_JSON)
	.build();
	}
	protected Response ok( boolean value ) {
	return Response.ok().entity(Json.createObjectBuilder()
	.add("status", STATUS_OK)
	.add("data", value).build() ).build();
	}
	/**
	* @param data Payload to return.
	* @param mediaType Non-JSON media type.
	* @return Non-JSON response, such as a shell script.
	*/
	protected Response ok(String data, MediaType mediaType) {
	return Response.ok().entity(data).type(mediaType).build();
	}
	protected Response created( String uri, JsonObjectBuilder bld ) {
	return Response.created( URI.create(uri) )
	.entity( Json.createObjectBuilder()
	.add("status", "OK")
	.add("data", bld).build())
	.type(MediaType.APPLICATION_JSON)
	.build();
	}
	protected Response accepted(JsonObjectBuilder bld) {
	return Response.accepted()
	.entity(Json.createObjectBuilder()
	.add("status", STATUS_WF_IN_PROGRESS)
	.add("data",bld).build()
	).build();
	}
	protected Response accepted() {
	return Response.accepted()
	.entity(Json.createObjectBuilder()
	.add("status", STATUS_WF_IN_PROGRESS).build()
	).build();
	}
	protected Response notFound( String msg ) {
	return error(Status.NOT_FOUND, msg);
	}
	protected Response badRequest( String msg ) {
	return error( Status.BAD_REQUEST, msg );
	}
	protected Response forbidden( String msg ) {
	return error( Status.FORBIDDEN, msg );
	}
	protected Response badApiKey( String apiKey ) {
	return error(Status.UNAUTHORIZED, (apiKey != null ) ? "Bad api key " : "Please provide a key query parameter (?key=XXX) or via the HTTP header " + DATAVERSE_KEY_HEADER_NAME );
	}
	protected Response permissionError( PermissionException pe ) {
	return permissionError( pe.getMessage() );
	}
	protected Response permissionError( String message ) {
	return unauthorized( message );
	}
	protected Response unauthorized( String message ) {
	return error( Status.UNAUTHORIZED, message );
	}
	protected static Response error( Status sts, String msg ) {
	return Response.status(sts)
	.entity( NullSafeJsonBuilder.jsonObjectBuilder()
	.add("status", STATUS_ERROR)
	.add( "message", msg ).build()
	).type(MediaType.APPLICATION_JSON_TYPE).build();
	}

[qqmyers]

per my review comment of changing the AbstractApiBean.error method - would it make sense to move these status-specific messages to the JSON builder? That way they'd apply to calls from error() as well versus only happening on the exception route.

[poikilotherm]

Yes, that makes sense as far as I can foresee now.

But for the moment I would leave them there until it becomes clearer how the API response stuff will be done in the future.
(If you ask me, the errors should be handled completely by throwing JAX-RS or POJO exceptions, so there is a single place to run to and look at, avoiding to many places where stuff is wrapped in JSON sauce.)

[poikilotherm]

@qqmyers If you feel like it, you might take a look at the freshly baked docs. I would love feedback on readability and comprehensibility...

[poikilotherm]

Alright @pdurbin @qqmyers what do you think? Is this ready for QA?

[qqmyers]

The documentation provided is for the developer guide. I'd suggest
a) also providing the common format being implemented in the api guide as well (e.g. for users of the api), and
b) in the developer section, focus on guidance and current state: Right now the new code only handles exceptions, and we still have the alternate AbstractApiBean error() methods being used in many places (which also mirror the same basic JSON structure in the JsonResponseBuilder) - particularly those where and endpoint-specific message is used.
In terms of guidance, I think it makes sense to point out that this new/expanded mechanism makes it easy/preferable to just bubble exceptions up from underlying code and let the framework handle them (versus adding custom logic to catch exceptions/detect issues and prepare custom responses). I'm less sure that telling devs to not use error() and ok() methods in the AbstractAPIBean in favor of directly working with the JsonResponseBuilder is the right thing (why not wire the ok and error methods to use it under the hood in the future? Do we really want two developers making different decisions about adding incident ids or logging for the same error codes?).

In terms of specific guidance, that might mean dropping (or de-emphasizing) the info about the JsonResponseBuilder class and focusing more on telling people that the best way to use the exception handling is something like:

• if you know what HTTP status code should be triggered, use the appropriate subclass of WebApplicationException as your exception class (see https://docs.oracle.com/javaee/7/api/javax/ws/rs/class-use/WebApplicationException.html for example)
• if you have a general exception, you can throw any exception/throwable and, if it isn't caught in Dataverse code, it will trigger a generic 500 error with full logging.
• if you have a specific custom message to send, use the ok or error methods of the AbstrsactApiBean class which also provide a standardized Json wrapper for the response.
• in new code, rather than using the WrappedResponse class, consider using the appropriate Exception

Others may have opinions on the specific guidance - my general point is that focusing on guidance rather than (in addition to) general info about what exists would be useful in the dev guide.

[qqmyers]

Not sure what procedure is, but I think the code itself is ready for QA - my comments were only about documentation which could be handled in parallel.

[djbrooke]

Thanks @qqmyers for checking. Let's take the time to get this into good shape before moving it on to QA. @poikilotherm I'll assign back to you.

[poikilotherm]

Guys, it's exhausting writing about this. Can we please schedule a call for this and talk about what we want, where to go from here and what is within scope of this issue and what's beyond? ("Small steps", to quote @pdurbin)

From the git log, I'd say we ping @pdurbin, @sekmiller, @michbarsinai, @landreev, @matthew-a-dunlap and @qqmyers for a schedule.

[donsizemore]

really minor tweak: uncatched might better be expressed as uncaught or not caught

[scolapasta]

Guys, it's exhausting writing about this. Can we please schedule a call for this and talk about what we want, where to go from here and what is within scope of this issue and what's beyond? ("Small steps", to quote @pdurbin)

From the git log, I'd say we ping @pdurbin, @sekmiller, @michbarsinai, @landreev, @matthew-a-dunlap and @qqmyers for a schedule.

@poikilotherm We discussed briefly at tech hours - in order to get this into Dataverse 5, could you separate this into two PRs: one the code, which he can review now and merge; the other, the discussion of developer guidance, which is out of scope for Dataverse 5. Thanks.

[poikilotherm]

I close this in favor of the new code PR #7170 and new docs/refactoring issue #7171 and #7172