Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent empty result behaviour #319

Closed
markembling opened this issue Apr 17, 2019 · 6 comments
Closed

Inconsistent empty result behaviour #319

markembling opened this issue Apr 17, 2019 · 6 comments

Comments

@markembling
Copy link

Not really a bug, but thought it was worth mentioning as it violates the rule of least surprise, at least for me: the behaviour of what is given by empty result sets varies between the APIs:

For the postcodes query endpoint, asking for something which doesn't exist results in the results being null:

> api.postcodes.io/postcodes?q=NOPE

{
    "status": 200,
    "result": null
}

Whereas doing the same thing for the places API gives us an empty list:

> api.postcodes.io/places?q=NOPE

{
    "status": 200,
    "result": []
}

Neither actually feels wrong to me, but I do feel like they ought to be consistent.

@markembling
Copy link
Author

The null behaviour also applies in at least one other place: outcode reverse geocaching. I'm guessing this is the more standard of the two, by the APIs own standards.

> api.postcodes.io/outcodes?lon=0&lat=0

{
    "status": 200,
    "result": null
}

@cblanc
Copy link
Member

cblanc commented Apr 18, 2019

Ah yes, thanks for reminding me. Yes /places api is inconsistent. However this is the correct way to go

The least surprising way to represent an empty set is an empty array. However "result": null is a mistake I'm unable to correct

There are some cases where you want to stop and react to an empty set. But there are also some where you're happy to operate on it as an array, like mapping it out to a list of html elements, further http requests, or some other operation. Effectively mapping to a list of noops which the program can tolerate and makes the resultant code looks more terse and just nicer.

Unfortunately null stops all that dead in its tracks. In most languages, null (or whatever its cousin in $language) is deadweight. In browsers, it does not define, map, reduce, forEach, length, etc, etc. It's easy to forget where it may be floating around your system and will upturn your program when you do. I only really resort to it if there is no other good way to represent a "nothing" instance of some model.

Seems to be better to have 1 type flowing through your system than 2. I've found the benefits are more pronounced either when writing in a functional style or when using a dynamic language - even so, I understand type checked languages like Java will still clobber you if you fail to keep a solid mental account of null in your head.

/outcodes and then years later /places were implemented some time after I acquired this view. When /outcodes was implemented I just held my nose because, IIRC, it too similar as a concept to postcodes. When it came to /places, I was more interested in being correct than consistent. There's probably a debate to be had where continuing to do the wrong thing is better than being inconsistent

There's barely an hour that goes by where I don't see some work some I did a while ago that feels wrong. "result": null is one of many painful instances

Unfortunately, last I checked, /postcodes is now getting hit 10-20 millions times a day and the null return implemented sometime in 2014 has now effectively been calcified. I don't think there's a straightforward way to correct this without creating disruption

@markembling
Copy link
Author

markembling commented Apr 18, 2019

Hmm, I see what you're saying. Thanks for the detailed response. I also agree that of the two, the empty array is preferable.

Do you think people are now relying on the fact that empties come back as null? The fact that the documentation doesn't specify it at all to me means perhaps it could still yet be corrected. I'm not sure how much disruption that might cause.

For me in my library, the inconsistency is weird - for the methods mapping to /postcodes, I've actually got tests which check for empty results coming back as null and I implemented the same yesterday for /places - needless to say, the latter fail which is why I picked up on this in the first place.

If you were looking to fix it in the API without breaking existing clients' behaviour, I see a couple of options: either you could version the API at this point and fix things like this only in the new version. That could be a lot of work just to fix something which feels wrong though. Another option might be to introduce some kind of additional query parameter or even HTTP header which opts into new behaviour.

If you feel that changing the API is too dangerous and/or not worth the effort, I can certainly respect that and understand your reasoning, but it leaves me with choices, none of which are particularly good. I'd appreciate your thoughts...

  • I stop testing for any consistent handling of empty/null results entirely, and leave it undefined in the library as well. This puts the onus on the consuming developers to deal with it as they see fit, discovering/handling the difference themselves if they run into it. It does however mean that any future API changes (i.e. if you later decide to change the behaviour) are also not my problem - my library will just provide back whatever it's given.
  • Specifically testing/documenting the inconsistency and treating that as the expected/correct behaviour: effectively saying "yes, /postcodes and /outcodes methods will give you null, /places methods will give you an empty list. You must handle this."
  • Having my library normalise all the empty results into the appropriate empty list type (i.e. if it gets a null where it expected a list, return an artificially-created empty list instead so consumers will always see consistency).

The last option feels like its best for consuming developers as, like you say, people can choose to handle the empties however they like without worrying about a null causing things to crash down around them. But it does mean the library behaviour will deviate from the API which it wraps, and I'm not sure if that's something to be recommended.

@cblanc
Copy link
Member

cblanc commented Apr 18, 2019

Do you think people are now relying on the fact that empties come back as null?

I have no choice but to assume this to be the case

The fundamental principle is, no developer or user can wake up to a broken integration. This informs the design and wins over every other concern

you could version the API at this point and fix things like this only in the new version. That could be a lot of work just to fix something which feels wrong though

Agreed 😀

With regards to your options, I would also agree with your preference (option 3). While it's good to explore the options, I would never consider 1 or give much consideration to 2.

You also have a 4th choice - or more like "3b". If you really need things to be consistent, then you can provide an abstraction on the old convention:

3a. Backwards Incompatible - return []
3b. Backwards Compatible - return null

But returning null or [] really depends on what you think is important.

I do apologise that your client needs to be stretched for past mistakes, however this provides a cleaner abstraction for developers to build on.

But it does mean the library behaviour will deviate from the API which it wraps, and I'm not sure if that's something to be recommended.

I wholehearted recommend creating an abstraction that deviates from the API if it solves a problem well. Why bother your users with poor design mistakes made in 2014?

API clients aren't just plumbing to get data. I think they should also provide the right abstraction to get some type of job done.

The last option feels like its best for consuming developers

Yes always. This is the prime concern and essentially why I guess this issue / discussion exists. Avoiding backward breaking changes trumps almost everything for this service. Apps must not break.

@markembling
Copy link
Author

Thanks for your thoughts and for reinforcing that my preferred approach is probably the best one in this case. And yes, I totally understand the focus on backwards-compatibility and not breaking existing integrations and on reflection, think you're definitely right there.

I'm in a privileged position in that I'm able to make backwards-incompatible changes on my side, as I'm going to be taking it from 0.x to 1.x and this is the point I'm hoping to get it right, otherwise I'll be locking myself in as well for the foreseeable future. I've given myself permission to make all necessary changes regardless of compatibility at this point to get it as right as possible, learning from the issues which exist in 0.x. But once I'm at 1.x, I'm expecting to also keep backwards-compatible as long as reasonably possible.

I've decided I'll go with the option of abstracting away of the differences and most likely will look to return the empty list in all cases as it seems to me we both feel best about that. I do like your idea of being able to opt in to API-consistent behaviour if it's wanted though (3a/3b), so may look at that as well once the basics are in place.

I do apologise that your client needs to be stretched for past mistakes, however this provides a cleaner abstraction for developers to build on.

Please don't apologise. It's a minor thing really, and in no way takes away any value from the many hours of work and resources which have obviously gone into making this available. It's a fantastic resource and I can only thank you. 🙂 I also really value understanding your reasoning as well, straight from the horse's mouth, as it were.

@cblanc
Copy link
Member

cblanc commented Apr 18, 2019

You're welcome. Thanks for the feedback. Please do post more issues - am happy to follow up - there will inevitably be bugs in the project and errors / omissions in the documentation

And likewise, thanks for putting your time and skills in the .NET client 👍

@cblanc cblanc closed this as completed Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants