Cascade DELETEs in the database (#363)

* add errors!

* made these changes when building the demo; this removes dangling templates/etc when deleting libraries

* Fix styling issues

* Add tests for cascading delete behavior on models and libraries

* formatting

* adding comments to tests, add another test case

* formatting

* delete dependant templates and libraries

* patch test by using overwrite=False to prevent library deletion cascade

* Implement lazy resolution

* begin refactor with new dependency design

* add error_on_missing_dependency

* Fix template library resolution for cases where library is being created from a ttl file

* fix dict to Dict

* default resolution is to current library

---------

Co-authored-by: TShapinsky <[email protected]>

Author: gtfierro

buildingmotif/database/errors.py

@@ -47,3 +47,11 @@ def __str__(self):
         if self.template_name:
             return f"Name: {self.template_name}"
         return f"ID: {self.template_id}"
+
+
+class TemplateDependencyNotFound(Exception):
+    def __init__(self, idnum: int):
+        self.template_dependency_id = idnum
+
+    def __str__(self):
+        return f"ID: {self.template_dependency_id}"

buildingmotif/database/table_connection.py

@@ -1,7 +1,7 @@
 import logging
 import uuid
 from functools import lru_cache
-from typing import Dict, List, Tuple
+from typing import Dict, List, Optional, Tuple
 
 from sqlalchemy.engine import Engine
 from sqlalchemy.exc import NoResultFound
@@ -17,7 +17,7 @@
     DBModel,
     DBShapeCollection,
     DBTemplate,
-    DepsAssociation,
+    DBTemplateDependency,
 )
 
 
@@ -287,7 +287,7 @@ def create_db_template(self, name: str, library_id: int) -> DBTemplate:
         template = DBTemplate(
             name=name,
             body_id=str(uuid.uuid4()),
-            optional_args=[],
+            optional_args=[],  # type: ignore
             library=library,
         )
 
@@ -347,7 +347,7 @@ def get_library_defining_db_template(self, id: int) -> DBLibrary:
         """
         return self.get_db_template(id).library
 
-    def get_db_template_dependencies(self, id: int) -> Tuple[DepsAssociation, ...]:
+    def get_db_template_dependencies(self, id: int) -> Tuple[DBTemplateDependency, ...]:
         """Get a template's dependencies and its arguments.
 
         If you don't need the arguments, consider using
@@ -360,12 +360,28 @@ def get_db_template_dependencies(self, id: int) -> Tuple[DepsAssociation, ...]:
         :rtype: tuple[tuple[int, list[str]]]
         """
         db_template_dependencies = tuple(
-            self.bm.session.query(DepsAssociation)
-            .filter(DepsAssociation.dependant_id == id)
+            self.bm.session.query(DBTemplateDependency)
+            .filter(DBTemplateDependency.template_id == id)
             .all()
         )
         return db_template_dependencies
 
+    def get_db_template_dependency(self, id: int) -> Optional[DBTemplateDependency]:
+        """Get template dependency object by its id
+
+        :param id: template dependency id
+        :type id: int
+        :return: the template dependency or None
+        :rtype: Optional[DBTemplateDependency]"""
+        try:
+            return (
+                self.bm.session.query(DBTemplateDependency)
+                .filter(DBTemplateDependency.id == id)
+                .one()
+            )
+        except NoResultFound:
+            return None
+
     def update_db_template_name(self, id: int, name: str) -> None:
         """Update database template name.
 
