Merge pull request #2637 from lonvia/keep-linked-place-names

Introduce separation of names from linked places

lib-php/ParameterParser.php

@@ -114,21 +114,27 @@ class ParameterParser
         }
 
         foreach ($aLanguages as $sLanguage => $fLanguagePref) {
-            $aLangPrefOrder['name:'.$sLanguage] = 'name:'.$sLanguage;
+            $this->addNameTag($aLangPrefOrder, 'name:'.$sLanguage);
         }
-        $aLangPrefOrder['name'] = 'name';
-        $aLangPrefOrder['brand'] = 'brand';
+        $this->addNameTag($aLangPrefOrder, 'name');
+        $this->addNameTag($aLangPrefOrder, 'brand');
         foreach ($aLanguages as $sLanguage => $fLanguagePref) {
-            $aLangPrefOrder['official_name:'.$sLanguage] = 'official_name:'.$sLanguage;
-            $aLangPrefOrder['short_name:'.$sLanguage] = 'short_name:'.$sLanguage;
+            $this->addNameTag($aLangPrefOrder, 'official_name:'.$sLanguage);
+            $this->addNameTag($aLangPrefOrder, 'short_name:'.$sLanguage);
         }
-        $aLangPrefOrder['official_name'] = 'official_name';
-        $aLangPrefOrder['short_name'] = 'short_name';
-        $aLangPrefOrder['ref'] = 'ref';
-        $aLangPrefOrder['type'] = 'type';
+        $this->addNameTag($aLangPrefOrder, 'official_name');
+        $this->addNameTag($aLangPrefOrder, 'short_name');
+        $this->addNameTag($aLangPrefOrder, 'ref');
+        $this->addNameTag($aLangPrefOrder, 'type');
         return $aLangPrefOrder;
     }
 
