From 5cd46130da1aed2972a8d70cf4f715914abf1cf3 Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Wed, 16 Sep 2026 17:38:31 -0600 Subject: [PATCH 1/9] perf(api): return every filter option in one request and one query - replace the per-field /filter-options contract with an all-fields response keyed by field name; drop the field and source params - add queries.filter_option_rows: one ROW_NUMBER pass over (resource_id, colname) answers all six fields, DISTINCT over the pair - derive the filterable field list from OGFieldFilterOptionsResponse so the model and the query cannot drift - FilterBar calls the options hook once instead of once per dropdown - values verified identical to the previous implementation for every licence profile before the old code was removed --- .../api/db/og_field_resource_actions.py | 39 ++---- deployments/api/src/stitch/api/db/queries.py | 82 +++++++++++- deployments/api/src/stitch/api/entities.py | 37 +++--- .../src/stitch/api/routers/oil_gas_fields.py | 8 +- .../api/tests/db/test_resource_actions.py | 124 +++++++++++------- .../routers/test_query_param_validation.py | 10 -- .../api/tests/routers/test_resources_unit.py | 44 +++++-- .../src/components/FilterBar.jsx | 37 ++---- .../src/components/ResourcesView.test.jsx | 51 +++---- .../stitch-frontend/src/hooks/useResources.js | 50 +++---- .../stitch-frontend/src/queries/api.js | 4 +- .../stitch-frontend/src/queries/api.test.js | 19 +-- .../stitch-frontend/src/queries/resources.js | 10 +- .../src/queries/resources.test.js | 5 +- 14 files changed, 289 insertions(+), 231 deletions(-) diff --git a/deployments/api/src/stitch/api/db/og_field_resource_actions.py b/deployments/api/src/stitch/api/db/og_field_resource_actions.py index 540c4a75..2a01bf8a 100644 --- a/deployments/api/src/stitch/api/db/og_field_resource_actions.py +++ b/deployments/api/src/stitch/api/db/og_field_resource_actions.py @@ -14,8 +14,8 @@ ) from stitch.api.auth import CurrentUser from stitch.api.entities import ( - FilterOptionField, - OGFieldFilterOptionsParams, + FILTER_OPTION_FIELDS, + OGFieldFilterOptionsResponse, OGFieldQueryParams, ) from stitch.api.db.og_field_source_actions import ( @@ -38,13 +38,11 @@ from .model.oil_gas_field_source_value import ( ATTRIBUTE_NAMES, materialize_value, - value_attr_for, ) from .queries import ( - add_ranking, base_resource_query, - construct_base_query_statement, field_source_candidates, + filter_option_rows, ) from .utils import ( coalesce_resources, @@ -53,7 +51,6 @@ ) -_FILTER_OPTION_FIELDS: frozenset[str] = frozenset(get_args(FilterOptionField)) _ALL_SOURCES: frozenset[OGSISrcKey] = frozenset(get_args(OGSISrcKey)) @@ -93,29 +90,19 @@ async def query( async def filter_options( session: AsyncSession, - params: OGFieldFilterOptionsParams, licensed_sources: Collection[OGSISrcKey] | None = None, -) -> list[str]: - """Return distinct coalesced resource values for one filterable field. +) -> OGFieldFilterOptionsResponse: + """Distinct coalesced values for every filterable field, in one query. - Reads the shared coalescing core narrowed to the single field, so values are - priority/override-coalesced and licensed before being deduped and sorted. - ``params.source`` is ignored (see ``query``). + Values are priority/override-coalesced and licensed before being deduped and + sorted, as they are on the list path. Every field is present, empty list + included: grouping appends into a pre-seeded dict, so it does not depend on + the SQL ORDER BY. """ - if params.field not in _FILTER_OPTION_FIELDS: - raise HTTPException( - status_code=422, - detail=f"field={params.field} is not supported for resource filter options.", - ) - - base_cte = construct_base_query_statement(licensed_sources) - filtered = select(base_cte).where(base_cte.c.colname == params.field).cte() - ranked = add_ranking(filtered).cte("ranked") - value_col = getattr(ranked.c, value_attr_for(params.field)) - labeled = value_col.label("value") - stmt = select(labeled).where(value_col.is_not(None)).distinct().order_by(labeled) - values = await session.scalars(stmt) - return list(values.all()) + options: dict[str, list[str]] = {field: [] for field in FILTER_OPTION_FIELDS} + for colname, value in await session.execute(filter_option_rows(licensed_sources)): + options[colname].append(value) + return OGFieldFilterOptionsResponse(**options) async def get( diff --git a/deployments/api/src/stitch/api/db/queries.py b/deployments/api/src/stitch/api/db/queries.py index 61a6781d..592602a7 100644 --- a/deployments/api/src/stitch/api/db/queries.py +++ b/deployments/api/src/stitch/api/db/queries.py @@ -38,7 +38,7 @@ ResourceModel, ) from stitch.api.db.model.oil_gas_field_source_value import value_attr_for -from stitch.api.entities import OGFieldQueryParams +from stitch.api.entities import FILTER_OPTION_FIELDS, OGFieldQueryParams from stitch.ogsi.model.types import OGSISrcKey # Single source of truth for the source-list field metadata. This is a shared @@ -250,6 +250,86 @@ def coalesced_winner_rows( ) +def filter_option_rows( + licensed_sources: Collection[OGSISrcKey] | None = None, +) -> Select[tuple[str, str]]: + """Distinct winning ``(colname, value)`` pairs for every filterable field. + + One pass. The window partitions by ``(resource_id, colname)``, so a single + ROW_NUMBER picks the winner for all of ``FILTER_OPTION_FIELDS`` at once; + ``rn == 1`` is the winner. ``DISTINCT`` is over the pair, so a string that is + a valid value for two fields survives as two rows and lands under both. + + Self-contained rather than built on ``construct_base_query_statement``: this + reads ``value_text`` and the ranking keys only, so it skips that CTE's joins + to ``og_field_resources`` and ``oil_gas_field_sources`` and its unused typed + value columns. Dropping the resource join relies on the invariant that a + merged resource's memberships are all INACTIVE (``_repoint_memberships``). + """ + m = MembershipModel + v = OilGasFieldSourceValueModel + p = OGFieldSourcePriority + o = OGFieldResourceSourcePriority + + ranked = ( + select( + v.colname, + v.value_text, + func.row_number() + .over( + partition_by=(m.resource_id, v.colname), + # Tiered exactly as ``_ranked``: a value a curator has re-ranked + # (an override row exists, so o.priority is NOT NULL) beats every + # value that has not been -- that is what NULLS LAST buys. Within + # a tier the global default source priority decides. + order_by=( + o.priority.asc().nulls_last(), + p.priority.asc(), + # Required, not a tiebreak of convenience: one resource can + # hold several ACTIVE records from the SAME source, which tie + # on p.priority (unique per source). Without these the winner + # is whatever the database happens to pick, so the endpoint + # can return a different value set on identical calls. + m.source.asc(), + m.source_pk.asc(), + ), + ) + .label("rn"), + ) + .select_from(m) + .join(v, v.source_pk == m.source_pk) + .join(p, p.source == m.source) + # At the value grain, and over the override table's full primary key, so + # at most one override row per value row: the outer join cannot fan out. + # ``o.source == m.source`` is the same dual-key guard the shared base CTE + # applies -- it fails safe to the default priority rather than applying an + # override whose (source, source_pk) pair is mismatched. + .outerjoin( + o, + and_( + o.resource_id == m.resource_id, + o.source_pk == m.source_pk, + o.source == m.source, + o.colname == v.colname, + ), + ) + .where( + m.status == MembershipStatus.ACTIVE, + v.colname.in_(FILTER_OPTION_FIELDS), + ) + ) + if licensed_sources is not None: + ranked = ranked.where(m.source.in_(list(dict.fromkeys(licensed_sources)))) + c = ranked.subquery().c + + return ( + select(c.colname, c.value_text) + .where(c.rn == 1, c.value_text.is_not(None)) + .distinct() + .order_by(c.colname, c.value_text) + ) + + def field_source_candidates( resource_id: int, field: str, diff --git a/deployments/api/src/stitch/api/entities.py b/deployments/api/src/stitch/api/entities.py index 92752b8c..5bc0a121 100644 --- a/deployments/api/src/stitch/api/entities.py +++ b/deployments/api/src/stitch/api/entities.py @@ -98,19 +98,6 @@ def total_pages(self) -> int: "resource_id", ] -FilterOptionField = Literal[ - "name", - "name_local", - "basin", - "state_province", - "region", - "country", - "field_status", - "location_type", - "production_conventionality", - "primary_hydrocarbon_group", -] - class OGFieldFilterParams(BaseModel): q: str | None = None @@ -136,14 +123,26 @@ class OGFieldQueryParams(PaginationParams, OGFieldFilterParams, OGFieldSortParam source: list[OGSISrcKey] = Field(default_factory=lambda: list(OGSI_SOURCE_DEFAULT)) -class OGFieldFilterOptionsParams(BaseModel): - field: FilterOptionField - source: list[OGSISrcKey] = Field(default_factory=lambda: list(OGSI_SOURCE_DEFAULT)) +class OGFieldFilterOptionsResponse(BaseModel): + """Every filterable field's distinct coalesced values, one list per field. + Fields are required, not defaulted: the action always supplies all of them, + empty list included, so a client never has to tell "no licensed values" apart + from "field missing from the payload". + """ -class OGFieldFilterOptionsResponse(BaseModel): - field: FilterOptionField - values: list[str] + basin: list[str] + country: list[str] + field_status: list[str] + primary_hydrocarbon_group: list[str] + region: list[str] + state_province: list[str] + + +# The response model is the single source of truth for which fields are +# filterable; the query derives its ``colname`` list from this, so the two can't +# drift. +FILTER_OPTION_FIELDS: tuple[str, ...] = tuple(OGFieldFilterOptionsResponse.model_fields) class MergeCandidateStatus(StrEnum): diff --git a/deployments/api/src/stitch/api/routers/oil_gas_fields.py b/deployments/api/src/stitch/api/routers/oil_gas_fields.py index 4b5d4d7b..ef5cd0eb 100644 --- a/deployments/api/src/stitch/api/routers/oil_gas_fields.py +++ b/deployments/api/src/stitch/api/routers/oil_gas_fields.py @@ -13,7 +13,6 @@ ) from stitch.api.entities import ( - OGFieldFilterOptionsParams, OGFieldFilterOptionsResponse, MergeCandidateCreateRequest, MergeCandidateDetailView, @@ -84,20 +83,19 @@ async def get_all_resources( ) +# Must stay above GET /{id}: Starlette matches routes in declaration order, so a +# later static path would be swallowed by the id route. @router.get("/filter-options", response_model=OGFieldFilterOptionsResponse) async def get_resource_filter_options( *, uow: UnitOfWorkDep, _user: CurrentUser, claims: Claims, - params: Annotated[OGFieldFilterOptionsParams, Query()], ) -> OGFieldFilterOptionsResponse: - values = await resource_actions.filter_options( + return await resource_actions.filter_options( session=uow.session, - params=params, licensed_sources=licensed_sources(claims), ) - return OGFieldFilterOptionsResponse(field=params.field, values=values) @router.get( diff --git a/deployments/api/tests/db/test_resource_actions.py b/deployments/api/tests/db/test_resource_actions.py index 1f048271..dd3b035f 100644 --- a/deployments/api/tests/db/test_resource_actions.py +++ b/deployments/api/tests/db/test_resource_actions.py @@ -21,8 +21,9 @@ OilGasFieldSourceValueModel, ResourceModel, ) +from stitch.api.db.queries import filter_option_rows from stitch.api.entities import ( - OGFieldFilterOptionsParams, + FILTER_OPTION_FIELDS, OGFieldQueryParams, User, ) @@ -967,12 +968,9 @@ async def test_returns_distinct_sorted_coalesced_values( {"source": "rmi", "country": None}, ) - values = await resource_actions.filter_options( - seeded_integration_session, - OGFieldFilterOptionsParams(field="country"), - ) + options = await resource_actions.filter_options(seeded_integration_session) - assert values == ["CAN", "USA"] + assert options.country == ["CAN", "USA"] @pytest.mark.anyio async def test_honors_licensed_sources_after_coalescing( @@ -992,42 +990,30 @@ async def test_honors_licensed_sources_after_coalescing( {"source": "rmi", "country": "USA"}, ) - values = await resource_actions.filter_options( + options = await resource_actions.filter_options( seeded_integration_session, - OGFieldFilterOptionsParams(field="country"), licensed_sources=frozenset({"gem", "wm", "llm"}), ) - assert values == ["CAN"] + assert options.country == ["CAN"] @pytest.mark.anyio - async def test_excludes_repointed_and_inactive_memberships( + async def test_excludes_inactive_memberships( self, seeded_integration_session: AsyncSession, test_user: User, ): - active_id = await _create_resource_with_sources( + await _create_resource_with_sources( seeded_integration_session, test_user, {"source": "rmi", "country": "USA"}, ) - repointed_to_id = await _create_resource_with_sources( - seeded_integration_session, - test_user, - {"source": "rmi", "country": "BRA"}, - ) inactive_id = await _create_resource_with_sources( seeded_integration_session, test_user, {"source": "rmi", "country": "CAN"}, ) - repointed_resource = await seeded_integration_session.get( - ResourceModel, repointed_to_id - ) - assert repointed_resource is not None - repointed_resource.repointed_id = active_id - inactive_membership = await seeded_integration_session.scalar( select(MembershipModel).where(MembershipModel.resource_id == inactive_id) ) @@ -1035,36 +1021,78 @@ async def test_excludes_repointed_and_inactive_memberships( inactive_membership.status = MembershipStatus.INACTIVE await seeded_integration_session.flush() - values = await resource_actions.filter_options( + options = await resource_actions.filter_options(seeded_integration_session) + + assert options.country == ["USA"] + + @pytest.mark.anyio + async def test_returns_every_field_including_empty_ones( + self, + seeded_integration_session: AsyncSession, + test_user: User, + ): + await _create_resource_with_sources( seeded_integration_session, - OGFieldFilterOptionsParams(field="country"), + test_user, + {"source": "rmi", "country": "USA"}, ) - assert values == ["USA"] + options = await resource_actions.filter_options(seeded_integration_session) - def test_postgres_distinct_query_orders_by_selected_value_alias(self): - """The rewritten filter_options construction compiles on Postgres. + assert set(options.model_dump()) == set(FILTER_OPTION_FIELDS) + assert options.country == ["USA"] + # A field no source carries is an empty list, never a missing key. + assert options.field_status == [] + assert options.basin == [] - Mirrors ``filter_options``: distinct over the coalesced value column for - one field, ordered by the selected alias. - """ - params = OGFieldFilterOptionsParams(field="basin") - base_cte = resource_actions.construct_base_query_statement( - licensed_sources=frozenset({"gem", "wm", "rmi", "llm"}), + @pytest.mark.anyio + async def test_returns_each_field_from_one_pass( + self, + seeded_integration_session: AsyncSession, + test_user: User, + ): + await _create_resource_with_sources( + seeded_integration_session, + test_user, + { + "source": "rmi", + "country": "USA", + "state_province": "Texas", + "basin": "Permian", + "region": "North America", + }, ) - filtered = ( - resource_actions.select(base_cte) - .where(base_cte.c.colname == params.field) - .cte() + # "Texas" is a legitimate value for two fields. Both must survive, each + # under its own key -- which is what proves DISTINCT is over the + # (colname, value) pair rather than the bare value. + await _create_resource_with_sources( + seeded_integration_session, + test_user, + { + "source": "gem", + "country": "CAN", + "state_province": "Alberta", + "basin": "Texas", + }, ) - ranked = resource_actions.add_ranking(filtered).cte("ranked") - value_col = getattr(ranked.c, resource_actions.value_attr_for(params.field)) - labeled = value_col.label("value") - stmt = ( - resource_actions.select(labeled) - .where(value_col.is_not(None), value_col != "") - .distinct() - .order_by(labeled) + + options = await resource_actions.filter_options(seeded_integration_session) + + assert options.country == ["CAN", "USA"] + assert options.state_province == ["Alberta", "Texas"] + assert options.basin == ["Permian", "Texas"] + assert options.region == ["North America"] + + def test_postgres_distinct_query_orders_by_selected_columns(self): + """``filter_option_rows`` compiles on Postgres. + + Postgres rejects SELECT DISTINCT ... ORDER BY over an expression that is + not in the select list. Both ordered columns are selected here, so it + compiles -- but the test suite runs on SQLite, which is laxer, so compile + the real statement against the Postgres dialect to catch that. + """ + stmt = filter_option_rows( + licensed_sources=frozenset({"gem", "wm", "rmi", "llm"}), ) sql = str( @@ -1074,8 +1102,10 @@ def test_postgres_distinct_query_orders_by_selected_value_alias(self): ) ) - assert "SELECT DISTINCT ranked.value_text AS value" in sql - assert "ORDER BY value" in sql + assert "SELECT DISTINCT" in sql + assert "ORDER BY" in sql + ordered = sql[sql.index("ORDER BY") :] + assert "colname" in ordered and "value_text" in ordered @pytest.mark.anyio async def test_only_unlicensed_selected_sources_still_return_resource( diff --git a/deployments/api/tests/routers/test_query_param_validation.py b/deployments/api/tests/routers/test_query_param_validation.py index 9e326814..2d6a97f0 100644 --- a/deployments/api/tests/routers/test_query_param_validation.py +++ b/deployments/api/tests/routers/test_query_param_validation.py @@ -61,16 +61,6 @@ async def test_sort_by_source_returns_422(self, integration_client: AsyncClient) ) assert resp.status_code == 422 - @pytest.mark.anyio - async def test_invalid_filter_options_field_returns_422( - self, async_client: AsyncClient - ): - """filter-options field must be one of the supported coalesced scalar fields.""" - resp = await async_client.get( - "/oil-gas-fields/filter-options", params={"field": "owners"} - ) - assert resp.status_code == 422 - @pytest.mark.anyio async def test_unknown_field_in_sources_path_returns_422( self, async_client: AsyncClient diff --git a/deployments/api/tests/routers/test_resources_unit.py b/deployments/api/tests/routers/test_resources_unit.py index e9388be6..d38d6e61 100644 --- a/deployments/api/tests/routers/test_resources_unit.py +++ b/deployments/api/tests/routers/test_resources_unit.py @@ -8,6 +8,7 @@ from stitch.ogsi.model import OGFieldListItemView, OilGasFieldBase from stitch.api.db.config import get_uow +from stitch.api.entities import OGFieldFilterOptionsResponse from stitch.api.main import app from tests.factories import ResourceCreateFactory, SourceFactory @@ -229,17 +230,31 @@ async def override_get_uow(): app.dependency_overrides[get_uow] = override_get_uow with patch("stitch.api.routers.oil_gas_fields.resource_actions") as mock_repo: - mock_repo.filter_options = AsyncMock(return_value=["CAN", "USA"]) - - response = await async_client.get( - "/oil-gas-fields/filter-options?field=country" + mock_repo.filter_options = AsyncMock( + return_value=OGFieldFilterOptionsResponse( + basin=["Permian"], + country=["CAN", "USA"], + field_status=[], + primary_hydrocarbon_group=[], + region=["North America"], + state_province=["Alberta", "Texas"], + ) ) + response = await async_client.get("/oil-gas-fields/filter-options") + assert response.status_code == 200 - assert response.json() == {"field": "country", "values": ["CAN", "USA"]} + assert response.json() == { + "basin": ["Permian"], + "country": ["CAN", "USA"], + "field_status": [], + "primary_hydrocarbon_group": [], + "region": ["North America"], + "state_province": ["Alberta", "Texas"], + } @pytest.mark.anyio - async def test_passes_licensed_sources_and_source_filters( + async def test_passes_licensed_sources( self, async_client, mock_uow, @@ -250,17 +265,22 @@ async def override_get_uow(): app.dependency_overrides[get_uow] = override_get_uow with patch("stitch.api.routers.oil_gas_fields.resource_actions") as mock_repo: - mock_repo.filter_options = AsyncMock(return_value=["CAN", "USA"]) - - response = await async_client.get( - "/oil-gas-fields/filter-options?field=country&source=gem&source=wm" + mock_repo.filter_options = AsyncMock( + return_value=OGFieldFilterOptionsResponse( + basin=[], + country=["CAN", "USA"], + field_status=[], + primary_hydrocarbon_group=[], + region=[], + state_province=[], + ) ) + response = await async_client.get("/oil-gas-fields/filter-options") + assert response.status_code == 200 mock_repo.filter_options.assert_awaited_once() call_kwargs = mock_repo.filter_options.call_args.kwargs assert call_kwargs["licensed_sources"] == frozenset( {"rmi", "gem", "wm", "ccr", "alb", "bc", "llm"} ) - assert call_kwargs["params"].field == "country" - assert call_kwargs["params"].source == ["gem", "wm"] diff --git a/deployments/stitch-frontend/src/components/FilterBar.jsx b/deployments/stitch-frontend/src/components/FilterBar.jsx index 3c14f3d0..ddd8b367 100644 --- a/deployments/stitch-frontend/src/components/FilterBar.jsx +++ b/deployments/stitch-frontend/src/components/FilterBar.jsx @@ -2,31 +2,11 @@ import FilterDropdown from "./FilterDropdown"; import { FILTER_FIELDS, EMPTY_FILTERS } from "../config/filters"; import { useResourceFilterOptions } from "../hooks/useResources"; -function FilterFieldDropdown({ - endpoint, - field, - label, - formatValue, - selected, - onChange, -}) { - const { data } = useResourceFilterOptions(endpoint, field); - const options = (data?.values ?? []).map((value) => ({ - value, - label: formatValue ? formatValue(value) : value, - })); - - return ( - - ); -} - export default function FilterBar({ endpoint, filters, onFiltersChange }) { + // One request covers every dropdown: the endpoint returns all filterable + // fields keyed by field name, each with its distinct values. + const { data: filterOptions } = useResourceFilterOptions(endpoint); + // Flatten active filters into chips: [{ field, label, value, displayValue }, ...] // `value` is the stored/API value; `displayValue` is what the user sees. const chips = FILTER_FIELDS.flatMap(({ key, label, formatValue }) => @@ -54,12 +34,13 @@ export default function FilterBar({ endpoint, filters, onFiltersChange }) { {/* Dropdowns row */}
{FILTER_FIELDS.map(({ key, label, formatValue }) => ( - ({ + value, + label: formatValue ? formatValue(value) : value, + }))} selected={filters[key] ?? []} onChange={(values) => handleDropdownChange(key, values)} /> diff --git a/deployments/stitch-frontend/src/components/ResourcesView.test.jsx b/deployments/stitch-frontend/src/components/ResourcesView.test.jsx index 16b7d87a..46c045f5 100644 --- a/deployments/stitch-frontend/src/components/ResourcesView.test.jsx +++ b/deployments/stitch-frontend/src/components/ResourcesView.test.jsx @@ -71,23 +71,18 @@ beforeEach(() => { ...defaultHookReturn, refetch: vi.fn(), }); - const FILTER_OPTION_VALUES = { - region: ["Middle East"], - basin: ["Arabian", "Permian"], - state_province: ["Kuwait"], - field_status: ["Producing"], - country: ["NOR", "SAU"], - primary_hydrocarbon_group: ["Oil", "Gas"], - }; - vi.mocked(useResourceFilterOptions).mockImplementation( - (_endpoint, field) => ({ - ...defaultHookReturn, - data: { - field, - values: FILTER_OPTION_VALUES[field] ?? [], - }, - }), - ); + // One call, one payload keyed by field -- the shape the endpoint now returns. + vi.mocked(useResourceFilterOptions).mockReturnValue({ + ...defaultHookReturn, + data: { + region: ["Middle East"], + basin: ["Arabian", "Permian"], + state_province: ["Kuwait"], + field_status: ["Producing"], + country: ["NOR", "SAU"], + primary_hydrocarbon_group: ["Oil", "Gas"], + }, + }); }); describe("ResourcesView", () => { @@ -454,7 +449,7 @@ describe("ResourcesView", () => { }); describe("filtering", () => { - it("loads dropdown options from filter-options queries", () => { + it("loads every dropdown's options from a single filter-options query", () => { vi.mocked(useResources).mockReturnValue({ ...defaultHookReturn, data: mockResourceData, @@ -462,24 +457,8 @@ describe("ResourcesView", () => { renderWithQueryClient(); - expect(useResourceFilterOptions).toHaveBeenCalledWith(ENDPOINT, "region"); - expect(useResourceFilterOptions).toHaveBeenCalledWith( - ENDPOINT, - "state_province", - ); - expect(useResourceFilterOptions).toHaveBeenCalledWith(ENDPOINT, "basin"); - expect(useResourceFilterOptions).toHaveBeenCalledWith( - ENDPOINT, - "field_status", - ); - expect(useResourceFilterOptions).toHaveBeenCalledWith( - ENDPOINT, - "country", - ); - expect(useResourceFilterOptions).toHaveBeenCalledWith( - ENDPOINT, - "primary_hydrocarbon_group", - ); + expect(useResourceFilterOptions).toHaveBeenCalledTimes(1); + expect(useResourceFilterOptions).toHaveBeenCalledWith(ENDPOINT); }); it("shows country options as conventional names but filters by the code", () => { diff --git a/deployments/stitch-frontend/src/hooks/useResources.js b/deployments/stitch-frontend/src/hooks/useResources.js index cb824443..2008440d 100644 --- a/deployments/stitch-frontend/src/hooks/useResources.js +++ b/deployments/stitch-frontend/src/hooks/useResources.js @@ -9,6 +9,7 @@ import { } from "../queries/resources"; import { createSourceForResource, reviewMergeCandidate } from "../queries/api"; import mockResources from "../mockData/og_field_resources.json"; +import { FILTER_FIELDS } from "../config/filters"; import { getResourceField, normalizeResourceListItem, @@ -54,15 +55,11 @@ function useResourcesReal( }); } -function useResourceFilterOptionsReal( - endpoint = "resources", - field, - enabled = true, -) { +function useResourceFilterOptionsReal(endpoint = "resources", enabled = true) { const config = useConfig(); return useAuthenticatedQuery({ - ...resourceQueries.filterOptions(config, endpoint, field), - enabled: enabled && Boolean(field), + ...resourceQueries.filterOptions(config, endpoint), + enabled, }); } @@ -217,16 +214,23 @@ function getMockResourcePage({ }; } -function getMockFilterOptions(field) { - const values = Array.from( - new Set( - MOCK_RESOURCE_ITEMS.map((resource) => getResourceField(resource, field)) - .filter((value) => value != null && value !== "") - .map(String), - ), - ).sort((a, b) => a.localeCompare(b)); - - return { field, values }; +// Mirrors the API's all-fields payload: one key per dropdown the UI renders, +// always present even when the mock data carries no values for it. +function getMockFilterOptions() { + return Object.fromEntries( + FILTER_FIELDS.map(({ key: field }) => [ + field, + Array.from( + new Set( + MOCK_RESOURCE_ITEMS.map((resource) => + getResourceField(resource, field), + ) + .filter((value) => value != null && value !== "") + .map(String), + ), + ).sort((a, b) => a.localeCompare(b)), + ]), + ); } function useResourcesMock( @@ -267,15 +271,11 @@ function useResourcesMock( }); } -function useResourceFilterOptionsMock( - endpoint = "resources", - field, - enabled = true, -) { +function useResourceFilterOptionsMock(endpoint = "resources", enabled = true) { return useQuery({ - ...resourceQueries.filterOptions(UNUSED_MOCK_CONFIG, endpoint, field), - queryFn: () => Promise.resolve(getMockFilterOptions(field)), - enabled: enabled && Boolean(field), + ...resourceQueries.filterOptions(UNUSED_MOCK_CONFIG, endpoint), + queryFn: () => Promise.resolve(getMockFilterOptions()), + enabled, }); } diff --git a/deployments/stitch-frontend/src/queries/api.js b/deployments/stitch-frontend/src/queries/api.js index b1398f7e..c7909045 100644 --- a/deployments/stitch-frontend/src/queries/api.js +++ b/deployments/stitch-frontend/src/queries/api.js @@ -27,10 +27,8 @@ export async function getResourceFilterOptions( config, fetcher, endpoint = "resources", - field, ) { - const params = new URLSearchParams({ field }); - const url = `${config.apiBaseUrl}/${endpoint}/filter-options?${params}`; + const url = `${config.apiBaseUrl}/${endpoint}/filter-options`; const response = await fetcher(url); if (!response.ok) { const error = new Error(`HTTP error! status: ${response.status}`); diff --git a/deployments/stitch-frontend/src/queries/api.test.js b/deployments/stitch-frontend/src/queries/api.test.js index e4a5b33c..5722bb08 100644 --- a/deployments/stitch-frontend/src/queries/api.test.js +++ b/deployments/stitch-frontend/src/queries/api.test.js @@ -129,7 +129,14 @@ describe("API Functions", () => { describe("getResourceFilterOptions", () => { it("fetches and returns filter options successfully", async () => { - const mockOptions = { field: "basin", values: ["Arabian", "Permian"] }; + const mockOptions = { + basin: ["Arabian", "Permian"], + country: ["NOR", "SAU"], + field_status: ["Producing"], + primary_hydrocarbon_group: [], + region: ["Middle East"], + state_province: ["Kuwait"], + }; mockFetcher.mockResolvedValueOnce({ ok: true, @@ -141,11 +148,10 @@ describe("API Functions", () => { config, mockFetcher, "oil-gas-fields", - "basin", ); expect(mockFetcher).toHaveBeenCalledWith( - "http://localhost:8000/api/v1/oil-gas-fields/filter-options?field=basin", + "http://localhost:8000/api/v1/oil-gas-fields/filter-options", ); expect(result).toEqual(mockOptions); }); @@ -157,12 +163,7 @@ describe("API Functions", () => { }); await expect( - getResourceFilterOptions( - config, - mockFetcher, - "oil-gas-fields", - "basin", - ), + getResourceFilterOptions(config, mockFetcher, "oil-gas-fields"), ).rejects.toThrow("HTTP error! status: 500"); }); }); diff --git a/deployments/stitch-frontend/src/queries/resources.js b/deployments/stitch-frontend/src/queries/resources.js index 7f3d6062..7aa4e5f0 100644 --- a/deployments/stitch-frontend/src/queries/resources.js +++ b/deployments/stitch-frontend/src/queries/resources.js @@ -26,10 +26,9 @@ const keys = { all: (endpoint = "resources") => [endpoint], lists: (endpoint = "resources") => [...keys.all(endpoint), "list"], list: (endpoint = "resources", filters) => [...keys.lists(endpoint), filters], - filterOptions: (endpoint = "resources", field) => [ + filterOptions: (endpoint = "resources") => [ ...keys.all(endpoint), "filter-options", - field, ], details: (endpoint = "resources") => [...keys.all(endpoint), "detail"], detail: (endpoint = "resources", id) => [...keys.details(endpoint), id], @@ -91,11 +90,10 @@ export const resourceQueries = { placeholderData: keepPreviousData, }), - filterOptions: (config, endpoint = "resources", field) => + filterOptions: (config, endpoint = "resources") => queryOptions({ - queryKey: keys.filterOptions(endpoint, field), - queryFn: (fetcher) => - getResourceFilterOptions(config, fetcher, endpoint, field), + queryKey: keys.filterOptions(endpoint), + queryFn: (fetcher) => getResourceFilterOptions(config, fetcher, endpoint), staleTime: DEFAULT_STALE_TIME, }), diff --git a/deployments/stitch-frontend/src/queries/resources.test.js b/deployments/stitch-frontend/src/queries/resources.test.js index b5383467..52099d67 100644 --- a/deployments/stitch-frontend/src/queries/resources.test.js +++ b/deployments/stitch-frontend/src/queries/resources.test.js @@ -20,10 +20,7 @@ describe("resourceQueries key prefixes", () => { const endpoint = "oil-gas-fields"; const cases = [ ["list", () => resourceQueries.list(config, endpoint, 1, 10, {})], - [ - "filterOptions", - () => resourceQueries.filterOptions(config, endpoint, "basin"), - ], + ["filterOptions", () => resourceQueries.filterOptions(config, endpoint)], ["detail", () => resourceQueries.detail(config, endpoint, 42)], [ "fieldSources", From ce54ec5e761ddbf87ccc4e5ffb141db6e9fcfc94 Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Wed, 16 Sep 2026 17:39:04 -0600 Subject: [PATCH 2/9] docs(api): regenerate API_REFERENCE.md Includes drift that predates this branch: new source types, the removed merge-candidate preview endpoint, the added field-priority endpoint, and requested_resource_id. Only the filter-options section is from STIT-763. --- deployments/api/API_REFERENCE.md | 57 ++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/deployments/api/API_REFERENCE.md b/deployments/api/API_REFERENCE.md index 91871712..ee975323 100644 --- a/deployments/api/API_REFERENCE.md +++ b/deployments/api/API_REFERENCE.md @@ -54,7 +54,7 @@ **Request Body:** `OGFieldResource` - `id`: integer | null -- `source_data`: array[GemSource-Input | WoodMacSource-Input | RMISource-Input | LLMSource-Input] +- `source_data`: array[GemSource-Input | WoodMacSource-Input | RMISource-Input | LLMSource-Input | CCRSource-Input | ALBSource-Input | BCSource-Input] - `repointed_to`: integer | null - `constituents`: array[integer] - `provenance`: object[string, array[object] | null] @@ -63,7 +63,7 @@ **Response:** `200` - `id`: integer | null -- `source_data`: array[GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView] +- `source_data`: array[GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView | CCRSourceView | ALBSourceView | BCSourceView] - `repointed_to`: integer | null - `constituents`: array[integer] - `provenance`: object[string, array[object] | null] @@ -77,8 +77,12 @@ **Response:** `200` -- `field`: string -- `values`: array[string] +- `basin`: array[string] +- `country`: array[string] +- `field_status`: array[string] +- `primary_hydrocarbon_group`: array[string] +- `region`: array[string] +- `state_province`: array[string] --- @@ -133,18 +137,7 @@ - `last_updated_by_id`: integer - `reviewed_at`: string | null - `reviewed_by_id`: integer | null - ---- - -### `GET /api/v1/oil-gas-fields/merge-candidates/{id}/preview` - - - -**Response:** `200` - -- `resource_ids`: array[integer] -- `data`: OilGasFieldBase -- `provenance`: object[string, string | null] +- `compare`: array[FieldComparisonView] --- @@ -219,6 +212,7 @@ - `fid_year`: integer | null - `field_status`: string | null - `id`: integer +- `requested_resource_id`: integer | null --- @@ -231,7 +225,8 @@ - `id`: integer - `data`: OilGasFieldBase - `provenance`: object[string, string | null] -- `source_data`: array[GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView] +- `source_data`: array[GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView | CCRSourceView | ALBSourceView | BCSourceView] +- `requested_resource_id`: integer | null --- @@ -245,16 +240,30 @@ --- +### `PUT /api/v1/oil-gas-fields/{id}/fields/{field}/sources/priority` + + + +**Request Body:** `SetFieldPriorityRequest` + +- `ordered_source_pks`: array[integer] + +**Response:** `200` + +- array[OGFieldSourceValueView] + +--- + ### `POST /api/v1/oil-gas-fields/{id}/sources` -**Request Body:** GemSource-Input | WoodMacSource-Input | RMISource-Input | LLMSource-Input +**Request Body:** GemSource-Input | WoodMacSource-Input | RMISource-Input | LLMSource-Input | CCRSource-Input | ALBSource-Input | BCSource-Input **Response:** `200` -- GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView +- GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView | CCRSourceView | ALBSourceView | BCSourceView --- @@ -264,12 +273,12 @@ -**Request Body:** GemSource-Input | WoodMacSource-Input | RMISource-Input | LLMSource-Input +**Request Body:** GemSource-Input | WoodMacSource-Input | RMISource-Input | LLMSource-Input | CCRSource-Input | ALBSource-Input | BCSource-Input **Response:** `200` -- GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView +- GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView | CCRSourceView | ALBSourceView | BCSourceView --- @@ -279,7 +288,7 @@ **Response:** `200` -- `items`: array[GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView] +- `items`: array[GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView | CCRSourceView | ALBSourceView | BCSourceView] - `total_count`: integer - `page`: integer - `page_size`: integer @@ -293,7 +302,7 @@ **Response:** `200` -- GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView +- GemSourceView | WoodMacSourceView | RMISourceView | LLMSourceView | CCRSourceView | ALBSourceView | BCSourceView --- @@ -303,6 +312,6 @@ **Response:** `200` -- GemSource-Output | WoodMacSource-Output | RMISource-Output | LLMSource-Output +- GemSource-Output | WoodMacSource-Output | RMISource-Output | LLMSource-Output | CCRSource-Output | ALBSource-Output | BCSource-Output --- From abc6e7d436357630142904dceafe2892e955885c Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Mon, 21 Sep 2026 13:56:52 -0600 Subject: [PATCH 3/9] fix(frontend): restore filter option label sort lost in merge - Merging #275 kept `FilterFieldDropdown`, which nothing referenced after this branch moved to a single filter-options query. - Its sort by display label (STIT-746) was the only copy, so deleting the dead wrapper would have dropped country ordering; moved it into the `FILTER_FIELDS.map` instead. - Updated the alphabetical-order test to mock the new keyed response shape rather than the old per-field `{ field, values }`. --- .../src/components/FilterBar.jsx | 57 +++++++------------ .../src/components/ResourcesView.test.jsx | 20 ++++--- 2 files changed, 30 insertions(+), 47 deletions(-) diff --git a/deployments/stitch-frontend/src/components/FilterBar.jsx b/deployments/stitch-frontend/src/components/FilterBar.jsx index 936a5e96..9aa94dc4 100644 --- a/deployments/stitch-frontend/src/components/FilterBar.jsx +++ b/deployments/stitch-frontend/src/components/FilterBar.jsx @@ -2,34 +2,6 @@ import FilterDropdown from "./FilterDropdown"; import { FILTER_FIELDS, EMPTY_FILTERS } from "../config/filters"; import { useResourceFilterOptions } from "../hooks/useResources"; -function FilterFieldDropdown({ - endpoint, - field, - label, - formatValue, - selected, - onChange, -}) { - const { data } = useResourceFilterOptions(endpoint, field); - // The API sorts by the stored value, which can differ from the order of the - // labels we display (country codes vs. country names), so sort by label. - const options = (data?.values ?? []) - .map((value) => ({ - value, - label: formatValue ? formatValue(value) : value, - })) - .sort((a, b) => a.label.localeCompare(b.label, "en")); - - return ( - - ); -} - export default function FilterBar({ endpoint, filters, onFiltersChange }) { // One request covers every dropdown: the endpoint returns all filterable // fields keyed by field name, each with its distinct values. @@ -61,18 +33,27 @@ export default function FilterBar({ endpoint, filters, onFiltersChange }) {
{/* Dropdowns row */}
- {FILTER_FIELDS.map(({ key, label, formatValue }) => ( - ({ + {FILTER_FIELDS.map(({ key, label, formatValue }) => { + // The API sorts by the stored value, which can differ from the order + // of the labels we display (country codes vs. country names), so + // sort by label. + const options = (filterOptions?.[key] ?? []) + .map((value) => ({ value, label: formatValue ? formatValue(value) : value, - }))} - selected={filters[key] ?? []} - onChange={(values) => handleDropdownChange(key, values)} - /> - ))} + })) + .sort((a, b) => a.label.localeCompare(b.label, "en")); + + return ( + handleDropdownChange(key, values)} + /> + ); + })}
{/* Chips + clear button — only rendered when at least one filter is active */} diff --git a/deployments/stitch-frontend/src/components/ResourcesView.test.jsx b/deployments/stitch-frontend/src/components/ResourcesView.test.jsx index 9e7b8aca..e6d98143 100644 --- a/deployments/stitch-frontend/src/components/ResourcesView.test.jsx +++ b/deployments/stitch-frontend/src/components/ResourcesView.test.jsx @@ -492,15 +492,17 @@ describe("ResourcesView", () => { it("lists country options alphabetically by displayed name", () => { // The API returns values sorted by the stored alpha-3 code, which is not // the same order as the country names the user actually sees. - vi.mocked(useResourceFilterOptions).mockImplementation( - (_endpoint, field) => ({ - ...defaultHookReturn, - data: { - field, - values: field === "country" ? ["CHN", "DEU", "DNK"] : [], - }, - }), - ); + vi.mocked(useResourceFilterOptions).mockReturnValue({ + ...defaultHookReturn, + data: { + region: [], + basin: [], + state_province: [], + field_status: [], + country: ["CHN", "DEU", "DNK"], + primary_hydrocarbon_group: [], + }, + }); vi.mocked(useResources).mockReturnValue({ ...defaultHookReturn, data: mockResourceData, From f600c3c6760d863e89d36604b9cb23201148779d Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Tue, 22 Sep 2026 14:33:31 -0600 Subject: [PATCH 4/9] refactor(api): route filter_option_rows through the shared ranker - filter_option_rows duplicated _ranked's tiering and override join logic - Extracted _override_join; routed filter_option_rows through add_ranking - Verified negligible perf delta on local dev-docker data (58.6k memberships) - Added test_licensing_promotes_next_priority_value (real coverage gap) - Commented, didn't duplicate, existing coverage for the winner-cut case --- deployments/api/src/stitch/api/db/queries.py | 138 +++++++++--------- .../api/tests/db/test_resource_actions.py | 45 ++++++ 2 files changed, 115 insertions(+), 68 deletions(-) diff --git a/deployments/api/src/stitch/api/db/queries.py b/deployments/api/src/stitch/api/db/queries.py index 592602a7..308f5c5d 100644 --- a/deployments/api/src/stitch/api/db/queries.py +++ b/deployments/api/src/stitch/api/db/queries.py @@ -88,6 +88,32 @@ def _participating_columns(params: OGFieldQueryParams) -> list[str]: return list(dict.fromkeys(participating)) +def _override_join(resource_id: Any) -> ColumnElement[bool]: + """The override join condition, shared by every base that ranks values. + + At the value grain (source_pk, colname), and over the override table's + full primary key, so at most one override row per value row -- the outer + join cannot fan out. ``o.source == m.source`` is a dual-key guard: source_pk + and source aren't FK-tied, so matching source_pk alone could apply a stray + override with a mismatched (source, source_pk) pair to the wrong + membership. Requiring both fails safe to the default instead. + + ``resource_id`` is the caller's own resource-identifying column (the + resource table's ``id`` for the full base, the membership's + ``resource_id`` for the lighter filter-option base) since not every base + joins the resource table. + """ + m = MembershipModel + v = OilGasFieldSourceValueModel + o = OGFieldResourceSourcePriority + return and_( + o.resource_id == resource_id, + o.source_pk == m.source_pk, + o.source == m.source, + o.colname == v.colname, + ) + + def construct_base_query_statement( licensed_sources: Collection[OGSISrcKey] | None = None, resource_ids: Collection[int] | None = None, @@ -120,21 +146,7 @@ def construct_base_query_statement( .join(p, p.source == m.source) .join(s, and_(s.id == m.source_pk, s.source == m.source)) .join(v, v.source_pk == m.source_pk) - # Override join at the value grain (source_pk, colname), matching v exactly, - # so at most one override row per value row -- no fan-out. o.source == - # m.source is the same dual-key guard as the header join above: source_pk - # and source aren't FK-tied, so matching source_pk alone could apply a - # stray override with a mismatched (source, source_pk) pair to the wrong - # membership. Requiring both fails safe to the default instead. - .outerjoin( - o, - and_( - o.resource_id == r.id, - o.source_pk == m.source_pk, - o.source == m.source, - o.colname == v.colname, - ), - ) + .outerjoin(o, _override_join(r.id)) .where( r.repointed_id.is_(None), m.status == MembershipStatus.ACTIVE, @@ -250,81 +262,71 @@ def coalesced_winner_rows( ) -def filter_option_rows( - licensed_sources: Collection[OGSISrcKey] | None = None, -) -> Select[tuple[str, str]]: - """Distinct winning ``(colname, value)`` pairs for every filterable field. - - One pass. The window partitions by ``(resource_id, colname)``, so a single - ROW_NUMBER picks the winner for all of ``FILTER_OPTION_FIELDS`` at once; - ``rn == 1`` is the winner. ``DISTINCT`` is over the pair, so a string that is - a valid value for two fields survives as two rows and lands under both. +def _filter_option_base(licensed_sources: Collection[OGSISrcKey] | None = None) -> CTE: + """Light ranking base for ``filter_option_rows``: value rows only, no + resource/source-header joins. - Self-contained rather than built on ``construct_base_query_statement``: this - reads ``value_text`` and the ranking keys only, so it skips that CTE's joins - to ``og_field_resources`` and ``oil_gas_field_sources`` and its unused typed - value columns. Dropping the resource join relies on the invariant that a - merged resource's memberships are all INACTIVE (``_repoint_memberships``). + Deliberately narrower than ``construct_base_query_statement``: it reads + ``value_text`` and the ranking keys only, over the six filterable + columns, so it skips the resource join and the typed ``value_num``/ + ``value_json`` columns that join brings along unused. Dropping the + resource join relies on the invariant that a merged resource's + memberships are all INACTIVE (``_repoint_memberships``). """ m = MembershipModel v = OilGasFieldSourceValueModel p = OGFieldSourcePriority o = OGFieldResourceSourcePriority - ranked = ( + stmt = ( select( - v.colname, + m.resource_id.label("resource_id"), + m.source.label("source"), + m.source_pk.label("source_pk"), + o.priority.label("override_priority"), + p.priority.label("default_priority"), + v.colname.label("colname"), v.value_text, - func.row_number() - .over( - partition_by=(m.resource_id, v.colname), - # Tiered exactly as ``_ranked``: a value a curator has re-ranked - # (an override row exists, so o.priority is NOT NULL) beats every - # value that has not been -- that is what NULLS LAST buys. Within - # a tier the global default source priority decides. - order_by=( - o.priority.asc().nulls_last(), - p.priority.asc(), - # Required, not a tiebreak of convenience: one resource can - # hold several ACTIVE records from the SAME source, which tie - # on p.priority (unique per source). Without these the winner - # is whatever the database happens to pick, so the endpoint - # can return a different value set on identical calls. - m.source.asc(), - m.source_pk.asc(), - ), - ) - .label("rn"), ) .select_from(m) .join(v, v.source_pk == m.source_pk) .join(p, p.source == m.source) - # At the value grain, and over the override table's full primary key, so - # at most one override row per value row: the outer join cannot fan out. - # ``o.source == m.source`` is the same dual-key guard the shared base CTE - # applies -- it fails safe to the default priority rather than applying an - # override whose (source, source_pk) pair is mismatched. - .outerjoin( - o, - and_( - o.resource_id == m.resource_id, - o.source_pk == m.source_pk, - o.source == m.source, - o.colname == v.colname, - ), - ) + .outerjoin(o, _override_join(m.resource_id)) .where( m.status == MembershipStatus.ACTIVE, v.colname.in_(FILTER_OPTION_FIELDS), ) ) if licensed_sources is not None: - ranked = ranked.where(m.source.in_(list(dict.fromkeys(licensed_sources)))) - c = ranked.subquery().c + stmt = stmt.where(m.source.in_(list(dict.fromkeys(licensed_sources)))) + return stmt.cte("filter_option_base") + +def filter_option_rows( + licensed_sources: Collection[OGSISrcKey] | None = None, +) -> Select[tuple[str, str]]: + """Distinct winning ``(colname, value)`` pairs for every filterable field. + + One pass over the light base: the window partitions by + ``(resource_id, colname)``, so a single ROW_NUMBER picks the winner for + all of ``FILTER_OPTION_FIELDS`` at once. ``DISTINCT`` is over the pair, so + a string that is a valid value for two fields survives as two rows and + lands under both. + + Ranking is ``add_ranking`` over ``_filter_option_base`` -- the same + ``_ranked`` tiering the list/detail paths use, so this can't drift from + them on "who beats whom". It still doesn't unify the *row universe*: the + light base skips the resource join, so it doesn't check + ``repointed_id IS NULL`` the way ``construct_base_query_statement`` does + (see the base's docstring for why that's safe). + """ + ranked = add_ranking(_filter_option_base(licensed_sources)).cte( + "filter_option_ranked" + ) + c = ranked.c return ( select(c.colname, c.value_text) - .where(c.rn == 1, c.value_text.is_not(None)) + .where(c.value_text.is_not(None)) # rn == 1 already applied by add_ranking .distinct() .order_by(c.colname, c.value_text) ) diff --git a/deployments/api/tests/db/test_resource_actions.py b/deployments/api/tests/db/test_resource_actions.py index dd3b035f..48b3ff52 100644 --- a/deployments/api/tests/db/test_resource_actions.py +++ b/deployments/api/tests/db/test_resource_actions.py @@ -952,10 +952,15 @@ async def test_returns_distinct_sorted_coalesced_values( {"source": "rmi", "country": None}, {"source": "gem", "country": "CAN"}, ) + # rmi and llm both carry a real, non-null country here. rmi outranks + # llm (SOURCE_PRIORITY), so this also pins the coalesced-winner-only + # cut: llm's GBR is a genuine, eligible value that must not appear + # below just because it lost to rmi's USA. await _create_resource_with_sources( seeded_integration_session, test_user, {"source": "rmi", "country": "USA"}, + {"source": "llm", "country": "GBR"}, ) await _create_resource_with_sources( seeded_integration_session, @@ -997,6 +1002,46 @@ async def test_honors_licensed_sources_after_coalescing( assert options.country == ["CAN"] + @pytest.mark.anyio + async def test_licensing_promotes_next_priority_value( + self, + seeded_integration_session: AsyncSession, + test_user: User, + ): + """Dropping the winning source's license promotes the runner-up value, + rather than blanking the field. + + One resource carries two competing, non-null values for the same + field: wm says USA, gem says CAN. wm outranks gem (SOURCE_PRIORITY), + so wm's USA is the option when both are licensed. The gap this + closes: the existing licensing test + (``test_honors_licensed_sources_after_coalescing``) drops a source + whose own value was already null, so it can't tell "the rank cut runs + after the licensing filter" from "the rank cut runs before it and + just happens to leave the same answer". Losing wm's license here + must promote gem's CAN, not remove the option entirely -- proving the + licensing filter is applied *before* ``rn == 1`` picks the winner, + not after. + """ + await _create_resource_with_sources( + seeded_integration_session, + test_user, + {"source": "wm", "country": "USA"}, + {"source": "gem", "country": "CAN"}, + ) + + with_wm = await resource_actions.filter_options( + seeded_integration_session, + licensed_sources=frozenset({"gem", "wm"}), + ) + assert with_wm.country == ["USA"] + + without_wm = await resource_actions.filter_options( + seeded_integration_session, + licensed_sources=frozenset({"gem"}), + ) + assert without_wm.country == ["CAN"] + @pytest.mark.anyio async def test_excludes_inactive_memberships( self, From 16e7184ccade3bc0f0dea339c66c75731df5b87e Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Tue, 22 Sep 2026 21:54:45 -0600 Subject: [PATCH 5/9] refactor(api): trim filter_option_rows to its own callers and cases - Inline _filter_option_base into filter_option_rows (only one caller) - Trim docstrings to what's short and specific, drop restated context --- deployments/api/src/stitch/api/db/queries.py | 58 +++----------------- 1 file changed, 8 insertions(+), 50 deletions(-) diff --git a/deployments/api/src/stitch/api/db/queries.py b/deployments/api/src/stitch/api/db/queries.py index 308f5c5d..5f4ec5b3 100644 --- a/deployments/api/src/stitch/api/db/queries.py +++ b/deployments/api/src/stitch/api/db/queries.py @@ -89,20 +89,7 @@ def _participating_columns(params: OGFieldQueryParams) -> list[str]: def _override_join(resource_id: Any) -> ColumnElement[bool]: - """The override join condition, shared by every base that ranks values. - - At the value grain (source_pk, colname), and over the override table's - full primary key, so at most one override row per value row -- the outer - join cannot fan out. ``o.source == m.source`` is a dual-key guard: source_pk - and source aren't FK-tied, so matching source_pk alone could apply a stray - override with a mismatched (source, source_pk) pair to the wrong - membership. Requiring both fails safe to the default instead. - - ``resource_id`` is the caller's own resource-identifying column (the - resource table's ``id`` for the full base, the membership's - ``resource_id`` for the lighter filter-option base) since not every base - joins the resource table. - """ + """Join condition matching an override row to its value row.""" m = MembershipModel v = OilGasFieldSourceValueModel o = OGFieldResourceSourcePriority @@ -262,23 +249,16 @@ def coalesced_winner_rows( ) -def _filter_option_base(licensed_sources: Collection[OGSISrcKey] | None = None) -> CTE: - """Light ranking base for ``filter_option_rows``: value rows only, no - resource/source-header joins. - - Deliberately narrower than ``construct_base_query_statement``: it reads - ``value_text`` and the ranking keys only, over the six filterable - columns, so it skips the resource join and the typed ``value_num``/ - ``value_json`` columns that join brings along unused. Dropping the - resource join relies on the invariant that a merged resource's - memberships are all INACTIVE (``_repoint_memberships``). - """ +def filter_option_rows( + licensed_sources: Collection[OGSISrcKey] | None = None, +) -> Select[tuple[str, str]]: + """Distinct winning ``(colname, value)`` pairs for every filterable field.""" m = MembershipModel v = OilGasFieldSourceValueModel p = OGFieldSourcePriority o = OGFieldResourceSourcePriority - stmt = ( + base = ( select( m.resource_id.label("resource_id"), m.source.label("source"), @@ -298,31 +278,9 @@ def _filter_option_base(licensed_sources: Collection[OGSISrcKey] | None = None) ) ) if licensed_sources is not None: - stmt = stmt.where(m.source.in_(list(dict.fromkeys(licensed_sources)))) - return stmt.cte("filter_option_base") + base = base.where(m.source.in_(list(dict.fromkeys(licensed_sources)))) - -def filter_option_rows( - licensed_sources: Collection[OGSISrcKey] | None = None, -) -> Select[tuple[str, str]]: - """Distinct winning ``(colname, value)`` pairs for every filterable field. - - One pass over the light base: the window partitions by - ``(resource_id, colname)``, so a single ROW_NUMBER picks the winner for - all of ``FILTER_OPTION_FIELDS`` at once. ``DISTINCT`` is over the pair, so - a string that is a valid value for two fields survives as two rows and - lands under both. - - Ranking is ``add_ranking`` over ``_filter_option_base`` -- the same - ``_ranked`` tiering the list/detail paths use, so this can't drift from - them on "who beats whom". It still doesn't unify the *row universe*: the - light base skips the resource join, so it doesn't check - ``repointed_id IS NULL`` the way ``construct_base_query_statement`` does - (see the base's docstring for why that's safe). - """ - ranked = add_ranking(_filter_option_base(licensed_sources)).cte( - "filter_option_ranked" - ) + ranked = add_ranking(base.cte("filter_option_base")).cte("filter_option_ranked") c = ranked.c return ( select(c.colname, c.value_text) From 331fcaf6a5dc78663b7e509d35d2861050eabcf2 Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Tue, 22 Sep 2026 22:12:07 -0600 Subject: [PATCH 6/9] chore: use response model only in route handler --- .../api/db/og_field_resource_actions.py | 5 ++-- .../src/stitch/api/routers/oil_gas_fields.py | 3 ++- .../api/tests/db/test_resource_actions.py | 26 +++++++++---------- .../api/tests/routers/test_resources_unit.py | 5 ++-- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/deployments/api/src/stitch/api/db/og_field_resource_actions.py b/deployments/api/src/stitch/api/db/og_field_resource_actions.py index 2a01bf8a..6adcb253 100644 --- a/deployments/api/src/stitch/api/db/og_field_resource_actions.py +++ b/deployments/api/src/stitch/api/db/og_field_resource_actions.py @@ -15,7 +15,6 @@ from stitch.api.auth import CurrentUser from stitch.api.entities import ( FILTER_OPTION_FIELDS, - OGFieldFilterOptionsResponse, OGFieldQueryParams, ) from stitch.api.db.og_field_source_actions import ( @@ -91,7 +90,7 @@ async def query( async def filter_options( session: AsyncSession, licensed_sources: Collection[OGSISrcKey] | None = None, -) -> OGFieldFilterOptionsResponse: +) -> dict[str, list[str]]: """Distinct coalesced values for every filterable field, in one query. Values are priority/override-coalesced and licensed before being deduped and @@ -102,7 +101,7 @@ async def filter_options( options: dict[str, list[str]] = {field: [] for field in FILTER_OPTION_FIELDS} for colname, value in await session.execute(filter_option_rows(licensed_sources)): options[colname].append(value) - return OGFieldFilterOptionsResponse(**options) + return options async def get( diff --git a/deployments/api/src/stitch/api/routers/oil_gas_fields.py b/deployments/api/src/stitch/api/routers/oil_gas_fields.py index ef5cd0eb..9e0c5c04 100644 --- a/deployments/api/src/stitch/api/routers/oil_gas_fields.py +++ b/deployments/api/src/stitch/api/routers/oil_gas_fields.py @@ -92,10 +92,11 @@ async def get_resource_filter_options( _user: CurrentUser, claims: Claims, ) -> OGFieldFilterOptionsResponse: - return await resource_actions.filter_options( + opts = await resource_actions.filter_options( session=uow.session, licensed_sources=licensed_sources(claims), ) + return OGFieldFilterOptionsResponse(**opts) @router.get( diff --git a/deployments/api/tests/db/test_resource_actions.py b/deployments/api/tests/db/test_resource_actions.py index 48b3ff52..bf59a225 100644 --- a/deployments/api/tests/db/test_resource_actions.py +++ b/deployments/api/tests/db/test_resource_actions.py @@ -975,7 +975,7 @@ async def test_returns_distinct_sorted_coalesced_values( options = await resource_actions.filter_options(seeded_integration_session) - assert options.country == ["CAN", "USA"] + assert options["country"] == ["CAN", "USA"] @pytest.mark.anyio async def test_honors_licensed_sources_after_coalescing( @@ -1000,7 +1000,7 @@ async def test_honors_licensed_sources_after_coalescing( licensed_sources=frozenset({"gem", "wm", "llm"}), ) - assert options.country == ["CAN"] + assert options["country"] == ["CAN"] @pytest.mark.anyio async def test_licensing_promotes_next_priority_value( @@ -1034,13 +1034,13 @@ async def test_licensing_promotes_next_priority_value( seeded_integration_session, licensed_sources=frozenset({"gem", "wm"}), ) - assert with_wm.country == ["USA"] + assert with_wm["country"] == ["USA"] without_wm = await resource_actions.filter_options( seeded_integration_session, licensed_sources=frozenset({"gem"}), ) - assert without_wm.country == ["CAN"] + assert without_wm["country"] == ["CAN"] @pytest.mark.anyio async def test_excludes_inactive_memberships( @@ -1068,7 +1068,7 @@ async def test_excludes_inactive_memberships( options = await resource_actions.filter_options(seeded_integration_session) - assert options.country == ["USA"] + assert options["country"] == ["USA"] @pytest.mark.anyio async def test_returns_every_field_including_empty_ones( @@ -1084,11 +1084,11 @@ async def test_returns_every_field_including_empty_ones( options = await resource_actions.filter_options(seeded_integration_session) - assert set(options.model_dump()) == set(FILTER_OPTION_FIELDS) - assert options.country == ["USA"] + assert set(options.keys()) == set(FILTER_OPTION_FIELDS) + assert options["country"] == ["USA"] # A field no source carries is an empty list, never a missing key. - assert options.field_status == [] - assert options.basin == [] + assert options["field_status"] == [] + assert options["basin"] == [] @pytest.mark.anyio async def test_returns_each_field_from_one_pass( @@ -1123,10 +1123,10 @@ async def test_returns_each_field_from_one_pass( options = await resource_actions.filter_options(seeded_integration_session) - assert options.country == ["CAN", "USA"] - assert options.state_province == ["Alberta", "Texas"] - assert options.basin == ["Permian", "Texas"] - assert options.region == ["North America"] + assert options["country"] == ["CAN", "USA"] + assert options["state_province"] == ["Alberta", "Texas"] + assert options["basin"] == ["Permian", "Texas"] + assert options["region"] == ["North America"] def test_postgres_distinct_query_orders_by_selected_columns(self): """``filter_option_rows`` compiles on Postgres. diff --git a/deployments/api/tests/routers/test_resources_unit.py b/deployments/api/tests/routers/test_resources_unit.py index d38d6e61..408d5c04 100644 --- a/deployments/api/tests/routers/test_resources_unit.py +++ b/deployments/api/tests/routers/test_resources_unit.py @@ -8,7 +8,6 @@ from stitch.ogsi.model import OGFieldListItemView, OilGasFieldBase from stitch.api.db.config import get_uow -from stitch.api.entities import OGFieldFilterOptionsResponse from stitch.api.main import app from tests.factories import ResourceCreateFactory, SourceFactory @@ -231,7 +230,7 @@ async def override_get_uow(): with patch("stitch.api.routers.oil_gas_fields.resource_actions") as mock_repo: mock_repo.filter_options = AsyncMock( - return_value=OGFieldFilterOptionsResponse( + return_value=dict( basin=["Permian"], country=["CAN", "USA"], field_status=[], @@ -266,7 +265,7 @@ async def override_get_uow(): with patch("stitch.api.routers.oil_gas_fields.resource_actions") as mock_repo: mock_repo.filter_options = AsyncMock( - return_value=OGFieldFilterOptionsResponse( + return_value=dict( basin=[], country=["CAN", "USA"], field_status=[], From 70d6e937011a795cae722b86e78c7edeb2d0ece9 Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Tue, 22 Sep 2026 22:26:05 -0600 Subject: [PATCH 7/9] chore: drop ai slop comments --- .../api/db/og_field_resource_actions.py | 8 +------ deployments/api/src/stitch/api/entities.py | 10 +------- .../api/tests/db/test_resource_actions.py | 23 +------------------ .../src/components/FilterBar.jsx | 5 ---- .../src/components/ResourcesView.test.jsx | 1 - .../stitch-frontend/src/hooks/useResources.js | 2 -- 6 files changed, 3 insertions(+), 46 deletions(-) diff --git a/deployments/api/src/stitch/api/db/og_field_resource_actions.py b/deployments/api/src/stitch/api/db/og_field_resource_actions.py index 6adcb253..0565aab2 100644 --- a/deployments/api/src/stitch/api/db/og_field_resource_actions.py +++ b/deployments/api/src/stitch/api/db/og_field_resource_actions.py @@ -91,13 +91,7 @@ async def filter_options( session: AsyncSession, licensed_sources: Collection[OGSISrcKey] | None = None, ) -> dict[str, list[str]]: - """Distinct coalesced values for every filterable field, in one query. - - Values are priority/override-coalesced and licensed before being deduped and - sorted, as they are on the list path. Every field is present, empty list - included: grouping appends into a pre-seeded dict, so it does not depend on - the SQL ORDER BY. - """ + """Distinct coalesced values for every filterable field, in one query.""" options: dict[str, list[str]] = {field: [] for field in FILTER_OPTION_FIELDS} for colname, value in await session.execute(filter_option_rows(licensed_sources)): options[colname].append(value) diff --git a/deployments/api/src/stitch/api/entities.py b/deployments/api/src/stitch/api/entities.py index 5bc0a121..ca841be0 100644 --- a/deployments/api/src/stitch/api/entities.py +++ b/deployments/api/src/stitch/api/entities.py @@ -124,12 +124,7 @@ class OGFieldQueryParams(PaginationParams, OGFieldFilterParams, OGFieldSortParam class OGFieldFilterOptionsResponse(BaseModel): - """Every filterable field's distinct coalesced values, one list per field. - - Fields are required, not defaulted: the action always supplies all of them, - empty list included, so a client never has to tell "no licensed values" apart - from "field missing from the payload". - """ + """Every filterable field's distinct coalesced values, one list per field.""" basin: list[str] country: list[str] @@ -139,9 +134,6 @@ class OGFieldFilterOptionsResponse(BaseModel): state_province: list[str] -# The response model is the single source of truth for which fields are -# filterable; the query derives its ``colname`` list from this, so the two can't -# drift. FILTER_OPTION_FIELDS: tuple[str, ...] = tuple(OGFieldFilterOptionsResponse.model_fields) diff --git a/deployments/api/tests/db/test_resource_actions.py b/deployments/api/tests/db/test_resource_actions.py index bf59a225..07b06f6c 100644 --- a/deployments/api/tests/db/test_resource_actions.py +++ b/deployments/api/tests/db/test_resource_actions.py @@ -1010,18 +1010,6 @@ async def test_licensing_promotes_next_priority_value( ): """Dropping the winning source's license promotes the runner-up value, rather than blanking the field. - - One resource carries two competing, non-null values for the same - field: wm says USA, gem says CAN. wm outranks gem (SOURCE_PRIORITY), - so wm's USA is the option when both are licensed. The gap this - closes: the existing licensing test - (``test_honors_licensed_sources_after_coalescing``) drops a source - whose own value was already null, so it can't tell "the rank cut runs - after the licensing filter" from "the rank cut runs before it and - just happens to leave the same answer". Losing wm's license here - must promote gem's CAN, not remove the option entirely -- proving the - licensing filter is applied *before* ``rn == 1`` picks the winner, - not after. """ await _create_resource_with_sources( seeded_integration_session, @@ -1107,9 +1095,6 @@ async def test_returns_each_field_from_one_pass( "region": "North America", }, ) - # "Texas" is a legitimate value for two fields. Both must survive, each - # under its own key -- which is what proves DISTINCT is over the - # (colname, value) pair rather than the bare value. await _create_resource_with_sources( seeded_integration_session, test_user, @@ -1129,13 +1114,7 @@ async def test_returns_each_field_from_one_pass( assert options["region"] == ["North America"] def test_postgres_distinct_query_orders_by_selected_columns(self): - """``filter_option_rows`` compiles on Postgres. - - Postgres rejects SELECT DISTINCT ... ORDER BY over an expression that is - not in the select list. Both ordered columns are selected here, so it - compiles -- but the test suite runs on SQLite, which is laxer, so compile - the real statement against the Postgres dialect to catch that. - """ + """``filter_option_rows`` compiles on Postgres.""" stmt = filter_option_rows( licensed_sources=frozenset({"gem", "wm", "rmi", "llm"}), ) diff --git a/deployments/stitch-frontend/src/components/FilterBar.jsx b/deployments/stitch-frontend/src/components/FilterBar.jsx index 9aa94dc4..64865730 100644 --- a/deployments/stitch-frontend/src/components/FilterBar.jsx +++ b/deployments/stitch-frontend/src/components/FilterBar.jsx @@ -3,8 +3,6 @@ import { FILTER_FIELDS, EMPTY_FILTERS } from "../config/filters"; import { useResourceFilterOptions } from "../hooks/useResources"; export default function FilterBar({ endpoint, filters, onFiltersChange }) { - // One request covers every dropdown: the endpoint returns all filterable - // fields keyed by field name, each with its distinct values. const { data: filterOptions } = useResourceFilterOptions(endpoint); // Flatten active filters into chips: [{ field, label, value, displayValue }, ...] @@ -34,9 +32,6 @@ export default function FilterBar({ endpoint, filters, onFiltersChange }) { {/* Dropdowns row */}
{FILTER_FIELDS.map(({ key, label, formatValue }) => { - // The API sorts by the stored value, which can differ from the order - // of the labels we display (country codes vs. country names), so - // sort by label. const options = (filterOptions?.[key] ?? []) .map((value) => ({ value, diff --git a/deployments/stitch-frontend/src/components/ResourcesView.test.jsx b/deployments/stitch-frontend/src/components/ResourcesView.test.jsx index e6d98143..3be830b5 100644 --- a/deployments/stitch-frontend/src/components/ResourcesView.test.jsx +++ b/deployments/stitch-frontend/src/components/ResourcesView.test.jsx @@ -71,7 +71,6 @@ beforeEach(() => { ...defaultHookReturn, refetch: vi.fn(), }); - // One call, one payload keyed by field -- the shape the endpoint now returns. vi.mocked(useResourceFilterOptions).mockReturnValue({ ...defaultHookReturn, data: { diff --git a/deployments/stitch-frontend/src/hooks/useResources.js b/deployments/stitch-frontend/src/hooks/useResources.js index 2008440d..14009924 100644 --- a/deployments/stitch-frontend/src/hooks/useResources.js +++ b/deployments/stitch-frontend/src/hooks/useResources.js @@ -214,8 +214,6 @@ function getMockResourcePage({ }; } -// Mirrors the API's all-fields payload: one key per dropdown the UI renders, -// always present even when the mock data carries no values for it. function getMockFilterOptions() { return Object.fromEntries( FILTER_FIELDS.map(({ key: field }) => [ From e5652da76e569633770fdb81d8cf1adb02ebe21e Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Tue, 22 Sep 2026 22:27:54 -0600 Subject: [PATCH 8/9] more comment removal --- deployments/api/tests/db/test_resource_actions.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/deployments/api/tests/db/test_resource_actions.py b/deployments/api/tests/db/test_resource_actions.py index 07b06f6c..5f5a648d 100644 --- a/deployments/api/tests/db/test_resource_actions.py +++ b/deployments/api/tests/db/test_resource_actions.py @@ -952,10 +952,6 @@ async def test_returns_distinct_sorted_coalesced_values( {"source": "rmi", "country": None}, {"source": "gem", "country": "CAN"}, ) - # rmi and llm both carry a real, non-null country here. rmi outranks - # llm (SOURCE_PRIORITY), so this also pins the coalesced-winner-only - # cut: llm's GBR is a genuine, eligible value that must not appear - # below just because it lost to rmi's USA. await _create_resource_with_sources( seeded_integration_session, test_user, From c3450d8b6ec69085c58b8f7bc03307fc2baba9a0 Mon Sep 17 00:00:00 2001 From: Michael Barlow Date: Tue, 22 Sep 2026 22:29:10 -0600 Subject: [PATCH 9/9] f**king comments!!! --- deployments/api/tests/db/test_resource_actions.py | 1 - 1 file changed, 1 deletion(-) diff --git a/deployments/api/tests/db/test_resource_actions.py b/deployments/api/tests/db/test_resource_actions.py index 5f5a648d..3699581f 100644 --- a/deployments/api/tests/db/test_resource_actions.py +++ b/deployments/api/tests/db/test_resource_actions.py @@ -1070,7 +1070,6 @@ async def test_returns_every_field_including_empty_ones( assert set(options.keys()) == set(FILTER_OPTION_FIELDS) assert options["country"] == ["USA"] - # A field no source carries is an empty list, never a missing key. assert options["field_status"] == [] assert options["basin"] == []