[WIP] Return additional request information

hkraal commented 8 years ago

It's a work in progress but I think a valid solution for retrieving extra data from the CREST request. It can easily be extended with additional fields. This PR is intended to take care of #7 and #18

I will be adding the API version header we get from CCP so the customer knows which version the output is.

I'm looking if I can find a way to prevent breaking backwards compatibility. Any input is appreciated

>>> from pycrest import EVE
>>> eve = EVE()
>>> print(eve())
{'result': {'systems': {'href': 'https://crest-tq.eveonline.com/solarsystems/'}, 'marketPrices': {'href': 'https://crest-tq.eveonline.com/market/prices/'}, 'opportunities': {'groups': {'href': 'https://crest-tq.eveonline.com/opportunities/groups/'}, 'tasks': {'href': 'https://crest-tq.eveonline.com/opportunities/tasks/'}}, 'marketTypes': {'href': 'https://crest-tq.eveonline.com/market/types/'}, 'constellations': {'href': 'https://crest-tq.eveonline.com/constellations/'}, 'serverName': 'TRANQUILITY', 'userCount_str': '17252', 'sovereignty': {'structures': {'href': 'https://crest-tq.eveonline.com/sovereignty/structures/'}, 'campaigns': {'href': 'https://crest-tq.eveonline.com/sovereignty/campaigns/'}}, 'races': {'href': 'https://crest-tq.eveonline.com/races/'}, 'itemTypes': {'href': 'https://crest-tq.eveonline.com/inventory/types/'}, 'wars': {'href': 'https://crest-tq.eveonline.com/wars/'}, 'itemGroups': {'href': 'https://crest-tq.eveonline.com/inventory/groups/'}, 'dogma': {'effects': {'href': 'https://crest-tq.eveonline.com/dogma/effects/'}, 'attributes': {'href': 'https://crest-tq.eveonline.com/dogma/attributes/'}}, 'industry': {'facilities': {'href': 'https://crest-tq.eveonline.com/industry/facilities/'}, 'systems': {'href': 'https://crest-tq.eveonline.com/industry/systems/'}}, 'itemCategories': {'href': 'https://crest-tq.eveonline.com/inventory/categories/'}, 'insurancePrices': {'href': 'https://crest-tq.eveonline.com/insuranceprices/'}, 'npcCorporations': {'href': 'https://crest-tq.eveonline.com/corporations/npccorps/'}, 'alliances': {'href': 'https://crest-tq.eveonline.com/alliances/'}, 'decode': {'href': 'https://crest-tq.eveonline.com/decode/'}, 'virtualGoodStore': {'href': 'https://vgs-tq.eveonline.com/'}, 'authEndpoint': {'href': 'https://login-tq.eveonline.com/oauth/token/'}, 'regions': {'href': 'https://crest-tq.eveonline.com/regions/'}, 'incursions': {'href': 'https://crest-tq.eveonline.com/incursions/'}, 'tournaments': {'href': 'https://crest-tq.eveonline.com/tournaments/'}, 'userCount': 17252, 'serverVersion': 'EVE-TRANQUILITY 14.06.1058053.1058052', 'bloodlines': {'href': 'https://crest-tq.eveonline.com/bloodlines/'}, 'corporations': {'href': 'https://crest-tq.eveonline.com/corporations/'}, 'serviceStatus': 'online', 'time': {'href': 'https://crest-tq.eveonline.com/time/'}, 'marketGroups': {'href': 'https://crest-tq.eveonline.com/market/groups/'}}, 'status_code': 200, 'expires': 20, 'timestamp': 1469542514.0203292}
>>> print(eve().time())
{'result': {'time': '2016-07-26T14:15:40'}, 'status_code': 200, 'expires': 10, 'timestamp': 1469542504.050126}
>>> print(eve().time().result)
{'time': '2016-07-26T14:15:40'}
>>> print(eve().time().expires)
10
>>> print(eve().time().status_code)
200

jonobrien commented 8 years ago

So you wrap the response in another dict for result?

Does this mess with dynamically calling an endpoint at all?

wtfrank commented 8 years ago

