diff --git a/dandiapi/api/management/commands/create_dev_dandiset.py b/dandiapi/api/management/commands/create_dev_dandiset.py index 54f6e6d61..8b1ae1520 100644 --- a/dandiapi/api/management/commands/create_dev_dandiset.py +++ b/dandiapi/api/management/commands/create_dev_dandiset.py @@ -42,10 +42,14 @@ def create_dev_dandiset(name: str, email: str): 'schemaVersion': settings.DANDI_SCHEMA_VERSION, 'encodingFormat': 'text/plain', 'schemaKey': 'Asset', - 'path': 'foo/bar.txt', } + asset_path = 'foo/bar.txt' asset = add_asset_to_version( - user=owner, version=draft_version, asset_blob=asset_blob, metadata=asset_metadata + user=owner, + version=draft_version, + asset_blob=asset_blob, + metadata=asset_metadata, + path=asset_path, ) calculate_sha256(blob_id=asset_blob.blob_id) diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index a87fd4fc5..a6b2f8f67 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -44,6 +44,7 @@ def change_asset( version: Version, new_asset_blob: AssetBlob | EmbargoedAssetBlob | None = None, new_zarr_archive: ZarrArchive | None = None, + new_path: str, new_metadata: dict, ) -> tuple[Asset, bool]: """ @@ -55,26 +56,24 @@ def change_asset( assert ( new_asset_blob or new_zarr_archive ), 'One of new_zarr_archive or new_asset_blob must be given to change_asset_metadata' - assert 'path' in new_metadata, 'Path must be present in new_metadata' if not user.has_perm('owner', version.dandiset): raise DandisetOwnerRequired() elif version.version != 'draft': raise DraftDandisetNotModifiable() - path = new_metadata['path'] new_metadata_stripped = Asset.strip_metadata(new_metadata) if not asset.is_different_from( asset_blob=new_asset_blob, zarr_archive=new_zarr_archive, metadata=new_metadata_stripped, - path=path, + path=new_path, ): return asset, False # Verify we aren't changing path to the same value as an existing asset - if version.assets.filter(path=path).exclude(asset_id=asset.asset_id).exists(): + if version.assets.filter(path=new_path).exclude(asset_id=asset.asset_id).exists(): raise AssetAlreadyExists() with transaction.atomic(): @@ -85,6 +84,7 @@ def change_asset( version=version, asset_blob=new_asset_blob, zarr_archive=new_zarr_archive, + path=new_path, metadata=new_metadata, ) # Set previous asset and save @@ -100,13 +100,13 @@ def add_asset_to_version( version: Version, asset_blob: AssetBlob | EmbargoedAssetBlob | None = None, zarr_archive: ZarrArchive | None = None, + path: str, metadata: dict, ) -> Asset: """Create an asset, adding it to a version.""" assert ( asset_blob or zarr_archive ), 'One of zarr_archive or asset_blob must be given to add_asset_to_version' - assert 'path' in metadata, 'Path must be present in metadata' if not user.has_perm('owner', version.dandiset): raise DandisetOwnerRequired() @@ -114,7 +114,6 @@ def add_asset_to_version( raise DraftDandisetNotModifiable() # Check if there are already any assets with the same path - path = metadata['path'] if version.assets.filter(path=path).exists(): raise AssetAlreadyExists() diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index d048e7329..2f1a37190 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -453,7 +453,6 @@ def test_asset_create(api_client, user, draft_version, asset_blob): path = 'test/create/asset.txt' metadata = { 'encodingFormat': 'application/x-nwb', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -462,7 +461,7 @@ def test_asset_create(api_client, user, draft_version, asset_blob): resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'blob_id': asset_blob.blob_id}, + {'path': path, 'metadata': metadata, 'blob_id': asset_blob.blob_id}, format='json', ).json() new_asset = Asset.objects.get(asset_id=resp['asset_id']) @@ -499,6 +498,29 @@ def test_asset_create(api_client, user, draft_version, asset_blob): assert draft_version.status == Version.Status.PENDING +@pytest.mark.django_db +def test_asset_create_incorrect_path_spec(api_client, user, draft_version, asset_blob): + assign_perm('owner', user, draft_version.dandiset) + api_client.force_authenticate(user=user) + + path = 'foo/bar' + metadata = { + 'path': path, + 'schemaVersion': settings.DANDI_SCHEMA_VERSION, + 'encodingFormat': 'application/x-nwb', + } + + # Intentionally specify path in both metadata and at top level + resp = api_client.post( + f'/api/dandisets/{draft_version.dandiset.identifier}' + f'/versions/{draft_version.version}/assets/', + {'path': path, 'metadata': metadata, 'blob_id': asset_blob.blob_id}, + format='json', + ) + assert resp.status_code == 400 + assert resp.json() == {'metadata': ['Path must only be specified at top level.']} + + @pytest.mark.parametrize( 'path,expected_status_code', [ @@ -527,13 +549,12 @@ def test_asset_create_path_validation( metadata = { 'schemaVersion': settings.DANDI_SCHEMA_VERSION, 'encodingFormat': 'application/x-nwb', - 'path': path, } resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'blob_id': asset_blob.blob_id}, + {'path': path, 'metadata': metadata, 'blob_id': asset_blob.blob_id}, format='json', ) @@ -550,10 +571,8 @@ def test_asset_create_conflicting_path(api_client, user, draft_version, asset_bl user=user, version=draft_version, asset_blob=asset_blob, - metadata={ - 'path': 'foo/bar.txt', - 'schemaVersion': settings.DANDI_SCHEMA_VERSION, - }, + path='foo/bar.txt', + metadata={'schemaVersion': settings.DANDI_SCHEMA_VERSION}, ) # Add an asset that has a path which fully contains that of the first asset @@ -562,10 +581,8 @@ def test_asset_create_conflicting_path(api_client, user, draft_version, asset_bl user=user, version=draft_version, asset_blob=asset_blob, - metadata={ - 'path': 'foo/bar.txt/baz.txt', - 'schemaVersion': settings.DANDI_SCHEMA_VERSION, - }, + path='foo/bar.txt/baz.txt', + metadata={'schemaVersion': settings.DANDI_SCHEMA_VERSION}, ) # Add an asset that's path is fully contained by the first asset @@ -574,10 +591,8 @@ def test_asset_create_conflicting_path(api_client, user, draft_version, asset_bl user=user, version=draft_version, asset_blob=asset_blob, - metadata={ - 'path': 'foo', - 'schemaVersion': settings.DANDI_SCHEMA_VERSION, - }, + path='foo', + metadata={'schemaVersion': settings.DANDI_SCHEMA_VERSION}, ) @@ -589,7 +604,6 @@ def test_asset_create_embargo(api_client, user, draft_version, embargoed_asset_b path = 'test/create/asset.txt' metadata = { 'encodingFormat': 'application/x-nwb', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -598,7 +612,7 @@ def test_asset_create_embargo(api_client, user, draft_version, embargoed_asset_b resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'blob_id': embargoed_asset_blob.blob_id}, + {'path': path, 'metadata': metadata, 'blob_id': embargoed_asset_blob.blob_id}, format='json', ).json() new_asset = Asset.objects.get(asset_id=resp['asset_id']) @@ -633,7 +647,6 @@ def test_asset_create_zarr(api_client, user, draft_version, zarr_archive): path = 'test/create/asset.txt' metadata = { 'encodingFormat': 'application/x-zarr', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -642,7 +655,7 @@ def test_asset_create_zarr(api_client, user, draft_version, zarr_archive): resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, + {'path': path, 'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, format='json', ).json() new_asset = Asset.objects.get(asset_id=resp['asset_id']) @@ -684,7 +697,6 @@ def test_asset_create_zarr_validated( metadata = { 'encodingFormat': 'application/x-zarr', 'schemaKey': 'Asset', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -694,18 +706,18 @@ def test_asset_create_zarr_validated( resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, + {'path': path, 'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, format='json', ).json() asset1 = Asset.objects.get(asset_id=resp['asset_id']) # Create second asset that points to the same zarr metadata['1'] = 3 - metadata['path'] = 'test/create/asset2.txt' + path = 'test/create/asset2.txt' resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, + {'path': path, 'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, format='json', ).json() asset2 = Asset.objects.get(asset_id=resp['asset_id']) @@ -735,7 +747,6 @@ def test_asset_create_zarr_wrong_dandiset( path = 'test/create/asset.txt' metadata = { 'encodingFormat': 'application/x-zarr', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -744,7 +755,7 @@ def test_asset_create_zarr_wrong_dandiset( resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, + {'path': path, 'metadata': metadata, 'zarr_id': zarr_archive.zarr_id}, format='json', ) assert resp.status_code == 400 @@ -759,7 +770,6 @@ def test_asset_create_no_blob_or_zarr(api_client, user, draft_version): path = 'test/create/asset.txt' metadata = { 'encodingFormat': 'application/x-zarr', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -768,7 +778,7 @@ def test_asset_create_no_blob_or_zarr(api_client, user, draft_version): resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata}, + {'path': path, 'metadata': metadata}, format='json', ) assert resp.status_code == 400 @@ -783,7 +793,6 @@ def test_asset_create_blob_and_zarr(api_client, user, draft_version, asset_blob, path = 'test/create/asset.txt' metadata = { 'encodingFormat': 'application/x-zarr', - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -792,7 +801,12 @@ def test_asset_create_blob_and_zarr(api_client, user, draft_version, asset_blob, resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'blob_id': asset_blob.blob_id, 'zarr_id': zarr_archive.zarr_id}, + { + 'path': path, + 'metadata': metadata, + 'blob_id': asset_blob.blob_id, + 'zarr_id': zarr_archive.zarr_id, + }, format='json', ) assert resp.status_code == 400 @@ -805,13 +819,13 @@ def test_asset_create_no_valid_blob(api_client, user, draft_version): api_client.force_authenticate(user=user) path = 'test/create/asset.txt' - metadata = {'path': path, 'foo': ['bar', 'baz'], '1': 2} + metadata = {'foo': ['bar', 'baz'], '1': 2} uuid = uuid4() resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'blob_id': uuid}, + {'path': path, 'metadata': metadata, 'blob_id': uuid}, format='json', ) assert resp.status_code == 404 @@ -831,7 +845,7 @@ def test_asset_create_no_path(api_client, user, draft_version, asset_blob): format='json', ) assert resp.status_code == 400 - assert resp.data == {'metadata': ['No path specified in metadata.']}, resp.data + assert resp.data == {'path': ['This field is required.']} @pytest.mark.django_db @@ -854,13 +868,16 @@ def test_asset_create_published_version(api_client, user, published_version, ass api_client.force_authenticate(user=user) published_version.assets.add(asset) + # Ensure no conflicting fields present + metadata = Asset.strip_metadata(asset.metadata) + # Set path so API request succeeds - asset.metadata['path'] = asset.path resp = api_client.post( f'/api/dandisets/{published_version.dandiset.identifier}' f'/versions/{published_version.version}/assets/', { - 'metadata': asset.metadata, + 'path': asset.path, + 'metadata': metadata, 'blob_id': asset.blob.blob_id, }, format='json', @@ -876,7 +893,6 @@ def test_asset_create_existing_path(api_client, user, draft_version, asset_blob, path = 'test/create/asset.txt' metadata = { - 'path': path, 'meta': 'data', 'foo': ['bar', 'baz'], '1': 2, @@ -889,7 +905,7 @@ def test_asset_create_existing_path(api_client, user, draft_version, asset_blob, resp = api_client.post( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', - {'metadata': metadata, 'blob_id': asset_blob.blob_id}, + {'path': path, 'metadata': metadata, 'blob_id': asset_blob.blob_id}, format='json', ) assert resp.status_code == 409 @@ -901,23 +917,23 @@ def test_asset_rest_rename(api_client, user, draft_version, asset_blob): api_client.force_authenticate(user=user) # Create asset - metadata = {'path': 'foo/bar', 'schemaVersion': settings.DANDI_SCHEMA_VERSION} + metadata = {'schemaVersion': settings.DANDI_SCHEMA_VERSION} asset = add_asset_to_version( - user=user, version=draft_version, asset_blob=asset_blob, metadata=metadata + user=user, version=draft_version, asset_blob=asset_blob, metadata=metadata, path='foo/bar' ) # Change path and make update request - metadata['path'] = 'foo/bar2' + path = 'foo/bar2' resp = api_client.put( f'/api/dandisets/{draft_version.dandiset.identifier}/versions/{draft_version.version}/' f'assets/{asset.asset_id}/', - {'metadata': metadata, 'blob_id': asset.blob.blob_id}, + {'path': path, 'metadata': metadata, 'blob_id': asset.blob.blob_id}, format='json', ) # Ensure new asset with new path was created assert resp.status_code == 200 - assert resp.json()['path'] == metadata['path'] + assert resp.json()['path'] == path assert resp.json()['asset_id'] != str(asset.asset_id) @@ -932,7 +948,6 @@ def test_asset_rest_update(api_client, user, draft_version, asset, asset_blob): new_path = 'test/asset/rest/update.txt' new_metadata = { 'encodingFormat': 'application/x-nwb', - 'path': new_path, 'foo': 'bar', 'num': 123, 'list': ['a', 'b', 'c'], @@ -941,7 +956,7 @@ def test_asset_rest_update(api_client, user, draft_version, asset, asset_blob): resp = api_client.put( f'/api/dandisets/{draft_version.dandiset.identifier}/' f'versions/{draft_version.version}/assets/{asset.asset_id}/', - {'metadata': new_metadata, 'blob_id': asset_blob.blob_id}, + {'path': new_path, 'metadata': new_metadata, 'blob_id': asset_blob.blob_id}, format='json', ).json() new_asset = Asset.objects.get(asset_id=resp['asset_id']) @@ -992,7 +1007,6 @@ def test_asset_rest_update_embargo(api_client, user, draft_version, asset, embar new_path = 'test/asset/rest/update.txt' new_metadata = { 'encodingFormat': 'application/x-nwb', - 'path': new_path, 'foo': 'bar', 'num': 123, 'list': ['a', 'b', 'c'], @@ -1001,7 +1015,7 @@ def test_asset_rest_update_embargo(api_client, user, draft_version, asset, embar resp = api_client.put( f'/api/dandisets/{draft_version.dandiset.identifier}/' f'versions/{draft_version.version}/assets/{asset.asset_id}/', - {'metadata': new_metadata, 'blob_id': embargoed_asset_blob.blob_id}, + {'path': new_path, 'metadata': new_metadata, 'blob_id': embargoed_asset_blob.blob_id}, format='json', ).json() new_asset = Asset.objects.get(asset_id=resp['asset_id']) @@ -1062,7 +1076,6 @@ def test_asset_rest_update_zarr( new_path = 'test/asset/rest/update.txt' new_metadata = { 'encodingFormat': 'application/x-zarr', - 'path': new_path, 'foo': 'bar', 'num': 123, 'list': ['a', 'b', 'c'], @@ -1070,7 +1083,7 @@ def test_asset_rest_update_zarr( resp = api_client.put( f'/api/dandisets/{draft_version.dandiset.identifier}/' f'versions/{draft_version.version}/assets/{asset.asset_id}/', - {'metadata': new_metadata, 'zarr_id': zarr_archive.zarr_id}, + {'path': new_path, 'metadata': new_metadata, 'zarr_id': zarr_archive.zarr_id}, format='json', ).json() new_asset = Asset.objects.get(asset_id=resp['asset_id']) @@ -1169,12 +1182,18 @@ def test_asset_rest_update_to_existing(api_client, user, draft_version, asset_fa assign_perm('owner', user, draft_version.dandiset) api_client.force_authenticate(user=user) + # Ensure no conflicting fields present + metadata = Asset.strip_metadata(existing_asset.metadata) + # Set path so API request succeeds - existing_asset.metadata['path'] = existing_asset.path resp = api_client.put( f'/api/dandisets/{draft_version.dandiset.identifier}/' f'versions/{draft_version.version}/assets/{old_asset.asset_id}/', - {'metadata': existing_asset.metadata, 'blob_id': existing_asset.blob.blob_id}, + { + 'path': existing_asset.path, + 'metadata': metadata, + 'blob_id': existing_asset.blob.blob_id, + }, format='json', ) assert resp.status_code == 409 @@ -1264,10 +1283,8 @@ def test_asset_rest_delete_zarr_modified( f'/api/dandisets/{draft_version.dandiset.identifier}' f'/versions/{draft_version.version}/assets/', { - 'metadata': { - 'path': 'sample.zarr', - 'schemaVersion': settings.DANDI_SCHEMA_VERSION, - }, + 'path': 'sample.zarr', + 'metadata': {'schemaVersion': settings.DANDI_SCHEMA_VERSION}, 'zarr_id': zarr_archive.zarr_id, }, format='json', diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 8b4ea28ac..1bf3863d8 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -182,6 +182,7 @@ def info(self, *args, **kwargs): class AssetRequestSerializer(serializers.Serializer): + path = serializers.CharField() metadata = serializers.JSONField() blob_id = serializers.UUIDField(required=False) zarr_id = serializers.UUIDField(required=False) @@ -213,12 +214,14 @@ def validate(self, data): raise serializers.ValidationError( {'blob_id': 'Exactly one of blob_id or zarr_id must be specified.'} ) - if 'path' not in data['metadata'] or not data['metadata']['path']: - raise serializers.ValidationError({'metadata': 'No path specified in metadata.'}) + if 'path' in data['metadata']: + raise serializers.ValidationError( + {'metadata': 'Path must only be specified at top level.'} + ) # Validate the asset path. If this fails, it will raise a django ValidationError, which # will be caught further up the stack and be converted to a DRF ValidationError - validate_asset_path(data['metadata']['path']) + validate_asset_path(data['path']) data['metadata'].setdefault('schemaVersion', settings.DANDI_SCHEMA_VERSION) return data @@ -312,6 +315,7 @@ def create(self, request, versions__dandiset__pk, versions__version): version=version, asset_blob=serializer.get_blob(), zarr_archive=serializer.get_zarr_archive(), + path=serializer.validated_data['path'], metadata=serializer.validated_data['metadata'], ) @@ -353,6 +357,7 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): version=version, new_asset_blob=serializer.get_blob(), new_zarr_archive=serializer.get_zarr_archive(), + new_path=serializer.validated_data['path'], new_metadata=serializer.validated_data['metadata'], ) diff --git a/dandiapi/zarr/tests/test_ingest_zarr_archive.py b/dandiapi/zarr/tests/test_ingest_zarr_archive.py index ffa1172cf..fe0e8a128 100644 --- a/dandiapi/zarr/tests/test_ingest_zarr_archive.py +++ b/dandiapi/zarr/tests/test_ingest_zarr_archive.py @@ -116,10 +116,8 @@ def test_ingest_zarr_archive_modified(user, draft_version, zarr_archive, zarr_fi user=user, version=draft_version, zarr_archive=zarr_archive, - metadata={ - 'path': 'sample.zarr', - 'schemaVersion': settings.DANDI_SCHEMA_VERSION, - }, + path='sample.zarr', + metadata={'schemaVersion': settings.DANDI_SCHEMA_VERSION}, ) assert asset.size == 100 ap = AssetPath.objects.filter(version=draft_version, asset=asset).first()