From dab8d4590d0a579591fddb6ccaac9072f3e15b45 Mon Sep 17 00:00:00 2001 From: amarcozzi Date: Mon, 8 Jun 2026 18:22:38 -0600 Subject: [PATCH 1/2] Stop padding optional inventory columns; guard absence at every boundary (#320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The uploader padded every optional column a file didn't provide with an all-null placeholder before writing the Parquet, turning "this data doesn't exist" (absent column — loud, checkable) into "this data exists and is null" (present column — silently wrong everywhere, since null comparisons are False in pandas/pyarrow/Parquet pushdown alike). #316 fixed the metadata write side so the document's `columns` records what the file provided; this removes the padding itself and makes absence loud at every consumer. Storage contract: the Parquet contains exactly the columns the file provided; the document's `columns` metadata is the source of truth that request-time guards check. Absence stays absence. - Uploader: drop the padding loop in `_validate`; write only provided columns. - Shared vocabulary: new stdlib-only `lib.inventory` holds the canonical column names and `VOXELIZE_REQUIRED_COLUMNS`, so the API guard, treevox, and standgen agree on what each role is called instead of re-asserting string literals. (Stdlib-only so the GDAL-free API can import it.) - API guards (422 from the inventory's `columns` metadata, same pattern as the treatments `dbh` guard): - tree voxelize create: reject inventories missing the per-tree measurements voxelization needs (a position-and-height-only CHM/ITD inventory), - in-place + create-time CHM modifications: reject rules referencing a column the inventory doesn't have. - treevox `read_inventory`: probe the Parquet `_metadata` footer and raise an actionable `MISSING_COLUMNS` instead of a misleading `INVENTORY_NOT_FOUND` when a required column is absent. - standgen backstop: the in-place modifications handler now checks the actual Parquet schema (no compute) before applying, mirroring the treatments handler, so a stale document fails with `MISSING_COLUMNS` instead of a mid-write KeyError or a silently materialized all-null column. --- .../grids/voxelize/inventory/tree/router.py | 16 +++ .../inventories/modification_models.py | 27 ++++ .../inventories/modifications/router.py | 17 ++- .../resources/inventories/tree/chm/router.py | 17 ++- .../api/api/resources/inventories/utils.py | 32 +++++ .../voxelize/inventory/tree/test_router.py | 39 ++++++ .../inventories/test_modifications_router.py | 83 +++++++++++++ .../inventories/tree/chm/test_router.py | 57 +++++++++ services/lib/lib/inventory.py | 27 ++++ .../standgen/handlers/modifications.py | 23 ++++ services/standgen/standgen/modifications.py | 30 +++++ services/standgen/standgen/treatments.py | 7 +- .../tests/handlers/test_modifications.py | 40 ++++++ services/standgen/tests/test_modifications.py | 32 +++++ services/treevox/tests/test_inventory_io.py | 117 +++++++++++++++--- services/treevox/treevox/errors.py | 2 +- services/treevox/treevox/inventory_io.py | 101 ++++++++++----- .../integration/test_inventory_upload.py | 15 ++- .../tests/unit/handlers/test_inventory.py | 10 +- .../uploader/uploader/handlers/inventory.py | 32 +++-- 20 files changed, 648 insertions(+), 76 deletions(-) create mode 100644 services/lib/lib/inventory.py diff --git a/services/api/api/resources/grids/voxelize/inventory/tree/router.py b/services/api/api/resources/grids/voxelize/inventory/tree/router.py index dbc1d2d7..178014d7 100644 --- a/services/api/api/resources/grids/voxelize/inventory/tree/router.py +++ b/services/api/api/resources/grids/voxelize/inventory/tree/router.py @@ -24,6 +24,7 @@ TreeInventoryVoxelizationSource, build_tree_bands, ) +from api.resources.inventories.utils import require_inventory_columns from api.schema import JobStatus from api.tasks import create_http_task_async from lib.config import ( @@ -32,6 +33,7 @@ TREEVOX_QUEUE, TREEVOX_SERVICE, ) +from lib.inventory import VOXELIZE_REQUIRED_COLUMNS router = APIRouter() @@ -122,6 +124,20 @@ async def create_tree_inventory_grid( ), ) + # Voxelization reads every per-tree measurement (diameter, species, crown + # ratio drive the crown-profile and biomass models; status keeps live trees). + # A position-and-height-only inventory (e.g. CHM/ITD extraction) can't be + # voxelized until those exist. Biomass / max-crown-radius inventory-column + # references are validated by treevox at read time. + require_inventory_columns( + {column["key"] for column in inventory_data.get("columns", [])}, + VOXELIZE_REQUIRED_COLUMNS, + detail=( + "This inventory lacks the per-tree measurements voxelization needs " + "(a position-and-height-only CHM/ITD inventory must be enriched first)." + ), + ) + grid_id = uuid.uuid4().hex request_time = datetime.now() diff --git a/services/api/api/resources/inventories/modification_models.py b/services/api/api/resources/inventories/modification_models.py index c44b0453..b9eddfea 100644 --- a/services/api/api/resources/inventories/modification_models.py +++ b/services/api/api/resources/inventories/modification_models.py @@ -360,3 +360,30 @@ def validate_remove_is_sole_action(self): if has_remove and len(self.actions) > 1: raise ValueError("RemoveAction must be the sole action if present") return self + + +def modification_referenced_columns( + modifications: list[InventoryModification], +) -> set[str]: + """Return the inventory column names a list of modifications references. + + Collects from both conditions and actions: attribute conditions/actions + contribute their ``attribute``; expression conditions contribute every name + used in the expression. Spatial conditions and ``RemoveAction`` reference no + measurement column (they test a tree's position or remove rows). Used to + reject a modification that references a column the target inventory lacks. + """ + columns: set[str] = set() + for mod in modifications: + for condition in mod.conditions: + if isinstance(condition, InventoryModificationCondition): + columns.add(condition.attribute.value) + elif isinstance(condition, InventoryExpressionCondition): + tree = ast.parse(condition.expression, mode="eval") + columns.update( + node.id for node in ast.walk(tree) if isinstance(node, ast.Name) + ) + for action in mod.actions: + if isinstance(action, InventoryModificationAction): + columns.add(action.attribute.value) + return columns diff --git a/services/api/api/resources/inventories/modifications/router.py b/services/api/api/resources/inventories/modifications/router.py index 6c3f4e32..f3191a26 100644 --- a/services/api/api/resources/inventories/modifications/router.py +++ b/services/api/api/resources/inventories/modifications/router.py @@ -15,12 +15,18 @@ from api.db.documents import get_document_async, set_document_async from api.dependencies import VerifiedDomain +from api.resources.inventories.modification_models import ( + modification_referenced_columns, +) from api.resources.inventories.modifications.examples import ( APPLY_MODIFICATIONS_OPENAPI_EXAMPLES, ) from api.resources.inventories.modifications.schema import ApplyModificationsRequest from api.resources.inventories.schema import Inventory -from api.resources.inventories.utils import validate_feature_conditions +from api.resources.inventories.utils import ( + require_inventory_columns, + validate_feature_conditions, +) from api.resources.modifications import stringify_modification_coordinates from api.schema import JobStatus from api.tasks import create_http_task_async @@ -127,6 +133,15 @@ async def apply_modifications( ) inventory_data = snapshot.to_dict() + # Reject rules that reference a column this inventory doesn't have (e.g. + # `dbh > 30` on an upload or CHM inventory with no dbh). Absence is loud now + # that the uploader no longer pads missing columns with nulls. + require_inventory_columns( + {column["key"] for column in inventory_data.get("columns", [])}, + modification_referenced_columns(body.modifications), + detail="A modification references column(s) this inventory doesn't have.", + ) + new_modifications = stringify_modification_coordinates( [m.model_dump() for m in body.modifications] ) diff --git a/services/api/api/resources/inventories/tree/chm/router.py b/services/api/api/resources/inventories/tree/chm/router.py index b8bbbac4..a89e1eb5 100644 --- a/services/api/api/resources/inventories/tree/chm/router.py +++ b/services/api/api/resources/inventories/tree/chm/router.py @@ -12,13 +12,19 @@ from api.db.documents import get_document_async, set_document_async from api.dependencies import VerifiedDomain +from api.resources.inventories.modification_models import ( + modification_referenced_columns, +) from api.resources.inventories.schema import CHM_INVENTORY_COLUMNS, Inventory from api.resources.inventories.tree.chm.examples import CREATE_CHM_OPENAPI_EXAMPLES from api.resources.inventories.tree.chm.schema import ( ChmInventorySource, CreateChmInventoryRequest, ) -from api.resources.inventories.utils import validate_feature_conditions +from api.resources.inventories.utils import ( + require_inventory_columns, + validate_feature_conditions, +) from api.resources.modifications import stringify_modification_coordinates from api.schema import JobStatus from api.tasks import create_http_task_async @@ -80,6 +86,15 @@ async def create_chm_inventory( [*body.modifications, *body.treatments], owner_id, domain_id ) + # A CHM inventory only ever carries position and height. Reject create-time + # modifications that reference columns it won't have (e.g. `dbh > 30`) at the + # boundary, mirroring the in-place modifications guard. + require_inventory_columns( + {column.key for column in CHM_INVENTORY_COLUMNS}, + modification_referenced_columns(body.modifications), + detail="A modification references column(s) a CHM inventory doesn't have.", + ) + # Validate source CHM grid exists, is owned, in this domain, and completed _, source_snapshot = await get_document_async( GRIDS_COLLECTION, diff --git a/services/api/api/resources/inventories/utils.py b/services/api/api/resources/inventories/utils.py index d78d0a80..d5d8578e 100644 --- a/services/api/api/resources/inventories/utils.py +++ b/services/api/api/resources/inventories/utils.py @@ -54,6 +54,38 @@ def validate_inventory_wide_treatment_area(domain: dict, treatments: list) -> No ) +def require_inventory_columns( + available_keys: set[str], + required: set[str], + *, + detail: str, +) -> None: + """Reject an operation whose required columns aren't all present in the + inventory. + + ``available_keys`` is the set of column keys the inventory provides (from its + ``columns`` metadata — the source of truth recorded by the uploader and + source services). ``required`` is the set of columns an operation needs or + that a modification rule references. ``detail`` is the lead-in message; the + required (asked-for) and available columns are appended so the caller sees + exactly what was requested versus what the inventory provides. + + Raises: + HTTPException(422): If any required column is absent. The columns the + client effectively asked for aren't in this inventory, so this is a + validation error on the request, not a path-level 404. + """ + if required <= available_keys: + return + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail=( + f"{detail} Required column(s): {sorted(required)}. " + f"Available column(s): {sorted(available_keys)}." + ), + ) + + async def validate_feature_conditions( items: list, owner_id: str, diff --git a/services/api/tests/resources/grids/voxelize/inventory/tree/test_router.py b/services/api/tests/resources/grids/voxelize/inventory/tree/test_router.py index 0e1a6d3d..1b1269e8 100644 --- a/services/api/tests/resources/grids/voxelize/inventory/tree/test_router.py +++ b/services/api/tests/resources/grids/voxelize/inventory/tree/test_router.py @@ -10,6 +10,7 @@ from api.resources.grids.voxelize.inventory.tree.examples import ( ALL_TREE_INVENTORY_EXAMPLE_VALUES, ) +from api.resources.inventories.schema import CHM_INVENTORY_COLUMNS from lib.config import DOMAINS_COLLECTION, INVENTORIES_COLLECTION from tests.fixtures import make_domain_data, make_inventory_data @@ -46,6 +47,25 @@ def second_domain_for_tree_voxelization(firestore_client): doc_ref.delete() +@pytest.fixture(scope="session") +def height_only_inventory(firestore_client, domain_for_testing): + """A completed tree inventory carrying only position and height (e.g. CHM/ITD + extraction) — it lacks the per-tree measurements voxelization needs.""" + inventory_data = make_inventory_data( + domain_id=domain_for_testing["id"], + name="Height-only inventory for voxelization guard", + status="completed", + inventory_type="tree", + ) + inventory_data["columns"] = [c.model_dump() for c in CHM_INVENTORY_COLUMNS] + doc_ref = firestore_client.collection(INVENTORIES_COLLECTION).document( + inventory_data["id"] + ) + doc_ref.set(inventory_data) + yield inventory_data + doc_ref.delete() + + @pytest.fixture(scope="session") def tree_inventory_in_different_domain( firestore_client, second_domain_for_tree_voxelization @@ -282,6 +302,25 @@ def test_source_inventory_not_completed_returns_422( finally: doc_ref.delete() + def test_height_only_inventory_returns_422( + self, client, domain_for_testing, height_only_inventory + ): + """A position-and-height-only inventory can't be voxelized — it's missing + diameter, species, crown ratio, and status. The error names what's + required versus what the inventory provides.""" + body = {"source_inventory_id": height_only_inventory["id"]} + response = client.post(self.route(domain_for_testing["id"]), json=body) + assert response.status_code == 422 + detail = response.json()["detail"] + assert "Required column(s)" in detail + assert "Available column(s)" in detail + # dbh is named as required but is not among the available columns — + # proving the guard reported the actually-missing column, not just that + # the word "dbh" appears somewhere in the (always-listed) required set. + required_part, available_part = detail.split("Available column(s)") + assert "dbh" in required_part + assert "dbh" not in available_part + # --- Request body validation --- def test_missing_source_inventory_id_returns_422(self, client, domain_for_testing): diff --git a/services/api/tests/resources/inventories/test_modifications_router.py b/services/api/tests/resources/inventories/test_modifications_router.py index 05b1e82a..cc6dbfdf 100644 --- a/services/api/tests/resources/inventories/test_modifications_router.py +++ b/services/api/tests/resources/inventories/test_modifications_router.py @@ -17,6 +17,7 @@ ALL_MODIFICATIONS_EXAMPLE_VALUES, ) from api.resources.inventories.modifications.schema import ApplyModificationsRequest +from api.resources.inventories.schema import CHM_INVENTORY_COLUMNS from lib.config import FEATURES_COLLECTION, INVENTORIES_COLLECTION from tests.fixtures import make_feature_data, make_inventory_data @@ -98,6 +99,32 @@ def completed_inventory_different_owner(firestore_client, domain_with_different_ doc_ref.delete() +@pytest.fixture +def no_dbh_inventory(firestore_client, domain_for_testing, cleanup_inventories): + """A completed CHM-derived inventory: position and height only, no dbh. + + Function-scoped because the success-path test mutates it (status -> pending). + """ + inv = make_inventory_data( + domain_id=domain_for_testing["id"], + name="CHM Inventory (no dbh) for modifications guard", + status="completed", + source={ + "name": "chm", + "source_chm_grid_id": f"test-{uuid.uuid4().hex}", + "algorithm": {"name": "lmf"}, + }, + georeference={ + "crs": "EPSG:32611", + "bounds": [500000.0, 5200000.0, 501000.0, 5201000.0], + }, + ) + inv["columns"] = [c.model_dump() for c in CHM_INVENTORY_COLUMNS] + firestore_client.collection(INVENTORIES_COLLECTION).document(inv["id"]).set(inv) + cleanup_inventories.append(inv["id"]) + return inv + + MINIMAL_MODIFICATIONS_BODY = { "modifications": [ { @@ -383,6 +410,62 @@ def test_divide_by_zero_returns_422( ) assert response.status_code == 422 + def test_modification_referencing_missing_column_returns_422( + self, client, domain_for_testing, no_dbh_inventory + ): + """A rule that filters on dbh can't apply to a CHM inventory with no dbh. + The error names what was required versus what the inventory provides.""" + response = client.post( + self.route(domain_for_testing["id"], no_dbh_inventory["id"]), + json={ + "modifications": [ + { + "conditions": { + "attribute": "dbh", + "operator": "lt", + "value": 5.0, + }, + "actions": {"modifier": "remove"}, + } + ] + }, + ) + assert response.status_code == 422 + detail = response.json()["detail"] + assert "Required column(s)" in detail + assert "Available column(s)" in detail + # dbh is the referenced-but-missing column: present in the required + # portion of the message but absent from the inventory's available set. + required_part, available_part = detail.split("Available column(s)") + assert "dbh" in required_part + assert "dbh" not in available_part + + def test_modification_referencing_present_column_succeeds( + self, client, domain_for_testing, no_dbh_inventory + ): + """The guard doesn't over-reject: a rule that only references height + applies fine to a position-and-height-only inventory.""" + response = client.post( + self.route(domain_for_testing["id"], no_dbh_inventory["id"]), + json={ + "modifications": [ + { + "conditions": { + "attribute": "height", + "operator": "gt", + "value": 40.0, + }, + "actions": { + "attribute": "height", + "modifier": "multiply", + "value": 0.9, + }, + } + ] + }, + ) + assert response.status_code == 200, response.json() + # Feature-based spatial conditions (issue #276 — end-to-end through the # in-place modifications endpoint). diff --git a/services/api/tests/resources/inventories/tree/chm/test_router.py b/services/api/tests/resources/inventories/tree/chm/test_router.py index fd601afd..81ffaac2 100644 --- a/services/api/tests/resources/inventories/tree/chm/test_router.py +++ b/services/api/tests/resources/inventories/tree/chm/test_router.py @@ -345,3 +345,60 @@ def test_invalid_algorithm_name_returns_422( "discriminator" in str(error).lower() or "tag" in str(error).lower() for error in detail ) + + def test_create_time_modification_referencing_dbh_returns_422( + self, client, domain_for_testing, chm_grid_for_inventory + ): + """A create-time modification that filters on dbh is rejected at the + boundary — a CHM inventory only ever carries position and height.""" + response = client.post( + self.route(domain_for_testing["id"]), + json={ + "source_chm_grid_id": chm_grid_for_inventory["id"], + "modifications": [ + { + "conditions": { + "attribute": "dbh", + "operator": "lt", + "value": 5.0, + }, + "actions": {"modifier": "remove"}, + } + ], + }, + ) + assert response.status_code == 422 + detail = response.json()["detail"] + assert "Available column(s)" in detail + # dbh is the referenced-but-missing column: it appears in the required + # portion of the message but not in the CHM inventory's available set. + required_part, available_part = detail.split("Available column(s)") + assert "dbh" in required_part + assert "dbh" not in available_part + + def test_create_time_modification_referencing_height_succeeds( + self, client, domain_for_testing, chm_grid_for_inventory + ): + """The guard doesn't over-reject: a create-time modification that only + references height is accepted (height is in CHM's column set).""" + response = client.post( + self.route(domain_for_testing["id"]), + json={ + "source_chm_grid_id": chm_grid_for_inventory["id"], + "modifications": [ + { + "conditions": { + "attribute": "height", + "operator": "gt", + "value": 40.0, + }, + "actions": { + "attribute": "height", + "modifier": "multiply", + "value": 0.9, + }, + } + ], + }, + ) + assert response.status_code == 201 diff --git a/services/lib/lib/inventory.py b/services/lib/lib/inventory.py new file mode 100644 index 00000000..d402ca6a --- /dev/null +++ b/services/lib/lib/inventory.py @@ -0,0 +1,27 @@ +"""Canonical tree-inventory column names, shared across services. + +A tree inventory's measurement roles are a fixed, known vocabulary: each role +has one canonical column name. The upload boundary normalizes arbitrary source +labels into these names (e.g. a file column ``DIA`` becomes ``dbh``), so +downstream consumers can rely on them directly. + +Declared here once so the API guards, the voxelizer (treevox), and stand +generation (standgen) share a single source of truth instead of re-asserting +string literals like ``"dbh"`` in each service. Stdlib-only so the GDAL-free API +can import it. +""" + +X = "x" +Y = "y" +HEIGHT = "height" +DIAMETER = "dbh" +SPECIES = "fia_species_code" +STATUS = "fia_status_code" +CROWN_RATIO = "crown_ratio" + +# Columns required to voxelize a tree inventory: position, the per-tree +# measurements the crown-profile and biomass-allometry models read, and the +# status code used to keep only live trees. +VOXELIZE_REQUIRED_COLUMNS = frozenset( + {X, Y, HEIGHT, DIAMETER, SPECIES, STATUS, CROWN_RATIO} +) diff --git a/services/standgen/standgen/handlers/modifications.py b/services/standgen/standgen/handlers/modifications.py index 525eb4d9..23a56d7e 100644 --- a/services/standgen/standgen/handlers/modifications.py +++ b/services/standgen/standgen/handlers/modifications.py @@ -10,9 +10,11 @@ import geopandas as gpd +from lib.errors import ProcessingError from standgen.modifications import ( _has_spatial_condition, apply_modifications, + referenced_columns, resolve_spatial_conditions, ) from standgen.storage import load_inventory_parquet, save_parquet_replace @@ -47,6 +49,27 @@ def apply_in_place_modifications( progress("Loading inventory...", 10) ddf = load_inventory_parquet(inventory_id) + # Rules filter/transform tree attributes. The API rejects rules referencing + # an absent column at request time from the document's column metadata; this + # checks the actual Parquet schema (no compute) so a stale document fails + # with an actionable error instead of a mid-write KeyError — or, worse, a + # `replace` action silently materializing an all-null column (the exact + # "absence becomes silently wrong data" this guard exists to prevent). + missing = sorted(referenced_columns(modifications) - set(ddf.columns)) + if missing: + raise ProcessingError( + code="MISSING_COLUMNS", + message=( + f"Modification references column(s) not present in inventory " + f"{inventory_id}'s data: {missing}." + ), + suggestion=( + "Modifications can only reference columns the inventory carries. " + "A position-and-height-only inventory (e.g. from CHM/ITD " + "extraction) has no dbh, species, status, or crown-ratio columns." + ), + ) + # Apply only the new delta. Resolve spatial-condition geometries once here # (off the per-partition path) when any are present. progress("Applying modifications...", 40) diff --git a/services/standgen/standgen/modifications.py b/services/standgen/standgen/modifications.py index 82967148..5e596c23 100644 --- a/services/standgen/standgen/modifications.py +++ b/services/standgen/standgen/modifications.py @@ -8,6 +8,7 @@ pint unit conversion. """ +import ast import copy import json import logging @@ -77,6 +78,35 @@ def convert_value(attribute: str, value, unit: str | None): return converted +def referenced_columns(modifications: list[dict]) -> set[str]: + """Return the inventory column names a list of modification dicts references. + + Mirrors the API's ``modification_referenced_columns`` for the Firestore-dict + shape standgen consumes: attribute conditions/actions contribute their + ``attribute``; expression conditions contribute every name used in the + expression. Spatial conditions and remove actions reference no measurement + column. Used by the handler to fail fast (with an actionable error) before a + rule touches a column the inventory's Parquet doesn't have. + """ + columns: set[str] = set() + for mod in modifications: + for cond in mod.get("conditions", []): + if cond.get("source") in ("geometry", "feature"): + continue + if "expression" in cond: + tree = ast.parse(cond["expression"], mode="eval") + columns.update( + node.id for node in ast.walk(tree) if isinstance(node, ast.Name) + ) + elif "attribute" in cond: + columns.add(cond["attribute"]) + for action in mod.get("actions", []): + attribute = action.get("attribute") + if attribute is not None: + columns.add(attribute) + return columns + + def apply_modifications(df: pd.DataFrame, modifications: list[dict]) -> pd.DataFrame: """Apply a list of modifications to a pandas DataFrame. diff --git a/services/standgen/standgen/treatments.py b/services/standgen/standgen/treatments.py index 6ee9d814..65d7c7d0 100644 --- a/services/standgen/standgen/treatments.py +++ b/services/standgen/standgen/treatments.py @@ -37,6 +37,7 @@ from lib.config import SUPPORT_EMAIL from lib.errors import ProcessingError +from lib.inventory import DIAMETER from standgen.modifications import build_condition_mask logger = logging.getLogger(__name__) @@ -44,8 +45,10 @@ ureg = pint.UnitRegistry() Q_ = ureg.Quantity -# v2 schema diameter column (fastfuels-core defaults to "DIA"). -DIA_COLUMN = "dbh" +# v2 schema diameter column (fastfuels-core defaults to "DIA"). Sourced from the +# shared canonical vocabulary so the API guard, treevox, and standgen agree on +# what "diameter" is. +DIA_COLUMN = DIAMETER # Native unit per metric — the unit `value` is stored/validated in at create time. NATIVE_UNITS = { diff --git a/services/standgen/tests/handlers/test_modifications.py b/services/standgen/tests/handlers/test_modifications.py index 2b31b7c6..4c00d5ca 100644 --- a/services/standgen/tests/handlers/test_modifications.py +++ b/services/standgen/tests/handlers/test_modifications.py @@ -16,6 +16,8 @@ from shapely.geometry import box from standgen.handlers.modifications import apply_in_place_modifications +from lib.errors import ProcessingError + @pytest.fixture def sample_ddf(): @@ -191,3 +193,41 @@ def test_multiple_pending_modifications_applied_sequentially( result_df = mock_save.call_args[0][1].compute() assert (result_df["dbh"] >= 3.0).all() assert len(result_df) < len(sample_ddf.compute()) + + @patch("standgen.handlers.modifications.save_parquet_replace") + @patch("standgen.handlers.modifications.load_inventory_parquet") + def test_rule_referencing_absent_column_raises_missing_columns( + self, mock_load, mock_save, domain_gdf + ): + """Backstop: if the actual Parquet lacks a column the rule references + (e.g. a stale document claims dbh but the data is height-only), fail with + an actionable MISSING_COLUMNS error instead of a mid-write KeyError or a + silently materialized all-null column.""" + height_only = dd.from_pandas( + pd.DataFrame({"x": [500000.0], "y": [5200000.0], "height": [12.0]}), + npartitions=1, + ) + inventory = { + "id": "inventory-id", + "domain_id": "domain-123", + "georeference": {"crs": "EPSG:32611", "bounds": [0, 0, 1, 1]}, + "source": {"name": "chm"}, + "modifications": [], + "pending_modifications": [ + { + "conditions": [ + {"attribute": "dbh", "operator": "lt", "value": 5.0} + ], + "actions": [{"modifier": "remove"}], + } + ], + } + mock_load.return_value = height_only + progress = MagicMock() + + with pytest.raises(ProcessingError) as exc: + apply_in_place_modifications(inventory, domain_gdf, progress) + assert exc.value.code == "MISSING_COLUMNS" + assert "dbh" in exc.value.message + # The guard fires before any write. + mock_save.assert_not_called() diff --git a/services/standgen/tests/test_modifications.py b/services/standgen/tests/test_modifications.py index f8831425..4e4d5e6d 100644 --- a/services/standgen/tests/test_modifications.py +++ b/services/standgen/tests/test_modifications.py @@ -22,6 +22,7 @@ evaluate_attribute_condition, evaluate_expression, evaluate_spatial_condition, + referenced_columns, resolve_spatial_conditions, ) @@ -134,6 +135,37 @@ def test_math_expression(self, sample_df): assert mask.tolist() == [True, False, False, True, False] +class TestReferencedColumns: + def test_attribute_condition_and_action(self): + mods = [ + { + "conditions": [{"attribute": "dbh", "operator": "lt", "value": 5.0}], + "actions": [ + {"attribute": "height", "modifier": "multiply", "value": 0.9} + ], + } + ] + assert referenced_columns(mods) == {"dbh", "height"} + + def test_expression_condition_collects_all_names(self): + mods = [ + { + "conditions": [{"expression": "height * crown_ratio < 1.0"}], + "actions": [{"modifier": "remove"}], + } + ] + assert referenced_columns(mods) == {"height", "crown_ratio"} + + def test_spatial_condition_and_remove_action_reference_nothing(self): + mods = [ + { + "conditions": [{"source": "feature", "feature_id": "f1"}], + "actions": [{"modifier": "remove"}], + } + ] + assert referenced_columns(mods) == set() + + class TestBuildConditionMask: def test_single_condition(self, sample_df): conditions = [{"attribute": "dbh", "operator": "lt", "value": 5.0}] diff --git a/services/treevox/tests/test_inventory_io.py b/services/treevox/tests/test_inventory_io.py index 5782cf64..2634cbf7 100644 --- a/services/treevox/tests/test_inventory_io.py +++ b/services/treevox/tests/test_inventory_io.py @@ -12,12 +12,37 @@ from treevox import inventory_io from treevox.errors import ProcessingError from treevox.inventory_io import ( - REQUIRED_COLUMNS, assign_tree_ids, drop_null_rows, read_inventory, ) +# Every column a full tree inventory carries (e.g. an upload or PIM expansion). +ALL_COLUMNS = [ + "x", + "y", + "fia_species_code", + "fia_status_code", + "dbh", + "height", + "crown_ratio", +] + + +def _stub_schema(monkeypatch, columns): + """Make `read_inventory`'s schema probe return ``columns`` without GCS.""" + monkeypatch.setattr(inventory_io, "_available_columns", lambda inv: set(columns)) + + +def _stub_open_raising(monkeypatch, exc): + """Make the schema probe's `gcsfs_client.open` raise ``exc``.""" + + class _FakeFS: + def open(self, *args, **kwargs): + raise exc + + monkeypatch.setattr(inventory_io, "gcsfs_client", _FakeFS()) + class TestReadInventory: def test_success_roundtrip(self, monkeypatch): @@ -41,22 +66,35 @@ def fake_read_parquet(path, columns=None, filters=None, **kwargs): captured["filters"] = filters return df_in[columns] if columns else df_in + _stub_schema(monkeypatch, ALL_COLUMNS) monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) result = read_inventory("inv123") - pd.testing.assert_frame_equal(result, df_in[REQUIRED_COLUMNS]) + assert set(result.columns) == set(ALL_COLUMNS) + assert list(result["dbh"]) == [20.0] assert captured["path"].startswith("gs://") assert captured["path"].endswith("inv123") - # Column projection and status pushdown both make it to parquet. - assert captured["columns"] == REQUIRED_COLUMNS + # Full column projection and the live-tree pushdown both reach parquet. + assert set(captured["columns"]) == set(ALL_COLUMNS) assert captured["filters"] == [("fia_status_code", "=", 1)] - def test_biomass_column_appended_to_projection(self, monkeypatch): - df_in = pd.DataFrame( - {col: [1.0] for col in REQUIRED_COLUMNS} | {"my_load": [42.0]} + def test_missing_required_column_raises_missing_columns(self, monkeypatch): + """A height-only (CHM/ITD) inventory can't be voxelized — surface that as + MISSING_COLUMNS, not the old misleading INVENTORY_NOT_FOUND.""" + _stub_schema(monkeypatch, ["x", "y", "height"]) + monkeypatch.setattr( + inventory_io.pd, + "read_parquet", + lambda *a, **k: pytest.fail("should not read data when columns missing"), ) - df_in["fia_species_code"] = [131] - df_in["fia_status_code"] = [1] + + with pytest.raises(ProcessingError) as exc: + read_inventory("chm") + assert exc.value.code == "MISSING_COLUMNS" + assert "dbh" in exc.value.message + + def test_biomass_column_appended_to_projection(self, monkeypatch): + df_in = pd.DataFrame({col: [1.0] for col in ALL_COLUMNS} | {"my_load": [42.0]}) captured: dict = {} @@ -64,20 +102,37 @@ def fake_read_parquet(path, columns=None, filters=None, **kwargs): captured["columns"] = columns return df_in[columns] + _stub_schema(monkeypatch, ALL_COLUMNS + ["my_load"]) monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) read_inventory("inv1", biomass_column="my_load") assert "my_load" in captured["columns"] + def test_missing_biomass_column_raises_missing_columns(self, monkeypatch): + """A requested biomass/crown column that isn't in the inventory is a + missing required column too.""" + _stub_schema(monkeypatch, ALL_COLUMNS) + monkeypatch.setattr( + inventory_io.pd, + "read_parquet", + lambda *a, **k: pytest.fail("should not read data when columns missing"), + ) + + with pytest.raises(ProcessingError) as exc: + read_inventory("inv1", biomass_column="my_load") + assert exc.value.code == "MISSING_COLUMNS" + assert "my_load" in exc.value.message + def test_biomass_column_already_required_not_duplicated(self, monkeypatch): - """If the biomass column name happens to collide with REQUIRED_COLUMNS, - it must not appear twice (pyarrow would reject a duplicated projection).""" + """If the biomass column name collides with a required column, it must + not appear twice (pyarrow would reject a duplicated projection).""" captured: dict = {} def fake_read_parquet(path, columns=None, filters=None, **kwargs): captured["columns"] = columns return pd.DataFrame({c: [] for c in columns}) + _stub_schema(monkeypatch, ALL_COLUMNS) monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) read_inventory("inv1", biomass_column="dbh") @@ -90,6 +145,7 @@ def fake_read_parquet(path, columns=None, filters=None, **kwargs): captured["columns"] = columns return pd.DataFrame({c: [] for c in columns}) + _stub_schema(monkeypatch, ALL_COLUMNS + ["lidar_max_radius"]) monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) read_inventory("inv1", crown_radius_column="lidar_max_radius") @@ -102,6 +158,7 @@ def fake_read_parquet(path, columns=None, filters=None, **kwargs): captured["columns"] = columns return pd.DataFrame({c: [] for c in columns}) + _stub_schema(monkeypatch, ALL_COLUMNS + ["my_load", "lidar_max_radius"]) monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) read_inventory( @@ -119,16 +176,29 @@ def fake_read_parquet(path, columns=None, filters=None, **kwargs): captured["columns"] = columns return pd.DataFrame({c: [] for c in columns}) + _stub_schema(monkeypatch, ALL_COLUMNS) monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) read_inventory("inv1", crown_radius_column="dbh") assert captured["columns"].count("dbh") == 1 - def test_missing_inventory_raises_processing_error(self, monkeypatch): - def raising(path, **kwargs): - raise FileNotFoundError(path) + def test_same_biomass_and_crown_radius_column_not_duplicated(self, monkeypatch): + """If both the biomass and max-crown-radius roles map to the same custom + column, it must appear once — pyarrow rejects a duplicated projection.""" + captured: dict = {} - monkeypatch.setattr(inventory_io.pd, "read_parquet", raising) + def fake_read_parquet(path, columns=None, filters=None, **kwargs): + captured["columns"] = columns + return pd.DataFrame({c: [] for c in columns}) + + _stub_schema(monkeypatch, ALL_COLUMNS + ["my_col"]) + monkeypatch.setattr(inventory_io.pd, "read_parquet", fake_read_parquet) + + read_inventory("inv1", biomass_column="my_col", crown_radius_column="my_col") + assert captured["columns"].count("my_col") == 1 + + def test_missing_inventory_raises_processing_error(self, monkeypatch): + _stub_open_raising(monkeypatch, FileNotFoundError("no _metadata")) with pytest.raises(ProcessingError) as exc: read_inventory("missing") @@ -136,14 +206,25 @@ def raising(path, **kwargs): def test_unexpected_io_error_also_maps_to_not_found(self, monkeypatch): """gcsfs / pyarrow may surface permission or transport errors; map all to NOT_FOUND.""" + _stub_open_raising(monkeypatch, PermissionError("denied")) + + with pytest.raises(ProcessingError) as exc: + read_inventory("x") + assert exc.value.code == "INVENTORY_NOT_FOUND" - def raising(path, **kwargs): - raise PermissionError("denied") + def test_data_read_failure_maps_to_not_found(self, monkeypatch): + """The footer probe can succeed (all columns present) yet the data read + still fail — e.g. a corrupt row group or transport error mid-scan. That + branch must also map to NOT_FOUND, distinct from the footer-probe path.""" + _stub_schema(monkeypatch, ALL_COLUMNS) + + def raising(*a, **k): + raise OSError("row group read failed") monkeypatch.setattr(inventory_io.pd, "read_parquet", raising) with pytest.raises(ProcessingError) as exc: - read_inventory("x") + read_inventory("inv1") assert exc.value.code == "INVENTORY_NOT_FOUND" diff --git a/services/treevox/treevox/errors.py b/services/treevox/treevox/errors.py index 34f2caff..bf43fbe8 100644 --- a/services/treevox/treevox/errors.py +++ b/services/treevox/treevox/errors.py @@ -17,7 +17,7 @@ class ProcessingError(Exception): """Structured error with a user-friendly message. Codes emitted by treevox: - INVENTORY_NOT_FOUND, EMPTY_INVENTORY, INVALID_RESOLUTION, + INVENTORY_NOT_FOUND, MISSING_COLUMNS, EMPTY_INVENTORY, INVALID_RESOLUTION, BIOMASS_COMPONENT_NOT_IMPLEMENTED, UNKNOWN_SOURCE, VOXELIZATION_FAILED, DOMAIN_NOT_FOUND, EMPTY_DOMAIN, INVALID_GEOMETRY. """ diff --git a/services/treevox/treevox/inventory_io.py b/services/treevox/treevox/inventory_io.py index d18a931f..cb43c821 100644 --- a/services/treevox/treevox/inventory_io.py +++ b/services/treevox/treevox/inventory_io.py @@ -16,19 +16,42 @@ import numpy as np import pandas as pd +import pyarrow.parquet as pq from lib.config import INVENTORIES_BUCKET +from lib.gcs import gcsfs_client +from lib.inventory import STATUS, VOXELIZE_REQUIRED_COLUMNS from treevox.errors import ProcessingError -REQUIRED_COLUMNS = [ - "x", - "y", - "fia_species_code", - "fia_status_code", - "dbh", - "height", - "crown_ratio", -] + +def _available_columns(inventory_id: str) -> set[str]: + """Return the column names an inventory parquet carries (footer only). + + Reads just the dask-written ``_metadata`` file — no data scan — so we can + fail with an actionable ``MISSING_COLUMNS`` error instead of a misleading + ``INVENTORY_NOT_FOUND`` when a required column is absent. + """ + gcs_path = f"gs://{INVENTORIES_BUCKET}/{inventory_id}" + try: + with gcsfs_client.open( + f"{INVENTORIES_BUCKET}/{inventory_id}/_metadata", "rb" + ) as f: + return set(pq.read_schema(f).names) + except FileNotFoundError as e: + raise ProcessingError( + code="INVENTORY_NOT_FOUND", + message=f"Inventory {inventory_id} not found at {gcs_path}.", + suggestion="Verify the inventory ID exists and has completed processing.", + ) from e + except Exception as e: + # gcsfs / pyarrow can surface permission or transport errors as arbitrary + # exception types; treat any I/O failure as missing for user-facing + # purposes. + raise ProcessingError( + code="INVENTORY_NOT_FOUND", + message=f"Could not read inventory {inventory_id}: {e}", + suggestion="Verify the inventory ID exists and has completed processing.", + ) from e def read_inventory( @@ -39,34 +62,44 @@ def read_inventory( """Read a tree-inventory parquet directly from GCS with column projection and a `fia_status_code == 1` predicate pushdown. - Only `REQUIRED_COLUMNS` (plus `biomass_column` and `crown_radius_column` - if supplied) are decoded; parquet row groups containing only dead trees - are skipped when statistics permit. This avoids staging the blob on the - Cloud Run tmpfs, cuts peak memory roughly in half during load, and - transfers less data over the wire. + Only `VOXELIZE_REQUIRED_COLUMNS` (plus `biomass_column` and + `crown_radius_column` if supplied) are decoded; parquet row groups containing + only dead trees are skipped when statistics permit. This avoids staging the + blob on the Cloud Run tmpfs, cuts peak memory roughly in half during load, + and transfers less data over the wire. + + Missing required columns raise `MISSING_COLUMNS` (the API voxelize guard + already rejects such inventories; this is the backstop for direct calls). """ - gcs_path = f"gs://{INVENTORIES_BUCKET}/{inventory_id}" - columns = list(REQUIRED_COLUMNS) - for optional in (biomass_column, crown_radius_column): - if optional and optional not in columns: - columns.append(optional) + # Dedupe so a request that maps both the biomass and max-crown-radius roles + # to the same inventory column doesn't produce a duplicated parquet + # projection (pyarrow rejects those). + optional = list( + dict.fromkeys(c for c in (biomass_column, crown_radius_column) if c) + ) + required = VOXELIZE_REQUIRED_COLUMNS | set(optional) + missing = sorted(required - _available_columns(inventory_id)) + if missing: + raise ProcessingError( + code="MISSING_COLUMNS", + message=( + f"Inventory {inventory_id} is missing column(s) required for " + f"voxelization: {missing}." + ), + suggestion=( + "Voxelization needs per-tree diameter, species, height, crown " + "ratio, and status. A position-and-height-only inventory (e.g. " + "from CHM/ITD extraction) must be enriched with those first." + ), + ) + columns = sorted(VOXELIZE_REQUIRED_COLUMNS) + columns += [c for c in optional if c not in VOXELIZE_REQUIRED_COLUMNS] + + gcs_path = f"gs://{INVENTORIES_BUCKET}/{inventory_id}" try: - return pd.read_parquet( - gcs_path, - columns=columns, - filters=[("fia_status_code", "=", 1)], - ) - except FileNotFoundError as e: - raise ProcessingError( - code="INVENTORY_NOT_FOUND", - message=f"Inventory {inventory_id} not found at {gcs_path}.", - suggestion="Verify the inventory ID exists and has completed processing.", - ) from e + return pd.read_parquet(gcs_path, columns=columns, filters=[(STATUS, "=", 1)]) except Exception as e: - # gcsfs / pyarrow can surface permission or transport errors as - # arbitrary exception types; treat any I/O failure as missing for - # user-facing purposes. raise ProcessingError( code="INVENTORY_NOT_FOUND", message=f"Could not read inventory {inventory_id}: {e}", @@ -87,7 +120,7 @@ def drop_null_rows( drop individual rows missing `dbh` / `height` / `crown_ratio`. That's this function's job. """ - required = list(REQUIRED_COLUMNS) + required = list(VOXELIZE_REQUIRED_COLUMNS) for optional in (biomass_column, crown_radius_column): if optional and optional not in required: required.append(optional) diff --git a/services/uploader/tests/integration/test_inventory_upload.py b/services/uploader/tests/integration/test_inventory_upload.py index 44ea8ae4..0702249e 100644 --- a/services/uploader/tests/integration/test_inventory_upload.py +++ b/services/uploader/tests/integration/test_inventory_upload.py @@ -9,6 +9,7 @@ import json from uuid import uuid4 +import dask.dataframe as dd import gcsfs import geopandas as gpd import pandas as pd @@ -177,8 +178,6 @@ def test_valid_csv_completes(self): assert exists(f"gs://{INVENTORIES_BUCKET}/{inventory_id}") assert not exists(f"gs://{UPLOADS_BUCKET}/{object_name}") - import dask.dataframe as dd - parquet_df = dd.read_parquet( f"gs://{INVENTORIES_BUCKET}/{inventory_id}" ).compute() @@ -187,6 +186,11 @@ def test_valid_csv_completes(self): assert list(parquet_df["y"]) == SAMPLE_Y assert list(parquet_df["height"]) == SAMPLE_HEIGHT + # The Parquet carries exactly the columns the file provided — the + # uploader no longer pads missing optional columns with all-null + # placeholders, so absence stays absent downstream. + assert set(parquet_df.columns) == {"x", "y", "height"} + finally: gcs_path = f"gs://{INVENTORIES_BUCKET}/{inventory_id}" if exists(gcs_path): @@ -219,6 +223,13 @@ def test_csv_with_dbh_records_dbh_column(self): result = snap.to_dict() assert result["status"] == "completed" assert [c["key"] for c in result["columns"]] == ["x", "y", "dbh", "height"] + + # Only the provided columns are written — the provided optional (dbh) + # appears, but the optionals the file omitted are not padded in. + parquet_df = dd.read_parquet( + f"gs://{INVENTORIES_BUCKET}/{inventory_id}" + ).compute() + assert set(parquet_df.columns) == {"x", "y", "height", "dbh"} finally: gcs_path = f"gs://{INVENTORIES_BUCKET}/{inventory_id}" if exists(gcs_path): diff --git a/services/uploader/tests/unit/handlers/test_inventory.py b/services/uploader/tests/unit/handlers/test_inventory.py index 32a29d62..01336be6 100644 --- a/services/uploader/tests/unit/handlers/test_inventory.py +++ b/services/uploader/tests/unit/handlers/test_inventory.py @@ -263,12 +263,14 @@ def test_missing_x_raises(self): _validate(df) assert exc_info.value.code == "SCHEMA_VALIDATION_ERROR" - def test_missing_optional_columns_added_as_null(self): - """Missing optional columns are added as NaN and do not cause errors.""" + def test_missing_optional_columns_stay_absent(self): + """Missing optional columns are NOT padded with nulls — they stay absent + so downstream consumers can tell the data was never provided.""" df = self._make_df() result = _validate(df) - assert "fia_species_code" in result.columns - assert result["fia_species_code"].isna().all() + for col in ("fia_species_code", "fia_status_code", "dbh", "crown_ratio"): + assert col not in result.columns + assert list(result.columns) == ["x", "y", "height"] def test_optional_columns_preserved_when_present(self): """Optional columns with valid values pass validation.""" diff --git a/services/uploader/uploader/handlers/inventory.py b/services/uploader/uploader/handlers/inventory.py index 4985e3d5..9175a914 100644 --- a/services/uploader/uploader/handlers/inventory.py +++ b/services/uploader/uploader/handlers/inventory.py @@ -32,9 +32,9 @@ # Doc metadata (type, unit) per v2 column, in canonical order. Only x, y, and # height are required in an upload, so the inventory document's `columns` field -# is written from the columns the file actually provided (before _validate pads -# the optional ones with nulls) — the API's treatments endpoint relies on it to -# tell whether an inventory has a `dbh` column to thin against. +# is written from the columns the file actually provided — the API's treatments +# and voxelize endpoints rely on it to tell whether an inventory carries the +# columns an operation needs (e.g. a `dbh` column to thin against). _COLUMN_METADATA = { "x": ("continuous", "m"), "y": ("continuous", "m"), @@ -78,8 +78,9 @@ def handle_inventory( domain_crs_str = _extract_crs_string(domain_data) df = _parse(fmt, local_path, col_map, domain_crs_str) - # Record which columns the file actually provided before _validate pads - # the missing optional ones with all-null placeholders. + # Record which columns the file actually provided. _validate no longer + # pads missing optionals, so the set is stable across validation — but + # capture it here so the intent (file-provided columns only) is explicit. provided_columns = [c for c in _COLUMN_METADATA if c in df.columns] df = _validate(df) @@ -117,9 +118,9 @@ def handle_inventory( ], } # Record the columns the file actually provided. The create endpoint - # wrote a provisional full column list, and _validate pads the Parquet - # with all-null optional columns for schema compatibility — but an - # all-null dbh is not a column treatments can thin against. + # wrote a provisional full column list; overwrite it with the real set + # so the metadata matches the Parquet, which now carries exactly these + # columns (no all-null padding). columns = [ {"key": key, "type": col_type, "unit": unit} for key, (col_type, unit) in _COLUMN_METADATA.items() @@ -199,11 +200,16 @@ def _parse( def _validate(df: pd.DataFrame) -> pd.DataFrame: - """Validate the parsed DataFrame against the inventory schema.""" - for col in _V2_COLUMNS - {"x", "y", "height"}: - if col not in df.columns: - df[col] = None - + """Validate the parsed DataFrame against the inventory schema. + + Only the columns the file actually provided are written to the Parquet. + Missing optional columns stay absent rather than being padded with all-null + placeholders: absence is loud and checkable downstream (the document's + `columns` metadata records it, and consumers reject operations that need a + column the inventory doesn't have), whereas a present-but-null column is + silently wrong everywhere. Pandera passes a bare x/y/height frame because the + optional fields are `Series[...] | None`. + """ try: return _InventorySchema.validate(df, lazy=True) except pa.errors.SchemaErrors as e: From c5f8c2e2e88814f826c1bd135ae46cf0185b5ccc Mon Sep 17 00:00:00 2001 From: amarcozzi Date: Mon, 8 Jun 2026 19:43:23 -0600 Subject: [PATCH 2/2] Route inventory column names through lib.inventory; harden column-key guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the absence-guarding work (#320): - Finish the lib.inventory single-source-of-truth migration. BASE_INVENTORY_COLUMNS and CHM_INVENTORY_COLUMNS (API schema) and BASE_COLUMNS / RENAME_MAP (standgen) now reference the canonical name constants instead of re-asserting string literals. Pure refactor — the resulting (key, type, unit) sets are unchanged. - Add inventory_column_keys() to read an inventory's provided column keys from its `columns` metadata, skipping any malformed/legacy entry missing a `key`. The voxelize and in-place modifications guards now use it, so a bad metadata entry degrades to a clean 422 (column treated as absent) rather than a 500 KeyError. --- .../grids/voxelize/inventory/tree/router.py | 7 +++-- .../inventories/modifications/router.py | 3 +- .../api/api/resources/inventories/schema.py | 21 ++++++------- .../api/api/resources/inventories/utils.py | 14 +++++++++ services/standgen/standgen/columns.py | 30 ++++++++++--------- 5 files changed, 48 insertions(+), 27 deletions(-) diff --git a/services/api/api/resources/grids/voxelize/inventory/tree/router.py b/services/api/api/resources/grids/voxelize/inventory/tree/router.py index 178014d7..56c59094 100644 --- a/services/api/api/resources/grids/voxelize/inventory/tree/router.py +++ b/services/api/api/resources/grids/voxelize/inventory/tree/router.py @@ -24,7 +24,10 @@ TreeInventoryVoxelizationSource, build_tree_bands, ) -from api.resources.inventories.utils import require_inventory_columns +from api.resources.inventories.utils import ( + inventory_column_keys, + require_inventory_columns, +) from api.schema import JobStatus from api.tasks import create_http_task_async from lib.config import ( @@ -130,7 +133,7 @@ async def create_tree_inventory_grid( # voxelized until those exist. Biomass / max-crown-radius inventory-column # references are validated by treevox at read time. require_inventory_columns( - {column["key"] for column in inventory_data.get("columns", [])}, + inventory_column_keys(inventory_data), VOXELIZE_REQUIRED_COLUMNS, detail=( "This inventory lacks the per-tree measurements voxelization needs " diff --git a/services/api/api/resources/inventories/modifications/router.py b/services/api/api/resources/inventories/modifications/router.py index f3191a26..f6196f01 100644 --- a/services/api/api/resources/inventories/modifications/router.py +++ b/services/api/api/resources/inventories/modifications/router.py @@ -24,6 +24,7 @@ from api.resources.inventories.modifications.schema import ApplyModificationsRequest from api.resources.inventories.schema import Inventory from api.resources.inventories.utils import ( + inventory_column_keys, require_inventory_columns, validate_feature_conditions, ) @@ -137,7 +138,7 @@ async def apply_modifications( # `dbh > 30` on an upload or CHM inventory with no dbh). Absence is loud now # that the uploader no longer pads missing columns with nulls. require_inventory_columns( - {column["key"] for column in inventory_data.get("columns", [])}, + inventory_column_keys(inventory_data), modification_referenced_columns(body.modifications), detail="A modification references column(s) this inventory doesn't have.", ) diff --git a/services/api/api/resources/inventories/schema.py b/services/api/api/resources/inventories/schema.py index 42c1467e..4b878a55 100644 --- a/services/api/api/resources/inventories/schema.py +++ b/services/api/api/resources/inventories/schema.py @@ -14,6 +14,7 @@ from api.resources.inventories.treatment_models import InventoryTreatment from api.resources.modifications import parse_modification_coordinates from api.schema import JobError, JobProgress, JobStatus, PaginatedResponse +from lib.inventory import CROWN_RATIO, DIAMETER, HEIGHT, SPECIES, STATUS, X, Y class InventoryType(StrEnum): @@ -99,22 +100,22 @@ class Column(BaseModel): # subset: CHM produces CHM_INVENTORY_COLUMNS; uploads carry whichever optional # columns the file contains (the uploader records the actual set on completion). BASE_INVENTORY_COLUMNS = [ - Column(key="x", type=ColumnType.continuous, unit="m"), - Column(key="y", type=ColumnType.continuous, unit="m"), - Column(key="fia_species_code", type=ColumnType.categorical), - Column(key="fia_status_code", type=ColumnType.categorical), - Column(key="dbh", type=ColumnType.continuous, unit="cm"), - Column(key="height", type=ColumnType.continuous, unit="m"), - Column(key="crown_ratio", type=ColumnType.continuous), + Column(key=X, type=ColumnType.continuous, unit="m"), + Column(key=Y, type=ColumnType.continuous, unit="m"), + Column(key=SPECIES, type=ColumnType.categorical), + Column(key=STATUS, type=ColumnType.categorical), + Column(key=DIAMETER, type=ColumnType.continuous, unit="cm"), + Column(key=HEIGHT, type=ColumnType.continuous, unit="m"), + Column(key=CROWN_RATIO, type=ColumnType.continuous), ] # Columns produced by CHM stem isolation: height and position only — no dbh, # species, or crown ratio. Treatments thin against dbh, so they cannot be # applied to a CHM-derived inventory. CHM_INVENTORY_COLUMNS = [ - Column(key="x", type=ColumnType.continuous, unit="m"), - Column(key="y", type=ColumnType.continuous, unit="m"), - Column(key="height", type=ColumnType.continuous, unit="m"), + Column(key=X, type=ColumnType.continuous, unit="m"), + Column(key=Y, type=ColumnType.continuous, unit="m"), + Column(key=HEIGHT, type=ColumnType.continuous, unit="m"), ] diff --git a/services/api/api/resources/inventories/utils.py b/services/api/api/resources/inventories/utils.py index d5d8578e..f7cc0db7 100644 --- a/services/api/api/resources/inventories/utils.py +++ b/services/api/api/resources/inventories/utils.py @@ -54,6 +54,20 @@ def validate_inventory_wide_treatment_area(domain: dict, treatments: list) -> No ) +def inventory_column_keys(inventory_data: dict) -> set[str]: + """Return the column keys an inventory provides, from its ``columns`` metadata. + + Skips any malformed/legacy entry missing a ``key`` so a downstream guard + degrades to a clean 422 (the column is treated as absent) rather than a 500 + (``KeyError`` on a bad entry). + """ + return { + key + for column in inventory_data.get("columns", []) + if (key := column.get("key")) is not None + } + + def require_inventory_columns( available_keys: set[str], required: set[str], diff --git a/services/standgen/standgen/columns.py b/services/standgen/standgen/columns.py index 0492316d..7d48e4d9 100644 --- a/services/standgen/standgen/columns.py +++ b/services/standgen/standgen/columns.py @@ -1,25 +1,27 @@ """Column definitions for tree inventories.""" +from lib.inventory import CROWN_RATIO, DIAMETER, HEIGHT, SPECIES, STATUS, X, Y + # Column rename mapping: fastfuels-core output → v2 schema RENAME_MAP = { - "SPCD": "fia_species_code", - "STATUSCD": "fia_status_code", - "DIA": "dbh", - "HT": "height", - "CR": "crown_ratio", - "X": "x", - "Y": "y", + "SPCD": SPECIES, + "STATUSCD": STATUS, + "DIA": DIAMETER, + "HT": HEIGHT, + "CR": CROWN_RATIO, + "X": X, + "Y": Y, } # Columns to drop from the fastfuels-core output (internal to point process) DROP_COLUMNS = {"TREE_ID", "PLOT_ID", "TPA"} BASE_COLUMNS = [ - "x", - "y", - "fia_species_code", - "fia_status_code", - "dbh", - "height", - "crown_ratio", + X, + Y, + SPECIES, + STATUS, + DIAMETER, + HEIGHT, + CROWN_RATIO, ]