Repository: geldata/gel
Pull request: Fix collisions between function parameters named id and the property
We name function params their actual names, which can collide with the
`id` column in tables and produce bad results. It could maybe collide
with source and target in link tables too. It doesn't collide with
everything, because most columns get uuid names.

Qualify references to params with the function name.

Fixes #5935.

Problem statement:
Parameter name conflicting with property name
- EdgeDB Version: 3.2+75dd258
- EdgeDB CLI Version: EdgeDB CLI 3.4.0+97cad0e
- OS Version: Mac OS 13.0.1 (22A400)

Steps to Reproduce:

Schema:
```module default {
    type User{ 
    }
    function getUser(id:uuid) -> set of User {
        using(
            select User filter .id = id
        )
    };
    function getUserStr(id:str) -> set of User {
        using(
            select User filter .id = <uuid>id
        )
    };
    function getUserById(idx:uuid) -> set of User {
        using(
            select User filter .id = idx
        )
    };
}```
edgedb> select getUser("a90edcd6-40af-11ee-8e22-4f46532b9eab");
{
  default::User {id: a90edcd6-40af-11ee-8e22-4f46532b9eab},
  default::User {id: a974e86e-40af-11ee-8e22-bffa776ab289},
  default::User {id: a9b8b292-40af-11ee-8e22-33a518703dff},
  default::User {id: a9faa440-40af-11ee-8e22-2b4b9170c10b},
}
Only getUserById works ok. getUserStr("") returns all users.

Note the difference is the parameter name.

Hints from issue discussion:

Commit message: Fix collisions between function parameters named id and the property

We name function params their actual names, which can collide with the
`id` column in tables and produce bad results. It could maybe collide
with source and target in link tables too. It doesn't collide with
everything, because most columns get uuid names.

Qualify references to params with the function name.

Fixes #5935.

