Skip to content

Commit e056ec3

Browse files
m1n0claude
andauthored
fix(check-collections): qualify check paths with collection kind to ensure uniqueness (#2792)
* refactor(check): rename path->relative_path, full_path->check_path * fix(test): complete path->relative_path / full_path->check_path rename across unit tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(check-selector): match --check-paths against full check_path (fixes data-standard partial runs) * feat(check-selector): add source=, collection=/standard=, relative_path= filter fields Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(check-path): emit option-3 format type.id:relative for non-contract collections Non-contract check collections (e.g. data standards) now emit checkPath as "{wire_source}.{collection_id}:{relative_path}" (option 3) instead of the old "{collection_id}.{relative_path}" format. Contracts stay bare (checkPath == relative_path). Adds a delimiter-safety guard in base.py verify() that rejects collection_id values containing '.' or ':' (reserved check-path delimiters). Updates all stub check_path values in selector and alignment tests to the option-3 format. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(check-path): migrate data-standard stubs to option-3 format; doc + name accuracy * test+guard: self-updating supported-fields test, wire_source delimiter guard, negative coverage * fix(check-collections): surface isolated parse/verify errors instead of failing silently A data standard with an invalid `name:` (e.g. `test.standard`) exited 3 while printing nothing at all. The name guard was firing correctly — the message just never reached the user. `build_error_result` is the isolation boundary for `abort_on_first_error=False` callers: it set `status=ERROR` (driving exit 3), stored the exception on `result.error`, set `log_records=None`, and never logged. The only reader of `result.error` is the Cloud `mark_scan_as_failed` path, gated on a scan_id — so a local run surfaced nothing. Not specific to the dot: every construct/verify-time error was silent this way. It hits data standards always (api.py pins `abort_on_first_error=False`) and any multi-file contract run (`abort_on_first_error=(len(sources) == 1)`). Only the single-contract path re-raised, which is why it went unnoticed. Log the exception where it is isolated, captured into the placeholder's own `Logs` so it is attributed to the failing file rather than to whichever sibling happens to be the active capture target (constructing a Logs leaves it active, so after the construct phase that is the last-constructed collection — a bare emit would file the error under an innocent sibling and ship it in that sibling's Cloud log payload). `log_records` now carries the error line, so an errored placeholder is self-diagnosing instead of an empty payload. Before: `Exiting with code 3` and nothing else. After: `🚨 data-standard 'name' must not contain '.' or ':': got 'test.standard', in <file>` Tests: two regression tests pinning that isolated parse and verify failures are logged. Full unit suite green (994 passed, 2 skipped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(check-selector): black formatting for SUPPORTED_FIELDS CI runs pre-commit --all-files; this file was left unformatted by an earlier commit on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(check-path): correct comments left describing the pre-option-3 wire format Three comments still documented the old "{collection_id}.{relative_path}" prefix and routing via firstSegmentOf(checkPath) — a backend function this change deletes. They now describe the option-3 form "{wire_source}.{collection_id}:{relative_path}" and how the backend parses it. Comments only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent ebd2cdd commit e056ec3

13 files changed

Lines changed: 328 additions & 184 deletions

soda-core/src/soda_core/check_collections/base.py

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
from soda_core.common.env_config_helper import EnvConfigHelper
2727
from soda_core.common.exceptions import SodaCoreException, get_exception_stacktrace
2828
from soda_core.common.logging_constants import Emoticons, ExtraKeys, soda_logger
29-
from soda_core.common.logs import Location, Logs
29+
from soda_core.common.logs import Location, Logs, preserve_active_logs
3030
from soda_core.common.metadata_types import SamplerType
3131
from soda_core.common.soda_cloud_converter import map_sampler_type_from_dto
3232
from soda_core.common.soda_cloud_dto import DatasetConfigurationDTO
@@ -801,6 +801,16 @@ def verify(self) -> CheckCollectionResult:
801801
f"collection_id (used to prefix checkPath for backend routing)."
802802
)
803803

