From d0ad65f69626664ee90a5f18a333a7addc2efc73 Mon Sep 17 00:00:00 2001 From: Emily Love Watson Date: Thu, 21 Aug 2025 11:35:23 -0500 Subject: [PATCH] Select all entrances for results in one query --- lib-sql/functions/place_triggers.sql | 37 ++----------- lib-sql/functions/placex_triggers.sql | 2 - lib-sql/functions/utils.sql | 18 ------- lib-sql/table-triggers.sql | 4 -- src/nominatim_api/results.py | 19 +++---- test/bdd/conftest.py | 17 ++++++ test/bdd/features/api/details/params.feature | 4 +- test/bdd/features/db/update/entrances.feature | 53 ++++++++++--------- test/bdd/utils/checks.py | 10 +++- 9 files changed, 72 insertions(+), 92 deletions(-) diff --git a/lib-sql/functions/place_triggers.sql b/lib-sql/functions/place_triggers.sql index f9e17805..216ab487 100644 --- a/lib-sql/functions/place_triggers.sql +++ b/lib-sql/functions/place_triggers.sql @@ -338,6 +338,11 @@ BEGIN END IF; END IF; + -- When an existing way is updated, recalculate entrances + IF existingplacex.osm_type = 'W' and (existingplacex.rank_search > 27 or existingplacex.class IN ('landuse', 'leisure')) THEN + PERFORM place_update_entrances(existingplacex.place_id, existingplacex.osm_id); + END IF; + -- Abort the insertion (we modified the existing place instead) RETURN NULL; END; @@ -365,35 +370,3 @@ BEGIN RETURN NULL; END; $$ LANGUAGE plpgsql; - -CREATE OR REPLACE FUNCTION place_after_insert() - RETURNS TRIGGER - AS $$ -BEGIN - {% if debug %} - RAISE WARNING 'place_after_insert: % % % % %',NEW.osm_type,NEW.osm_id,NEW.class,NEW.type,st_area(NEW.geometry); - {% endif %} - - IF NEW.class IN ('routing:entrance', 'entrance') THEN - PERFORM place_update_entrances_for_node(NEW.osm_id); - END IF; - - RETURN NEW; -END; -$$ LANGUAGE plpgsql; - -CREATE OR REPLACE FUNCTION place_after_delete() - RETURNS TRIGGER - AS $$ -BEGIN - {% if debug %} - RAISE WARNING 'place_after_delete: % % % % %',OLD.osm_type,OLD.osm_id,OLD.class,OLD.type,st_area(OLD.geometry); - {% endif %} - - IF OLD.class IN ('routing:entrance', 'entrance') THEN - PERFORM place_update_entrances_for_node(OLD.osm_id); - END IF; - - RETURN NULL; -END; -$$ LANGUAGE plpgsql; diff --git a/lib-sql/functions/placex_triggers.sql b/lib-sql/functions/placex_triggers.sql index 14a1228d..d5aafa3f 100644 --- a/lib-sql/functions/placex_triggers.sql +++ b/lib-sql/functions/placex_triggers.sql @@ -818,8 +818,6 @@ DECLARE nameaddress_vector INTEGER[]; addr_nameaddress_vector INTEGER[]; - entrances JSONB; - linked_place BIGINT; linked_node_id BIGINT; diff --git a/lib-sql/functions/utils.sql b/lib-sql/functions/utils.sql index ee5af047..6f9db61f 100644 --- a/lib-sql/functions/utils.sql +++ b/lib-sql/functions/utils.sql @@ -625,24 +625,6 @@ END; $$ LANGUAGE plpgsql; -CREATE OR REPLACE FUNCTION place_update_entrances_for_node(osmid BIGINT) - RETURNS INTEGER - AS $$ -DECLARE - entrance_way RECORD; -BEGIN - FOR entrance_way IN - SELECT osm_id, place_id FROM planet_osm_ways JOIN placex ON placex.osm_id = planet_osm_ways.id WHERE osmid=ANY(nodes) - LOOP - PERFORM place_update_entrances(entrance_way.place_id, entrance_way.osm_id); - END LOOP; - - RETURN NULL; -END; -$$ -LANGUAGE plpgsql; - - CREATE OR REPLACE FUNCTION place_update_entrances(placeid BIGINT, osmid BIGINT) RETURNS INTEGER AS $$ diff --git a/lib-sql/table-triggers.sql b/lib-sql/table-triggers.sql index 078a9d98..d1a37316 100644 --- a/lib-sql/table-triggers.sql +++ b/lib-sql/table-triggers.sql @@ -24,10 +24,6 @@ CREATE TRIGGER place_before_delete BEFORE DELETE ON place FOR EACH ROW EXECUTE PROCEDURE place_delete(); CREATE TRIGGER place_before_insert BEFORE INSERT ON place FOR EACH ROW EXECUTE PROCEDURE place_insert(); -CREATE TRIGGER place_after_insert AFTER INSERT ON place - FOR EACH ROW EXECUTE PROCEDURE place_after_insert(); -CREATE TRIGGER place_after_delete AFTER DELETE ON place - FOR EACH ROW EXECUTE PROCEDURE place_after_delete(); CREATE TRIGGER location_postcode_before_update BEFORE UPDATE ON location_postcode FOR EACH ROW EXECUTE PROCEDURE postcode_update(); diff --git a/src/nominatim_api/results.py b/src/nominatim_api/results.py index 918b78c2..0b887280 100644 --- a/src/nominatim_api/results.py +++ b/src/nominatim_api/results.py @@ -470,8 +470,7 @@ async def add_result_details(conn: SearchConnection, results: List[BaseResultT], await complete_parented_places(conn, result) if details.entrances: log().comment('Query entrances details') - for result in results: - await complete_entrances_details(conn, result) + await complete_entrances_details(conn, results) if details.keywords: log().comment('Query keywords') for result in results: @@ -723,17 +722,19 @@ async def complete_linked_places(conn: SearchConnection, result: BaseResult) -> result.linked_rows.append(_result_row_to_address_row(row)) -async def complete_entrances_details(conn: SearchConnection, result: BaseResult) -> None: +async def complete_entrances_details(conn: SearchConnection, results: List[BaseResultT]) -> None: """ Retrieve information about tagged entrances for this place. """ - if result.source_table != SourceTable.PLACEX: - return - t = conn.t.place_entrance - sql = sa.select(t.c.entrances).where(t.c.place_id == result.place_id) + place_ids = (r.place_id for r in results) + sql = sa.select(t.c.place_id, t.c.entrances).where(t.c.place_id.in_(place_ids)) - for results in await conn.execute(sql): - result.entrances = [EntranceDetails(**r) for r in results[0]] + current_result = None + for place_id, entrances in await conn.execute(sql): + if current_result is None or place_id != current_result.place_id: + current_result = next((r for r in results if r.place_id == place_id), None) + assert current_result is not None + current_result.entrances = [EntranceDetails(**r) for r in entrances] async def complete_keywords(conn: SearchConnection, result: BaseResult) -> None: diff --git a/test/bdd/conftest.py b/test/bdd/conftest.py index 6d2b0b69..2135083f 100644 --- a/test/bdd/conftest.py +++ b/test/bdd/conftest.py @@ -247,6 +247,23 @@ def check_result_for_field_absence(nominatim_result, attributes): assert all(a not in nominatim_result.result for a in attributes) +@then(step_parse( + r'the result contains array field (?P\S+) where element (?P\d+) contains'), + converters={'num': int}) +def check_result_array_field_for_attributes(nominatim_result, datatable, field, num): + assert nominatim_result.is_simple() + + if datatable[0] == ['param', 'value']: + pairs = datatable[1:] + else: + pairs = zip(datatable[0], datatable[1]) + + prefix = f"{field}+{num}+" + + for k, v in pairs: + assert ResultAttr(nominatim_result.result, prefix + k) == v + + @then(step_parse('the result set contains(?P exactly)?')) def check_result_list_match(nominatim_result, datatable, exact): assert not nominatim_result.is_simple() diff --git a/test/bdd/features/api/details/params.feature b/test/bdd/features/api/details/params.feature index 5f76ded1..a9a340fc 100644 --- a/test/bdd/features/api/details/params.feature +++ b/test/bdd/features/api/details/params.feature @@ -36,7 +36,9 @@ Feature: Object details | W | 429210603 | 1 | Then a HTTP 200 is returned And the result is valid json - And the result has attributes entrances + And the result contains array field entrances where element 0 contains + | osm_id | type | lat | lon | + | 6580031131 | yes | 47.2489382 | 9.5284033 | Scenario: Details with linkedplaces When sending v1/details diff --git a/test/bdd/features/db/update/entrances.feature b/test/bdd/features/db/update/entrances.feature index 4cabe2a5..8d0091d5 100644 --- a/test/bdd/features/db/update/entrances.feature +++ b/test/bdd/features/db/update/entrances.feature @@ -9,8 +9,8 @@ Feature: Entrance nodes are recorded | osm | class | type | geometry | | W1 | building | yes | (1,2,3,4,1) | And the ways - | id | nodes | - | 1 | 10,20,30,40 | + | id | nodes | + | 1 | 1, 2, 3, 4, 1 | When importing Then placex contains exactly | object | place_id | @@ -18,63 +18,68 @@ Feature: Entrance nodes are recorded Then place_entrance contains exactly | place_id | entrances | When updating places - | osm | class | type | geometry | - | N10 | entrance | main | 1 | + | osm | class | type | geometry | + | N1 | entrance | main | 1 | + | W1 | building | yes | (1,2,3,4,1) | Then placex contains exactly | object | place_id | | W1 | 1 | And place_entrance contains exactly | place_id | entrances | - | 1 | [{'lat': 0, 'lon': 0, 'type': 'main', 'osm_id': 10, 'extratags': None}] | + | 1 | [{'lat': 0, 'lon': 0, 'type': 'main', 'osm_id': 1, 'extratags': None}] | Scenario: A building with a updated entrance node Given the grid | 1 | 2 | | 4 | 3 | Given the places - | osm | class | type | geometry | - | W1 | building | yes | (1,2,3,4,1) | - | N10 | barrier | gate | 1 | + | osm | class | type | geometry | + | N1 | barrier | gate | 1 | + | W1 | building | yes | (1,2,3,4,1) | And the ways - | id | nodes | - | 1 | 10,20,30,40 | + | id | nodes | + | 1 | 1, 2, 3, 4, 1 | When importing Then placex contains exactly | object | place_id | - | W1 | 1 | - | N10 | 2 | + | N1 | 1 | + | W1 | 2 | Then place_entrance contains exactly | place_id | entrances | When updating places - | osm | class | type | geometry | - | N10 | entrance | main | 1 | + | osm | class | type | geometry | + | N1 | entrance | main | 1 | + | W1 | building | yes | (1,2,3,4,1) | Then placex contains exactly | object | place_id | - | W1 | 1 | - | N10 | 2 | + | N1 | 1 | + | W1 | 2 | And place_entrance contains exactly | place_id | entrances | - | 1 | [{'lat': 0, 'lon': 0, 'type': 'main', 'osm_id': 10, 'extratags': None}] | + | 2 | [{'lat': 0, 'lon': 0, 'type': 'main', 'osm_id': 1, 'extratags': None}] | Scenario: A building with a removed entrance Given the grid | 1 | 2 | | 4 | 3 | Given the places - | osm | class | type | geometry | - | W1 | building | yes | (1,2,3,4,1) | - | N10 | entrance | main | 1 | + | osm | class | type | geometry | + | N1 | entrance | main | 1 | + | W1 | building | yes | (1,2,3,4,1) | And the ways - | id | nodes | - | 1 | 10,20,30,40 | + | id | nodes | + | 1 | 1, 2, 3, 4, 1 | When importing Then placex contains exactly | object | place_id | | W1 | 1 | And place_entrance contains exactly | place_id | entrances | - | 1 | [{'lat': 0, 'lon': 0, 'type': 'main', 'osm_id': 10, 'extratags': None}] | - When marking for delete N10 + | 1 | [{'lat': 0, 'lon': 0, 'type': 'main', 'osm_id': 1, 'extratags': None}] | + When marking for delete N1 + And updating places + | osm | class | type | geometry | + | W1 | building | yes | (2,3,4,2) | Then placex contains exactly | object | place_id | | W1 | 1 | diff --git a/test/bdd/utils/checks.py b/test/bdd/utils/checks.py index 592dad69..c6822d4a 100644 --- a/test/bdd/utils/checks.py +++ b/test/bdd/utils/checks.py @@ -7,6 +7,7 @@ """ Helper functions to compare expected values. """ +import collections.abc import json import re import math @@ -102,8 +103,13 @@ class ResultAttr: self.subobj = self.obj for sub in self.key.split('+'): done += f"[{sub}]" - assert sub in self.subobj, \ - f"Missing attribute {done}. Full object:\n{_pretty(self.obj)}" + if isinstance(self.subobj, collections.abc.Sequence) and sub.isdigit(): + sub = int(sub) + assert sub < len(self.subobj), \ + f"Out of bound index {done}. Full object:\n{_pretty(self.obj)}" + else: + assert sub in self.subobj, \ + f"Missing attribute {done}. Full object:\n{_pretty(self.obj)}" self.subobj = self.subobj[sub] def __eq__(self, other): -- 2.39.5