slachiewicz commented on PR #898: URL: https://github.com/apache/maven-wagon/pull/898#issuecomment-5221940418
A second review pass turned up two things the new code asserted about itself that were simply untrue. Both corrected in the latest commit. **The javadoc invented an RFC guarantee.** It said `getFileList` may take the first response to be the requested collection "as mandated by RFC 4918 section 9.1". RFC 4918 mandates no response ordering whatsoever. The comment this replaced had it right — it attributed first-entry-is-parent to observed server behaviour. The `i == 0` skip is unchanged from before this PR, so there is no regression, but the justification is now stated honestly rather than dressed up as a spec requirement. Worth recording for whoever touches this next: against a server that does not list the request URI first, the parent collection stays in the listing and the first child collection is dropped. That is pre-existing behaviour, not something introduced here. The robust fix is to match hrefs against the request path instead of relying on position — out of scope for this PR. **`HttpStatus.SC_MULTI_STATUS` exists.** A local constant was added with a comment claiming HttpCore does not define it. It does, as `207`, in httpcore 4.4.16. The constant is gone and the library one is used. Also in this commit: dropped the local-name fallback in `isDavElement`, which a namespace-aware parser can never reach, and switched the PROPFIND body to `application/xml; charset=UTF-8`, which is what Jackrabbit put on the wire (`text/xml` is equally legal per RFC 4918 §8.2, but matching the old bytes is the safer port). ### Deliberate relaxations versus the Jackrabbit behaviour Both are documented in javadoc and covered by tests, and both make this Wagon work against servers it previously failed on: - A `propstat` **without** a `DAV:status` is read as successful. `MultiStatusResponse.createFromXml` required the element and dropped every property of such a response, so a directory served that way was reported "not a collection" and `getFileList` then threw `ResourceDoesNotExistException`. - Elements in **no namespace** are accepted; Jackrabbit required `DAV:` exactly. Three incidental robustness gains over the code being replaced, all from the old `MultiStatusResponse` handling: an empty multistatus no longer throws `ArrayIndexOutOfBoundsException` out of `isDirectory`, a response missing its `href` no longer throws `IllegalArgumentException`, and an empty `resourcetype` no longer risks a cast/NPE. 293 tests pass, 0 checkstyle violations. -- 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]
