Copilot commented on code in PR #845:
URL: https://github.com/apache/maven-wagon/pull/845#discussion_r3214975779
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -122,16 +122,27 @@ protected void mkdirs(String dir) throws IOException {
do {
String url = baseUrl + "/" + navigator.getPath();
status = doMkCol(url);
- if (status == HttpStatus.SC_CREATED || status ==
HttpStatus.SC_METHOD_NOT_ALLOWED) {
+ // RFC 4918: Accept 201 Created, 200 OK (already exists), 405
Method Not Allowed (already exists)
+ if (status == HttpStatus.SC_CREATED
+ || status == HttpStatus.SC_OK
+ || status == HttpStatus.SC_METHOD_NOT_ALLOWED) {
break;
}
+ // RFC 4918: 409 Conflict means intermediate collection is
missing, continue traversing backwards
+ if (status == HttpStatus.SC_CONFLICT) {
+ continue;
+ }
+ // Any other status is unexpected here: stop traversal and report
an error
+ throw new IOException("Unexpected status code while creating
collection: " + url
+ + "; status code = " + status);
} while (navigator.backward());
// traverse forward creating missing directories
while (navigator.forward()) {
String url = baseUrl + "/" + navigator.getPath();
status = doMkCol(url);
- if (status != HttpStatus.SC_CREATED) {
+ // RFC 4918: Accept 201 Created or 200 OK (if collection already
exists from another request)
+ if (status != HttpStatus.SC_CREATED && status != HttpStatus.SC_OK)
{
Review Comment:
In the forward traversal, MKCOL responses of 405 Method Not Allowed
(collection already exists) are still treated as errors. This can cause
mkdirs() to fail if intermediate collections already exist (e.g., created
concurrently) and the server follows RFC 4918 by returning 405 for existing
collections. Consider accepting SC_METHOD_NOT_ALLOWED here as well, consistent
with the backward traversal logic above.
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -140,7 +151,21 @@ protected void mkdirs(String dir) throws IOException {
private int doMkCol(String url) throws IOException {
HttpMkcol method = new HttpMkcol(url);
try (CloseableHttpResponse closeableHttpResponse = execute(method)) {
- return closeableHttpResponse.getStatusLine().getStatusCode();
+ int statusCode =
closeableHttpResponse.getStatusLine().getStatusCode();
+
+ // RFC 4918: Handle redirects for MKCOL
+ // 3xx redirects should be followed to the new location
+ if (statusCode >= HttpStatus.SC_MULTIPLE_CHOICES && statusCode <
HttpStatus.SC_BAD_REQUEST) {
+ org.apache.http.Header locationHeader =
closeableHttpResponse.getFirstHeader("Location");
+ if (locationHeader != null) {
+ String redirectUrl = locationHeader.getValue();
+ // Follow the redirect by recursively invoking doMkCol on
the new URL
+ // and return the status code from the final resolved
location
+ return doMkCol(redirectUrl);
+ }
Review Comment:
doMkCol() follows redirects by recursively calling itself before the current
response has been closed. Because the return expression evaluates
doMkCol(redirectUrl) while the try-with-resources response is still open, this
can tie up a connection and potentially exhaust the connection pool under
redirect scenarios. Capture the Location, exit/close the response, then follow
the redirect (ideally with a bounded redirect count / loop detection).
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -140,7 +151,21 @@ protected void mkdirs(String dir) throws IOException {
private int doMkCol(String url) throws IOException {
HttpMkcol method = new HttpMkcol(url);
try (CloseableHttpResponse closeableHttpResponse = execute(method)) {
- return closeableHttpResponse.getStatusLine().getStatusCode();
+ int statusCode =
closeableHttpResponse.getStatusLine().getStatusCode();
+
+ // RFC 4918: Handle redirects for MKCOL
+ // 3xx redirects should be followed to the new location
+ if (statusCode >= HttpStatus.SC_MULTIPLE_CHOICES && statusCode <
HttpStatus.SC_BAD_REQUEST) {
+ org.apache.http.Header locationHeader =
closeableHttpResponse.getFirstHeader("Location");
+ if (locationHeader != null) {
+ String redirectUrl = locationHeader.getValue();
+ // Follow the redirect by recursively invoking doMkCol on
the new URL
+ // and return the status code from the final resolved
location
+ return doMkCol(redirectUrl);
+ }
+ }
+
+ return statusCode;
Review Comment:
This manual 3xx handling duplicates the HttpClient redirect behavior already
configured via WagonRedirectStrategy (which explicitly supports MKCOL) and it
treats any 300-399 as redirect (including statuses like 304) without resolving
relative Location values. Prefer relying on the existing redirect strategy, or
at least restrict to actual redirect status codes and resolve the Location URI
against the original request URL with a maximum redirect limit.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
