Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent duplicate serialization of dandiset in detail view #2125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjnesbitt
Copy link
Member

On any endpoint that used DandisetDetailSerializer for its response, the dandiset information was being included twice. Once at the top level, and once nested under each nested version serializer (draft_version and most_recent_published_version).

@jjnesbitt jjnesbitt force-pushed the nested-version-serializer branch from 3981678 to 50f6c9a Compare January 2, 2025 18:37
@jjnesbitt jjnesbitt requested a review from mvandenburgh January 3, 2025 16:46
Copy link
Member

@waxlamp waxlamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but technically this would be a breaking change (someone may be relying on the Dandiset info appearing in one of the version records). Not sure what to do about that (maybe nothing).

@@ -100,6 +100,12 @@ class Meta:
dandiset = DandisetSerializer()
# name = serializers.SlugRelatedField(read_only=True, slug_field='name')

def __init__(self, *args, **kwargs):
if kwargs.get('context', {}).get('child', False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to reflect into kwargs rather than just include context=None as a keyword arg?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You raise a good point. I've reworked the implementation to pluck this kwarg out, no longer passing it to the underlying base serializer class.

@jjnesbitt jjnesbitt force-pushed the nested-version-serializer branch from 50f6c9a to 1b8c3cf Compare January 6, 2025 17:24
@jjnesbitt
Copy link
Member Author

Looks good overall, but technically this would be a breaking change (someone may be relying on the Dandiset info appearing in one of the version records). Not sure what to do about that (maybe nothing).

You're correct in that it's technically a breaking change, however there are no CLI integration failures, so it would appear the CLI makes no use of this nested duplicated field. @jwodder Are you able to confirm this?

@jwodder
Copy link
Member

jwodder commented Jan 6, 2025

@jjnesbitt We're talking about removing the "dandiset" field in each of the *_version fields below, right? The CLI won't care.

{
  "identifier": "000027",
  "created": "2020-07-08T21:54:42.543000Z",
  "modified": "2022-01-17T16:22:43.888215Z",
  "contact_person": "Halchenko, Yaroslav O.",
  "embargo_status": "OPEN",
  "most_recent_published_version": {
    "version": "0.210831.2033",
    "name": "Test dataset for testing dandi-cli.",
    "asset_count": 1,
    "active_uploads": 0,
    "size": 18792,
    "status": "Valid",
    "created": "2021-08-31T20:34:01.209046Z",
    "modified": "2021-08-31T20:34:01.283878Z",
    "dandiset": {
      "identifier": "000027",
      "created": "2020-07-08T21:54:42.543000Z",
      "modified": "2022-01-17T16:22:43.888215Z",
      "contact_person": "Halchenko, Yaroslav O.",
      "embargo_status": "OPEN"
    }
  },
  "draft_version": {
    "version": "draft",
    "name": "Test dataset for testing dandi-cli.",
    "asset_count": 1,
    "active_uploads": 0,
    "size": 18792,
    "status": "Valid",
    "created": "2020-07-08T21:54:42.543000Z",
    "modified": "2023-06-20T00:56:23.249993Z",
    "dandiset": {
      "identifier": "000027",
      "created": "2020-07-08T21:54:42.543000Z",
      "modified": "2022-01-17T16:22:43.888215Z",
      "contact_person": "Halchenko, Yaroslav O.",
      "embargo_status": "OPEN"
    }
  }
}

@jjnesbitt
Copy link
Member Author

@jjnesbitt We're talking about removing the "dandiset" field in each of the *_version fields below, right? The CLI won't care.

Yep that's correct. Thanks for the confirmation 👍.

@jjnesbitt jjnesbitt requested a review from waxlamp January 6, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants