From bc214e2834391e776bffddf4e5b50de7cdba9761 Mon Sep 17 00:00:00 2001 From: Kevin Costner <120246174+kevincostner17@users.noreply.github.com> Date: Tue, 15 Sep 2026 02:45:53 +0530 Subject: [PATCH] fix(domains): correct EIDR check char, GTIN float text, ambiguous dates, tz compares Retail (#229): a GTIN column with a blank cell loads from CSV as float64, so _gtin_text rendered "4012345678901.0" and GS1-002 flagged it. The strip_nondigits repair then kept the "0" from ".0" and could write a different, mod-10-valid GTIN-14 ("40123456789010"). Integral float cells are now rendered as integer text for validation and repair, and the repair skips non-integral numbers and any text with a "." followed by digits, so such values are only flagged. Media (#259): eidr_check_char implemented the pure ISO 7064 MOD 37-2 system, but the EIDR ID Format spec uses the hybrid MOD 37,36 system over 0-9A-Z. All published EIDR IDs failed MD-C002. It now uses MOD 37,36, and "*" is no longer accepted as a check character. The known-answer test used an ID produced by the old algorithm; it now uses ten published IDs (four registry IDs and six examples from the EIDR ID Format document). CONTRIBUTING_DOMAINS.md is corrected. Finance (#260): the FIN-003 ambiguity guard was anchored at end of string, so "03/04/2024 09:30" skipped it and was parsed month-first. The guard now matches the numeric D/M/Y prefix with any trailing time, so such values are left unresolved like their date-only form. Healthcare (#233 part 2): deceased-after-birth, age-range, encounter-duration and the shared ge_date (end >= start) check compared tz-aware FHIR dateTime values with naive dates or mixed offsets and raised TypeError. They now parse with utc=True (naive values read as UTC) and use pd.Timestamp.now(tz="UTC"). Closes #229 Closes #259 Closes #260 Refs #233 (part 2: healthcare validator) --- CONTRIBUTING_DOMAINS.md | 2 +- src/freshdata/domains/_common.py | 7 ++- src/freshdata/domains/finance/validator.py | 6 ++- src/freshdata/domains/healthcare/validator.py | 15 ++++--- src/freshdata/domains/media/validator.py | 29 ++++++------ src/freshdata/domains/retail/validator.py | 28 +++++++++++- tests/domains/test_finance.py | 21 +++++++++ tests/domains/test_healthcare.py | 40 +++++++++++++++++ tests/domains/test_media.py | 44 +++++++++++++++++-- tests/domains/test_retail.py | 42 ++++++++++++++++++ tests/domains/test_spec_conformance.py | 4 ++ 11 files changed, 208 insertions(+), 30 deletions(-) diff --git a/CONTRIBUTING_DOMAINS.md b/CONTRIBUTING_DOMAINS.md index 9bd67ba0..4c650eb7 100644 --- a/CONTRIBUTING_DOMAINS.md +++ b/CONTRIBUTING_DOMAINS.md @@ -172,6 +172,6 @@ key so the repair stays config-driven. ### Tested pure check-digit functions Identifier check digits are pure, separately unit-tested functions: see -`eidr_check_char` / `is_valid_eidr` (ISO 7064 Mod 37,2) and `is_valid_icpn` +`eidr_check_char` / `is_valid_eidr` (ISO 7064 hybrid MOD 37,36) and `is_valid_icpn` (GS1 mod-10 for UPC/EAN) in `media/validator.py`, anchored by a published known-answer plus round-trip and tamper tests. diff --git a/src/freshdata/domains/_common.py b/src/freshdata/domains/_common.py index 2c20249b..88cfc360 100644 --- a/src/freshdata/domains/_common.py +++ b/src/freshdata/domains/_common.py @@ -293,8 +293,11 @@ def check_at_least_one(df: pd.DataFrame, mapping: ColumnMapping, rule: Rule) -> def check_ge_date(df: pd.DataFrame, mapping: ColumnMapping, rule: Rule) -> list[Any]: """Flag rows where ``fields[0]`` is earlier than ``fields[1]`` (``fields[0] >= fields[1]``).""" - later = to_datetime_safe(df[mapping.actual(rule.fields[0])]) - earlier = to_datetime_safe(df[mapping.actual(rule.fields[1])]) + # utc=True: one side may carry a UTC offset (e.g. a FHIR dateTime) while the other + # is naive or uses a different offset; comparing in UTC (naive read as UTC) never + # raises and does not change the ordering of two naive values. + later = to_datetime_safe(df[mapping.actual(rule.fields[0])], utc=True) + earlier = to_datetime_safe(df[mapping.actual(rule.fields[1])], utc=True) both = later.notna() & earlier.notna() return df.index[both & (later < earlier)].tolist() diff --git a/src/freshdata/domains/finance/validator.py b/src/freshdata/domains/finance/validator.py index 308e0109..89cfb07e 100644 --- a/src/freshdata/domains/finance/validator.py +++ b/src/freshdata/domains/finance/validator.py @@ -27,8 +27,10 @@ _PACK_DIR = Path(__file__).resolve().parent _ISO_DATE_RE = re.compile(r"\d{4}-\d{2}-\d{2}") # Numeric date with both leading components <= 12 is genuinely ambiguous -# (could be DD/MM or MM/DD), so we refuse to coerce it. -_AMBIGUOUS_DATE_RE = re.compile(r"^\s*(\d{1,2})[/.-](\d{1,2})[/.-](\d{2,4})\s*$") +# (could be DD/MM or MM/DD), so we refuse to coerce it. Anything after the year +# (a time such as "03/04/2024 09:30", a "T" separator, a comma) keeps the date part +# just as ambiguous, so only the numeric date prefix is matched. +_AMBIGUOUS_DATE_RE = re.compile(r"^\s*(\d{1,2})[/.-](\d{1,2})[/.-](\d{2,4})(?!\d)") @lru_cache(maxsize=1) diff --git a/src/freshdata/domains/healthcare/validator.py b/src/freshdata/domains/healthcare/validator.py index c3b2bb4f..d19a9310 100644 --- a/src/freshdata/domains/healthcare/validator.py +++ b/src/freshdata/domains/healthcare/validator.py @@ -406,8 +406,11 @@ def _check_gender(self, df: pd.DataFrame, mapping: ColumnMapping, rule: Rule) -> def _check_deceased_after_birth( self, df: pd.DataFrame, mapping: ColumnMapping, rule: Rule ) -> list[Any]: - deceased_date = to_datetime_safe(df[mapping.actual("deceased_date")]) - birth_date = to_datetime_safe(df[mapping.actual("birth_date")]) + # utc=True: FHIR dateTime values may carry a UTC offset while birthDate is a + # plain date; normalising both to UTC (naive values read as UTC) keeps the + # comparison from raising on tz-aware vs naive or mixed offsets. + deceased_date = to_datetime_safe(df[mapping.actual("deceased_date")], utc=True) + birth_date = to_datetime_safe(df[mapping.actual("birth_date")], utc=True) deceased_col = mapping.actual("deceased") truthy = ( self._truthy(df[deceased_col]) if deceased_col is not None @@ -419,8 +422,8 @@ def _check_deceased_after_birth( def _check_age_range( self, df: pd.DataFrame, mapping: ColumnMapping, rule: Rule ) -> list[Any]: - birth_date = to_datetime_safe(df[mapping.actual("birth_date")]) - age_years = (pd.Timestamp.now() - birth_date).dt.days / _DAYS_PER_YEAR + birth_date = to_datetime_safe(df[mapping.actual("birth_date")], utc=True) + age_years = (pd.Timestamp.now(tz="UTC") - birth_date).dt.days / _DAYS_PER_YEAR bad = birth_date.notna() & ((age_years < 0) | (age_years > _MAX_AGE_YEARS)) return df.index[bad].tolist() @@ -474,8 +477,8 @@ def _check_not_future_finished( def _check_duration( self, df: pd.DataFrame, mapping: ColumnMapping, rule: Rule ) -> list[Any]: - start = to_datetime_safe(df[mapping.actual("period_start")]) - end = to_datetime_safe(df[mapping.actual("period_end")]) + start = to_datetime_safe(df[mapping.actual("period_start")], utc=True) + end = to_datetime_safe(df[mapping.actual("period_end")], utc=True) finished = self._is_finished(df, mapping) both = start.notna() & end.notna() & finished return df.index[both & ((end - start).dt.days >= _MAX_ENCOUNTER_DAYS)].tolist() diff --git a/src/freshdata/domains/media/validator.py b/src/freshdata/domains/media/validator.py index c302a650..ece1ff2e 100644 --- a/src/freshdata/domains/media/validator.py +++ b/src/freshdata/domains/media/validator.py @@ -5,7 +5,7 @@ When ``media_type`` is omitted it is auto-detected from the column signature; an indeterminate signature raises :class:`AmbiguousMediaTypeError`. -The EIDR DOI check character (ISO 7064 Mod 37,2) and the ICPN (UPC/EAN) GS1 mod-10 check +The EIDR DOI check character (ISO 7064 hybrid MOD 37,36) and the ICPN (UPC/EAN) GS1 mod-10 check digit are implemented here as pure, unit-tested functions. """ @@ -47,9 +47,13 @@ def _sort_key(value: Any) -> tuple[int, Any]: return (0, value) return (1, str(value)) _EIDR_RE = re.compile( - r"10\.5240/([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z*])" + r"10\.5240/([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z]{4})-([0-9A-Z])" ) -_ISO7064_MOD = 37 +# ISO/IEC 7064 hybrid system MOD 37,36: modulus M = 36 over the alphabet 0-9A-Z, +# with M + 1 = 37 as the second modulus. The EIDR ID Format specification names +# "ISO 7064 Mod 37,36"; the pure MOD 37-2 system (with the supplementary ``*``) is +# a different algorithm and rejects real EIDR IDs. +_ISO7064_M = 36 # -- pure check-digit functions (unit-tested in tests/domains/test_media.py) -- @@ -60,27 +64,24 @@ def _char_value(char: str) -> int: def _value_char(value: int) -> str: - """Inverse of :func:`_char_value`; 36 maps to the ISO 7064 supplementary ``*``.""" - if value < 10: - return chr(48 + value) - if value < 36: - return chr(55 + value) - return "*" + """Inverse of :func:`_char_value` for values 0-35.""" + return chr(48 + value) if value < 10 else chr(55 + value) def eidr_check_char(payload: str) -> str: - """Return the EIDR check character for *payload* via ISO 7064 Mod 37,2. + """Return the EIDR check character for *payload* via ISO 7064 hybrid MOD 37,36. *payload* is the 20-character DOI suffix (hyphens removed, check char excluded). """ - remainder = 0 + product = _ISO7064_M for char in payload: - remainder = (remainder + _char_value(char)) * 2 % _ISO7064_MOD - return _value_char((_ISO7064_MOD + 1 - remainder) % _ISO7064_MOD) + total = (product + _char_value(char)) % _ISO7064_M or _ISO7064_M + product = total * 2 % (_ISO7064_M + 1) + return _value_char((_ISO7064_M + 1 - product) % _ISO7064_M) def is_valid_eidr(value: Any) -> bool: - """True if *value* is a well-formed EIDR DOI with a valid Mod 37,2 check character.""" + """True if *value* is a well-formed EIDR DOI with a valid MOD 37,36 check character.""" if not isinstance(value, str): return False match = _EIDR_RE.fullmatch(value.strip()) diff --git a/src/freshdata/domains/retail/validator.py b/src/freshdata/domains/retail/validator.py index 23f172a0..673a21d2 100644 --- a/src/freshdata/domains/retail/validator.py +++ b/src/freshdata/domains/retail/validator.py @@ -10,11 +10,13 @@ from __future__ import annotations import json +import math import re from functools import lru_cache from pathlib import Path from typing import Any +import numpy as np import pandas as pd from ..base import ColumnMapping, ConfigDrivenValidator, Rule, RuleResult @@ -23,6 +25,7 @@ _BUNDLED_DIR = _PACK_DIR.parent / "bundled" _GTIN_LENGTHS = (8, 12, 13, 14) _NONDIGIT = re.compile(r"\D") +_DECIMAL_POINT = re.compile(r"\.\d") @lru_cache(maxsize=1) @@ -56,6 +59,22 @@ def _gtin_well_formed(text: str) -> bool: return text.isdigit() and len(text) in _GTIN_LENGTHS +def _integral_float_text(value: Any) -> Any: + """Render an integral float cell as integer text (``4012345678901.0`` -> ``"4012345678901"``). + + A GTIN column with a blank cell loads from CSV as float64; its ``str()`` form + carries a ``.0`` suffix whose ``0`` would otherwise be read as an extra digit. + Every other value is returned unchanged. + """ + if ( + isinstance(value, (float, np.floating)) + and math.isfinite(value) + and float(value).is_integer() + ): + return str(int(value)) + return value + + class RetailValidator(ConfigDrivenValidator): """Validator for GS1-aligned product catalog frames.""" @@ -109,7 +128,7 @@ def reference_sources(self) -> list[dict[str, Any]]: def _gtin_text(self, df: pd.DataFrame, mapping: ColumnMapping) -> tuple[pd.Series, pd.Series]: series = df[mapping.actual("gtin")] - return series, series.astype("string").str.strip() + return series, series.map(_integral_float_text).astype("string").str.strip() def _check_gtin_length( self, df: pd.DataFrame, mapping: ColumnMapping, rule: Rule @@ -172,7 +191,12 @@ def _repair_strip_nondigits( value = df.at[row, col] if pd.isna(value): continue - digits = _NONDIGIT.sub("", str(value)) + text = _integral_float_text(value) + if not isinstance(text, str): + continue # a non-integral number is not a GTIN with separators + if _DECIMAL_POINT.search(text): + continue # "4012345678901.0": dropping the "." would invent a digit + digits = _NONDIGIT.sub("", text) if _gtin_well_formed(digits) and _mod10_valid(digits): fixes[row] = digits return fixes diff --git a/tests/domains/test_finance.py b/tests/domains/test_finance.py index 0935c2f9..ecad3d1a 100644 --- a/tests/domains/test_finance.py +++ b/tests/domains/test_finance.py @@ -209,3 +209,24 @@ def test_balance_check_survives_all_missing_transaction_ids(good_finance): _, rep2 = fd.clean(df2, domain="finance", return_report=True, verbose=False) fin006 = next(f for f in rep2.domain_findings if f["rule_id"] == "FIN-006") assert fin006["n_violations"] == 3 + + +@pytest.mark.parametrize("value", ["03/04/2024 09:30", "03-04-2024T09:30:00"]) +def test_ambiguous_date_with_time_not_silently_coerced(good_finance, value): + # Regression (#260): a trailing time used to bypass the DD/MM vs MM/DD guard, + # so "03/04/2024 09:30" was silently read month-first as 2024-03-04. + df = good_finance.copy() + df.loc[0, "date"] = value + out, rep = fd.clean(df, domain="finance", return_report=True, verbose=False) + assert out.loc[0, "date"] == value + assert any(r["rule_id"] == "FIN-003" and r["status"] == "unresolvable" + for r in rep.domain_repairs) + + +def test_unambiguous_date_with_time_still_coerced(good_finance): + df = good_finance.copy() + df.loc[0, "date"] = "01/15/2024 09:30" # 15 can't be a month + out, rep = fd.clean(df, domain="finance", return_report=True, verbose=False) + assert out.loc[0, "date"] == "2024-01-15" + assert any(r["rule_id"] == "FIN-003" and r["status"] == "applied" + and r["to"] == "2024-01-15" for r in rep.domain_repairs) diff --git a/tests/domains/test_healthcare.py b/tests/domains/test_healthcare.py index 1b2bc76c..ac42b1df 100644 --- a/tests/domains/test_healthcare.py +++ b/tests/domains/test_healthcare.py @@ -351,3 +351,43 @@ def test_unknown_domain_lists_available(good_patient): def test_standalone_import(): assert HealthcareValidator(fhir_resource="Patient").domain_name == "healthcare" + + +# -- timezone handling (#233 part 2) ----------------------------------------- + +def test_parsed_fhir_patient_with_offset_deceased_datetime_validates(): + # FHIR dateTime keeps its UTC offset while birthDate is a plain date; comparing + # the two used to raise "Invalid comparison between ... and DatetimeArray". + resource = {"resourceType": "Patient", "id": "1", "birthDate": "1970-01-01", + "gender": "male", "deceasedDateTime": "2015-02-14T13:42:00+10:00"} + frame = fd.parse_domain(resource, format="fhir").frames["patient"] + _, rep = fd.clean(frame, domain="healthcare", return_report=True, verbose=False) + assert not _violated(rep, "HC-P006") + + +def test_deceased_before_birth_detected_with_offset_datetime(good_patient): + df = good_patient.copy() + df["deceased_date"] = [None, None, "1950-01-01T10:00:00+10:00"] # before 1955 birth + _, rep = fd.clean(df, domain="healthcare", fhir_resource="Patient", + return_report=True, verbose=False) + assert _violated(rep, "HC-P006") + + +def test_implausible_age_with_offset_birth_datetime(good_patient): + df = good_patient.copy() + df.loc[0, "birth_date"] = "1820-01-01T00:00:00+05:00" # aware among naive dates + _, rep = fd.clean(df, domain="healthcare", fhir_resource="Patient", + return_report=True, verbose=False) + assert _violated(rep, "HC-P009") + + +def test_encounter_mixed_offsets_compare_in_utc(good_encounter): + df = good_encounter.copy() + # E1: 08:00Z -> 07:30Z ends before it starts in UTC (wall-clock would look fine). + # E2: aware start with a naive end more than 365 days later. + df["period_start"] = ["2024-03-10T03:00:00-05:00", "2023-01-01T00:00:00+10:00"] + df["period_end"] = ["2024-03-10T03:30:00-04:00", "2024-06-01"] + _, rep = fd.clean(df, domain="healthcare", fhir_resource="Encounter", + return_report=True, verbose=False) + assert _violated(rep, "HC-E005") + assert _violated(rep, "HC-E008") diff --git a/tests/domains/test_media.py b/tests/domains/test_media.py index 90989368..24dbde54 100644 --- a/tests/domains/test_media.py +++ b/tests/domains/test_media.py @@ -69,10 +69,48 @@ def good_release() -> pd.DataFrame: # -- pure check-digit functions ------------------------------------------- +# Published EIDR IDs: the first four are real registry IDs, the rest are the examples +# printed in the EIDR ID Format document. Their check characters use ISO 7064 hybrid +# MOD 37,36, the system the spec names ("C is the ISO 7064 Mod 37,36 check character"). +PUBLISHED_EIDRS = [ + "10.5240/7791-8534-2C23-9030-8610-5", + "10.5240/B752-5B47-DBBE-E5D4-5A3F-N", + "10.5240/0EF3-54F9-2642-0B49-6829-R", + "10.5240/1489-49A2-3956-4B2D-FE16-5", + "10.5240/F85A-E100-B068-5B8F-B1C8-T", + "10.5240/3466-F12C-391A-D60B-206B-Y", + "10.5240/CA51-02D0-3269-23C9-DB5A-E", + "10.5240/7481-838B-59CA-63D0-B9A8-E", + "10.5240/CE43-9B6A-2C41-35C3-42CA-V", + "10.5240/823E-5DE9-0816-7BB5-A37F-X", +] + + +@pytest.mark.parametrize("eidr", PUBLISHED_EIDRS) +def test_eidr_published_ids_are_valid(eidr): + assert is_valid_eidr(eidr) + payload = eidr[len("10.5240/"):].replace("-", "")[:-1] + assert eidr_check_char(payload) == eidr[-1] + + def test_eidr_check_char_matches_published_example(): - # EIDR's canonical published identifier and its check character '7'. - assert is_valid_eidr("10.5240/7791-8534-2C23-9030-8004-7") - assert eidr_check_char("779185342C2390308004") == "7" + # EIDR's canonical published identifier and its check character '5'. + assert eidr_check_char("779185342C2390308610") == "5" + assert eidr_check_char("F85AE100B0685B8FB1C8") == "T" + + +def test_eidr_old_mod_37_2_check_chars_are_rejected(): + # Regression (#259): the pure MOD 37-2 system gave '7' here and could emit '*'; + # neither is a valid EIDR check character. + assert not is_valid_eidr("10.5240/7791-8534-2C23-9030-8004-7") + assert eidr_check_char("779185342C2390308004") == "C" + assert not is_valid_eidr("10.5240/7791-8534-2C23-9030-8610-*") + + +def test_eidr_check_char_is_alphanumeric(): + for payload in ("00000000000000000000", "ZZZZZZZZZZZZZZZZZZZZ", "0123456789ABCDEFGHIJ"): + char = eidr_check_char(payload) + assert len(char) == 1 and char.isalnum() and char == char.upper() def test_eidr_roundtrip_and_tamper(): diff --git a/tests/domains/test_retail.py b/tests/domains/test_retail.py index d9f4121e..f8abb917 100644 --- a/tests/domains/test_retail.py +++ b/tests/domains/test_retail.py @@ -2,6 +2,8 @@ from __future__ import annotations +import io + import pandas as pd import pytest @@ -139,3 +141,43 @@ def test_unknown_domain_lists_available(good_retail): def test_standalone_import(): assert RetailValidator().domain_name == "retail" # importable on its own (top of file) + + +# -- float-loaded GTINs (#229) --------------------------------------------- + +def _mod10_ok(code: str) -> bool: + return _check_digit(code[:-1]) == code[-1] + + +def test_csv_gtin_with_blank_cell_is_not_rewritten(): + # A blank GTIN cell makes the CSV column float64 ("4012345678901.0"); the ".0" + # used to be stripped to "40123456789010", a different but mod-10-valid GTIN-14. + df = pd.read_csv(io.StringIO("gtin,product_description\n4012345678901,Widget\n,Missing\n")) + assert df["gtin"].dtype == "float64" + assert _mod10_ok("40123456789010") # why the old rewrite looked legitimate + out, rep = fd.clean(df, domain="retail", return_report=True, verbose=False) + assert out["gtin"].iloc[0] == 4012345678901 + assert not _violated(rep, "GS1-002") + assert not _violated(rep, "GS1-003") + assert not [r for r in rep.domain_repairs + if r["rule_id"] == "GS1-002" and r["status"] == "applied"] + + +def test_float_gtin_column_is_validated_as_integer_text(): + df = pd.DataFrame({"gtin": [4012345678901.0, float("nan"), 4012345678902.0]}) + report = RetailValidator().validate(df) + by_id = {r.rule_id: r for r in report.results} + assert not by_id["GS1-002"].violated # 13 digits once ".0" is dropped + assert by_id["GS1-003"].violation_rows == [2] # wrong check digit is still caught + + +@pytest.mark.parametrize("value", ["4012345678901.0", 4012345678901.5]) +def test_strip_nondigits_never_drops_a_decimal_point(good_retail, value): + df = good_retail.copy() + df["gtin"] = df["gtin"].astype(object) + df.loc[0, "gtin"] = value + validator = RetailValidator() + report = validator.validate(df) + assert any(r.rule_id == "GS1-002" and r.violated for r in report.results) + out, _ = validator.repair(df, report) + assert out.loc[0, "gtin"] == value # flagged, never rewritten diff --git a/tests/domains/test_spec_conformance.py b/tests/domains/test_spec_conformance.py index 72c8a04c..d0fbf890 100644 --- a/tests/domains/test_spec_conformance.py +++ b/tests/domains/test_spec_conformance.py @@ -305,6 +305,10 @@ def test_media_eidr_content_conformance(): return_report=True, verbose=False) assert _violated(rep, "MD-C001") # null eidr_id assert _violated(rep, "MD-C002") # invalid EIDR DOI format + c002 = next(f for f in rep.domain_findings if f["rule_id"] == "MD-C002") + # Row 0 is a published EIDR ID (valid ISO 7064 MOD 37,36 check char); rows 1-3 + # are malformed, carry the wrong DOI prefix, or have a bad check character. + assert c002["violation_rows"] == [1, 2, 3] assert _violated(rep, "MD-C005") # invalid country ZZ assert _violated(rep, "MD-C006") # invalid language 'xx'