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.

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

Corrrectly filter /place queries by layer #1036

Merged
merged 1 commit into from Oct 24, 2017

Conversation

Projects
None yet
2 participants
@orangejulius
Member

orangejulius commented Oct 18, 2017

/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 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

Corrrectly filter /place queries by layer
`/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

@orangejulius orangejulius requested a review from dianashk Oct 23, 2017

@dianashk

Oops! 😬

@orangejulius orangejulius merged commit ffc0f45 into master Oct 24, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Oct 24, 2017

@orangejulius orangejulius deleted the improve-place-query branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment