Skip to content

Commit 355bf3c

Browse files
committed
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.
1 parent bef2df0 commit 355bf3c

3 files changed

Lines changed: 298 additions & 5 deletions

File tree

‎app/validate.py‎

Lines changed: 108 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
"""Validate seed JSON against the schema and conventions (§9.3, §15.3).
22
33
Checks: required fields, slug convention (§14.1), value ranges/units (§14.3),
4-
and foreign-key integrity by slug. Run with ``python -m app.validate``;
4+
integer-typed fields, and foreign-key integrity by slug. Run with ``python -m app.validate``;
55
exits non-zero on the first failure set (used by CI ``validate-data.yml``).
66
77
``DATA_DIR`` defaults to the nearest TechAPI data checkout and can be
@@ -146,6 +146,83 @@
146146

147147
DATE_RE = re.compile(r"^\d{4}-\d{2}-\d{2}$")
148148

149+
# Fields the models (app/models/) type as int, per category. Kept by hand so
150+
# this module needs nothing but the standard library: TechAPI's PR checks run
151+
# it without installing the engine. tests/unit/test_validate_integers.py fails
152+
# when a model and this list disagree.
153+
BRAND_INTEGERS = {"founded_year"}
154+
SOC_INTEGERS = {
155+
"gpu_cores",
156+
"gpu_clock_mhz",
157+
"geekbench_single",
158+
"geekbench_multi",
159+
"antutu_score",
160+
}
161+
# smartphone, tablet, watch and pda share these.
162+
DEVICE_INTEGERS = {"msrp_usd", "battery_mah"}
163+
DEVICE_INTEGER_LISTS = {"storage_options_gb"}
164+
GPU_INTEGERS = {
165+
"msrp_usd",
166+
"cuda_cores",
167+
"stream_processors",
168+
"rt_cores",
169+
"tensor_cores",
170+
"memory_bus_bit",
171+
"base_clock_mhz",
172+
"boost_clock_mhz",
173+
"tdp_w",
174+
"timespy_score",
175+
"passmark_g3d_mark",
176+
"timespy_extreme_score",
177+
"speedway_score",
178+
"octanebench_score",
179+
}
180+
CPU_INTEGERS = {
181+
"cores",
182+
"threads",
183+
"p_cores",
184+
"e_cores",
185+
"tdp_w",
186+
"max_tdp_w",
187+
"cinebench_r23_single",
188+
"cinebench_r23_multi",
189+
"cinebench_2024_single",
190+
"cinebench_2024_multi",
191+
"geekbench_single",
192+
"geekbench_multi",
193+
"cinebench_r15_single",
194+
"cinebench_r15_multi",
195+
"cinebench_r10_single",
196+
"cinebench_r10_multi",
197+
"passmark_single",
198+
"passmark_cpu_mark",
199+
"msrp_usd",
200+
}
201+
LAPTOP_INTEGERS = {"msrp_usd", "ram_gb", "storage_gb"}
202+
MONITOR_INTEGERS = {"msrp_usd", "refresh_hz", "ppi"}
203+
DEVICE_CATALOG_INTEGERS = {
204+
"screen_density_dpi",
205+
"android_sdk_min",
206+
"android_sdk_max",
207+
"release_year",
208+
}
209+
# category -> (integer fields, list-of-integer fields)
210+
INTEGER_FIELDS: dict[str, tuple[set[str], set[str]]] = {
211+
"brand": (BRAND_INTEGERS, set()),
212+
"soc": (SOC_INTEGERS, set()),
213+
"smartphone": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS),
214+
"tablet": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS),
215+
"watch": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS),
216+
"pda": (DEVICE_INTEGERS, DEVICE_INTEGER_LISTS),
217+
"gpu": (GPU_INTEGERS, set()),
218+
"cpu": (CPU_INTEGERS, set()),
219+
"laptop": (LAPTOP_INTEGERS, set()),
220+
"monitor": (MONITOR_INTEGERS, set()),
221+
"software": (set(), set()),
222+
"website": (set(), set()),
223+
"device_catalog": (DEVICE_CATALOG_INTEGERS, set()),
224+
}
225+
149226

150227
def _load(subdir: str, only: set[str] | None = None) -> list[tuple[str, dict[str, Any]]]:
151228
"""Load every ``*.json`` under ``subdir``; with ``only``, just those paths.
@@ -237,6 +314,34 @@ def _check_range(
237314
errors.append(f"{name}: {field}={value} out of range [{lo}, {hi}]")
238315

239316

317+
def _is_integer(value: object) -> bool:
318+
# 65.0 passes: the read schemas load it as 65 and existing records hold it.
319+
if isinstance(value, bool):
320+
return False
321+
return isinstance(value, int) or (isinstance(value, float) and value.is_integer())
322+
323+
324+
def _check_integers(
325+
name: str, record: dict[str, Any], category: str, errors: list[str]
326+
) -> None:
327+
"""Fields the API types as integers must not hold 11.5, true or "12".
328+
329+
The table models skip validation, so such a value only fails later, when
330+
the dump builds the read schema.
331+
"""
332+
scalars, lists = INTEGER_FIELDS[category]
333+
for field in sorted(scalars):
334+
value = record.get(field)
335+
if value is not None and not _is_integer(value):
336+
errors.append(f"{name}: {field}={value!r} must be an integer")
337+
for field in sorted(lists):
338+
value = record.get(field)
339+
if value is not None and not (
340+
isinstance(value, list) and all(_is_integer(item) for item in value)
341+
):
342+
errors.append(f"{name}: {field}={value!r} must be a list of integers")
343+
344+
240345
def _check_date(name: str, value: object, errors: list[str]) -> None:
241346
if not isinstance(value, str) or not DATE_RE.match(value):
242347
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]:
394499
_check_unique_slugs(category, records, errors)
395500
for fname, rec in records:
396501
_check_media(fname, rec, errors)
502+
_check_integers(fname, rec, category, errors)
397503

