ropensci / fishbaseapi

Fishbase API
https://fishbaseapi.readme.io/
MIT License
42 stars 12 forks source link

problem with trailing slashes #113

Closed sckott closed 5 years ago

sckott commented 7 years ago

@EvilScott regarding #111 and #112 with respect to trailing slashes:

people would often report that when hitting the base url https://fishbase.ropensci.org they get a Forbidden response - and I think this mostly just happened on the base url

my solution was https://github.com/ropensci/fishbaseapi/blob/master/api.rb#L74-L80 - but as you saw this redirects all routes without query params to same route but with trailing slash

The https://github.com/ropensci/fishbaseapi/blob/master/api.rb#L74-L80 change had a nice side effect in that now two routes like /foo and /foo/ from Redis caching perspective are the same thing - BUT means that a /foo leads to a redirect 301

Any thoughts on a better way to do this?

MollsReis commented 7 years ago

So the Sinatra routes as they stand now should have an optional trailing slash (that's the question mark after the route). The root path didn't have that optional trailing slash, so perhaps that's why it would come up forbidden.

Your solution is fine; just document that missing trailing slashes will be redirected as some REST clients need to be instructed to follow redirects.

If you aren't happy with the current state then I'd say the other solution would be adding the trailing slash (it not already present) when generating a redis key to read/write for caching. Then you support optional trailing slashes in your routes and can remove the redirect, but you still have the same cache keys regardless. Just an idea to let percolate. :)

On Sun, Mar 12, 2017 at 4:22 PM, Scott Chamberlain <notifications@github.com

wrote:

@EvilScott https://github.com/EvilScott regarding #111 https://github.com/ropensci/fishbaseapi/issues/111 and #112 https://github.com/ropensci/fishbaseapi/pull/112 with respect to trailing slashes:

people would often report that when hitting the base url https://fishbase.ropensci.org they get a Forbidden response - and I think this mostly just happened on the base url

my solution was https://github.com/ropensci/fishbaseapi/blob/master/api. rb#L74-L80 - but as you saw this redirects all routes without query params to same route but with trailing slash

The https://github.com/ropensci/fishbaseapi/blob/master/api.rb#L74-L80 change had a nice side effect in that now two routes like /foo and /foo/ from Redis caching perspective are the same thing - BUT means that a /foo leads to a redirect 301

Any thoughts on a better way to do this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ropensci/fishbaseapi/issues/113, or mute the thread https://github.com/notifications/unsubscribe-auth/AAd3C6VDYnEvt4P5YSRsBRmbt_VakRbGks5rlH4lgaJpZM4Mat9O .

sckott commented 7 years ago

thanks, I'll play around, but i thought even with /? people still reported that problem

I like idea of always adding a trailing slash to redis keys - and leaving /? for routes

sckott commented 5 years ago

sorted now