Code organisation in Django JSONB

[szoupanos]

We should check if the following tests are valid under DjangoJSONB

• aiida.backends.djsite.db.subtests.test_query.TestQueryBuilderDjango
• aiida.backends.djsite.db.subtests.test_nodes.TestNodeBasicDjango

Moreover, there is some code duplication in the following files related to EAV functionality, needed in migrations and migration tests. it is good to see how it can be avoided.

• aiida/backends/djsite/db/migrations/init.py
• aiida/backends/djsite/db/subtests/migrations/test_migrations_0034_attributes_extras_settings_json.py

Furthermore, the following QueryBuilder implementation files

• aiida/orm/implementation/sqlalchemy/querybuilder.py
• aiida/orm/implementation/django/querybuilder.py

have some common parts that are identical (e.g. get_filter_expr_from_attributes). We should see how we can merge them in a reasonable way. E.g. maybe the common functionality can now go to aiida/orm/implementation/querybuilder.py

With the work on Django JSONB, the import methods of AiiDA (for each backend) are very similar. The only difference is the way we create objects and we handle transactions (I hope). We should see if these two methods can now be unified in one.

[sphuber]

The two sub tests have been removed as they were no longer applicable. This functionality is now tested in backend independent way in aiida.backends.tests.orm.implementation.

Regarding the duplication of EAV functionality, I would leave it as it is. This code is there as a record of what it was at the time of the migration and as such should most likely remain static. In any case, hopefully in the near future we will reset the database schemas and we can get rid of all migration code including these files.

The QueryBuilder implementation duplication is still something worthwhile investigating, but I would say this is no longer a Django JSONB migration specific issue. If you agree, we close this issue and open one specifically for this code reorganization goal.

[sphuber]

Most of the points have been addressed. The refactoring of the query builder implementation continues in #3082

[szoupanos]

Hello Seb. My recollection is what you wrote.
So if this is your opinion too, let's continue like this.

My main doubt was the tests that you removed - but I understand from your comment that you had a close look at them and they are no longer needed.

For the code duplication at the migration, I agree and I also agree for QB.