Repository: PyFilesystem/pyfilesystem2 (python)

Issue:
FTPFS should use MDTM
Regarding fs.ftpfs.FS.getinfo (https://github.com/PyFilesystem/pyfilesystem2/blob/64d7a52e1f3037f33937f91e9db812cafdbbafc0/fs/ftpfs.py#L648)

That function tries to use MLST to retrieve arbitrary information of a single file and defaults to LIST if MLST is not supported. 
This makes `fs.copy_dir_if_newer` incredibly inefficient on systems that do not support MLST. 

Imo, an easy solution would be to possibly support MDTM as a fallback if only the modified datetime is requested, e.g. if `namespace == ("detail", "modified")`.
Another, probably more complex improvement, would be to cache the LIST response rather than call it for every single file.

I can submit a PR for the first suggestion, if I get the okay that this is something that should be included. Possibly also the second, although I'm not sure yet how to best implement that.

Pull request:
Added support for MDTM command in FTPFS
## Type of changes

- Other

## Checklist

- [x] I've run the latest [black](https://github.com/ambv/black) with default args on new code.
- [x] I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
- [x] I've added tests for new code.
- [x] I accept that @PyFilesystem/maintainers may be pedantic in the code review.

## Description

Fixes #456

Added a `getmodified` function to `fs.base.FS` which defaults to calling `getinfo`. In `FTPFS`, a check for the support of `MDTM` was added to provide a more efficient command for this specialized request.

Two calls to `FS.getinfo(...).modified` in `fs.copy` were replaced by calls to `FS.getmodified`.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c24b0e6..968dc67 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Added `fs.copy.copy_file_if`, `fs.copy.copy_dir_if`, and `fs.copy.copy_fs_if`. 
   Closes [#458](https://github.com/PyFilesystem/pyfilesystem2/issues/458).
+- Added `fs.base.FS.getmodified`.
+  
+### Changed
+
+- FTP servers that do not support the MLST command now try to use the MDTM command to
+  retrieve the last modification timestamp of a resource. 
+  Closes [#456](https://github.com/PyFilesystem/pyfilesystem2/pull/456).
   
 ### Fixed
 
diff --git a/docs/source/interface.rst b/docs/source/interface.rst
index e2da135..0d67c0c 100644
--- a/docs/source/interface.rst
+++ b/docs/source/interface.rst
@@ -20,6 +20,7 @@ The following is a complete list of methods on PyFilesystem objects.
 * :meth:`~fs.base.FS.getdetails` Get details info namespace for a resource.
 * :meth:`~fs.base.FS.getinfo` Get info regarding a file or directory.
 * :meth:`~fs.base.FS.getmeta` Get meta information for a resource.
+* :meth:`~fs.base.FS.getmodified` Get info regarding the last modified time of a resource.
 * :meth:`~fs.base.FS.getospath` Get path with encoding expected by the OS.
 * :meth:`~fs.base.FS.getsize` Get the size of a file.
 * :meth:`~fs.base.FS.getsyspath` Get the system path of a resource, if one exists.
diff --git a/fs/base.py b/fs/base.py
index 8d8ee4c..82a5978 100644
--- a/fs/base.py
+++ b/fs/base.py
@@ -697,6 +697,23 @@ class FS(object):
 
     gettext = _new_name(readtext, "gettext")
 
+    def getmodified(self, path):
+        # type: (Text) -> Optional[datetime]
+        """Get the timestamp of the last modifying access of a resource.
+
+        Arguments:
+            path (str): A path to a resource.
+
+        Returns:
+            datetime: The timestamp of the last modification.
+
+        The *modified timestamp* of a file is the point in time
+        that the file was last changed. Depending on the file system,
+        it might only have limited accuracy.
+
+        """
+        return self.getinfo(path, namespaces=["details"]).modified
+
     def getmeta(self, namespace="standard"):
         # type: (Text) -> Mapping[Text, object]
         """Get meta information regarding a filesystem.
diff --git a/fs/copy.py b/fs/copy.py
index 95650b9..6ffd83d 100644
--- a/fs/copy.py
+++ b/fs/copy.py
@@ -463,9 +463,8 @@ def _copy_is_necessary(
 
     elif condition == "newer":
         try:
-            namespace = ("details",)
-            src_modified = src_fs.getinfo(src_path, namespace).modified
-            dst_modified = dst_fs.getinfo(dst_path, namespace).modified
+            src_modified = src_fs.getmodified(src_path)
+            dst_modified = dst_fs.getmodified(dst_path)
         except ResourceNotFound:
             return True
         else:
@@ -477,9 +476,8 @@ def _copy_is_necessary(
 
     elif condition == "older":
         try:
-            namespace = ("details",)
-            src_modified = src_fs.getinfo(src_path, namespace).modified
-            dst_modified = dst_fs.getinfo(dst_path, namespace).modified
+            src_modified = src_fs.getmodified(src_path)
+            dst_modified = dst_fs.getmodified(dst_path)
         except ResourceNotFound:
             return True
         else:
diff --git a/fs/ftpfs.py b/fs/ftpfs.py
index 86f0495..b7a4998 100644
--- a/fs/ftpfs.py
+++ b/fs/ftpfs.py
@@ -40,6 +40,7 @@ from .path import dirname
 from .path import basename
 from .path import normpath
 from .path import split
+from .time import epoch_to_datetime
 from . import _ftp_parse as ftp_parse
 
 if typing.TYPE_CHECKING:
@@ -572,6 +573,12 @@ class FTPFS(FS):
         """bool: whether the server supports MLST feature."""
         return "MLST" in self.features
 
+    @property
+    def supports_mdtm(self):
+        # type: () -> bool
+        """bool: whether the server supports the MDTM feature."""
+        return "MDTM" in self.features
+
     def create(self, path, wipe=False):
         # type: (Text, bool) -> bool
         _path = self.validatepath(path)
@@ -692,8 +699,21 @@ class FTPFS(FS):
         if namespace == "standard":
             _meta = self._meta.copy()
             _meta["unicode_paths"] = "UTF8" in self.features
+            _meta["supports_mtime"] = "MDTM" in self.features
         return _meta
 
+    def getmodified(self, path):
+        # type: (Text) -> Optional[datetime.datetime]
+        if self.supports_mdtm:
+            _path = self.validatepath(path)
+            with self._lock:
+                with ftp_errors(self, path=path):
+                    cmd = "MDTM " + _encode(_path, self.ftp.encoding)
+                    response = self.ftp.sendcmd(cmd)
+                    mtime = self._parse_ftp_time(response.split()[1])
+                    return epoch_to_datetime(mtime)
+        return super(FTPFS, self).getmodified(path)
+
     def listdir(self, path):
         # type: (Text) -> List[Text]
         _path = self.validatepath(path)


Test patch:
diff --git a/tests/test_ftpfs.py b/tests/test_ftpfs.py
index 43028da..d4143aa 100644
--- a/tests/test_ftpfs.py
+++ b/tests/test_ftpfs.py
@@ -261,6 +261,23 @@ class TestFTPFS(FSTestCases, unittest.TestCase):
         del self.fs.features["UTF8"]
         self.assertFalse(self.fs.getmeta().get("unicode_paths"))
 
+    def test_getinfo_modified(self):
+        self.assertIn("MDTM", self.fs.features)
+        self.fs.create("bar")
+        mtime_detail = self.fs.getinfo("bar", ("basic", "details")).modified
+        mtime_modified = self.fs.getmodified("bar")
+        # Microsecond and seconds might not actually be supported by all
+        # FTP commands, so we strip them before comparing if it looks
+        # like at least one of the two values does not contain them.
+        replacement = {}
+        if mtime_detail.microsecond == 0 or mtime_modified.microsecond == 0:
+            replacement["microsecond"] = 0
+        if mtime_detail.second == 0 or mtime_modified.second == 0:
+            replacement["second"] = 0
+        self.assertEqual(
+            mtime_detail.replace(**replacement), mtime_modified.replace(**replacement)
+        )
+
     def test_opener_path(self):
         self.fs.makedir("foo")
         self.fs.writetext("foo/bar", "baz")
diff --git a/tests/test_memoryfs.py b/tests/test_memoryfs.py
index bb5096f..67d92ac 100644
--- a/tests/test_memoryfs.py
+++ b/tests/test_memoryfs.py
@@ -72,14 +72,13 @@ class TestMemoryFS(FSTestCases, unittest.TestCase):
         self.fs.makedir("bar")
         self.fs.touch("foo/file.txt")
 
-        namespaces = ("details", "modified")
-        src_info = self.fs.getinfo("foo/file.txt", namespaces)
+        src_datetime = self.fs.getmodified("foo/file.txt")
 
         self.fs.copy("foo/file.txt", "bar/file.txt", preserve_time=True)
         self.assertTrue(self.fs.exists("bar/file.txt"))
 
-        dst_info = self.fs.getinfo("bar/file.txt", namespaces)
-        self.assertEqual(dst_info.modified, src_info.modified)
+        dst_datetime = self.fs.getmodified("bar/file.txt")
+        self.assertEqual(dst_datetime, src_datetime)
 
 
 class TestMemoryFile(unittest.TestCase):