Merge pull request #3889 from lonvia/improve-linkage-code

Small improvements to place linking code

docs/develop/Testing.md

@@ -52,6 +52,15 @@ To run the functional tests, do
 
     pytest test/bdd
 
+You can run a single feature file using expression matching:
+
+    pytest test/bdd -k osm2pgsql/import/entrances.feature
+
+This even works for running single tests by adding the line number of the
+scenario header like that:
+
+    pytest test/bdd -k 'osm2pgsql/import/entrances.feature and L4'
+
 The BDD tests create databases for the tests. You can set name of the databases
 through configuration variables in your `pytest.ini`:
 

lib-sql/functions/placex_triggers.sql

@@ -341,6 +341,22 @@ BEGIN
     END IF;
   END IF;
 
+  IF bnd.extratags ? 'wikidata' THEN
+    FOR linked_placex IN
+      SELECT * FROM placex
+      WHERE placex.class = 'place' AND placex.osm_type = 'N'
+        AND placex.extratags ? 'wikidata' -- needed to select right index
+        AND placex.extratags->'wikidata' = bnd.extratags->'wikidata'
+        AND (placex.linked_place_id is null or placex.linked_place_id = bnd.place_id)
+        AND placex.rank_search < 26
+        AND _st_covers(bnd.geometry, placex.geometry)
+      ORDER BY lower(name->'name') = bnd_name desc
+    LOOP
+      {% if debug %}RAISE WARNING 'Found wikidata-matching place node %', linked_placex.osm_id;{% endif %}
+      RETURN linked_placex;
+    END LOOP;
+  END IF;
+
   -- If extratags has a place tag, look for linked nodes by their place type.
   -- Area and node still have to have the same name.
   IF bnd.extratags ? 'place' and bnd.extratags->'place' != 'postcode'
@@ -361,22 +377,6 @@ BEGIN
     END LOOP;
   END IF;
 
-  IF bnd.extratags ? 'wikidata' THEN
-    FOR linked_placex IN
-      SELECT * FROM placex
-      WHERE placex.class = 'place' AND placex.osm_type = 'N'
-        AND placex.extratags ? 'wikidata' -- needed to select right index
-        AND placex.extratags->'wikidata' = bnd.extratags->'wikidata'
-        AND (placex.linked_place_id is null or placex.linked_place_id = bnd.place_id)
-        AND placex.rank_search < 26
-        AND _st_covers(bnd.geometry, placex.geometry)
-      ORDER BY lower(name->'name') = bnd_name desc
-    LOOP
-      {% if debug %}RAISE WARNING 'Found wikidata-matching place node %', linked_placex.osm_id;{% endif %}
-      RETURN linked_placex;
-    END LOOP;
-  END IF;
-
   -- Name searches can be done for ways as well as relations
   IF bnd_name is not null THEN
     {% if debug %}RAISE WARNING 'Looking for nodes with matching names';{% endif %}
@@ -874,7 +874,7 @@ BEGIN
   -- Remove linkage, if we have computed a different new linkee.
   UPDATE placex SET linked_place_id = null, indexed_status = 2
     WHERE linked_place_id = NEW.place_id
-          and (linked_place is null or linked_place_id != linked_place);
+          and (linked_place is null or place_id != linked_place);
   -- update not necessary for osmline, cause linked_place_id does not exist
 
   -- Postcodes are just here to compute the centroids. They are not searchable

src/nominatim_api/server/falcon/server.py

@@ -7,6 +7,8 @@
 """
 Server implementation using the falcon webserver framework.
 """
+from __future__ import annotations
+
 from typing import Optional, Mapping, Any, List, cast
 from pathlib import Path
 import asyncio
@@ -161,7 +163,7 @@ class APIMiddleware:
 
     def __init__(self, project_dir: Path, environ: Optional[Mapping[str, str]]) -> None:
         self.api = NominatimAPIAsync(project_dir, environ)
-        self.app: Optional[App] = None
+        self.app: Optional[App[Request, Response]] = None
 
     @property
     def config(self) -> Configuration:
@@ -169,7 +171,7 @@ class APIMiddleware:
         """
         return self.api.config
 
-    def set_app(self, app: App) -> None:
+    def set_app(self, app: App[Request, Response]) -> None:
         """ Set the Falcon application this middleware is connected to.
         """
         self.app = app
@@ -193,7 +195,7 @@ class APIMiddleware:
 
 
 def get_application(project_dir: Path,
-                    environ: Optional[Mapping[str, str]] = None) -> App:
+                    environ: Optional[Mapping[str, str]] = None) -> App[Request, Response]:
     """ Create a Nominatim Falcon ASGI application.
     """
     apimw = APIMiddleware(project_dir, environ)
@@ -215,7 +217,7 @@ def get_application(project_dir: Path,
     return app
 
 
-def run_wsgi() -> App:
+def run_wsgi() -> App[Request, Response]:
     """ Entry point for uvicorn.
 
         Make sure uvicorn is run from the project directory.

src/nominatim_db/data/postcode_format.py

@@ -29,6 +29,9 @@ class CountryPostcodeMatcher:
         self.norm_pattern = re.compile(f'\\s*(?:{country_code.upper()}[ -]?)?({pc_pattern})\\s*')
         self.pattern = re.compile(pc_pattern)
 
+        # We want to exclude 0000, 00-000, 000 00 etc
+        self.zero_pattern = re.compile(r'^[0\- ]+$')
+
         self.output = config.get('output', r'\g<0>')
 
     def match(self, postcode: str) -> Optional[Match[str]]:
@@ -40,7 +43,10 @@ class CountryPostcodeMatcher:
         normalized = self.norm_pattern.fullmatch(postcode.upper())
 
         if normalized:
-            return self.pattern.fullmatch(normalized.group(1))
+            match = self.pattern.fullmatch(normalized.group(1))
+            if match and self.zero_pattern.match(match.string):
+                return None
+            return match
 
         return None
 

test/bdd/conftest.py

@@ -9,6 +9,7 @@ Fixtures for BDD test steps
 """
 import sys
 import json
+import re
 from pathlib import Path
 
 import psycopg
@@ -20,7 +21,8 @@ sys.path.insert(0, str(SRC_DIR / 'src'))
 
 import pytest
 from pytest_bdd.parsers import re as step_parse
-from pytest_bdd import given, when, then
+from pytest_bdd import given, when, then, scenario
+from pytest_bdd.feature import get_features
 
 pytest.register_assert_rewrite('utils')
 
@@ -373,3 +375,57 @@ def check_place_missing_lines(db_conn, table, osm_type, osm_id, osm_class):
 
     with db_conn.cursor() as cur:
         assert cur.execute(sql, params).fetchone()[0] == 0
+
+
+if pytest.version_tuple >= (8, 0, 0):
+    def pytest_pycollect_makemodule(module_path, parent):
+        return BddTestCollector.from_parent(parent, path=module_path)
+
+
+class BddTestCollector(pytest.Module):
+
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)
+
+    def collect(self):
+        for item in super().collect():
+            yield item
+
+        if hasattr(self.obj, 'PYTEST_BDD_SCENARIOS'):
+            for path in self.obj.PYTEST_BDD_SCENARIOS:
+                for feature in get_features([str(Path(self.path.parent, path).resolve())]):
+                    yield FeatureFile.from_parent(self,
+                                                  name=str(Path(path, feature.rel_filename)),
+                                                  path=Path(feature.filename),
+                                                  feature=feature)
+
+
+# borrowed from pytest-bdd: src/pytest_bdd/scenario.py
+def make_python_name(string: str) -> str:
+    """Make python attribute name out of a given string."""
+    string = re.sub(r"\W", "", string.replace(" ", "_"))
+    return re.sub(r"^\d+_*", "", string).lower()
+
+
+class FeatureFile(pytest.File):
+    class obj:
+        pass
+
+    def __init__(self, feature, **kwargs):
+        self.feature = feature
+        super().__init__(**kwargs)
+
+    def collect(self):
+        for sname, sobject in self.feature.scenarios.items():
+            class_name = f"L{sobject.line_number}"
+            test_name = "test_" + make_python_name(sname)
+
+            @scenario(self.feature.filename, sname)
+            def _test():
+                pass
+
+            tclass = type(class_name, (),
+                          {test_name: staticmethod(_test)})
+            setattr(self.obj, class_name, tclass)
+
+            yield pytest.Class.from_parent(self, name=class_name, obj=tclass)

test/bdd/test_api.py

@@ -15,7 +15,7 @@ import xml.etree.ElementTree as ET
 
 import pytest
 from pytest_bdd.parsers import re as step_parse
-from pytest_bdd import scenarios, when, given, then
+from pytest_bdd import when, given, then
 
 from nominatim_db import cli
 from nominatim_db.config import Configuration
@@ -150,4 +150,8 @@ def parse_api_json_response(api_response, fmt, num):
     return result
 
 
-scenarios('features/api')
+if pytest.version_tuple >= (8, 0, 0):
+    PYTEST_BDD_SCENARIOS = ['features/api']
+else:
+    from pytest_bdd import scenarios
+    scenarios('features/api')

test/bdd/test_db.py

@@ -15,7 +15,7 @@ import re
 import psycopg
 
 import pytest
-from pytest_bdd import scenarios, when, then, given
+from pytest_bdd import when, then, given
 from pytest_bdd.parsers import re as step_parse
 
 from utils.place_inserter import PlaceColumn
@@ -276,4 +276,8 @@ def then_check_interpolation_table_negative(db_conn, oid):
         assert cur.fetchone()[0] == 0
 
 
-scenarios('features/db')
+if pytest.version_tuple >= (8, 0, 0):
+    PYTEST_BDD_SCENARIOS = ['features/db']
+else:
+    from pytest_bdd import scenarios
+    scenarios('features/db')

test/bdd/test_osm2pgsql.py

@@ -11,7 +11,7 @@ import asyncio
 import random
 
 import pytest
-from pytest_bdd import scenarios, when, then, given
+from pytest_bdd import when, then, given
 from pytest_bdd.parsers import re as step_parse
 
 from nominatim_db import cli
@@ -106,4 +106,8 @@ def check_place_content(db_conn, datatable, node_grid, table, exact):
     check_table_content(db_conn, table, datatable, grid=node_grid, exact=bool(exact))
 
 
-scenarios('features/osm2pgsql')
+if pytest.version_tuple >= (8, 0, 0):
+    PYTEST_BDD_SCENARIOS = ['features/osm2pgsql']
+else:
+    from pytest_bdd import scenarios
+    scenarios('features/osm2pgsql')

test/python/tokenizer/sanitizers/test_clean_postcodes.py

@@ -237,3 +237,9 @@ def test_postcode_default_pattern_pass(sanitize, postcode):
 @pytest.mark.sanitizer_params(convert_to_address=False, default_pattern='[A-Z0-9- ]{3,12}')
 def test_postcode_default_pattern_fail(sanitize, postcode):
     assert sanitize(country='an', postcode=postcode) == []
+
+
+@pytest.mark.parametrize("postcode", ('00000', '00-000', 'PL-00000', 'PL 00-000'))
+@pytest.mark.sanitizer_params(convert_to_address=False)
+def test_postcode_zeros(sanitize, postcode):
+    assert sanitize(country='pl', postcode=postcode) == []