Add boundary.gid parameter to search and autocomplete endpoints #1249
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
missinglink
reviewed
Dec 3, 2018
sanitizer/_boundary_wof.js
Outdated
| } | ||
|
|
||
| // must be only digits | ||
| else if ( /^\d+$/.test(wof_id) ) { |
There was a problem hiding this comment.
Looks like it's possible for the wof_id to be a non-string value here, for instance, it could be an array if someone used the [] array URI scheme.
What would be the effect of calling .test() on a non-string value?
There was a problem hiding this comment.
Do you know of a sample query that would result in a non-string value that I could use to test with? I tried eg: boundary.wof[]=420551221 and boundary.wof[0]=420551221&boundary.wof[1]=123 but they both get caught by the _single_scalar_parameters sanitizer.
There was a problem hiding this comment.
Nice. Good to know one of @trescube's first ever contributions long ago is still preventing problems.
|
Thanks @thismakessand, looks great. It'll take us a few days at least to read through and review your work, we're currently in transit from a conference in Australia so please bear with us. @orangejulius this is a really great contribution, I'll schedule some time to do a more thorough review of the code. |
|
This is an awesome PR, thanks @thismakessand. Like Peter said, a few days of jetlag recovery are needed for a proper review, but it's really good to see this feature be implemented. It will be big for a lot of people. |
|
Hey @thismakessand, We'd like to support more administrative datasets than just WOF in the future, so it would be nice if the parameter was instead called something like Since internally we are a long way from supporting multiple administrative datasets, I think it's enough for you to only change the API interface. We can easily make any refactoring or rewriting of code as needed later, but changing the API interface to Pelias would be a breaking change for our users. I think your existing @missinglink, let me know if this matches with what you think is best, and @thismakessand, let me know if you have any questions! |
|
That sounds perfect, it would be great if the I guess that also means coming up with another name for it, what about Should we support specifying multiple GIDs such as |
|
Thanks - I should be able to make these changes this week. For the Also, should I add boundary.gid to the /reverse endpoint as well? Thanks |
|
@thismakessand awesome. You shouldn't have to bother with specifying only a single layer. Elasticsearch is plenty fast at searching multiple fields and the ID itself is unique.
|
|
@orangejulius I made the api changes requested and updated the name 'boundary wof' to 'boundary gid' everywhere (including pelias-query) so it's consistent. Let me know if there's something I need to do re: the failing CI test, it appears unrelated to anything I worked on and I can't reproduce it locally. |
|
@thismakessand awesome, thanks. Everything is looking great. The failing tests are just because the pelias/query PR wasn't merged. In a few minutes, once a new version of the pelias-query NPM package is published, I'll restart the tests and they should pass. @missinglink as it stands, this code works great for everything except |
orangejulius
added a commit
to pelias/query
that referenced
this pull request
Jan 3, 2019
This was done in ad0d7f4 Connects pelias/api#1249 Connects pelias/api#373
|
@orangejulius Oops, I thought I tested the placeholder pathway but apparently not. It looks like the placeholder results contain the ids in the hierarchy so I just pushed a new commit that adds a filter function to the placeholder controller, similar to how the boundary.country is working. |
orangejulius
added a commit
to pelias/acceptance-tests
that referenced
this pull request
Jan 10, 2019
Some simple tests for boundary.gid! Connects pelias/api#1249
orangejulius
added a commit
that referenced
this pull request
Jan 10, 2019
|
@thismakessand thanks so much for this PR. I've tested it out, and it all looks great. I put some acceptance-tests together in pelias/acceptance-tests#486 so this is all good to go. We might end up writing a blog post about this feature at some point soon, would you mind if we credit you and perhaps learned a bit about your motivation for working on this, how it went in general, etc? Shoot us an email at hello@geocode.earth if so. Thanks again :) |
orangejulius
changed the title
orangejulius
added a commit
to pelias/documentation
that referenced
this pull request
Mar 5, 2019
Documentation for this new parameter with a full description in the `/v1/search` documentation, and mentions in the documentation for `/v1/autocomplete` and `/v1/reverse` Connects pelias/api#1249 Connects pelias/api#373
jsvrcek
added a commit
to EventKit/pelias-api
that referenced
this pull request
Oct 8, 2019
* chore(package): update ciao to version 2.0.0 * chore(ciao): Update ciao tests Amazingly in over a year since running these, only two things had to be changed. * fix(errors): Only return 400 status on known parameter errors Pelias has for a long time returned 400 as a default status whenever anything goes wrong, as well as when a user has passed invalid parameters. By using a new exception class, it is now possible to differetiate between known parameter errors, and unexpected errors that truly represent an HTTP 500. * feat(trimByGranularity): refactor code to add logging and debug output, modify loop to break after first match * feat(trimByGranularity): refactor to use an index instead of multiple loops over the results * feat(address_search_using_ids): add postcode to variable store if we have it * Update pelias-query to 9.7.0 This is required for the other half of postalcode matching * fix(log): Log for each interpolation request The groundwork for this was laid in pelias#1220, but it wasn't completed. By logging information about each individual interpolation request (a single API response may require many calls to the interpolation service if several streets were returned from a query), we can get a good overview of the interpolation service's performance. * feat(log): Add full clean context to parser logs Without the unmodified text input, it's hard to tell what cases is causing these messages. * feat(max_character_count_layer_filter): filter layer=address records for character counts <= 2 * feat(autocomplete): drive address filter by config * feat(unit_number_only_queries): do not consider parses with only the unit number as "admin only" * feat(libpostal_numeric_inputs): recast entirely numeric parses to label "house" * feat(focus.point): use exponential instead of linear scoring Linear scoring, by design, gives all records the same score past a certain point. This has the disadvantage that identical records that are very far away cannot be sorted by distance. By using exponential scoring, we can achieve decent sorting of even very far away records. This is very helpful for cities and postalcodes. Connects pelias#1206 * fix(autocomplete focus.point): Use 50km scale parameter The `scale` parameter controls how quickly scores decrease from the maximum as the distance from the `center_point` to the record in question increases. Set this to 50km, which is the same as search. Connects pelias#1206 * Use test description consistent with fixture file * feat(test): add test cases for autocomplete token matching permutations * feat(refactor): refactor addressit sanitizer for readability * feat(refactor): refactor addressit sanitizer - bugfix * Update service/configurations/Interpolation.js Change Language to Interpolation to fix copy/paste error. * feat(docs): update gitter link Most of our chatting happens in pelias/pelias, not pelias/api * chore(package): update dependencies * travis: reflect new best-practice travis-ci configuration Travis-CI has or will shortly make in early December 2018 a number of beneficial changes to their Linux continuous integration testing infrastructure. Changes that impact pelias/api are: * Linux infrastructure combined into one (virtualized), from two previously (virtualized and container-based). [0][1] * Offering a more modern, supported Ubuntu Xenial (16.04 LTS) [2] Projects using "sudo: false" (container-based infrastructure), have been recommended to remove that configuration soon. In any case, the transition will happen regardless for projects by December 7, 2018. [0] https://blog.travis-ci.com/2018-10-04-combining-linux-infrastructures [1] https://blog.travis-ci.com/2018-11-19-required-linux-infrastructure-migration [2] https://docs.travis-ci.com/user/reference/xenial/ * Add boundary.wof parameter to search, search/structured and autocomplete endpoints * fix(dedupe_query_size_1): check size when deduping * feat(autocomplete_empty_tokens): remove empty tokens * Change name from boundary.wof to boundary.gid and update param format * Add boundary.gid to /reverse endpoint * Add filter function for Placeholder results * Cleanup * feat(params): Add boundary.gid parameter Connects pelias#1249 * feat(stable_sort): fix stable sorting on node 11.6.0 * feat(dedupe): improved matching across languages * feat(dedupe): consider the user agent language in deduplication * feat(dedupe_improved_parent_matching): only check parent fields above the layer of the least granular match * feat(inter_layer_deduping): dedupe between layers and prefer some layers over others * feat(dedupe_improved_parent_matching): do not compare parent items at same level, use explicit list of synonymous layers * feat(dedupe_improved_parent_matching): add Vientiane test case * fix typos * fix(dedupe): Compare parent layers above lowest record This expands the layers checked for hierarchy difference to be any layer higher than the _lower_ of the two records being compared. Previously, it was any layer higher than the _higher_ of the two record (another way to think about it was it was only comparing hierarchy elements that BOTH records have). This fixes the issue where the city of Pennsylvania, Illinois was being considered a duplicate of the state of Pennsylvania. * feat(query) Set cutoff_frequency in all queries * feat(addendum): display addendum data in geojson * docs: Add link to Pelias services documentation The rules for when different Pelias services are required or recommended is now too complex to fit in the API readme. Fortunately we have it all written down in one place. * fix(package): update addressit to version 1.7.0 * feat(search): enable fallback queries when placeholder returns 0 results * feat(admin_ngram): query ngram fields for admin matching on autocomplete * fix(jshint): change syntax to silence jshint eerror on commit * feat(admin_ngrams): revert ccb9bc8 * fix(logging): Remove log of every query sent to addressit This is pretty verbose as its fairly common, and we can determine what queries were sent to addressit in other ways. * fix(logging): Clean up coarse reverse error log It was logging both info and error level messages. While not commonly called, this seems unnecessary. * fix(logging): allow access log to be empty This permits disabling access logs, which may or may not be useful in different use cases. * correct -> incorrect in documentation This error completely changed the meaning of a line in documentation * feat(deps): Remove `tmp` devDependency This package hasn't been used since 33341b7, which was removed in favor of the [microservice-wrapper](https://github.com/pelias/microservice-wrapper). Closes pelias#1266 * feat(admin_ngrams): Restore admin ngrams functionality This restores the functionality from pelias#1259 to allow autocomplete on partial administrative area names. It's a solid improvement, and we are waiting to merge only to give the community time to update their schema settings. This reverts commit 43a4bed. * feat(deps): Remove unused source-map dependency Like pelias#1269, this dev dependency was not being used. We added it long ago to try to fix issues with resolving NPM dependencies, but those issues haven't been seen recently. * fix(package): update check-types to version 8.0.0 * feat(categories): enable filter for autocomplete, show categories in geojson even when categories param is blank * feat(categories): gracefully handle a mixture of valid and invalid categories * Allow size parameter for autocomplete endpoint. * Add support for boundary circle to autocomplete. * Make sanitizer aware of boundary.circle. * feat(multi boundary.country): Support for multi `boundary.country` query parameter * fix(query): add code comments and sanity check to text_parser_addressit * feat(performance): add single_token_address_filter sanitizer * feat(performance): update single_token_address_filter to consider only layers relevant to clean.sources * feat(performance): rename single_token_address_filter to _address_layer_filter, add numeric check, emit warning * feat(performance): use parsed_text input to determine token length where available * fix(package): update pelias-config to version 4.0.0 Closes pelias#1289 * feat(localNamingConventions): fix for ESP street number formatting * Allow postalcode + admin area queries We currently have very strict logic for which combinations of inputs the fallback search query supports. Any input that is out of alignment with those expectations will be deferred to the lower quality addressit query system. This changes some of that expectation logic. Where previously it ignored inputs with a postcode and any other field, now a postcode in combination with any administrative fields are allowed. This is so, for example, a user can query for '90210, California' and hopefully see postalcodes called 90210 that are in California first in the results. Obviously postalcodes are complex so there might be a few cases that don't work, but it should help. As a bonus, it will remove fragility and complexity from our query generation. * feat(cutoff_frequency): enable cutoff_frequency for multi_match queries * feat(docs): Add README section for custom sources/layers This documentation exists only in the notes for a pull request, which inevitably will get out of date over time and is not as readily accessible. Connects pelias#1131 * Add Romania to flipped street number countries. * fix(package): update geolib to version 3.0.0 * Use new function names in geolib 3 * Update distance test expectation for precision changes in geolib 3 * fix(naming conventions): Columbian addresses should be flipped Fixes pelias#972 * Add comment explaining purpose of helper/placeTypes.js Its a list of _administrative_ placetypes. * fix(search_with_ids): Skip query when not address related The search_with_ids query (in some places more accurately referred to as the `address_search_with_ids` query), can really ONLY return addresses or related results such as streets, cities, etc. It will never return venues. However, if the user passed `layers=venue` as a query param, this query was still executing and if any results were returned, then a non-venue record would be returned as a result. * feat(translation) Translate name to request language when available * fix(package): update async to version 3.0.1 * feat(debug_population_popularity): display population & popularity in geojson when debug flag enabled * feat(improved_es_debugging): display more information about the elasticsearch request when in debug mode * fix(improved_es_debugging): remove unused statement * feat(max_text_length): limit max text length to 140 characters * feat(auto_discover_type_mapping): discover sources & layers by querying elasticsearch * feat(auto_discover_type_mapping): remove _replica_first preference * fix(predicate): Fix address related layers detection This fixes an error in the logic from pelias#1307. The `address_search_with_ids` query can only return results in the address or street layers. However, the predicate logic was allowing the query to execute if _any_ admin layer was specified, which could allow such a query to return street or address layers when not intended. * feat(controller): Remove search_with_ids controller I was attempting to use the new debug output from pelias#1313 only to realize it was not added to the `search_with_ids` controller, only the `search` controller. These two controllers are very similar, so this PR combines them into one that can be used in all cases. * feat(query): Use more consistent names The search endpoint uses multiple query types that are executed one after the other: if the first does not return acceptable results, the next is executed, and so on. The last query in this list used to be the only query the search endpoint had. Partially because it's been around for a long time, it is referred to in several different ways across different parts of the code. This change unifies all of them so that the query is called `search_addressit` everywhere. * chore(package): update pelias-query to version 9.13.0 * feat(query) use full match_phrase query This updates the fixture expectations for pelias/query#100 which uses the full `match_phrase` query format everywhere for consistency and to allow adding future parameters easily. * chore(package): update pelias-query to version 9.14.0 * feat(query): Remove deprecated optimize_bbox param This parameter of the `geo_distance` query was deprecated in Elasticsearch 2.2 Connects pelias/query#102 * chore(tests): Fix test description * fix(package): update elasticsearch to version 16.0.0 * chore(tests): Ensure test ES documents have proper format `source_id` should (currently) match `_id`, and should be present. * feat(geojsonify): Use `source_id` rather than `_id` field This change does not have any impact on the output of the API, but allows us to make changes to the contents of the `_id` field in order to correct the issue described in pelias/pelias#672 * fix(interpolation): Use street source_id if none found in interpolated result This fix ensures the interpolation middleware always returns results with a `source_id` field, fixing two issues: 1. It ensures the `source_id` property is always present in Pelias API responses 2. It allows `helper/geojsonify.js` to rely on `source_id` * feat(place): Query for old and new contents of `_id` field _This change completes step 2 of the `_id` field contents migration described in pelias/pelias#672 (comment) _ As part of the transition away from Elasticsearch types, we must change the contents of the `_id` field in our Elasticsearch documents. This change modifies the `/v1/place` endpoint so that it queries Elasticsearch for documents based on both the old and new contents of the `_id` field, allowing the same API code to work regardless of the version of Pelias used to import data. Connects pelias/pelias#672 * feat(auto_discover_type_mapping): update README * fix(auto_discover_type_mapping): bugfix to allow existing references to the type mapping to continue functioning after update * fix(auto_discover_type_mapping): update README * Netherlands use 'street number' The localized results of Pelias for the Netherlands are in the wrong order. The Dutch way is `street number`, not `number street` eg `damstraat 1` * chore(tests): Ensure fixture name is in test output This makes it faster to update the correct fixture * feat(search): Use minimum_should_match to filter hits This change utilizes the [minimum_should_match](https://www.elastic.co/guide/en/elasticsearch/reference/5.6/query-dsl-minimum-should-match.html) Elasticsearch query parameter to reduce the number of hits to search queries. Previously, only one token in an input had to match, regardless of the input size. This allows for queries to potentially match a huge number of documents, especially as the number of tokens grows. Now, most `ngrams` queries have thee parameter set to `1<-1 3<-25%` Breaking this down, it means that for the given number of tokens in a query, there is a certain number of optional tokens: | token count | optional tokens | | --- | --- | | 1 | 0 (obviously) | | 2 | 1 | | 3 | 1 | | 4 | 1 | | 5 | 1 | | 6+ | at least 75% of tokens must match | This should help ensure quality results are returned where possible, even if long inputs contain a few extraneous bits of information, while reducing the chance of extremely expensive queries. * feat(autocomplete): focus point distance filter This change uses an extra filter clause that takes into account a focus point (if used) and the input text length to filter out very far away street and adress records, improving performance. Replaces pelias#1215 * feat(peliasQueryAnalyzer): replace usage of peliasQueryPartialToken and peliasQueryFullToken analyzers with the new peliasQuery analyzer * feat(dedupe): Prefer OSM records over Geonames In the case of a venue, we should prefer OSM records over Geonames. * feat(geocodeJSON): remove geocoding.query.tokens array * fix(package): update check-types to version 9.0.0 * feat(elasticsearch): do not reuse config for elasticsearch client * Handle signals in a Docker friendly way Similar to pelias/placeholder#160, this small change allows `ctrl+C` to immediately and gracefully shut down a docker container started with `docker run pelias/api`. It's a nice user experience improvement for people running our containers on the command line. * chore(Node.js): Add Node.js 12 to CI Node.js 12 (and especially Node.js 12.9) is supposed to be significantly faster than previous versions, especially when it comes to parsing JSON, which is often a bottleneck for Pelias importers. https://nodejs.org/en/blog/release/v12.9.0/ https://v8.dev/blog/v8-release-76 Connects pelias/pelias#800 * feat(flip_housenumber_street): update config to cover more country codes where the label format is "{{street}} {{housenumber}}" * fix(package): update pelias-model to version 6.0.0 * chore(tests): Update tests for GID changes Connects pelias/pelias#672 * feat(deps): Move from joi to hapi/joi This just keeps the output of `npm install` free of warnings * fix(package): update pelias-model to version 7.0.0 * chore(tests): Update test expectations for pelias-model-7.0.0 * fix(package): update check-types to version 10.0.0 * feat(140_chars): restrict autocomplete inputs to 140 chars * chore(test): Remove all instances of `_type` from ES docs This will help ensure that in the future no code depends on the `_type` value from Elasticsearch. Connects pelias/pelias#719 * chore(travis): Pin to Node.js 12.10 Pending resolution of issues that may be related to the `geolib` module. See manuelbieh/geolib#208 * feat(parser): replace addressit with pelias native parser * feat(parser): improved postfix cursor position for text with no admin classification * feat(parser): pelias/parser improvements * feat(parser): bump pelias/parser version * feat(parser): bump pelias/parser version * feat(parser): bump pelias/parser version * feat(parser): bump pelias/parser version * feat(parser): updates to tokenizer sanitizer * typo * feat(parser): stricter tokenization of exact matching admin queries * feat(parser): switch to using multi_match for admin subqueries * feat(admin_subqueries): test cross_fields query * feat(admin_subqueries): test operator:and query * feat(admin_subqueries): set all boosts to 1 * feat(admin_subqueries): add locality_a and country_a to multi_match * feat(admin_subqueries): revert to operator:or * feat(admin_subqueries): remove cutoff_frequency * feat(admin_subqueries): move admin matching to MUST condition * feat(tokenizer): consider query as complete if the final char is a numeral * feat(autocomplete): test removing exact_matching subquery * feat(admin_subqueries): add cutoff_frequency * feat(pelias_parser): admin queries - remove subject from admin subquery * feat(admin_subqueries): remove cutoff_frequency * feat(autocomplete): use phrase index for complete tokens * feat(parser): remove parsed_text.name * feat(parser): so not consider address parses as safe to use with an ngrams index due to parses potentially containing partial suffixes * feat(parser): bump pelias/parser version * feat(tokenizer): consider query as complete if the $subject is not at the end of $clean.text * feat(parser): bump pelias/parser version * feat(autocomplete): experiment adding name.default to admin multi_match * feat(autocomplete): progess commit * feat(autocomplete): typo * feat(autocomplete): improved matching at the cusp * feat(autocomplete): improved performance and reduced noise for admin matching * feat(deps): bump parser dep version * feat(deps): bump parser dep version * feat(deps): bump parser dep version * test: disable parserConsumedAllTokens for admin parses * feat(deps): bump parser dep version * feat(query): add should subquery for cross_street matching * feat(logging): add summary logging for pelias parser * feat(deps): bump parser dep version * feat(pelias_parser): additional parser tests * feat(deps): bump parser dep version * feat(pelias_parser): fix tests * feat(search_addressit): generate cross_street subquery where available * feat(pelias_parser): limit input text to 140 characters * feat(pelias_parser): replace peliasQueryPartialToken analyzer with peliasQuery * feat(pelias_parser): disable "ngrams_last_token_only_multi" view when every "completed" token is numeric * Add context to pelias parser logs * Pin to pelias-parser-1.38.0 for now * refactor(pelias_parser): add code comments relating to "add_name_to_multimatch", clean up related code * refactor(pelias_parser): remove disused code/comments * feat(pelias_parser): completely remove "addressit" and references to it * refactor(pelias_parser): remove references to "original style queries" * chore(Travis): Use string to specify minor version Otherwise 12.10 is interpreted as a number and becomes version 12.1. * chore(query): Remove empty pop_subquery view `pelias-query` now includes basic views for standard Elasticsearch queries, including `match_all`. The API esentially created a wrapper around the `match_all` view, which was used in some cases and resulted in an extra query view that didn't do much. This change removes that boilerplate code and makes it a bit more clear what the underlying functionality of a query will be. * chore(tests): Remove unused fixture * chore(query): Remove unused function parameter At first glance, this code made it appear that the phrase query was being used across the focus point filter functionality on libpostal based search queries, but it turns out it's not! The `focus_only_function` view constructor takes no parameters, so this code wasn't having any effect. * updated with latest api * fix(local_naming_conventions): improve localNamingConventions middleware to support records with name/street aliases * fixed lint errors * added back placeholder polygon * fixed geometry results
Hi Everyone, I have a simple question. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#373
Related PR: pelias/query#93
Added optional 'boundary.wof' parameter to /search, /search/structured and /autocomplete endpoints. If present, it adds a multi_match filter to the elasticsearch query using parent.*_id as the fields.
I didn't add it to the /reverse endpoint because I wasn't sure if it made sense, but let me know if I should add it there.
For the validation/sanitation it only checks that the param string is all digits, I wasn't sure if there was a better way to validate the WOF id.
Any feedback would be great!