Repository: geldata/gel
Pull request: add header to disable implicit ids
fixes https://github.com/edgedb/edgedb/issues/2430

Problem statement:
query header to toggle implicit object id
A query header that toggles implicit object ids should be added. This would make bindings in statically typed languages more flexible since they would not have to require that users provide an id field to decode the value into.

Hints from issue discussion:
Feel free to work on this Frederick!

Commit message: add header to disable implicit ids

https://github.com/edgedb/edgedb/issues/2430

Patch to review:
diff --git a/docs/internals/protocol/messages.rst b/docs/internals/protocol/messages.rst
index b74abdb24c3..8963f2e483f 100644
--- a/docs/internals/protocol/messages.rst
+++ b/docs/internals/protocol/messages.rst
@@ -280,6 +280,10 @@ Known headers:
 * 0xFF04 ``ALLOW_CAPABILITIES``: ``uint64`` -- optional bitmask of
   capabilities allowed for this query.  See RFC1004_ for more information.
 
+* 0xFF05 ``IMPLICIT_SHAPEIDS`` -- "true" by default.
+  If set to "false" returned objects will not have an implicit ``id`` property
+  i.e. query shapes will have to explicitly list id properties.
+
 .. eql:struct:: edb.testbase.protocol.Cardinality
 
 
@@ -446,6 +450,10 @@ Known headers:
 * 0xFF04 ``ALLOW_CAPABILITIES``: ``uint64`` -- optional bitmask of
   capabilities allowed for this query.  See RFC1004_ for more information.
 
+* 0xFF05 ``IMPLICIT_SHAPEIDS`` -- "true" by default.
+  If set to "false" returned objects will not have an implicit ``id`` property
+  i.e. query shapes will have to explicitly list id properties.
+
 .. _ref_protocol_msg_data:
 
 Data
diff --git a/edb/server/compiler/compiler.py b/edb/server/compiler/compiler.py
index 55d481789fb..f08d0ba0959 100644
--- a/edb/server/compiler/compiler.py
+++ b/edb/server/compiler/compiler.py
@@ -104,6 +104,7 @@ class CompileContext:
     implicit_limit: int = 0
     inline_typeids: bool = False
     inline_typenames: bool = False
+    inline_shapeids: bool = True
     schema_object_ids: Optional[Mapping[s_name.Name, uuid.UUID]] = None
     source: Optional[edgeql.Source] = None
     backend_runtime_params: Any = (
@@ -549,10 +550,13 @@ def _compile_ql_query(
 
         single_stmt_mode = ctx.stmt_mode is enums.CompileStatementMode.SINGLE
 
-        can_have_implicit_fields = (
-            native_out_format and
-            single_stmt_mode
-        )
+        if not ctx.inline_shapeids:
+            can_have_implicit_fields = False
+        else:
+            can_have_implicit_fields = (
+                native_out_format and
+                single_stmt_mode
+            )
 
         disable_constant_folding = self.get_config_val(
             ctx,
@@ -1929,6 +1933,7 @@ def compile(
         inline_typeids: bool,
         inline_typenames: bool,
         stmt_mode: enums.CompileStatementMode,
+        inline_shapeids: bool=True,
         json_parameters: bool=False,
     ) -> Tuple[List[dbstate.QueryUnit],
                Optional[dbstate.CompilerConnectionState]]:
@@ -1965,6 +1970,7 @@ def compile(
             implicit_limit=implicit_limit,
             inline_typeids=inline_typeids,
             inline_typenames=inline_typenames,
+            inline_shapeids=inline_shapeids,
             stmt_mode=enums.CompileStatementMode(stmt_mode),
             json_parameters=json_parameters,
             source=source,
@@ -1993,6 +1999,7 @@ def compile_in_tx(
         inline_typeids: bool,
         inline_typenames: bool,
         stmt_mode: enums.CompileStatementMode,
+        inline_shapeids: bool = True,
     ) -> Tuple[List[dbstate.QueryUnit], dbstate.CompilerConnectionState]:
         state.sync_tx(txid)
 
@@ -2003,6 +2010,7 @@ def compile_in_tx(
             implicit_limit=implicit_limit,
             inline_typeids=inline_typeids,
             inline_typenames=inline_typenames,
+            inline_shapeids=inline_shapeids,
             stmt_mode=enums.CompileStatementMode(stmt_mode),
             source=source,
         )
diff --git a/edb/server/protocol/binary.pxd b/edb/server/protocol/binary.pxd
index f6744235072..bf82a9727ba 100644
--- a/edb/server/protocol/binary.pxd
+++ b/edb/server/protocol/binary.pxd
@@ -58,6 +58,7 @@ cdef class QueryRequestInfo:
     cdef public int implicit_limit
     cdef public bint inline_typeids
     cdef public bint inline_typenames
+    cdef public bint inline_shapeids
     cdef public uint64_t allow_capabilities
 
     cdef int cached_hash
diff --git a/edb/server/protocol/binary.pyx b/edb/server/protocol/binary.pyx
index 7ee6ba22a94..2a2fbbf398e 100644
--- a/edb/server/protocol/binary.pyx
+++ b/edb/server/protocol/binary.pyx
@@ -101,6 +101,7 @@ DEF QUERY_HEADER_IMPLICIT_LIMIT = 0xFF01
 DEF QUERY_HEADER_IMPLICIT_TYPENAMES = 0xFF02
 DEF QUERY_HEADER_IMPLICIT_TYPEIDS = 0xFF03
 DEF QUERY_HEADER_ALLOW_CAPABILITIES = 0xFF04
+DEF QUERY_HEADER_IMPLICIT_SHAPEIDS = 0xFF05
 
 DEF SERVER_HEADER_CAPABILITIES = 0x1001
 
@@ -140,6 +141,7 @@ cdef class QueryRequestInfo:
         implicit_limit: int = 0,
         inline_typeids: bint = False,
         inline_typenames: bint = False,
+        inline_shapeids: bint = True,
         allow_capabilities: uint64_t = ALL_CAPABILITIES,
     ):
         self.source = source
@@ -148,6 +150,7 @@ cdef class QueryRequestInfo:
         self.implicit_limit = implicit_limit
         self.inline_typeids = inline_typeids
         self.inline_typenames = inline_typenames
+        self.inline_shapeids = inline_shapeids
         self.allow_capabilities = allow_capabilities
 
         self.cached_hash = hash((
@@ -157,6 +160,7 @@ cdef class QueryRequestInfo:
             self.implicit_limit,
             self.inline_typeids,
             self.inline_typenames,
+            self.inline_shapeids,
         ))
 
     def __hash__(self):
@@ -169,7 +173,8 @@ cdef class QueryRequestInfo:
             self.expect_one == other.expect_one and
             self.implicit_limit == other.implicit_limit and
             self.inline_typeids == other.inline_typeids and
-            self.inline_typenames == other.inline_typenames
+            self.inline_typenames == other.inline_typenames and
+            self.inline_shapeids == other.inline_shapeids
         )
 
 
@@ -773,6 +778,7 @@ cdef class EdgeConnection:
                 query_req.inline_typeids,
                 query_req.inline_typenames,
                 'single',
+                query_req.inline_shapeids,
             )
         else:
             units, self.last_state = await compiler_pool.compile(
@@ -791,6 +797,7 @@ cdef class EdgeConnection:
                 query_req.inline_typeids,
                 query_req.inline_typenames,
                 'single',
+                query_req.inline_shapeids,
             )
         return units
 
@@ -1171,6 +1178,7 @@ cdef class EdgeConnection:
             bint inline_typeids = self.protocol_version <= (0, 8)
             uint64_t allow_capabilities = ALL_CAPABILITIES
             bint inline_typenames = False
+            bint inline_shapeids = True
             bytes stmt_name = b''
 
         headers = self.parse_headers()
@@ -1187,6 +1195,9 @@ cdef class EdgeConnection:
                 elif k == QUERY_HEADER_ALLOW_CAPABILITIES:
                     self.version_check("ALLOW_CAPABILITIES header", (0, 9))
                     allow_capabilities = parse_capabilities_header(v)
+                elif k == QUERY_HEADER_IMPLICIT_SHAPEIDS:
+                    self.version_check("IMPLICIT_SHAPEIDS header", (0, 9))
+                    inline_shapeids = parse_boolean(v, "IMPLICIT_SHAPEIDS")
                 else:
                     raise errors.BinaryProtocolError(
                         f'unexpected message header: {k}'
@@ -1216,6 +1227,7 @@ cdef class EdgeConnection:
             implicit_limit=implicit_limit,
             inline_typeids=inline_typeids,
             inline_typenames=inline_typenames,
+            inline_shapeids=inline_shapeids,
             allow_capabilities=allow_capabilities,
         )
 


Code review comments:
diff_hunk: @@ -104,6 +104,7 @@ class CompileContext:
     implicit_limit: int = 0
     inline_typeids: bool = False
     inline_typenames: bool = False
+    inline_shapeids: bool = True
path: edb/server/compiler/compiler.py
text: @user1:
```suggestion
    inline_objectids: bool = True
```

@author:
fixed

diff_hunk: @@ -549,10 +550,13 @@ def _compile_ql_query(
 
         single_stmt_mode = ctx.stmt_mode is enums.CompileStatementMode.SINGLE
 
-        can_have_implicit_fields = (
-            native_out_format and
-            single_stmt_mode
-        )
+        if not ctx.inline_shapeids:
+            can_have_implicit_fields = False
path: edb/server/compiler/compiler.py
text: @user1:
Why are object ids special here and not like typeid?

@author:
I'm not sure exactly what you have in mind. I didn't want to break backward compatibility. Should this behavior be controlled entirely by the header instead of the combination of the header, protocol, and statement mode?

@user2:
Well, there can be a case of the user does not want to inline IDs but wants typenames. Your change disables typenames and typeids if object IDs are omitted. 

@author:
oh got it!

@author:
changed