Return additional request information - mk2 #39
Conversation
|
looks good - one piece of information I'd want from the cache is the time that the cached object was originally valid. I had a quick look at http headers and didn't notice that information. So if its not there, then the time it was cached would do. Scenario (based on xml api calls...for now) - you fetch your corp assets at 16:00, they are cached until 22:00. However if a second person fetched the corp assets at 18:00, they would get the version that was cached by CCP at 16:00, and they would want to know that this is the assets from 2 hours ago. |
|
Isn't the xml api out of scope since we are hitting the json endpoints here? |
|
It's an example as we do get information about untill when the object is cached at CCP it's side. In short: you know when you are good to retry. On CREST it's sadly a guessing game |
|
I mentioned the xml api to give an example of why you'd want to know when the data was cached by CCP (separately to when it was cached by pycrest) |
|
Sadly you only know in how much time you are sure to get new data with the CREST expire time and not when, the earliest, you can get new data. I think I saw many people that already requested it on the tweetfleet slack and the eve forum (but in vain atm... maybe one day) . As for the header informations using getItem, it looks good to me too, and it's great to finally have a (correct) solution to retrieve these informations :D |
|
Yeah, the lack of information when the cache in CREST is expiring is really just to bad. I hope they will implement it through the Cache-Control header which adjusts itself as time passes by, if that would happen we would pick it up and process it without any change. |
|
oh god so CREST always gives the same validity period, and you don't even know when the data truly expires? |
|
Uhuh, which makes the data for now worth very little but that should hopefully change in the future :) |
|
@wtfrank about your response to my xml comment, seems fine, was just curious about use case/scope-creep 👍 no worries it may be possible to jury-rig something together to know how long ccp caches for internally andthen only requery after certain intervals are met with regards to cache timers locally as well. |
|
As far as I know, each endpoint has the same format mime type with just varying numbers. If that's true then I reckon we can return the entire version string but also try and parse out the version number and return that also, because its less hassle checking the version against a number, than having to look up the exact string to paste into your code. |
|
Changed so you can reuse the string (in manipulated form) to request a specific version. As each endpoint has it's unique identifier just a >>> eve()['version']
5 # Before
'application/vnd.ccp.eve.Api-v5' # NowWhat I'm getting at is this: >>> eve()['version']
'application/vnd.ccp.eve.Api-v5'
>>> eve().time()['version']
'application/vnd.ccp.eve.Time-v1'
>>> eve().incursions()['version']
'application/vnd.ccp.eve.IncursionCollection-v1' |
|
Ah ok that's a shame. It would still be parseable in this response part, but it wouldn't be easy to set it from a number in the request part, as it would require knowledge of each endpoint. So I agree with that change :) |
|
ah I did not know the header string was actually different, unfortunate, but ok. |
|
Do you think we should pass the meta info down the chain when navigating to a sub-section of the json? |
|
if you want specific info about the endpoint then it should be ok at the root, I don't see any uses for it to be needed when interacting with response data. unless constellations is a different endpoint than regions? You should have it at every endpoint but not when you're doing the response items parsing, unless it really doesn't matter? |
|
Next stop would be for me to fix up the other methods like POST, PUT, DELETE. Not sure if those need adjusting but I would think they do. |
|
Regarding fixing up post/put/delete, they aren't returning any data so maybe they don't need anything doing? Or was it not the return data that you meant by fixing them up? |
|
Well they do return a |
|
are we sure that response header is just telling you which endpoint you are reaching? it may just accept the single apiVxx headers from client requests. |
|
No I'm not sure, maybe sending "v2" is enough and does work. This however doesn't mean we should take a shortcut which isn't documented. In the docs it states that it should be passed as a whole and thus that's the only correct thing to do. Quote from the docs about VersioningIn order to ensure that you always receive this version, you simply need to add this header to your POST (ignore the charset for versioning): Accept: application/vnd.ccp.eve.Api-v3+jsonIf you need version 2 of this resource, you can send this header instead: Accept: application/vnd.ccp.eve.Api-v2+json |
|
Actually, CREST will always accept "vnd.ccp.eve.Api-vXX+json" as it's the base version of all type. But instead of specifying "vnd.ccp.eve.Api-v5+json" you may also use "market-v1" (it's just for the example), but in the Content-Type, in the response, you'll have the "real" type (market-v1 and not api-v5) (so far it's what I observed, didn't find any doc about it). In terms of response for POST/PUT call, it depends on which endpoint you are using. For example, if you use the contact endpoint, in the response you have the url for the newly added contact in the Location header. You may also have something in the other you can "put" (autopilot routes, fits). For DELETE, i don't really know.. |
|
Regarding post/put/delete, according to https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html both put and delete are not cacheable, and post is not cacheable unless there is appropriate Cache-Control or Expires header fields. So a question arises: does it make sense to always expose 'expires' and the other cache related properties if its a response to one of those methods? And if it does, is the logic in the _get_expires() correct when it comes to the non-cacheable methods? (I'm thinking, what if there is a cache-control header on the response from put/delete?) |
|
I'm not considering if for caching purposes (obviously) but for handling other properties like status_code or perhaps the |
|
Do we need a subclass of the APIResponse object that contains caching properties, leaving the status_code and headers and stuff in the base class? Or should expiry etc return AttributeError or something like that if its not a cached response? Or is returning 0 or None for cache-related properties ok? |
|
I would say return a value or boolean for it not being cached, but that's just because I don't like try,catch in my code except for debugging. more communication is better I think. |
|
Fair enough - throwing exceptions should probably be an exception, not the normal way you found out if something was cached or not! subclass - might be a bit overengineering it. so something like 0 or None is prob fine for the non-relevant methods ! |
|
probably None so that 0 is not confused as a time or offset. |
…as each endpoint has its own string
hkraal
force-pushed
the
7-add-extra-request-information-attempt-2
branch
from
ee4ef74 to
86fdad4
Compare
After valuable feedback on #37 we have decided that we do not want to mix CREST data with other data such as response_codes, expiry dates etc. The call() and getattr() are reserved for CREST and thus we give access to the data using getitem().
There should be no impact on existing behavior