From 490d460a5e75d78a00da116703698041d4e9ce8f Mon Sep 17 00:00:00 2001 From: "Michael J. Sullivan" Date: Fri, 19 Jan 2024 14:49:54 -0800 Subject: [PATCH] [4.x] Fix spurious delete/create of constraints with arguments in migrations (#6712) The bug here was that `@index` was not populated in the `params` link for concrete constraints by the reflection writer, and so when we reloaded the schema from the database, it came back in an arbitrary order. This bug has been present for a *long* time, as far as I can tell, but we tended to get "lucky" with the order pg returned the params. Fixes #6699. Unfortunately fixing newly created constraints doesn't help us on 4.x where @index may already be missing. Fix this during introspection by falling back to the nearest abstract ancestor, which will have a correct @index. I've only been able to reproduce this issue with fairly large schemas, so there's no test attached, but I tested with: https://github.com/SeedCompany/cord-api-v3/tree/9028ad7f05d51e4b3b07f5154cb0ed62ad43dc7d/dbschema which is from the originally reported issue. --- edb/pgsql/patches.py | 2 ++ edb/schema/reflection/structure.py | 34 +++++++++++++++++++++++++++++- edb/schema/reflection/writer.py | 7 +++--- tests/test_edgeql_ddl.py | 20 ++++++++++++++++++ 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/edb/pgsql/patches.py b/edb/pgsql/patches.py index d9115bd86a6..4307e8cdda0 100644 --- a/edb/pgsql/patches.py +++ b/edb/pgsql/patches.py @@ -102,4 +102,6 @@ def _setup_patches(patches: list[tuple[str, str]]) -> list[tuple[str, str]]: '''), ('edgeql+schema', ''), # pg_attribute, pg_attribute_ext, edgedbsql.columns ('edgeql+schema', ''), # __type__ + # === 4.6 + ('edgeql+schema+globalonly', ''), # recreate introspection query ]) diff --git a/edb/schema/reflection/structure.py b/edb/schema/reflection/structure.py index a21a1edc277..12ee09a07b4 100644 --- a/edb/schema/reflection/structure.py +++ b/edb/schema/reflection/structure.py @@ -30,6 +30,7 @@ from edb.edgeql import qltypes +from edb.schema import constraints as s_constr from edb.schema import ddl as s_ddl from edb.schema import delta as sd from edb.schema import expr as s_expr @@ -529,7 +530,38 @@ def generate_structure( f'{read_ptr}: {{name, value := .{proxy_link}.id}}' ) - if ptr.issubclass(schema, ordered_link): + # HACK: Pre 4.6, we did not correctly populate the + # @index linkprop for concrete constraint params, + # which can cause migration trouble if they happen to + # get read back in the wrong order. We fixed it + # forward, but we need to be able to handle cases + # where it is still missing, so we read the order out + # of the nearest abstract *ancestor*, since abstract + # constraint @indexs always got populated properly. + if ( + sfn == 'params' + and issubclass(py_cls, s_constr.Constraint) + ): + read_ptr = ''' + params := (select .params { + id, + baseindex := assert_single(( + # fetch the first non-concrete ancestor + with anc := ( + select schema::Constraint.ancestors[ + is schema::Constraint] + filter .abstract order by @index limit 1 + ), + # ... and pull out the index of the matching param + select anc { + params + filter .name = schema::Constraint.params.name + }.params@index + )) + } order by @index ?? .baseindex) { id } + ''' + + elif ptr.issubclass(schema, ordered_link): read_ptr = f'{read_ptr} ORDER BY @index' read_shape.append(read_ptr) diff --git a/edb/schema/reflection/writer.py b/edb/schema/reflection/writer.py index 30c3e83f5f0..fb80509ddde 100644 --- a/edb/schema/reflection/writer.py +++ b/edb/schema/reflection/writer.py @@ -299,12 +299,13 @@ def _build_object_mutation_shape( # an ObjectKeyDict collection that allow associating objects # with arbitrary values (a transposed ObjectDict). target_expr = f"""assert_distinct(( - FOR v IN {{ json_array_unpack(${var_n}) }} + FOR v IN {{ enumerate(json_array_unpack(${var_n})) }} UNION ( SELECT {target.get_name(schema)} {{ - @value := v[1] + @index := v.0, + @value := v.1[1], }} - FILTER .id = v[0] + FILTER .id = v.1[0] ) ))""" args = props.get('args', []) diff --git a/tests/test_edgeql_ddl.py b/tests/test_edgeql_ddl.py index 0e2577e95ae..cfd740616b5 100644 --- a/tests/test_edgeql_ddl.py +++ b/tests/test_edgeql_ddl.py @@ -10190,6 +10190,26 @@ async def test_edgeql_ddl_constraint_09(self): CREATE TYPE Comment EXTENDING Text; """) + await self.assert_query_result( + """ + select schema::Constraint { + name, params: {name, @index} order by @index + } + filter .name = 'std::max_len_value' + and .subject.name = 'body' + and .subject[is schema::Pointer].source.name ='default::Text'; + """, + [ + { + "name": "std::max_len_value", + "params": [ + {"name": "__subject__", "@index": 0}, + {"name": "max", "@index": 1} + ] + } + ], + ) + await self.con.execute(""" ALTER TYPE Text ALTER PROPERTY body