Skip to content

Commit c0743ba

Browse files
authored
Merge pull request #136 from GetTechAPI/fix/validate-integer-fields
fix(validate): reject non-integer values in integer-typed fields
2 parents bef2df0 + 355bf3c commit c0743ba

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)