Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce separation of names from linked places #2637

Merged
merged 6 commits into from
Mar 17, 2022
Merged

Introduce separation of names from linked places #2637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42cd021
save differing linked polace names in extra fields
lonvia Mar 16, 2022
17da5f4
fix return code for PHP exceptions
lonvia Mar 16, 2022
524dc64
make sure outputs take into account linked place names
lonvia Mar 16, 2022
e133476
merge linked names correctly into namedetails
lonvia Mar 17, 2022
ce14964
fix linting
lonvia Mar 17, 2022
23de4c7
adapt ParameterParser tests to new key list
lonvia Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions lib-php/ParameterParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,21 +114,27 @@ 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) {
Expand Down
35 changes: 28 additions & 7 deletions lib-php/PlaceLookup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -489,8 +512,6 @@ function ($v) {
* lat
* lon
*/


public function getOutlines($iPlaceID, $fLon = null, $fLat = null, $fRadius = null, $fLonReverse = null, $fLatReverse = null)
{

Expand Down
4 changes: 2 additions & 2 deletions lib-php/init-website.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ 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();
}

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');
Expand Down
22 changes: 19 additions & 3 deletions lib-sql/functions/placex_triggers.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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;

Expand Down
38 changes: 36 additions & 2 deletions test/bdd/db/query/linking.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
39 changes: 38 additions & 1 deletion test/bdd/db/update/linked_places.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,54 @@ 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 |
| N3 | place | city | newname | 30 | 0.00001 0 |
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 |
Expand Down
14 changes: 8 additions & 6 deletions test/bdd/steps/http_responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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):
Expand All @@ -103,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)
Expand Down
1 change: 1 addition & 0 deletions test/bdd/steps/steps_db_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down
65 changes: 19 additions & 46 deletions test/php/Nominatim/ParameterParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down