From b8d557e4f38cd5ab0a45233e7cdf852a01af9bf8 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 14:51:10 -0500 Subject: [PATCH 1/7] Fix Within an `except` clause, raise exceptions with `raise ... from err` or `raise ... from None` to distinguish them from errors in exception handling --- dandiapi/api/asset_paths.py | 2 +- .../commands/migrate_published_version_metadata.py | 2 +- dandiapi/api/views/auth.py | 4 ++-- dandiapi/api/views/dandiset.py | 4 ++-- dandiapi/api/views/upload.py | 6 +++--- dandiapi/zarr/views/__init__.py | 4 ++-- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/dandiapi/api/asset_paths.py b/dandiapi/api/asset_paths.py index a24323043..1d6ac6892 100644 --- a/dandiapi/api/asset_paths.py +++ b/dandiapi/api/asset_paths.py @@ -130,7 +130,7 @@ def insert_asset_paths(asset: Asset, version: Version): # If there are simultaneous requests to create the same asset, this check constraint can # fail, and should be handled directly, rather than be allowed to bubble up if 'unique-version-path' in str(e): - raise AssetAlreadyExistsError + raise AssetAlreadyExistsError from e # Re-raise original exception otherwise raise diff --git a/dandiapi/api/management/commands/migrate_published_version_metadata.py b/dandiapi/api/management/commands/migrate_published_version_metadata.py index c9e617043..010f8141e 100644 --- a/dandiapi/api/management/commands/migrate_published_version_metadata.py +++ b/dandiapi/api/management/commands/migrate_published_version_metadata.py @@ -26,7 +26,7 @@ def migrate_published_version_metadata(dandiset: str, published_version: str, to except Exception as e: click.echo(f'Failed to migrate {dandiset}/{published_version}') click.echo(e) - raise click.Abort + raise click.Abort from e if metadata == metanew: click.echo('No changes detected') diff --git a/dandiapi/api/views/auth.py b/dandiapi/api/views/auth.py index 149b40b31..b3a8f383a 100644 --- a/dandiapi/api/views/auth.py +++ b/dandiapi/api/views/auth.py @@ -147,8 +147,8 @@ def user_questionnaire_form_view(request: HttpRequest) -> HttpResponse: try: # questions to display in the form questions = json.loads(request.GET.get('QUESTIONS')) - except (JSONDecodeError, TypeError): - raise Http404 + except (JSONDecodeError, TypeError) as e: + raise Http404 from e return render( request, diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 8dd026598..3c691ab46 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -146,8 +146,8 @@ def get_object(self): lookup_url = self.kwargs[self.lookup_url_kwarg] try: lookup_value = int(lookup_url) - except ValueError: - raise Http404('Not a valid identifier.') + except ValueError as e: + raise Http404('Not a valid identifier.') from e self.kwargs[self.lookup_url_kwarg] = lookup_value dandiset = super().get_object() diff --git a/dandiapi/api/views/upload.py b/dandiapi/api/views/upload.py index 98884e5a3..c9fe71ac1 100644 --- a/dandiapi/api/views/upload.py +++ b/dandiapi/api/views/upload.py @@ -197,7 +197,7 @@ def upload_complete_view(request: Request, upload_id: str) -> HttpResponseBase: except Upload.DoesNotExist: upload = get_object_or_404(EmbargoedUpload, upload_id=upload_id) if not request.user.has_perm('owner', upload.dandiset): - raise Http404 + raise Http404 from None completion = TransferredParts( object_key=upload.blob.name, @@ -237,7 +237,7 @@ def upload_validate_view(request: Request, upload_id: str) -> HttpResponseBase: except Upload.DoesNotExist: upload = get_object_or_404(EmbargoedUpload, upload_id=upload_id) if not request.user.has_perm('owner', upload.dandiset): - raise Http404 + raise Http404 from None # Verify that the upload was successful if not upload.object_key_exists(): @@ -275,7 +275,7 @@ def upload_validate_view(request: Request, upload_id: str) -> HttpResponseBase: asset_blob = upload.to_asset_blob() asset_blob.save() else: - raise ValueError(f'Unknown Upload type {type(upload)}') + raise ValueError(f'Unknown Upload type {type(upload)}') from None # Clean up the upload upload.delete() diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index cac64a412..e1ad3b26f 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/dandiapi/zarr/views/__init__.py @@ -138,8 +138,8 @@ def create(self, request): zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset) try: zarr_archive.save() - except IntegrityError: - raise ValidationError('Zarr already exists') + except IntegrityError as e: + raise ValidationError('Zarr already exists') from e serializer = ZarrSerializer(instance=zarr_archive) return Response(serializer.data, status=status.HTTP_200_OK) From 5dfa6c4f3f0c584915ef2e84d3fa4735b394e540 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 16:26:00 -0500 Subject: [PATCH 2/7] Fix DTZ005 The use of `datetime.datetime.now()` without `tz` argument is not allowed --- dandiapi/api/models/version.py | 2 +- dandiapi/api/tests/factories.py | 4 +++- dandiapi/api/tests/test_dandiset.py | 10 +++++----- dandiapi/api/tests/test_version.py | 16 ++++++++-------- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/dandiapi/api/models/version.py b/dandiapi/api/models/version.py index f30b1267d..286c694f3 100644 --- a/dandiapi/api/models/version.py +++ b/dandiapi/api/models/version.py @@ -131,7 +131,7 @@ def next_published_version(cls, dandiset: Dandiset) -> str: @classmethod def citation(cls, metadata): - year = datetime.datetime.now().year + year = datetime.datetime.now(datetime.UTC).year name = metadata['name'].rstrip('.') url = f'https://doi.org/{metadata["doi"]}' if 'doi' in metadata else metadata['url'] version = metadata['version'] diff --git a/dandiapi/api/tests/factories.py b/dandiapi/api/tests/factories.py index 516e38569..d8451c38d 100644 --- a/dandiapi/api/tests/factories.py +++ b/dandiapi/api/tests/factories.py @@ -57,7 +57,9 @@ def extra_data(self): # Supply a fake created date at least 1 year before now created = ( faker.Faker() - .date_time_between(end_date=datetime.datetime.now() - datetime.timedelta(days=365)) + .date_time_between( + end_date=datetime.datetime.now(datetime.UTC) - datetime.timedelta(days=365) + ) .isoformat() ) diff --git a/dandiapi/api/tests/test_dandiset.py b/dandiapi/api/tests/test_dandiset.py index 91b00e7e7..bfc3faa19 100644 --- a/dandiapi/api/tests/test_dandiset.py +++ b/dandiapi/api/tests/test_dandiset.py @@ -1,4 +1,4 @@ -from datetime import datetime +import datetime from django.conf import settings from django.contrib.auth.models import AnonymousUser @@ -357,7 +357,7 @@ def test_dandiset_rest_create(api_client, user): assert dandiset.draft_version.status == Version.Status.PENDING # Verify that computed metadata was injected - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'{settings.DANDI_WEB_APP_URL}/dandiset/{dandiset.identifier}/draft' assert dandiset.draft_version.metadata == { **metadata, @@ -450,7 +450,7 @@ def test_dandiset_rest_create_with_identifier(api_client, admin_user): assert dandiset.draft_version.status == Version.Status.PENDING # Verify that computed metadata was injected - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'{settings.DANDI_WEB_APP_URL}/dandiset/{dandiset.identifier}/draft' assert dandiset.draft_version.metadata == { **metadata, @@ -557,7 +557,7 @@ def test_dandiset_rest_create_with_contributor(api_client, admin_user): assert dandiset.draft_version.status == Version.Status.PENDING # Verify that computed metadata was injected - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'{settings.DANDI_WEB_APP_URL}/dandiset/{dandiset.identifier}/draft' assert dandiset.draft_version.metadata == { **metadata, @@ -647,7 +647,7 @@ def test_dandiset_rest_create_embargoed(api_client, user): assert dandiset.draft_version.status == Version.Status.PENDING # Verify that computed metadata was injected - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'{settings.DANDI_WEB_APP_URL}/dandiset/{dandiset.identifier}/draft' assert dandiset.draft_version.metadata == { **metadata, diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index ec4277ddf..ea820b514 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -1,6 +1,6 @@ from __future__ import annotations -from datetime import datetime +import datetime from time import sleep from typing import TYPE_CHECKING @@ -138,7 +138,7 @@ def test_published_version_metadata_computed(published_version: Version): @pytest.mark.django_db() def test_version_metadata_citation_draft(draft_version): name = draft_version.metadata['name'].rstrip('.') - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'{settings.DANDI_WEB_APP_URL}/dandiset/{draft_version.dandiset.identifier}/{draft_version.version}' # noqa: E501 assert ( draft_version.metadata['citation'] @@ -149,7 +149,7 @@ def test_version_metadata_citation_draft(draft_version): @pytest.mark.django_db() def test_version_metadata_citation_published(published_version): name = published_version.metadata['name'].rstrip('.') - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'https://doi.org/{published_version.doi}' assert ( published_version.metadata['citation'] @@ -163,7 +163,7 @@ def test_version_metadata_citation_no_contributors(version): version.save() name = version.metadata['name'].rstrip('.') - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year assert version.metadata['citation'].startswith( f'{name} ({year}). (Version {version.version}) [Data set]. DANDI archive. ' ) @@ -178,7 +178,7 @@ def test_version_metadata_citation_contributor_not_in_citation(version): version.save() name = version.metadata['name'].rstrip('.') - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year assert version.metadata['citation'].startswith( f'{name} ({year}). (Version {version.version}) [Data set]. DANDI archive. ' ) @@ -190,7 +190,7 @@ def test_version_metadata_citation_contributor(version): version.save() name = version.metadata['name'].rstrip('.') - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year assert version.metadata['citation'].startswith( f'Doe, Jane ({year}) {name} (Version {version.version}) [Data set]. DANDI archive. ' ) @@ -205,7 +205,7 @@ def test_version_metadata_citation_multiple_contributors(version): version.save() name = version.metadata['name'].rstrip('.') - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year assert version.metadata['citation'].startswith( f'John Doe; Jane Doe ({year}) {name} (Version {version.version}) [Data set]. ' f'DANDI archive. ' @@ -507,7 +507,7 @@ def test_version_rest_update(api_client, user, draft_version): # This should be stripped out 'dateCreated': 'foobar', } - year = datetime.now().year + year = datetime.datetime.now(datetime.UTC).year url = f'{settings.DANDI_WEB_APP_URL}/dandiset/{draft_version.dandiset.identifier}/draft' saved_metadata = { **new_metadata, From ac690848f6f39ea3c52310026342c37fbee82373 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 16:28:15 -0500 Subject: [PATCH 3/7] Fix T201 `print` found --- dandiapi/api/asset_paths.py | 3 +-- dandiapi/api/management/commands/ingest_asset_paths.py | 4 ++-- .../api/management/commands/migrate_version_metadata.py | 8 ++++---- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/dandiapi/api/asset_paths.py b/dandiapi/api/asset_paths.py index 1d6ac6892..5ac209bd6 100644 --- a/dandiapi/api/asset_paths.py +++ b/dandiapi/api/asset_paths.py @@ -240,7 +240,7 @@ def update_asset_paths(old_asset: Asset, new_asset: Asset, version: Version): @transaction.atomic def add_version_asset_paths(version: Version): """Add every asset from a version.""" - print('\t Leaves...') + # Leaves for asset in tqdm(version.assets.all()): insert_asset_paths(asset, version) @@ -249,7 +249,6 @@ def add_version_asset_paths(version: Version): # https://stackoverflow.com/a/60221875 # Get all nodes which haven't been computed yet - print('\t Nodes...') nodes = AssetPath.objects.filter(version=version, aggregate_files=0) for node in tqdm(nodes): child_link_ids = node.child_links.distinct('child_id').values_list('child_id', flat=True) diff --git a/dandiapi/api/management/commands/ingest_asset_paths.py b/dandiapi/api/management/commands/ingest_asset_paths.py index 8c413d427..42c99fa0f 100644 --- a/dandiapi/api/management/commands/ingest_asset_paths.py +++ b/dandiapi/api/management/commands/ingest_asset_paths.py @@ -7,6 +7,6 @@ @click.command() def ingest_asset_paths(): for version in Version.objects.iterator(): - print(f'Version: {version}') - print(f'\t {version.assets.count()} assets') + click.echo(f'Version: {version}') + click.echo(f'\t {version.assets.count()} assets') add_version_asset_paths(version) diff --git a/dandiapi/api/management/commands/migrate_version_metadata.py b/dandiapi/api/management/commands/migrate_version_metadata.py index 3a9538416..45cc2e99d 100644 --- a/dandiapi/api/management/commands/migrate_version_metadata.py +++ b/dandiapi/api/management/commands/migrate_version_metadata.py @@ -7,17 +7,17 @@ @click.command() @click.argument('to_version') def migrate_version_metadata(to_version: str): - print(f'Migrating all version metadata to version {to_version}') + click.echo(f'Migrating all version metadata to version {to_version}') for version in Version.objects.filter(version='draft'): - print(f'Migrating {version.dandiset.identifier}/{version.version}') + click.echo(f'Migrating {version.dandiset.identifier}/{version.version}') metadata = version.metadata try: metanew = migrate(metadata, to_version=to_version, skip_validation=True) except Exception as e: - print(f'Failed to migrate {version.dandiset.identifier}/{version.version}') - print(e) + click.echo(f'Failed to migrate {version.dandiset.identifier}/{version.version}') + click.echo(e) continue if version.metadata != metanew: From 10594a3936ff745cdc81bf86873aa303506f12f3 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 17:11:38 -0500 Subject: [PATCH 4/7] Fix TRY400 Use `logging.exception` instead of `logging.error` --- dandiapi/api/doi.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dandiapi/api/doi.py b/dandiapi/api/doi.py index a3322487c..07b9800dc 100644 --- a/dandiapi/api/doi.py +++ b/dandiapi/api/doi.py @@ -46,10 +46,10 @@ def create_doi(version: Version) -> str: ), ).raise_for_status() except requests.exceptions.HTTPError as e: - logging.error('Failed to create DOI %s', doi) - logging.error(request_body) + logging.exception('Failed to create DOI %s', doi) + logging.exception(request_body) if e.response: - logging.error(e.response.text) + logging.exception(e.response.text) raise return doi @@ -68,11 +68,11 @@ def delete_doi(doi: str) -> None: logging.warning('Tried to get data for nonexistent DOI %s', doi) return else: - logging.error('Failed to fetch data for DOI %s', doi) + logging.exception('Failed to fetch data for DOI %s', doi) raise if r.json()['data']['attributes']['state'] == 'draft': try: s.delete(doi_url).raise_for_status() except requests.exceptions.HTTPError: - logging.error('Failed to delete DOI %s', doi) + logging.exception('Failed to delete DOI %s', doi) raise From da80f9a6678166cbd075c27333db4ab9899dd103 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 17:31:04 -0500 Subject: [PATCH 5/7] Fix DJ012 Order of model does not follow the Django Style Guide --- dandiapi/analytics/models.py | 16 ++++++++-------- dandiapi/api/models/upload.py | 8 ++++---- dandiapi/search/models.py | 4 ++-- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dandiapi/analytics/models.py b/dandiapi/analytics/models.py index 5436bd0da..757acfce6 100644 --- a/dandiapi/analytics/models.py +++ b/dandiapi/analytics/models.py @@ -3,14 +3,6 @@ class ProcessedS3Log(models.Model): - class Meta: - constraints = [ - models.UniqueConstraint( - fields=['name', 'embargoed'], - name='%(app_label)s_%(class)s_unique_name_embargoed', - ) - ] - name = models.CharField( max_length=36, validators=[ @@ -20,3 +12,11 @@ class Meta: ) # This is necessary to determine which bucket the logfile corresponds to embargoed = models.BooleanField() + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=['name', 'embargoed'], + name='%(app_label)s_%(class)s_unique_name_embargoed', + ) + ] diff --git a/dandiapi/api/models/upload.py b/dandiapi/api/models/upload.py index 5479e593e..d6acd8803 100644 --- a/dandiapi/api/models/upload.py +++ b/dandiapi/api/models/upload.py @@ -22,10 +22,6 @@ class BaseUpload(models.Model): ETAG_REGEX = r'[0-9a-f]{32}(-[1-9][0-9]*)?' - class Meta: - indexes = [models.Index(fields=['etag'])] - abstract = True - created = CreationDateTimeField() # This is the key used to generate the object key, and the primary identifier for the upload. @@ -41,6 +37,10 @@ class Meta: multipart_upload_id = models.CharField(max_length=128, unique=True, db_index=True) size = models.PositiveBigIntegerField() + class Meta: + indexes = [models.Index(fields=['etag'])] + abstract = True + @staticmethod @abstractmethod def object_key(upload_id, *, dandiset: Dandiset): # noqa: N805 diff --git a/dandiapi/search/models.py b/dandiapi/search/models.py index 6b1a03a18..ad308f2cf 100644 --- a/dandiapi/search/models.py +++ b/dandiapi/search/models.py @@ -22,13 +22,13 @@ def visible_to(self, user: User) -> models.QuerySet[AssetSearch]: class AssetSearch(models.Model): - objects = AssetSearchManager() - dandiset_id = models.PositiveBigIntegerField() asset_id = models.PositiveBigIntegerField(primary_key=True) asset_metadata = models.JSONField() asset_size = models.PositiveBigIntegerField() + objects = AssetSearchManager() + class Meta: managed = False db_table = 'asset_search' From 07dc7607def96424b94f7865e4fb91f0d1526fbc Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 19:46:15 -0500 Subject: [PATCH 6/7] Fix PLW2901 `for` loop variable overwritten by assignment target --- dandiapi/api/views/dandiset.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/dandiapi/api/views/dandiset.py b/dandiapi/api/views/dandiset.py index 3c691ab46..2576ca4ac 100644 --- a/dandiapi/api/views/dandiset.py +++ b/dandiapi/api/views/dandiset.py @@ -402,16 +402,19 @@ def users(self, request, dandiset__pk): send_ownership_change_emails(dandiset, removed_owners, added_owners) owners = [] - for owner in dandiset.owners: + for owner_user in dandiset.owners: try: - owner = SocialAccount.objects.get(user=owner) - owner_dict = {'username': owner.extra_data['login']} - if 'name' in owner.extra_data: - owner_dict['name'] = owner.extra_data['name'] + owner_account = SocialAccount.objects.get(user=owner_user) + owner_dict = {'username': owner_account.extra_data['login']} + if 'name' in owner_account.extra_data: + owner_dict['name'] = owner_account.extra_data['name'] owners.append(owner_dict) except SocialAccount.DoesNotExist: # Just in case some users aren't using social accounts, have a fallback owners.append( - {'username': owner.username, 'name': f'{owner.first_name} {owner.last_name}'} + { + 'username': owner_user.username, + 'name': f'{owner_user.first_name} {owner_user.last_name}', + } ) return Response(owners, status=status.HTTP_200_OK) From 66861ca1ccdd9d26c398d7b6ebd8f54db3dbf9dd Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 19:59:55 -0500 Subject: [PATCH 7/7] Fix A002 Argument is shadowing a Python builtin --- dandiapi/api/storage.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dandiapi/api/storage.py b/dandiapi/api/storage.py index 5644d66fb..a4d1e5c27 100644 --- a/dandiapi/api/storage.py +++ b/dandiapi/api/storage.py @@ -26,8 +26,8 @@ class ChecksumCalculatorFile: def __init__(self): self.h = hashlib.sha256() - def write(self, bytes): - self.h.update(bytes) + def write(self, data: bytes) -> None: + self.h.update(data) @property def checksum(self):