-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
/characters/{character_id}/fleet/ cache timer is wrong? #883
Comments
how does returning |
FWIW, RFC 7231 does not say they should be cacheable, but that it is the default behavior, unless otherwise modified by method definition or explicit cache controls. Beyond that, other alternatives would be a bit of a hackjob 503 with a |
A not-counting-as-an-error response sounds great. Would it be a code 2xx? What sort of cache period is likely? Can we get confirmation that the 60secs spec value is accurate, is it intended that non-200 responses expire so much sooner/instantly and you can currently effectively query the endpoint much faster? |
"204 - The character is not in a fleet" seems fine to me. |
If that's just a description, and not the actual body/content (a 204 shouldn't have any), kinda like But if we're changing this endpoint, and not just clarifying the cache timers, please don't forget #774 |
Inconsistency
Routes
/characters/{character_id}/fleet/
Description
The swagger spec says
This route is cached for up to 60 seconds
. Response code:200 responses also seem to haveExpires:
headers consistent with this (the 304 response is also defined as having this header), but..:HTTP 404 responses (in this case for fleet not found) defined by RFC 7231 should be cacheable. This would be sensible for ESI to support, enabling an app to know to wait until a meaningful change in the response is possible to be seen.
But using the 60seconds value for these responses doesn't seem correct here (whether taken from
x-cached-seconds:
in the endpoint spec, or hard-coded to match 200'sExpires:
period), if you ignore that and query this endpoint again within a few seconds & having just dropped or formed a fleet, you get the new values from it.I haven't yet tested ignoring the 200 response Expires header and seeing if the 60secs claim is inaccurate.
Resolution
Either change the spec value to reflect the true cache period for this endpoint, if it's really something much closer to the other fleet endpoints of 5 seconds, in all response cases.
Having to assume that non-200 reponses are to be cached for the same duration as 200s/
x-cached-seconds:
isn't a huge issue, and can be dynamically determined from that spec.OR
Possibly enhance the spec to include (& trigger generation of) dedicated
Expires:
headers for 404 & other cacheable responses, which are potentially a different cache period than the 200 response.The text was updated successfully, but these errors were encountered: