API Purge

[vforgione]

So, I kinda sorta totally gutted and rewrote the API. I have a good reason for doing this, I swear.

We had a handful of seemingly unrelated bugs in the API, and being the good Orkin Man that I am I started fixing them. At first I was doing them one at a time, but then that wasn't really fixing anything -- it was more like handling edge cases. So then I started looking for patterns in the bugs, and how far down the stack they could go.

The way I started this was with the tests. They sucked. I'm sorry for being so blunt, but they really weren't that great. I probably should have scrutinized them earlier when the PRs came up, but I think that was back in late April/early May when I was getting married and buying a house. Not an excuse, just some context. Or at least that's when the bulk of the API was being developed.

Anywho, I rewrote the tests and, lo and behold, it turns out that we were not delivering on what we promised. We also had a ton of bugs, and some really sloppy hacks to force things together.

At the same time, I asked the CDO folks to start hammering the Shim API and they found tons of broken stuff: the datasets endpoint didn't return the same shape as the existing app, the fields endpoint was missing, they didn't even try the detail endpoint but my inspection found it to also be mishapened.

Given all of that, it seemed like giving this a fresh start was the best approach. The controllers are a lot slimmer, the views delegate where appropriate, the plugs are now purely functions, the utils are cleanly laid out, and best of all the errors are all being caught and handled properly.

I've also done away with all references to 422 status codes. I'm sorry, that's just crazy talk. 400 is the right way to do it -- it's a bad request.

Fixes #401 Fixes #403 Fixes #404 Fixes #405 Fixes #406 Fixes #407 Fixes #408 Fixes #415 Fixes #416 Fixes #417 Fixes #418

[vforgione]

Also fixes #411 and fixes #412

[brlodi]

I'm willing to have the fight over status codes (though I agree in most cases 422 is wrong).

I believe they should be applied as follows:

Page

• page=wrong – 400 Bad Request
• page=0 – 200 OK I would generally expect an API to ignore falsy values for non-boolean parameters or take them as an explicit direction to "use defaults"; I would not make this an HTTP error at all, and simply add a note to the documentation
• page=-3 – 400 Bad Request (though it'd be great to offer reverse indexing like Python, so e.g. -1 selects the last page, in which case it would be valid)
• page=999999999999 – Probably should just return the very last (necessary incomplete) page, but if we must error then it's absolutely a 422; RFC 4918 states that 422 Unprocessable Entity is applicable when the request entity is syntactically well-formed, but semantically erroneous.

Page Size

• page_size=wrong – 400 Bad Request
• page_size=0 – 200 OK It seems straightforward enough to return the default size of page; see arguments for page=0 above
• page_size=-232 – 400 Bad Request
• page_size=1276971623108 – 403 Forbidden because we absolutely do understand the request, but we just refuse to do it because it would be a huge response

As a summary statement, I'd like to reiterate that HTTP Status Codes are intended to describe the status of the HTTP transaction itself. It is perfectly reasonable to return some amount of error status under a 200 Accepted code if the error is unrelated to the request itself, e.g. "give me the first 100 records of dataset foobar" could very well merit a response of 200 OK {"error": "Foo Bar has not had its first ingest yet, so no records are available"} (I realize that doesn't match our return spec, but the point is the request is 100% valid but there is a business behavior issue with responding)

[vforgione]

Page should be a positive, non zero integer; otherwise, it's a bad request. This goes the same for page size.

I'm not entirely convinced that 0 is a valid value, especially if we're strictly evaluating and enforcing these values.

403 seems inappropriate -- I would expect to see that for a resource I cannot access, not a query that I tried to ham fist.

422 means that we've fully interpreted the value and determined that we don't like it. The plugs only check for type and lower bound range validity and pass the request along. If a user passes page=999999999 then it's up to the database to determine if that combination of limit/offset is an error.

I've also never seen 422 used -- I know it's a legit status per the RFCs, but I have yet to see it in the wild. I think most clients are smart enough to determine that a 4xx status is an error, but are any of them actually going to handle the robust error message and adjust accordingly?

Additionally, I just hate it. I don't know why. I have this really awful, negative emotional reaction to it :)

[vforgione]

While working on the docs, I found some funky stuff I don't really like in the AoT endpoint. Hold up on reviewing that for now.

[vforgione]

Ok, fixed the AoT funk.

[HeyZoos]

I'd also like to :+1: Ben's comment about reverse indexing. It would be a nice touch.