Re: SlingPostServlet should not throw 500 on any exception
Hi Joerg, On Wed, 2020-12-23 at 11:25 +0100, Jörg Hoh wrote: > I would love to get some feedback on > https://github.com/apache/sling-org-apache-sling-servlets-post/pull/7 > , so > we can get a fix for my problem committed soon. I had a single comment on the conceptual level, it would be good to discuss it, and hopefully the others involved will find the time to review as well. Thanks, Robert > > Thanks, > Jörg > > Am Do., 17. Dez. 2020 um 12:54 Uhr schrieb Jörg Hoh < > jhoh...@googlemail.com > > : > > > Thanks Bertrand, I implemented your feedback. > > > > As my initial implementation was deemed to simple and not covering > > relevant cases, I created a new PR at > > https://github.com/apache/sling-org-apache-sling-servlets-post/pull/7 > > which tries to return a different status code based on various JCR > > exceptions. > > > > Thanks > > > > Am Mo., 14. Dez. 2020 um 16:29 Uhr schrieb Bertrand Delacretaz < > > bdelacre...@apache.org>: > > > > > Hi, > > > > > > I'm coming late to this discussion, with a smallish thing.. > > > > > > On Wed, Nov 11, 2020 at 1:10 PM Jörg Hoh < > > > jhoh...@googlemail.com.invalid> > > > wrote: > > > > ...Having that in mind, I would nevertheless argue to switch > > > > the > > > behavior of > > > > the SlingPostServlet to return a 405 "Method not allowed" in > > > > the case > > > of a > > > > PersistenceError [2]... > > > > > > I would much prefer a 409 "Conflict" status which as per rfc7231 > > > indicates "a conflict with the current state of the target > > > resource" > > > where "the user might be able to resolve the conflict and > > > resubmit the > > > request." > > > > > > I think it's suitably vague, whereas 405 is meant for when the > > > HTTP > > > method used is not appropriate, which is not the problem here > > > IMO. > > > > > > Also, 405 is indicated to be cacheable by default which is > > > probably > > > not what we want here. > > > > > > -Bertrand > > > > > > > [2] > > > > > > > https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237 > > > > > > > > > -- > > Cheers, > > Jörg Hoh, > > > > http://cqdump.wordpress.com > > Twitter: @joerghoh > > > >
Re: SlingPostServlet should not throw 500 on any exception
I would love to get some feedback on https://github.com/apache/sling-org-apache-sling-servlets-post/pull/7, so we can get a fix for my problem committed soon. Thanks, Jörg Am Do., 17. Dez. 2020 um 12:54 Uhr schrieb Jörg Hoh : > Thanks Bertrand, I implemented your feedback. > > As my initial implementation was deemed to simple and not covering > relevant cases, I created a new PR at > https://github.com/apache/sling-org-apache-sling-servlets-post/pull/7 > which tries to return a different status code based on various JCR > exceptions. > > Thanks > > Am Mo., 14. Dez. 2020 um 16:29 Uhr schrieb Bertrand Delacretaz < > bdelacre...@apache.org>: > >> Hi, >> >> I'm coming late to this discussion, with a smallish thing.. >> >> On Wed, Nov 11, 2020 at 1:10 PM Jörg Hoh >> wrote: >> > ...Having that in mind, I would nevertheless argue to switch the >> behavior of >> > the SlingPostServlet to return a 405 "Method not allowed" in the case >> of a >> > PersistenceError [2]... >> >> I would much prefer a 409 "Conflict" status which as per rfc7231 >> indicates "a conflict with the current state of the target resource" >> where "the user might be able to resolve the conflict and resubmit the >> request." >> >> I think it's suitably vague, whereas 405 is meant for when the HTTP >> method used is not appropriate, which is not the problem here IMO. >> >> Also, 405 is indicated to be cacheable by default which is probably >> not what we want here. >> >> -Bertrand >> >> > [2] >> > >> https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237 >> > > > -- > Cheers, > Jörg Hoh, > > http://cqdump.wordpress.com > Twitter: @joerghoh > -- Cheers, Jörg Hoh, http://cqdump.wordpress.com Twitter: @joerghoh
Re: SlingPostServlet should not throw 500 on any exception
Thanks Bertrand, I implemented your feedback. As my initial implementation was deemed to simple and not covering relevant cases, I created a new PR at https://github.com/apache/sling-org-apache-sling-servlets-post/pull/7 which tries to return a different status code based on various JCR exceptions. Thanks Am Mo., 14. Dez. 2020 um 16:29 Uhr schrieb Bertrand Delacretaz < bdelacre...@apache.org>: > Hi, > > I'm coming late to this discussion, with a smallish thing.. > > On Wed, Nov 11, 2020 at 1:10 PM Jörg Hoh > wrote: > > ...Having that in mind, I would nevertheless argue to switch the > behavior of > > the SlingPostServlet to return a 405 "Method not allowed" in the case of > a > > PersistenceError [2]... > > I would much prefer a 409 "Conflict" status which as per rfc7231 > indicates "a conflict with the current state of the target resource" > where "the user might be able to resolve the conflict and resubmit the > request." > > I think it's suitably vague, whereas 405 is meant for when the HTTP > method used is not appropriate, which is not the problem here IMO. > > Also, 405 is indicated to be cacheable by default which is probably > not what we want here. > > -Bertrand > > > [2] > > > https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237 > -- Cheers, Jörg Hoh, http://cqdump.wordpress.com Twitter: @joerghoh
Re: SlingPostServlet should not throw 500 on any exception
Hi, I'm coming late to this discussion, with a smallish thing.. On Wed, Nov 11, 2020 at 1:10 PM Jörg Hoh wrote: > ...Having that in mind, I would nevertheless argue to switch the behavior of > the SlingPostServlet to return a 405 "Method not allowed" in the case of a > PersistenceError [2]... I would much prefer a 409 "Conflict" status which as per rfc7231 indicates "a conflict with the current state of the target resource" where "the user might be able to resolve the conflict and resubmit the request." I think it's suitably vague, whereas 405 is meant for when the HTTP method used is not appropriate, which is not the problem here IMO. Also, 405 is indicated to be cacheable by default which is probably not what we want here. -Bertrand > [2] > https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237
Re: SlingPostServlet should not throw 500 on any exception
Hi. Strongly agree this should not be a 5xx response. A 4xx makes sense. - Andrei On Wed, 11 Nov 2020 at 14:25, Carsten Ziegeler wrote: > Hi, > > agreed - in many cases we're return a 500 where a 4xx would be more > appropriate. As you mention, the post servlet can't decide which case it > is, so we have to map all exceptions either to 500 (like today) or to > 4xx. In both cases, we have a false positives. > But I agree, that it's more likely that its a 4xx case if the exception > is thrown. When there is a real server problem, I would assume you get > exceptions all over the place anyway. > So, I think the change makes sense - we should also not log the > stacktrace as part of the warning anymore. I'm not even sure if a 4xx > should log a warning at all. > > Regards > Carsten > > Am 11.11.2020 um 13:10 schrieb Jörg Hoh: > > Hi all, > > > > while trying to assess a number of internal server errors, I came across > > the behavior that the SlingPostServlet is always returning a 500 if it > has > > been invoked and wasn't able to write to the repository because of > missing > > write permissions [2]. See > https://issues.apache.org/jira/browse/SLING-9896 > > for a sample of such a stacktrace. > > > > I don't think that this type of error qualifies for a HTTP statuscode > 500, > > but it's rather an expected behavior, and therefor it should return with > a > > 4xx statuscode. > > > > In the example mentioned above, the PersistenceException is thrown by the > > JcrResourceProvider [1], but this PersistenceException is generated on > > every RepositoryException. So technically, on the one hand side it could > be > > caused by a defunct repository (and imo that would qualify for an > internal > > server error) on the other hand side it might be caused just by missing > > permissions. > > > > Having that in mind, I would nevertheless argue to switch the behavior of > > the SlingPostServlet to return a 405 "Method not allowed" in the case of > a > > PersistenceError [2]. It isn't 100% accurate either but still better than > > the internal server error. Making it more accurate would require major > > changes to the Sling-JCR implementation, and I am not sure if this > > improvement in semantics justifies it. > > All other exceptions are thandled the same way as before and continue to > > return an internal server error. > > > > WDYT? > > > > Jörg > > > > > > [1] > > > https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/org.apache.sling.jcr.resource-3.0.22/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L476 > > > > [2] > > > https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237 > > > > -- > -- > Carsten Ziegeler > Adobe Research Switzerland > cziege...@apache.org >
Re: SlingPostServlet should not throw 500 on any exception
Hi, agreed - in many cases we're return a 500 where a 4xx would be more appropriate. As you mention, the post servlet can't decide which case it is, so we have to map all exceptions either to 500 (like today) or to 4xx. In both cases, we have a false positives. But I agree, that it's more likely that its a 4xx case if the exception is thrown. When there is a real server problem, I would assume you get exceptions all over the place anyway. So, I think the change makes sense - we should also not log the stacktrace as part of the warning anymore. I'm not even sure if a 4xx should log a warning at all. Regards Carsten Am 11.11.2020 um 13:10 schrieb Jörg Hoh: Hi all, while trying to assess a number of internal server errors, I came across the behavior that the SlingPostServlet is always returning a 500 if it has been invoked and wasn't able to write to the repository because of missing write permissions [2]. See https://issues.apache.org/jira/browse/SLING-9896 for a sample of such a stacktrace. I don't think that this type of error qualifies for a HTTP statuscode 500, but it's rather an expected behavior, and therefor it should return with a 4xx statuscode. In the example mentioned above, the PersistenceException is thrown by the JcrResourceProvider [1], but this PersistenceException is generated on every RepositoryException. So technically, on the one hand side it could be caused by a defunct repository (and imo that would qualify for an internal server error) on the other hand side it might be caused just by missing permissions. Having that in mind, I would nevertheless argue to switch the behavior of the SlingPostServlet to return a 405 "Method not allowed" in the case of a PersistenceError [2]. It isn't 100% accurate either but still better than the internal server error. Making it more accurate would require major changes to the Sling-JCR implementation, and I am not sure if this improvement in semantics justifies it. All other exceptions are thandled the same way as before and continue to return an internal server error. WDYT? Jörg [1] https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/org.apache.sling.jcr.resource-3.0.22/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L476 [2] https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237 -- -- Carsten Ziegeler Adobe Research Switzerland cziege...@apache.org
SlingPostServlet should not throw 500 on any exception
Hi all, while trying to assess a number of internal server errors, I came across the behavior that the SlingPostServlet is always returning a 500 if it has been invoked and wasn't able to write to the repository because of missing write permissions [2]. See https://issues.apache.org/jira/browse/SLING-9896 for a sample of such a stacktrace. I don't think that this type of error qualifies for a HTTP statuscode 500, but it's rather an expected behavior, and therefor it should return with a 4xx statuscode. In the example mentioned above, the PersistenceException is thrown by the JcrResourceProvider [1], but this PersistenceException is generated on every RepositoryException. So technically, on the one hand side it could be caused by a defunct repository (and imo that would qualify for an internal server error) on the other hand side it might be caused just by missing permissions. Having that in mind, I would nevertheless argue to switch the behavior of the SlingPostServlet to return a 405 "Method not allowed" in the case of a PersistenceError [2]. It isn't 100% accurate either but still better than the internal server error. Making it more accurate would require major changes to the Sling-JCR implementation, and I am not sure if this improvement in semantics justifies it. All other exceptions are thandled the same way as before and continue to return an internal server error. WDYT? Jörg [1] https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/org.apache.sling.jcr.resource-3.0.22/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProvider.java#L476 [2] https://github.com/apache/sling-org-apache-sling-servlets-post/blob/master/src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java#L237 -- Cheers, Jörg Hoh, http://cqdump.wordpress.com Twitter: @joerghoh
