{"action":"created","issue":{"url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/issues/419","repository_url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2","labels_url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/issues/419/labels{/name}","comments_url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/issues/419/comments","events_url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/issues/419/events","html_url":"https://github.com/UrbanCCD-UChicago/plenario2/pull/419","id":349637086,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA3NzA2NTYw","number":419,"title":"API Purge","user":{"login":"vforgione","id":4360430,"node_id":"MDQ6VXNlcjQzNjA0MzA=","avatar_url":"https://avatars1.githubusercontent.com/u/4360430?v=4","gravatar_id":"","url":"https://api.github.com/users/vforgione","html_url":"https://github.com/vforgione","followers_url":"https://api.github.com/users/vforgione/followers","following_url":"https://api.github.com/users/vforgione/following{/other_user}","gists_url":"https://api.github.com/users/vforgione/gists{/gist_id}","starred_url":"https://api.github.com/users/vforgione/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vforgione/subscriptions","organizations_url":"https://api.github.com/users/vforgione/orgs","repos_url":"https://api.github.com/users/vforgione/repos","events_url":"https://api.github.com/users/vforgione/events{/privacy}","received_events_url":"https://api.github.com/users/vforgione/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-08-10T19:48:54Z","updated_at":"2018-08-11T01:29:32Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/pulls/419","html_url":"https://github.com/UrbanCCD-UChicago/plenario2/pull/419","diff_url":"https://github.com/UrbanCCD-UChicago/plenario2/pull/419.diff","patch_url":"https://github.com/UrbanCCD-UChicago/plenario2/pull/419.patch"},"body":"So, I kinda sorta totally gutted and rewrote the API. I have a good\r\nreason for doing this, I swear.\r\n\r\nWe had a handful of seemingly unrelated bugs in the API, and being the\r\ngood Orkin Man that I am I started fixing them. At first I was doing\r\nthem one at a time, but then that wasn't really fixing anything -- it\r\nwas more like handling edge cases. So then I started looking for\r\npatterns in the bugs, and how far down the stack they could go.\r\n\r\nThe way I started this was with the tests. They sucked. I'm sorry for\r\nbeing so blunt, but they really weren't that great. I probably should\r\nhave scrutinized them earlier when the PRs came up, but I think that was\r\nback in late April/early May when I was getting married and buying a\r\nhouse. Not an excuse, just some context. Or at least that's when the\r\nbulk of the API was being developed.\r\n\r\nAnywho, I rewrote the tests and, lo and behold, it turns out that we\r\nwere not delivering on what we promised. We also had a ton of bugs, and\r\nsome really sloppy hacks to force things together.\r\n\r\nAt the same time, I asked the CDO folks to start hammering the Shim API\r\nand they found tons of broken stuff: the datasets endpoint didn't return\r\nthe same shape as the existing app, the fields endpoint was missing,\r\nthey didn't even try the detail endpoint but my inspection found it to\r\nalso be mishapened.\r\n\r\nGiven all of that, it seemed like giving this a fresh start was the best\r\napproach. The controllers are a lot slimmer, the views delegate where\r\nappropriate, the plugs are now purely functions, the utils are cleanly\r\nlaid out, and best of all the errors are all being caught and handled\r\nproperly.\r\n\r\nI've also done away with all references to 422 status codes. I'm sorry,\r\nthat's just crazy talk. 400 is the right way to do it -- it's a bad\r\nrequest.\r\n\r\nFixes #401\r\nFixes #403\r\nFixes #404\r\nFixes #405\r\nFixes #406\r\nFixes #407\r\nFixes #408\r\nFixes #415\r\nFixes #416\r\nFixes #417\r\nFixes #418"},"comment":{"url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/issues/comments/412241564","html_url":"https://github.com/UrbanCCD-UChicago/plenario2/pull/419#issuecomment-412241564","issue_url":"https://api.github.com/repos/UrbanCCD-UChicago/plenario2/issues/419","id":412241564,"node_id":"MDEyOklzc3VlQ29tbWVudDQxMjI0MTU2NA==","user":{"login":"vforgione","id":4360430,"node_id":"MDQ6VXNlcjQzNjA0MzA=","avatar_url":"https://avatars1.githubusercontent.com/u/4360430?v=4","gravatar_id":"","url":"https://api.github.com/users/vforgione","html_url":"https://github.com/vforgione","followers_url":"https://api.github.com/users/vforgione/followers","following_url":"https://api.github.com/users/vforgione/following{/other_user}","gists_url":"https://api.github.com/users/vforgione/gists{/gist_id}","starred_url":"https://api.github.com/users/vforgione/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vforgione/subscriptions","organizations_url":"https://api.github.com/users/vforgione/orgs","repos_url":"https://api.github.com/users/vforgione/repos","events_url":"https://api.github.com/users/vforgione/events{/privacy}","received_events_url":"https://api.github.com/users/vforgione/received_events","type":"User","site_admin":false},"created_at":"2018-08-11T01:29:32Z","updated_at":"2018-08-11T01:29:32Z","author_association":"MEMBER","body":"Page should be a _positive, non zero_ integer; otherwise, it's a bad request. This goes the same for page size.\r\n\r\nI'm not entirely convinced that `0` is a valid value, especially if we're strictly evaluating and enforcing these values.\r\n\r\n403 seems inappropriate -- I would expect to see that for a resource I cannot access, not a query that I tried to ham fist.\r\n\r\n422 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.\r\n\r\nI'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?\r\n\r\nAdditionally, I just hate it. I don't know why. I have this really awful, negative emotional reaction to it :)"}}
{"actor":{"display_login":"vforgione"}}