[wagon-3.x] Replace the jackrabbit-webdav dependency with the WebDAV requests actually used - #900
Merged
slachiewicz merged 6 commits intoAug 8, 2026
Conversation
MultiStatus was forked here so that responses would keep their document order, which getFileList depends on. Jackrabbit's own MultiStatus has since been changed to a LinkedHashMap, so the fork no longer differs from the class it shadows. Shadowing is the problem: the released jar carried org/apache/jackrabbit/webdav/MultiStatus.class, which also exists in jackrabbit-webdav, so which of the two won depended on classpath order. XmlRequestEntity is left over from Jackrabbit 2.14, which no longer ships or references it, and nothing in this module ever used it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This Wagon speaks a very small subset of WebDAV: MKCOL to create a collection, PROPFIND Depth:0 on resourcetype to tell a collection from a plain resource, and PROPFIND Depth:1 to list a collection. The Depth:1 request nominally asks for displayname but never reads it, deriving every entry from the response href instead. jackrabbit-webdav models all of WebDAV -- locking, observation, ordering, versioning, search, transactions -- so those three requests were pulling in a dependency whose scope far exceeds what is needed. The requests and the multistatus parsing now live in this module, in Wagon's own package rather than squatting Jackrabbit's. The parser keeps the behaviour the callers rely on: responses stay in document order, because getFileList expects the requested collection first per RFC 4918 section 9.1, and hrefs are deduplicated the way Jackrabbit's href-keyed map did. Only propstat elements reporting 200 are consulted for resourcetype. Since a multistatus body is remote input, the parser also refuses DOCTYPE declarations. Behaviour changes in two spots, both in the safe direction: a PROPFIND answering something other than 207 Multi-Status is no longer parsed for a body, and an empty multistatus no longer raises ArrayIndexOutOfBoundsException from isDirectory. The artifactId keeps the jackrabbit name so that consumers do not break. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Do not let parser hardening break the transport. disallow-doctype-decl is a Xerces feature and setFeature rejects features a parser does not know, so a JAXP implementation other than the platform one would have made every PROPFIND fail. It is now applied only if supported, falling back to refusing external entities, and a configuration failure is reported as such instead of being blamed on the response body. Install an ErrorHandler on the DocumentBuilder. Without one the parser writes its own diagnostics to stderr before throwing, so a server answering 207 with a broken body would have littered the build output. Fatal errors still surface as an exception. Keep a 207 carrying no body a transport failure. It was being read as "not a collection", which surfaced as ResourceDoesNotExistException and told the resolver the artifact was simply absent. Also correct the comment about repeated hrefs, which described dropping later duplicates when the last one in fact wins, and record why a propstat without a status is read as successful. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The multistatus javadoc cited RFC 4918 section 9.1 as mandating that a server list the request URI first. It mandates no ordering at all. The assumption is an observed server behaviour and predates this class, which the comment it replaced described accurately; getFileList still skips the first entry as before, but the reason is now stated honestly. HttpStatus.SC_MULTI_STATUS does exist in HttpCore, so the local constant claiming otherwise is gone. Also drop the local-name fallback in isDavElement, which a namespace aware parser can never reach, and send the PROPFIND body as application/xml to match what Jackrabbit put on the wire. Tolerating an absent namespace is kept and now noted as a deliberate relaxation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Where disallow-doctype-decl is unavailable the parser falls back to refusing external entities, but those two calls were allowed to fail as quietly as the first. A parser supporting none of the three would then have read remote input with only secure processing in force, which bounds resource use and says nothing about what the document may reach. The fallback now insists on both entity features and reports a parser it cannot secure as a configuration error. Also deny the external DTD subset, which the entity features do not govern, tolerating parsers that do not recognise the property. The branch only runs on a JAXP implementation other than the platform one, so it cannot be reached from a test; it was exercised by configuring a factory the same way by hand, where a DOCTYPE parses, as it did under Jackrabbit, and an external entity yields empty text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A DAV:response carrying no href was being skipped. That moves the next response into its place, and both callers read meaning into position: isDirectory inspects the first entry, and getFileList takes the first entry to be the collection it asked about. A server omitting an href could therefore have had a child directory silently dropped from a listing and another one classified from the wrong resource. Such a response is now a parse error, as it was before. Stop reading a propstat that carries no status as successful, too. RFC 4918 requires the element, so a propstat without one reports success for nothing, and treating it as 200 contradicted the rule stated on Response.isCollection that only propstats reporting 200 are consulted. It also went beyond porting the behaviour, on the strength of a lenient server nobody has actually met. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Backport that removes the broad jackrabbit-webdav dependency from wagon-webdav-jackrabbit by inlining only the small subset of WebDAV request/response handling the provider actually uses (MKCOL + narrow PROPFIND + 207 Multi-Status parsing).
Changes:
- Removed the
org.apache.jackrabbit:jackrabbit-webdavMaven dependency and eliminated remaining Jackrabbit-namespaced vendored classes. - Introduced minimal in-module WebDAV request implementations (
DavMethods) and a focused207 Multi-Statusparser (MultiStatus), updatingWebDavWagonto use them. - Added
MultiStatusTestcoverage for parsing behaviors and safety checks (including DOCTYPE rejection).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wagon-providers/wagon-webdav-jackrabbit/src/test/java/org/apache/maven/wagon/providers/webdav/MultiStatusTest.java | Adds unit tests for the new Multi-Status parser behavior and safety guarantees. |
| wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java | Switches MKCOL/PROPFIND handling from Jackrabbit types to new in-module request/parser code. |
| wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/MultiStatus.java | Adds an in-module Multi-Status parser with ordering, dedupe, and XML hardening. |
| wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/DavMethods.java | Adds minimal MKCOL/PROPFIND request classes used by WebDavWagon. |
| wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/jackrabbit/webdav/MultiStatus.java | Removes vendored Jackrabbit MultiStatus class from the module. |
| wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/jackrabbit/webdav/client/methods/XmlRequestEntity.java | Removes unused vendored Jackrabbit XmlRequestEntity. |
| wagon-providers/wagon-webdav-jackrabbit/pom.xml | Drops the jackrabbit-webdav dependency block. |
| pom.xml | Updates Javadoc package grouping to no longer include removed Jackrabbit packages. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+32
to
+36
| * deploying, and discovering whether a resource is a collection and what it contains. See | ||
| * <a href="http://www.webdav.org/specs/rfc4918.html">RFC 4918</a>. | ||
| * | ||
| * @since 4.0.0 | ||
| */ |
Comment on lines
+97
to
+99
| /** | ||
| * {@code getFileList} relies on the requested collection arriving first, per RFC 4918 9.1. | ||
| */ |
Comment on lines
+50
to
+54
| * be the requested collection itself. RFC 4918 does not order responses; that a server lists the | ||
| * request URI first is an observed behaviour, and the assumption predates this class. | ||
| * | ||
| * @since 4.0.0 | ||
| */ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #898 to
wagon-3.x.The six commits cherry-picked with one conflict, in
wagon-providers/wagon-webdav-jackrabbit/pom.xml: the branch carriesjackrabbit-webdav2.20.16 with the "latest version compatible with Java 8" comment rather than master's 2.20.17. Since the commit removes the dependency outright, the resolution was to drop the whole block.One further difference needed a decision.
wagon-3.xguards thereleaseConnection()call indoMkColwith a null check that master does not have:The commit that replaces the Jackrabbit request classes moves the assignment up to the declaration, so
methodcan no longer be null and the guard becomes dead. It is folded into that commit rather than left behind, which keeps the two branches identical across every WebDAV source file —git diff masteroverwagon-webdav-jackrabbit/src/main/javais now empty.Testing
wagon-webdav-jackrabbiton this branch: 295 tests, 0 failures. The branch point runs 283; the extra 12 are theMultiStatusTestcases the change brings with it.One caveat worth recording: on the first full-suite run
WebDavsWagonTest.testWagonPutDirectoryWhenDirectoryAlreadyExistsfailed with aTransferFailedExceptionagainst its localhost HTTPS port. It passed on the next three runs in isolation and on a repeat of the full suite, and the branch point behaves the same way, so it looks like a port or timing flake in the HTTPS variant rather than anything this change introduces. Flagging it because it will presumably surface in CI now and then.