Ok so basically you're overloading getattr to return the extra info we need to be able to get from the cache. What if CCP created link within the CREST namespace that was called "result" (or endpoint_version, status_code, expires_in, expires_at)? There's a potential namespace clash issue (stemming from the design of having "." refer to parts of the crest namespace). Basically the way the APIConnection objects use ".", it makes it difficult to add functions to the object.

I'm trying to think if there's another operator we could use to traverse crest instead of using "." It would look understandable if we used ">" (ge) so we've have urls like eve() > marketData > ... however > is a binary operator so that wouldn't work.

wtfrank commented 8 years ago

One option could be to also implement getitem and have only crest stuff accessible by there, so that if crest defined blah and we had also defined a function blah() then calling APIObject.blah would give our function, but APIObject['blah'] would give the crest result. Thats not really ideal though.

hkraal commented 8 years ago

@jonobrien I've intended not to and I haven't seen any issues with it so far.

The funny thing here is that we do not clash with our namespaces at all. It took me quite some time to get it working but the unittest gives me the values I'd expect, even if they are in the "same" namespace. I've added a unittest to demonstrate.

As far as I see it we're only missing backwards compatibility (adding .result to contain the data) but this will introduce namespace clashes if I'm not mistaken (haven't gotten that far just yet). If we would like to offer that I would suggest we make it an configurable option or something so the user has a choice.

jonobrien commented 8 years ago

If we are being concerned about clashes with possibilities we could go on for days about possibilities that CCP could add. I wouldn't change the dot operator, that sounds like a huge breaking change.

hkraal commented 8 years ago

I agree, for now that just asking to much. I do feel we should take it into account in the future but that should be more of a PyCrest 2.0 thing perhaps?

wtfrank commented 8 years ago

What about if we return the "meta" attributes such as cache time etc via getitem, and leave getattr for the crest namespace.

That would mean you could do eve().time()['expires'], then we could add any information we can think of without worrying whether they clash with something CCP have added or might add in the future.

jonobrien commented 8 years ago

I feel like this package is just supposed to handle interacting with CREST, it seems to have/will have a pretty stable set of functions to handle those different use cases. What else would we really need to add to make this a feature-complete CREST api interacting package?

we have:

public
authed
get/post/etc
authed refresh

we need:

maybe some docs/usage examples
versioning -- soon (tm)
proper cache logic for specific endpoints

jonobrien commented 8 years ago

@wtfrank sounds good as long as we document old and new, at least for now, as people would still be using the old via pypy

hkraal commented 8 years ago

@jonobrien https://github.com/pycrest/PyCrest/milestone/1, if you have input which isn't there and you think it should gitter or raising an issue would be the places to continue that

jonobrien commented 8 years ago

@hkraal Oh I missed the milestone, nevermind then. :+1:

wtfrank commented 8 years ago

@jonobrien I think the suggestion wouldn't change any of the old way of doing things, as these attributes didn't exist on APIObject in the past!

hkraal commented 8 years ago

About @wtfrank 's __getitem__: It would certainly be viable option for me to use that so we don't mingle in the crest namespace. At this point I've no clear view on how we would differentiate crest vs meta attributes but I will take a look at it soon (tm)

wtfrank commented 8 years ago

I would say cache returns a tuple: (actual_response, dict_of_meta_info)

Then instead of storing 'result' in the APIObject's _dict attribute, you would store the meta info in APIObject's _meta attribute, then you have the actual data and the metadata separated cleanly.

wtfrank commented 8 years ago

So the possibly silly case that I'm worried about: lets say in the future CCP returns expires: {'href' : 'xxx'} in their JSON, you would do eve().expires() to find out whatever data CCP was returning (as current behaviour), you would do eve().expires()['expires'] to find out the expiry of the cached data from the expires crest section, or if you wanted to find the expiry of the root data from the eve object, you would do eve()['expires'].

hkraal commented 8 years ago

As far as I'm aware there is only 1 edge case with my code: Before these changes eve().expires would give you the url self._endpoint + '/expires', with my changed eve().expires would give you the expires metadata property of the eve() request. If you would like to check the metadata of the expires endpoint you could do eve().expires().expires and it would return you the expires metadata property.

You can see it actually working in the 10918f6d082c4cfb744ca28c0a0d6333db1ec737 unittest

wtfrank commented 8 years ago

Does it work if the JSON returned from eve() contained a key called 'expires' that was like serverName in the code section I pasted below? So rather than a wrapped dictionary that pycrest would "browse" through, it was data that pycrest would return to you directly.

{'constellations': {href: 'xxx'}, 'serverName': 'TRANQUILITY'}

>>> print eve().serverName, type(eve().serverName)
TRANQUILITY <type 'unicode'>

hkraal commented 8 years ago

It does (or I must be misunderstanding you). Let's take an example for clarity:

Let the body being returned when requesting "/market/prices/" is as follows:

{
    "result": "10213",
    "items": [],
    "status_code": 500,
    "pageCount_str": "1",
    "totalCount": 10213
}

This would be the result:

>>> res = self.api().marketData()
>>> res.status_code # HTTP request response code
200
>>> res.result.status_code # body status_code value
500
>>> res.result.result # body result value 
'10213'

wtfrank commented 8 years ago

lets get a bit more perverse then...what about this?

{
    "result": { "result" : "10213", status_code : 500},
    "items": [],
    "status_code": { "result" : "10214", status_code: 501},
    "pageCount_str": "1",
    "totalCount": 10213
}

hkraal commented 8 years ago

Seems to do what I intended:

self.assertEqual(res.status_code, 200)
self.assertEqual(res.pageCount_str, '1')
self.assertEqual(res.result.pageCount_str, '1')
self.assertEqual(res.totalCount, 10213)
self.assertEqual(res.result.totalCount, 10213)
self.assertEqual(res.items, [])
self.assertEqual(res.result.items, [])
self.assertEqual(res.result.status_code.status_code, 501)
self.assertEqual(res.result.status_code.result, '10214')
self.assertEqual(res.result.result.result, '10213')

Unittest is completed successfully

wtfrank commented 8 years ago

haha fair enough so it could work!

hkraal commented 8 years ago

Ok... so what would these commits change:

each call foo(), foo().bar() etc will result a dictionary being returned with additional information like the following eve().time() example and will contain the "old" response in .result

>>> eve().time()
{
  'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json',
  'expires_at': 1469641381.3412392,
  'status_code': 200,
  'result': {
    'time': '2016-07-27T17:42:50'
  },
  'expires_in': 10
}

The properties of the call are still accessible via the old way (should we sent out deprecation notices when this get's used?):

>>> eve().time().time
'2016-07-27T17:49:40'

The properties of the call should however be accessed by .result:

>>> eve().time().result.time
'2016-07-27T17:51:50'

In case __call__() returns an href attribute like eve() has marketData and marketData is an property of the response object you will not be able to retrieve the url by calling eve().marketData. This will return the property of the response object instead. This would happen now if we would get an .endpoint_version, .expires_at, .status_code, .result or .expires_in.
Attribute failures haven't changed afaik:

>>> eve().time().localtime
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/henk/Documents/Projects/pyCrest/src/pycrest/eve.py", line 450, in __getattr__
    return self._dict['result'].__getattr__(item)
  File "/home/henk/Documents/Projects/pyCrest/src/pycrest/eve.py", line 452, in __getattr__
    raise AttributeError(item)
AttributeError: localtime

>>> eve().time().result.localtime
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/henk/Documents/Projects/pyCrest/src/pycrest/eve.py", line 452, in __getattr__
    raise AttributeError(item)
AttributeError: localtime

You can move deeper into crest (can we get T-shirts with that?) by the response object or .result

>>> root = eve()
>>> root.result.time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642176.007282, 'status_code': 200, 'result': {'time': '2016-07-27T17:56:00'}, 'expires_in': 10}

>>> root.time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642187.9248161, 'status_code': 200, 'result': {'time': '2016-07-27T17:56:10'}, 'expires_in': 10}

>>> eve().result.time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642290.9267902, 'status_code': 200, 'result': {'time': '2016-07-27T17:58:00'}, 'expires_in': 10}

>>> eve().time()
{'endpoint_version': 'application/vnd.ccp.eve.Time-v1+json', 'expires_at': 1469642290.9267902, 'status_code': 200, 'result': {'time': '2016-07-27T17:58:00'}, 'expires_in': 10}

jonobrien commented 8 years ago

this might be a good time to pull in versioning (#17) notices since you mentioned deprecation notices.

have you tried pulling market data? If we are saving the entire old response as well, is there going to be a lot of overhead moving around large amounts of data this way, with high item counts?

Or an I just misreading this and we just have to access the response via obj.result now instead of just parsing it directly?

hkraal commented 8 years ago

It's shouldn't make a huge difference in performance, there are some extra calls but as these are so amazingly fast I doubt we would ever notice.

Had a chat with @wtfrank on gitter and we ended up not wanting to do this. It's funny to see it works but as he put it:

(08:35:54 PM) wtfrank: I would describe the old behaviour as: "." references the object within the json returned by crest, "()" loads the link specified in an href at the current position of the josn returned by crest
(08:36:34 PM) wtfrank: but in that patch, "." means either a) reference an object within the json returned by crest, or b) reference a magic attribute added in that you have to memorise

I started over and now have the following.

>>> eve = EVE()
>>> eve()
{'userCount_str': '26249', 'marketTypes': {'href': 'https://crest-tq.eveonline.com/market/types/'}, 'constellations': {'href': 'https://crest-tq.eveonline.com/constellations/'}, 'decode': {'href': 'https://crest-tq.eveonline.com/decode/'}, 'virtualGoodStore': {'href': 'https://vgs-tq.eveonline.com/'}, 'itemTypes': {'href': 'https://crest-tq.eveonline.com/inventory/types/'}, 'itemGroups': {'href': 'https://crest-tq.eveonline.com/inventory/groups/'}, 'tournaments': {'href': 'https://crest-tq.eveonline.com/tournaments/'}, 'serverVersion': 'EVE-TRANQUILITY 14.06.1058053.1058052', 'insurancePrices': {'href': 'https://crest-tq.eveonline.com/insuranceprices/'}, 'sovereignty': {'structures': {'href': 'https://crest-tq.eveonline.com/sovereignty/structures/'}, 'campaigns': {'href': 'https://crest-tq.eveonline.com/sovereignty/campaigns/'}}, 'regions': {'href': 'https://crest-tq.eveonline.com/regions/'}, 'userCount': 26249, 'corporations': {'href': 'https://crest-tq.eveonline.com/corporations/'}, 'industry': {'facilities': {'href': 'https://crest-tq.eveonline.com/industry/facilities/'}, 'systems': {'href': 'https://crest-tq.eveonline.com/industry/systems/'}}, 'dogma': {'attributes': {'href': 'https://crest-tq.eveonline.com/dogma/attributes/'}, 'effects': {'href': 'https://crest-tq.eveonline.com/dogma/effects/'}}, 'incursions': {'href': 'https://crest-tq.eveonline.com/incursions/'}, 'systems': {'href': 'https://crest-tq.eveonline.com/solarsystems/'}, 'races': {'href': 'https://crest-tq.eveonline.com/races/'}, 'opportunities': {'tasks': {'href': 'https://crest-tq.eveonline.com/opportunities/tasks/'}, 'groups': {'href': 'https://crest-tq.eveonline.com/opportunities/groups/'}}, 'alliances': {'href': 'https://crest-tq.eveonline.com/alliances/'}, 'authEndpoint': {'href': 'https://login-tq.eveonline.com/oauth/token/'}, 'time': {'href': 'https://crest-tq.eveonline.com/time/'}, 'serverName': 'TRANQUILITY', 'marketGroups': {'href': 'https://crest-tq.eveonline.com/market/groups/'}, 'itemCategories': {'href': 'https://crest-tq.eveonline.com/inventory/categories/'}, 'serviceStatus': 'online', 'bloodlines': {'href': 'https://crest-tq.eveonline.com/bloodlines/'}, 'marketPrices': {'href': 'https://crest-tq.eveonline.com/market/prices/'}, 'wars': {'href': 'https://crest-tq.eveonline.com/wars/'}, 'npcCorporations': {'href': 'https://crest-tq.eveonline.com/corporations/npccorps/'}}
>>> eve()['status_code']
200
>>> eve()['endpoint_version']
application/vnd.ccp.eve.Api-v5+json
>>> eve()['expires_in']
20
>>> eve()['expires_at']
1469648871.1075354

hkraal commented 8 years ago

Closing this in favor of #39

pycrest / PyCrest

[WIP] Return additional request information #37