slachiewicz commented on code in PR #898:
URL: https://github.com/apache/maven-wagon/pull/898#discussion_r3738879429
##########
wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/WebDavWagon.java:
##########
@@ -209,21 +191,17 @@ public List<String> getFileList(String
destinationDirectory)
String repositoryUrl = repository.getUrl();
String url = repositoryUrl + (repositoryUrl.endsWith("/") ? "" : "/")
+ destinationDirectory;
- HttpPropfind method = null;
+ DavMethods.HttpPropfind method = null;
CloseableHttpResponse closeableHttpResponse = null;
try {
if (isDirectory(url)) {
- DavPropertyNameSet nameSet = new DavPropertyNameSet();
-
nameSet.add(DavPropertyName.create(DavConstants.PROPERTY_DISPLAYNAME));
-
- method = new HttpPropfind(url, nameSet, DavConstants.DEPTH_1);
+ method = new DavMethods.HttpPropfind(url,
PROPERTY_DISPLAYNAME, DEPTH_1);
closeableHttpResponse = execute(method);
- if (method.succeeded(closeableHttpResponse)) {
+ List<MultiStatus.Response> responses =
readMultiStatus(closeableHttpResponse);
+ if (responses != null) {
ArrayList<String> dirs = new ArrayList<>();
- MultiStatus multiStatus =
method.getResponseBodyAsMultiStatus(closeableHttpResponse);
- for (int i = 0; i < multiStatus.getResponses().length;
i++) {
- MultiStatusResponse response =
multiStatus.getResponses()[i];
- String entryUrl = response.getHref();
+ for (int i = 0; i < responses.size(); i++) {
+ String entryUrl = responses.get(i).getHref();
Review Comment:
Agreed on the diagnosis, declining the fix in this PR.
The `i == 0` skip is not introduced here — it is unchanged from the code
being replaced, which carried the comment "by design jackrabbit WebDAV sticks
parent directory as the first entry". So the fragility predates this change and
is not a regression it causes. The related defect that *was* in this PR — a
dropped response shifting a child into position 0 — is fixed in the latest
commit.
Matching the normalized href against the request URI is the right fix, but
it is a behavioural change to directory listing rather than part of replacing a
dependency, and it is fiddlier than it looks: hrefs arrive as absolute URLs or
as paths, percent-encoded or not, with or without a trailing slash, and
`getFileList` already does its own decoding and trailing-slash handling
downstream. Getting that wrong breaks listing against every server rather than
the non-compliant ones, and this PR's safety net is an integration suite
running against a single WebDAV implementation, which would not catch it.
I would rather land the dependency removal on a green, behaviour-preserving
diff and do the href matching as a separate change that can be argued and
tested on its own. Noted in the PR description as a follow-up.
--
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]
