jikan-me / jikan-rest

The REST API for Jikan
https://docs.api.jikan.moe/
MIT License
427 stars 270 forks source link

ETag header missing from response #322

Open Chris-Peterson444 opened 1 year ago

Chris-Peterson444 commented 1 year ago

It looks like the ETag information isn't getting returned in the response headers. I've checked a few different endpoints (anime, characters, and manga) but it hasn't shown up in any of them.

Minimum working example:

import requests
resp = requests.get('http://api.jikan.moe/v4/anime/1')
for k in resp.headers.keys():
    print(k)

Which gives:

'Server'
'Date'
'Content-Type'
'Transfer-Encoding'
'Connection'
'Access-Control-Allow-Origin'
'Access-Control-Allow-Methods'
'Cache-Control'
'X-Request-Fingerprint'
'Expires'
'Last-Modified'
'Content-Encoding'
'Vary'
'X-Cache-Status'

I would be happy to put some time in on starting a fix, but I'm a little new to the project (and PHP), so I could use some pointers on where to start looking to solve this.

pushrbx commented 1 year ago

I did a bit of digging:

Also #318 introduces changes which would make the CacheResolver class cache driver agnostic.

In the light of these I would say that caching is not there yet. Also it would be nice to know @irfan-dahir which caching mechanism is which. I can see CacheResolver and Microcaching. Microcaching only works when redis is the cache driver. Why did you drop CacheResolver?

irfan-dahir commented 1 year ago

CacheResolver is from v3 where there's no database and everything is being cached on Redis.

Also, ETag from the code was removed because Nginx was supposed to be handling ETag. 😓

This should be resolvable from the master server directly. I'll look into it.

irfan-dahir commented 1 year ago

Microcaching only works when redis is the cache driver.

This should be fixed with #318 .

irfan-dahir commented 1 year ago

Also, ETag from the code was removed because Nginx was supposed to be handling ETag. 😓 This should be resolvable from the master server directly. I'll look into it.

Nevermind, it is not - just for static caching. Although I could've sworn it was handling it before with Nginx caching. 🤔

A fix for this would be an ETag middleware that handles comparing the Etag's (If-None-Match) hash with the controller's response hash since we are not just cashing responses now, we are querying it as well.

pushrbx commented 1 year ago

A fix for this would be an ETag middleware that handles comparing the Etag's (If-None-Match) hash with the controller's response hash since we are not just cashing responses now, we are querying it as well.

Watch out for this then: https://stackoverflow.com/questions/34238911/how-add-etag-header-on-json-response-in-laravel-lumen Also RoadRunner comes with an RFC7234 compliant cache driver which includes etags: https://roadrunner.dev/docs/middleware-cache/2.x/en Which also can be used to reduce caching related code in Jikan API. It also supports distributed caches if anyone would fancy running Jikan API on Kubernetes/Openshift/AWS ECS 🤣