[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

slachiewicz · 2025-12-30T22:20:41Z

Handle 409 Conflict status when parent collections don't exist
Handle redirects on MKCOL operations
Accept 200 OK status for already-existing collections
Fix tests to use WebDAV PROPFIND instead of filesystem checks

Copilot

Pull request overview

This PR updates the WebDavWagon implementation to comply with RFC 4918 WebDAV specifications, improving the robustness of collection creation operations and refactoring tests to use proper WebDAV PROPFIND requests instead of direct filesystem checks.

Enhances MKCOL operation to handle additional RFC 4918 status codes (200 OK, 409 Conflict)
Adds redirect handling for MKCOL requests
Introduces a new collectionExists method for WebDAV-based collection verification

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
WebDavWagon.java	Added RFC 4918 compliance for status code handling in mkdirs(), implemented redirect handling in doMkCol(), and added collectionExists() method for WebDAV-based collection verification
WebDavWagonTest.java	Refactored testMkdirs() and testMkdirsWithNoBasedir() to use WebDAV PROPFIND via collectionExists() instead of filesystem File.exists() checks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-30T22:23:24Z

...gon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java

+            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);
+                }


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.

Copilot · 2025-12-30T22:23:24Z

...gon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java

+            // RFC 4918: 409 Conflict means intermediate collection is missing, continue traversing backwards
+            if (status == HttpStatus.SC_CONFLICT) {
+                continue;
+            }


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.

Suggested change

}

}

// 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);

Copilot · 2025-12-30T22:23:24Z

...gon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java

+        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();
+            }


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.

Suggested change

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();

}

try {

return isDirectory(url);

} catch (DavException e) {

throw new IOException(e.getMessage(), e);

Copilot · 2025-12-30T22:23:25Z

...gon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java

+                    // Recursive call to handle redirect - execute() will handle the redirect automatically
+                    // but we need to return the final status


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.

Suggested change

// Recursive call to handle redirect - execute() will handle the redirect automatically

// but we need to return the final status

// Follow the redirect by recursively invoking doMkCol on the new URL

// and return the status code from the final resolved location

Copilot · 2025-12-30T22:23:25Z

...webdav-jackrabbit/src/test/java/org/apache/maven/wagon/providers/webdav/WebDavWagonTest.java

+            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));


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.

- Handle 409 Conflict status when parent collections don't exist - Handle redirects on MKCOL operations - Accept 200 OK status for already-existing collections - Fix tests to use WebDAV PROPFIND instead of filesystem checks

slachiewicz requested a review from Copilot December 30, 2025 22:20

slachiewicz marked this pull request as draft December 30, 2025 22:21

Copilot started reviewing on behalf of slachiewicz December 30, 2025 22:21 View session

Copilot AI reviewed Dec 30, 2025

View reviewed changes

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918

3f095c3

- Handle 409 Conflict status when parent collections don't exist - Handle redirects on MKCOL operations - Accept 200 OK status for already-existing collections - Fix tests to use WebDAV PROPFIND instead of filesystem checks

slachiewicz force-pushed the vk/03fd-wagon-583-webdav branch from f8c7ea0 to 3f095c3 Compare January 1, 2026 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

Uh oh!

slachiewicz commented Dec 30, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Copilot AI Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-            }
+            }
+            // 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);

		// Recursive call to handle redirect - execute() will handle the redirect automatically
		// but we need to return the final status

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

Are you sure you want to change the base?

[WAGON-583] WebDavWagon and tests now adhere to RFC 4918 #845

Uh oh!

Conversation

slachiewicz commented Dec 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant