Copilot commented on code in PR #845:
URL: https://github.com/apache/maven-wagon/pull/845#discussion_r2654024717
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -140,7 +148,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();
+ // Recursive call to handle redirect - execute() will
handle the redirect automatically
+ // but we need to return the final status
Review Comment:
The comment on line 159 is inconsistent with the actual implementation. The
comment states "execute() will handle the redirect automatically" but then the
code makes a recursive call to handle the redirect manually. Either the
redirect is handled automatically by the HTTP client (in which case the manual
handling code is unnecessary), or it needs to be handled manually (in which
case the comment is misleading). Clarify the comment to accurately reflect the
implementation.
```suggestion
// Follow the redirect by recursively invoking doMkCol
on the new URL
// and return the status code from the final resolved
location
```
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -140,7 +148,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();
+ // Recursive call to handle redirect - execute() will
handle the redirect automatically
+ // but we need to return the final status
+ return doMkCol(redirectUrl);
+ }
Review Comment:
The redirect handling logic has a potential infinite recursion issue. If a
server responds with a redirect that points back to the original URL or creates
a redirect loop, this will cause a stack overflow. The recursive call should
include protection against redirect loops, such as tracking visited URLs or
limiting the maximum number of redirects.
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -122,16 +122,24 @@ 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;
+ }
Review Comment:
The backward traversal loop doesn't handle unexpected status codes properly.
If doMkCol returns a status code that is not CREATED, OK, METHOD_NOT_ALLOWED,
or CONFLICT (e.g., 403 Forbidden, 401 Unauthorized, 500 Internal Server Error),
the loop will continue traversing backwards indefinitely until reaching the
root. This could lead to incorrect behavior or silent failures. Consider
throwing an exception for unexpected error status codes.
```suggestion
}
// 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);
```
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -172,6 +194,29 @@ public void putDirectory(File sourceDirectory, String
destinationDirectory)
}
}
+ protected boolean collectionExists(String url) throws IOException {
+ DavPropertyNameSet nameSet = new DavPropertyNameSet();
+
nameSet.add(DavPropertyName.create(DavConstants.PROPERTY_RESOURCETYPE));
+
+ CloseableHttpResponse closeableHttpResponse = null;
+ HttpPropfind method = null;
+ try {
+ method = new HttpPropfind(url, nameSet, DavConstants.DEPTH_0);
+ closeableHttpResponse = execute(method);
+ int statusCode =
closeableHttpResponse.getStatusLine().getStatusCode();
+ return statusCode == HttpStatus.SC_OK || statusCode ==
HttpStatus.SC_MULTI_STATUS;
+ } catch (HttpException e) {
+ throw new IOException(e.getMessage(), e);
+ } finally {
+ if (method != null) {
+ method.releaseConnection();
+ }
+ if (closeableHttpResponse != null) {
+ closeableHttpResponse.close();
+ }
Review Comment:
The collectionExists method doesn't validate whether the resource is
actually a collection. It returns true for any resource (file or collection)
that returns OK or MULTI_STATUS. According to RFC 4918, PROPFIND should verify
that the resourcetype property contains a collection element. Consider parsing
the response to check if the resource is specifically a collection, similar to
the isDirectory method implementation.
```suggestion
try {
return isDirectory(url);
} catch (DavException e) {
throw new IOException(e.getMessage(), e);
```
##########
wagon-providers/wagon-webdav-jackrabbit/src/test/java/org/apache/maven/wagon/providers/webdav/WebDavWagonTest.java:
##########
@@ -137,34 +137,35 @@ public void testMkdirs() throws Exception {
wagon.connect(testRepository, getAuthInfo());
try {
- File dir = getRepositoryDirectory();
-
- // check basedir also doesn't exist and will need to be created
- dir = new File(dir, testRepository.getBasedir());
- assertFalse(dir.exists());
-
+ String repositoryUrl = testRepository.getUrl();
+
// test leading /
- assertFalse(new File(dir, "foo").exists());
+ String fooUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? ""
: "/") + "foo";
+ assertFalse("Collection should not exist before creation",
wagon.collectionExists(fooUrl));
wagon.mkdirs("/foo");
- assertTrue(new File(dir, "foo").exists());
+ assertTrue("Collection should exist after creation",
wagon.collectionExists(fooUrl));
// test trailing /
- assertFalse(new File(dir, "bar").exists());
+ String barUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? ""
: "/") + "bar";
+ assertFalse("Collection should not exist before creation",
wagon.collectionExists(barUrl));
wagon.mkdirs("bar/");
- assertTrue(new File(dir, "bar").exists());
+ assertTrue("Collection should exist after creation",
wagon.collectionExists(barUrl));
- // test when already exists
+ // test when already exists (should not fail)
wagon.mkdirs("bar");
+ assertTrue("Collection should still exist",
wagon.collectionExists(barUrl));
// test several parts
- assertFalse(new File(dir, "1/2/3/4").exists());
+ String deepUrl = repositoryUrl + (repositoryUrl.endsWith("/") ? ""
: "/") + "1/2/3/4";
+ assertFalse("Deep collection should not exist before creation",
wagon.collectionExists(deepUrl));
wagon.mkdirs("1/2/3/4");
- assertTrue(new File(dir, "1/2/3/4").exists());
+ assertTrue("Deep collection should exist after creation",
wagon.collectionExists(deepUrl));
// test additional part and trailing /
- assertFalse(new File(dir, "1/2/3/4/5").exists());
+ String deeperUrl = repositoryUrl + (repositoryUrl.endsWith("/") ?
"" : "/") + "1/2/3/4/5";
+ assertFalse("Deeper collection should not exist before creation",
wagon.collectionExists(deeperUrl));
wagon.mkdirs("1/2/3/4/5/");
- assertTrue(new File(dir, "1/2/3/4").exists());
+ assertTrue("Deeper collection should exist after creation",
wagon.collectionExists(deeperUrl));
Review Comment:
The URL construction pattern is repetitive and error-prone. The same logic
for appending paths to repositoryUrl is duplicated multiple times. Consider
extracting this into a helper method to reduce duplication and improve
maintainability.
--
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]