+    private function addNameTag(&$aLangPrefOrder, $sTag)
+    {
+        $aLangPrefOrder[$sTag] = $sTag;
+        $aLangPrefOrder['_place_'.$sTag] = '_place_'.$sTag;
+    }
+
     public function hasSetAny($aParamNames)
     {
         foreach ($aParamNames as $sName) {

lib-php/PlaceLookup.php

@@ -452,11 +452,7 @@ class PlaceLookup
             }
 
             if ($this->bNameDetails) {
-                if ($aPlace['names']) {
-                    $aPlace['sNameDetails'] = json_decode($aPlace['names']);
-                } else {
-                    $aPlace['sNameDetails'] = (object) array();
-                }
+                $aPlace['sNameDetails'] = $this->extractNames($aPlace['names']);
             }
 
             $aPlace['addresstype'] = ClassTypes\getLabelTag(
@@ -479,6 +475,33 @@ class PlaceLookup
         return $aResults;
     }
 
+
+    private function extractNames($sNames)
+    {
+        if (!$sNames) {
+            return (object) array();
+        }
+
+        $aFullNames = json_decode($sNames);
+        $aNames = array();
+
+        foreach ($aFullNames as $sKey => $sValue) {
+            if (strpos($sKey, '_place_') === 0) {
+                $sSubKey = substr($sKey, 7);
+                if (array_key_exists($sSubKey, $aFullNames)) {
+                    $aNames[$sKey] = $sValue;
+                } else {
+                    $aNames[$sSubKey] = $sValue;
+                }
+            } else {
+                $aNames[$sKey] = $sValue;
+            }
+        }
+
+        return $aNames;
+    }
+
+
     /* returns an array which will contain the keys
      *   aBoundingBox
      * and may also contain one or more of the keys
@@ -489,8 +512,6 @@ class PlaceLookup
      *   lat
      *   lon
      */
-
-
     public function getOutlines($iPlaceID, $fLon = null, $fLat = null, $fRadius = null, $fLonReverse = null, $fLatReverse = null)
     {
 

lib-php/init-website.php

@@ -26,7 +26,7 @@ function userError($sMsg)
 
 function exception_handler_json($exception)
 {
-    http_response_code($exception->getCode());
+    http_response_code($exception->getCode() == 0 ? 500 : $exception->getCode());
     header('Content-type: application/json; charset=utf-8');
     include(CONST_LibDir.'/template/error-json.php');
     exit();
@@ -34,7 +34,7 @@ function exception_handler_json($exception)
 
 function exception_handler_xml($exception)
 {
-    http_response_code($exception->getCode());
+    http_response_code($exception->getCode() == 0 ? 500 : $exception->getCode());
     header('Content-type: text/xml; charset=utf-8');
     echo '<?xml version="1.0" encoding="UTF-8" ?>'."\n";
     include(CONST_LibDir.'/template/error-xml.php');

lib-sql/functions/placex_triggers.sql

@@ -26,6 +26,7 @@ CREATE OR REPLACE FUNCTION placex_indexing_prepare(p placex)
 DECLARE
   location RECORD;
   result prepare_update_info;
+  extra_names HSTORE;
 BEGIN
   -- For POI nodes, check if the address should be derived from a surrounding
   -- building.
@@ -58,8 +59,11 @@ BEGIN
     END LOOP;
   END IF;
 
+  -- remove internal and derived names
   result.address := result.address - '_unlisted_place'::TEXT;
-  result.name := p.name;
+  SELECT hstore(array_agg(key), array_agg(value)) INTO result.name
+    FROM each(p.name) WHERE key not like '\_%';
+
   result.class := p.class;
   result.type := p.type;
   result.country_code := p.country_code;
@@ -72,8 +76,20 @@ BEGIN
   IF location.place_id is not NULL THEN
     result.linked_place_id := location.place_id;
 
-    IF NOT location.name IS NULL THEN
-      result.name := location.name || result.name;
+    IF location.name is not NULL THEN
+      {% if debug %}RAISE WARNING 'Names original: %, location: %', result.name, location.name;{% endif %}
+      -- Add all names from the place nodes that deviate from the name
+      -- in the relation with the prefix '_place_'. Deviation means that
+      -- either the value is different or a given key is missing completely
+      SELECT hstore(array_agg('_place_' || key), array_agg(value)) INTO extra_names
+        FROM each(location.name - result.name);
+      {% if debug %}RAISE WARNING 'Extra names: %', extra_names;{% endif %}
+
+      IF extra_names is not null THEN
+          result.name := result.name || extra_names;
+      END IF;
+
+      {% if debug %}RAISE WARNING 'Final names: %', result.name;{% endif %}
     END IF;
   END IF;
 

test/bdd/db/query/linking.feature

@@ -17,6 +17,40 @@ Feature: Searching linked places
          | object  | linked_place_id |
          | N2      | R13 |
         When sending search query "Vario"
+         | namedetails |
+         | 1 |
         Then results contain
-         | osm |
-         | R13 |
+         | osm | display_name | namedetails |
+         | R13 | Garbo | "name": "Garbo", "name:it": "Vario" |
+        When sending search query "Vario"
+         | accept-language |
+         | it |
+        Then results contain
+         | osm | display_name |
+         | R13 | Vario |
+
+
+    Scenario: Differing names from linked places are searchable
+        Given the places
+         | osm  | class    | type           | admin | name  | geometry |
+         | R13  | boundary | administrative | 6     | Garbo | poly-area:0.1 |
+        Given the places
+         | osm  | class    | type           | admin | name  | geometry |
+         | N2   | place    | hamlet         | 15    | Vario | 0.006 0.00001 |
+        And the relations
+         | id | members       | tags+type |
+         | 13 | N2:label      | boundary |
+        When importing
+        Then placex contains
+         | object  | linked_place_id |
+         | N2      | R13 |
+        When sending search query "Vario"
+         | namedetails |
+         | 1 |
+        Then results contain
+         | osm | display_name | namedetails |
+         | R13 | Garbo        | "name": "Garbo", "_place_name": "Vario" |
+        When sending search query "Garbo"
+        Then results contain
+         | osm | display_name |
+         | R13 | Garbo |

test/bdd/db/update/linked_places.feature

@@ -117,8 +117,10 @@ Feature: Updates of linked places
          | 1  | N3:label |
         When importing
         Then placex contains
-         | object | linked_place_id | name+name:de |
+         | object | linked_place_id | name+_place_name:de |
          | R1     | -               | pnt  |
+        And placex contains
+         | object | linked_place_id | name+name:de |
          | N3     | R1              | pnt  |
         When updating places
          | osm | class    | type        | name+name:de | admin | geometry |
@@ -126,8 +128,43 @@ Feature: Updates of linked places
         Then placex contains
          | object | linked_place_id | name+name:de |
          | N3     | R1              | newname  |
+        And placex contains
+         | object | linked_place_id | name+_place_name:de |
          | R1     | -               | newname  |
 
+    Scenario: Update linking relation when linkee name is deleted
+        Given the places
+         | osm | class    | type           | name | admin | geometry |
+         | R1  | boundary | administrative | rel  | 8     | poly-area:0.1 |
+        And the places
+         | osm | class    | type           | name | admin | geometry |
+         | N3  | place    | city           | pnt  | 30    | 0.00001 0 |
+        And the relations
+         | id | members  |
+         | 1  | N3:label |
+        When importing
+        Then placex contains
+         | object | linked_place_id | name+_place_name | name+name |
+         | R1     | -               | pnt              | rel       |
+        And placex contains
+         | object | linked_place_id | name+name |
+         | N3     | R1              | pnt  |
+        When sending search query "pnt"
+        Then results contain
+          | osm |
+          | R1  |
+        When updating places
+         | osm | class    | type        | name+name:de | admin | geometry |
+         | N3  | place    | city        | depnt        | 30    | 0.00001 0 |
+        Then placex contains
+         | object | linked_place_id | name+name:de |
+         | N3     | R1              | depnt  |
+        And placex contains
+         | object | linked_place_id | name+_place_name:de | name+name |
+         | R1     | -               | depnt               | rel       |
+        When sending search query "pnt"
+        Then exactly 0 results are returned
+
     Scenario: Updating linkee extratags keeps linker's extratags
         Given the named places
          | osm | class    | type           | extra+wikidata | admin | geometry |

test/bdd/steps/http_responses.py

@@ -62,8 +62,6 @@ class GenericResponse:
 
         if errorcode == 200 and fmt != 'debug':
             getattr(self, '_parse_' + fmt)()
-        else:
-            print("Bad response: ", page)
 
     def _parse_json(self):
         m = re.fullmatch(r'([\w$][^(]*)\((.*)\)', self.page)
@@ -74,13 +72,14 @@ class GenericResponse:
             self.header['json_func'] = m.group(1)
         self.result = json.JSONDecoder(object_pairs_hook=OrderedDict).decode(code)
         if isinstance(self.result, OrderedDict):
-            self.result = [self.result]
+            if 'error' in self.result:
+                self.result = []
+            else:
+                self.result = [self.result]
 
     def _parse_geojson(self):
         self._parse_json()
-        if 'error' in self.result[0]:
-            self.result = []
-        else:
+        if self.result:
             self.result = list(map(_geojson_result_to_json_result, self.result[0]['features']))
 
     def _parse_geocodejson(self):
@@ -103,6 +102,9 @@ class GenericResponse:
         elif value.startswith("^"):
             assert re.fullmatch(value, self.result[idx][field]), \
                    BadRowValueAssert(self, idx, field, value)
+        elif isinstance(self.result[idx][field], OrderedDict):
+            assert self.result[idx][field] == eval('{' + value + '}'), \
+                   BadRowValueAssert(self, idx, field, value)
         else:
             assert str(self.result[idx][field]) == str(value), \
                    BadRowValueAssert(self, idx, field, value)

test/bdd/steps/steps_db_ops.py

@@ -93,6 +93,7 @@ def add_data_to_planet_ways(context):
 def import_and_index_data_from_place_table(context):
     """ Import data previously set up in the place table.
     """
+    context.nominatim.run_nominatim('refresh', '--functions')
     context.nominatim.run_nominatim('import', '--continue', 'load-data',
                                               '--index-noanalyse', '-q')
 

test/php/Nominatim/ParameterParserTest.php

@@ -184,75 +184,48 @@ class ParameterParserTest extends \PHPUnit\Framework\TestCase
         $oParams = new ParameterParser(array('accept-language' => ''));
         $this->assertSame(array(
                            'name:default' => 'name:default',
+                           '_place_name:default' => '_place_name:default',
                            'name' => 'name',
-                           'brand' => 'brand',
-                           'official_name:default' => 'official_name:default',
-                           'short_name:default' => 'short_name:default',
-                           'official_name' => 'official_name',
-                           'short_name' => 'short_name',
-                           'ref' => 'ref',
-                           'type' => 'type'
-                          ), $oParams->getPreferredLanguages('default'));
+                           '_place_name' => '_place_name'
+                          ), array_slice($oParams->getPreferredLanguages('default'), 0, 4));
 
         $oParams = new ParameterParser(array('accept-language' => 'de,en'));
         $this->assertSame(array(
                            'name:de' => 'name:de',
+                           '_place_name:de' => '_place_name:de',
                            'name:en' => 'name:en',
+                           '_place_name:en' => '_place_name:en',
                            'name' => 'name',
-                           'brand' => 'brand',
-                           'official_name:de' => 'official_name:de',
-                           'short_name:de' => 'short_name:de',
-                           'official_name:en' => 'official_name:en',
-                           'short_name:en' => 'short_name:en',
-                           'official_name' => 'official_name',
-                           'short_name' => 'short_name',
-                           'ref' => 'ref',
-                           'type' => 'type'
-                          ), $oParams->getPreferredLanguages('default'));
+                           '_place_name' => '_place_name'
+                          ), array_slice($oParams->getPreferredLanguages('default'), 0, 6));
 
         $oParams = new ParameterParser(array('accept-language' => 'fr-ca,fr;q=0.8,en-ca;q=0.5,en;q=0.3'));
         $this->assertSame(array(
                            'name:fr-ca' => 'name:fr-ca',
+                           '_place_name:fr-ca' => '_place_name:fr-ca',
                            'name:fr' => 'name:fr',
+                           '_place_name:fr' => '_place_name:fr',
                            'name:en-ca' => 'name:en-ca',
+                           '_place_name:en-ca' => '_place_name:en-ca',
                            'name:en' => 'name:en',
+                           '_place_name:en' => '_place_name:en',
                            'name' => 'name',
-                           'brand' => 'brand',
-                           'official_name:fr-ca' => 'official_name:fr-ca',
-                           'short_name:fr-ca' => 'short_name:fr-ca',
-                           'official_name:fr' => 'official_name:fr',
-                           'short_name:fr' => 'short_name:fr',
-                           'official_name:en-ca' => 'official_name:en-ca',
-                           'short_name:en-ca' => 'short_name:en-ca',
-                           'official_name:en' => 'official_name:en',
-                           'short_name:en' => 'short_name:en',
-                           'official_name' => 'official_name',
-                           'short_name' => 'short_name',
-                           'ref' => 'ref',
-                           'type' => 'type',
-                          ), $oParams->getPreferredLanguages('default'));
+                           '_place_name' => '_place_name'
+                          ), array_slice($oParams->getPreferredLanguages('default'), 0, 10));
 
         $oParams = new ParameterParser(array('accept-language' => 'ja_rm,zh_pinyin'));
         $this->assertSame(array(
                            'name:ja_rm' => 'name:ja_rm',
+                           '_place_name:ja_rm' => '_place_name:ja_rm',
                            'name:zh_pinyin' => 'name:zh_pinyin',
+                           '_place_name:zh_pinyin' => '_place_name:zh_pinyin',
                            'name:ja' => 'name:ja',
+                           '_place_name:ja' => '_place_name:ja',
                            'name:zh' => 'name:zh',
+                           '_place_name:zh' => '_place_name:zh',
                            'name' => 'name',
-                           'brand' => 'brand',
-                           'official_name:ja_rm' => 'official_name:ja_rm',
-                           'short_name:ja_rm' => 'short_name:ja_rm',
-                           'official_name:zh_pinyin' => 'official_name:zh_pinyin',
-                           'short_name:zh_pinyin' => 'short_name:zh_pinyin',
-                           'official_name:ja' => 'official_name:ja',
-                           'short_name:ja' => 'short_name:ja',
-                           'official_name:zh' => 'official_name:zh',
-                           'short_name:zh' => 'short_name:zh',
-                           'official_name' => 'official_name',
-                           'short_name' => 'short_name',
-                           'ref' => 'ref',
-                           'type' => 'type',
-                          ), $oParams->getPreferredLanguages('default'));
+                           '_place_name' => '_place_name'
+                          ), array_slice($oParams->getPreferredLanguages('default'), 0, 10));
     }
 
     public function testHasSetAny()