Skip to content

Commit

Permalink
Produce proper error messages when defining a field twice
Browse files Browse the repository at this point in the history
Fixes #5725.
  • Loading branch information
msullivan committed Jul 13, 2023
1 parent 5193609 commit 110ed67
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 13 deletions.
50 changes: 37 additions & 13 deletions edb/edgeql/declarative.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ def ensure_pointer_kind(

def get_verbosename_from_fqname(
fq_name: s_name.QualName,
ctx: DepTraceContext,
ctx: DepTraceContext | LayoutTraceContext,
) -> str:
traceobj = ctx.objects[fq_name]
assert traceobj is not None
Expand All @@ -246,13 +246,15 @@ def get_verbosename_from_fqname(
elif isinstance(traceobj, qltracer.ScalarType):
clsname = 'scalar'
elif isinstance(traceobj, qltracer.Function):
node = ctx.ddlgraph[fq_name].item
assert isinstance(node, qlast.FunctionCommand)
params = ','.join(
qlcodegen.generate_source(param, sdlmode=True)
for param in node.params
)
name = f"{str(fq_name).split('@@', 1)[0]}({params})"
name = str(fq_name).split('@@', 1)[0]
if isinstance(ctx, DepTraceContext):
node = ctx.ddlgraph[fq_name].item
assert isinstance(node, qlast.FunctionCommand)
params = ','.join(
qlcodegen.generate_source(param, sdlmode=True)
for param in node.params
)
name = f"{name}({params})"
elif isinstance(traceobj, qltracer.Pointer):
ofobj, name = str(fq_name).split('@', 1)
ofobj = f" of object type '{ofobj}'"
Expand All @@ -273,6 +275,11 @@ def get_verbosename_from_fqname(
if name == str(s_indexes.DEFAULT_INDEX):
name = ''
ofobj = f" of object type '{ofobj}'"
elif isinstance(traceobj, qltracer.Field):
clsname = 'field'
obj, name = fq_name.name.rsplit('@', 1)
ofobj = ' of ' + get_verbosename_from_fqname(
s_name.QualName(fq_name.module, obj), ctx)

if name:
return f"{clsname} '{name}'{ofobj}"
Expand Down Expand Up @@ -749,6 +756,22 @@ def _trace_item_layout(
)
ctx.objects[idx_name] = qltracer.ConcreteIndex(idx_name)

elif isinstance(decl, qlast.SetField):
field_name = s_name.QualName(
module=fq_name.module,
name=f'{fq_name.name}@{decl.name}',
)

# Trivial fields don't get added to the ddlgraph, which is
# where duplication checks are normally done, so do the
# check here instead.
if field_name in ctx.objects:
vn = get_verbosename_from_fqname(field_name, ctx)
msg = f'{vn} was already declared'
raise errors.InvalidDefinitionError(msg, context=decl.context)

ctx.objects[field_name] = qltracer.Field(field_name)


RECURSION_GUARD: Set[s_name.QualName] = set()

Expand Down Expand Up @@ -1079,9 +1102,7 @@ def _register_item(
if fq_name in ctx.ddlgraph:
vn = get_verbosename_from_fqname(fq_name, ctx)
msg = f'{vn} was already declared'

raise errors.InvalidDefinitionError(
msg, context=decl.context)
raise errors.InvalidDefinitionError(msg, context=decl.context)

if deps:
deps = set(deps)
Expand Down Expand Up @@ -1328,8 +1349,11 @@ def _get_pointer_deps(
# is is important for properly doing cardinality inference
# on expressions involving it.
# PERF: We should avoid actually searching all the objects.
for propname in ctx.objects:
if str(propname).startswith(str(pointer) + '@'):
for propname, prop in ctx.objects.items():
if (
str(propname).startswith(str(pointer) + '@')
and not isinstance(prop, qltracer.Field)
):
result.add(propname)

return result
Expand Down
4 changes: 4 additions & 0 deletions edb/edgeql/tracer.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ class ConcreteIndex(NamedObject):
pass


class Field(NamedObject):
pass


class Type(NamedObject):
def is_scalar(self) -> bool:
return False
Expand Down
43 changes: 43 additions & 0 deletions tests/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,49 @@ def test_schema_module_reserved_01(self):
}
"""

@tb.must_fail(
errors.InvalidDefinitionError,
"field 'default' .*was already declared"
)
def test_schema_field_dupe_01(self):
"""
type SimpleNumbers {
property bar: str;
property foo: str {
default := '';
default := '';
}
}
"""

@tb.must_fail(
errors.InvalidDefinitionError,
"field 'default' .*was already declared"
)
def test_schema_field_dupe_02(self):
"""
type SimpleNumbers {
property bar: str;
property foo: str {
default := .bar;
default := .bar;
}
}
"""

@tb.must_fail(
errors.InvalidDefinitionError,
"field 'foo' .*was already declared"
)
def test_schema_field_dupe_03(self):
"""
type SimpleNumbers {
bar: str;
foo := .bar ++ "!";
foo := .bar ++ "!";
}
"""

@tb.must_fail(
errors.InvalidFunctionDefinitionError,
r"cannot create the `test::foo\(VARIADIC bar: "
Expand Down

0 comments on commit 110ed67

Please sign in to comment.