REST endpoint exceptions

[mleduc]

Currently, we have an issue with exceptions thrown by REST endpoints.
More precisely, some exceptions are not logged and an HTTP 500 response with the 'No ExceptionMapper was found, but must be found' is returned to the client, without any additional log being recorded. This makes it very difficult to find the root cause of the error (for instance, https://forum.xwiki.org/t/wiki-index-empty/)

This is the case for all exceptions that:

• are checked
• does not have a corresponding ExceptionMapper.
  In summary that’s all checked exceptions except XWikiRestException, WebAppException and their children).

So first I propose to introduce a generic ExceptionMapper<Exception> that would do two things:

1. log the error so that we have access to the stracktrace of the exceptions (and hence be able to fix the issue more easily)
2. return a response exactly similar to the one returned by JaxRsProviders (i.e., the HTTP 500 + 'No ExceptionMapper was found, but must be found' response) to make the change transparent for the clients.

In addition, I propose to improve the best practices regarding exceptions in REST endpoints, by introducing the following rules:

• rest endpoints must never throw Exception because it is too generic
• rest endpoints must always throw unchecked exceptions, XWikiRestException, or WebAppException
• other checked exceptions are not allowed unless an ExceptionMapper is provided for them.

Existing code will be updated according to the best practices (if agreed on) on case by case basis.

WDYT?

[vmassol]

@mleduc

More precisely, some exceptions are not logged and an HTTP 500 response with the 'No ExceptionMapper was found, but must be found' is returned to the client, without any additional log being recorded.

That looks like a bug that needs to be fixed when that happens, whether the REST endpoint is new or old, no?

@mleduc

log the error so that we have access to the stracktrace of the exceptions (and hence be able to fix the issue more easily)

Do we need to separate real errors (like a DB error) from expected error (like when a user doesn’t have the proper permission for ex - maybe we don’t throw an exception in this case, idk)? Do we need to log something for both? It seems to me only the first type of error should be logged (ie something unexpected) and that the other type of error should be logged as a debug option. WDYT?

@mleduc

return a response exactly similar to the one returned by JaxRsProviders (i.e., the HTTP 500 + 'No ExceptionMapper was found, but must be found' response) to make the change transparent for the clients.

See my first point above. This looks like a bug so not sure if we need to keep the behavior.

@mleduc

rest endpoints must never throw Exception because it is too generic

What do you propose to enforce it?

@mleduc

rest endpoints must always throw unchecked exceptions, XWikiRestException, or WebAppException

What’s the difference between both exceptions? When to use one or the other?

How do you enforce it?

@mleduc

other checked exceptions are not allowed unless an ExceptionMapper is provided for them.

How could we enforce it?

Also I don’t understand why new code would return:

@mleduc

return a response exactly similar to the one returned by JaxRsProviders (i.e., the HTTP 500 + 'No ExceptionMapper was found, but must be found' response) to make the change transparent for the clients.

This doesn’t look very nice to me.

Thanks

[mleduc]

@vmassol

That looks like a bug that needs to be fixed when that happens, whether the REST endpoint is new or old, no?

It is a bug, but currently we have no information of what happened in the rest endpoint since nothing is logged. Which makes it difficult to fix the root problem in the rest endpoints.
The goal of the introduction of a ExceptionMapper<Exception> is to be able to log the unchecked exception to improve debuggability.

Regarding the lack of logging and the 'No ExceptionMapper was found, but must be found' message, comes from the restlet jax-rs library that does not seem to be easy to replace or upgrade (and afaics, this hasn’t been fixed in more recent releases) (see https://jira.xwiki.org/browse/XWIKI-12674, https://jira.xwiki.org/browse/XWIKI-12955). @tmortagne can you confirm?
Of course, if we can switch to another library that might solve the problem too.

@vmassol

Do we need to separate real errors (like a DB error) from expected error (like when a user doesn’t have the proper permission for ex - maybe we don’t throw an exception in this case, idk)? Do we need to log something for both? It seems to me only the first type of error should be logged (ie something unexpected) and that the other type of error should be logged as a debug option. WDYT?

I agree but for now we don’t even know what are the thrown exceptions. Once the badly handled exceptions are properly logged, it will be easier to fix them in the endpoints.

@vmassol

How do you enforce it?

Documentation and code review. It might be possible to add a static analysis but it might lead to false positive errors.

@vmassol

This doesn’t look very nice to me.

I agree. But this is still an improvement since currently, we return the same response to the client without any additional details about what caused the error.