Repository: CS-SI/eodag (python)

Issue:
Default behaviour for extracting products
I've just used `eodag-sentinelsat` and was surprised to see I had to set the `extract` parameter to a value (True or False). Before that, I never set this value and it seems to me that products were always extracted.

Currently in providers.yml:
* usgs: true
* aws_eos: (irrelevant as AWS download)
* theia: true
* peps: true
* sobloo: true
* creodias: true
* mundi: true
* (wekeo): broken now
* onda: true
* astraea_eod: (irrelevant as AWS download)
* usgs_satapi_aws: (irrelevant as AWS download)
* earth_search: (irrelevant as AWS download)
* earth_search_cog: (irrelevant as COG)

The logic in `plugins.download.base.Download._finalize` is to set it to False if `extract` is not provided (either dynamically through kwargs or via configuration):
https://github.com/CS-SI/eodag/blob/0632e4147f26119bea43e0672f6d06e75d216a20/eodag/plugins/download/base.py#L151-L161

It is called by `HttpDownload.download` and `UsgsApi.download` (and by `Download._prepare_download` to finalize extracting a product if it couldn't be achieved previously for some reason).

The template indicates:
```
            extract:  # whether to extract the downloaded products (true or false).
```

Should we:

* Specify that the default behavior is to automatically extract products?
* Update `_finalize` accordingly?
* Indicate to plugin developers that they should extract automatically by default?
* Update eodag-sentinelsat accordingly?

Pull request:
Fix auto extract
Fixes #238 

Patch:
diff --git a/eodag/plugins/download/base.py b/eodag/plugins/download/base.py
index bfcb8ac1..46632f7d 100644
--- a/eodag/plugins/download/base.py
+++ b/eodag/plugins/download/base.py
@@ -199,7 +199,7 @@ class Download(PluginTopic):
         """
         extract = kwargs.pop("extract", None)
         extract = (
-            extract if extract is not None else getattr(self.config, "extract", False)
+            extract if extract is not None else getattr(self.config, "extract", True)
         )
         outputs_extension = kwargs.pop("outputs_extension", ".zip")
 
diff --git a/eodag/resources/user_conf_template.yml b/eodag/resources/user_conf_template.yml
index a3fdf94e..a3b92f90 100644
--- a/eodag/resources/user_conf_template.yml
+++ b/eodag/resources/user_conf_template.yml
@@ -19,8 +19,8 @@ peps:
     priority: # Lower value means lower priority (Default: 1)
     search:  # Search parameters configuration
     download:
-        extract:  # whether to extract the downloaded products (true or false).
-        outputs_prefix: # where to store downloaded products.
+        extract:  # whether to extract the downloaded products, only applies to archived products (true or false, Default: true).
+        outputs_prefix: # where to store downloaded products, as an absolute file path (Default: local temporary directory)
         dl_url_params:  # additional parameters to pass over to the download url as an url parameter
     auth:
         credentials:


Test patch:
diff --git a/tests/units/test_eoproduct.py b/tests/units/test_eoproduct.py
index 0faf7704..29da9d2c 100644
--- a/tests/units/test_eoproduct.py
+++ b/tests/units/test_eoproduct.py
@@ -283,29 +283,36 @@ class TestEOProduct(EODagTestCase):
         downloader = HTTPDownload(provider=self.provider, config=dl_config)
         product.register_downloader(downloader, None)
 
-        # Download
-        product_file_path = product.download()
-
-        # Check that the mocked request was properly called.
-        self.requests_http_get.assert_called_with(
-            self.download_url, stream=True, auth=None, params={}
-        )
-        download_records_dir = pathlib.Path(product_file_path).parent / ".downloaded"
-        # A .downloaded folder should be created, including a text file that
-        # lists the downloaded product by their url
-        self.assertTrue(download_records_dir.is_dir())
-        files_in_records_dir = list(download_records_dir.iterdir())
-        self.assertEqual(len(files_in_records_dir), 1)
-        records_file = files_in_records_dir[0]
-        actual_download_url = records_file.read_text()
-        self.assertEqual(actual_download_url, self.download_url)
-        # Check that the downloaded product is a zipfile
-        self.assertTrue(zipfile.is_zipfile(product_file_path))
-
-        # Teardown
-        os.remove(product_file_path)
-        os.remove(str(records_file))
-        os.rmdir(str(download_records_dir))
+        try:
+            # Download
+            product_dir_path = product.download()
+
+            # Check that the mocked request was properly called.
+            self.requests_http_get.assert_called_with(
+                self.download_url, stream=True, auth=None, params={}
+            )
+            download_records_dir = pathlib.Path(product_dir_path).parent / ".downloaded"
+            # A .downloaded folder should be created, including a text file that
+            # lists the downloaded product by their url
+            self.assertTrue(download_records_dir.is_dir())
+            files_in_records_dir = list(download_records_dir.iterdir())
+            self.assertEqual(len(files_in_records_dir), 1)
+            records_file = files_in_records_dir[0]
+            actual_download_url = records_file.read_text()
+            self.assertEqual(actual_download_url, self.download_url)
+            # Since extraction is True by default, check that the returned path is the
+            # product's directory.
+            self.assertTrue(os.path.isdir(product_dir_path))
+            # Check that the ZIP file is still there
+            product_dir_path = pathlib.Path(product_dir_path)
+            product_zip = product_dir_path.parent / (product_dir_path.name + ".zip")
+            self.assertTrue(zipfile.is_zipfile(product_zip))
+        finally:
+            # Teardown
+            os.remove(product_zip)
+            shutil.rmtree(product_dir_path)
+            os.remove(str(records_file))
+            os.rmdir(str(download_records_dir))
 
     def test_eoproduct_download_http_extract(self):
         """eoproduct.download over must be able to extract a product"""
@@ -324,24 +331,25 @@ class TestEOProduct(EODagTestCase):
         downloader = HTTPDownload(provider=self.provider, config=dl_config)
         product.register_downloader(downloader, None)
 
-        # Download
-        product_dir_path = product.download()
-        product_dir_path = pathlib.Path(product_dir_path)
-        # The returned path must be a directory.
-        self.assertTrue(product_dir_path.is_dir())
-        # Check that the extracted dir has at least one file, there are more
-        # but that should be enough.
-        self.assertGreaterEqual(len(list(product_dir_path.glob("**/*"))), 1)
-        # The zip file should is around
-        product_zip_file = product_dir_path.with_suffix(".SAFE.zip")
-        self.assertTrue(product_zip_file.is_file)
-
-        download_records_dir = pathlib.Path(product_dir_path).parent / ".downloaded"
-
-        # Teardown
-        shutil.rmtree(str(product_dir_path))
-        os.remove(str(product_zip_file))
-        shutil.rmtree(str(download_records_dir))
+        try:
+            # Download
+            product_dir_path = product.download()
+            product_dir_path = pathlib.Path(product_dir_path)
+            # The returned path must be a directory.
+            self.assertTrue(product_dir_path.is_dir())
+            # Check that the extracted dir has at least one file, there are more
+            # but that should be enough.
+            self.assertGreaterEqual(len(list(product_dir_path.glob("**/*"))), 1)
+            # The zip file should is around
+            product_zip_file = product_dir_path.with_suffix(".SAFE.zip")
+            self.assertTrue(product_zip_file.is_file)
+
+            download_records_dir = pathlib.Path(product_dir_path).parent / ".downloaded"
+        finally:
+            # Teardown
+            shutil.rmtree(str(product_dir_path))
+            os.remove(str(product_zip_file))
+            shutil.rmtree(str(download_records_dir))
 
     def test_eoproduct_download_http_dynamic_options(self):
         """eoproduct.download must accept the download options to be set automatically"""
@@ -358,34 +366,35 @@ class TestEOProduct(EODagTestCase):
 
         output_dir_name = "_testeodag"
         output_dir = pathlib.Path(tempfile.gettempdir()) / output_dir_name
-        if output_dir.is_dir():
+        try:
+            if output_dir.is_dir():
+                shutil.rmtree(str(output_dir))
+            output_dir.mkdir()
+
+            # Download
+            product_dir_path = product.download(
+                outputs_prefix=str(output_dir),
+                extract=True,
+                dl_url_params={"fakeparam": "dummy"},
+            )
+            # Check that dl_url_params are properly passed to the GET request
+            self.requests_http_get.assert_called_with(
+                self.download_url, stream=True, auth=None, params={"fakeparam": "dummy"}
+            )
+            # Check that "outputs_prefix" is respected.
+            product_dir_path = pathlib.Path(product_dir_path)
+            self.assertEqual(product_dir_path.parent.name, output_dir_name)
+            # We've asked to extract the product so there should be a directory.
+            self.assertTrue(product_dir_path.is_dir())
+            # Check that the extracted dir has at least one file, there are more
+            # but that should be enough.
+            self.assertGreaterEqual(len(list(product_dir_path.glob("**/*"))), 1)
+            # The downloaded zip file is still around
+            product_zip_file = product_dir_path.with_suffix(".SAFE.zip")
+            self.assertTrue(product_zip_file.is_file)
+        finally:
+            # Teardown (all the created files are within outputs_prefix)
             shutil.rmtree(str(output_dir))
-        output_dir.mkdir()
-
-        # Download
-        product_dir_path = product.download(
-            outputs_prefix=str(output_dir),
-            extract=True,
-            dl_url_params={"fakeparam": "dummy"},
-        )
-        # Check that dl_url_params are properly passed to the GET request
-        self.requests_http_get.assert_called_with(
-            self.download_url, stream=True, auth=None, params={"fakeparam": "dummy"}
-        )
-        # Check that "outputs_prefix" is respected.
-        product_dir_path = pathlib.Path(product_dir_path)
-        self.assertEqual(product_dir_path.parent.name, output_dir_name)
-        # We've asked to extract the product so there should be a directory.
-        self.assertTrue(product_dir_path.is_dir())
-        # Check that the extracted dir has at least one file, there are more
-        # but that should be enough.
-        self.assertGreaterEqual(len(list(product_dir_path.glob("**/*"))), 1)
-        # The downloaded zip file is still around
-        product_zip_file = product_dir_path.with_suffix(".SAFE.zip")
-        self.assertTrue(product_zip_file.is_file)
-
-        # Teardown (all the created files are within outputs_prefix)
-        shutil.rmtree(str(output_dir))
 
     def _download_response_archive(self):
         class Response(object):