npeltier commented on a change in pull request #6:
URL:
https://github.com/apache/sling-org-apache-sling-servlets-post/pull/6#discussion_r531437758
##########
File path:
src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java
##########
@@ -234,6 +235,11 @@ protected void doPost(final SlingHttpServletRequest
request,
} catch (ResourceNotFoundException rnfe) {
htmlResponse.setStatus(HttpServletResponse.SC_NOT_FOUND,
rnfe.getMessage());
+ } catch (final PersistenceException pe) {
+ log.warn("PersistenceException while handling POST "
Review comment:
don't we have redundant information here then? we'll see that
SlingPostServlet is logging a PersistenceException, and we'll have a line
saying "PersistenceException while handling POST"?
##########
File path:
src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java
##########
@@ -234,6 +235,11 @@ protected void doPost(final SlingHttpServletRequest
request,
} catch (ResourceNotFoundException rnfe) {
htmlResponse.setStatus(HttpServletResponse.SC_NOT_FOUND,
rnfe.getMessage());
+ } catch (final PersistenceException pe) {
+ log.warn("PersistenceException while handling POST "
+ + request.getResource().getPath() + " with "
+ + operation.getClass().getName(), pe);
+
htmlResponse.setStatus(HttpServletResponse.SC_METHOD_NOT_ALLOWED,"invalid POST
request");
Review comment:
How are we sure (or just mostly certain) that a persistence exception is
a 405 and not a 500? while agreeing on the general topic of that change, we
should not revert the fake info and say everything is an ACL issue. Is this
what you were mentioning at the beginning of the PR? Do you think we could add
tests mocking real world issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
