Update GET /data/upload/<GUID> default filename (#1042)

fence/blueprints/data/blueprint.py

@@ -290,8 +290,8 @@ def upload_file(file_id):
     """
     file_name = flask.request.args.get("file_name")
     if not file_name:
-        logger.warning(f"file_name not provided, using GUID: {file_id}")
-        file_name = str(file_id)
+        file_name = str(file_id).replace("/", "_")
+        logger.warning(f"file_name not provided, using '{file_name}'")
 
     result = get_signed_url_for_file("upload", file_id, file_name=file_name)
     return flask.jsonify(result)

fence/sync/sync_users.py

@@ -1481,10 +1481,9 @@ def _sync(self, sess):
                 self.sync_from_local_yaml_file, encrypted=False, logger=self.logger
             )
         except (EnvironmentError, AssertionError) as e:
-            # TODO return an error code so usersync doesn't fail silently
             self.logger.error(str(e))
             self.logger.error("aborting early")
-            return
+            raise
 
         # parse all projects
         user_projects_csv = self.parse_projects(user_projects_csv)
@@ -2334,10 +2333,9 @@ def sync_single_user_visas(self, user, ga4gh_visas, sess=None, expires=None):
                 self.sync_from_local_yaml_file, encrypted=False, logger=self.logger
             )
         except (EnvironmentError, AssertionError) as e:
-            # TODO return an error code so usersync doesn't fail silently
             self.logger.error(str(e))
             self.logger.error("aborting early")
-            return
+            raise
 
         user_projects = dict()
         user_info = dict()

tests/data/test_data.py

@@ -257,6 +257,8 @@ def test_indexd_upload_file_key_error(
     ["gs", "s3", "gs_acl", "s3_acl", "s3_external", "az", "https"],
     indirect=True,
 )
+@pytest.mark.parametrize("guid", ["1", "prefix/1"])
+@pytest.mark.parametrize("file_name", ["some_test_file.txt", None])
 def test_indexd_upload_file_filename(
     client,
     oauth_client,
@@ -268,12 +270,15 @@ def test_indexd_upload_file_filename(
     primary_google_service_account,
     cloud_manager,
     google_signed_url,
+    guid,
+    file_name,
 ):
     """
-    Test ``GET /data/upload/1?file_name=``.
+    Test ``GET /data/upload/<guid>?file_name=<file_name>``.
     """
-    file_name = "some_test_file.txt"
-    path = "/data/upload/1?file_name=" + file_name
+    path = f"/data/upload/{guid}"
+    if file_name:
+        path += "?file_name=" + file_name
     headers = {
         "Authorization": "Bearer "
         + jwt.encode(
@@ -288,7 +293,9 @@ def test_indexd_upload_file_filename(
     response = client.get(path, headers=headers)
     assert response.status_code == 200
     assert "url" in list(response.json.keys())
-    assert file_name in response.json.get("url")
+
+    name_in_url = file_name if file_name else guid.replace("/", "_")
+    assert name_in_url in response.json.get("url")
 
 
 @pytest.mark.parametrize(

tests/dbgap_sync/test_user_sync.py

@@ -50,8 +50,8 @@ def test_sync_missing_file(syncer, monkeypatch, db_session):
     anything with the arborist client
     """
     monkeypatch.setattr(syncer, "sync_from_local_yaml_file", "this-file-is-not-real")
-    # should fail gracefully
-    syncer.sync()
+    with pytest.raises(FileNotFoundError):
+        syncer.sync()
     assert syncer.arborist_client.create_resource.not_called()
     assert syncer.arborist_client.create_role.not_called()
     assert syncer.arborist_client.create_policy.not_called()
@@ -67,8 +67,8 @@ def test_sync_incorrect_user_yaml_file(syncer, monkeypatch, db_session):
         os.path.dirname(os.path.realpath(__file__)), "data/yaml/incorrect_user.yaml"
     )
     monkeypatch.setattr(syncer, "sync_from_local_yaml_file", path)
-    # should fail gracefully
-    syncer.sync()
+    with pytest.raises(AssertionError):
+        syncer.sync()
     assert syncer.arborist_client.create_resource.not_called()
     assert syncer.arborist_client.create_role.not_called()
     assert syncer.arborist_client.create_policy.not_called()