From 02f0fe50f4825c2b46580a2ff9dfe8acb4388d35 Mon Sep 17 00:00:00 2001 From: jakeross Date: Sat, 22 Aug 2026 17:20:53 -0700 Subject: [PATCH] fix(edr): implement pygeoapi's instance contract /ogcapi/collections/waterlevels/instances returned a 500: TypeError: 'NotImplementedError' object is not iterable pygeoapi calls p.instances() and p.instance(id); this provider spelled them get_instances() and get_instance(). Because BaseEDRProvider *returns* a NotImplementedError instance from both rather than raising one, the mismatch was silent -- /instances iterated that object, and /instances/{id}/... validated the id against a truthy object, so any identifier at all was accepted. Transducer deployments have therefore never been reachable as EDR instances, which is exactly what the waterlevels description advertises. Renamed; nothing called the old names. The behave feature that covers this has existed since ADR3 and caught it on the first run, but it is tagged @backend @edr with no @production, and CI runs "@backend and @production and not @skip" -- so it has never run there. Tagged @production. Two fixture defects were hiding behind that, both of which made the chemistry scenarios fail once the feature ran: * The fixture seeded chemistry as observation rows, but d9e0f1a2b3c4 rebuilt ogc_water_chemistry over the legacy NMA_* tables, so the collection saw nothing -- a 400 (pH is not a known parameter) and a 204. It now seeds NMA_Chemistry_SampleInfo/NMA_FieldParameters and refreshes the materialized view, without which the rows stay invisible anyway. * ogc_water_chemistry gates on the thing's release_status as well as the sample's, and wells seed as 'draft', so the fixture published no chemistry at all. It now promotes its own well to public. Full production behave suite: 85 scenarios, 0 failed. Co-Authored-By: Claude Opus 5 --- core/edr_provider.py | 18 ++++++++--- tests/features/edr-water-data.feature | 2 +- tests/features/environment.py | 40 ++++++++++++++++++++++++ tests/test_edr_provider.py | 45 +++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 5 deletions(-) diff --git a/core/edr_provider.py b/core/edr_provider.py index eba54880d..95af6a79d 100644 --- a/core/edr_provider.py +++ b/core/edr_provider.py @@ -181,8 +181,18 @@ def fields(self): return self.get_fields() # ----------------------------------------------------------- instances - def get_instances(self): - """List transducer-deployment instance identifiers.""" + def instances(self): + """List transducer-deployment instance identifiers. + + Named for pygeoapi's EDR contract, not ours: ``get_collection_edr_ + instances`` calls ``p.instances()`` and ``p.instance(id)``, and + ``BaseEDRProvider`` *returns* (rather than raises) a + ``NotImplementedError`` instance from both. A provider that spells + these ``get_instances``/``get_instance`` therefore does not override + anything -- /instances iterates the NotImplementedError object and + 500s, and /instances/{id}/... validates against a truthy object, so + any id at all is accepted. + """ if not self.instance_field: return [] rows = self._fetch( @@ -192,9 +202,9 @@ def get_instances(self): ) return [str(row["iid"]) for row in rows] - def get_instance(self, instance): + def instance(self, instance): """Validate an instance identifier.""" - return instance in set(self.get_instances()) + return str(instance) in set(self.instances()) # ------------------------------------------------------------ queries def locations( diff --git a/tests/features/edr-water-data.feature b/tests/features/edr-water-data.feature index 9e21f56b9..08cd063f8 100644 --- a/tests/features/edr-water-data.feature +++ b/tests/features/edr-water-data.feature @@ -1,4 +1,4 @@ -@backend @edr +@backend @edr @production Feature: OGC API - EDR delivery of water-level and water-chemistry data As a consumer of Bureau observational data I want to query groundwater levels and water chemistry through the standard diff --git a/tests/features/environment.py b/tests/features/environment.py index 4d0f69034..2a7af12dc 100644 --- a/tests/features/environment.py +++ b/tests/features/environment.py @@ -527,6 +527,15 @@ def add_edr_water_data(context, session, well, deployment): lex_term = "(SELECT term FROM lexicon_term LIMIT 1)" + # Wells seed as 'draft', but ogc_water_chemistry gates on the *thing's* + # release status as well as the sample's, so a draft well publishes no + # chemistry at all. Promote this one well -- the fixture exists to give + # the EDR collections something to serve. + session.execute( + text("UPDATE thing SET release_status = 'public' WHERE id = :tid"), + {"tid": well.id}, + ) + # Promote the seeded transducer data to public and give the deployment a # bounded window + recording interval so it reads as an EDR instance. session.execute( @@ -595,6 +604,37 @@ def add_edr_water_data(context, session, well, deployment): {"sid": sid, "pid": pid, "dt": dt, "val": value, "st": status}, ) + # ogc_water_chemistry is built from the legacy NMA_* chemistry tables + # (d9e0f1a2b3c4), not from observation: nothing populates the + # observation -> sample -> parameter chain with analyte data. Seeding + # only observations left the EDR chemistry collection empty, which is + # why its scenarios failed with 400 (pH not a known parameter) and 204. + for public_release, ph_value in ((True, 7.1), (False, 99.0)): + sample_info_id = session.execute( + text( + 'INSERT INTO "NMA_Chemistry_SampleInfo" ' + '(thing_id, "CollectionDate", "PublicRelease", ' + '"nma_SamplePointID") ' + "VALUES (:tid, '2022-06-01T00:00:00Z', :pub, 'EDR-TEST') " + "RETURNING id" + ), + {"tid": well.id, "pub": public_release}, + ).scalar() + session.execute( + text( + 'INSERT INTO "NMA_FieldParameters" ' + '(chemistry_sample_info_id, "FieldParameter", "SampleValue", ' + "\"Units\") VALUES (:csi, 'pH', :val, 'std units')" + ), + {"csi": sample_info_id, "val": ph_value}, + ) + + session.commit() + + # Materialized view: without a refresh the rows just inserted are + # invisible to every chemistry query. + session.execute(text("REFRESH MATERIALIZED VIEW ogc_water_chemistry")) + session.execute(text("REFRESH MATERIALIZED VIEW ogc_internal_water_chemistry")) session.commit() diff --git a/tests/test_edr_provider.py b/tests/test_edr_provider.py index f7ba87dcc..9f60fa4cd 100644 --- a/tests/test_edr_provider.py +++ b/tests/test_edr_provider.py @@ -30,3 +30,48 @@ def test_station_properties_omits_thing_type_when_the_view_lacks_it(): properties = _provider(False)._station_properties({"station_name": "NM-28368"}) assert properties == {"name": "NM-28368"} + + +# ---------------------------------------------------------------- instances + + +def test_provider_implements_pygeoapis_instance_contract(): + """The method names pygeoapi actually calls. + + BaseEDRProvider.instances/instance *return* a NotImplementedError rather + than raising one, so a provider that spells these get_instances/ + get_instance overrides nothing and fails silently at the API layer: + /instances iterates the NotImplementedError object (TypeError -> 500), and + /instances/{id}/... validates the id against a truthy object, accepting + anything. + """ + from pygeoapi.provider.base_edr import BaseEDRProvider + + for name in ("instances", "instance"): + assert name in WaterEDRProvider.__dict__, ( + f"WaterEDRProvider must override {name}() -- pygeoapi calls that " + "name, and the base implementation returns a NotImplementedError " + "object instead of raising." + ) + assert getattr(WaterEDRProvider, name) is not getattr(BaseEDRProvider, name) + + +def test_instances_are_empty_without_an_instance_field(): + # ogc_water_chemistry has no deployments, so its provider declares no + # instance_field and must report an empty list rather than querying. + provider = object.__new__(WaterEDRProvider) + provider.instance_field = None + + assert provider.instances() == [] + + +def test_instance_validation_compares_as_strings(monkeypatch): + # instances() reports identifiers as strings; the id arrives from the URL + # as a string too, but an int must not slip through as valid. + provider = object.__new__(WaterEDRProvider) + provider.instance_field = "deployment_id" + monkeypatch.setattr(WaterEDRProvider, "instances", lambda self: ["7", "9"]) + + assert provider.instance("7") is True + assert provider.instance(7) is True + assert provider.instance("8") is False