Repository: geldata/gel
Pull request: Separate triggers by kind in compilation.
Fix #5794

When checking for recursive triggers, the compilation now checks `insert`, `update`, and `delete` separately.

Given the schema in #5794, running the following:
```
insert Theme {name := "hello"};
update Theme filter Theme.name = "hello" set {name := "world
"};
select Theme {*}
```

Produces:
```
{
  default::Theme {id: c893e790-e645-11ee-a4e6-a74c0e50141c, name: 'world'},
  default::Theme {id: f4f70b0a-e645-11ee-a4e6-c360cb91f3c6, name: 'hello'},
}
```

As another example, this schema is now valid but becomes invalid if `update` is changed to `insert`
```  type A {
    required name: str;
    trigger create_A after update for each do (
      insert B {
        name := __new__.name
      }
    );
  }

  type B {
    required name: str;
    trigger create_B after insert for each do (
      insert A {
        name := __new__.name
      }
    );
  }
```


Problem statement:
[Triggers] False positive recursive trigger
<!-- Please search existing issues to avoid creating duplicates. -->

<!--
For the EdgeDB Version: run `edgedb query 'select sys::get_version_as_str()'` from your project directory (or run `select sys::get_version_as_str();` in the EdgeDB interactive shell).
For the EdgeDB CLI Version: Run `edgedb --version` from anywhere
-->

- EdgeDB Version: "3.1+a33a828"
- EdgeDB CLI Version: `EdgeDB CLI 3.4.0+4d95a2c`
- OS Version: MacOS

Steps to Reproduce:

1. Migrate
2. see the error

The trigger should happen on update only, and only inserts an object. This wouldn't trigger another update.
```
error: trigger on default::Theme is recursive
```

<!-- If the issue is about a query error, please also provide your schema -->

Schema:
```
abstract type WithRevisions {}
	type Revision {
		required object: WithRevisions;
	}
type Theme extending WithRevisions {
	        required name: str;
		trigger revision_create after update for each do (
			insert Revision {
				object := (
					insert Theme {
						name := __old__.name
					}
				)
			}
		);
	}
```

Hints from issue discussion:
I have same error when using `when` clause (https://github.com/edgedb/edgedb/pull/6184)

```edgeql
type User {
  link chats := .<members[is ChatBase];
  multi chat_memberships: ChatMembership {
     on source delete delete target;
     constraint exclusive;
  };
  trigger RequireChatsMembership after insert, update for each when (count(__new__.chats) > count(__new__.chat_memberships)) do (
     for new_chat in __new__.chats
     union (
        update User 
        filter .num = __new__.num
        set {
           chat_memberships += (
              insert ChatMembership {
                 chat := new_chat,
              }
           )
        }
     )
  );
}
```

```
error: trigger on default::User is recursive
```
@MrFoxPro's example is being rejected correctly. It is doing an update from within an update trigger, and so should be rejected (and is).

The original issue does seem like a false positive, though.

Commit message: Separate triggers by kind in compilation.

Patch to review:
diff --git a/edb/edgeql/compiler/triggers.py b/edb/edgeql/compiler/triggers.py
index 047f915dd3f..ad1b4354d60 100644
--- a/edb/edgeql/compiler/triggers.py
+++ b/edb/edgeql/compiler/triggers.py
@@ -138,6 +138,7 @@ def compile_trigger(
 def compile_triggers_phase(
     dml_stmts: Collection[irast.MutatingStmt],
     defining_trigger_on: Optional[s_types.Type],
+    defining_trigger_kinds: Optional[Collection[qltypes.TriggerKind]],
     *,
     ctx: context.ContextLevel,
 ) -> tuple[irast.Trigger, ...]:
@@ -165,11 +166,13 @@ def compile_triggers_phase(
 
         # Process all the types, starting with the base type
         for subtype in sorted(stypes, key=lambda t: t != stype):
-            if defining_trigger_on and subtype.issubclass(
-                    ctx.env.schema, defining_trigger_on):
+            if (defining_trigger_on and defining_trigger_kinds and
+                kind in defining_trigger_kinds and
+                subtype.issubclass(ctx.env.schema, defining_trigger_on)
+            ):
                 name = str(defining_trigger_on.get_name(ctx.env.schema))
                 raise errors.SchemaDefinitionError(
-                    f"trigger on {name} is recursive"
+                    f"trigger on {name} after {kind} is recursive"
                 )
 
             for trigger in subtype.get_relevant_triggers(kind, schema):
@@ -201,31 +204,51 @@ def compile_triggers(
     defining_trigger = (
         ctx.env.options.schema_object_context == s_triggers.Trigger)
     defining_trigger_on = None
+    defining_trigger_kinds = None
     if defining_trigger:
         defining_trigger_on = setgen.get_set_type(
             ctx.anchors['__trigger_type__'], ctx=ctx)
+        trigger_kinds_anchor = ctx.anchors['__trigger_kinds__']
+        if isinstance(trigger_kinds_anchor.expr, irast.Array):
+            defining_trigger_kinds = [
+                qltypes.TriggerKind(element.expr.value)
+                for element in trigger_kinds_anchor.expr.elements
+                if isinstance(element.expr, irast.StringConstant)
+            ]
 
     ir_triggers: list[tuple[irast.Trigger, ...]] = []
     start = 0
-    all_trigger_types: set[irast.TypeRef] = set()
+    all_trigger_causes: set[tuple[irast.TypeRef, qltypes.TriggerKind]] = set()
     while start < len(ctx.env.dml_stmts):
         end = len(ctx.env.dml_stmts)
-        new = compile_triggers_phase(
-            ctx.env.dml_stmts[start:], defining_trigger_on, ctx=ctx)
-        new_types = {x for t in new for x in t.all_affected_types}
+        compiled_triggers = compile_triggers_phase(
+            ctx.env.dml_stmts[start:],
+            defining_trigger_on,
+            defining_trigger_kinds,
+            ctx=ctx
+        )
+        new_causes = {
+            (affected_type, kind)
+            for compiled_trigger in compiled_triggers
+            for affected_type in compiled_trigger.all_affected_types
+            for kind in compiled_trigger.kinds
+        }
 
         # Any given type is allowed allowed to have its triggers fire
         # in *one* phase of trigger execution, since the semantics get
         # a little unclear otherwise. We might relax this later.
-        overlap = new_types & all_trigger_types
+        overlap = new_causes & all_trigger_causes
         if overlap:
-            names = sorted(str(t.name_hint) for t in overlap)
+            names = sorted(
+                f"{str(cause[0].name_hint)} after {cause[1]}"
+                for cause in overlap
+            )
             raise errors.QueryError(
                 f"trigger would need to be executed in multiple stages on "
                 f"{', '.join(names)}"
             )
-        all_trigger_types |= new_types
-        ir_triggers.append(new)
+        all_trigger_causes |= new_causes
+        ir_triggers.append(compiled_triggers)
         start = end
 
     return tuple(ir_triggers)
diff --git a/edb/schema/triggers.py b/edb/schema/triggers.py
index c8929164219..521d805e0ff 100644
--- a/edb/schema/triggers.py
+++ b/edb/schema/triggers.py
@@ -204,16 +204,16 @@ def compile_expr_field(
             scope = self._get_scope(schema)
             kinds = self._get_kinds(schema)
 
-            anchors: dict[str, pathid.PathId | s_types.Type] = {}
+            singleton_anchors: dict[str, pathid.PathId | s_types.Type] = {}
             if qltypes.TriggerKind.Insert not in kinds:
-                anchors['__old__'] = pathid.PathId.from_type(
+                singleton_anchors['__old__'] = pathid.PathId.from_type(
                     schema,
                     source,
                     typename=sn.QualName(module='__derived__', name='__old__'),
                     env=None,
                 )
             if qltypes.TriggerKind.Delete not in kinds:
-                anchors['__new__'] = pathid.PathId.from_type(
+                singleton_anchors['__new__'] = pathid.PathId.from_type(
                     schema,
                     source,
                     typename=sn.QualName(module='__derived__', name='__new__'),
@@ -221,10 +221,19 @@ def compile_expr_field(
                 )
 
             singletons = (
-                frozenset(anchors.values())
+                frozenset(singleton_anchors.values())
                 if scope == qltypes.TriggerScope.Each else frozenset()
             )
+            anchors: dict[
+                str, pathid.PathId | s_types.Type | qlast.Expr
+            ] = {k: v for k, v in singleton_anchors.items()}
             anchors['__trigger_type__'] = source
+            anchors['__trigger_kinds__'] = qlast.Array(
+                elements=[
+                    qlast.StringConstant.from_python(kind)
+                    for kind in kinds
+                ]
+            )
 
             assert isinstance(source, s_types.Type)
 


Code review comments:
diff_hunk: @@ -204,27 +204,36 @@ def compile_expr_field(
             scope = self._get_scope(schema)
             kinds = self._get_kinds(schema)
 
-            anchors: dict[str, pathid.PathId | s_types.Type] = {}
+            singleton_anchors: dict[str, pathid.PathId | s_types.Type] = {}
             if qltypes.TriggerKind.Insert not in kinds:
-                anchors['__old__'] = pathid.PathId.from_type(
+                singleton_anchors['__old__'] = pathid.PathId.from_type(
                     schema,
                     source,
                     typename=sn.QualName(module='__derived__', name='__old__'),
                     env=None,
                 )
             if qltypes.TriggerKind.Delete not in kinds:
-                anchors['__new__'] = pathid.PathId.from_type(
+                singleton_anchors['__new__'] = pathid.PathId.from_type(
                     schema,
                     source,
                     typename=sn.QualName(module='__derived__', name='__new__'),
                     env=None,
                 )
 
             singletons = (
-                frozenset(anchors.values())
+                frozenset(singleton_anchors.values())
                 if scope == qltypes.TriggerScope.Each else frozenset()
             )
+            anchors: dict[
+                str, pathid.PathId | s_types.Type | qlast.Expr
+            ] = {k: v for k, v in singleton_anchors.items()}
             anchors['__trigger_type__'] = source
+            anchors['__trigger_kinds__'] = qlast.Array(
+                elements=[
+                    qlast.StringConstant.from_python(kind)
+                    for kind in kinds
+                ]
+            )
path: edb/schema/triggers.py
text: @user1:
I think we probably should add a new field to Options rather than smuggle it in through anchors

(Though smuggling it in via an array literal *is* clever)