From 42cd021d0406970d6c96c029ab83d08dfbe02792 Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Wed, 16 Mar 2022 16:38:52 +0100 Subject: [PATCH 1/6] save differing linked polace names in extra fields This keeps the names tracable and ensures that all names are searchable when they differ. Do not keep names when they are exactly the same to save some space. Linked names are cleaned out before relinking. --- lib-sql/functions/placex_triggers.sql | 19 ++++++++++++++++--- test/bdd/db/query/linking.feature | 20 ++++++++++++++++++++ test/bdd/steps/steps_db_ops.py | 1 + 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/lib-sql/functions/placex_triggers.sql b/lib-sql/functions/placex_triggers.sql index 1eae353e1..e6f083c89 100644 --- a/lib-sql/functions/placex_triggers.sql +++ b/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,17 @@ 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 %} + + result.name := location.name || result.name || extra_names; + {% if debug %}RAISE WARNING 'Final names: %', result.name;{% endif %} END IF; END IF; diff --git a/test/bdd/db/query/linking.feature b/test/bdd/db/query/linking.feature index 4e6c47d88..d11ba31f8 100644 --- a/test/bdd/db/query/linking.feature +++ b/test/bdd/db/query/linking.feature @@ -20,3 +20,23 @@ Feature: Searching linked places Then results contain | osm | | R13 | + + + 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" + Then results contain + | osm | + | R13 | diff --git a/test/bdd/steps/steps_db_ops.py b/test/bdd/steps/steps_db_ops.py index 8df5d6170..e02cad8f4 100644 --- a/test/bdd/steps/steps_db_ops.py +++ b/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') From 17da5f45be95420139ce94adf6e76db8dd5d9c99 Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Wed, 16 Mar 2022 21:44:02 +0100 Subject: [PATCH 2/6] fix return code for PHP exceptions These have returned a 0 until now. --- lib-php/init-website.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib-php/init-website.php b/lib-php/init-website.php index 770c245d4..60367503a 100644 --- a/lib-php/init-website.php +++ b/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 ''."\n"; include(CONST_LibDir.'/template/error-xml.php'); From 524dc64ab77afc08ecaa5bc92e4237c89fb4bc86 Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Wed, 16 Mar 2022 21:44:52 +0100 Subject: [PATCH 3/6] make sure outputs take into account linked place names --- lib-php/ParameterParser.php | 23 ++++++++------ lib-sql/functions/placex_triggers.sql | 5 ++- test/bdd/db/query/linking.feature | 18 ++++++++--- test/bdd/db/update/linked_places.feature | 39 +++++++++++++++++++++++- test/bdd/steps/http_responses.py | 11 +++---- 5 files changed, 75 insertions(+), 21 deletions(-) diff --git a/lib-php/ParameterParser.php b/lib-php/ParameterParser.php index dd637722a..e70b47be0 100644 --- a/lib-php/ParameterParser.php +++ b/lib-php/ParameterParser.php @@ -114,21 +114,26 @@ public function getPreferredLanguages($sFallback = null) } 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) { diff --git a/lib-sql/functions/placex_triggers.sql b/lib-sql/functions/placex_triggers.sql index e6f083c89..9463bb27a 100644 --- a/lib-sql/functions/placex_triggers.sql +++ b/lib-sql/functions/placex_triggers.sql @@ -85,7 +85,10 @@ BEGIN FROM each(location.name - result.name); {% if debug %}RAISE WARNING 'Extra names: %', extra_names;{% endif %} - result.name := location.name || result.name || extra_names; + 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; diff --git a/test/bdd/db/query/linking.feature b/test/bdd/db/query/linking.feature index d11ba31f8..cf1fa20c2 100644 --- a/test/bdd/db/query/linking.feature +++ b/test/bdd/db/query/linking.feature @@ -18,8 +18,14 @@ Feature: Searching linked places | N2 | R13 | When sending search query "Vario" Then results contain - | osm | - | R13 | + | osm | display_name | + | R13 | Garbo | + When sending search query "Vario" + | accept-language | + | it | + Then results contain + | osm | display_name | + | R13 | Vario | Scenario: Differing names from linked places are searchable @@ -38,5 +44,9 @@ Feature: Searching linked places | N2 | R13 | When sending search query "Vario" Then results contain - | osm | - | R13 | + | osm | display_name | + | R13 | Garbo | + When sending search query "Garbo" + Then results contain + | osm | display_name | + | R13 | Garbo | diff --git a/test/bdd/db/update/linked_places.feature b/test/bdd/db/update/linked_places.feature index 7a0fa21a8..99614b7f7 100644 --- a/test/bdd/db/update/linked_places.feature +++ b/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 | diff --git a/test/bdd/steps/http_responses.py b/test/bdd/steps/http_responses.py index fa841d25b..035838a5a 100644 --- a/test/bdd/steps/http_responses.py +++ b/test/bdd/steps/http_responses.py @@ -62,8 +62,6 @@ def __init__(self, page, fmt, errorcode=200): 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 @@ def _parse_json(self): 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): From e133476c355c07dd08a4203dd3f85e96c03105ff Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Thu, 17 Mar 2022 11:02:02 +0100 Subject: [PATCH 4/6] merge linked names correctly into namedetails Convert the '_place_*' entries back to normal entries before returning them in the 'namedetails' section. If the name field is duplicated, kept the '_place_*' notation. This preserves the previous behaviour before _place_ names were introduces but adds the additional names from the linked place for reference. --- lib-php/PlaceLookup.php | 35 ++++++++++++++++++++++++------- test/bdd/db/query/linking.feature | 12 +++++++---- test/bdd/steps/http_responses.py | 3 +++ 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/lib-php/PlaceLookup.php b/lib-php/PlaceLookup.php index 120f55436..715f1ced3 100644 --- a/lib-php/PlaceLookup.php +++ b/lib-php/PlaceLookup.php @@ -452,11 +452,7 @@ public function lookup($aResults, $iMinRank = 0, $iMaxRank = 30) } 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 @@ function ($v) { 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 @@ function ($v) { * lat * lon */ - - public function getOutlines($iPlaceID, $fLon = null, $fLat = null, $fRadius = null, $fLonReverse = null, $fLatReverse = null) { diff --git a/test/bdd/db/query/linking.feature b/test/bdd/db/query/linking.feature index cf1fa20c2..bd8e1da03 100644 --- a/test/bdd/db/query/linking.feature +++ b/test/bdd/db/query/linking.feature @@ -17,9 +17,11 @@ Feature: Searching linked places | object | linked_place_id | | N2 | R13 | When sending search query "Vario" + | namedetails | + | 1 | Then results contain - | osm | display_name | - | R13 | Garbo | + | osm | display_name | namedetails | + | R13 | Garbo | "name": "Garbo", "name:it": "Vario" | When sending search query "Vario" | accept-language | | it | @@ -43,9 +45,11 @@ Feature: Searching linked places | object | linked_place_id | | N2 | R13 | When sending search query "Vario" + | namedetails | + | 1 | Then results contain - | osm | display_name | - | R13 | Garbo | + | osm | display_name | namedetails | + | R13 | Garbo | "name": "Garbo", "_place_name": "Vario" | When sending search query "Garbo" Then results contain | osm | display_name | diff --git a/test/bdd/steps/http_responses.py b/test/bdd/steps/http_responses.py index 035838a5a..3b9f59ebc 100644 --- a/test/bdd/steps/http_responses.py +++ b/test/bdd/steps/http_responses.py @@ -102,6 +102,9 @@ def assert_field(self, idx, field, value): 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) From ce149649431bd0c1a4bc4f04533b305674a4529d Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Thu, 17 Mar 2022 11:05:32 +0100 Subject: [PATCH 5/6] fix linting --- lib-php/ParameterParser.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib-php/ParameterParser.php b/lib-php/ParameterParser.php index e70b47be0..98b95388c 100644 --- a/lib-php/ParameterParser.php +++ b/lib-php/ParameterParser.php @@ -129,7 +129,8 @@ public function getPreferredLanguages($sFallback = null) return $aLangPrefOrder; } - private function addNameTag(&$aLangPrefOrder, $sTag) { + private function addNameTag(&$aLangPrefOrder, $sTag) + { $aLangPrefOrder[$sTag] = $sTag; $aLangPrefOrder['_place_'.$sTag] = '_place_'.$sTag; } From 23de4c7aca63f87db7f2ee003fda0982ac9f2196 Mon Sep 17 00:00:00 2001 From: Sarah Hoffmann Date: Thu, 17 Mar 2022 11:45:05 +0100 Subject: [PATCH 6/6] adapt ParameterParser tests to new key list --- test/php/Nominatim/ParameterParserTest.php | 65 +++++++--------------- 1 file changed, 19 insertions(+), 46 deletions(-) diff --git a/test/php/Nominatim/ParameterParserTest.php b/test/php/Nominatim/ParameterParserTest.php index 57cf5f354..1488c987a 100644 --- a/test/php/Nominatim/ParameterParserTest.php +++ b/test/php/Nominatim/ParameterParserTest.php @@ -184,75 +184,48 @@ public function testGetPreferredLanguages() $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()