804+
if self.collection_id and (("." in self.collection_id) or (":" in self.collection_id)):
805+
raise ValueError(
806+
f"collection_id {self.collection_id!r} must not contain '.' or ':' (reserved check-path delimiters)"
807+
)
808+
809+
if self.wire_source and (("." in self.wire_source) or (":" in self.wire_source)):
810+
raise ValueError(
811+
f"wire_source {self.wire_source!r} must not contain '.' or ':' (reserved check-path delimiters)"
812+
)
813+
804814
if self.data_source_impl and self.soda_config.is_running_on_runner:
805815
self.data_source_impl.switch_warehouse(self.compute_warehouse, contract_impl=self)
806816
data_source: Optional[DataSource] = None
@@ -846,7 +856,7 @@ def verify(self) -> CheckCollectionResult:
846856
# Evaluate the checks
847857
for check_impl in self.all_check_impls:
848858
if check_impl.skip:
849-
logger.info(f"Skipping evaluation of check at path '{check_impl.path}'")
859+
logger.info(f"Skipping evaluation of check at path '{check_impl.relative_path}'")
850860
check_result: CheckResult = CheckResult(
851861
check=check_impl._build_check_info(), outcome=CheckOutcome.EXCLUDED
852862
)
@@ -1046,6 +1056,30 @@ def build_error_result(
10461056
Used by per-item isolation in ``execute_check_collections``.
10471057
"""
10481058
now = datetime.now(tz=timezone.utc)
1059+
# Surface the failure instead of only stashing it on ``result.error``.
1060+
# This is the isolation boundary for ``abort_on_first_error=False``
1061+
# callers — always data standards, and any multi-file contract run.
1062+
# They never re-raise, and the only other reader of ``result.error`` is
1063+
# the Cloud ``mark_scan_as_failed`` path (gated on a scan_id), so
1064+
# without this a local run exits non-zero having printed nothing.
1065+
#
1066+
# Capture into this placeholder's OWN Logs rather than emitting bare:
1067+
# constructing a Logs leaves it active, so after the construct phase the
1068+
# active target is the LAST-constructed collection. A bare emit here
1069+
# would file this file's error under an unrelated sibling — and ship it
1070+
# inside that sibling's Cloud log payload. ``preserve_active_logs``
1071+
# stops the throwaway target leaking past this call.
1072+
# No emoticon here: the console formatter already prefixes ERROR records
1073+
# with one. Yaml parse errors also already carry their own
1074+
# ", in <file>[line,column]" suffix, so only name the source when the
1075+
# message does not locate itself.
1076+
source_description = getattr(yaml_source, "file_path", None) or getattr(yaml_source, "description", None)
1077+
message = str(exception) or type(exception).__name__
1078+
locates_itself = bool(source_description) and str(source_description) in message
1079+
location = f", in {source_description}" if source_description and not locates_itself else ""
1080+
with preserve_active_logs():
1081+
error_logs = Logs()
1082+
logger.error(f"{message}{location}")
10491083
# Invariant: this placeholder Contract is never uploaded to Soda Cloud.
10501084
# ``build_error_result`` is only invoked when the YAML failed to parse
10511085
# before a real ``Contract`` could be constructed; the result it
@@ -1074,7 +1108,10 @@ def build_error_result(
10741108
measurements=[],
10751109
check_results=[],
10761110
sending_results_to_soda_cloud_failed=False,
1077-
log_records=None,
1111+
# Carries the error line captured above, so the placeholder is
1112+
# self-diagnosing (a FAILED scan in Cloud no longer ships an empty
1113+
# log payload) instead of leaving ``result.error`` the only record.
1114+
log_records=error_logs.get_log_records(),
10781115
post_processing_stages=[],
10791116
)
10801117
result.error = exception
@@ -1218,7 +1255,7 @@ def _verify_check_sources_aligned(self, verification_result: "CheckCollectionRes
12181255
# ``result.get_errors()`` with no re-collection.
12191256
for check_result in offending:
12201257
logger.error(
1221-
f"Source mismatch — check '{check_result.check.full_path}' has "
1258+
f"Source mismatch — check '{check_result.check.check_path}' has "
12221259
f"source={check_result.check.source!r} but parent collection "
12231260
f"declares wire_source={self.wire_source!r}. Skipping Cloud upload to avoid "
12241261
f"a backend-side source-mismatch failure on the whole batch."

soda-core/src/soda_core/common/soda_cloud.py

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1714,15 +1714,16 @@ def _build_check_result_cloud_dict(
17141714
) -> dict:
17151715
return {
17161716
"identities": {"vc1": check_result.check.identity},
1717-
# Wire path: ``Check.full_path`` is the yaml-internal ``path`` for
1718-
# subtypes that emit byte-identical historical paths, and
1719-
# ``"{collection_id}.{path}"`` for subtypes that need a prefix so
1720-
# the backend's ``firstSegmentOf(checkPath)`` can match the
1721-
# subtype's identifier. Selector matching still uses ``check.path``.
1722-
# Falls back to ``path`` when ``full_path`` is empty (the
1717+
# Wire path: ``Check.check_path`` is the yaml-internal ``relative_path``
1718+
# for subtypes that emit byte-identical historical paths (contracts),
1719+
# and the option-3 form
1720+
# ``"{wire_source}.{collection_id}:{relative_path}"`` for subtypes that
1721+
# need a prefix, so the backend can parse out the subtype's identifier.
1722+
# Selector matching still uses ``check.relative_path``.
1723+
# Falls back to ``relative_path`` when ``check_path`` is empty (the
17231724
# dataclass default) — protects external constructors of
1724-
# ``Check(...)`` that don't set ``full_path`` explicitly.
1725-
"checkPath": check_result.check.full_path or check_result.check.path,
1725+
# ``Check(...)`` that don't set ``check_path`` explicitly.
1726+
"checkPath": check_result.check.check_path or check_result.check.relative_path,
17261727
"name": check_result.check.name,
17271728
"type": "generic",
17281729
"checkType": check_result.check.type,

soda-core/src/soda_core/contracts/contract_verification.py

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -306,25 +306,26 @@ class Check:
306306
307307
i.e. "after" CheckImpl has been executed and check result is being created.
308308
309-
``path`` is the YAML-internal stripped path (e.g.
309+
``relative_path`` is the YAML-internal stripped path (e.g.
310310
``"columns.email.checks.missing"``). It is what selector matching and
311311
user-facing messages use; it is byte-identical to the value emitted by
312312
every prior contract verification.
313313
314-
``full_path`` is the wire path emitted to Soda Cloud as ``checkPath``.
315-
For contracts (``wire_source == "soda-contract"``) it equals ``path``.
316-
For non-contract subtypes (data standards, ...) it is prefixed with
317-
``"{collection_id}.{path}"`` so the backend's
318-
``firstSegmentOf(checkPath)`` filter matches the
319-
``DataStandard.name``. Splitting the two means the prefix never leaks
320-
into selector matching or error display.
314+
``check_path`` is the wire path emitted to Soda Cloud as ``checkPath``.
315+
For contracts (``wire_source == "soda-contract"``) it equals
316+
``relative_path``. For non-contract subtypes (data standards, ...) it is
317+
the option-3 form ``"{wire_source}.{collection_id}:{relative_path}"``, which
318+
the backend parses (split on the first ':', then the ref on the first '.')
319+
to recover the collection id and match it against ``DataStandard.name``.
320+
Splitting the two means the prefix never leaks into selector matching or
321+
error display.
321322
"""
322323

323324
column_name: Optional[str]
324325
type: str
325326
qualifier: Optional[str]
326327
name: Optional[str]
327-
path: str
328+
relative_path: str
328329
identity: str
329330
definition: str
330331
contract_file_line: int
@@ -333,13 +334,13 @@ class Check:
333334
attributes: Optional[dict[str, Any]]
334335
location: Optional[Location]
335336
# Wire-path emitted to Soda Cloud as ``checkPath``. For contracts it
336-
# equals ``path``; for non-contract subtypes the engine prefixes it
337-
# with ``"{collection_id}.{path}"`` so the backend's
338-
# ``firstSegmentOf(checkPath)`` filter matches ``DataStandard.name``.
339-
# Defaults to ``""`` so external callers that construct ``Check(...)``
340-
# without specifying it still work — emission code falls back to
341-
# ``path`` when ``full_path`` is empty.
342-
full_path: str = ""
337+
# equals ``relative_path``; for non-contract subtypes it is the option-3
338+
# form ``"{wire_source}.{collection_id}:{relative_path}"``, which the
339+
# backend parses to recover the collection id and match
340+
# ``DataStandard.name``. Defaults to ``""`` so external callers that
341+
# construct ``Check(...)`` without specifying it still work — emission code
342+
# falls back to ``relative_path`` when ``check_path`` is empty.
343+
check_path: str = ""
343344
# Per-check source override. ``None`` (the default) means: emit the
344345
# enclosing ``CheckCollectionImpl.wire_source`` as the wire ``"source"``
345346
# field. A non-None value is exercised by future extensions that emit

soda-core/src/soda_core/contracts/impl/check_selector.py

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,18 @@ class CheckSelector:
1818
- Different fields: AND (all groups must match)
1919
"""
2020

21-
SUPPORTED_FIELDS = {"type", "name", "column", "path", "qualifier"}
21+
SUPPORTED_FIELDS = {
22+
"type",
23+
"name",
24+
"column",
25+
"path",
26+
"relative_path",
27+
"check_path",
28+
"qualifier",
29+
"source",
30+
"collection",
31+
"standard",
32+
}
2233
ATTRIBUTES_PREFIX = "attributes."
2334

2435
def __init__(self, field: str, value: str, raw: str):
@@ -71,10 +82,10 @@ def parse_all(cls, expressions: Optional[list[str]]) -> list[CheckSelector]:
7182

7283
@classmethod
7384
def from_check_paths(cls, check_paths: Optional[list[str]]) -> list[CheckSelector]:
74-
"""Convert --check-paths values to path selectors."""
85+
"""Convert --check-paths values to full check_path selectors (the wire form the FE sends)."""
7586
if not check_paths:
7687
return []
77-
return [cls(field="path", value=path, raw=f"path={path}") for path in check_paths]
88+
return [cls(field="check_path", value=p, raw=f"check_path={p}") for p in check_paths]
7889

7990
def matches(self, check_impl) -> bool:
8091
"""Returns True if the given CheckImpl matches this selector."""
@@ -99,10 +110,16 @@ def _get_check_value(self, check_impl) -> Optional[str | list[str]]:
99110
return check_impl.name
100111
elif self.field == "column":
101112
return check_impl.column_impl.column_yaml.name if check_impl.column_impl else None
102-
elif self.field == "path":
103-
return check_impl.path
113+
elif self.field in ("path", "relative_path"):
114+
return check_impl.relative_path
115+
elif self.field == "check_path":
116+
return check_impl.check_path
104117
elif self.field == "qualifier":
105118
return check_impl.check_yaml.qualifier
119+
elif self.field == "source":
120+
return check_impl.contract_impl.wire_source
121+
elif self.field in ("collection", "standard"):
122+
return check_impl.contract_impl.collection_id
106123
elif self.field.startswith(self.ATTRIBUTES_PREFIX):
107124
attr_key = self.field[len(self.ATTRIBUTES_PREFIX) :]
108125
attr_value = check_impl.attributes.get(attr_key)

soda-core/src/soda_core/contracts/impl/contract_verification_impl.py

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,7 +1143,7 @@ def column_expression(self) -> Optional[SqlExpressionStr | COLUMN]:
11431143
}
11441144

11451145
@property
1146-
def path(self) -> str:
1146+
def relative_path(self) -> str:
11471147
parts: list[str] = []
11481148

11491149
column_name = self.column_impl.column_yaml.name if self.column_impl else None
@@ -1161,35 +1161,44 @@ def path(self) -> str:
11611161
return ".".join(parts)
11621162

11631163
@property
1164-
def full_path(self) -> str:
1164+
def check_path(self) -> str:
11651165
"""Wire path emitted to Soda Cloud as ``checkPath``.
11661166
1167-
For contracts (``wire_source == "soda-contract"``) this is identical
1168-
to ``self.path`` — byte-identical to today's emission. For
1169-
non-contract subtypes it is prefixed with
1170-
``"{collection_id}.{path}"`` so the backend's
1171-
``firstSegmentOf(checkPath)`` filter can match the subtype's
1172-
identifier.
1167+
Format (option 3): ``"{wire_source}.{collection_id}:{relative_path}"``
1168+
using exactly one ``:`` as the delimiter between the ``{type}.{id}``
1169+
prefix and the ``{relative}`` path.
11731170
1174-
Selector matching uses ``self.path`` (not ``full_path``) so the
1175-
prefix never leaks into ``--check-selector`` matching.
1171+
- Contracts (``wire_source == "soda-contract"``): bare
1172+
``self.relative_path``, byte-identical to today's emission.
1173+
- Non-contract subtypes (e.g. data standards): the full option-3
1174+
prefix ``f"{wire_source}.{collection_id}:{relative_path}"``.
1175+
- Defensive fallback (no ``collection_id``): bare ``self.relative_path``.
1176+
1177+
The ``type`` (wire_source) and ``id`` (collection_id) segments must not
1178+
contain ``.`` or ``:``. ``wire_source`` is delimiter-safe **by
1179+
convention** (hardcoded subclass literals, never user input); the
1180+
``base.py`` guard in ``verify()`` enforces this for ``collection_id``
1181+
(which does come from user-supplied config).
1182+
1183+
Selector matching uses ``self.relative_path`` (not ``check_path``) so
1184+
the prefix never leaks into ``--check-selector`` matching.
11761185
"""
11771186
# ``contract_impl`` is the back-ref to the enclosing
11781187
# ``CheckCollectionImpl`` (name preserved during the rename slice;
11791188
# rename deferred to a follow-up). The wire_source check matches
11801189
# ``ContractImpl.wire_source`` literally so any non-contract
11811190
# subtype automatically opts into prefixing.
11821191
if self.contract_impl.wire_source == "soda-contract":
1183-
return self.path
1192+
return self.relative_path
11841193
collection_id: Optional[str] = self.contract_impl.collection_id
11851194
# Non-contract subtypes MUST declare collection_id: the
11861195
# ``CheckCollectionImpl.verify()`` guard raises before this property
11871196
# is reached when collection_id is missing, but we defensively fall
11881197
# back to the bare path so dataclass-build callers can still
11891198
# instantiate a Check during error paths.
11901199
if not collection_id:
1191-
return self.path
1192-
return f"{collection_id}.{self.path}"
1200+
return self.relative_path
1201+
return f"{self.contract_impl.wire_source}.{collection_id}:{self.relative_path}"
11931202

11941203
def _get_name_with_default(self, check_yaml: CheckYaml) -> str:
11951204
if isinstance(check_yaml.name, str):
@@ -1222,8 +1231,8 @@ def _build_check_info(self) -> Check:
12221231
type=self.type,
12231232
qualifier=self.check_yaml.qualifier,
12241233
name=self.name,
1225-
path=self.path,
1226-
full_path=self.full_path,
1234+
relative_path=self.relative_path,
1235+
check_path=self.check_path,
12271236
identity=self.identity,
12281237
definition=self._build_definition(),
12291238
column_name=self.column_impl.column_yaml.name if self.column_impl else None,

soda-tests/tests/unit/test_alignment_guard.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ def _make_check(*, source):
6868
type="missing",
6969
qualifier=None,
7070
name="No missing values",
71-
path="columns.email.checks.missing",
72-
full_path="my_pii_standard.columns.email.checks.missing",
71+
relative_path="columns.email.checks.missing",
72+
check_path="data-standard.my_pii_standard:columns.email.checks.missing",
7373
identity="abc",
7474
definition="...",
7575
contract_file_line=1,

soda-tests/tests/unit/test_check_collections_base.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
from __future__ import annotations
1111

12+
import logging
1213
from datetime import datetime, timezone
1314
from typing import Optional
1415

@@ -228,6 +229,35 @@ def test_execute_check_collections_handles_parse_failures():
228229
assert isinstance(session_result.results[0].error, RuntimeError)
229230

230231

232+
def test_execute_check_collections_logs_isolated_parse_failure(caplog):
233+
"""An isolated parse failure must be LOGGED, not only stored on ``result.error``.
234+
235+
Regression guard: ``build_error_result`` used to stash the exception on
236+
``result.error`` with ``log_records=None`` and never emit it. The only
237+
reader of ``result.error`` is the Soda Cloud ``mark_scan_as_failed``
238+
path (gated on ``soda_scan_id``), so a local run — every
239+
``abort_on_first_error=False`` caller, i.e. always for data standards
240+
and any multi-file contract run — exited 3 in total silence.
241+
"""
242+
caplog.set_level(logging.ERROR)
243+
sources = [_LabelledSource("bad", kind=_RAISING_KIND)]
244+
session_result = execute_check_collections(yaml_sources=sources, data_source_impl=None)
245+
246+
assert session_result.results[0].status is CheckCollectionStatus.ERROR
247+
assert "parse failure for bad" in caplog.text
248+
249+
250+
def test_execute_check_collections_logs_isolated_verify_failure(caplog):
251+
"""Same contract for a verify()-time failure under per-item isolation."""
252+
caplog.set_level(logging.ERROR)
253+
exc = RuntimeError("verify exploded")
254+
sources = [_LabelledSource("a", raise_on_verify=exc)]
255+
session_result = execute_check_collections(yaml_sources=sources, data_source_impl=None)
256+
257+
assert session_result.results[0].status is CheckCollectionStatus.ERROR
258+
assert "verify exploded" in caplog.text
259+
260+
231261
def test_execute_check_collections_records_error_for_unknown_kind():
232262
"""An unknown ``kind:`` produces an ERROR-status placeholder result;
233263
other items in the batch still run.

0 commit comments

Comments
 (0)