On 2024/10/02 10:21:07 Mark Thomas wrote: > -1 veto > > This change is not compliant with RFC 9110. > > If Tomcat sends METHOD_NOT_ALLOWED it MUST also send an Allow header. > > That is why the default servlet has a dedicated method for sending > METHOD_NOT_ALLOWED. > > Further comments in-line. > > Mark > > On 02/10/2024 10:07, micha...@apache.org wrote: > > This is an automated email from the ASF dual-hosted git repository. > > > > michaelo pushed a commit to branch main > > in repository https://gitbox.apache.org/repos/asf/tomcat.git > > > > > > The following commit(s) were added to refs/heads/main by this push: > > new 142c45a427 BZ 69360: Inconsistent DELETE behavior between > > DefaultServlet and WebdavServlet > > 142c45a427 is described below > > > > commit 142c45a4271e1fd8d400196a883fb560ebded110 > > Author: Michael Osipov <micha...@apache.org> > > AuthorDate: Wed Oct 2 11:04:44 2024 +0200 > > > > BZ 69360: Inconsistent DELETE behavior between DefaultServlet and > > WebdavServlet > > --- > > java/org/apache/catalina/servlets/WebdavServlet.java | 6 +++--- > > webapps/docs/changelog.xml | 5 +++++ > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/java/org/apache/catalina/servlets/WebdavServlet.java > > b/java/org/apache/catalina/servlets/WebdavServlet.java > > index d32868a9b3..c2cf8f3d78 100644 > > --- a/java/org/apache/catalina/servlets/WebdavServlet.java > > +++ b/java/org/apache/catalina/servlets/WebdavServlet.java > > @@ -1696,7 +1696,7 @@ public class WebdavServlet extends DefaultServlet > > implements PeriodicEventListen > > > > if (!resource.isDirectory()) { > > if (!resource.delete()) { > > - resp.sendError(WebdavStatus.SC_INTERNAL_SERVER_ERROR); > > + resp.sendError(WebdavStatus.SC_METHOD_NOT_ALLOWED); > > May be able to utilise sendNotAllowed() for this one. Thinking about it > probably not as WebDAV has additional methods that need to be taken > account of. It will need a dedicated method for WebDAV.
I agree, infact I have written this in the BZ issue: https://bz.apache.org/bugzilla/show_bug.cgi?id=69360#c2 For a single resource (not collection) I will use the method mentioned. Agreed? > > return false; > > } > > } else { > > @@ -1705,7 +1705,7 @@ public class WebdavServlet extends DefaultServlet > > implements PeriodicEventListen > > > > deleteCollection(req, path, errorList); > > if (!resource.delete()) { > > - errorList.put(path, > > Integer.valueOf(WebdavStatus.SC_INTERNAL_SERVER_ERROR)); > > + errorList.put(path, > > Integer.valueOf(WebdavStatus.SC_METHOD_NOT_ALLOWED)); > > This is going to be part of a multi-status response. The allow header > doesn't make sense for a multi-status response. There I think this is OK. Same here. > > } > > > > if (!errorList.isEmpty()) { > > @@ -1772,7 +1772,7 @@ public class WebdavServlet extends DefaultServlet > > implements PeriodicEventListen > > if (!childResource.isDirectory()) { > > // If it's not a collection, then it's an unknown > > // error > > - errorList.put(childName, > > Integer.valueOf(WebdavStatus.SC_INTERNAL_SERVER_ERROR)); > > + errorList.put(childName, > > Integer.valueOf(WebdavStatus.SC_METHOD_NOT_ALLOWED)); > > As above. Ditto. > > } > > } > > } > > Generally, SC_METHOD_NOT_ALLOWED is probably the right status code as > the most likely failure mode is a lack of permissions. I just wonder if > there are circumstances where we would want to return > SC_INTERNAL_SERVER_ERROR and how we would determine if that were the case. This I have described here: https://bz.apache.org/bugzilla/show_bug.cgi?id=69360#c0 Files#delete(Path) can give us a proper exception to map to INTERNAL_SERVER_ERROR. It the first proposed change good for you pull back the vote? Michael > > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml > > index ca652cd604..f4eb84d1b4 100644 > > --- a/webapps/docs/changelog.xml > > +++ b/webapps/docs/changelog.xml > > @@ -128,6 +128,11 @@ > > <code>getRelativePath()</code> method from super class with > > incorrect Javadoc. (michaelo) > > </fix> > > + <fix> > > + <bug>69360</bug>: Inconsistent <code>DELETE</code> behavior between > > + <code>WebdavServlet</code> and <code>DefaultServlet</code>. > > + (michaelo) > > + </fix> > > </changelog> > > </subsection> > > <subsection name="Coyote"> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
