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

Migrations for legacy and now illegal default link label _return #3561

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 21, 2019

Fixes #3527

The second commit adds the migration for export archives, however, it does not include the respective tests. @CasperWA would be willing to add those?

The original default link label that was used when no explicit one was
specified by the user was set to `_return`. In a later change, link
labels starting or ending with an underscore were banned because it
would interfere with use of double underscores to indicate namespaces.
As a result the default link was renamed to `result` however a data
migration was never put in place.
Only the tests are missing.
@CasperWA
Copy link
Contributor

For future reference, I refer to: https://github.com/aiidateam/aiida-core/wiki/Checklist-when-updating-the-export-version

And I can add the files, sure 👍

@CasperWA CasperWA force-pushed the fix_3527_migrate_return_default_label branch from 4742cb2 to ca96e83 Compare November 21, 2019 15:51
Copy link
Contributor

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

I have updated the aiida-export-migration-tests to v0.8.0 and updated the dependency here as well.
All tests should have been added, along with updating test archives to v0.8.

@ltalirz
Copy link
Member

ltalirz commented Nov 21, 2019

Don't wait for me - fine to go ahead

@CasperWA CasperWA merged commit 952282e into aiidateam:develop Nov 21, 2019
@sphuber
Copy link
Contributor Author

sphuber commented Nov 21, 2019

Would be good if @giovannipizzi could still have a look to see if the migrations are ok.

@sphuber sphuber deleted the fix_3527_migrate_return_default_label branch November 21, 2019 21:50
@giovannipizzi
Copy link
Member

I gave - I'm not fully comfortable with changing all link labels being _return (Maybe I would have limited to calcfunction and work function CREATE/RETURN links) - on the other hand there shouldn't be other people using this name, and now it's illegal so maybe ok?

This on the other hand raises a different question - should we migrate link labels of people that used an invalid one before validation is in place? Or it's ok to keep as is, and they just cannot create new ones? (I guess these are typically created by plugins, so plugin developers will anyway have to upgrade their plugins if using invalid names - the main question is if anything 'crashes' or raises if an invalid link is in the DB

@sphuber
Copy link
Contributor Author

sphuber commented Nov 21, 2019

there shouldn't be other people using this name, and now it's illegal so maybe ok?

This was my thinking exactly. I don't think there is actual exceptions that can occur when just loading the node or links, but it will certainly prevent from importing archives that contain them, since then the link validation will stop the link from being created.

This on the other hand raises a different question - should we migrate link labels of people that used an invalid one before validation is in place? Or it's ok to keep as is, and they just cannot create new ones?

I don't think we can start to add data migrations for things done by plugins. We could remove leading and trailing underscores from link labels as that is what is now illegal, but not sure that we should. So far we have never considered plugin controlled data in data migrations and not sure if we should start now.

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.

verdi import chokes on _return link label
4 participants