dcache-webdav: revert improve efficiency of directory listing (14085/14088)

Motivation:

See `RT 10505: Problem with webdav PROPFIND request for dirs with symlinks`
https://rt.dcache.org/Ticket/Display.html?id=10505

The problem may be more generalized than the discussion
suggests.   Since we cut off user requests when at the
`PERFORMANCE` setting, if the user wants locality, for
instance, it fails.

Modification:

We propose to commit #14082, #14086, and #14087 on top
of the reverted base.  We will also add a modification
to ignore quota requests when the default setting is
`PERFORMANCE`.

Result:

reverts commit 3567dde
https://rb.dcache.org/r/14088/
`dcache-webdav:  fix incorrect parameter value given to DcacheDirectoryResource constructor`

reverts commit 707000c.
https://rb.dcache.org/r/14085/
`dcache-webdav: improve efficiency of directory listing – revised`
(metalink addition conflicts resolved)

Target: master
Request: 9.1
Request: 9.0
Request: 8.2
Requires-notes:  absolutely
Patch:  https://rb.dcache.org/r/14101/
Acked-by: Dmitry

Author: alrossi

modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheDirectoryResource.java

@@ -2,7 +2,6 @@
 
 import static io.milton.property.PropertySource.PropertyAccessibility.READ_ONLY;
 import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.dcache.namespace.FileAttribute.STORAGEINFO;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.net.MediaType;
@@ -21,7 +20,6 @@
 import io.milton.http.LockToken;
 import io.milton.http.Range;
 import io.milton.http.Request;
-import io.milton.http.Response;
 import io.milton.http.exceptions.BadRequestException;
 import io.milton.http.exceptions.ConflictException;
 import io.milton.http.exceptions.NotAuthorizedException;
@@ -49,7 +47,6 @@
 import java.util.Map;
 import java.util.Optional;
 import javax.xml.namespace.QName;
-import org.dcache.space.ReservationCaches;
 import javax.xml.stream.XMLStreamException;
 import org.dcache.vehicles.FileAttributes;
 import org.slf4j.Logger;
@@ -94,12 +91,9 @@ private interface EntityWriter {
     private final Map<String,MediaType> supportedResponseMediaTypes = Map.of(
         "metalink", METALINK_ENTITY_TYPE);
 
-    private final boolean _allAttributes;
-
     public DcacheDirectoryResource(DcacheResourceFactory factory,
-          FsPath path, FileAttributes attributes, boolean allAttributes) {
+          FsPath path, FileAttributes attributes) {
         super(factory, path, attributes);
-        _allAttributes = allAttributes;
     }
 
     @Override
@@ -333,12 +327,6 @@ public LockToken createAndLock(String name, LockTimeout timeout, LockInfo lockIn
         return createNullLock();
     }
 
-    private Optional<String> getWriteToken() {
-        return _attributes.isDefined(STORAGEINFO)
-                ? ReservationCaches.writeToken(_attributes)
-                :  _factory.lookupWriteToken(_path);
-    }
-
     @Override
     public Object getProperty(QName name) {
         Object value = super.getProperty(name);
@@ -349,13 +337,11 @@ public Object getProperty(QName name) {
 
         try {
             if (name.equals(QUOTA_AVAILABLE)) {
-                var maybeToken = getWriteToken();
-                return _factory.spaceForToken(maybeToken).getAvailableSpaceInBytes();
+                return _factory.spaceForPath(_path).getAvailableSpaceInBytes();
             }
 
             if (name.equals(QUOTA_USED)) {
-                var maybeToken = getWriteToken();
-                Space space = _factory.spaceForToken(maybeToken);
+                Space space = _factory.spaceForPath(_path);
                 return space.getUsedSizeInBytes() + space.getAllocatedSpaceInBytes();
             }
         } catch (SpaceException e) {
@@ -369,17 +355,14 @@ public Object getProperty(QName name) {
     public PropertyMetaData getPropertyMetaData(QName name) {
         PropertyMetaData metadata = super.getPropertyMetaData(name);
 
-        if (!_allAttributes) {
+        if (!_factory.isSpaceManaged(_path)) {
             return metadata;
         }
 
         // Milton accepts null and PropertyMetaData.UNKNOWN to mean the
         // property is unknown.
         if ((metadata == null || metadata.isUnknown()) && QUOTA_PROPERTIES.contains(name)) {
-            var maybeToken = getWriteToken();
-            if (_factory.isSpaceManaged(maybeToken)) {
-                return READONLY_LONG;
-            }
+            metadata = READONLY_LONG;
         }
 
         return metadata;
@@ -389,12 +372,7 @@ public PropertyMetaData getPropertyMetaData(QName name) {
     public List<QName> getAllPropertyNames() {
         List<QName> genericNames = super.getAllPropertyNames();
 
-        if (!_allAttributes) {
-            return genericNames;
-        }
-
-        var maybeToken = getWriteToken();
-        if (!_factory.isSpaceManaged(maybeToken)) {
+        if (!_factory.isSpaceManaged(_path)) {
             return genericNames;
         }
 

modules/dcache-webdav/src/main/java/org/dcache/webdav/DcacheResourceFactory.java

@@ -18,7 +18,6 @@
 import static org.dcache.namespace.FileAttribute.PNFSID;
 import static org.dcache.namespace.FileAttribute.RETENTION_POLICY;
 import static org.dcache.namespace.FileAttribute.SIZE;
-import static org.dcache.namespace.FileAttribute.STORAGEINFO;
 import static org.dcache.namespace.FileAttribute.TYPE;
 import static org.dcache.namespace.FileAttribute.XATTR;
 import static org.dcache.namespace.FileType.DIR;
@@ -176,10 +175,6 @@ public class DcacheResourceFactory
 
     public static final String TRANSACTION_ATTRIBUTE = "org.dcache.transaction";
 
-    private static final Set<FileAttribute> MINIMALLY_REQUIRED_ATTRIBUTES =
-          EnumSet.of(TYPE, PNFSID, CREATION_TIME, MODIFICATION_TIME, SIZE,
-                MODE, OWNER, OWNER_GROUP);
-
     private static final Set<FileAttribute> REQUIRED_ATTRIBUTES =
           EnumSet.of(TYPE, PNFSID, CREATION_TIME, MODIFICATION_TIME, SIZE,
                 MODE, OWNER, OWNER_GROUP, XATTR);
@@ -190,7 +185,7 @@ public class DcacheResourceFactory
     // Additional attributes needed for PROPFIND requests; e.g., to supply
     // values for properties.
     private static final Set<FileAttribute> PROPFIND_ATTRIBUTES = Sets.union(
-          EnumSet.of(CHECKSUM, ACCESS_LATENCY, RETENTION_POLICY, STORAGEINFO),
+          EnumSet.of(CHECKSUM, ACCESS_LATENCY, RETENTION_POLICY),
           PoolMonitorV5.getRequiredAttributesForFileLocality());
 
     private static final String PROTOCOL_INFO_NAME = "Http";
@@ -205,11 +200,6 @@ public class DcacheResourceFactory
     private static final Splitter PATH_SPLITTER =
           Splitter.on('/').omitEmptyStrings();
 
-    enum PropfindProperties {
-        PERFORMANCE,
-        CLIENT_COMPATIBLE
-    };
-
     private static final PercentEscaper METALINK_NAME_ESCAPER = new PercentEscaper("", false);
 
     // See https://www.iana.org/assignments/hash-function-text-names/hash-function-text-names.xhtml
@@ -261,7 +251,6 @@ enum PropfindProperties {
     private boolean _impatientClientProxied = true;
     private boolean _isOverwriteAllowed;
     private boolean _isAnonymousListingAllowed;
-    private boolean _includeAllAttributesForPropfind;
 
     private String _staticContentPath;
     private ReloadableTemplate _template;
@@ -633,9 +622,9 @@ public Resource getResource(String host, String requestPath) {
     public DcacheResource getResource(FsPath path) {
         checkPathAllowed(path);
 
+        String requestPath = getRequestPath();
         boolean haveRetried = false;
         Subject subject = getSubject();
-        String requestPath = getRequestPath();
 
         try {
             while (true) {
@@ -677,8 +666,7 @@ public DcacheResource getResource(FsPath path) {
      */
     private DcacheResource getResource(FsPath path, FileAttributes attributes) {
         if (attributes.getFileType() == DIR) {
-            return new DcacheDirectoryResource(this, path, attributes,
-                  isFetchAllAttributes());
+            return new DcacheDirectoryResource(this, path, attributes);
         } else {
             return new DcacheFileResource(this, path, attributes);
         }
@@ -947,11 +935,6 @@ public void print(FsPath dir, FileAttributes dirAttr, DirectoryEntry entry) {
         return result;
     }
 
-    public void setDefaultPropfindProperties(PropfindProperties defaultPropfindProperties) {
-        _includeAllAttributesForPropfind =
-              defaultPropfindProperties == PropfindProperties.CLIENT_COMPATIBLE;
-    }
-
     private class FileLocalityWrapper {
 
         private final FileLocality _inner;
@@ -1274,8 +1257,7 @@ public void deleteDirectory(PnfsId pnfsid, FsPath path)
         PnfsCreateEntryMessage reply =
               pnfs.createPnfsDirectory(path.toString(), REQUIRED_ATTRIBUTES);
 
-        return new DcacheDirectoryResource(this, path, reply.getFileAttributes(),
-              isFetchAllAttributes());
+        return new DcacheDirectoryResource(this, path, reply.getFileAttributes());
     }
 
     public void move(FsPath sourcePath, PnfsId pnfsId, FsPath newPath)
@@ -1566,16 +1548,13 @@ private void initializeTransfer(HttpTransfer transfer, Subject subject)
     }
 
     private Set<FileAttribute> buildRequestedAttributes() {
-        boolean all = isFetchAllAttributes();
-
-        Set<FileAttribute> attributes = all ? EnumSet.copyOf(REQUIRED_ATTRIBUTES) :
-              EnumSet.copyOf(MINIMALLY_REQUIRED_ATTRIBUTES);
+        Set<FileAttribute> attributes = EnumSet.copyOf(REQUIRED_ATTRIBUTES);
 
         if (isDigestRequested()) {
             attributes.add(CHECKSUM);
         }
 
-        if (isPropfindRequest() && all) {
+        if (isPropfindRequest()) {
             // FIXME: Unfortunately, Milton parses the request body after
             // requesting the Resource, so we cannot know which additional
             // attributes are being requested; therefore, we must request all
@@ -1586,14 +1565,6 @@ private Set<FileAttribute> buildRequestedAttributes() {
         return attributes;
     }
 
-    private boolean isFetchAllAttributes() {
-        if (!isPropfindRequest()) {
-            return true;
-        }
-
-        return _includeAllAttributesForPropfind;
-    }
-
     /**
      * Return the RFC 3230 Want-Digest header value, if present.  If multiple headers are present
      * then return a single value obtained by taking the values and creating the equivalent
@@ -1651,7 +1622,7 @@ private Optional<Space> lookupSpaceById(String id) {
         }
     }
 
-    public Optional<String> lookupWriteToken(FsPath path) {
+    private Optional<String> lookupWriteToken(FsPath path) {
         try {
             return _writeTokenCache.get(path);
         } catch (ExecutionException e) {
@@ -1662,14 +1633,14 @@ public Optional<String> lookupWriteToken(FsPath path) {
         }
     }
 
-    public Space spaceForToken(Optional<String> maybeToken) throws SpaceException {
-        return maybeToken
+    public Space spaceForPath(FsPath path) throws SpaceException {
+        return lookupWriteToken(path)
               .flatMap(this::lookupSpaceById)
               .orElseThrow(() -> new SpaceException("Path not under space management"));
     }
 
-    public boolean isSpaceManaged(Optional<String> maybeToken) {
-        return maybeToken
+    public boolean isSpaceManaged(FsPath path) {
+        return lookupWriteToken(path)
               .flatMap(this::lookupSpaceById)
               .isPresent();
     }

modules/dcache-webdav/src/main/resources/org/dcache/webdav/webdav.xml

@@ -230,7 +230,6 @@
         <property name="overwriteAllowed" value="${webdav.enable.overwrite}"/>
         <property name="redirectToHttps" value="${webdav.redirect.allow-https}"/>
         <property name="poolMonitor" ref="pool-monitor"/>
-        <property name="defaultPropfindProperties" value="${webdav.default-propfind-properties}"/>
     </bean>
 
     <bean id="pool-manager-handler" class="org.dcache.poolmanager.PoolManagerHandlerSubscriber">

modules/dcache/src/main/java/org/dcache/space/ReservationCaches.java

@@ -251,11 +251,6 @@ public void failure(int rc, Object error) {
                     });
     }
 
-    public static java.util.Optional<String> writeToken(FileAttributes attr) {
-        StorageInfo info = attr.getStorageInfo();
-        return java.util.Optional.ofNullable(info.getMap().get("writeToken"));
-    }
-
     /**
      * Cache queries to discover if a directory has the "WriteToken" tag set.
      */
@@ -267,6 +262,11 @@ public static LoadingCache<FsPath, java.util.Optional<String>> buildWriteTokenLo
               .refreshAfterWrite(5, MINUTES)
               .recordStats()
               .build(new CacheLoader<FsPath, java.util.Optional<String>>() {
+                  private java.util.Optional<String> writeToken(FileAttributes attr) {
+                      StorageInfo info = attr.getStorageInfo();
+                      return java.util.Optional.ofNullable(info.getMap().get("writeToken"));
+                  }
+
                   @Override
                   public java.util.Optional<String> load(FsPath path)
                         throws CacheException, NoRouteToCellException, InterruptedException {

skel/share/defaults/webdav.properties

@@ -763,6 +763,8 @@ webdav.allowed.client.origins =
 #
 (one-of?true|false)webdav.redirect.allow-https=true
 
+
+
 #  ---- Kafka service enabled
 #
 (one-of?true|false|${dcache.enable.kafka})webdav.enable.kafka = ${dcache.enable.kafka}
@@ -775,35 +777,4 @@ webdav.kafka.topic = ${dcache.kafka.topic}
 webdav.kafka.producer.bootstrap.servers =  ${dcache.kafka.bootstrap-servers}
 
 
-(prefix)webdav.kafka.producer.configs =  Configuration for Kafka Producer
-
-#  --- Default PROPFIND properties
-#
-#  The PROPFIND request allows a client to discover information
-#  (properties) of dCache content.  When making a PROPFIND request,
-#  the client normally indicates which properties are of interest.  If
-#  not specified then the WebDAV server (dCache) is free to return a
-#  default set of information.
-#
-#  Certain clients make PROPFIND requests without specifying
-#  the desired set of properties, triggering a default set of
-#  properties.  There are two problems with this: first, the
-#  clients may react badly if information is missing from this default;
-#  second, some of the required properties may have an adverse
-#  performance impact for dCache.
-#
-#  This property controls whether to support client-side requests for
-#  properties beyond a minimal set or not.
-#
-#      PERFORMANCE -- always return only a minimal set of information that does
-#                     not incur any additional overhead. (These are basically
-#                     the same as what a POSIX list request would proviode).
-#
-#      CLIENT_COMPATIBLE -- return the set of information required by
-#                     the clients.  If the client does not specify the properties,
-#                     it will get a maximal default set.
-#
-#  Because the performance impact of the latter under the most common usage scenarios
-#  could be considerable, the default is set to PERFORMANCE.
-#
-(one-of?PERFORMANCE|CLIENT_COMPATIBLE)webdav.default-propfind-properties = PERFORMANCE
+(prefix)webdav.kafka.producer.configs =  Configuration for Kafka Producer