search

pelias / api

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

Fix category sanitizer for new call format #1588

Closed orangejulius closed 2 years ago

orangejulius commented 2 years ago

It turns out the changes to the parameters for sanitizer functions in #1583 wasn't quite backwards compatible.

What broke?

The categories parameter sanitizer supports taking a 3rd parameter that defines how to determine valid categories. If there's only two parameters, it uses a default function that always returns true.

We intentionally haven't documented the categories parameter because it's not really done, and the categories mapping is not finalized. As a result almost no one uses it.

However, if they did use it, and passed in a category, they could cause the API to generate a 500 error.

This is no problem to fix, just expect the 3rd parameter to be the req object, and the 4th parameter, if we ever even use it, is now that category validation function.

Aside: commentary on the categories sanitizer code and parameter

Realistically, the design here probably isn't useful, I'm not sure how we'd want to pass in a validation function in practice. I thought a lot about removing this code.

There's also some nearby code from https://github.com/pelias/api/pull/1401 that is effectively a second categories sanitizer just for the place endpoint. If we ever take action on https://github.com/pelias/api/issues/1405 we'd probably end up rewriting all of this 🤷

Thoughts on testing

Finally, I checked through all the other sanitizers to see if any might be affected similarly to this one. I don't think so, but I did clean up one unused parameter to another little used sanitizer.

I'm trying to think of an easy way for us to have caught this error. It showed up in the acceptance tests, but only for the little-used nearby endpoint tests, which we basically ignore.

It didn't cause any unit tests to fail, and the ciao tests (if we ran them regularly), wouldn't have currently caught it.

What should we add to our testing to catch stuff like this?

missinglink commented 2 years ago

Oh nice catch, I grepped and grepped again looking for code that touched that arguments change.

At the time I remember being worried mostly about usages of Function.bind() since they are harder to detect.

I would love to bring back the idea of spinning up the API server and running tests against it (as per ciao).

But doing that is a little annoying to configure (especially if data and all microservices are required), plus I haven't found a simple replacement for ciao yet.

Yeah dunno, good catch