SLING-12834 Expose the cause of PersistenceException in its getMessage()#61
SLING-12834 Expose the cause of PersistenceException in its getMessage()#61
Conversation
kwin
force-pushed
the
feature/expose-persistence-exception-cause
branch
from
b870036 to
ab5c3b1
Compare
This is beneficial for cases where the stacktrace is not available (e.g. in UI using PersistenceException.getMessage() or PersistenceException.toString()) like SlingPostServlets HtmlResponse.
kwin
force-pushed
the
feature/expose-persistence-exception-cause
branch
from
ab5c3b1 to
47f0093
Compare
| * @return The message for this exception | ||
| */ | ||
| @Override | ||
| public String getMessage() { |
There was a problem hiding this comment.
I feel we should fix this in SlingPostServlet and in the appropriate UI where this goes wrong, now we're adding this override in PersistenceException, but what if another exception bubbles up somewhere, do we also need to fix the getMessage-method there then?
There was a problem hiding this comment.
In general I agree with your point of view, however for PersistenceException everywhere only a very generic message is used and almost all of them are triggered by downstream exceptions in the providers. Also we don’t control all places which evaluate the exception. Do you see any potential downsides of this except for deviating a bit from Java standard?
There was a problem hiding this comment.
Also I would appreciate your opinion whether this should be fixed rather in the consumer or when throwing the exception, I don't really know to be honest. I have seen patterns like this https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/fe35d53a1b0f8ff1a7616909f16539b759bd0bdd/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L540 which seem wrong to me as that clearly duplicates information. However always exposing the root cause for every exception in https://github.com/apache/sling-org-apache-sling-servlets-post/blob/7ff7280c7ba89592186ac76d33b481d73abba323/src/main/java/org/apache/sling/servlets/post/AbstractPostResponse.java#L286 might be too much as well...
There was a problem hiding this comment.
I think something can be done in SlingPostServlet to provide more information about the failure.
The other question would be: Do we actually want to expose this information? Isn't it fine to have this detailed information about the system (that is a blackbox for / transparent to the user) in the logs and just let the user know that something went wrong internally?
Indeed, https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/fe35d53a1b0f8ff1a7616909f16539b759bd0bdd/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L540 is also wrong to me, it should provide additional information and context as to why this PersistenceException was thrown, which is coming from an IllegalArgumentException. It should at least tell that it was trying to provide properties which were not ignored, specifically what key and value as that would tell a developer immediately which property was causing the issue.
There was a problem hiding this comment.
One example where the root cause is helpful for less technical users is a PersistenceException for ResourceResolver.create(). The most common causes (at least with the JCR provider) are:
a) Resource with that name does already exist
b) User does not have permission to create a resource there or
c) Resource with the given type is not allowed there
I think this is crucial to transmit even to end users because at least a) can be solved by the user himself without involving IT.
There was a problem hiding this comment.
I think that this is hard to generalize. In some cases it is ok to expose this information to users, while in other situations it would expose details which makes it easier for an attacker.
rombert
left a comment
rombert
left a comment
There was a problem hiding this comment.
I'm not going to +1 or -1 this because I don't have enough experience in the area to say whether it's generally useful and safe to apply.
My expectation would be that the added detail is something that a developer would look at during, well, development and that would lead to code adjustment. I don't expect users to try and create content in various places in the repository and see what sticks. Well, not well-intentioned users anyway :-)
But since @kwin raised this there must be a use case but I can't really review it.
And I don't know how sensitive the details that will be exposed are.
|
5 similar comments
|
|
|
|
|
|
The exception object contains all the required information, so this looks to me like a logging problem and I would rather fix it where the exception is logged - I understand that this are potentially many places, but it is the least magic |
5 participants
This is beneficial for cases where the stacktrace is not available (e.g. in UI using PersistenceException.getMessage() or
PersistenceException.toString()) like SlingPostServlets HtmlResponse.