Skip to content

Commit 44661ad

Browse files
fix: raise when ignoring case creates ambiguous fields (#1798)
# Context follow-up to #1797, which handles duplicate labels when ignoring case. This PR handles ambiguous fields under "ignore-case=True", which were silently ignored during validation. # Fix Ambiguous fields under "ignore-case" raise now an error : there is no obvious default behavior and the schema schould *not* be used with "ignore-case=True" in this circumstances.
1 parent 9d53938 commit 44661ad

4 files changed

Lines changed: 82 additions & 0 deletions

File tree

frictionless/resource/__spec__/test_validate_schema.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,34 @@ def test_resource_validate_duplicate_labels_ignoring_header_case():
333333
]
334334

335335

336+
def test_resource_validate_fields_only_distinguished_by_case_are_rejected():
337+
# The schema is valid: "a" and "A" are distinct field names. But with
338+
# `header_case` off they collapse onto the same key.
339+
schema = Schema.from_descriptor(
340+
{
341+
"fields": [
342+
{"name": "a", "type": "string"},
343+
{"name": "A", "type": "integer"},
344+
],
345+
"fieldsMatch": "partial",
346+
}
347+
)
348+
resource = TableResource(
349+
[["A"], ["x"]],
350+
schema=schema,
351+
dialect=Dialect(header_case=False),
352+
)
353+
report = resource.validate()
354+
assert report.flatten(["type", "note"]) == [
355+
[
356+
"metadata-error",
357+
'matching fields by name ("fieldsMatch": "partial") is ambiguous: '
358+
'fields "a", "A" differ only by case, which "header_case" is set '
359+
"to ignore",
360+
],
361+
]
362+
363+
336364
def test_resource_validate_less_actual_fields_with_required_constraint_issue_950():
337365
schema = Schema.describe("data/table.csv")
338366
schema.add_field(fields.AnyField(name="bad", constraints={"required": True}))

frictionless/table/__spec__/test_header.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,35 @@ def test_get_expected_fields_raises_on_duplicate_labels(fields_match):
153153
header.get_expected_fields()
154154

155155

156+
@pytest.mark.parametrize("fields_match", NAME_MATCHED)
157+
def test_get_expected_fields_raises_on_fields_colliding_under_ignore_case(fields_match):
158+
header = _make_header(["a"], ["a", "A"], fields_match=fields_match, ignore_case=True)
159+
with pytest.raises(frictionless.FrictionlessException) as excinfo:
160+
header.get_expected_fields()
161+
assert excinfo.value.error.type == "metadata-error"
162+
163+
164+
def test_get_expected_fields_colliding_fields_error_names_the_culprits():
165+
header = _make_header(["a"], ["a", "A"], fields_match="partial", ignore_case=True)
166+
with pytest.raises(frictionless.FrictionlessException) as excinfo:
167+
header.get_expected_fields()
168+
note = excinfo.value.error.note
169+
assert "header_case" in note
170+
assert '"a"' in note and '"A"' in note
171+
172+
173+
def test_get_expected_fields_exact_tolerates_fields_colliding_under_ignore_case():
174+
# Mapping is positional, so the fields are never told apart by name.
175+
header = _make_header(["a", "A"], ["a", "A"], fields_match="exact", ignore_case=True)
176+
assert [f.name for f in header.get_expected_fields()] == ["a", "A"]
177+
178+
179+
def test_get_expected_fields_tolerates_case_distinct_fields_when_case_matters():
180+
# Without `ignore_case`, `a` and `A` are simply two distinct fields.
181+
header = _make_header(["a", "A"], ["a", "A"], fields_match="partial")
182+
assert [f.name for f in header.get_expected_fields()] == ["a", "A"]
183+
184+
156185
def test_get_expected_fields_exact_tolerates_duplicate_labels():
157186
# Mapping is positional, so duplicates are unambiguous here; they are
158187
# reported as a `duplicate-label` error rather than raising.

frictionless/table/header.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,17 @@ def get_expected_fields(self) -> List[Field]:
155155
self.__expected_fields = self.__fields
156156
return self.__expected_fields
157157

158+
# ignore_case can make fields ambiguous as their keys are identical,
159+
# e.g. "A" and "a"
160+
for group in self.__matching.ambiguous_fields:
161+
names = ", ".join(f'"{field.name}"' for field in group)
162+
note = (
163+
f'matching fields by name ("fieldsMatch": "{self.__fields_match}") '
164+
f"is ambiguous: fields {names} differ only by case, which "
165+
'"header_case" is set to ignore'
166+
)
167+
raise FrictionlessException(errors.MetadataError(note=note))
168+
158169
if self.__matching.has_duplicate_labels:
159170
note = (
160171
f'matching fields by name ("fieldsMatch": "{self.__fields_match}") '

frictionless/table/label_matching.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,20 @@ def unmatched_fields(self) -> List[Field]:
4848
if self.__normalize(field.name) not in matched
4949
]
5050

51+
@property
52+
def ambiguous_fields(self) -> List[List[Field]]:
53+
"""Groups of fields that normalization merges into a single key
54+
55+
Fields sharing the very same name make the schema itself invalid, which is caught
56+
before a header is ever built.
57+
"""
58+
groups: Dict[str, List[Field]] = {}
59+
for field in self.__fields:
60+
groups.setdefault(self.__normalize(field.name), []).append(field)
61+
return [
62+
group for group in groups.values() if len({field.name for field in group}) > 1
63+
]
64+
5165
@property
5266
def has_duplicate_labels(self) -> bool:
5367
"""Whether two labels match the same field, which makes the mapping ambiguous

0 commit comments

Comments
 (0)