Skip to content

7134 errorhandlers refactored - code only#7170

Merged
kcondon merged 13 commits intoIQSS:developfrom
poikilotherm:7134-errorhandlers-code
Aug 10, 2020
Merged

7134 errorhandlers refactored - code only#7170
kcondon merged 13 commits intoIQSS:developfrom
poikilotherm:7134-errorhandlers-code

Conversation

@poikilotherm
Copy link
Contributor

NOTE: this is a successor to PR #7136 as requested by @scolapasta, containing the code only.

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.

@poikilotherm poikilotherm added Feature: API User Role: API User Makes use of APIs User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh labels Aug 10, 2020
@coveralls
Copy link

coveralls commented Aug 10, 2020

Coverage Status

Coverage increased (+0.03%) to 19.563% when pulling 8f879d7 on poikilotherm:7134-errorhandlers-code into c859b16 on IQSS:develop.

@kcondon kcondon self-assigned this Aug 10, 2020
@kcondon
Copy link
Contributor

kcondon commented Aug 10, 2020

@poikilotherm I'm still seeing the 406 errors:
[2020-08-10T20:00:42.722+0000] [Payara 5.201] [SEVERE] [] [org.glassfish.jersey.server.ServerRuntime$Responder] [tid: _ThreadID=97 _ThreadName=http-thread-pool::jk-conn
ector(2)] [timeMillis: 1597089642722] [levelValue: 1000] [[
An exception has been thrown from an exception mapper class edu.harvard.iq.dataverse.api.errorhandlers.WebApplicationExceptionHandler.
java.lang.NullPointerException: Value in JsonObjects name/value pair cannot be null
at org.glassfish.json.JsonObjectBuilderImpl.validateValue(JsonObjectBuilderImpl.java:198)
at org.glassfish.json.JsonObjectBuilderImpl.add(JsonObjectBuilderImpl.java:66)
at edu.harvard.iq.dataverse.api.util.JsonResponseBuilder.requestContentType(JsonResponseBuilder.java:118)
at edu.harvard.iq.dataverse.api.errorhandlers.WebApplicationExceptionHandler.toResponse(WebApplicationExceptionHandler.java:76)
at edu.harvard.iq.dataverse.api.errorhandlers.WebApplicationExceptionHandler.toResponse(WebApplicationExceptionHandler.java:24)
at org.glassfish.jersey.server.ServerRuntime$Responder.mapException(ServerRuntime.java:528)
at org.glassfish.jersey.server.ServerRuntime$Responder.process(ServerRuntime.java:403)
at org.glassfish.jersey.server.ServerRuntime$1.run(ServerRuntime.java:261)
at org.glassfish.jersey.internal.Errors$1.call(Errors.java:248)
at org.glassfish.jersey.internal.Errors$1.call(Errors.java:244)
at org.glassfish.jersey.internal.Errors.process(Errors.java:292)
at org.glassfish.jersey.internal.Errors.process(Errors.java:274)
at org.glassfish.jersey.internal.Errors.process(Errors.java:244)

Not sure what is causing it but might be in export since I had just published the dataset. I was also just creating a new one but had not saved yet, just opened the edit form.

Oh Sorry, that was the initial stack trace, the following stack held the 406:
[2020-08-10T20:00:42.723+0000] [Payara 5.201] [SEVERE] [] [org.glassfish.jersey.server.ServerRuntime$Responder] [tid: _ThreadID=97 _ThreadName=http-thread-pool::jk-connector(2)] [timeMillis: 1597089642723] [levelValue: 1000] [[
An exception was not mapped due to exception mapper failure. The HTTP 500 response will be returned.
javax.ws.rs.NotAcceptableException: HTTP 406 Not Acceptable
at org.glassfish.jersey.server.internal.routing.MethodSelectingRouter.getMethodRouter(MethodSelectingRouter.java:463)
at org.glassfish.jersey.server.internal.routing.MethodSelectingRouter.access$000(MethodSelectingRouter.java:73)
at org.glassfish.jersey.server.internal.routing.MethodSelectingRouter$4.apply(MethodSelectingRouter.java:665)
at org.glassfish.jersey.server.internal.routing.MethodSelectingRouter.apply(MethodSelectingRouter.java:305)
at org.glassfish.jersey.server.internal.routing.RoutingStage._apply(RoutingStage.java:86)
at org.glassfish.jersey.server.internal.routing.RoutingStage._apply(RoutingStage.java:89)
at org.glassfish.jersey.server.internal.routing.RoutingStage._apply(RoutingStage.java:89)
at org.glassfish.jersey.server.internal.routing.RoutingStage._apply(RoutingStage.java:89)
at org.glassfish.jersey.server.internal.routing.RoutingStage.apply(RoutingStage.java:69)
at org.glassfish.jersey.server.internal.routing.RoutingStage.apply(RoutingStage.java:38)
at org.glassfish.jersey.process.internal.Stages.process(Stages.java:173)
at org.glassfish.jersey.server.ServerRuntime$1.run(ServerRuntime.java:245)
at org.glassfish.jersey.internal.Errors$1.call(Errors.java:248)
at org.glassfish.jersey.internal.Errors$1.call(Errors.java:244)
at org.glassfish.jersey.internal.Errors.process(Errors.java:292)
at org.glassfish.jersey.internal.Errors.process(Errors.java:274)
at org.glassfish.jersey.internal.Errors.process(Errors.java:244)

Adding a null check on the content type to fix what @kcondon saw in testing. @poikilotherm - if there's a better fix, go ahead and revert this - just wanted to keep testing moving.
@kcondon
Copy link
Contributor

kcondon commented Aug 10, 2020

@poikilotherm In the spirit of not following instructions and acting randomly, I have started creating a dataset in the UI, modified the host dv so it is invalid and not completed any required fields other than title, then saved. Big stack traces in log, ui kind of stuck in grayed out view. On Demo, form validation catches it and presents error to user. Not sure whether it is related to these changes or not.

[2020-08-10T21:00:00.567+0000] [Payara 5.201] [WARNING] [] [javax.enterprise.resource.webcontainer.jsf.lifecycle] [tid: _ThreadID=90 _ThreadName=http-thread-pool::jk-connector(4)] [timeMillis: 1597093200567] [levelValue: 900] [[
/dataset.xhtml @1579,68 rendered="#{publishDataset}": /dataset.xhtml @37,91 value="#{DatasetPage.publishDatasetPopup()}": java.lang.NullPointerException
javax.el.ELException: /dataset.xhtml @1579,68 rendered="#{publishDataset}": /dataset.xhtml @37,91 value="#{DatasetPage.publishDatasetPopup()}": java.lang.NullPointerException

@kcondon
Copy link
Contributor

kcondon commented Aug 10, 2020

@poikilotherm After discussing with Jim, I understand these error handlers are for REST api only and that the UI, though it may call same underlying code, does not follow the same error handling path. So, I will focus on API errors here and maybe open a new issue for UI behavior I'm seeing.

@kcondon kcondon merged commit 3029502 into IQSS:develop Aug 10, 2020
@poikilotherm
Copy link
Contributor Author

Thx @qqmyers for handling this. Was a bit late yesterday for me... 😉

I'm fine with returning a JSON null value, although it's debatable if one would rather exclude the field from the response. Let's stick with it as is now 👍

@kcondon are any problems remaining with the 406 errors or all set now? (I assume as you merged, but wanted to make sure everything is fine now...)

@kcondon
Copy link
Contributor

kcondon commented Aug 11, 2020

@poikilotherm Everything is set for now, thanks.

@poikilotherm poikilotherm deleted the 7134-errorhandlers-code branch February 1, 2021 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature: API User Role: API User Makes use of APIs User Role: Sysadmin Installs, upgrades, and configures the system, connects via ssh

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error Handling: Multiple server.log errors indicating http 406 Not Acceptable

4 participants