From 7b015078e9870c845e6c049c5664180b374dbd33 Mon Sep 17 00:00:00 2001 From: AdrianoDev Date: Mon, 21 Sep 2026 22:47:58 +0200 Subject: [PATCH] fix(measurements): value NaN/Inf/scala e coerenza quota/versione/run (#5, #20) #20 - models/api/measurement.py: value accettava NaN/Infinity/fuori scala, ogni confronto tornava falso ("pass") e poi l'INSERT falliva. Il controllo sposta in measurement_service._validate_measured_value (math.isfinite + |value|<1e6): un Field pydantic (allow_inf_nan/gt/lt) rifiuta la stessa cosa ma FastAPI, costruendo l'errore 422, riecheggia il NaN nel campo "input" e Starlette lo nega con allow_nan=False, producendo un 500 al posto del 422 - verificato con una chiamata reale, non solo a livello di modello. calculate_pass_fail e save_measurement arrotondano value a 6 decimali (stessa scala di DECIMAL(12,6)) prima di calcolare e salvare, cosi' il verdetto e il valore persistito non divergono mai. #5 - measurement_service.save_measurement: prima dell'insert verifica che la quota appartenga a un task della version_id dichiarata (422 altrimenti), e se production_run_id e' dato usa la nuova validate_run_for_version(db, run_id, version_id): il run deve esistere (404), essere della stessa ricetta della versione (422), essere misurabile - riusa production_service._require_measurable (409 se chiuso o in pausa). Import locale per evitare un ciclo con production_service, che gia' importa measurement_service. Test: gia' presenti in test_measurements.py (TestMeasurementValueBounds, TestSaveMeasurementCoherence) - finiti nel commit 0dce229 di un altro agente che lavorava in parallelo sullo stesso file (export CSV, #22); confermato con `git show 0dce229 -- test_measurements.py`, contenuto intatto. Co-Authored-By: Claude Sonnet 5 --- src/backend/models/api/measurement.py | 7 ++ src/backend/services/measurement_service.py | 90 +++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/src/backend/models/api/measurement.py b/src/backend/models/api/measurement.py index 7aabeb3..0f8a5f9 100644 --- a/src/backend/models/api/measurement.py +++ b/src/backend/models/api/measurement.py @@ -9,6 +9,13 @@ class MeasurementCreate(BaseModel): """Schema for creating a single measurement.""" subtask_id: int version_id: int + # Finiteness and the DECIMAL(12,6) scale bound are enforced in + # measurement_service.save_measurement, not with a Field constraint here: + # a NaN/Infinity that fails a pydantic Field constraint gets echoed back + # in the validation error's "input", and FastAPI's default error response + # then crashes with a 500 trying to JSON-encode that same NaN/Infinity + # (json.dumps(allow_nan=False) in Starlette's JSONResponse). A plain + # HTTPException raised after parsing sidesteps that. value: float lot_number: Optional[str] = Field(None, max_length=100) serial_number: Optional[str] = Field(None, max_length=100) diff --git a/src/backend/services/measurement_service.py b/src/backend/services/measurement_service.py index 23fadcb..c31fbe9 100644 --- a/src/backend/services/measurement_service.py +++ b/src/backend/services/measurement_service.py @@ -1,6 +1,8 @@ """Measurement service - pass/fail calculation, data storage.""" +import math from datetime import datetime from decimal import Decimal +from typing import TYPE_CHECKING from fastapi import HTTPException, status from sqlalchemy import func, select @@ -13,6 +15,38 @@ from src.backend.models.orm.task import ( ) from src.backend.models.orm.user import User +if TYPE_CHECKING: + from src.backend.models.orm.production import ProductionRun + + +# What the Measurement.value column (DECIMAL(12,6)) can actually hold. +_MAX_ABS_VALUE = 1_000_000 + + +def _validate_measured_value(value: float) -> None: + """Refuse a value that would corrupt the verdict or the INSERT. + + NaN makes every tolerance comparison false, so a bad reading silently + comes out as "pass"; NaN and Infinity then both fail the DECIMAL(12,6) + column at insert time with an IntegrityError the operator never asked + for. Raised as a plain HTTPException with a string detail rather than as + a pydantic Field constraint on the request model: a Field rejecting NaN + (via allow_inf_nan or a gt/lt bound) makes pydantic echo that same NaN + back in the validation error's "input", and FastAPI's default error + response then crashes with a 500 trying to JSON-encode it + (Starlette's JSONResponse calls json.dumps(..., allow_nan=False)). + """ + if not math.isfinite(value): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail="Measured value must be a finite number (no NaN/Infinity)", + ) + if abs(value) >= _MAX_ABS_VALUE: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail=f"Measured value out of range: |value| must be < {_MAX_ABS_VALUE}", + ) + def calculate_pass_fail( value: float, subtask: RecipeSubtask @@ -25,7 +59,12 @@ def calculate_pass_fail( - If value is outside UTL/LTL -> 'fail' - If value is outside UWL/LWL but inside UTL/LTL -> 'warning' - Otherwise -> 'pass' + + The value is rounded to 6 decimals first - the precision the DECIMAL(12,6) + column keeps - so the verdict is computed on the same number that ends up + stored, and a value like 10.5000004 does not fail against a 10.5 limit. """ + value = round(value, 6) deviation = None if subtask.nominal is not None: deviation = float(Decimal(str(value)) - Decimal(str(subtask.nominal))) @@ -202,6 +241,35 @@ async def task_progress( return progress +async def validate_run_for_version( + db: AsyncSession, run_id: int, version_id: int, +) -> "ProductionRun": + """The run a measurement is being filed against, checked for coherence. + + Three things have to hold before a value goes in against a production: the + run has to exist, it has to belong to the same recipe as the version being + measured (a run started for another product must not collect another + product's history), and it has to be in a state that accepts measurements + (not closed, not paused). Shared with vision_service, which files camera + measurements against a run the same way a manual reading does. + """ + # Local import: production_service imports this module at module scope, + # so importing it back here at module scope would be circular. + from src.backend.services import production_service + + run = await production_service.get_run(db, run_id) # 404 if missing + + recipe = await recipe_of_version(db, version_id) + if recipe is None or run.recipe_id != recipe.id: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail="Production run belongs to a different recipe than this version", + ) + + production_service._require_measurable(run) # 409 if closed or paused + return run + + async def authorise_measurement( db: AsyncSession, measurement: Measurement, supervisor: User, ) -> Measurement: @@ -236,6 +304,8 @@ async def save_measurement( the gate models "moving on" between measurements taken one after another, which does not exist inside a single camera acquisition. """ + _validate_measured_value(value) + # Get subtask for tolerance values result = await db.execute( select(RecipeSubtask).where(RecipeSubtask.id == subtask_id) @@ -244,6 +314,22 @@ async def save_measurement( if subtask is None: raise ValueError(f"Subtask {subtask_id} not found") + # The quote has to belong to a task of the declared version - otherwise the + # tolerances just read off `subtask` are not the ones this version's operator + # was shown, and pass/fail is computed against the wrong recipe. + task_row = await db.execute( + select(RecipeTask.version_id).where(RecipeTask.id == subtask.task_id) + ) + subtask_version_id = task_row.scalar_one_or_none() + if subtask_version_id != version_id: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail="Subtask does not belong to the given version", + ) + + if production_run_id is not None: + await validate_run_for_version(db, production_run_id, version_id) + recipe = await recipe_of_version(db, version_id) _enforce_recipe_rules(recipe, lot_number, serial_number, input_method) @@ -263,6 +349,10 @@ async def save_measurement( ), ) + # Rounded once, here, so the stored value is exactly the one the verdict + # above was computed on - calculate_pass_fail rounds internally too, but + # matching it here keeps the persisted row from disagreeing with itself. + value = round(value, 6) pass_fail, deviation = calculate_pass_fail(value, subtask) measurement = Measurement(