Repository: geldata/gel
Pull request: Fix tuples in global variables and do error checks on arrays
We need to recode tuples to inject OIDs, like we do with arrays and
the like. Somewhat annoyingly, this ends up as a totally separate code
path from the existing re-encoding of tuples for parameters.

This is because for globals, which are schema declared, we have
postgres composite types, and it would be wasteful and convoluted
to split the tuples up instead of just using them.

And then, once we are injecting the OIDs, we can do missing error
checking that could allow invalid data in. (This was the main thing
that motivated fixing this now instead of deffering.)

Fixes #5356.

Problem statement:
ISE when providing tuple global
- EdgeDB Version: 3.0-dev.7404+local

Providing a tuple global value throws the error `InternalServerError: binary data has type 0 (-) instead of expected 25 (text) in record column 1`. (This error happens in both js and python clients, so I think it's a server bug instead of a problem with the client's encoding of globals).

Steps to Reproduce:

```js
const {createClient} = require("edgedb");

const client = createClient("_localdev");

(async () => {

  console.log(
    await client
      .withGlobals({"default::tupleGlobal": ["test", 132]})
      .query(`select global tupleGlobal`)
  );
  // InternalServerError: binary data has type 0 (-) instead of expected 25 (text) in record column 1

})();
```

```python
import edgedb

client = edgedb.create_client('_localdev')

print(
    client.with_globals({'default::tupleGlobal': ('test', 123)}).query('select global tupleGlobal')
)
# edgedb.errors.InternalServerError: binary data has type 0 (-) instead of expected 25 (text) in record column 1

client.close()
```

Schema:

```esdl
global default::tupleGlobal -> tuple<std::str, std::int64>;
```

Hints from issue discussion:

Commit message: Fix tuples in global variables and do error checks on arrays

We need to recode tuples to inject OIDs, like we do with arrays and
the like. Somewhat annoyingly, this ends up as a totally separate code
path from the existing re-encoding of tuples for parameters.

This is because for globals, which are schema declared, we have
postgres composite types, and it would be wasteful and convoluted
to split the tuples up instead of just using them.

And then, once we are injecting the OIDs, we can do missing error
checking that could allow invalid data in. (This was the main thing
that motivated fixing this now instead of deffering.)

Fixes #5356.