Patch to review:
diff --git a/edb/pgsql/compiler/__init__.py b/edb/pgsql/compiler/__init__.py
index 1fa5f97b90f..e48e893f96c 100644
--- a/edb/pgsql/compiler/__init__.py
+++ b/edb/pgsql/compiler/__init__.py
@@ -57,7 +57,7 @@ def compile_ir_to_sql_tree(
     ignore_shapes: bool = False,
     explicit_top_cast: Optional[irast.TypeRef] = None,
     singleton_mode: bool = False,
-    use_named_params: bool = False,
+    use_named_params: Optional[tuple[str, ...]] = None,
     expected_cardinality_one: bool = False,
     expand_inhviews: bool = False,
     external_rvars: Optional[
diff --git a/edb/pgsql/compiler/clauses.py b/edb/pgsql/compiler/clauses.py
index 8dcc5c5ee92..f5a3ddaa49c 100644
--- a/edb/pgsql/compiler/clauses.py
+++ b/edb/pgsql/compiler/clauses.py
@@ -397,7 +397,7 @@ def fini_toplevel(
     stmt.ctes[:0] = list(ctx.param_ctes.values())
     stmt.ctes[:0] = list(ctx.type_ctes.values())
 
-    if not ctx.env.use_named_params:
+    if ctx.env.use_named_params is None:
         # Adding unused parameters into a CTE
 
         # Find the used parameters by searching the query, so we don't
@@ -437,7 +437,10 @@ def populate_argmap(
     logical_index = 1
     for map_extra in (False, True):
         for param in params:
-            if ctx.env.use_named_params and not param.name.isdecimal():
+            if (
+                ctx.env.use_named_params is not None
+                and not param.name.isdecimal()
+            ):
                 continue
             if param.name.startswith('__edb_arg_') != map_extra:
                 continue
diff --git a/edb/pgsql/compiler/context.py b/edb/pgsql/compiler/context.py
index 45d48707bac..8d973a73d19 100644
--- a/edb/pgsql/compiler/context.py
+++ b/edb/pgsql/compiler/context.py
@@ -475,7 +475,7 @@ class Environment:
 
     aliases: aliases.AliasGenerator
     output_format: Optional[OutputFormat]
-    use_named_params: bool
+    use_named_params: Optional[tuple[str, ...]]
     ptrref_source_visibility: Dict[irast.BasePointerRef, bool]
     expected_cardinality_one: bool
     ignore_object_shapes: bool
@@ -496,7 +496,7 @@ def __init__(
         self,
         *,
         output_format: Optional[OutputFormat],
-        use_named_params: bool,
+        use_named_params: Optional[tuple[str, ...]],
         expected_cardinality_one: bool,
         ignore_object_shapes: bool,
         singleton_mode: bool,
diff --git a/edb/pgsql/compiler/expr.py b/edb/pgsql/compiler/expr.py
index 8fcd6045271..017b92aab50 100644
--- a/edb/pgsql/compiler/expr.py
+++ b/edb/pgsql/compiler/expr.py
@@ -126,9 +126,9 @@ def compile_Parameter(
     params = [p for p in ctx.env.query_params if p.name == expr.name]
     param = params[0] if params else None
 
-    if not is_decimal and ctx.env.use_named_params:
+    if not is_decimal and ctx.env.use_named_params is not None:
         result = pgast.ColumnRef(
-            name=(expr.name, ),
+            name=ctx.env.use_named_params + (expr.name,),
             nullable=not expr.required,
         )
     elif param and param.sub_params:
diff --git a/edb/pgsql/delta.py b/edb/pgsql/delta.py
index 5ebbcc5ce3a..63d206424f4 100644
--- a/edb/pgsql/delta.py
+++ b/edb/pgsql/delta.py
@@ -1241,7 +1241,8 @@ def compile_edgeql_function_body(
             explicit_top_cast=irtyputils.type_to_typeref(  # note: no cache
                 schema, func.get_return_type(schema)),
             output_format=compiler.OutputFormat.NATIVE,
-            use_named_params=True)
+            use_named_params=self.get_pgname(func, schema)[-1:],
+        )
 
         return codegen.generate_source(sql_res.ast)
 
@@ -1366,7 +1367,7 @@ def compile_edgeql_function(self, func: s_funcs.Function, schema, context):
                 explicit_top_cast=irtyputils.type_to_typeref(  # note: no cache
                     schema, func.get_return_type(schema)),
                 output_format=compiler.OutputFormat.NATIVE,
-                use_named_params=True,
+                use_named_params=self.get_pgname(func, schema)[-1:],
                 backend_runtime_params=context.backend_runtime_params,
             )
             body = codegen.generate_source(sql_res.ast)
diff --git a/edb/pgsql/metaschema.py b/edb/pgsql/metaschema.py
index f840cab037b..9a3a70f0c2c 100644
--- a/edb/pgsql/metaschema.py
+++ b/edb/pgsql/metaschema.py
@@ -5060,7 +5060,7 @@ def _make_json_caster(
 
     cast_sql_res = compiler.compile_ir_to_sql_tree(
         cast_ir,
-        use_named_params=True,
+        use_named_params=(),
         singleton_mode=True,
     )
     cast_sql = codegen.generate_source(cast_sql_res.ast)
diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py
index a251e287e66..ca94f945d93 100644
--- a/tests/test_edgeql_ddl.py
+++ b/tests/test_edgeql_ddl.py
@@ -4876,6 +4876,25 @@ async def test_edgeql_ddl_function_36(self):
                 USING (assert_exists(foo));
             """)
 
+    async def test_edgeql_ddl_function_37(self):
+        obj = await self.con.query_single('''
+            create type X;
+            create function getUser(id: uuid) -> set of X {
+                using(
+                    select X filter .id = id
+                )
+            };
+            insert X;
+            insert X;
+        ''')
+        val = await self.con.query_single(
+            '''
+                select count(getUser(<uuid>$0))
+            ''',
+            obj.id,
+        )
+        self.assertEqual(val, 1)
+
     async def test_edgeql_ddl_function_rename_01(self):
         await self.con.execute("""
             CREATE FUNCTION foo(s: str) -> str {


Code review comments:
diff_hunk: @@ -57,7 +57,7 @@ def compile_ir_to_sql_tree(
     ignore_shapes: bool = False,
     explicit_top_cast: Optional[irast.TypeRef] = None,
     singleton_mode: bool = False,
-    use_named_params: bool = False,
+    use_named_params: Optional[tuple[str, ...]] = None,
path: edb/pgsql/compiler/__init__.py
text: @user1:
Should we rename the arg to "named_arg_prefix" or something?