Repository: fatiando/pooch

Issue:
If Unzip/Untar fails because of a non-existent member, calling without members won't work
**Description of the problem:**

Unfortunately #316 still doesn't quite fix the problem of dealing with members in archives. It creates a race condition where calling with members before calling without them will cause the `members=None` call to return an incomplete archive — and in the case of a failed earlier call, it will be empty.

The issue is that if members is None, we still only check for the existence of the unzip directory:

https://github.com/fatiando/pooch/blob/9c57dd38c8fa8d20e2c4f2cfa41ac3d33be78138/pooch/processors.py#L84-L86

But the directory does not necessarily contain every member we need.

**Full code that generated the error**

(With thanks to @dokempf for the example 😃)

```python
import pooch

P = pooch.create(
    path=pooch.os_cache("test"),
    base_url="https://github.com/ssciwr/afwizard-test-data/releases/download/2022-06-09/",
    registry={
        "data.tar.gz": "sha256:fae90a3cf758e2346b81fa0e3b005f2914d059ca182202a1de8f627b1ec0c160"
    }
)
try:
    files1 = P.fetch("data.tar.gz", processor=pooch.Untar(members=["i don't exist"]))
except:
    pass
files2 = P.fetch("data.tar.gz", processor=pooch.Untar())

assert len(files2) > 0
```

The assertion fails, and will fail forever now, even if we remove the faulty fetch! (State is hard. 😅)

```
Downloading file 'data.tar.gz' from 'https://github.com/ssciwr/afwizard-test-data/releases/download/2022-06-09/data.tar.gz' to '/Users/jni/Library/Caches/test'.
Extracting 'i don't exist' from '/Users/jni/Library/Caches/test/data.tar.gz' to '/Users/jni/Library/Caches/test/data.tar.gz.untar'
Traceback (most recent call last):
  File "/Users/jni/projects/play/pooch-test.py", line 16, in <module>
    assert len(files2) > 0
AssertionError
```

**Suggested fix**

At the start of `__call__`, we should check if `members` is None, and if so, set `members` to the full list of files from the archive. Then I think everything else should work nicely. 😊 And it might simplify some of the later conditionals.

**System information**

* Operating system: macOS 13.4
* Python installation (Anaconda, system, ETS): conda
* Version of Python: 3.10
* Version of this package: 1.7.0
* If using conda, paste the output of `conda list` below: (nah you don't want this 😂)

Discussion:
welcome[bot]: 💖 **Thank you for opening your first pull request in this repository!** 💖

A few things to keep in mind:

* [Authorship Guidelines](https://github.com/fatiando/community/blob/main/AUTHORSHIP.md): Our rules for giving you credit for your contributions, including authorship on publications and [Zenodo](https://zenodo.org/communities/fatiando) archives.
* [Code of Conduct](https://github.com/fatiando/community/blob/main/CODE_OF_CONDUCT.md): How we expect people to interact in our projects.

⭐ **No matter what, we are really grateful that you put in the effort to do this!** ⭐

jni: 👋 
leouieda: Hi @jni sorry for the very very long delay on this. We'll try to put out the fire on #373 and then get on this.
leouieda: @jni I think the problem is that you're changing the value of `self.members`. There is a check in `_extract_file` for `self.members is None` to decide how to extract and that check is now always false. I think the fix would be to set a local `members` variable in `__call__` that either is `self._all_members(fname)` or `self.members`.
jni: Thanks @leouieda! I'm sure I have older neglected PRs as a maintainer. 😂 And your suggestion seems to have done the trick! 🚀 
leouieda: @jni sorry for the delay on this. I'll get these documentation issues fixed in main and then merge this asap. Thanks for your help!
jni: @leouieda don't worry, I am very comfortable with long-lived PRs. 😂 I appreciate your efforts! 🙏 Please let me know if I can do anything else.

Patch:
diff --git a/pooch/processors.py b/pooch/processors.py
index dbe1a91..16670f9 100644
--- a/pooch/processors.py
+++ b/pooch/processors.py
@@ -8,6 +8,7 @@
 """
 Post-processing hooks
 """
+import abc
 import os
 import bz2
 import gzip
@@ -19,9 +20,9 @@ from tarfile import TarFile
 from .utils import get_logger
 
 
-class ExtractorProcessor:  # pylint: disable=too-few-public-methods
+class ExtractorProcessor(abc.ABC):  # pylint: disable=too-few-public-methods
     """
-    Base class for extractions from compressed archives.
+    Abstract base class for extractions from compressed archives.
 
     Subclasses can be used with :meth:`pooch.Pooch.fetch` and
     :func:`pooch.retrieve` to unzip a downloaded data file into a folder in the
@@ -34,16 +35,43 @@ class ExtractorProcessor:  # pylint: disable=too-few-public-methods
         If None, will unpack all files in the archive. Otherwise, *members*
         must be a list of file names to unpack from the archive. Only these
         files will be unpacked.
+    extract_dir : str or None
+        If None, files will be unpacked to the default location (a folder in
+        the same location as the downloaded zip file, with a suffix added).
+        Otherwise, files will be unpacked to ``extract_dir``, which is
+        interpreted as a *relative path* (relative to the cache location
+        provided by :func:`pooch.retrieve` or :meth:`pooch.Pooch.fetch`).
 
     """
 
-    # String appended to unpacked archive. To be implemented in subclass
-    suffix = None
-
     def __init__(self, members=None, extract_dir=None):
         self.members = members
         self.extract_dir = extract_dir
 
+    @property
+    @abc.abstractmethod
+    def suffix(self):
+        """
+        String appended to unpacked archive folder name.
+        Only used if extract_dir is None.
+        MUST BE IMPLEMENTED BY CHILD CLASSES.
+        """
+
+    @abc.abstractmethod
+    def _all_members(self, fname):
+        """
+        Return all the members in the archive.
+        MUST BE IMPLEMENTED BY CHILD CLASSES.
+        """
+
+    @abc.abstractmethod
+    def _extract_file(self, fname, extract_dir):
+        """
+        This method receives an argument for the archive to extract and the
+        destination path.
+        MUST BE IMPLEMENTED BY CHILD CLASSES.
+        """
+
     def __call__(self, fname, action, pooch):
         """
         Extract all files from the given archive.
@@ -69,27 +97,23 @@ class ExtractorProcessor:  # pylint: disable=too-few-public-methods
             A list of the full path to all files in the extracted archive.
 
         """
-        if self.suffix is None and self.extract_dir is None:
-            raise NotImplementedError(
-                "Derived classes must define either a 'suffix' attribute or "
-                "an 'extract_dir' attribute."
-            )
         if self.extract_dir is None:
             self.extract_dir = fname + self.suffix
         else:
             archive_dir = fname.rsplit(os.path.sep, maxsplit=1)[0]
             self.extract_dir = os.path.join(archive_dir, self.extract_dir)
+        # Get a list of everyone who is supposed to be in the unpacked folder
+        # so we can check if they are all there or if we need to extract new
+        # files.
+        if self.members is None or not self.members:
+            members = self._all_members(fname)
+        else:
+            members = self.members
         if (
             (action in ("update", "download"))
             or (not os.path.exists(self.extract_dir))
-            or (
-                (self.members is not None)
-                and (
-                    not all(
-                        os.path.exists(os.path.join(self.extract_dir, m))
-                        for m in self.members
-                    )
-                )
+            or not all(
+                os.path.exists(os.path.join(self.extract_dir, m)) for m in members
             )
         ):
             # Make sure that the folder with the extracted files exists
@@ -111,13 +135,6 @@ class ExtractorProcessor:  # pylint: disable=too-few-public-methods
 
         return fnames
 
-    def _extract_file(self, fname, extract_dir):
-        """
-        This method receives an argument for the archive to extract and the
-        destination path. MUST BE IMPLEMENTED BY CHILD CLASSES.
-        """
-        raise NotImplementedError
-
 
 class Unzip(ExtractorProcessor):  # pylint: disable=too-few-public-methods
     """
@@ -146,7 +163,18 @@ class Unzip(ExtractorProcessor):  # pylint: disable=too-few-public-methods
 
     """
 
-    suffix = ".unzip"
+    @property
+    def suffix(self):
+        """
+        String appended to unpacked archive folder name.
+        Only used if extract_dir is None.
+        """
+        return ".unzip"
+
+    def _all_members(self, fname):
+        """Return all members from a given archive."""
+        with ZipFile(fname, "r") as zip_file:
+            return zip_file.namelist()
 
     def _extract_file(self, fname, extract_dir):
         """
@@ -207,7 +235,18 @@ class Untar(ExtractorProcessor):  # pylint: disable=too-few-public-methods
         :meth:`pooch.Pooch.fetch`).
     """
 
-    suffix = ".untar"
+    @property
+    def suffix(self):
+        """
+        String appended to unpacked archive folder name.
+        Only used if extract_dir is None.
+        """
+        return ".untar"
+
+    def _all_members(self, fname):
+        """Return all members from a given archive."""
+        with TarFile.open(fname, "r") as tar_file:
+            return [info.name for info in tar_file.getmembers()]
 
     def _extract_file(self, fname, extract_dir):
         """


Test patch:
diff --git a/pooch/tests/test_processors.py b/pooch/tests/test_processors.py
index 578b478..1a2a1e2 100644
--- a/pooch/tests/test_processors.py
+++ b/pooch/tests/test_processors.py
@@ -14,7 +14,7 @@ import warnings
 import pytest
 
 from .. import Pooch
-from ..processors import Unzip, Untar, ExtractorProcessor, Decompress
+from ..processors import Unzip, Untar, Decompress
 
 from .utils import pooch_test_url, pooch_test_registry, check_tiny_data, capture_log
 
@@ -97,22 +97,6 @@ def test_decompress_fails():
         assert "pooch.Unzip/Untar" in exception.value.args[0]
 
 
-@pytest.mark.network
-def test_extractprocessor_fails():
-    "The base class should be used and should fail when passed to fecth"
-    with TemporaryDirectory() as local_store:
-        # Setup a pooch in a temp dir
-        pup = Pooch(path=Path(local_store), base_url=BASEURL, registry=REGISTRY)
-        processor = ExtractorProcessor()
-        with pytest.raises(NotImplementedError) as exception:
-            pup.fetch("tiny-data.tar.gz", processor=processor)
-        assert "'suffix'" in exception.value.args[0]
-        processor.suffix = "tar.gz"
-        with pytest.raises(NotImplementedError) as exception:
-            pup.fetch("tiny-data.tar.gz", processor=processor)
-        assert not exception.value.args
-
-
 @pytest.mark.network
 @pytest.mark.parametrize(
     "target_path", [None, "some_custom_path"], ids=["default_path", "custom_path"]
@@ -255,3 +239,51 @@ def _unpacking_expected_paths_and_logs(archive, members, path, name):
             log_lines.append(f"Extracting '{member}'")
         true_paths = set(true_paths)
     return true_paths, log_lines
+
+
+@pytest.mark.network
+@pytest.mark.parametrize(
+    "processor_class,extension",
+    [(Unzip, ".zip"), (Untar, ".tar.gz")],
+)
+def test_unpacking_members_then_no_members(processor_class, extension):
+    """
+    Test that calling with valid members then without them works.
+    https://github.com/fatiando/pooch/issues/364
+    """
+    with TemporaryDirectory() as local_store:
+        pup = Pooch(path=Path(local_store), base_url=BASEURL, registry=REGISTRY)
+
+        # Do a first fetch with an existing member
+        processor1 = processor_class(members=["store/tiny-data.txt"])
+        filenames1 = pup.fetch("store" + extension, processor=processor1)
+        assert len(filenames1) == 1
+
+        # Do a second fetch with no members
+        processor2 = processor_class()
+        filenames2 = pup.fetch("store" + extension, processor=processor2)
+        assert len(filenames2) > 1
+
+
+@pytest.mark.network
+@pytest.mark.parametrize(
+    "processor_class,extension",
+    [(Unzip, ".zip"), (Untar, ".tar.gz")],
+)
+def test_unpacking_wrong_members_then_no_members(processor_class, extension):
+    """
+    Test that calling with invalid members then without them works.
+    https://github.com/fatiando/pooch/issues/364
+    """
+    with TemporaryDirectory() as local_store:
+        pup = Pooch(path=Path(local_store), base_url=BASEURL, registry=REGISTRY)
+
+        # Do a first fetch with incorrect member
+        processor1 = processor_class(members=["not-a-valid-file.csv"])
+        filenames1 = pup.fetch("store" + extension, processor=processor1)
+        assert len(filenames1) == 0
+
+        # Do a second fetch with no members
+        processor2 = processor_class()
+        filenames2 = pup.fetch("store" + extension, processor=processor2)
+        assert len(filenames2) > 0