Refactor the code dealing with database into BackendManager
#3582
Conversation
|
This is the refactoring work that I originally implemented in combination with the reset of the database schema generation. However, having been convinced to postpone this, I still think the refactoring of the backend utilities should go in now. All the groundwork for the schema reset for |
|
I'll leave this review to @giovannipizzi if that's ok |
sphuber
force-pushed
the
fix_3530_refactor_backend_manager
branch
3 times, most recently
from
9b28013
to
1f07d8d
Compare
The code that acts directly with the database, outside of the ORM, for example to check the database schema generation and version, or to get data from the `DbSettings` table has been refactored. The ORM requires the database environment to be loaded, but to do that the database schema needs to be verified which also requires accessing the database. This functionality was implemented mostly in free-standing functions and separately for both backends. This is reorganized into a `BackendManager` abstract class and implemented for Django and SqlAlchemy. This centralizes the functionality required to verify database schema and perform the migrations thereof.
sphuber
force-pushed
the
fix_3530_refactor_backend_manager
branch
from
1f07d8d
to
952a5e9
Compare
There was a problem hiding this comment.
Thanks! I think the refactoring is good - As there are many changes I hope I didn't miss anything bad.
I added a comment, but it's more a question and in case can be addressed in a different issue.
Maybe before merging it would be good to have a small wiki page explaining the concept of schema revisions and how we intend to use it in the future (as a quick note for now, we might convert to an AEP when actually implementing it), and then link it from this PR
| closure_table_name, | ||
| closure_table_parent_field, | ||
| closure_table_child_field): | ||
| def get_pg_tc( |
There was a problem hiding this comment.
Why do we still have transitive-closure code in our codebase?
There was a problem hiding this comment.
Because this is part of the original database schema and so is installed in the initial migration and then subsequently dropped in one of the following migrations. One of the reasons I want to reset the schema at some point :) so we can get rid of code like this
In aiidateam#3582, SQLA migrations were broken, by starting a top-level transaction, instead of letting alembic open per-migration transaction.
Fixes #3530 and fixes #3584
The code that acts directly with the database, outside of the ORM, for
example to check the database schema generation and version, or to get
data from the
DbSettingstable has been refactored. The ORM requiresthe database environment to be loaded, but to do that the database
schema needs to be verified which also requires accessing the database.
This functionality was implemented mostly in free-standing functions and
separately for both backends. This is reorganized into a
BackendManagerabstract class and implemented for Django and SqlAlchemy. This
centralizes the functionality required to verify database schema and
perform the migrations thereof.