slachiewicz commented on code in PR #898: URL: https://github.com/apache/maven-wagon/pull/898#discussion_r3738878512
########## wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/MultiStatus.java: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.wagon.providers.webdav; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import java.io.IOException; +import java.io.InputStream; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; +import org.xml.sax.helpers.DefaultHandler; + +import static org.apache.maven.wagon.providers.webdav.DavMethods.DAV_NAMESPACE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.PROPERTY_RESOURCETYPE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.XML_COLLECTION; + +/** + * The {@code 207 Multi-Status} body of a PROPFIND response, reduced to what this Wagon needs: the + * href of each response, in document order, and whether that response describes a collection. + * <p> + * Responses keep their document order because {@code getFileList} skips the first one, taking it to + * 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 + */ +final class MultiStatus { + + private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl";; + + private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";; + + private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";; + + private final List<Response> responses; + + private MultiStatus(List<Response> responses) { + this.responses = responses; + } + + /** + * A single {@code DAV:response} element. + */ + static final class Response { + private final String href; + private final boolean collection; + + Response(String href, boolean collection) { + this.href = href; + this.collection = collection; + } + + String getHref() { + return href; + } + + /** + * Whether the {@code resourcetype} property carried a {@code collection} child. Only + * {@code propstat} elements with a {@code 200} status are considered; a resource whose + * {@code resourcetype} is absent or empty is not a collection. + */ + boolean isCollection() { + return collection; + } + } + + List<Response> getResponses() { + return responses; + } + + /** + * Parses a multistatus document. + * + * @param in the response body, never {@code null} + * @return the parsed responses, possibly empty but never {@code null} + * @throws IOException if the parser cannot be configured, or if the body is not a well-formed + * multistatus document + */ + static MultiStatus parse(InputStream in) throws IOException { + DocumentBuilder builder; + try { + builder = newDocumentBuilder(); + } catch (ParserConfigurationException e) { + // a configuration problem is not the server's fault, so say so rather than blaming the + // response body + throw new IOException("XML parser configuration error: " + e.getMessage(), e); + } + + Document document; + try { + document = builder.parse(in); + } catch (SAXException e) { + throw new IOException("Cannot parse multistatus response: " + e.getMessage(), e); + } + + Element root = document.getDocumentElement(); + if (root == null || !isDavElement(root, "multistatus")) { + throw new IOException("Expected a DAV:multistatus response body"); + } + + // an href must occur only once per RFC 4918; should a server repeat one, the last wins and + // keeps the position of the first, which is what the Jackrabbit-backed code did. A + // LinkedHashMap also preserves the document order the callers depend on. + Map<String, Response> responses = new LinkedHashMap<>(); + for (Element response : childElements(root, "response")) { + String href = null; + for (Element hrefElement : childElements(response, "href")) { + href = hrefElement.getTextContent(); + break; + } + if (href != null) { + href = href.trim(); + responses.put(href, new Response(href, isCollection(response))); + } Review Comment: Fixed. You are right that the consequence is worse than the dropped response: both callers read meaning into position, so a shifted list makes `isDirectory` inspect the wrong resource and `getFileList` skip the wrong entry — silently, with a plausible-looking listing. A response without an href is now a parse error, which also restores the prior strictness, though as an `IOException` reaching callers as `TransferFailedException` rather than the unchecked `IllegalArgumentException` Jackrabbit let escape. Blank hrefs are rejected too. Two tests added. ########## wagon-providers/wagon-webdav-jackrabbit/src/main/java/org/apache/maven/wagon/providers/webdav/MultiStatus.java: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.wagon.providers.webdav; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import java.io.IOException; +import java.io.InputStream; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.xml.sax.SAXException; +import org.xml.sax.helpers.DefaultHandler; + +import static org.apache.maven.wagon.providers.webdav.DavMethods.DAV_NAMESPACE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.PROPERTY_RESOURCETYPE; +import static org.apache.maven.wagon.providers.webdav.DavMethods.XML_COLLECTION; + +/** + * The {@code 207 Multi-Status} body of a PROPFIND response, reduced to what this Wagon needs: the + * href of each response, in document order, and whether that response describes a collection. + * <p> + * Responses keep their document order because {@code getFileList} skips the first one, taking it to + * 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 + */ +final class MultiStatus { + + private static final String DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl";; + + private static final String EXTERNAL_GENERAL_ENTITIES = "http://xml.org/sax/features/external-general-entities";; + + private static final String EXTERNAL_PARAMETER_ENTITIES = "http://xml.org/sax/features/external-parameter-entities";; + + private final List<Response> responses; + + private MultiStatus(List<Response> responses) { + this.responses = responses; + } + + /** + * A single {@code DAV:response} element. + */ + static final class Response { + private final String href; + private final boolean collection; + + Response(String href, boolean collection) { + this.href = href; + this.collection = collection; + } + + String getHref() { + return href; + } + + /** + * Whether the {@code resourcetype} property carried a {@code collection} child. Only + * {@code propstat} elements with a {@code 200} status are considered; a resource whose + * {@code resourcetype} is absent or empty is not a collection. + */ + boolean isCollection() { + return collection; + } + } + + List<Response> getResponses() { + return responses; + } + + /** + * Parses a multistatus document. + * + * @param in the response body, never {@code null} + * @return the parsed responses, possibly empty but never {@code null} + * @throws IOException if the parser cannot be configured, or if the body is not a well-formed + * multistatus document + */ + static MultiStatus parse(InputStream in) throws IOException { + DocumentBuilder builder; + try { + builder = newDocumentBuilder(); + } catch (ParserConfigurationException e) { + // a configuration problem is not the server's fault, so say so rather than blaming the + // response body + throw new IOException("XML parser configuration error: " + e.getMessage(), e); + } + + Document document; + try { + document = builder.parse(in); + } catch (SAXException e) { + throw new IOException("Cannot parse multistatus response: " + e.getMessage(), e); + } + + Element root = document.getDocumentElement(); + if (root == null || !isDavElement(root, "multistatus")) { + throw new IOException("Expected a DAV:multistatus response body"); + } + + // an href must occur only once per RFC 4918; should a server repeat one, the last wins and + // keeps the position of the first, which is what the Jackrabbit-backed code did. A + // LinkedHashMap also preserves the document order the callers depend on. + Map<String, Response> responses = new LinkedHashMap<>(); + for (Element response : childElements(root, "response")) { + String href = null; + for (Element hrefElement : childElements(response, "href")) { + href = hrefElement.getTextContent(); + break; + } + if (href != null) { + href = href.trim(); + responses.put(href, new Response(href, isCollection(response))); + } + } + return new MultiStatus(Collections.unmodifiableList(new ArrayList<>(responses.values()))); + } + + /** + * Looks for {@code resourcetype/collection} inside any {@code propstat} that reported a + * {@code 200} status. + */ + private static boolean isCollection(Element response) { + for (Element propstat : childElements(response, "propstat")) { + if (!isOkStatus(propstat)) { + continue; + } + for (Element prop : childElements(propstat, "prop")) { + for (Element resourceType : childElements(prop, PROPERTY_RESOURCETYPE)) { + if (!childElements(resourceType, XML_COLLECTION).isEmpty()) { + return true; + } + } + } + } + return false; + } + + /** + * Reads the {@code status} child, whose text is a status line such as {@code HTTP/1.1 200 OK}. + * <p> + * RFC 4918 requires the element, and a {@code propstat} lacking one used to be skipped + * outright, which made every property of such a response invisible. It is read as successful + * here instead, so that a server omitting the status still gets its properties honoured. + */ + private static boolean isOkStatus(Element propstat) { + List<Element> statusElements = childElements(propstat, "status"); + if (statusElements.isEmpty()) { + return true; Review Comment: Fixed — you caught a contradiction I had written into my own code. `Response.isCollection` states that only propstats reporting 200 are consulted, and `isOkStatus` then returned true when the status was absent. Those cannot both be right. Worse, the leniency was speculative. I justified it as making the Wagon work against servers Jackrabbit failed on, but I have no evidence such a server exists — no bug report, nothing in the test suite. Inventing a behavioural divergence inside a dependency port, for a hypothetical, is not something I should have done. It now returns false, and the test asserts that. If a real non-compliant server ever turns up, that is its own change with evidence behind it. -- 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]
