Skip to content

Commit

Permalink
♻️ REFACTOR: Fully abstract QueryBuilder (#5093)
Browse files Browse the repository at this point in the history
There are four primary modules involved in the QueryBuilder abstraction: 

- `aiida/orm/implementation/django/querybuilder.py::DjagoQueryBuilder`
- `aiida/orm/implementation/sqlalchemy/querybuilder.py::SqlaQueryBuilder`
- `aiida/orm/implementation/querybuilder.py::BackendQueryBuilder`
- `aiida/orm/querybuilder.py::QueryBuilder`

Previously, all of these modules imported objects from SQLAlchemy,
i.e. there was actually no abstraction, since everything was tightly coupled to SQLAlchemy.
As well as this lack of abstraction, the spread of logic and state across these modules
made it very difficult to decipher the workings of the code.

This commit fully abstracts the QueryBuilder;
moving all SQLAlchemy code to `SqlaQueryBuilder`,
making the `BackendQueryBuilder` a proper abstract class,
and reducing `QueryBuilder` backend interaction to the 6 abstract methods:
`count`, `iterall`, `iterdict`, `as_sql`, `analyze_query`.

These methods all pass the (JSONable) query dict to the backend,
which it uses to build the query.
As was previously the case on the frontend,
`SqlaQueryBuilder` hashes this dict, to ensure the query is only rebuilt on changes.

Some other changes of note:

- `aiida/orm/implementation/sqlalchemy/querybuilder.py` is split into a few modules, to improve modularity
- The `QueryBuilder.get_query` and `QueryBuilder.inject_query` method have been removed,
  since naturally these break abstraction (relying on an `sqlalchemy.Query` object)
- The `QueryBuilder.distinct` method now savez its state to the query dict,
  rather than directly calling the `sqlachemy.Query.distinct`.
- Correctly reset `limit` after calling `QueryBuilder.one()`
- Improve validation of `joining_keyword`;
  before it would only validate against all possible keywords,
  now it validates against only the keywords for the entity type.
- Improve the defaults for joinin_keywords, 
  i.e. rather than always setting `with_incoming` for all entities, set `with_node` for non-node entities.
- `sqlalchemy_utils` is now only used in `tests/backends/aiida_sqlalchemy/test_utils.py`, so I have moved it to the `tests` extras, rather than `install_requires`
- lots of typing!
  • Loading branch information
chrisjsewell authored Aug 25, 2021
1 parent eae3c50 commit 4174e5d
Show file tree
Hide file tree
Showing 18 changed files with 2,287 additions and 2,278 deletions.
2 changes: 2 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ repos:
aiida/manage/manager.py|
aiida/manage/database/delete/nodes.py|
aiida/orm/querybuilder.py|
aiida/orm/implementation/querybuilder.py|
aiida/orm/implementation/sqlalchemy/querybuilder/.*py|
aiida/orm/nodes/data/jsonable.py|
aiida/orm/nodes/node.py|
aiida/orm/nodes/process/.*py|
Expand Down
4 changes: 4 additions & 0 deletions aiida/orm/groups.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
###########################################################################
"""AiiDA Group entites"""
from abc import ABCMeta
from typing import ClassVar, Optional
import warnings

from aiida.common import exceptions
Expand Down Expand Up @@ -67,6 +68,9 @@ def __new__(cls, name, bases, namespace, **kwargs):
class Group(entities.Entity, entities.EntityExtrasMixin, metaclass=GroupMeta):
"""An AiiDA ORM implementation of group of nodes."""

# added by metaclass
_type_string = ClassVar[Optional[str]]

class Collection(entities.Collection):
"""Collection of Groups"""

Expand Down
2 changes: 1 addition & 1 deletion aiida/orm/implementation/django/querybuilder.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class DjangoQueryBuilder(SqlaQueryBuilder):
We use aldjemy to generate SQLAlchemy models by introspecting the Django models.
"""

def extra_init(self):
def set_field_mappings(self):
pass

@property
Expand Down
Loading

0 comments on commit 4174e5d

Please sign in to comment.