search

MartinsOnuoha / countriesNowAPI

CountriesNow is an Open source API for retrieving geo-information for countries, including their states, cities, population, etc. 🌎
http://countriesnow.space
MIT License
179 stars 55 forks source link

Deprecate POST requests and route them to GET requests #93

Closed TimAagaard closed 2 years ago

TimAagaard commented 2 years ago

None of the POST requests change anything so they should semantically be GET requests.

Redirecting these POST requests to parameterized GET requests will allow caching of these routes in the future while maintaining compatibility for anything built using the POST requests.

TimAagaard commented 2 years ago

@MartinsOnuoha what's the status of getting my pending PR merged?

I have written a Middleware that redirects all POST requests to originalUrl + '/q' and appends the body as a query string. Then updated all POST functions to use req.query instead of req.body and replaced the POST routes with GET routes with "/q" at the end of the route.

Full backwards compatibility and all unit tests are passing...but I'm branched off of the pending PR because it was more convenient to work off of the non-crashable API.

It will be a simple single commit PR but you'll have to merge the pending PR commit first if you don't want conflicts. For that reason I'm holding off on this PR...but it will be up on my fork in a few hours.

TimAagaard commented 2 years ago

Caching will also be branched off of this PR and will be a simple single small commit. Probably do that tomorrow. So you could potentially just wait until that and merge that PR if you want to get everything all at once...but I'm a bigger fan of deploying in smaller pieces in case something breaks.

MartinsOnuoha commented 2 years ago

Hi Tim, Great job with the last PR, seems like wrapping most of the controller methods in try/catch saved us a lot of crashing.

That PR is merged now and in master, so I think you can stash your current changes and pull the updates from master, branch out of master and unstash your changes into the new branch perhaps if that works for you (although however you find it best to create a new PR works too).

About the semantics around the POST requests, I was fully aware of the fact this unconventional approach of using a post request for non-changing request, the idea (above all) was to make the usage as simple as possible, and like you mentioned, it's easier to send an object payload in a POST request than trying to remember the format of a GET URL with long query params.

This redirect solution is pretty optimal so I agree with it. I think it would be best to raise the PR so we can have a look.

TimAagaard commented 2 years ago

@MartinsOnuoha PR up - it is branched off of master, so it should fast-forward merge no problem