398504
for fname, rec in brands:
399505
_check_required(fname, rec, BRAND_REQUIRED, errors)
@@ -580,9 +686,7 @@ def validate(only: set[str] | None = None) -> list[str]:
580686
("release_year", 1990, 2100),
581687
):
582688
value = rec.get(field)
583-
if value is not None and (isinstance(value, bool) or not isinstance(value, int)):
584-
errors.append(f"{fname}: {field} must be an integer")
585-
else:
689+
if _is_integer(value): # anything else is reported by _check_integers
586690
_check_range(fname, field, value, lo, hi, errors)
587691
sdk_min, sdk_max = rec.get("android_sdk_min"), rec.get("android_sdk_max")
588692
if isinstance(sdk_min, int) and isinstance(sdk_max, int) and sdk_max < sdk_min:

‎tests/unit/test_validate_identity.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ def test_catalog_valid_and_required(tmp_path, monkeypatch) -> None:
7979
@pytest.mark.parametrize("patch,needle", [
8080
({"form_factor": "phablet"}, "form_factor 'phablet'"),
8181
({"device_type_guess": "tv-box"}, "device_type_guess 'tv-box'"),
82-
({"android_sdk_min": "30"}, "android_sdk_min must be an integer"),
82+
({"android_sdk_min": "30"}, "android_sdk_min='30' must be an integer"),
8383
({"android_sdk_min": 0}, "android_sdk_min=0"),
8484
({"promoted_to": "laptop/x"}, "promoted_to 'laptop/x'"),
8585
({"promoted_to": "smartphone"}, "promoted_to 'smartphone'"),
Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
"""Fields the models type as integers must hold integers in the seed JSON."""
2+
3+
from __future__ import annotations
4+
5+
import json
6+
import subprocess
7+
import sys
8+
from pathlib import Path
9+
from types import NoneType, UnionType
10+
from typing import Any, Union, get_args, get_origin
11+
12+
import pytest
13+
from pydantic import BaseModel
14+
15+
from app import validate
16+
from app.models.brand import Brand
17+
from app.models.cpu import CPU
18+
from app.models.device_catalog import DeviceCatalog
19+
from app.models.gpu import DiscreteGPU
20+
from app.models.laptop import Laptop
21+
from app.models.mobile_device import PDA, Tablet, Watch
22+
from app.models.monitor import Monitor
23+
from app.models.smartphone import Smartphone
24+
from app.models.soc import SoC
25+
from app.models.software import Software
26+
from app.models.website import Website
27+
from app.schemas import (
28+
BrandRead,
29+
DeviceCatalogRead,
30+
LaptopRead,
31+
MonitorRead,
32+
SmartphoneRead,
33+
SoCRead,
34+
SoftwareRead,
35+
)
36+
from app.schemas.cpu import CPURead
37+
from app.schemas.gpu import GPURead
38+
from app.schemas.mobile_device import MobileDeviceRead
39+
from app.schemas.website import WebsiteRead
40+
41+
REPO = Path(__file__).resolve().parents[2]
42+
43+
MODELS: dict[str, type[BaseModel]] = dict(zip(validate.CATEGORIES, (
44+
Brand, SoC, Smartphone, Tablet, Watch, PDA, DiscreteGPU, CPU,
45+
Laptop, Monitor, Software, Website, DeviceCatalog,
46+
), strict=True))
47+
READ_SCHEMAS: dict[str, type[BaseModel]] = {
48+
"brand": BrandRead, "soc": SoCRead, "smartphone": SmartphoneRead,
49+
"tablet": MobileDeviceRead, "watch": MobileDeviceRead, "pda": MobileDeviceRead,
50+
"gpu": GPURead, "cpu": CPURead, "laptop": LaptopRead, "monitor": MonitorRead,
51+
"software": SoftwareRead, "website": WebsiteRead, "device_catalog": DeviceCatalogRead,
52+
}
53+
54+
# One record path and one integer field per category that has any.
55+
SAMPLES = [
56+
("brand/us/acme.json", "founded_year"),
57+
("soc/acme/c1.json", "gpu_cores"),
58+
("smartphone/acme/2020/p.json", "battery_mah"),
59+
("tablet/acme/2020/t/t.json", "battery_mah"),
60+
("watch/acme/2020/w/w.json", "msrp_usd"),
61+
("pda/acme/2020/d/d.json", "battery_mah"),
62+
("gpu/acme/g1.json", "tdp_w"),
63+
("cpu/acme/x1.json", "tdp_w"),
64+
("laptop/acme/2020/l.json", "ram_gb"),
65+
("monitor/acme/2020/m.json", "refresh_hz"),
66+
("device_catalog/acme/acme-x.json", "android_sdk_min"),
67+
]
68+
69+
70+
def _put(root: Path, rel: str, rec: dict[str, Any]) -> None:
71+
path = root / rel
72+
path.parent.mkdir(parents=True, exist_ok=True)
73+
path.write_text(json.dumps(rec), encoding="utf-8")
74+
75+
76+
def _integer_errors(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, rel: str,
77+
rec: dict[str, Any], only: set[str] | None = None) -> list[str]:
78+
monkeypatch.setattr(validate, "DATA_DIR", tmp_path)
79+
_put(tmp_path, rel, rec)
80+
return [e for e in validate.validate(only) if "must be an integer" in e
81+
or "must be a list of integers" in e]
82+
83+
84+
def _integer_fields(model: type[BaseModel]) -> tuple[set[str], set[str]]:
85+
"""Fields a model types as ``int`` and as ``list[int]``.
86+
87+
Primary and foreign keys are skipped: seed fills them in, the JSON carries
88+
slugs instead.
89+
"""
90+
scalars: set[str] = set()
91+
lists: set[str] = set()
92+
for field, info in model.model_fields.items():
93+
if getattr(info, "primary_key", None) is True or isinstance(
94+
getattr(info, "foreign_key", None), str
95+
):
96+
continue
97+
annotation = info.annotation
98+
options = get_args(annotation) if get_origin(annotation) in (Union, UnionType) else (
99+
annotation,
100+
)
101+
kinds = {option for option in options if option is not NoneType}
102+
if kinds == {int}:
103+
scalars.add(field)
104+
elif kinds == {list[int]}:
105+
lists.add(field)
106+
return scalars, lists
107+
108+
109+
def test_validator_imports_with_the_standard_library_only() -> None:
110+
"""TechAPI's PR checks run the validator without installing the engine."""
111+
code = f"import sys; sys.path.insert(0, {str(REPO)!r}); import app.validate"
112+
# -S keeps site-packages off the path, -I ignores PYTHONPATH and the cwd
113+
done = subprocess.run([sys.executable, "-I", "-S", "-c", code],
114+
capture_output=True, text=True)
115+
assert done.returncode == 0, done.stderr
116+
117+
118+
def test_every_category_is_mapped() -> None:
119+
assert set(validate.INTEGER_FIELDS) == set(validate.CATEGORIES)
120+
121+
122+
@pytest.mark.parametrize("category", validate.CATEGORIES)
123+
def test_mapping_matches_the_model(category: str) -> None:
124+
"""Fails when a model gains or loses an integer column the validator does not list."""
125+
assert validate.INTEGER_FIELDS[category] == _integer_fields(MODELS[category])
126+
127+
128+
def test_keys_floats_and_bools_are_not_integer_fields() -> None:
129+
scalars, _ = _integer_fields(CPU)
130+
assert not {"id", "manufacturer_id", "base_clock_ghz", "verified"} & scalars
131+
assert "ram_gb" in _integer_fields(Laptop)[0]
132+
assert "ram_gb" not in _integer_fields(Smartphone)[0]
133+
134+
135+
@pytest.mark.parametrize("category", validate.CATEGORIES)
136+
def test_read_schema_integers_are_covered(category: str) -> None:
137+
"""A read schema must not type a seed field as int unless the model does."""
138+
schema_scalars, schema_lists = _integer_fields(READ_SCHEMAS[category])
139+
scalars, lists = validate.INTEGER_FIELDS[category]
140+
assert schema_scalars - {"id"} <= scalars
141+
assert schema_lists <= lists
142+
143+
144+
@pytest.mark.parametrize("rel,field", SAMPLES)
145+
@pytest.mark.parametrize("value", [11.5, True, "12", "12.0", [12], float("nan")])
146+
def test_non_integer_rejected(tmp_path, monkeypatch, rel, field, value) -> None:
147+
errs = _integer_errors(tmp_path, monkeypatch, rel, {"slug": "s", field: value})
148+
assert errs == [f"{rel}: {field}={value!r} must be an integer"]
149+
150+
151+
@pytest.mark.parametrize("rel,field", SAMPLES)
152+
@pytest.mark.parametrize("value", [12, 0, 65.0, None])
153+
def test_integer_and_null_accepted(tmp_path, monkeypatch, rel, field, value) -> None:
154+
assert _integer_errors(tmp_path, monkeypatch, rel, {"slug": "s", field: value}) == []
155+
156+
157+
def test_absent_field_accepted(tmp_path, monkeypatch) -> None:
158+
assert _integer_errors(tmp_path, monkeypatch, "cpu/acme/x1.json", {"slug": "s"}) == []
159+
160+
161+
def test_float_typed_field_keeps_fractions(tmp_path, monkeypatch) -> None:
162+
rec = {"slug": "s", "ram_gb": 0.5, "weight_g": 180.5}
163+
assert _integer_errors(tmp_path, monkeypatch, "smartphone/acme/2020/p.json", rec) == []
164+
assert _integer_errors(tmp_path, monkeypatch, "cpu/acme/x1.json",
165+
{"slug": "s", "base_clock_ghz": 3.5, "l3_cache_mb": 1.5}) == []
166+
167+
168+
@pytest.mark.parametrize("value,ok", [
169+
([], True), ([64, 128], True), ([64.0], True), (None, True),
170+
([64, 128.5], False), (["64"], False), ([True], False), (64, False), ("64", False),
171+
])
172+
def test_integer_list(tmp_path, monkeypatch, value, ok) -> None:
173+
rel = "tablet/acme/2020/t/t.json"
174+
errs = _integer_errors(tmp_path, monkeypatch, rel, {"slug": "s", "storage_options_gb": value})
175+
expected = [] if ok else [f"{rel}: storage_options_gb={value!r} must be a list of integers"]
176+
assert errs == expected
177+
178+
179+
def test_scoped_run_checks_changed_and_fk_records_only(tmp_path, monkeypatch) -> None:
180+
_put(tmp_path, "laptop/acme/2020/old.json", {"slug": "old", "ram_gb": 7.5})
181+
_put(tmp_path, "cpu/acme/x1.json", {"slug": "x1", "tdp_w": 5.9})
182+
rel = "laptop/acme/2020/new.json"
183+
errs = _integer_errors(tmp_path, monkeypatch, rel, {"slug": "new", "ram_gb": 15.5}, {rel})
184+
# cpu is a foreign-key target, so a scoped run still reads all of it
185+
assert errs == [
186+
"cpu/acme/x1.json: tdp_w=5.9 must be an integer",
187+
f"{rel}: ram_gb=15.5 must be an integer",
188+
]
189+
assert len(_integer_errors(tmp_path, monkeypatch, rel, {"slug": "new"})) == 2

0 commit comments

Comments
 (0)