From 03acb5f2b9168198effa4eb5c221af9b38e64aa1 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Tue, 7 Oct 2025 17:24:38 -0600 Subject: [PATCH 1/7] fix: eager load active location for things when a different object needs to call a thing, like a contact, the `active_location` should be present in the thing response. this is achieved by eagerly loading the `active_location` --- api/search.py | 5 ++- api/thing.py | 2 - db/location.py | 4 +- db/thing.py | 15 +++++++- schemas/thing.py | 53 +++++++++++++++++++++----- services/geospatial_helper.py | 3 +- services/thing_helper.py | 71 ++++++++++++----------------------- tests/conftest.py | 5 +++ tests/test_geospatial.py | 4 ++ tests/test_thing.py | 62 ++++++++++++------------------ 10 files changed, 120 insertions(+), 104 deletions(-) diff --git a/api/search.py b/api/search.py index a3ff05b73..40b066cbf 100644 --- a/api/search.py +++ b/api/search.py @@ -81,8 +81,9 @@ def _get_thing_results(session: Session, q: str, limit: int) -> list[dict]: select(Thing).where(Thing.thing_type == "spring"), q, vector=vector, limit=limit ) - wells = session.scalars(water_well_query).all() - springs = session.scalars(spring_well_query).all() + # unique needs to be called because of eager loads + wells = session.scalars(water_well_query).unique().all() + springs = session.scalars(spring_well_query).unique().all() def _make_response(group: str, thing: Thing, properties: dict) -> dict: diff --git a/api/thing.py b/api/thing.py index 7e673de08..d0207f00b 100644 --- a/api/thing.py +++ b/api/thing.py @@ -62,7 +62,6 @@ add_well_screen, get_db_things, get_thing_of_a_thing_type_by_id, - get_active_location, ) from services.lexicon_helper import get_terms_by_category @@ -328,7 +327,6 @@ async def get_thing_by_id( Retrieve a thing by ID from the database. """ thing = simple_get_by_id(session, Thing, thing_id) - thing.active_location = get_active_location(session, thing) return thing diff --git a/db/location.py b/db/location.py index 6ef7ce8f5..b1113eaad 100644 --- a/db/location.py +++ b/db/location.py @@ -99,7 +99,9 @@ class LocationThingAssociation(Base, AutoBaseMixin): ) # --- Relationship Definitions --- - location: Mapped["Location"] = relationship(back_populates="thing_associations") + location: Mapped["Location"] = relationship( + back_populates="thing_associations", lazy="joined" + ) thing: Mapped["Thing"] = relationship(back_populates="location_associations") diff --git a/db/thing.py b/db/thing.py index 2ec61dff6..b84ff9d8e 100644 --- a/db/thing.py +++ b/db/thing.py @@ -56,7 +56,7 @@ class Thing(Base, AutoBaseMixin, ReleaseMixin, StatusHistoryMixin, PermissionMix # TODO: what is the purpose of the `description` field? Is it ever used? # description: Mapped[str] = mapped_column(String(500), nullable=True) thing_type: Mapped[str] = lexicon_term( - nullable=True, + nullable=False, comment="A controlled vocabulary field defining the type of infrastructure (e.g., 'Well', 'Spring', 'Stream Gauge').", ) first_visit_date: Mapped[Date] = mapped_column( @@ -126,6 +126,7 @@ class Thing(Base, AutoBaseMixin, ReleaseMixin, StatusHistoryMixin, PermissionMix cascade="all, delete-orphan", passive_deletes=True, order_by="LocationThingAssociation.effective_start.desc()", + lazy="joined", ) contact_associations = relationship( @@ -199,6 +200,18 @@ class Thing(Base, AutoBaseMixin, ReleaseMixin, StatusHistoryMixin, PermissionMix ) ) + @property + def active_location(self): + """ + Returns the currently active Location by sorting the effective_start + field. Thing eagerly loads location_association, which eagerly loads + location, which will hopefully prevent N+1 query problems. + """ + active_location = sorted( + self.location_associations, key=lambda x: x.effective_start + ) + return active_location[0].location if active_location else None + class ThingIdLink(Base, AutoBaseMixin, ReleaseMixin): """ diff --git a/schemas/thing.py b/schemas/thing.py index fc28b3558..4103e9a50 100644 --- a/schemas/thing.py +++ b/schemas/thing.py @@ -20,6 +20,35 @@ from schemas import BaseCreateModel, BaseUpdateModel, BaseResponseModel from schemas.location import LocationResponse +# -------- VALIDATE ---------- + + +class ValidateWell(BaseModel): + well_depth: float | None = None # in feet + hole_depth: float | None = None # in feet + well_casing_depth: float | None = None # in feet + + @model_validator(mode="after") + def check_depths(self): + # TODO: ask AMP if hole depth is needed if well depth is provided + # TODO: ask AMP if well depth is needed if well casing depth is provided + if ( + self.well_depth is not None + and self.hole_depth is not None + and self.well_depth > self.hole_depth + ): + raise ValueError("well depth must be less than than or equal to hole depth") + elif ( + self.well_depth is not None + and self.well_casing_depth is not None + and self.well_casing_depth > self.well_depth + ): + raise ValueError( + "well casing depth must be greater than or equal to well depth" + ) + + return self + # -------- CREATE ---------- class CreateThingIdLink(BaseModel): @@ -43,13 +72,13 @@ class CreateBaseThing(BaseCreateModel): e.g. POST /thing/water-well, POST /thing/spring determines the thing_type """ - location_id: int | None = None # Optional location ID for the thing + location_id: int | None group_id: int | None = None # Optional group ID for the thing name: str # Name of the thing first_visit_date: PastDate # Date of NMBGMR's first visit -class CreateWell(CreateBaseThing): +class CreateWell(CreateBaseThing, ValidateWell): """ Schema for creating a well. """ @@ -58,6 +87,9 @@ class CreateWell(CreateBaseThing): well_depth: float | None = None # in feet hole_depth: float | None = None # in feet well_construction_notes: str | None = None + well_casing_diameter: float | None = None # in inches + well_casing_depth: float | None = None # in feet + well_casing_material: str | None = None class CreateSpring(CreateBaseThing): @@ -93,8 +125,8 @@ def check_depths(self): class BaseThingResponse(BaseResponseModel): name: str thing_type: str - active_location: LocationResponse | None = None - first_visit_date: PastDate | None = None + active_location: LocationResponse | None + first_visit_date: PastDate | None class WellResponse(BaseThingResponse): @@ -102,15 +134,13 @@ class WellResponse(BaseThingResponse): Response schema for well details. """ - # api_id: str | None = None - # ose_pod_id: str | None = None - # usgs_id: str | None = None - well_purpose: str | None = None # e.g., "Production", "Observation", etc. well_depth: float | None = None # in feet hole_depth: float | None = None # in feet well_construction_notes: str | None = None - # Additional fields can be added as needed + well_casing_diameter: float | None = None # in inches + well_casing_depth: float | None = None # in feet + well_casing_material: str | None = None class SpringResponse(BaseThingResponse): @@ -197,12 +227,15 @@ class UpdateThing(BaseUpdateModel): first_visit_date: PastDate | None = None # Date of NMBGMR's first visit -class UpdateWell(UpdateThing): +class UpdateWell(UpdateThing, ValidateWell): well_purpose: str | None = None well_depth: float | None = None # in feet hole_depth: float | None = None # in feet well_construction_notes: str | None = None + well_casing_diameter: float | None = None # in inches + well_casing_depth: float | None = None # in feet + well_casing_material: str | None = None class UpdateSpring(UpdateThing): diff --git a/services/geospatial_helper.py b/services/geospatial_helper.py index 0cebfe640..0279d6d24 100644 --- a/services/geospatial_helper.py +++ b/services/geospatial_helper.py @@ -65,7 +65,8 @@ def get_thing_features( else: sql = sql.where(Group.id == group) - return session.execute(sql).all() + # unique needs to be invoked to prevent duplicates from eager loading + return session.execute(sql).unique().all() def create_shapefile(things: list, filename: str = "things.shp") -> None: diff --git a/services/thing_helper.py b/services/thing_helper.py index 5d88cd564..d737f2343 100644 --- a/services/thing_helper.py +++ b/services/thing_helper.py @@ -38,21 +38,6 @@ def wkb_to_geojson(wkb_element): return mapping(geom) -def get_active_location(session: Session, thing: Thing) -> Location | None: - """ - The following SQL query retrieves the active location associated with by - assuming that the latest effective_start is the active location. - """ - sql = ( - select(Location) - .join(LocationThingAssociation) - .where(LocationThingAssociation.thing_id == thing.id) - .order_by(LocationThingAssociation.effective_start.desc()) - ) - active_location = session.execute(sql).scalars().one_or_none() - return active_location - - def get_db_things( filter_, order, @@ -63,47 +48,39 @@ def get_db_things( within: str = None, ) -> list: - latest_assoc = ( - select( - LocationThingAssociation.thing_id, - func.max(LocationThingAssociation.effective_start).label("max_start"), - ) - .group_by(LocationThingAssociation.thing_id) - .subquery() - ) - if query: - sql = select(Thing, Location).where(make_query(Thing, query)) + sql = select(Thing).where(make_query(Thing, query)) else: - sql = select(Thing, Location) - - lta_alias = aliased(LocationThingAssociation) - sql = ( - sql.join(lta_alias, Thing.id == lta_alias.thing_id) - .join(Location, lta_alias.location_id == Location.id) - .join( - latest_assoc, - (latest_assoc.c.thing_id == lta_alias.thing_id) - & (latest_assoc.c.max_start == lta_alias.effective_start), - ) - ) + sql = select(Thing) if thing_type: sql = sql.where(Thing.thing_type == thing_type) if within: + latest_assoc = ( + select( + LocationThingAssociation.thing_id, + func.max(LocationThingAssociation.effective_start).label("max_start"), + ) + .group_by(LocationThingAssociation.thing_id) + .subquery() + ) + + lta_alias = aliased(LocationThingAssociation) + sql = ( + sql.join(lta_alias, Thing.id == lta_alias.thing_id) + .join(Location, lta_alias.location_id == Location.id) + .join( + latest_assoc, + (latest_assoc.c.thing_id == lta_alias.thing_id) + & (latest_assoc.c.max_start == lta_alias.effective_start), + ) + ) sql = make_within_wkt(sql, within) sql = order_sort_filter(sql, Thing, sort, order, filter_) - def transformer(records): - def make_new_record(thing, location): - thing.active_location = location - return thing - - return [make_new_record(*record) for record in records] - - return paginate(query=sql, conn=session, transformer=transformer) + return paginate(query=sql, conn=session) def get_thing_type_from_request(request: Request) -> str: @@ -141,7 +118,6 @@ def get_thing_of_a_thing_type_by_id(session: Session, request: Request, thing_id verify_thing_type_correspondence(thing, request) - thing.active_location = get_active_location(session, thing) return thing @@ -187,11 +163,11 @@ def add_thing( session.add(assoc) session.commit() + session.refresh(thing) except Exception as e: session.rollback() raise e - thing.active_location = get_active_location(session, thing) return thing @@ -237,7 +213,6 @@ def patch_thing( verify_thing_type_correspondence(thing, request) thing = model_patcher(session, Thing, thing_id, payload, user) - thing.active_location = get_active_location(session, thing) return thing diff --git a/tests/conftest.py b/tests/conftest.py index 74f5c0858..b3d7cd1d4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -59,6 +59,9 @@ def water_well_thing(location): well_depth=10, hole_depth=10, well_construction_notes="Test well construction notes", + well_casing_diameter=5.0, + well_casing_depth=10.0, + well_casing_material="PVC", ) session.add(water_well) session.commit() @@ -70,6 +73,8 @@ def water_well_thing(location): assoc.effective_start = "2025-02-01T00:00:00Z" session.add(assoc) session.commit() + session.refresh(water_well) + session.refresh(assoc) yield water_well session.delete(water_well) session.delete(assoc) diff --git a/tests/test_geospatial.py b/tests/test_geospatial.py index d8ff95e14..4d7a288db 100644 --- a/tests/test_geospatial.py +++ b/tests/test_geospatial.py @@ -195,6 +195,10 @@ def test_get_within_things(): }, ) data = response.json() + print() + from pprint import pprint + + pprint(data, indent=2) assert response.status_code == 200 assert "items" in data assert len(data["items"]) == 1 diff --git a/tests/test_thing.py b/tests/test_thing.py index b5470458d..87af3d471 100644 --- a/tests/test_thing.py +++ b/tests/test_thing.py @@ -65,6 +65,9 @@ def test_add_water_well(location, group): "well_depth": 100.0, "hole_depth": 110, "well_construction_notes": "this is a test of notes", + "well_casing_diameter": 5.0, + "well_casing_depth": 10.0, + "well_casing_material": "PVC", } response = client.post("/thing/water-well", json=payload) @@ -80,6 +83,9 @@ def test_add_water_well(location, group): assert data["hole_depth"] == payload["hole_depth"] assert data["well_depth"] == payload["well_depth"] assert data["well_construction_notes"] == payload["well_construction_notes"] + assert data["well_casing_diameter"] == payload["well_casing_diameter"] + assert data["well_casing_depth"] == payload["well_casing_depth"] + assert data["well_casing_material"] == payload["well_casing_material"] expected_location = LocationResponse.model_validate(location).model_dump() expected_location["created_at"] = ( @@ -102,6 +108,9 @@ def test_add_water_well_409_bad_group_id(location): "well_depth": 100.0, "hole_depth": 110, "well_construction_notes": "this is a test of notes", + "well_casing_diameter": 5.0, + "well_casing_depth": 10.0, + "well_casing_material": "PVC", } response = client.post("/thing/water-well", json=payload) @@ -350,6 +359,16 @@ def test_get_water_wells(water_well_thing, location): data["items"][0]["well_construction_notes"] == water_well_thing.well_construction_notes ) + assert ( + data["items"][0]["well_casing_diameter"] + == water_well_thing.well_casing_diameter + ) + assert data["items"][0]["well_casing_depth"] == water_well_thing.well_casing_depth + assert ( + data["items"][0]["well_casing_material"] + == water_well_thing.well_casing_material + ) + expected_location = LocationResponse.model_validate(location).model_dump() expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") @@ -373,6 +392,10 @@ def test_get_water_well_by_id(water_well_thing, location): assert data["well_depth"] == water_well_thing.well_depth assert data["hole_depth"] == water_well_thing.hole_depth assert data["well_construction_notes"] == water_well_thing.well_construction_notes + assert data["well_casing_diameter"] == water_well_thing.well_casing_diameter + assert data["well_casing_depth"] == water_well_thing.well_casing_depth + assert data["well_casing_material"] == water_well_thing.well_casing_material + expected_location = LocationResponse.model_validate(location).model_dump() expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") @@ -623,45 +646,6 @@ def test_get_things(water_well_thing, spring_thing, location): data = response.json() assert data["total"] == 2 - assert data["items"][0]["id"] == water_well_thing.id - assert data["items"][0][ - "created_at" - ] == water_well_thing.created_at.isoformat().replace("+00:00", "Z") - assert data["items"][0]["name"] == water_well_thing.name - assert ( - data["items"][0]["first_visit_date"] - == water_well_thing.first_visit_date.isoformat() - ) - assert data["items"][0]["thing_type"] == water_well_thing.thing_type - assert data["items"][0]["release_status"] == water_well_thing.release_status - assert data["items"][0]["well_purpose"] == water_well_thing.well_purpose - assert data["items"][0]["well_depth"] == water_well_thing.well_depth - assert data["items"][0]["hole_depth"] == water_well_thing.hole_depth - assert ( - data["items"][0]["well_construction_notes"] - == water_well_thing.well_construction_notes - ) - assert data["items"][0]["spring_type"] is None - assert data["items"][0]["active_location"] == expected_location - - assert data["items"][1]["id"] == spring_thing.id - assert data["items"][1][ - "created_at" - ] == spring_thing.created_at.isoformat().replace("+00:00", "Z") - assert data["items"][1]["name"] == spring_thing.name - assert ( - data["items"][1]["first_visit_date"] - == spring_thing.first_visit_date.isoformat() - ) - assert data["items"][1]["thing_type"] == spring_thing.thing_type - assert data["items"][1]["release_status"] == spring_thing.release_status - assert data["items"][1]["spring_type"] == spring_thing.spring_type - assert data["items"][1]["well_purpose"] is None - assert data["items"][1]["well_depth"] is None - assert data["items"][1]["hole_depth"] is None - assert data["items"][1]["well_construction_notes"] is None - assert data["items"][1]["active_location"] == expected_location - def test_get_thing_by_id(water_well_thing, location): response = client.get(f"/thing/{water_well_thing.id}") From 1f9eca022229886d52efe34b4e14026535efadf6 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 8 Oct 2025 11:19:06 -0600 Subject: [PATCH 2/7] feat: add depth validations for a well --- schemas/thing.py | 10 ++++++++-- tests/test_thing.py | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/schemas/thing.py b/schemas/thing.py index 4103e9a50..33246ea1e 100644 --- a/schemas/thing.py +++ b/schemas/thing.py @@ -30,8 +30,6 @@ class ValidateWell(BaseModel): @model_validator(mode="after") def check_depths(self): - # TODO: ask AMP if hole depth is needed if well depth is provided - # TODO: ask AMP if well depth is needed if well casing depth is provided if ( self.well_depth is not None and self.hole_depth is not None @@ -46,6 +44,14 @@ def check_depths(self): raise ValueError( "well casing depth must be greater than or equal to well depth" ) + elif ( + self.hole_depth is not None + and self.well_casing_depth is not None + and self.well_casing_depth > self.hole_depth + ): + raise ValueError( + "well casing depth must be less than or equal to hole depth" + ) return self diff --git a/tests/test_thing.py b/tests/test_thing.py index 87af3d471..95170bdf5 100644 --- a/tests/test_thing.py +++ b/tests/test_thing.py @@ -27,6 +27,7 @@ amp_viewer_function, ) from schemas.location import LocationResponse +from schemas.thing import ValidateWell @pytest.fixture(scope="module", autouse=True) @@ -51,6 +52,32 @@ def override_authentication_dependency_fixture(): app.dependency_overrides = {} +# VALIDATE tests =============================================================== + + +def test_validate_well_depth_hole_depth(): + with pytest.raises( + ValueError, match="well depth must be less than than or equal to hole depth" + ): + ValidateWell(well_depth=100.0, hole_depth=90.0) + + +def test_validate_well_depth_casing_depth(): + with pytest.raises( + ValueError, + match="well casing depth must be greater than or equal to well depth", + ): + ValidateWell(well_depth=100.0, well_casing_depth=110.0) + + +def test_validate_hole_depth_casing_depth(): + with pytest.raises( + ValueError, + match="well casing depth must be less than or equal to hole depth", + ): + ValidateWell(hole_depth=100.0, well_casing_depth=110.0) + + # POST tests =================================================================== From 017182199431f38f4d3af133a5606c1416076a05 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 8 Oct 2025 13:18:12 -0600 Subject: [PATCH 3/7] feat: add units in well response | describe depth fields in well create --- db/thing.py | 8 ++++++-- schemas/thing.py | 36 +++++++++++++++++++++++++----------- tests/test_thing.py | 12 ++++++++++++ 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/db/thing.py b/db/thing.py index b84ff9d8e..6aa8afc5b 100644 --- a/db/thing.py +++ b/db/thing.py @@ -208,9 +208,13 @@ def active_location(self): location, which will hopefully prevent N+1 query problems. """ active_location = sorted( - self.location_associations, key=lambda x: x.effective_start + self.location_associations, key=lambda x: x.effective_start, reverse=True + ) + return ( + active_location[0].location + if active_location and active_location[0].effective_end is None + else None ) - return active_location[0].location if active_location else None class ThingIdLink(Base, AutoBaseMixin, ReleaseMixin): diff --git a/schemas/thing.py b/schemas/thing.py index 33246ea1e..781f3a162 100644 --- a/schemas/thing.py +++ b/schemas/thing.py @@ -15,7 +15,7 @@ # =============================================================================== from typing import List -from pydantic import BaseModel, model_validator, PastDate +from pydantic import BaseModel, model_validator, PastDate, Field from schemas import BaseCreateModel, BaseUpdateModel, BaseResponseModel from schemas.location import LocationResponse @@ -90,11 +90,19 @@ class CreateWell(CreateBaseThing, ValidateWell): """ well_purpose: str | None = None - well_depth: float | None = None # in feet - hole_depth: float | None = None # in feet + well_depth: float | None = Field( + default=None, gt=0, description="Well depth in feet" + ) + hole_depth: float | None = Field( + default=None, gt=0, description="Hole depth in feet" + ) well_construction_notes: str | None = None - well_casing_diameter: float | None = None # in inches - well_casing_depth: float | None = None # in feet + well_casing_diameter: float | None = Field( + default=None, gt=0, description="Well casing diameter in inches" + ) + well_casing_depth: float | None = Field( + default=None, gt=0, description="Well casing depth in feet" + ) well_casing_material: str | None = None @@ -112,8 +120,8 @@ class CreateWellScreen(BaseCreateModel): """ thing_id: int - screen_depth_bottom: float - screen_depth_top: float + screen_depth_bottom: float = Field(gt=0, description="Screen depth bottom in feet") + screen_depth_top: float = Field(gt=0, description="Screen depth top in feet") screen_type: str | None = None screen_description: str | None = None @@ -141,12 +149,16 @@ class WellResponse(BaseThingResponse): """ well_purpose: str | None = None # e.g., "Production", "Observation", etc. - well_depth: float | None = None # in feet - hole_depth: float | None = None # in feet - well_construction_notes: str | None = None + well_depth: float | None = None + well_depth_unit: str = "ft" + hole_depth: float | None = None + hole_depth_unit: str = "ft" well_casing_diameter: float | None = None # in inches - well_casing_depth: float | None = None # in feet + well_casing_diameter_unit: str = "in" + well_casing_depth: float | None = None + well_casing_depth_unit: str = "ft" well_casing_material: str | None = None + well_construction_notes: str | None = None class SpringResponse(BaseThingResponse): @@ -185,7 +197,9 @@ class WellScreenResponse(BaseResponseModel): thing_id: int thing: WellResponse screen_depth_bottom: float + screen_depth_bottom_unit: str = "ft" screen_depth_top: float + screen_depth_top_unit: str = "ft" screen_type: str | None = None screen_description: str | None = None diff --git a/tests/test_thing.py b/tests/test_thing.py index 95170bdf5..4a17269a7 100644 --- a/tests/test_thing.py +++ b/tests/test_thing.py @@ -108,10 +108,14 @@ def test_add_water_well(location, group): assert data["thing_type"] == "water well" assert data["well_purpose"] == payload["well_purpose"] assert data["hole_depth"] == payload["hole_depth"] + assert data["hole_depth_unit"] == "ft" assert data["well_depth"] == payload["well_depth"] + assert data["well_depth_unit"] == "ft" assert data["well_construction_notes"] == payload["well_construction_notes"] assert data["well_casing_diameter"] == payload["well_casing_diameter"] + assert data["well_casing_diameter_unit"] == "in" assert data["well_casing_depth"] == payload["well_casing_depth"] + assert data["well_casing_depth_unit"] == "ft" assert data["well_casing_material"] == payload["well_casing_material"] expected_location = LocationResponse.model_validate(location).model_dump() @@ -381,7 +385,9 @@ def test_get_water_wells(water_well_thing, location): assert data["items"][0]["release_status"] == water_well_thing.release_status assert data["items"][0]["well_purpose"] == water_well_thing.well_purpose assert data["items"][0]["well_depth"] == water_well_thing.well_depth + assert data["items"][0]["well_depth_unit"] == "ft" assert data["items"][0]["hole_depth"] == water_well_thing.hole_depth + assert data["items"][0]["hole_depth_unit"] == "ft" assert ( data["items"][0]["well_construction_notes"] == water_well_thing.well_construction_notes @@ -390,7 +396,9 @@ def test_get_water_wells(water_well_thing, location): data["items"][0]["well_casing_diameter"] == water_well_thing.well_casing_diameter ) + assert data["items"][0]["well_casing_diameter_unit"] == "in" assert data["items"][0]["well_casing_depth"] == water_well_thing.well_casing_depth + assert data["items"][0]["well_casing_depth_unit"] == "ft" assert ( data["items"][0]["well_casing_material"] == water_well_thing.well_casing_material @@ -417,10 +425,14 @@ def test_get_water_well_by_id(water_well_thing, location): assert data["release_status"] == water_well_thing.release_status assert data["well_purpose"] == water_well_thing.well_purpose assert data["well_depth"] == water_well_thing.well_depth + assert data["well_depth_unit"] == "ft" assert data["hole_depth"] == water_well_thing.hole_depth + assert data["hole_depth_unit"] == "ft" assert data["well_construction_notes"] == water_well_thing.well_construction_notes assert data["well_casing_diameter"] == water_well_thing.well_casing_diameter + assert data["well_casing_diameter_unit"] == "in" assert data["well_casing_depth"] == water_well_thing.well_casing_depth + assert data["well_casing_depth_unit"] == "ft" assert data["well_casing_material"] == water_well_thing.well_casing_material expected_location = LocationResponse.model_validate(location).model_dump() From 4ef32fe42846c37e21925b98ff7fd449ca8951ea Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 8 Oct 2025 14:51:16 -0600 Subject: [PATCH 4/7] note: add note about eager loading of location associations --- db/thing.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/db/thing.py b/db/thing.py index 6aa8afc5b..459d7811d 100644 --- a/db/thing.py +++ b/db/thing.py @@ -119,6 +119,43 @@ class Thing(Base, AutoBaseMixin, ReleaseMixin, StatusHistoryMixin, PermissionMix # One-To-Many: A Thing can be at many locations over time. # If the Thing is deleted, its location history will be deleted. + """ + Developer's notes + + If there are many location associations related to a thing, eagerly loading + the location associations may overburden the API and DB and introduce + performance issues. If this becomes an issue, active/current locations can + be fetched in queries when retrieving things. Be careful, though, as this + will need to be included everywhere where a thing record is needed, such as + for GET /thing, GET /thing/{thing_id}, and GET /contact. See below for an + example of a way to retrieve the active/current location in a query + + from sqlalchemy.orm import aliased + from sqlalchemy import select, func + + LTA = aliased(LocationThingAssociation) + latest_assoc = ( + select( + LTA.thing_id, + func.max(LTA.effective_start).label("max_start") + ) + .where(LTA.effective_end == None) + .group_by(LTA.thing_id) + .subquery() + ) + + lta_alias = aliased(LocationThingAssociation) + query = ( + select(Thing, Location) + .join(lta_alias, Thing.id == lta_alias.thing_id) + .join(Location, lta_alias.location_id == Location.id) + .join( + latest_assoc, + (latest_assoc.c.thing_id == lta_alias.thing_id) & + (latest_assoc.c.max_start == lta_alias.effective_start) + ) + ) + """ location_associations = relationship( "LocationThingAssociation", back_populates="thing", From 1c5d8143fa678b040eadd010410c34dec58d84d6 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 8 Oct 2025 14:54:34 -0600 Subject: [PATCH 5/7] fix: only get geospatial things with current location --- services/geospatial_helper.py | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/services/geospatial_helper.py b/services/geospatial_helper.py index 0279d6d24..356c55634 100644 --- a/services/geospatial_helper.py +++ b/services/geospatial_helper.py @@ -25,6 +25,8 @@ from geoalchemy2.shape import to_shape from shapely.wkt import loads as wkt_loads from sqlalchemy import Select, select +from sqlalchemy.orm import aliased +from sqlalchemy import func def get_thing_features( @@ -42,10 +44,30 @@ def get_thing_features( # elif thing_type == "spring": # selection_args.append(SpringThing) + # Subquery: get the latest association for each thing (optionally only active) + lta_alias = aliased(LocationThingAssociation) + + latest_assoc = ( + select( + LocationThingAssociation.thing_id, + func.max(LocationThingAssociation.effective_start).label("max_start"), + ) + .where( + LocationThingAssociation.effective_end == None + ) # Only active, remove if you want most recent regardless of end + .group_by(LocationThingAssociation.thing_id) + .subquery() + ) + sql = ( select(Thing, ST_AsGeoJSON(Location.point).label("geojson"), Location.elevation) - .join(LocationThingAssociation, Thing.id == LocationThingAssociation.thing_id) - .join(Location, LocationThingAssociation.location_id == Location.id) + .join(lta_alias, Thing.id == lta_alias.thing_id) + .join(Location, lta_alias.location_id == Location.id) + .join( + latest_assoc, + (latest_assoc.c.thing_id == lta_alias.thing_id) + & (latest_assoc.c.max_start == lta_alias.effective_start), + ) ) if thing_type: From a900dd696fb1c4346627490a7ca34dbdf129c1a3 Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 8 Oct 2025 14:58:15 -0600 Subject: [PATCH 6/7] update note --- db/thing.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/db/thing.py b/db/thing.py index 459d7811d..2d282f9e6 100644 --- a/db/thing.py +++ b/db/thing.py @@ -125,10 +125,11 @@ class Thing(Base, AutoBaseMixin, ReleaseMixin, StatusHistoryMixin, PermissionMix If there are many location associations related to a thing, eagerly loading the location associations may overburden the API and DB and introduce performance issues. If this becomes an issue, active/current locations can - be fetched in queries when retrieving things. Be careful, though, as this - will need to be included everywhere where a thing record is needed, such as - for GET /thing, GET /thing/{thing_id}, and GET /contact. See below for an - example of a way to retrieve the active/current location in a query + be fetched in queries when retrieving things. Be thorough if following this + route as it will need to be included everywhere where a thing record is + needed, such as for GET /thing, GET /thing/{thing_id}, and GET /contact. See + below for an example of a way to retrieve the active/current location in a + query: from sqlalchemy.orm import aliased from sqlalchemy import select, func From 111aef48449fdf18fa9538f2b7ab5f0ab402d38d Mon Sep 17 00:00:00 2001 From: Jacob Brown Date: Wed, 8 Oct 2025 16:26:41 -0600 Subject: [PATCH 7/7] refactor: rename active_location to current_location --- db/thing.py | 8 ++++---- schemas/thing.py | 2 +- tests/test_thing.py | 18 +++++++++--------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/db/thing.py b/db/thing.py index 2d282f9e6..c191ca1fd 100644 --- a/db/thing.py +++ b/db/thing.py @@ -239,18 +239,18 @@ class Thing(Base, AutoBaseMixin, ReleaseMixin, StatusHistoryMixin, PermissionMix ) @property - def active_location(self): + def current_location(self): """ Returns the currently active Location by sorting the effective_start field. Thing eagerly loads location_association, which eagerly loads location, which will hopefully prevent N+1 query problems. """ - active_location = sorted( + current_location = sorted( self.location_associations, key=lambda x: x.effective_start, reverse=True ) return ( - active_location[0].location - if active_location and active_location[0].effective_end is None + current_location[0].location + if current_location and current_location[0].effective_end is None else None ) diff --git a/schemas/thing.py b/schemas/thing.py index 781f3a162..686d0b502 100644 --- a/schemas/thing.py +++ b/schemas/thing.py @@ -139,7 +139,7 @@ def check_depths(self): class BaseThingResponse(BaseResponseModel): name: str thing_type: str - active_location: LocationResponse | None + current_location: LocationResponse | None first_visit_date: PastDate | None diff --git a/tests/test_thing.py b/tests/test_thing.py index 4a17269a7..05dccdda1 100644 --- a/tests/test_thing.py +++ b/tests/test_thing.py @@ -122,7 +122,7 @@ def test_add_water_well(location, group): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location cleanup_post_test(Thing, data["id"]) @@ -199,7 +199,7 @@ def test_add_spring(location, group): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location cleanup_post_test(Thing, data["id"]) @@ -408,7 +408,7 @@ def test_get_water_wells(water_well_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["items"][0]["active_location"] == expected_location + assert data["items"][0]["current_location"] == expected_location def test_get_water_well_by_id(water_well_thing, location): @@ -439,7 +439,7 @@ def test_get_water_well_by_id(water_well_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location def test_get_water_well_by_id_404_not_found(water_well_thing): @@ -485,7 +485,7 @@ def test_get_springs(spring_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["items"][0]["active_location"] == expected_location + assert data["items"][0]["current_location"] == expected_location def test_get_spring_by_id(spring_thing, location): @@ -505,7 +505,7 @@ def test_get_spring_by_id(spring_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location def test_get_spring_by_id_404_not_found(spring_thing): @@ -710,7 +710,7 @@ def test_get_thing_by_id(water_well_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location def test_get_thing_by_id_404_not_found(water_well_thing): @@ -798,7 +798,7 @@ def test_patch_water_well(water_well_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location cleanup_patch_test(Thing, payload, water_well_thing) @@ -861,7 +861,7 @@ def test_patch_spring(spring_thing, location): expected_location["created_at"] = ( expected_location["created_at"].isoformat().replace("+00:00", "Z") ) - assert data["active_location"] == expected_location + assert data["current_location"] == expected_location cleanup_patch_test(Thing, payload, spring_thing)