Split format correction into separate method with tests

Only set url_type to api when there's a url if it is not already set to upload (alternatively could check if url looks like a filestore one but I think there are some datasets set up linking to other datasets' resources)

Author: mcarans

requirements.txt

@@ -260,7 +260,7 @@ rpds-py==0.27.0
     #   referencing
 rsa==4.9.1
     # via google-auth
-ruamel-yaml==0.18.14
+ruamel-yaml==0.18.15
     # via hdx-python-utilities
 ruamel-yaml-clib==0.2.12
     # via ruamel-yaml

src/hdx/api/utilities/filestore_helper.py

@@ -62,6 +62,7 @@ def check_filestore_resource(
             int: Status code
         """
         resource_data_to_update.set_types()
+        resource_data_to_update.correct_format(resource_data_to_update.data)
         cls.resource_check_required_fields(resource_data_to_update, **kwargs)
         file_to_upload = resource_data_to_update.get_file_to_upload()
         if file_to_upload:
@@ -120,19 +121,30 @@ def dataset_update_filestore_resource(
             else:
                 # update file if size or hash has changed
                 filestore_resources[resource_index] = file_to_upload
+                resource_data_to_update["resource_type"] = "file.upload"
+                resource_data_to_update["url_type"] = "upload"
+                if "tracking_summary" in resource_data_to_update:
+                    del resource_data_to_update["tracking_summary"]
                 resource_data_to_update["url"] = cls.temporary_url
                 resource_data_to_update["size"] = size
                 resource_data_to_update["hash"] = hash
                 resource_data_to_update._url_backup = None
                 status = 2
-        elif resource_data_to_update.is_marked_data_updated():
-            # Should not output timezone info here
-            resource_data_to_update["last_modified"] = now_utc_notz().isoformat(
-                timespec="microseconds"
-            )
-            resource_data_to_update._data_updated = False
-            status = 0
         else:
-            status = 1
-        resource_data_to_update.set_types()
+            if (
+                "url" in resource_data_to_update
+                and resource_data_to_update.get("url_type") != "upload"
+            ):
+                resource_data_to_update["resource_type"] = "api"
+                resource_data_to_update["url_type"] = "api"
+            if resource_data_to_update.is_marked_data_updated():
+                # Should not output timezone info here
+                resource_data_to_update["last_modified"] = now_utc_notz().isoformat(
+                    timespec="microseconds"
+                )
+                resource_data_to_update._data_updated = False
+                status = 0
+            else:
+                status = 1
+        resource_data_to_update.correct_format(resource_data_to_update.data)
         return status

src/hdx/data/resource.py

@@ -332,37 +332,43 @@ def check_neither_url_filetoupload(self) -> None:
         if self._file_to_upload is None and "url" not in self.data:
             raise HDXError("Either a url or a file to upload must be supplied!")
 
-    def set_types(self, data: Optional[Dict] = None) -> None:
-        """Add resource_type and url_type if not supplied based on url or file to
-        upload. Correct the file type.
+    def correct_format(self, data: Dict = None) -> None:
+        """Correct the format of the file
 
         Args:
-            data (Dict): Resource data. Defaults to None (self.data)
+            data (Dict): Resource data.
 
         Returns:
             None
         """
-        if data is None:
-            data = self.data
-        if self._file_to_upload is None:
-            if "url" in data:
-                data["resource_type"] = "api"
-                data["url_type"] = "api"
-        else:
-            data["resource_type"] = "file.upload"
-            data["url_type"] = "upload"
-            if "tracking_summary" in data:
-                del data["tracking_summary"]
         file_format = data.get("format")
-        if file_format is not None:
-            file_format = self.get_mapped_format(
-                file_format, configuration=self.configuration
+        if file_format is None:
+            return
+        file_format = self.get_mapped_format(
+            file_format, configuration=self.configuration
+        )
+        if not file_format:
+            raise HDXError(
+                f"Supplied file format {file_format} is invalid and could not be mapped to a known type!"
             )
-            if not file_format:
-                raise HDXError(
-                    f"Supplied file type {file_format} is invalid and could not be mapped to a known type!"
-                )
-            data["format"] = file_format
+        data["format"] = file_format
+
+    def set_types(self) -> None:
+        """Add resource_type and url_type if not supplied based on url or file to
+        upload.
+
+        Returns:
+            None
+        """
+        if self._file_to_upload is None:
+            if "url" in self.data:
+                self.data["resource_type"] = "api"
+                self.data["url_type"] = "api"
+        else:
+            self.data["resource_type"] = "file.upload"
+            self.data["url_type"] = "upload"
+            if "tracking_summary" in self.data:
+                del self.data["tracking_summary"]
 
     def check_required_fields(self, ignore_fields: ListTuple[str] = tuple()) -> None:
         """Check that metadata for resource is complete. The parameter ignore_fields
@@ -417,21 +423,29 @@ def _resource_merge_hdx_update(
             else:
                 # update file if size or hash has changed
                 files["upload"] = self._file_to_upload
+                self._old_data["resource_type"] = "file.upload"
+                self._old_data["url_type"] = "upload"
+                if "tracking_summary" in self._old_data:
+                    del self._old_data["tracking_summary"]
                 self._old_data["size"] = size
                 self._old_data["hash"] = hash
                 status = 2
             self._url_backup = None
-        elif data_updated:
-            # Should not output timezone info here
-            self._old_data["last_modified"] = now_utc_notz().isoformat(
-                timespec="microseconds"
-            )
-            self._data_updated = False
-            status = 0
         else:
-            status = 1
+            if "url" in self._old_data:
+                self._old_data["resource_type"] = "api"
+                self._old_data["url_type"] = "api"
+            if data_updated:
+                # Should not output timezone info here
+                self._old_data["last_modified"] = now_utc_notz().isoformat(
+                    timespec="microseconds"
+                )
+                self._data_updated = False
+                status = 0
+            else:
+                status = 1
 
-        self.set_types(self._old_data)
+        self.correct_format(self._old_data)
         # old_data will be merged into data in the next step
         self._merge_hdx_update("resource", "id", files, True, **kwargs)
         return status
@@ -498,6 +512,7 @@ def create_in_hdx(self, **kwargs: Any) -> int:
             return self._resource_merge_hdx_update(**kwargs)
 
         self.set_types()
+        self.correct_format(self.data)
         if "ignore_check" not in kwargs:  # allow ignoring of field checks
             self.check_required_fields()
         files = {}

tests/hdx/__init__.py

@@ -84,7 +84,7 @@ def json(self):
         "id": "de6549d8-268b-4dfe-adaf-a4ae5c8510d5",
         "description": "Resource1",
         "name": "Resource1",
-        "url": "http://resource1.xlsx",
+        "url": "http://data.humdata.org/dataset/resource/resource1.xlsx",
         "format": "xlsx",
     },
     {
@@ -148,9 +148,9 @@ def json(self):
             "hash": "",
             "package_id": "6f36a41c-f126-4b18-aaaf-6c2ddfbc5d4d",
             "name": "Resource1",
-            "url": "http://resource1.xlsx",
-            "resource_type": "api",
-            "url_type": "api",
+            "url": "http://data.humdata.org/dataset/resource/resource1.xlsx",
+            "resource_type": "file.upload",
+            "url_type": "upload",
             "size": None,
             "mimetype_inner": None,
             "cache_last_updated": None,

tests/hdx/data/test_dataset_core.py

@@ -365,6 +365,12 @@ def post(url, data, headers, files, allow_redirects, auth=None):
                                 datadict["resources"][i] = resultdictcopy["resources"][
                                     i
                                 ]
+                            if datadict["id"] == "TEST5":
+                                resultdictcopy["resources"][i].update(
+                                    datadict["resources"][i]
+                                )
+                        if datadict["id"] == "TEST5":
+                            del datadict["resources"]
                         merge_two_dictionaries(resultdictcopy, datadict)
                         for i, resource in enumerate(resultdictcopy["resources"]):
                             resource["package_id"] = resultdictcopy["id"]

tests/hdx/data/test_resource.py

@@ -678,26 +678,37 @@ def test_read_from_hdx(self, configuration, read):
         with pytest.raises(HDXError):
             Resource.read_from_hdx("ABC")
 
-    def test_check_types_url_filetoupload(self, configuration):
-        resource_data_copy = copy.deepcopy(resource_data)
-        del resource_data_copy["url"]
-        resource = Resource(resource_data_copy)
+    def test_url_filetoupload(self, configuration):
+        resource = Resource(resource_data)
+        del resource["url"]
         with pytest.raises(HDXError):
             resource.check_neither_url_filetoupload()
-        resource.set_types()
         resource.set_file_to_upload("abc")
-        resource.set_types()
         resource["url"] = "lala"
         with pytest.raises(HDXError):
             resource.check_both_url_filetoupload()
-        resource = Resource(resource_data_copy)
+
+    def test_check_types(self, configuration):
+        resource = Resource(resource_data)
+        resource.set_types()
+        assert resource["resource_type"] == "api"
+        assert resource["url_type"] == "api"
+        resource.set_file_to_upload("abc")
+        resource.set_types()
+        assert resource["resource_type"] == "file.upload"
+        assert resource["url_type"] == "upload"
+
+    def test_correct_format(self, configuration):
+        resource = Resource(resource_data)
+        resource["format"] = "XLSX"
+        resource.correct_format(resource.data)
+        assert resource.get_format() == "xlsx"
         resource["format"] = "NOTEXIST"
         with pytest.raises(HDXError):
-            resource.set_types()
-        with pytest.raises(HDXError):
-            resource.set_format("NOTEXIST")
+            resource.correct_format(resource.data)
         del resource["format"]
-        resource.set_types()
+        resource.correct_format(resource.data)
+        assert resource.get_format() is None
 
     def test_get_set_date_of_resource(self, configuration):
         resource = Resource({"daterange_for_data": "[2020-01-07T00:00:00 TO *]"})