Skip to content

Fix migrate() to update schemaVersion for all migrations, not just pre-0.6.0#354

Merged
yarikoptic merged 4 commits intomasterfrom
copilot/fix-migrate-assignment-bug
Nov 25, 2025
Merged

Fix migrate() to update schemaVersion for all migrations, not just pre-0.6.0#354
yarikoptic merged 4 commits intomasterfrom
copilot/fix-migrate-assignment-bug

Conversation

Copy link
Contributor

Copilot AI commented Nov 24, 2025

The migrate() function only updated schemaVersion when migrating from versions below 0.6.0, leaving it unchanged for migrations between 0.6.0+ versions (e.g., 0.6.4 → 0.7.0).

Changes

  • dandischema/metadata.py: Moved schemaVersion assignment outside the pre-0.6.0 conditional block so it executes for all migrations
  • dandischema/tests/test_metadata.py: Added regression test covering 0.6.0+ → current version migrations

Example

# Before: schemaVersion incorrectly remained 0.6.0
obj = {"schemaKey": "Dandiset", "schemaVersion": "0.6.0", "identifier": "DANDI:000000"}
result = migrate(obj, to_version="0.7.0")
# result["schemaVersion"] == "0.6.0"  ❌

# After: schemaVersion correctly updated to 0.7.0
# result["schemaVersion"] == "0.7.0"  ✅
Original prompt

This section details on the original issue you should resolve

<issue_title>bug: migrate would assign target version only for records with schema starting versions under 0.6.0</issue_title>
<issue_description>I didn't validate that issue exists, so first the issue should be demonstrated in our existing tests -- may be I am not reviewing code correctly. But looking at https://github.com/dandi/dandi-schema/blob/master/dandischema/metadata.py#L461 we seems to assign new version in

obj_migrated["schemaVersion"] = to_version

only if we change the record, although should always as a result of migration. So plan should

  • add/change test so it fails now
  • move that assigment out of if block and see that it fixes the issue
    </issue_description>

<agent_instructions>add patch and release labels</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.89%. Comparing base (5cf181a) to head (883d6fb).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #354   +/-   ##
=======================================
  Coverage   97.89%   97.89%           
=======================================
  Files          18       18           
  Lines        2370     2377    +7     
=======================================
+ Hits         2320     2327    +7     
  Misses         50       50           
Flag Coverage Δ
unittests 97.89% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI and others added 2 commits November 24, 2025 20:39
The schemaVersion field is now updated for all migrations, not just
migrations from versions below 0.6.0. Added test to verify the fix.

Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
@yarikoptic
Copy link
Member

@copilot remember running pre-commit when you commit, or otherwise reacting to lints of that CI run

Copilot AI changed the title [WIP] Fix migrate version assignment issue for schema records Fix migrate() to update schemaVersion for all migrations, not just pre-0.6.0 Nov 24, 2025
Copilot AI requested a review from yarikoptic November 24, 2025 20:44
@yarikoptic
Copy link
Member

@copilot -- address pre-commit fixes:

trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing dandischema/tests/test_metadata.py
Fixing dandischema/metadata.py

and remember to generally prune trailing whitespaces unless they are intentionally added!

Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 24, 2025

@copilot remember running pre-commit when you commit, or otherwise reacting to lints of that CI run

Fixed trailing whitespace issues flagged by pre-commit in commit 883d6fb. All pre-commit checks pass now.

@yarikoptic
Copy link
Member

isn't that what we want to do -- always add target schemaVersion, even if no changes were needed? ;)

@yarikoptic yarikoptic added the patch Increment the patch version when merged label Nov 24, 2025
@yarikoptic yarikoptic added the release Create a release when this pr is merged label Nov 25, 2025
@yarikoptic yarikoptic merged commit 0af35b5 into master Nov 25, 2025
111 checks passed
@yarikoptic yarikoptic deleted the copilot/fix-migrate-assignment-bug branch November 25, 2025 19:39
@yarikoptic yarikoptic removed the release Create a release when this pr is merged label Nov 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: migrate would assign target version only for records with schema starting versions under 0.6.0

3 participants