Fix querybuilder toEdgeQL when doing 'insert many-to-one, select on…

…e' queries using `e.with` (#1191)

Fixes a bug where we were not also checking the child exprs of the
additional valid scopes added in this fix:
#1154.

Also fixes bug where scope validity was not being checked in the correct
order when some expressions in the `e.with` refs array are children of
other exprs.

integration-tests/lts/insert.test.ts

@@ -432,4 +432,84 @@ describe("insert", () => {
 
     await query.run(client);
   });
+
+  test("insert many-to-one and select one", async () => {
+    const edgeql = e
+      .params(
+        {
+          name: e.str,
+          nemeses: e.array(e.tuple({ name: e.str })),
+        },
+        (params) => {
+          const hero = e.insert(e.Hero, {
+            name: params.name,
+          });
+          const villains = e.for(e.array_unpack(params.nemeses), (nemesis) => {
+            return e.insert(e.Villain, {
+              name: nemesis.name,
+              nemesis: hero,
+            });
+          });
+
+          return e.with([villains], e.select(hero));
+        },
+      )
+      .toEdgeQL();
+
+    // Also test including `hero` in the `with` refs
+    assert.equal(
+      edgeql,
+      e
+        .params(
+          {
+            name: e.str,
+            nemeses: e.array(e.tuple({ name: e.str })),
+          },
+          (params) => {
+            const hero = e.insert(e.Hero, {
+              name: params.name,
+            });
+            const villains = e.for(
+              e.array_unpack(params.nemeses),
+              (nemesis) => {
+                return e.insert(e.Villain, {
+                  name: nemesis.name,
+                  nemesis: hero,
+                });
+              },
+            );
+
+            return e.with([hero, villains], e.select(hero));
+          },
+        )
+        .toEdgeQL(),
+    );
+    assert.equal(
+      edgeql,
+      e
+        .params(
+          {
+            name: e.str,
+            nemeses: e.array(e.tuple({ name: e.str })),
+          },
+          (params) => {
+            const hero = e.insert(e.Hero, {
+              name: params.name,
+            });
+            const villains = e.for(
+              e.array_unpack(params.nemeses),
+              (nemesis) => {
+                return e.insert(e.Villain, {
+                  name: nemesis.name,
+                  nemesis: hero,
+                });
+              },
+            );
+
+            return e.with([villains, hero], e.select(hero));
+          },
+        )
+        .toEdgeQL(),
+    );
+  });
 });

integration-tests/lts/update.test.ts

@@ -105,6 +105,38 @@ describe("update", () => {
     }).toEdgeQL();
   });
 
+  test("nested update and explicit with, unwrapped select should fail", async () => {
+    assert.throws(
+      () =>
+        e
+          .params({ movieId: e.uuid }, (params) => {
+            const movie = e.select(e.Movie, (m) => ({
+              filter: e.op(m.id, "=", params.movieId),
+            }));
+
+            const updateChar = e.update(movie.characters, (c) => ({
+              set: {
+                name: e.str_lower(c.name),
+              },
+            }));
+
+            const updateProfile = e.update(movie.profile, (p) => ({
+              set: {
+                a: e.str_upper(p.a),
+              },
+            }));
+
+            return e.with([updateChar, updateProfile], movie);
+          })
+          .toEdgeQL(),
+      {
+        message:
+          `Ref expressions in with() cannot reference the expression to which the ` +
+          `'WITH' block is being attached. Consider wrapping the expression in a select.`,
+      },
+    );
+  });
+
   test("scoped update", async () => {
     const query = e.update(e.Hero, (hero) => ({
       filter_single: e.op(hero.name, "=", data.spidey.name),

integration-tests/lts/with.test.ts

@@ -371,11 +371,11 @@ SELECT {
       `WITH
   __withVar_0 := (
     WITH
-      __withVar_1 := { <std::int64>1, <std::int64>2, <std::int64>3 },
-      __withVar_2 := __withVar_1
+      __withVar_2 := { <std::int64>1, <std::int64>2, <std::int64>3 },
+      __withVar_3 := __withVar_2
     SELECT {
-      multi numbers := assert_exists(__withVar_1),
-      multi numbersAlias := assert_exists(__withVar_2)
+      multi numbers := assert_exists(__withVar_2),
+      multi numbersAlias := assert_exists(__withVar_3)
     }
   )
 SELECT {

packages/generate/src/syntax/toEdgeQL.ts

@@ -209,7 +209,8 @@ export function $toEdgeQL(this: any) {
       refData.aliases.length > 0
     ) {
       // first, check if expr is bound to scope
-      let withBlock = refData.boundScope;
+      let withBlock = (refData.boundScope?.__expr__ ??
+        null) as WithScopeExpr | null;
 
       const parentScopes = [...refData.parentScopes];
 
@@ -275,13 +276,31 @@ export function $toEdgeQL(this: any) {
         withBlocks.set(withBlock, new Set());
       }
 
+      if (
+        refData.boundScope &&
+        refData.boundScope.__refs__.some(
+          (ref: any) =>
+            ref !== expr &&
+            seen.has(ref) &&
+            walkExprCtx.seen.get(ref)?.childExprs.includes(expr),
+        )
+      ) {
+        // if expr is bound to a e.with, and any of it's siblings in the e.with
+        // refs contain this expr as a child and haven't been resolved yet,
+        // return this expr to `seen` to be resolved later
+        seen.set(expr, refData);
+        continue;
+      }
+
       // check all references and aliases are within this block
-      const validScopes = new Set([
-        withBlock,
-        // expressions already explictly bound to with block are also valid scopes
-        ...(withBlocks.get(withBlock) ?? []),
-        ...walkExprCtx.seen.get(withBlock)!.childExprs,
-      ]);
+      const validScopes = new Set(
+        [
+          withBlock,
+          // expressions already explictly bound to with block are also valid scopes
+          ...(withBlocks.get(withBlock) ?? []),
+        ] // expand out child exprs of valid with scopes
+          .flatMap((expr) => [expr, ...walkExprCtx.seen.get(expr)!.childExprs]),
+      );
       for (const scope of [
         ...refData.parentScopes,
         ...util.flatMap(refData.aliases, (alias) => [
@@ -350,7 +369,7 @@ interface WalkExprTreeCtx {
       // tracks all child exprs
       childExprs: SomeExpression[];
       // tracks bound scope from e.with
-      boundScope: WithScopeExpr | null;
+      boundScope: $expr_With | null;
       // tracks aliases from e.alias
       aliases: SomeExpression[];
       linkProps: $expr_PathLeaf[];
@@ -444,7 +463,7 @@ function walkExprTree(
         if (seenRef.boundScope) {
           throw new Error(`Expression bound to multiple 'WITH' blocks`);
         }
-        seenRef.boundScope = expr.__expr__;
+        seenRef.boundScope = expr;
       }
       break;
     case ExpressionKind.Literal: