Skip to content
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

Open
DaneelTrevize opened this issue Apr 26, 2018 · 5 comments
Open

/characters/{character_id}/fleet/ cache timer is wrong? #883

DaneelTrevize opened this issue Apr 26, 2018 · 5 comments

Comments

@DaneelTrevize
Copy link

DaneelTrevize commented Apr 26, 2018

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 have Expires: 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's Expires: 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.

@a-tal
Copy link
Contributor

a-tal commented Apr 26, 2018

how does returning 0 if you're not in a fleet sound?

@Aidansavage
Copy link

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 Retry-After: header or a 304 (with a Retry-After: header as well).

@DaneelTrevize
Copy link
Author

DaneelTrevize commented Apr 27, 2018

how does returning 0 if you're not in a fleet sound?

A not-counting-as-an-error response sounds great. Would it be a code 2xx? What sort of cache period is likely?
For consistency with the IDs in the response for not being in a squad or wing (when you're in the top FC position), a -1 value for fleet_id would be more in line.
Would this be the only response field? If you use the current 200 model, would you introduce a new role enumeration? Doing so might be easier for those currently supporting caching this endpoint, the response would still contain the primary key of character_id, schemas & systems would only need to be adjusted to permit a -1/NULL value for fleet_id, and this new enum case.

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?

@Seavert
Copy link

Seavert commented Apr 27, 2018

"204 - The character is not in a fleet" seems fine to me.

@DaneelTrevize
Copy link
Author

If that's just a description, and not the actual body/content (a 204 shouldn't have any), kinda like DELETE /fleets/{fleet_id}/members/{member_id}/. Sounds fine indeed.

But if we're changing this endpoint, and not just clarifying the cache timers, please don't forget #774

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants