pelias / api

HTTP API for Pelias Geocoder
http://pelias.io
MIT License
221 stars 162 forks source link

default-parameters: extend functionality to be more general purpose #1600

Closed missinglink closed 2 years ago

missinglink commented 2 years ago

in https://github.com/pelias/api/pull/928 we introduced the ability to apply default values to the focus.point.lat and focus.point.lon query string params if/when they were unset.

this PR refactors that code in such a way that it allows us to extend the list of properties which can be overridden by defaults.

note: there is a concept of 'parameter groups' which I've maintained, the idea being that it makes sense for some parameters to be treated as a cohesive group (such as lat/lon pairs) and if one or the other is missing then we don't make modifications.

Example:

To set the default size param (ie. when it is unset in the query-string), edit pelias.json as such: note: validation logic such as MIN_SIZE and MAX_SIZE are still enforced later in the request lifecycle.

{
  "api": {
    "defaultParameters": {
      "size": "40"
     }
   }
}
missinglink commented 2 years ago

should we emit a warning when a default parameter is used?

orangejulius commented 2 years ago

This looks good. Since the PR as it stands is a pure refactoring no-op, how about the following:

Let's set up another PR on top of this that adds a couple parameters to the list that can have a default. Maybe sources, layers, boundary.country? Then we can test that PR and make sure it all works as we expect. There's not really much of a reason to merge anything until it impacts actual functionality.

I think we can skip the warning for default parameters. Whoever is configuring Pelias will be aware of what's going on.

missinglink commented 2 years ago

Yeah, so before we merge this we should add all params for each endpoint to the 'group lists' and add the functionality to any endpoints which don't currently have it (such as reverse).

Since it still requires adding new properties to the pelias.json config, this would still be a no-op unless a developer opted in to it.