Patch to review:
diff --git a/edb/server/compiler/sertypes.py b/edb/server/compiler/sertypes.py
index 95b92dc49b5..e343a5750e2 100644
--- a/edb/server/compiler/sertypes.py
+++ b/edb/server/compiler/sertypes.py
@@ -1090,6 +1090,21 @@ def _parse_range_descriptor(
     return RangeDesc(tid=tid, inner=subtype)
 
 
+def _make_global_rep(typ: s_types.Type, ctx: Context) -> object:
+    if isinstance(typ, s_types.Tuple):
+        subtyps = typ.get_subtypes(ctx.schema)
+        return (
+            1,
+            tuple(subtyp.id for subtyp in subtyps),
+            tuple(_make_global_rep(subtyp, ctx) for subtyp in subtyps),
+        )
+    elif isinstance(typ, s_types.Array):
+        subtyp = typ.get_element_type(ctx.schema)
+        return (2, subtyp.id, _make_global_rep(subtyp, ctx))
+    else:
+        return None
+
+
 class StateSerializerFactory:
     def __init__(self, std_schema: s_schema.Schema):
         """
@@ -1178,14 +1193,14 @@ def make(
             self._schema, user_schema, global_schema)
 
         globals_shape = []
-        array_type_ids = {}
+        global_reps = {}
         for g in ctx.schema.get_objects(type=s_globals.Global):
             if g.is_computable(ctx.schema):
                 continue
             name = str(g.get_name(ctx.schema))
             s_type = g.get_target(ctx.schema)
-            if isinstance(s_type, s_types.Array):
-                array_type_ids[name] = s_type.get_element_type(ctx.schema).id
+            if isinstance(s_type, (s_types.Array, s_types.Tuple)):
+                global_reps[name] = _make_global_rep(s_type, ctx)
             cardinality = cardinality_from_ptr(g, ctx.schema)
             globals_shape.append((name, s_type, cardinality))
 
@@ -1209,7 +1224,7 @@ def make(
         assert isinstance(codec, InputShapeDesc)
         codec.fields['globals'][1].__dict__['data_raw'] = True
 
-        return StateSerializer(type_id, type_data, codec, array_type_ids)
+        return StateSerializer(type_id, type_data, codec, global_reps)
 
 
 class StateSerializer:
@@ -1218,12 +1233,12 @@ def __init__(
         type_id: uuid.UUID,
         type_data: bytes,
         codec: TypeDesc,
-        globals_array_type_ids: typing.Dict[str, uuid.UUID],
+        global_reps: typing.Dict[str, object],
     ) -> None:
         self._type_id = type_id
         self._type_data = type_data
         self._codec = codec
-        self._globals_array_type_ids = globals_array_type_ids
+        self._global_reps = global_reps
 
     @property
     def type_id(self) -> uuid.UUID:
@@ -1238,11 +1253,11 @@ def encode(self, state: Any) -> bytes:
     def decode(self, state: bytes) -> Any:
         return self._codec.decode(state)
 
-    def get_global_array_type_id(
+    def get_global_type_rep(
         self,
         global_name: str,
-    ) -> Optional[uuid.UUID]:
-        return self._globals_array_type_ids.get(global_name)
+    ) -> Optional[object]:
+        return self._global_reps.get(global_name)
 
 
 def derive_alias(
diff --git a/edb/server/dbview/dbview.pxd b/edb/server/dbview/dbview.pxd
index 4e8a467f21a..6ec285c686a 100644
--- a/edb/server/dbview/dbview.pxd
+++ b/edb/server/dbview/dbview.pxd
@@ -210,4 +210,3 @@ cdef class DatabaseConnectionView:
     cdef describe_state(self)
     cdef encode_state(self)
     cdef decode_state(self, type_id, data)
-    cdef inline recode_global(self, serializer, k, v)
diff --git a/edb/server/dbview/dbview.pyx b/edb/server/dbview/dbview.pyx
index 0ae0ee5ffd2..11e8939ce3b 100644
--- a/edb/server/dbview/dbview.pyx
+++ b/edb/server/dbview/dbview.pyx
@@ -44,6 +44,10 @@ from edb.pgsql import dbops
 
 cimport cython
 
+from edb.server.protocol.args_ser cimport (
+    recode_global,
+)
+
 __all__ = ('DatabaseIndex', 'DatabaseConnectionView', 'SideEffects')
 
 cdef DEFAULT_MODALIASES = immutables.Map({None: defines.DEFAULT_MODULE_ALIAS})
@@ -675,7 +679,7 @@ cdef class DatabaseConnectionView:
         globals_ = immutables.Map({
             k: config.SettingValue(
                 name=k,
-                value=self.recode_global(serializer, k, v),
+                value=recode_global(self, v, serializer.get_global_type_rep(k)),
                 source='global',
                 scope=qltypes.ConfigScope.GLOBAL,
             ) for k, v in state.get('globals', {}).items()
@@ -687,17 +691,6 @@ cdef class DatabaseConnectionView:
             aliases, session_config, globals_, type_id, data
         )
 
-    cdef inline recode_global(self, serializer, k, v):
-        if v and v[:4] == b'\x00\x00\x00\x01':
-            array_type_id = serializer.get_global_array_type_id(k)
-            if array_type_id:
-                va = bytearray(v)
-                va[8:12] = INT32_PACKER(
-                    self.resolve_backend_type_id(array_type_id)
-                )
-                v = bytes(va)
-        return v
-
     property txid:
         def __get__(self):
             return self._txid
diff --git a/edb/server/protocol/args_ser.pxd b/edb/server/protocol/args_ser.pxd
index 76ddd54e990..04707808038 100644
--- a/edb/server/protocol/args_ser.pxd
+++ b/edb/server/protocol/args_ser.pxd
@@ -36,3 +36,9 @@ cdef recode_bind_args_for_script(
     ssize_t start,
     ssize_t end,
 )
+
+cdef bytes recode_global(
+    dbview.DatabaseConnectionView dbv,
+    bytes glob,
+    object glob_descriptor,
+)
diff --git a/edb/server/protocol/args_ser.pyx b/edb/server/protocol/args_ser.pyx
index 990b4ecfe12..8c184055769 100644
--- a/edb/server/protocol/args_ser.pyx
+++ b/edb/server/protocol/args_ser.pyx
@@ -184,7 +184,7 @@ cdef WriteBuffer recode_bind_args(
                 if param.array_type_id is not None:
                     array_tid = dbv.resolve_backend_type_id(
                         param.array_type_id)
-                    recode_array(dbv, &in_buf, out_buf, in_len, array_tid)
+                    recode_array(dbv, &in_buf, out_buf, in_len, array_tid, None)
                 else:
                     data = frb_read(&in_buf, in_len)
                     out_buf.write_cstr(data, in_len)
@@ -208,17 +208,112 @@ cdef WriteBuffer recode_bind_args(
     return out_buf
 
 
-cdef WriteBuffer recode_array(
+cdef bytes recode_global(
+    dbv: dbview.DatabaseConnectionView,
+    glob: bytes,
+    glob_descriptor: object,
+):
+    cdef:
+        WriteBuffer out_buf
+        FRBuffer in_buf
+
+    if glob_descriptor is None:
+        return glob
+
+    out_buf = WriteBuffer.new()
+
+    assert cpython.PyBytes_CheckExact(glob)
+    frb_init(
+        &in_buf,
+        cpython.PyBytes_AS_STRING(glob),
+        cpython.Py_SIZE(glob))
+
+    _recode_global(dbv, &in_buf, out_buf, in_buf.len, glob_descriptor)
+
+    if frb_get_len(&in_buf):
+        raise errors.InputDataError('unexpected trailing data in buffer')
+
+    return bytes(memoryview(out_buf))
+
+
+cdef _recode_global(
+    dbv: dbview.DatabaseConnectionView,
+    FRBuffer* in_buf,
+    out_buf: WriteBuffer,
+    in_len: ssize_t,
+    glob_descriptor: object,
+):
+    if glob_descriptor is None:
+        data = frb_read(in_buf, in_len)
+        out_buf.write_cstr(data, in_len)
+    elif glob_descriptor[0] == 1:  # tuple
+        _, el_tids, el_infos = glob_descriptor
+        recode_global_tuple(dbv, in_buf, out_buf, in_len, el_tids, el_infos)
+    elif glob_descriptor[0] == 2:  # array
+        _, el_tid, tuple_info = glob_descriptor
+        btid = dbv.resolve_backend_type_id(el_tid)
+        recode_array(dbv, in_buf, out_buf, in_len, btid, tuple_info)
+
+
+cdef recode_global_tuple(
+    dbv: dbview.DatabaseConnectionView,
+    FRBuffer* in_buf,
+    out_buf: WriteBuffer,
+    in_len: ssize_t,
+    el_tids: tuple,
+    el_infos: tuple,
+):
+    """
+    Tuples in globals need to have NULLs checked and oids injected,
+    like arrays do.
+
+    Annoyingly this is a *totally separate* code path than tuple query
+    parameters go through. This is because global tuples actually can
+    get passed as postgres composite types, since they are declared in
+    the schema.
+    """
+    cdef:
+        WriteBuffer buf
+        ssize_t cnt
+        ssize_t idx
+        ssize_t num
+        ssize_t tag
+        FRBuffer sub_buf
+
+    frb_slice_from(&sub_buf, in_buf, in_len)
+
+    cnt = <uint32_t>hton.unpack_int32(frb_read(&sub_buf, 4))
+    out_buf.write_int32(cnt)
+    num = len(el_tids)
+    if cnt != num:
+        raise errors.InputDataError(
+            f"tuple length mismatch: {cnt} vs {num}")
+    for idx in range(num):
+        frb_read(&sub_buf, 4)
+        el_btid = dbv.resolve_backend_type_id(el_tids[idx])
+        out_buf.write_int32(<int32_t>el_btid)
+
+        in_len = hton.unpack_int32(frb_read(&sub_buf, 4))
+        if in_len < 0:
+            raise errors.InputDataError("invalid NULL inside type")
+        out_buf.write_int32(in_len)
+        _recode_global(dbv, &sub_buf, out_buf, in_len, el_infos[idx])
+
+    if frb_get_len(&sub_buf):
+        raise errors.InputDataError('unexpected trailing data in buffer')
+
+
+cdef recode_array(
     dbv: dbview.DatabaseConnectionView,
     FRBuffer* in_buf,
     out_buf: WriteBuffer,
     in_len: ssize_t,
     array_tid: int32_t,
+    tuple_info: object,
 ):
     # For a standalone array, we still need to inject oids and reject
     # NULL elements.
     cdef:
-        WriteBuffer buf
         ssize_t cnt
         ssize_t idx
         ssize_t num
@@ -252,13 +347,16 @@ cdef WriteBuffer recode_array(
             if in_len < 0:
                 raise errors.InputDataError("invalid NULL inside type")
             out_buf.write_int32(in_len)
-            data = frb_read(&sub_buf, in_len)
-            out_buf.write_cstr(data, in_len)
+            if tuple_info is None:
+                data = frb_read(&sub_buf, in_len)
+                out_buf.write_cstr(data, in_len)
+            else:
+                _recode_global(dbv, &sub_buf, out_buf, in_len, tuple_info)
         if frb_get_len(&sub_buf):
             raise errors.InputDataError('unexpected trailing data in buffer')
 
 
-cdef WriteBuffer _decode_tuple_args_core(
+cdef _decode_tuple_args_core(
     FRBuffer* in_buf,
     out_bufs: tuple[WriteBuffer],
     counts: list[int],
diff --git a/edb/testbase/connection.py b/edb/testbase/connection.py
index 94b3710584a..b7809fe520b 100644
--- a/edb/testbase/connection.py
+++ b/edb/testbase/connection.py
@@ -348,6 +348,9 @@ def add_log_listener(self, callback):
     def remove_log_listener(self, callback):
         self._log_listeners.discard(callback)
 
+    def _get_state(self):
+        return self._options.state
+
     def _on_log_message(self, msg):
         if self._log_listeners:
             loop = asyncio.get_running_loop()
@@ -380,6 +383,7 @@ async def _execute(self, script: abstract.ExecuteContext) -> None:
             qc=script.cache.query_cache,
             output_format=protocol.OutputFormat.NONE,
             allow_capabilities=edgedb_enums.Capability.ALL,  # type: ignore
+            state=script.state and script.state.as_dict(),
         )
 
     async def ensure_connected(self):
@@ -398,6 +402,7 @@ async def raw_query(self, query_context: abstract.QueryContext):
             expect_one=query_context.query_options.expect_one,
             required_one=query_context.query_options.required_one,
             allow_capabilities=edgedb_enums.Capability.ALL,  # type: ignore
+            state=query_context.state and query_context.state.as_dict(),
         )
 
     async def _fetchall(
diff --git a/tests/test_edgeql_globals.py b/tests/test_edgeql_globals.py
index 91c87e2fe82..fb9295cfeeb 100644
--- a/tests/test_edgeql_globals.py
+++ b/tests/test_edgeql_globals.py
@@ -17,6 +17,7 @@
 #
 
 
+import dataclasses
 import os.path
 
 import edgedb
@@ -62,6 +63,13 @@ class TestEdgeQLGlobals(tb.QueryTestCase):
                 select get_current_user().deck
                 filter .name not in array_unpack(global banned_cards)
             );
+
+            create global tupleStr -> tuple<str, int64>;
+            create global tupleNums -> tuple<int64, int64>;
+            create global arrayTuple -> array<tuple<int64, int64>>;
+            create global arrayTuple2 -> array<tuple<str, array<int64>>>;
+            create global arrayTuple3 ->
+              array<tuple<tuple<str, bool>, array<int64>>>;
         '''
     ]
 
@@ -403,3 +411,32 @@ async def test_edgeql_globals_state_cardinality(self):
         with self.assertRaises(edgedb.CardinalityViolationError):
             await self.con.execute("select global cur_user")
         self.con._protocol.last_state = None
+
+    async def test_edgeql_globals_composite(self):
+        # Test various composite global variables.
+
+        # HACK: Using with_globals on testbase.Connection doesn't
+        # work, and I timed out on understanding why; I got the state
+        # plumbed into the real client library code, where the state
+        # codec was not encoding it.
+        # It isn't actually important for that to work, so for now
+        # we create a connection with the real honest client library.
+        con = edgedb.create_async_client(
+            **self.get_connect_args(database=self.con.dbname)
+        )
+        try:
+            globs = dict(
+                tupleStr=('foo', 42),
+                tupleNums=(1, 2),
+                arrayTuple=[(10, 20), (30, 40)],
+                arrayTuple2=[('a', [1]), ('b', [3, 4])],
+                arrayTuple3=[(('a', True), [1]), (('b', False), [3, 4])],
+            )
+            scon = con.with_globals(**globs)
+            chunks = [f'{name} := global {name}' for name in globs]
+            res = await scon.query_single(f'select {{ {", ".join(chunks)} }}')
+            dres = dataclasses.asdict(res)
+            del dres['id']
+            self.assertEqual(dres, globs)
+        finally:
+            await con.aclose()
diff --git a/tests/test_protocol.py b/tests/test_protocol.py
index edcdb4db00d..60c745afb5d 100644
--- a/tests/test_protocol.py
+++ b/tests/test_protocol.py
@@ -560,6 +560,37 @@ async def test_proto_execute_bad_array_03(self):
             message='unsupported array dimensions'
         )
 
+    async def test_proto_global_bad_array(self):
+        await self.con.connect()
+
+        # Create a global
+        await self._execute('CREATE GLOBAL array_glob -> array<str>')
+        sdd1 = await self.con.recv_match(protocol.StateDataDescription)
+        await self.con.recv_match(protocol.CommandComplete)
+        await self.con.recv_match(protocol.ReadyForCommand)
+        self.assertNotEqual(sdd1.typedesc_id, b'\0' * 16)
+
+        # Set an array in the state
+        await self._execute('SET GLOBAL array_glob := ["AAAA", "", "CCCC"]')
+        cc1 = await self.con.recv_match(
+            protocol.CommandComplete,
+            state_typedesc_id=sdd1.typedesc_id,
+        )
+        await self.con.recv_match(protocol.ReadyForCommand)
+
+        # Blow away the empty second string from the encoding and
+        # replace it with NULL
+        cc1.state_data = cc1.state_data.replace(
+            b'AAAA' + b'\x00' * 4, b'AAAA' + b'\xff' * 4
+        )
+
+        # Verify the state is set
+        await self._execute('SELECT 1', data=True, cc=cc1)
+        await self.con.recv_match(
+            protocol.ErrorResponse,
+            message='invalid NULL'
+        )
+
 
 class TestServerCancellation(tb.TestCase):
     @contextlib.asynccontextmanager


Code review comments:
diff_hunk: @@ -1090,6 +1090,21 @@ def _parse_range_descriptor(
     return RangeDesc(tid=tid, inner=subtype)
 
 
+def _make_global_rep(typ: s_types.Type, ctx: Context) -> object:
+    if isinstance(typ, s_types.Tuple):
+        subtyps = typ.get_subtypes(ctx.schema)
+        return (
+            1,
path: edb/server/compiler/sertypes.py
text: @user1:
Can you add an enum?