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

Jump to bottom

Place returns venue version of record when address was requested #643

Closed
rmglennon opened this issue Sep 13, 2017 · 3 comments
Closed

Place returns venue version of record when address was requested #643

rmglennon opened this issue Sep 13, 2017 · 3 comments
Assignees
@orangejulius
Labels
bug help wanted on-deck

Comments

@rmglennon
Copy link
Member

Here's what I did 😇

This came in through support (see Desk 980)

  1. Search for Hansaring (this is simplified from the user's query)
    http://pelias.github.io/compare/#/v1/autocomplete%3Ftext=Hansaring
  2. Use the gid of the first result for Hansaring 96 and do a place search on it
    http://pelias.github.io/compare/#/v1/place%3Fids=openstreetmap:address:node:2420772655

Here's what I got 🙀

Properties from autocomplete:

"properties": {
        "id": "node:2420772655",
        "gid": "openstreetmap:address:node:2420772655",
        "layer": "address",
        "source": "openstreetmap",
        "source_id": "node:2420772655",
        "name": "Hansaring 96",
        "housenumber": "96",
        "street": "Hansaring",
        "accuracy": "point",
        "country": "Germany",
        "country_gid": "whosonfirst:country:85633111",
        "country_a": "DEU",
        "region": "Nordrhein-Westfalen",
        "region_gid": "whosonfirst:region:85682513",
        "region_a": "NRW",
        "macrocounty": "Köln",
        "macrocounty_gid": "whosonfirst:macrocounty:404227541",
        "county": "Köln",
        "county_gid": "whosonfirst:county:102063715",
        "locality": "Köln",
        "locality_gid": "whosonfirst:locality:101752771",
        "label": "Hansaring 96, Köln, Germany"
      }

Then, take this gid openstreetmap:address:node:2420772655 and do a place search:

"properties": {
        "id": "node:2420772655",
        "gid": "openstreetmap:venue:node:2420772655",
        "layer": "venue",
        "source": "openstreetmap",
        "source_id": "node:2420772655",
        "name": "Centro Hotel Royal",
        "housenumber": "96",
        "street": "Hansaring",
        "accuracy": "point",
        "country": "Germany",
        "country_gid": "whosonfirst:country:85633111",
        "country_a": "DEU",
        "region": "Nordrhein-Westfalen",
        "region_gid": "whosonfirst:region:85682513",
        "region_a": "NRW",
        "macrocounty": "Köln",
        "macrocounty_gid": "whosonfirst:macrocounty:404227541",
        "county": "Köln",
        "county_gid": "whosonfirst:county:102063715",
        "locality": "Köln",
        "locality_gid": "whosonfirst:locality:101752771",
        "label": "Centro Hotel Royal, Köln, Germany"
      }

Here's what I was expecting ✨

Expecting the osm:address record to be returned and not the venue version.

When you do place with openstreetmap:venue:node:2420772655, the venue record comes back.

Here's what I think could be improved 🏆

Behind the scenes, this is finding a record from OSM that is originally a venue that also contains a full address.

In those cases, we create two records, one in the address layer without venue name and the other in venue layer with all the information.

The place endpoint should not be returning the venue version of the record when address has been requested.

(thanks to @dianashk for explanation about the two records)
@rschreijer
Copy link

I'd like to note that the postalcode field is missing as well.

@orangejulius
Copy link
Member

orangejulius commented Sep 13, 2017 •
edited

We have had other issues like this, such as the following query:

http://pelias.github.io/compare/#/v1/place%3Fids=whosonfirst:country:123
image

In this case there is no Who's on First record with ID 123, but the underlying Elasticsearch query is searching for any record with ID 123, and finds a Geonames locality

@dianashk dianashk added on-deck and removed Q3-2017 labels Oct 16, 2017
@orangejulius orangejulius mentioned this issue Oct 17, 2017
Merged
orangejulius added a commit to pelias/api that referenced this issue Oct 18, 2017
@orangejulius
Corrrectly filter /place queries by layer
be17250 
`/place` queries have been executing in a way where only the ID, but not
the layer, has been considered when returning records from
Elasticsearch.

It turns out this bug was introduced almost a year and a half ago in
#407. A little, relatively unimportant
bit of code was looking for a property called `layers`:

```
    const cmd = req.clean.ids.map( function(id) {
      return {
        _index: apiConfig.indexName,
        _type: id.layers,
        _id: id.id
      };
    });
```

The correct property was `layer`, so no filtering on layer was done in
the resulting [mget](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-multi-get.html) query.

There was never an acceptance test for these sorts of queries, but there
is now one in pelias/acceptance-tests#446

Fixes pelias/pelias#643
orangejulius added a commit to pelias/api that referenced this issue Oct 18, 2017
@orangejulius
Corrrectly filter /place queries by layer
feeb0d2 
`/place` queries have been executing in a way where only the ID, but not
the layer, has been considered when returning records from
Elasticsearch.

It turns out this bug was introduced almost a year and a half ago in
#407. A little, relatively unimportant
bit of code was looking for a property called `layers`:

```
    const cmd = req.clean.ids.map( function(id) {
      return {
        _index: apiConfig.indexName,
        _type: id.layers,
        _id: id.id
      };
    });
```

The correct property was `layer`, so no filtering on layer was done in
the resulting [mget](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-multi-get.html) query.

There was never an acceptance test for these sorts of queries, but there
is now one in pelias/acceptance-tests#446. The
unit tests were enforcing the incorrect behavior.

Fixes pelias/pelias#643
This was referenced Oct 18, 2017
Merged
Closed
@orangejulius
Copy link
Member

We should have a fix for the originally reported issue in pelias/api#1036.

The issue I reported is actually a different one, and is being discussed over in #672.

orangejulius added a commit to pelias/api that referenced this issue Oct 23, 2017
@orangejulius
Corrrectly filter /place queries by layer
f928119 
`/place` queries have been executing in a way where only the ID, but not
the layer, has been considered when returning records from
Elasticsearch.

It turns out this bug was introduced almost a year and a half ago in
#407. A little, relatively unimportant
bit of code was looking for a property called `layers`:

```
    const cmd = req.clean.ids.map( function(id) {
      return {
        _index: apiConfig.indexName,
        _type: id.layers,
        _id: id.id
      };
    });
```

The correct property was `layer`, so no filtering on layer was done in
the resulting [mget](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-multi-get.html) query.

There was never an acceptance test for these sorts of queries, but there
is now one in pelias/acceptance-tests#446. The
unit tests were enforcing the incorrect behavior.

Fixes pelias/pelias#643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@orangejulius orangejulius

Labels
bug help wanted on-deck
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@orangejulius @rschreijer @rmglennon @dianashk