request params with a key but no value should be considered unset

[missinglink]

Regarding #1405 (comment) this PR changes the behaviour of the hasRequestParameter predicate.

Prior to this change it would return true when a query param key existed regardless of the value.

This PR modifies this behaviour so that the value must also be non-empty.

note: I believe the only place this has any consequence is for ?categories as it's the only 'empty param' we currently support.

I'm not sure why the tests cover Number and Object, but I just maintained what we currently had 🤷‍♂️ .

[CeallachKN]

As far as i understand this change:

This would make results less arbitrary in regard to "weird" venues / other places because of the placeholder bypass mentioned by @Joxit . München without &categories and München with &categories is an even better case then Paris. so basically a very good thing for small /easy requests.

But wouldn't this make the categorie attribute in the resultset redundant / only useful to differentiate the requested categories... unless the categorie attribute is always part of the result, if available, regardless of the request parameter? Wouldn't this be non-breaking since it is an addition?

we actually use &categories to include the categorie attribute in the resultset, since it is a pretty useful attribute to work with in the frontend and we make extensive use of the csv-importer for venues importing prefiltered POI-categories.
But this could be done by including the categorie in the addendum fields instead (even if this seems more brittle?)

@arnesetzer: We should at least be aware of this change and react accordingly in our setup

[missinglink]

I'm not sure I fully understand, but I'm interested to hear your feedback before merging.

For clarification, the intended change of behavior is:

• When not specifying any ?categories param, nothing changes.
• When specifying a ?categories=xxx param with a value, nothing changes.
• When specifying a ?categories param with an empty value:
  • The hasRequestCategories predicate returns false now instead of true
  • Therefore placeholderGeodisambiguationShouldExecute returns true.
  • Therefore the query logic changes, it seems to be agreed that the results are preferred?
  • No changes to the display of categories, an empty ?category will still trigger them to be rendered in the JSON.
  • No changes to filtering (an empty value means no category filters).

[missinglink]

I think this a bug fix, when the original predicate logic was written we didn't have the concept of an 'empty parameter' (ie. a query parameter with a key but no value).

[CeallachKN]

   This PR modifies this behaviour so that the value must also be non-empty.

Then I misunderstood the implication of this change based on this line. What i understood was that non-empty values for the categories-param wouldn't go through anymore and therefore changing the behaviour entirely. (My bad for not looking more into the actually change in code)

  * No changes to the display of categories, an empty `?category` will still trigger them to be rendered in the JSON.
  * No changes to filtering (an empty value means no category filters).

These two points were what i was worried about changing. But since this isn't the case, the bugfix is more then welcome (see my München example)

[Joxit]

I did some tests and it seems that hasRequestParameter is used only for lang and categories but:

• In the case of lang, it's triggered for middleware.changeLanguage after middleware.requestLanguage that will set clean.lang to an object defaulted to English when empty/not present. The use of hasRequestParameter is useless for this parameter and this PR will not change it.
• In the case of categories, it's triggered for controllers.search with fallbackQueryShouldExecute on the /search endpoint only. In this case it will be a "breaking change" since the behaviour with empty categories will change. The behaviour will also defer between /search and /autocomplete where the first one will forbid empty categories and not the other one, it's a bit inconsistent (but not the first time). Maybe this must be added in the other endpoints ?

At work, I'm moving my clients from /search to /autocomplete and I don't think they are using categories anyway. But I still like the fact that it's possible to have all the categories. Maybe with categories=all ?
Or another parameter not related to categories, I had a feedback from a client who wanted a lighter response from the API, a response only with the label for example 🤔.

[missinglink]

I've put it up on api.dev.geocode.earth so we can take it for a spin:
https://pelias.github.io/compare/#/v1/search?categories=&text=paris

The use of hasRequestParameter is useless for this parameter and this PR will not change it.

I'm not sure I fully understand the change to lang yet, although it's concerning, is this something which we should tackle in this PR or can we split it out to a separate issue since you say 'this PR will not change it'?

In this case it will be a "breaking change" since the behaviour with empty categories will change.

While it's true that the functionality will change, the categories feature is still undocumented (something I'm trying to get fixed, by working through the remaining issues!).

In particular the behaviour of categories with an empty param was never well defined, I understand both the commenters in this thread rely on the existing behaviour but isn't it a welcome improvement?

The behaviour will also defer between /search and /autocomplete where the first one will forbid empty categories and not the other one.

I played with the compare tool and I don't think this is the case?

Maybe with categories=all?

Honestly I didn't think supporting a valueless param would have this many footguns, but I still prefer it to having a 'special string'.

[missinglink]

One thing to clarify is that there are essentially two discrete codepaths operating on the categories request param.

1. The 'sanitizer' code which takes user input and validates it, this is responsible for setting clean.categories to and Array of categories requested, or undefined. This Array is used later to enforce the MUST filtering query conditions, if the Array is empty or undefined, it doesn't do anything.

2. The predicates logic which checks if the categories param was present or not. This adjusts which queries should run. The logic here is that categories only belong to the venue layer and therefore we don't need to run Placeholder.

[missinglink]

Regarding the lang param, I'm not able to reproduce, could you please explain @Joxit

I set my browser to French and then tried both lang and lang= and neither caused the request to change to English:

note: I had to keep my console open and Disable Cache checked in the Network Tab to avoid the browser caching between user agent changes.

[Joxit]

For the lang parameter, what I meant is the middleware.changeLanguage is setting the value of clean.lang before the use of hasRequestParameter on the lang parameter.
So clean.lang is always an object regardless the query parameter lang => never empty or a number => predicates.hasRequestParameter('lang') is useless as long as middleware.changeLanguage is working properly.

[Joxit]

The behaviour will also defer between /search and /autocomplete where the first one will forbid empty categories and not the other one.

I played with the compare tool and I don't think this is the case?

Oh yes you're right, with the change it will no longer bypass the use of placeholder

Maybe with categories=all?

Honestly I didn't think supporting a valueless param would have this many footguns, but I still prefer it to having a 'special string'.

What about a parameter to control categories, meta, addendum and hierarchy display ?

[Joxit]

LGTM

[missinglink]

@CeallachKN / @arnesetzer do you have any concerns with merging this?