Skip to content

Commit

Permalink
[4.x] Fix spurious delete/create of constraints with arguments in mig…
Browse files Browse the repository at this point in the history
…rations (#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.
  • Loading branch information
msullivan authored Jan 19, 2024
1 parent 080c7ce commit 490d460
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 4 deletions.
2 changes: 2 additions & 0 deletions edb/pgsql/patches.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
])
34 changes: 33 additions & 1 deletion edb/schema/reflection/structure.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions edb/schema/reflection/writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(<json>${var_n}) }}
FOR v IN {{ enumerate(json_array_unpack(<json>${var_n})) }}
UNION (
SELECT {target.get_name(schema)} {{
@value := <str>v[1]
@index := v.0,
@value := <str>v.1[1],
}}
FILTER .id = <uuid>v[0]
FILTER .id = <uuid>v.1[0]
)
))"""
args = props.get('args', [])
Expand Down
20 changes: 20 additions & 0 deletions tests/test_edgeql_ddl.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 490d460

Please sign in to comment.