From 355bf3c475b592ae0cc27e34c9bb93fc1a1c1c6a Mon Sep 17 00:00:00 2001 From: Seungpyo1007 Date: Sat, 14 Nov 2026 00:31:28 +0900 Subject: [PATCH] fix(validate): reject non-integer values in integer-typed fields A CPU record with tdp_w 11.5 passed validation and only failed in the dump, where the read schema types the field as an integer. The validator now lists the integer fields of each category and reports a fractional number, a boolean or a string in any of them, and does the same for the members of list-of-integer fields. The lists are written out next to the required-field sets so the module still imports with the standard library alone; TechAPI's PR checks run it without installing the engine. A unit test derives the same lists from the models and fails when the two disagree, and another imports the validator with site-packages off the path. Whole floats such as 65.0 stay valid: the read schemas load them as integers and 15 CPU records hold one. The device catalog's own integer check is replaced by the shared one. --- app/validate.py | 112 +++++++++++++++- tests/unit/test_validate_identity.py | 2 +- tests/unit/test_validate_integers.py | 189 +++++++++++++++++++++++++++ 3 files changed, 298 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_validate_integers.py diff --git a/app/validate.py b/app/validate.py index 584972a..f2de7ec 100644 --- a/app/validate.py +++ b/app/validate.py @@ -1,7 +1,7 @@ """Validate seed JSON against the schema and conventions (§9.3, §15.3). Checks: required fields, slug convention (§14.1), value ranges/units (§14.3), -and foreign-key integrity by slug. Run with ``python -m app.validate``; +integer-typed fields, and foreign-key integrity by slug. Run with ``python -m app.validate``; exits non-zero on the first failure set (used by CI ``validate-data.yml``). ``DATA_DIR`` defaults to the nearest TechAPI data checkout and can be @@ -146,6 +146,83 @@ DATE_RE = re.compile(r"^\d{4}-\d{2}-\d{2}$") +# Fields the models (app/models/) type as int, per category. Kept by hand so +# this module needs nothing but the standard library: TechAPI's PR checks run +# it without installing the engine. tests/unit/test_validate_integers.py fails +# when a model and this list disagree. +BRAND_INTEGERS = {"founded_year"} +SOC_INTEGERS = { + "gpu_cores", + "gpu_clock_mhz", + "geekbench_single", + "geekbench_multi", + "antutu_score", +} +# smartphone, tablet, watch and pda share these. +DEVICE_INTEGERS = {"msrp_usd", "battery_mah"} +DEVICE_INTEGER_LISTS = {"storage_options_gb"} +GPU_INTEGERS = { + "msrp_usd", + "cuda_cores", + "stream_processors", + "rt_cores", + "tensor_cores", + "memory_bus_bit", + "base_clock_mhz", + "boost_clock_mhz", + "tdp_w", + "timespy_score", + "passmark_g3d_mark", + "timespy_extreme_score", + "speedway_score", + "octanebench_score", +} +CPU_INTEGERS = { + "cores", + "threads", + "p_cores", + "e_cores", + "tdp_w", + "max_tdp_w", + "cinebench_r23_single", + "cinebench_r23_multi", + "cinebench_2024_single", + "cinebench_2024_multi", + "geekbench_single", + "geekbench_multi", + "cinebench_r15_single", + "cinebench_r15_multi", + "cinebench_r10_single", + "cinebench_r10_multi", + "passmark_single", + "passmark_cpu_mark", + "msrp_usd", +} +LAPTOP_INTEGERS = {"msrp_usd", "ram_gb", "storage_gb"} +MONITOR_INTEGERS = {"msrp_usd", "refresh_hz", "ppi"} +DEVICE_CATALOG_INTEGERS = { + "screen_density_dpi", + "android_sdk_min", + "android_sdk_max", + "release_year", +} +# category -> (integer fields, list-of-integer fields) +INTEGER_FIELDS: dict[str, tuple[set[str], set[str]]] = { + "brand": (BRAND_INTEGERS, set()), + "soc": (SOC_INTEGERS, set()), + "smartphone": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS), + "tablet": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS), + "watch": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS), + "pda": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS), + "gpu": (GPU_INTEGERS, set()), + "cpu": (CPU_INTEGERS, set()), + "laptop": (LAPTOP_INTEGERS, set()), + "monitor": (MONITOR_INTEGERS, set()), + "software": (set(), set()), + "website": (set(), set()), + "device_catalog": (DEVICE_CATALOG_INTEGERS, set()), +} + def _load(subdir: str, only: set[str] | None = None) -> list[tuple[str, dict[str, Any]]]: """Load every ``*.json`` under ``subdir``; with ``only``, just those paths. @@ -237,6 +314,34 @@ def _check_range( errors.append(f"{name}: {field}={value} out of range [{lo}, {hi}]") +def _is_integer(value: object) -> bool: + # 65.0 passes: the read schemas load it as 65 and existing records hold it. + if isinstance(value, bool): + return False + return isinstance(value, int) or (isinstance(value, float) and value.is_integer()) + + +def _check_integers( + name: str, record: dict[str, Any], category: str, errors: list[str] +) -> None: + """Fields the API types as integers must not hold 11.5, true or "12". + + The table models skip validation, so such a value only fails later, when + the dump builds the read schema. + """ + scalars, lists = INTEGER_FIELDS[category] + for field in sorted(scalars): + value = record.get(field) + if value is not None and not _is_integer(value): + errors.append(f"{name}: {field}={value!r} must be an integer") + for field in sorted(lists): + value = record.get(field) + if value is not None and not ( + isinstance(value, list) and all(_is_integer(item) for item in value) + ): + errors.append(f"{name}: {field}={value!r} must be a list of integers") + + def _check_date(name: str, value: object, errors: list[str]) -> None: if not isinstance(value, str) or not DATE_RE.match(value): errors.append(f"{name}: release_date '{value}' must be ISO 8601 YYYY-MM-DD (§14.2)") @@ -394,6 +499,7 @@ def validate(only: set[str] | None = None) -> list[str]: _check_unique_slugs(category, records, errors) for fname, rec in records: _check_media(fname, rec, errors) + _check_integers(fname, rec, category, errors) for fname, rec in brands: _check_required(fname, rec, BRAND_REQUIRED, errors) @@ -580,9 +686,7 @@ def validate(only: set[str] | None = None) -> list[str]: ("release_year", 1990, 2100), ): value = rec.get(field) - if value is not None and (isinstance(value, bool) or not isinstance(value, int)): - errors.append(f"{fname}: {field} must be an integer") - else: + if _is_integer(value): # anything else is reported by _check_integers _check_range(fname, field, value, lo, hi, errors) sdk_min, sdk_max = rec.get("android_sdk_min"), rec.get("android_sdk_max") if isinstance(sdk_min, int) and isinstance(sdk_max, int) and sdk_max < sdk_min: diff --git a/tests/unit/test_validate_identity.py b/tests/unit/test_validate_identity.py index 5f7d1bc..9cebab1 100644 --- a/tests/unit/test_validate_identity.py +++ b/tests/unit/test_validate_identity.py @@ -79,7 +79,7 @@ def test_catalog_valid_and_required(tmp_path, monkeypatch) -> None: @pytest.mark.parametrize("patch,needle", [ ({"form_factor": "phablet"}, "form_factor 'phablet'"), ({"device_type_guess": "tv-box"}, "device_type_guess 'tv-box'"), - ({"android_sdk_min": "30"}, "android_sdk_min must be an integer"), + ({"android_sdk_min": "30"}, "android_sdk_min='30' must be an integer"), ({"android_sdk_min": 0}, "android_sdk_min=0"), ({"promoted_to": "laptop/x"}, "promoted_to 'laptop/x'"), ({"promoted_to": "smartphone"}, "promoted_to 'smartphone'"), diff --git a/tests/unit/test_validate_integers.py b/tests/unit/test_validate_integers.py new file mode 100644 index 0000000..50fd4b2 --- /dev/null +++ b/tests/unit/test_validate_integers.py @@ -0,0 +1,189 @@ +"""Fields the models type as integers must hold integers in the seed JSON.""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path +from types import NoneType, UnionType +from typing import Any, Union, get_args, get_origin + +import pytest +from pydantic import BaseModel + +from app import validate +from app.models.brand import Brand +from app.models.cpu import CPU +from app.models.device_catalog import DeviceCatalog +from app.models.gpu import DiscreteGPU +from app.models.laptop import Laptop +from app.models.mobile_device import PDA, Tablet, Watch +from app.models.monitor import Monitor +from app.models.smartphone import Smartphone +from app.models.soc import SoC +from app.models.software import Software +from app.models.website import Website +from app.schemas import ( + BrandRead, + DeviceCatalogRead, + LaptopRead, + MonitorRead, + SmartphoneRead, + SoCRead, + SoftwareRead, +) +from app.schemas.cpu import CPURead +from app.schemas.gpu import GPURead +from app.schemas.mobile_device import MobileDeviceRead +from app.schemas.website import WebsiteRead + +REPO = Path(__file__).resolve().parents[2] + +MODELS: dict[str, type[BaseModel]] = dict(zip(validate.CATEGORIES, ( + Brand, SoC, Smartphone, Tablet, Watch, PDA, DiscreteGPU, CPU, + Laptop, Monitor, Software, Website, DeviceCatalog, +), strict=True)) +READ_SCHEMAS: dict[str, type[BaseModel]] = { + "brand": BrandRead, "soc": SoCRead, "smartphone": SmartphoneRead, + "tablet": MobileDeviceRead, "watch": MobileDeviceRead, "pda": MobileDeviceRead, + "gpu": GPURead, "cpu": CPURead, "laptop": LaptopRead, "monitor": MonitorRead, + "software": SoftwareRead, "website": WebsiteRead, "device_catalog": DeviceCatalogRead, +} + +# One record path and one integer field per category that has any. +SAMPLES = [ + ("brand/us/acme.json", "founded_year"), + ("soc/acme/c1.json", "gpu_cores"), + ("smartphone/acme/2020/p.json", "battery_mah"), + ("tablet/acme/2020/t/t.json", "battery_mah"), + ("watch/acme/2020/w/w.json", "msrp_usd"), + ("pda/acme/2020/d/d.json", "battery_mah"), + ("gpu/acme/g1.json", "tdp_w"), + ("cpu/acme/x1.json", "tdp_w"), + ("laptop/acme/2020/l.json", "ram_gb"), + ("monitor/acme/2020/m.json", "refresh_hz"), + ("device_catalog/acme/acme-x.json", "android_sdk_min"), +] + + +def _put(root: Path, rel: str, rec: dict[str, Any]) -> None: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(rec), encoding="utf-8") + + +def _integer_errors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, rel: str, + rec: dict[str, Any], only: set[str] | None = None) -> list[str]: + monkeypatch.setattr(validate, "DATA_DIR", tmp_path) + _put(tmp_path, rel, rec) + return [e for e in validate.validate(only) if "must be an integer" in e + or "must be a list of integers" in e] + + +def _integer_fields(model: type[BaseModel]) -> tuple[set[str], set[str]]: + """Fields a model types as ``int`` and as ``list[int]``. + + Primary and foreign keys are skipped: seed fills them in, the JSON carries + slugs instead. + """ + scalars: set[str] = set() + lists: set[str] = set() + for field, info in model.model_fields.items(): + if getattr(info, "primary_key", None) is True or isinstance( + getattr(info, "foreign_key", None), str + ): + continue + annotation = info.annotation + options = get_args(annotation) if get_origin(annotation) in (Union, UnionType) else ( + annotation, + ) + kinds = {option for option in options if option is not NoneType} + if kinds == {int}: + scalars.add(field) + elif kinds == {list[int]}: + lists.add(field) + return scalars, lists + + +def test_validator_imports_with_the_standard_library_only() -> None: + """TechAPI's PR checks run the validator without installing the engine.""" + code = f"import sys; sys.path.insert(0, {str(REPO)!r}); import app.validate" + # -S keeps site-packages off the path, -I ignores PYTHONPATH and the cwd + done = subprocess.run([sys.executable, "-I", "-S", "-c", code], + capture_output=True, text=True) + assert done.returncode == 0, done.stderr + + +def test_every_category_is_mapped() -> None: + assert set(validate.INTEGER_FIELDS) == set(validate.CATEGORIES) + + +@pytest.mark.parametrize("category", validate.CATEGORIES) +def test_mapping_matches_the_model(category: str) -> None: + """Fails when a model gains or loses an integer column the validator does not list.""" + assert validate.INTEGER_FIELDS[category] == _integer_fields(MODELS[category]) + + +def test_keys_floats_and_bools_are_not_integer_fields() -> None: + scalars, _ = _integer_fields(CPU) + assert not {"id", "manufacturer_id", "base_clock_ghz", "verified"} & scalars + assert "ram_gb" in _integer_fields(Laptop)[0] + assert "ram_gb" not in _integer_fields(Smartphone)[0] + + +@pytest.mark.parametrize("category", validate.CATEGORIES) +def test_read_schema_integers_are_covered(category: str) -> None: + """A read schema must not type a seed field as int unless the model does.""" + schema_scalars, schema_lists = _integer_fields(READ_SCHEMAS[category]) + scalars, lists = validate.INTEGER_FIELDS[category] + assert schema_scalars - {"id"} <= scalars + assert schema_lists <= lists + + +@pytest.mark.parametrize("rel,field", SAMPLES) +@pytest.mark.parametrize("value", [11.5, True, "12", "12.0", [12], float("nan")]) +def test_non_integer_rejected(tmp_path, monkeypatch, rel, field, value) -> None: + errs = _integer_errors(tmp_path, monkeypatch, rel, {"slug": "s", field: value}) + assert errs == [f"{rel}: {field}={value!r} must be an integer"] + + +@pytest.mark.parametrize("rel,field", SAMPLES) +@pytest.mark.parametrize("value", [12, 0, 65.0, None]) +def test_integer_and_null_accepted(tmp_path, monkeypatch, rel, field, value) -> None: + assert _integer_errors(tmp_path, monkeypatch, rel, {"slug": "s", field: value}) == [] + + +def test_absent_field_accepted(tmp_path, monkeypatch) -> None: + assert _integer_errors(tmp_path, monkeypatch, "cpu/acme/x1.json", {"slug": "s"}) == [] + + +def test_float_typed_field_keeps_fractions(tmp_path, monkeypatch) -> None: + rec = {"slug": "s", "ram_gb": 0.5, "weight_g": 180.5} + assert _integer_errors(tmp_path, monkeypatch, "smartphone/acme/2020/p.json", rec) == [] + assert _integer_errors(tmp_path, monkeypatch, "cpu/acme/x1.json", + {"slug": "s", "base_clock_ghz": 3.5, "l3_cache_mb": 1.5}) == [] + + +@pytest.mark.parametrize("value,ok", [ + ([], True), ([64, 128], True), ([64.0], True), (None, True), + ([64, 128.5], False), (["64"], False), ([True], False), (64, False), ("64", False), +]) +def test_integer_list(tmp_path, monkeypatch, value, ok) -> None: + rel = "tablet/acme/2020/t/t.json" + errs = _integer_errors(tmp_path, monkeypatch, rel, {"slug": "s", "storage_options_gb": value}) + expected = [] if ok else [f"{rel}: storage_options_gb={value!r} must be a list of integers"] + assert errs == expected + + +def test_scoped_run_checks_changed_and_fk_records_only(tmp_path, monkeypatch) -> None: + _put(tmp_path, "laptop/acme/2020/old.json", {"slug": "old", "ram_gb": 7.5}) + _put(tmp_path, "cpu/acme/x1.json", {"slug": "x1", "tdp_w": 5.9}) + rel = "laptop/acme/2020/new.json" + errs = _integer_errors(tmp_path, monkeypatch, rel, {"slug": "new", "ram_gb": 15.5}, {rel}) + # cpu is a foreign-key target, so a scoped run still reads all of it + assert errs == [ + "cpu/acme/x1.json: tdp_w=5.9 must be an integer", + f"{rel}: ram_gb=15.5 must be an integer", + ] + assert len(_integer_errors(tmp_path, monkeypatch, rel, {"slug": "new"})) == 2