search

islamic-network / api.aladhan.com

The AlAdhan API
GNU General Public License v3.0
120 stars 31 forks source link

[refactor] Timings endpoints - timestamp to date #16

Open meezaan opened 5 years ago

meezaan commented 5 years ago

Redirect all timings endpoints from timestamp to date so they can be cached.

This would be mean that the API would only actually calculate timings for one location once per day.

papasmile commented 4 years ago

Salam, @meezaan are you talking about e.g:

https://github.com/islamic-network/api.aladhan.com/blob/fcaa0630e0c110978ecbae2095303c39d5cfdcd3/routes/timings/v1/prayerTimes.php#L84

I don't see any corresponding /{date} routes... sorry, can you explain a bit?

meezaan commented 4 years ago

Alaykum Salaam @papasmile.

This issue has to do with some lazy coding I did back in 2013 - so basically if you do not specify a timestamp in any of the timings endpoints (/timings, /timingsByCity, /timingsByAddress) it gets the current date and returns timings for that. Also, if you specify a UNIX timestamp, it converts that to a date and returns the timings.

Whilst convenient, both these approaches have the same problem - we cannot cache prayer times for a particular request. Now really, there are 2 ways to deal with this:

  1. When a request comes in the following formats:
/timings?method=x...
/timingsByCity?method=x...
/timingsByAddress?method=x...
/timingsByAddress/1567912983?method=x...
/timingsByCity/1567912983?method=x...
/timings/1567912983?method=x...

we compute the date, and do a 301 so we redirect to something like /timings/2020-01-11?method=x.... Then we can start caching results for a particular day and never run the computation again for those apps that query the API with identical data hundreds of times a day (like the aladhan.com web app for instance).

  1. The second option is to leave the URLs as is, but just convert to the date internally and cache the result. This will not allow caching at an edge server like in 1 (if we ever felt like that was necessary), but we could stick the numbers locally in memcached or redis.

My initial view was to do 1, which is the reason for the title of the issue.

What do you think?

papasmile commented 4 years ago

OK, gotcha, so lazy coding resulting a non-idempotent results (thus non-cacheable).

First off, I'm thinking that route probably should not accept a timestamp since results are always based on a 24-hour period... but then I dunno maybe someday we'd need a partial-day option.

Redirect you propose seems ok, however, we're still paying for a calculation (though smaller) and it's still not cacheable. What is your policy (or thoughts) on deprecation and eventual removal of routes/features?

meezaan commented 4 years ago

@papasmile For the redirect, yes, we pay a very small price, but the bigger calculation we can cache. I don't expect to do a partial calculation, the response payload is already pretty small, so that would almost be nitpicking.

I am quite happy to deprecate stuff, provided we roll out a new version that has been running for 90 days. I don't actually think that is needed here - a 301 is pretty lightweight on the web server and we would break potentially hundreds of applications since the API is open for use by anyone.

papasmile commented 4 years ago

Salam, @meezaan, I was referring to your statement "we compute the date, and do a 301". That is, we still need to dynamically convert whatever timestamp they send in to a date; am I understanding correctly? So would we want to provide that service (converting timestamp to date) forever?