crnk-framework icon indicating copy to clipboard operation
crnk-framework copied to clipboard

crnk throws 500 Internal server error (RepositoryNotFoundException) when we provide wrong type/null type

Open bunnypr opened this issue 4 years ago • 7 comments

when we provide invalid data to type field for post/patch request, we get 500 Internal server error (internally RepositoryNotFoundException) because RepositoryNotFoundException extends InternalServerErrorException. we should get 404 and meaningful error message.

bunnypr avatar Jun 16 '20 12:06 bunnypr

a second exception to fix:

{"exception":"io.crnk.core.exception.InternalServerErrorException: not a valid uuid" (internally java.lang.IllegalArgumentException: Invalid UUID string: abcd" . what can be done here?

these are the logs :

{"exception":"io.crnk.core.exception.InternalServerErrorException: not a valid uuid\n\tat io.crnk.core.engine.internal.http.JsonApiRequestProcessor.toErrorResponse(JsonApiRequestProcessor.java:188)\n\tat io.crnk.core.engine.internal.http.JsonApiRequestProcessor.processAsync(JsonApiRequestProcessor.java:161)\n\tat io.crnk.core.engine.internal.http.JsonApiRequestProcessor.processAsync(JsonApiRequestProcessor.java:121)\n\tat io.crnk.core.engine.internal.http.HttpRequestDispatcherImpl.process(HttpRequestDispatcherImpl.java:75)\n\tat io.crnk.rs.CrnkFilter.filter(CrnkFilter.java:49)\n\tat

{"exception":"java.lang.IllegalArgumentException: Invalid UUID string: bjn\n\tat java.base/java.util.UUID.fromString(UUID.java:215)\n\tat io.crnk.core.engine.parser.DefaultStringParsers$11.parse(DefaultStringParsers.java:99)\n\t... 80 common frames omitted\nWrapped by: io.crnk.core.engine.parser.ParserException: not a valid uuid\n\tat io.crnk.core.engine.parser.DefaultStringParsers$11.parse(DefaultStringParsers.java:101)\n\tat io.crnk.core.engine.parser.DefaultStringParsers$11.parse(DefaultStringParsers.java:95)\n\tat io.crnk.core.engine.parser.TypeParser.parse(TypeParser.java:136)\n\tat io.crnk.core.engine.information.resource.ResourceInformation$1.parse(ResourceInformation.java:132)\n\tat io.crnk.core.engine.information.resource.ResourceInformation.parseIdString(ResourceInformation.java:662)\n\tat io.crnk.core.engine.internal.dispatcher.controller.ResourceUpsert.setId(ResourceUpsert.java:84)\n\tat io.crnk.core.engine.internal.dispatcher.controller.ResourcePostController.handleAsync(ResourcePostController.java:56)\n\tat io.crnk.core.engine.internal.dispatcher.controller.BaseController.handle(BaseController.java:52)\n\tat io.crnk.core.engine.internal.http.DocumentFilterChainImpl.doFilter(DocumentFilterChainImpl.java:28)\n\tat io.crnk.core.engine.internal.http.JsonApiRequestProcessor.processAsync(JsonApiRequestProcessor.java:157)\n\tat io.crnk.core.engine.internal.http.JsonApiRequestProcessor.processAsync(JsonApiRequestProcessor.java:121)\n\tat io.crnk.core.engine.internal.http.HttpRequestDispatcherImpl.process(HttpRequestDispatcherImpl.java:75)\n\tat io.crnk.rs.CrnkFilter.filter(CrnkFilter.java:49)\n\tat 

should be a 400 rather than 500 by throwing a BadRequestException.

remmeier avatar Jun 17 '20 06:06 remmeier

in case you have time, feel free to create a PR to change it. Otherwise I should have time the coming, two, three days.

remmeier avatar Jun 17 '20 06:06 remmeier

I have created other ticket for the above issue . also mentioned other issue in the same ticket. https://github.com/crnk-project/crnk-framework/issues/753

bunnypr avatar Jun 17 '20 08:06 bunnypr

+1

400 should be returned instead of 500 (and 404 since it's a malformed request syntax).

cgendreau avatar Jul 07 '20 13:07 cgendreau

I have the similar problem. I use ObjectId as a resource id and in case users provide an id in a wrong format in GET request the ParserException is thrown (as in @remmeier example). From my point of view the better solution would be not just a good default (400 instead of 500) but also a possibility to implement a custom ExceptionMapper to map the error to a suitable response code. Even in this discussion, there are different opinions regarding the correct default, so it would be great to have flexibility here. Unfortunately, even if a class extending ExceptionMapper<ParserException> is implemented and added to the module context, it's currently not called for ParserException. As far as I understand this is because of current implementation of HttpRequestDispatcherImpl. When the process method is called and a suitable processor is found, it checks that the request can be accepted. In my case the processor is JsonApiRequestProcessor. In the accepts method it tries to get a json path and because the ID is in a wrong format, the ParserException is thrown. The problem is that the exception is not caught by the framework. That's why no custom ExceptionMapper is called. So on the upper level, the exception is mapped to 500 error. The ExceptionMapper is called only if the accepts returned true and processor.processAsync is called, it has try/catch block and the exception mapping. So, my proposal is to add exception handling to the accepts method and in case of exception, follow the same logic as implemented for processor.processAsync. This is how the ParserException is processed by the framework and can be mapped to any suitable http code.

konoplev avatar Jul 30 '20 08:07 konoplev

Any updates?

konoplev avatar Aug 28 '20 14:08 konoplev

any updates on this one ?

vgangavati avatar Oct 11 '21 08:10 vgangavati