Merge pull request #161 from LSSTDESC/u/jrbogart/allowing_duplicate_r…

…elpath

U/jrbogart/allowing duplicate relpath

src/dataregistry/registrar/dataset.py

@@ -15,7 +15,6 @@
     _bump_version,
     _copy_data,
     _form_dataset_path,
-    _name_from_relpath,
     _parse_version_string,
     _read_configuration_file,
     get_directory_info,
@@ -68,7 +67,7 @@ def _validate_register_inputs(
         if name is None or version is None:
             raise ValueError("A valid `name` and `version` are required")
         for att in [name, version]:
-            if type(att) != str:
+            if not isinstance(att, str):
                 raise ValueError(f"{att} is not a valid string")
 
         # Make sure `name` is legal (i.e., no illegal characters)
@@ -122,7 +121,7 @@ def _validate_register_inputs(
 
         # Make sure owner type is valid
         if kwargs_dict["owner_type"] not in self._OWNER_TYPES:
-            raise ValueError(f"{owner_type} is not a valid owner_type")
+            raise ValueError(f"{kwargs_dict['owner_type']} is not a valid owner_type")
 
         # Checks for production datasets
         if kwargs_dict["owner_type"] == "production":
@@ -437,6 +436,7 @@ def register(
 
         # Make sure the relative_path in the `root_dir` is avaliable
         if kwargs_dict["location_type"] in ["dataregistry", "dummy"]:
+            will_copy = kwargs_dict["old_location"]
             previous_datasets = self._find_previous(
                 None,
                 None,
@@ -456,17 +456,34 @@ def register(
                     root_dir=self._root_dir,
                 )
 
+                warned = False
                 if get_dataset_status(previous_datasets[-1].status, "archived"):
-                    raise ValueError(
-                        f"Relative path {dest} is reserved "
-                        f"for archived datasetid={previous_datasets[-1].dataset_id}"
-                    )
+                    if will_copy:
+                        raise ValueError(
+                            f"Relative path {dest} is reserved "
+                            f"for archived datasetid={previous_datasets[-1].dataset_id}"
+                        )
+                    else:
+                        warnings.warn(
+                            "Warning: found existing entry with path "
+                            f"{kwargs_dict['relative_path']}",
+                            UserWarning,
+                        )
+                        warned = True
 
                 if not get_dataset_status(previous_datasets[-1].status, "deleted"):
-                    raise ValueError(
-                        f"Relative path {dest} is taken by "
-                        f"datasetid={previous_datasets[-1].dataset_id}"
-                    )
+                    if will_copy:
+                        raise ValueError(
+                            "Relative path {dest} is taken by "
+                            f"datasetid={previous_datasets[-1].dataset_id}"
+                        )
+                    else:
+                        if not warned:
+                            warnings.warn(
+                                "Warning: found existing entry with path "
+                                f"{kwargs_dict['relative_path']}",
+                                UserWarning,
+                            )
 
         # Make sure there is not already a database entry with this
         # name/version combination
@@ -571,7 +588,7 @@ def replace(
                 raise ValueError(f"Dataset {full_name} does not exist")
 
             # Cannot replace (valid) non-overwritable datasets
-            if previous_datasets[-1].is_overwritable == False and get_dataset_status(
+            if previous_datasets[-1].is_overwritable is False and get_dataset_status(
                 previous_datasets[-1].status, "valid"
             ):
                 raise ValueError(
@@ -865,7 +882,7 @@ def _validate_keywords(self, keywords):
 
         for k in keywords:
             # Make sure keyword is a string
-            if type(k) != str:
+            if not isinstance(k, str):
                 raise ValueError(f"{k} is not a valid keyword string")
 
         # Make sure keywords are all in the keywords table
@@ -885,7 +902,7 @@ def _validate_keywords(self, keywords):
 
         # Keyword not found
         if len(keyword_ids) != len(keywords):
-            raise ValueError(f"Not all keywords selected are registered")
+            raise ValueError("Not all keywords selected are registered")
 
         return keyword_ids
 
@@ -904,11 +921,11 @@ def add_keywords(self, dataset_id, keywords):
         """
 
         # Make sure things are valid
-        if type(keywords) != list:
+        if not isinstance(keywords, list):
             raise ValueError("Passed keywords object must be a list")
 
         for k in keywords:
-            if type(k) != str:
+            if not isinstance(k, str):
                 raise ValueError(f"Keyword {k} is not a valid string")
 
         if len(keywords) == 0:

tests/end_to_end_tests/test_register_dataset_real_data.py

@@ -54,6 +54,7 @@ def test_copy_data(dummy_file, data_org):
     [
         ("file", "file1.txt"),
         ("directory", "dummy_dir"),
+        ("same_directory", "dummy_dir")
     ],
 )
 def test_on_location_data(dummy_file, data_org, data_path):
@@ -84,9 +85,12 @@ def test_on_location_data(dummy_file, data_org, data_path):
         [f],
     )
 
-    assert len(results["dataset.data_org"]) == 1
+    if data_org == "same_directory":
+        assert len(results["dataset.data_org"]) == 2
+    else:
+        assert len(results["dataset.data_org"]) == 1
 
-    assert results["dataset.data_org"][0] == data_org
+    assert data_org.endswith(results["dataset.data_org"][0])
     assert results["dataset.nfiles"][0] == 1
     assert results["dataset.total_disk_space"][0] > 0