@@ -396,7 +412,11 @@ def update_db_template_optional_args(
         db_template.optional_args = optional_args
 
     def add_template_dependency_preliminary(
-        self, template_id: int, dependency_id: int, args: Dict[str, str]
+        self,
+        template_id: int,
+        dependency_library: str,
+        dependency_template: str,
+        args: Dict[str, str],
     ):
         """Creates a *preliminary* dependency between two templates. This dependency
         is preliminary because the bindings between the dependent/dependency templates
@@ -421,7 +441,7 @@ def add_template_dependency_preliminary(
         :raises ValueError: if dependant and dependency template don't share a
         """
         self.logger.debug(
-            f"Creating depencency from templates with ids: '{template_id}' and: '{dependency_id}'"
+            f"Creating depencency from templates with id: '{template_id}' and target: '{dependency_library}:{dependency_template}'"
         )
         templ = self.get_db_template(template_id)
         if "name" not in args.keys():
@@ -431,14 +451,16 @@ def add_template_dependency_preliminary(
         # In the past we had a check here to make sure the two templates were in the same library.
         # This has been removed because it wasn't actually necessary, but we may add it back in
         # in the future.
-        relationship = DepsAssociation(
-            dependant_id=template_id,
-            dependee_id=dependency_id,
-            args=args,
+        relationship = DBTemplateDependency(
+            template_id=template_id,
+            dependency_library_name=dependency_library,
+            dependency_template_name=dependency_template,
+            args=args,  # type: ignore
         )
 
         self.bm.session.add(relationship)
         self.bm.session.flush()
+        self.logger.debug(f"Created Dependency with id: '{relationship.id}'")
 
     def check_all_template_dependencies(self):
         """
@@ -453,7 +475,7 @@ def check_all_template_dependencies(self):
                 self.check_template_dependency_relationship(dep)
 
     @lru_cache(maxsize=128)
-    def check_template_dependency_relationship(self, dep: DepsAssociation):
+    def check_template_dependency_relationship(self, dep: DBTemplateDependency):
         """Verify that the dependency between two templates is well-formed. This involves
         a series of checks:
         - existence of the dependent and dependency templates is performed during the
@@ -469,17 +491,17 @@ def check_template_dependency_relationship(self, dep: DepsAssociation):
         :raises ValueError: if dependant and dependency template don't share a
             library
         """
-        template_id = dep.dependant_id
-        dependency_id = dep.dependee_id
+        template_id = dep.template_id
         args = dep.args
         self.logger.debug(
-            f"Creating depencency from templates with ids: '{template_id}' and: '{dependency_id}'"
+            f"Creating depencency from templates with ids: '{template_id}' and: '{dep.dependency_template_name}'"
         )
         from buildingmotif.dataclasses import Template
 
         templ = Template.load(template_id)
         params = templ.transitive_parameters
-        dep_templ = Template.load(dependency_id)
+
+        dep_templ = Template.load(dep.dependency_template.id)
         dep_params = dep_templ.transitive_parameters
 
         # check parameters are valid
@@ -520,11 +542,16 @@ def delete_template_dependency(self, template_id: int, dependency_id: int):
             f"Deleting depencency from templates with ids: '{template_id}' and: '{dependency_id}'"  # noqa
         )
 
+        dependency_template = self.get_db_template(dependency_id)
+
         relationship = (
-            self.bm.session.query(DepsAssociation)
+            self.bm.session.query(DBTemplateDependency)
             .filter(
-                DepsAssociation.dependant_id == template_id,
-                DepsAssociation.dependee_id == dependency_id,
+                DBTemplateDependency.template_id == template_id,
+                DBTemplateDependency.dependency_library_name
+                == dependency_template.library.name,
+                DBTemplateDependency.dependency_template_name
+                == dependency_template.name,
             )
             .one()
         )

buildingmotif/database/tables.py

@@ -1,14 +1,34 @@
-from typing import Dict, List
-
-from sqlalchemy import Column, ForeignKey, Integer, String, Text, UniqueConstraint
-from sqlalchemy.orm import Mapped, declarative_base, relationship
+from typing import Dict, List, Optional
+
+from sqlalchemy import (
+    Column,
+    ForeignKey,
+    Integer,
+    String,
+    Text,
+    UniqueConstraint,
+    event,
+    select,
+)
+from sqlalchemy.engine import Engine
+from sqlalchemy.exc import NoResultFound
+from sqlalchemy.ext.hybrid import hybrid_property
+from sqlalchemy.orm import Mapped, Session, declarative_base, relationship
 
 # from sqlalchemy.dialects.postgresql import JSON
 from buildingmotif.database.utils import JSONType
 
 Base = declarative_base()
 
 
+# https://docs.sqlalchemy.org/en/14/dialects/sqlite.html#foreign-key-support
+@event.listens_for(Engine, "connect")
+def set_sqlite_pragma(dbapi_connection, connection_record):
+    cursor = dbapi_connection.cursor()
+    cursor.execute("PRAGMA foreign_keys=ON")
+    cursor.close()
+
+
 class DBModel(Base):
     """A Model is a metadata model of all or part of a building."""
 
@@ -18,12 +38,13 @@ class DBModel(Base):
     description: Mapped[str] = Column(Text(), default="", nullable=False)
     graph_id: Mapped[str] = Column(String())
     manifest_id: Mapped[int] = Column(
-        Integer, ForeignKey("shape_collection.id"), nullable=False
+        Integer, ForeignKey("shape_collection.id", ondelete="CASCADE"), nullable=False
     )
     manifest: "DBShapeCollection" = relationship(
         "DBShapeCollection",
         uselist=False,
-        cascade="all,delete",
+        cascade="all",
+        passive_deletes=True,
     )
 
 
@@ -44,38 +65,19 @@ class DBLibrary(Base):
     id: Mapped[int] = Column(Integer, primary_key=True)
     name: Mapped[str] = Column(String(), nullable=False, unique=True)
 
+    # do not use passive_deletes here because we want to handle the deletion of templates
     templates: Mapped[List["DBTemplate"]] = relationship(
-        "DBTemplate", back_populates="library", cascade="all,delete"
+        "DBTemplate", back_populates="library", cascade="all"
     )
 
     shape_collection_id = Column(
-        Integer, ForeignKey("shape_collection.id"), nullable=False
+        Integer, ForeignKey("shape_collection.id", ondelete="CASCADE"), nullable=False
     )
     shape_collection: DBShapeCollection = relationship(
         "DBShapeCollection",
         uselist=False,
-        cascade="all,delete",
-    )
-
-
-class DepsAssociation(Base):
-    """Many-to-many relationship between dependant templates."""
-
-    __tablename__ = "deps_association_table"
-
-    id: Mapped[int] = Column(Integer, primary_key=True)
-    dependant_id: Mapped[int] = Column(ForeignKey("template.id"))
-    dependee_id: Mapped[int] = Column(ForeignKey("template.id"))
-    # args are a mapping of dependee args to dependant args
-    args: Mapped[Dict[str, str]] = Column(JSONType)  # type: ignore
-
-    __table_args__ = (
-        UniqueConstraint(
-            "dependant_id",
-            "dependee_id",
-            "args",
-            name="deps_association_unique_constraint",
-        ),
+        cascade="all",
+        passive_deletes=True,
     )
 
 
@@ -89,21 +91,13 @@ class DBTemplate(Base):
     body_id: Mapped[str] = Column(String())
     optional_args: Mapped[List[str]] = Column(JSONType)  # type: ignore
 
-    library_id: Mapped[int] = Column(Integer, ForeignKey("library.id"), nullable=False)
-    library: Mapped[DBLibrary] = relationship("DBLibrary", back_populates="templates")
-    dependencies: Mapped[List["DBTemplate"]] = relationship(
-        "DBTemplate",
-        secondary="deps_association_table",
-        primaryjoin=id == DepsAssociation.dependant_id,
-        secondaryjoin=id == DepsAssociation.dependee_id,
-        back_populates="dependants",
+    library_id: Mapped[int] = Column(
+        Integer, ForeignKey("library.id", ondelete="CASCADE"), nullable=False
     )
-    dependants: Mapped[List["DBTemplate"]] = relationship(
-        "DBTemplate",
-        secondary="deps_association_table",
-        primaryjoin=id == DepsAssociation.dependee_id,
-        secondaryjoin=id == DepsAssociation.dependant_id,
-        back_populates="dependencies",
+    library: Mapped[DBLibrary] = relationship("DBLibrary", back_populates="templates")
+
+    dependencies: Mapped["DBTemplateDependency"] = relationship(
+        "DBTemplateDependency", back_populates="template", cascade="all,delete-orphan"
     )
 
     __table_args__ = (
@@ -113,3 +107,58 @@ class DBTemplate(Base):
             name="name_library_unique_constraint",
         ),
     )
+
+
+class DBTemplateDependency(Base):
+    __tablename__ = "template_dependency"
+    id: Mapped[int] = Column(Integer, primary_key=True)
+
+    template_id: Mapped[int] = Column(
+        Integer, ForeignKey("template.id", ondelete="CASCADE"), nullable=False
+    )
+    template: Mapped[DBTemplate] = relationship(
+        DBTemplate, back_populates="dependencies"
+    )
+
+    dependency_library_name: Mapped[str] = Column(String, nullable=False)
+    dependency_template_name: Mapped[str] = Column(String, nullable=False)
+
+    # args are a mapping of dependee args to dependant args
+    args: Mapped[Dict[str, str]] = Column(JSONType)  # type: ignore
+
+    @hybrid_property
+    def dependency_template(self) -> Optional[DBTemplate]:
+        session = Session.object_session(self)
+        statement = (
+            select(DBTemplate)
+            .join(DBTemplate.library)
+            .where(
+                DBTemplate.name == self.dependency_template_name,
+                DBLibrary.name == self.dependency_library_name,
+            )
+        )
+        try:
+            return session.scalars(statement).one()
+        except NoResultFound:
+            return None
+
+    @dependency_template.expression
+    def _dependency_tempalate(self):
+        return (
+            select(DBTemplate)
+            .join(DBTemplate.library)
+            .where(
+                DBTemplate.name == self.dependency_template_name,
+                DBLibrary.name == self.dependency_library_name,
+            )
+        )
+
+    __table_args__ = (
+        UniqueConstraint(
+            "template_id",
+            "dependency_library_name",
+            "dependency_template_name",
+            "args",
+            name="template_dependency_unique_constraint",
+        ),
+    )