Filter by object type

[macolu]

As discussed in #666, here comes a first attempt of adding a new object_type filter.

I also added debug flag on reverse API because I needed it for testing.

Thanks

[lonvia]

The code looks good to me. I see you allows parameter repetitions, which is good. Might be worth mentioning explicitly that this is an 'or' in the docs.

The only question that remains is the name of the parameter. 'object_type' is already not the best choice for the index name and worse for a user-facing parameter. Do other geocoders have a similar parameter? @karussell any suggestions?

The CI failure is not your fault. Github actions has broken something again in their image. I'll check.

[lonvia]

Github actions fixed. Please rebase on master.

[macolu]

Rebase done. Doc updated.

I agree that we can probably find a better name :-)

[lonvia]

I did a bit of research. Pelias calls the parameter layer and uses 'data type' in the description. Mapbox uses types. Most of the other geocoders don't have a comparable filter.

Looks like we are not so far off with object_type. I'll leave this PR open for a couple of days, in case somebody has a better idea. But by and large, I'm inclined to just merge as is.

[lonvia]

I had some more off-line discussions and the consensus was that Pelias' layer is most appropriate.

@macolu Can I ask you to change the parameter name to layer. You can also change the name of the index in the database, if that makes things easier. After that, I'm happy to merge.

[macolu]

Done.

I renamed everything:

• Attribute in Elastic mapping
• New query string param
• Attribute in returned results. Since attribute had previously been renamed since last 0.3.5 release (type -> object_type), I assume there is no additional BC break.

Thanks

[lonvia]

Attribute in returned results. Since attribute had previously been renamed since last 0.3.5 release (type -> object_type), I assume there is no additional BC break.

Oh dear. The renaming was not intentional. That's an accident of the recent code refactoring. Good thing that you spotted it. We should stick with 'type' for the output. There may already be clients that rely on that name.

[macolu]

In that case, maybe query string param should be named type as well?

It feels inconsistent to have a param named layer that filters on an attribute named type in output results... Two different names for the same notion.

[macolu]

Here comes a few changes:

• reverted attribute name to type in response, to fix accidental BC break. Achieved by renaming field in Elastic mapping. type is quite vague, so maybe we should keep object_type there and have some mapping in Json Formatter... Your call :)
• renamed new input query param to type, to be consistent with response.

I kept objectType in most of the code, again because type is quite vague, so it felt wrong to rename everything.

[lonvia]

Sorry for staling here. I needed to ponder a bit on this.

So type in the response comes from the geocodejson spec, which means we are stuck with it for better or worse. It explains the 'object_type' column name though. Obviously I also thought that 'type' is a bit vague. However, in general I agree with your changes including renaming the ES field.

That leaves the API parameter. I'd still like to go with layer here, for the simple reason that I had in mind that it is not exactly the same as type but can be a superset. In particular, I was thinking right now of a special layer=address that solves https://github.com/osm-search/Nominatim/issues/1469. Photon has exactly the same issue here. layer=house doesn't guarantee that you get a housenumber. You might also get a named postbox or fountain or other curious POI objects.

[macolu]

Ok for me. So here comes a new version:

• type in response and ES documents
• layer in query param and most of the code

[lonvia]

Merged. Thanks you for your patience.

If all goes well the change should already be included in the next experimental dumps. That will be a good occasion to officially announce them.