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

StorageBackend: Remove recreate_user from _clear #5772

Merged
merged 1 commit into from Nov 22, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 16, 2022

The _clear method on the StorageBackend deletes all data in the storage. This is used by the unit tests when a test requires a clean storage to test against. However, the ORM assumes that a User instance, designated by the profile as the default user, is always present. As soon as an ORM entity is stored that requires a user but that is not explicitly defined, the default user is retrieved.

When a new profile is configured, the setup command ensures that the default user is properly created. However, when the storage is cleared, the user is also deleted and so it needs to be recreated. This job had been assigned to the StorageBackend but really this should not be its responsibility. It is specific to the use case of the test cases that reset the storage and need to reconstruct the user, so it should take care of that.

The recreate_user argument is removed from StorageBackend._clear and the logic to recreate a user after storage reset is moved to the ProfileManager. The clear_profile method, which calls the storage clear, recreates the user at the end. The TemporaryProfileManager, which subclasses the ProfileManager does the same when a temporary test profile is created at the start of a test session.

@sphuber sphuber force-pushed the fix/remove-recreate-user-from-clear branch 3 times, most recently from 1999ee0 to 453a0c0 Compare November 18, 2022 17:35
@sphuber sphuber requested a review from ltalirz November 18, 2022 22:26
@ltalirz
Copy link
Member

ltalirz commented Nov 19, 2022

Thanks @sphuber , makes sense.

This job had been assigned to the StorageBackend but really this should not be its responsibility. It is specific to the use case of the test cases that reset the storage and need to reconstruct the user, so it should take care of that.

Are there any other users of StorageBackend._clear besides the tests?

@sphuber
Copy link
Contributor Author

sphuber commented Nov 19, 2022

Thanks @sphuber , makes sense.

This job had been assigned to the StorageBackend but really this should not be its responsibility. It is specific to the use case of the test cases that reset the storage and need to reconstruct the user, so it should take care of that.

Are there any other users of StorageBackend._clear besides the tests?

Currently not, but I imagine at some point this would become the generic entry point to clear a database when a profile is deleted. The profile delete functionality is currently still hardcoded to the psql-dos backend in aiida-core, but this should become generic at some point. But I think rather than _clear we would add a method like erase/destroy on the StorageBackend interface that all plugins should implemented. This could optionally call _clear internally of course if it first needs to drop the data manually before deleting tables/repo/whathaveyou.

@sphuber sphuber force-pushed the fix/remove-recreate-user-from-clear branch 2 times, most recently from 994eabc to 586f2cd Compare November 19, 2022 14:03
The `_clear` method on the `StorageBackend` deletes all data in the
storage. This is used by the unit tests when a test requires a clean
storage to test against. However, the ORM assumes that a `User` instance,
designated by the profile as the default user, is always present. As
soon as an ORM entity is stored that requires a user but that is not
explicitly defined, the default user is retrieved.

When a new profile is configured, the setup command ensures that the
default user is properly created. However, when the storage is cleared,
the user is also deleted and so it needs to be recreated. This job had
been assigned to the `StorageBackend` but really this should not be its
responsibility. It is specific to the use case of the test cases that
reset the storage and need to reconstruct the user, so it should take
care of that.

The `recreate_user` argument is removed from `StorageBackend._clear` and
the logic to recreate a user after storage reset is moved to the
`ProfileManager`. The `clear_profile` method, which calls the storage
clear, recreates the user at the end. The `TemporaryProfileManager`,
which subclasses the `ProfileManager` does the same when a temporary
test profile is created at the start of a test session.
@sphuber sphuber force-pushed the fix/remove-recreate-user-from-clear branch from 586f2cd to 20a312e Compare November 21, 2022 13:17
@sphuber
Copy link
Contributor Author

sphuber commented Nov 21, 2022

@ltalirz is there anything you still wanted to check here?

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Currently not

Ok, just wanted to be sure that we're not changing behavior that some other place relies on. In that case, good to go!

@sphuber sphuber merged commit c2e7b9d into aiidateam:main Nov 22, 2022
@sphuber sphuber deleted the fix/remove-recreate-user-from-clear branch November 22, 2022 11:33
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.

None yet

2 participants