Merge pull request #139 from Waszker/fix/postgresql-trigger-identity

fix: support duplicate PostgreSQL trigger names

Author: DanCardin

src/sqlalchemy_declarative_extensions/dialects/postgresql/trigger.py

@@ -94,6 +94,10 @@ class Trigger(base.Trigger):
     old_table: str | None = None
     new_table: str | None = None
 
+    @property
+    def identity(self) -> tuple[str, str]:
+        return self.on, self.name
+
     @classmethod
     def before(
         cls, *events: TriggerEvents | str, on: str, execute: str, name: str = ""

src/sqlalchemy_declarative_extensions/trigger/base.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 from dataclasses import dataclass, field, replace
-from typing import Iterable, Sequence
+from typing import Hashable, Iterable, Sequence
 
 from sqlalchemy import MetaData
 from sqlalchemy.engine import Connection
@@ -18,6 +18,11 @@ class Trigger:
     on: str
     execute: str
 
+    @property
+    def identity(self) -> Hashable:
+        """Return the dialect-specific identity used to compare triggers."""
+        return self.name
+
     def named(self, name: str):
         return replace(self, name=name)
 

src/sqlalchemy_declarative_extensions/trigger/compare.py

@@ -51,34 +51,36 @@ def to_sql(self, connection: Connection | None = None) -> list[str]:
 def compare_triggers(connection: Connection, triggers: Triggers) -> list[Operation]:
     result: list[Operation] = []
 
-    triggers_by_name = {r.name: r for r in triggers.triggers}
-    expected_trigger_names = set(triggers_by_name)
+    triggers_by_identity = {r.identity: r for r in triggers.triggers}
+    expected_trigger_identities = set(triggers_by_identity)
 
     raw_existing_triggers = get_triggers(connection)
     existing_triggers = filter_triggers(
         raw_existing_triggers, exclude=triggers.ignore, include=triggers.include
     )
 
-    existing_triggers_by_name = {r.name: r for r in existing_triggers}
-    existing_trigger_names = set(existing_triggers_by_name)
+    existing_triggers_by_identity = {r.identity: r for r in existing_triggers}
+    existing_trigger_identities = set(existing_triggers_by_identity)
 
-    new_trigger_names = expected_trigger_names - existing_trigger_names
-    removed_trigger_names = existing_trigger_names - expected_trigger_names
+    new_trigger_identities = expected_trigger_identities - existing_trigger_identities
+    removed_trigger_identities = (
+        existing_trigger_identities - expected_trigger_identities
+    )
 
     for trigger in triggers:
-        trigger_created = trigger.name in new_trigger_names
+        trigger_created = trigger.identity in new_trigger_identities
 
         if trigger_created:
             result.append(CreateTriggerOp(trigger))
         else:
-            existing_trigger = existing_triggers_by_name[trigger.name]
+            existing_trigger = existing_triggers_by_identity[trigger.identity]
 
             if existing_trigger != trigger:
                 result.append(UpdateTriggerOp(existing_trigger, trigger))
 
     if not triggers.ignore_unspecified:
-        for removed_trigger in removed_trigger_names:
-            trigger = existing_triggers_by_name[removed_trigger]
+        for removed_trigger in removed_trigger_identities:
+            trigger = existing_triggers_by_identity[removed_trigger]
             result.append(DropTriggerOp(trigger))
 
     return result

tests/trigger/test_create_postgres.py

@@ -37,6 +37,10 @@ class Base(_Base):  # type: ignore
         .named("on_insert_foo")
         .when("pg_trigger_depth() < 1")
         .for_each_row(),
+        Trigger.after("insert", on="bar", execute="gimme")
+        .named("on_insert_foo")
+        .when("pg_trigger_depth() < 1")
+        .for_each_row(),
     )
 
 
@@ -46,6 +50,12 @@ class Foo(Base):
     id = Column(types.Integer(), primary_key=True)
 
 
+class Bar(Base):
+    __tablename__ = "bar"
+
+    id = Column(types.Integer(), primary_key=True)
+
+
 register_sqlalchemy_events(Base.metadata, functions=True, triggers=True)
 
 pg = create_postgres_fixture(engine_kwargs={"echo": True}, session=True)

tests/trigger/test_same_name_postgres.py

@@ -0,0 +1,63 @@
+from unittest.mock import Mock
+
+from sqlalchemy_declarative_extensions import Triggers
+from sqlalchemy_declarative_extensions.dialects.postgresql import Trigger
+from sqlalchemy_declarative_extensions.trigger.compare import (
+    CreateTriggerOp,
+    DropTriggerOp,
+    UpdateTriggerOp,
+    compare_triggers,
+)
+
+
+def trigger(on: str, event: str = "insert"):
+    return Trigger.after(event, on=on, execute="gimme").named("on_change")
+
+
+def test_same_name_on_different_tables_matches_existing(monkeypatch):
+    expected = Triggers().are(trigger("foo"), trigger("bar"))
+    monkeypatch.setattr(
+        "sqlalchemy_declarative_extensions.trigger.compare.get_triggers",
+        Mock(return_value=list(expected)),
+    )
+
+    assert compare_triggers(Mock(), expected) == []
+
+
+def test_same_name_on_different_table_is_created(monkeypatch):
+    foo_trigger = trigger("foo")
+    bar_trigger = trigger("bar")
+    expected = Triggers().are(foo_trigger, bar_trigger)
+    monkeypatch.setattr(
+        "sqlalchemy_declarative_extensions.trigger.compare.get_triggers",
+        Mock(return_value=[foo_trigger]),
+    )
+
+    assert compare_triggers(Mock(), expected) == [CreateTriggerOp(bar_trigger)]
+
+
+def test_same_name_updates_only_trigger_on_matching_table(monkeypatch):
+    foo_trigger = trigger("foo")
+    old_bar_trigger = trigger("bar")
+    new_bar_trigger = trigger("bar", event="update")
+    expected = Triggers().are(foo_trigger, new_bar_trigger)
+    monkeypatch.setattr(
+        "sqlalchemy_declarative_extensions.trigger.compare.get_triggers",
+        Mock(return_value=[foo_trigger, old_bar_trigger]),
+    )
+
+    assert compare_triggers(Mock(), expected) == [
+        UpdateTriggerOp(old_bar_trigger, new_bar_trigger)
+    ]
+
+
+def test_same_name_drops_only_trigger_on_unspecified_table(monkeypatch):
+    foo_trigger = trigger("foo")
+    bar_trigger = trigger("bar")
+    expected = Triggers().are(foo_trigger)
+    monkeypatch.setattr(
+        "sqlalchemy_declarative_extensions.trigger.compare.get_triggers",
+        Mock(return_value=[foo_trigger, bar_trigger]),
+    )
+
+    assert compare_triggers(Mock(), expected) == [DropTriggerOp(bar_trigger)]