Repository: geldata/gel
Pull request: Fix properties of type array<uuid> in migrations
The issue is that the type array<uuid> was in an inconsistent
state. The shadow reflection schema uses array<uuid>, so it is always
in the reflected schema, but since it is created by the reflection
schema code and does not appear in the stdlib, it didn't appear in the
stdschema.

Fix this by adding any collection types that appear in the reflschema
into the stdschema.

Also to avoid related inconsistencies, skip loading objects that
appear in the stdschema when processing the json from the reflection
query. The query filters out qualified objects from the std schema but
not "global" ones, so collections and some other things were being
duplicated between _base_schema and _top_schema.

Fixes #4107.

Problem statement:
Properties or type `array<uuid>` are broken
```
edgedb> start migration to {
....... module default {
....... type Foo {
....... property myArray -> array<uuid>;
....... }; }; };
OK: START MIGRATION
edgedb[tx]> populate migration;
edgedb error: SchemaError: Array ('d2a2fc36-29e2-20f2-9861-9f2633371c73') is already present in the schema <FlatSchema gen:1 at 0x7fd2da28fee0>
```

Hints from issue discussion:
Hah, but it works if you create it with ddl. Amazing.

Commit message: Fix properties of type array<uuid> in migrations

The issue is that the type array<uuid> was in an inconsistent
state. The shadow reflection schema uses array<uuid>, so it is always
in the reflected schema, but since it is created by the reflection
schema code and does not appear in the stdlib, it didn't appear in the
stdschema.

Fix this by adding any collection types that appear in the reflschema
into the stdschema.

Also to avoid related inconsistencies, skip loading objects that
appear in the stdschema when processing the json from the reflection
query. The query filters out qualified objects from the std schema but
not "global" ones, so collections and some other things were being
duplicated between _base_schema and _top_schema.

Fixes #4107.

Patch to review:
diff --git a/edb/buildmeta.py b/edb/buildmeta.py
index 415cb22ac32..5df785bda53 100644
--- a/edb/buildmeta.py
+++ b/edb/buildmeta.py
@@ -44,7 +44,7 @@
 
 
 # Increment this whenever the database layout or stdlib changes.
-EDGEDB_CATALOG_VERSION = 2022_07_15_00_00
+EDGEDB_CATALOG_VERSION = 2022_07_20_00_00
 EDGEDB_MAJOR_VERSION = 3
 
 
diff --git a/edb/schema/reflection/reader.py b/edb/schema/reflection/reader.py
index 1209e5f7946..a529e6489df 100644
--- a/edb/schema/reflection/reader.py
+++ b/edb/schema/reflection/reader.py
@@ -100,6 +100,9 @@ def parse_into(
         name = s_name.name_from_string(entry['name__internal'])
         layout = schema_class_layout[mcls]
 
+        if base_schema.has_object(objid) and str(name) != '__schema_version__':
+            continue
+
         if isinstance(obj, s_obj.QualifiedObject):
             name_to_id[name] = objid
         else:
diff --git a/edb/server/bootstrap.py b/edb/server/bootstrap.py
index f348df72041..d77d5a15128 100644
--- a/edb/server/bootstrap.py
+++ b/edb/server/bootstrap.py
@@ -49,6 +49,7 @@
 from edb.schema import reflection as s_refl
 from edb.schema import schema as s_schema
 from edb.schema import std as s_std
+from edb.schema import types as s_types
 
 from edb.server import args as edbargs
 from edb.server import config
@@ -490,7 +491,7 @@ async def _make_stdlib(
     testmode: bool,
     global_ids: Mapping[str, uuid.UUID],
 ) -> StdlibBits:
-    schema = s_schema.ChainedSchema(
+    schema: s_schema.Schema = s_schema.ChainedSchema(
         s_schema.FlatSchema(),
         s_schema.FlatSchema(),
         s_schema.FlatSchema(),
@@ -552,6 +553,16 @@ async def _make_stdlib(
     reflschema, reflplan = _process_delta(
         ctx, reflection.intro_schema_delta, schema)
 
+    # Any collection types that made it into reflschema need to get
+    # to get pulled back into the stdschema, or else they will be in
+    # an inconsistent state.
+    for obj in reflschema.get_objects(type=s_types.Collection):
+        if not schema.has_object(obj.id):
+            delta = sd.DeltaRoot()
+            delta.add(obj.as_shell(reflschema).as_create_delta(reflschema))
+            schema = delta.apply(schema, sd.CommandContext())
+    assert isinstance(schema, s_schema.ChainedSchema)
+
     assert current_block is not None
     reflplan.generate(current_block)
     subblock = current_block.add_block()
diff --git a/tests/test_edgeql_data_migration.py b/tests/test_edgeql_data_migration.py
index 1651a4a890d..590339a3f2d 100644
--- a/tests/test_edgeql_data_migration.py
+++ b/tests/test_edgeql_data_migration.py
@@ -10858,6 +10858,13 @@ async def test_edgeql_migration_fiddly_delete_01(self):
             }
         ''')
 
+    async def test_edgeql_migration_uuid_array_01(self):
+        await self.migrate(r'''
+            type Foo {
+                property x -> array<uuid>;
+            }
+        ''')
+
 
 class TestEdgeQLDataMigrationNonisolated(EdgeQLDataMigrationTestCase):
     TRANSACTION_ISOLATION = False


Code review comments:
diff_hunk: @@ -100,6 +100,9 @@ def parse_into(
         name = s_name.name_from_string(entry['name__internal'])
         layout = schema_class_layout[mcls]
 
+        if base_schema.has_object(objid) and str(name) != '__schema_version__':
path: edb/schema/reflection/reader.py
text: @user1:
Why `__schema_version` specifically?  Is there a more general rule?

@author:
I made it handle the Version types more generally. I don't know if there's something more general than that

@author:
But it is because `__schema_version__` is special and get updated despite also being in the base schema