Cache expiry time should be accessible to users
ayust opened this issue · 22 comments
As mentioned in the following posts:
https://forums.eveonline.com/default.aspx?g=posts&m=1983409#post1983409
https://forums.eveonline.com/default.aspx?g=posts&m=1988780#post1988780
it is sometimes useful to know how long the results from a given call will be hitting cache.
Notes from reply in-thread:
I think that there's probably two kinds of data from the EVE API - some of it, no one really cares about cache times on because it either doesn't change or changes frequently enough that you can just assume it updates every X minutes and be fine.
The rest of it actually has some meaningful cache duration (say, an hour). It seems like the latter is what we really want to expose cache expiry timestamps for, and that could probably be worked into the returned data structures in a meaningful way (probably as an cache_expiry_ts or some such).
#132 has partially implemented this - the .last_timestamps field on an API object is now set when making calls.
However, we should also eventually try to add this information to the wrapper layer.
I've begun work on this overhaul here: https://github.com/ayust/evelink/tree/expose_timestamps
@flexd, @metatoaster - take a look at #139 and see if I've missed anything.
I guess returning 3-tuple is a way to do it. I really much prefer the current single result item.
May I suggest this:
- have
API.getreturn the whole tree, then - split (or change) the
elem_gettersto a result getter and have it fetch from the result node (i.e. callelem = tree.find('result'))? - return the result with the timestamps as new fields for all the wrapped access methods.
This should break only just the users of the lowest level API objects and not so much the existing code that only expects a single dict as result for the wrapped access methods.
The thing is that not all of the wrapped access methods return dictionaries - some return lists, some return straight integers, etc. So trying to integrate the timestamps into the existing result values would be inconsistent at best.
The tuple solution allows for consistency across all of the API calls.
Oh whoops I should have known better. My other concern is that CCP might later provide more metadata that user care about (thus more keys for the keys throne?).
Another idea I am toying with is that the API can accept a result set constructor to return the final data + metadata layout that the user might desire - that may provide a way to maintain backward compatibility such that existing users who completely disregard metadata can provide a constructor that just return the data without the timestamps. Might potentially allow backward and future compatibility better because who knows if/when CCP change their things around.
Downside is that there will be potential inconsistencies between the different users of evelink, but yeah this is just an idea at trying to limiting the impact but also try to tackle this problem.
What if, for example, the following code returned everything like it does now, completely unchanged.
eveapi = evelink.api.API(api_key=(api_key, vcode))
eve = evelink.eve.EVE()
charid = eve.character_id_from_name(eve_character)
char = evelink.char.Char(char_id=charid, api=eveapi)
api_transactions = char.wallet_transactions()But if we changed it to
api_transactions = char.wallet_transactions().cache()Or some other clever idea, then it returns a dict {"currentTime": now(), "expiresAt": blabla, "content": booo}, or a tuple of values.
Basically rewrite it so that any method called returns that dict or tuple, and the method at it currently is would just return the "content" bit?
So you would have to explicitly call a method to get the same call with cache times?
That would require whatever char.wallet_transactions() returns to have a cache() method, which means it couldn't be the existing return value.
Well I still think some form of this is the right way to go,
And if there are breaking changes to be made, some of the call names should be changed, I’ve more than once had to look in the code to figure out how to do something after having looked at the EVE API documentation, but I guess that should be in another issue.
On 17 Nov 2013, at 03:06, Amber Yust notifications@github.com wrote:
That would require whatever char.wallet_transactions() returns to have a cache() method, which means it couldn't be the existing return value.
—
Reply to this email directly or view it on GitHub.
I think I got a fully backwards/forwards compatible method with very minimum changes required to the existing code. Give me a few hours as I work out some changes + code to see if what I suggested is actually feasible because this really gave me a good nerdsniping.
If you mention the gist of it, I might be able to offer suggestions...
Okay so that was a bit more than a few hours. Took quite a bit of thinking, but I think I nailed it down.
Easier to explain what I really wanted to do in code rather than in English, especially since I had no idea if it was even feasible at all, but looks like it is.
Note how the unmigrated classes (corp) still work by default. To enable the new behaviour simply just set the default_result_key to an empty string and classes supporting the new result set will return the new values in a dict.
>>> char = Char(p_code, p_api)
>>> char.wallet_balance()
110779143.4
>>> char.api.default_result_key = '' # change to new style.
>>> char.wallet_balance()
{'current_time': 1384689840, 'result': 110779143.4, 'cached_until': 1384690740}
>>> char.current_training()
{'current_time': 1384689891, 'result': {'end_sp': 1280000, 'level': 5, 'current_ts': 1384689891, 'start_sp': 226275, 'type_id': 3456, 'active': None, 'end_ts': 1385585453, 'start_ts': 1383899493}, 'cached_until': 1384693311}
>>> char.api.default_result_key = 'result' # changing it back to legacy default
>>> char.contract_bids()
[]
...
>>> corp.starbases()
{1011818553441: {'state':... # opsec lol
>>> corp.api.default_result_key = ''
>>> corp.starbases() # not supporting the new style
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "evelink/corp.py", line 356, in starbases
for row in rowset.findall('row'):
AttributeError: 'NoneType' object has no attribute 'findall'
As to what I actually did? Relatively simple, but the testing pretty much needs to be redone. The mocks became a liability so I pretty much ended up writing up an entire new testsuite for the corp tests (with the aid of a few scripts/regex) but it works, and test coverage mostly improved.
Will finish the rest tomorrow or whenever I have time later this week (looks like Wednesday earliest).
I like it.
Edit:
Actually it would probably be best if you can do:
eveapi = evelink.api.API(api_key=(api_key, vcode), default_result_key='result')or this to signify that we want all the results returned as a dict.
eveapi = evelink.api.API(api_key=(api_key, vcode), advanced=True)Actually I lied about being simple. It doesn't just support the result key thing. The default_result_key just so happens to have the value of the key that contains the result the user happens to be interested in for a particular node, and the actual thing that uses this is a callable class that provides a format and unformat methods that act on the result with the source that the result is derived from in a very predictable way. Naming this once everything is finalized is going to be pretty hard. It's pretty complex, but well defined.
I would not vote for a key like that. Who knows what advanced really means. I am considering using inheritance (as much as I have grown to dislike using this because it's too often used like a hammer on a screw) to represent this. Use the standard API object for previous behaviour, and define a new API class that will do this behaviour I am introducing.
I feel like this is over-complicated, all to try to address a minimal problem. The EVE API's metadata historically has been extremely stable.
the advanced option should be the least backwards compatible breaking changes though.
If you do not specify advanced=True all is normal, if you do it returns a dict with the metadata?
That's the thing, though - I'm don't really care nearly as much about breaking backwards compatibility at this point in the library's development, relative to the amount of code complexity it would introduce. I'd rather keep simpler library code and break backwards compatibility in version 0.3, than I would want to introduce a much more complex system simply to maintain compatibility with 0.2's API.
Existing projects can continue to use 0.2 until or unless they have time to migrate to the new API (given that anything using 0.2 probably doesn't care about timestamps anyway, and thus doesn't need the new functionality).
Really though it only looks complicated. The majority of the changes I've introduced are done on the testing level (basically use the full XML document and not using mocks). Result value is essentially the same as how you did it, but rather than directly return a 3-tuple everywhere it just returns a result formatted to how the user wants. The core of it is done at a31b137 which isn't all that much complexity. The additional complexity only only comes in the handling of the two forms of data returned, i.e. root eveapi node or the result node that has the timestamps stripped.
A neat outcome after this is that you can create a new formatter that would return ORM objects from a result dict, for instance - basically you can treat the existing classes as accessors and you allow users to define how they want their results returned.
It's still introducing another layer of indirection where I don't think there really needs to be one, not to mention embedding backwards-compatibility logic where I don't think it's necessary. Having functions that sometimes return one thing and sometimes return another seems less than ideal.
Right now I'm leaning more towards returning a namedtuple from API calls, which could be updated in the future with more fields if necessary, and allow people to use char.wallet_balance().result instead of char.wallet_balance()[0], but would be a consistent return type across all of the wrapper functions.
I second the namedtuple approach.