New architecture & Tests

[Ogaday]

I've been working on this throughout the week and think it could probably do with more work & reviewing before merging: I'd definitely appreciate feedback.

This PR is mostly a proof of concept of the architecture laid out by @pocin in #6: I've tried to stay close to the API and so you could call it quite a thin client?

I've also had a play with some tests. I used the very nice responses library to mock requests, and that way we don't need real tokens or even an Internet connection to test the client. In the tests you define what the API response should be and assert that the client isn't making any mistakes before or after calling requests. I think it might still be useful to have some real API calls to make sure everything is working sensibly, but I don't think I have access to the test environment.

I also modified setup.py a bit. Essentially, previously upon install a package called tests was also being installed. Now, that should no longer be the case. If you want to run the tests, you should clone the directory and run them. There are several options for running tests:

After pip install responses, do

cd path/to/sapi-python-client/ && python -m unittest discover

or

python -m discover -s path/to/sapi-python-client/

or

pytest path/to/sapi-python-client/tests/

or

green path/to/sapi-python-client/tests/

etc.

Alternatively, without needing to even install the package:

git clone git@github.com:Ogaday/sapi-python-client.git && cd sapi-python-client && python setup.py test

Which takes care of all dependencies.

[odinuv]

needs to be KBC_TEST_TOKEN - my mistake, sorry about that

[odinuv]

this is perfecly ok

[odinuv]

the client must allow overriding of https://connection.keboola.com/ but not v2/ or /v2/storage/ part. The base url may change (actually we do already have https://connection.eu-central-1.keboola.com, but v2 is tied to API (and client) version

[Ogaday]

In my mind the client constructs the string and passes it to the Endpoint. ie.

class Client:
    def __init__(self, base_url, token):
        api_version = 'v2/storage'
        root_url = base_url + '/' + api_version
        self.buckets = Buckets(root_url, token)
        ...

Does that work? Should API version also be a parameter? Or do you think each endpoint should construct the API string?

[odinuv]

yeah, that looks perffect, the api version should be hardcoded (there is no other version, not even in plans and when it will be who knows how it will look) and the endpoint should not repeat do the constructing

[odinuv]

i'd say we need dict to for params because of this http://docs.keboola.apiary.io/#reference/events/list-bucket-events/bucket-events-list or maybe better list of dicts because of wherevalues over here http://docs.keboola.apiary.io/#reference/tables/unload-data-asynchronously/asynchronous-export

[Ogaday]

Maybe params is the wrong variable name for that argument. From what I can see, there are two types of params that can be passed: parameters which form the query string, like the parameters for list-bucket-events, such as sinceId etc; and parameters which go in the path of the url, such as workspace_id. The params arg as it stands it stands is solely for the latter, and maybe should be renamed. Query string params can be passed as a dict to requests.get.

Writing this code, and trying to made the Endpoint._get and Endpoint._post methods as versatile as possible it seems that we have to essentially replicate the requests.get and requests.post functionality which is redundant. I've been thinking about suggesting some changes in #6. the Endpoint class is still useful, but perhaps each endpoint subclass should be constructing its own requests, as they are so straightforward and the Endpoint superclass should instead implement response handling and header generation.

[pocin]

I see the Endpoints _get() _post,... methods merely as a very thin wrappers for the requests native http methods, which only appends the X-StorageApi-token: TOKEN header to the request. I think it makes sense to delegate the url construction to the endpoints. And if some repetitive code appears, we can always refactor it into the base Endpoint class.

fwiw it's probably not even necessary to hide them with the leading _underscore but make them "public" i.e Endpoint._get(*args, **kwargs) --> Endpoint.get(*args, **kwargs).

[Ogaday]

I think then that the base Endpoint get and post methods don't add any value. I do still think that the base endpoint class is still useful for a few things and I have half implemented some ideas, so hopefully I'll be able to share them in a few days.

[odinuv]

come to think of it - wondering how to pass array parameters (whereValues) - http://docs.keboola.apiary.io/#reference/tables/unload-data-asynchronously/asynchronous-export
as it is now it would have to be - not sure how requests library handles those

[pocin]

requests can handle array parameters, but it encodes the [] into %5B%5D. That should not be an issue for the storage api, right?

try this:

>>> import requests
>>> r =requests.get('https://httpbin.org/', params={'whereValues[]':['12','34','56']})
>>> r.url
'https://httpbin.org/?whereValues%5B%5D=12&whereValues%5B%5D=34&whereValues%5B%5D=56'
>>>

So really all we need to do is to expose the native requests functionality in the Endpoints get method and we should be good to go

[pivnicek]

That will be ok for GET requests, but for array values in the form-data body I'm not sure. I see this in the docs

But it's something we'll have to try out.
Currently it constructs this manually which is obviously not ideal: https://github.com/keboola/sapi-python-client/blob/master/kbcstorage/client.py#L124

[Ogaday]

@pivnicek I'm not sure I understand, sorry. When I try that code snippet I get this, which is what I would expect. Is this not what we want?

>>> import requests
>>> payload = (('key1', 'value1'), ('key2', 'value2'))
>>> r = requests.post('http://httpbin.org/post', data=payload)
>>> print(r.text)
{
  "args": {}, 
  "data": "", 
  "files": {}, 
  "form": {
    "key1": "value1", 
    "key2": "value2"
  }, ...

And @pocin that's what I found as well, arrays params for urls should be fine.

[pivnicek]

@Ogaday there may be an issue may be with the whereValues in the request body http://docs.keboola.apiary.io/reference/tables/unload-data-asynchronously/asynchronous-export.
The backend expects it to be an array which is why it's constructed by hand that way currently. Maybe the requests library formulation (below) will work, but maybe not, we'll see :-)

{
   "whereValues": [
      "value1",
      "value2
   ]
}

[odinuv]

I'm playing with the idea that it might be better to pass the endpoint class to workspace class as a ctor parameter - it's just that this way one has to pass the token all around the code and it's bound to leak somewhere...

[Ogaday]

I'm not sure what that would look like.

How about passing the client to the endpoint as an alternative...

class Endpoint:
    def __init__(self, client):
        self.client = client
    def _get(self, url, *args, **kwargs):
        headers = {'X-SorageAPI-Token': self.client.token}
        ...    # etc

class Client:
    def __init__(self, root_url, token):
        self.token = token
        self.endpoint = Endpoint(self)
        ...    # ?

Or is that no better?

Maybe this can be taken up in #6

[odinuv]

hmm, I was probably thinking more like:

cl = new Client('my-token', 'https://connection') # <- only one place in code where token is passed to the client
workspace = new Workspace(cl)
workspace.reset_password()
configuration = new Configuration(cl)
configuration.get('my-config')

[pocin]

@odinuv so instead of passing the token to each endpoint you'd pass the client class, I don't see any benefit in that.

.. it's just that this way one has to pass the token all around the code ...

Not really, you would only pass the token in the Client.__init__() method, which would internally distribute it to the endpoints.

i.e

from urllib.parse import urljoin

class StorageClient:
    def __init__(self, token):
        self.base_url = 'https://connection.keboola.com/v2/storage/'
        workspaces_url = self.base_url
        buckets_url = urljoin(self.base_url, 'buckets')

        self.workspaces = Workspaces(workspaces_url, token)
        self.buckets = Buckets(buckets_url, token)

and use like this

>>> client = StorageClient('token123')
>>> client.workspaces.list()
{
    "id": 234,
    "name": "boring_wozniak",
    "component": "wr-db",
    "configurationId": "aws-1"
   # ... omitted for clarity
}
>>> client.buckets.list()
{ response listing all buckets }

I think this has several benefits, the client class is essentially your gateway to the world of the keboola storage api:

• you don't pass anything anywhere (tokens, clients,...)
• you don't have to import and instantiate the endpoints individually
• All endpoints are in one place, convinient and ready to use

---

Now when I am thinking about it, what would the Client class do in your example? To me it looks just like overly complicated conatiner for the base_url and token

class Client:
    def __init__(self, base_url, token):
        self.base_url = base_url
        self.token = token

[Ogaday]

I agree with @pocin - The token is only passed internally by the __init__ method of the client. Ultimately, the Endpoint instance needs to make a request with the token, so it needs access to it somehow.

I think having the capability to do client.endpoint1.list() and client.endpoint2.list() is very elegant :)

[odinuv]

table mapping has other parameters like days, rows, etc. while it is not important to support them now, the structure has to be extensible, i.e. i'd say it either has to be array of dicts (each wit keys source, destination, etc) or a designated object

[Ogaday]

I definitely want to support those additional parameters because it's a use case I require personally but I don't think this is fully documented. I have link from keboola support which is helpful of course, but would be good to have a full spec.

[pivnicek]

@Ogaday this is the API doc for the workspace load command http://docs.keboola.apiary.io/#reference/workspaces/manage-workspace/load-data. It could probably use a full example though. Is there a particular part that is missing for you?

[ujovlado]

@Ogaday we already tracking that as "documentation issue"

[Ogaday]

@ujovlado Thanks! I recognize your name from my support thread. I don't doubt that you are handling this :)

[odinuv]

is not used here

[Ogaday]

Good catch

[odinuv]

@Ogaday I think you did a Great Job! I left a bunch of notes, i hope it does not put you off. What if feel is missing:

• move the stuff from current client to use the endpoint class and get rid of that one so that we don't have a papal schizm - i'd really like that before merging
• handling errors http://docs.keboola.apiary.io/#introduction/http-response-codes/success-responses - especially handling maintenance and perhaps having own exceptions with more readable errors - all this can probably be done later - this probably deserves own issues and pr
• some validation of parameters, e.g that the passed workspaceid is not empty because e.g. calling https://connection.keboola.com/v2/storage/workspaces/workspace_id/password with empty workspace_id essentially means that https://connection.keboola.com/v2/storage/workspaces/password is called which gives confusing error messages - again, this probably deserve own issue and pr.
• i'm going on hols for a week, so i won't be much help

[odinuv]

Also with the mocks - they are good, but i wouldn't spent too much time on them. The client is and will be thin and it is most easily and reliably tested against the actual API, we are used to this. It's probably no problem for you to have a testing project for this.

[Ogaday]

Thanks for the feedback @odinuv, it doesn't put me off at all :) I'll respond to the other comments in line above.

Using mocks makes my life easier for now, happy to add real API call tests later too.

move the stuff from current client to use the endpoint class ... handling errors ... some validation of parameters

Good points, I'll look into these. I agree handling errors can wait a bit, as HTTP errors currently fail pretty fast!

RE travis failures: I don't know enough about docker or CI to guess why the build process always fails whenever I commit anything. I don't suppose you know why this is either?

i'm going on hols for a week, so i won't be much help

Not a problem, I'm taking a long weekend on Wednesday too

[odinuv]

You should be able to see the errors in https://travis-ci.org/keboola/sapi-python-client/builds/266261167?utm_source=github_status&utm_medium=notification - it's something wrong with the token, i'll check it

[odinuv]

Ok, i learned something new:

Travis CI makes encrypted variables and data available only to pull requests coming from the same repository. These are considered trustworthy, as only members with write access to the repository can send them.
Pull requests sent from forked repositories do not have access to encrypted variables or data

So the tests against real API won't pass in Ogaday:dev, but will work in keboola:dev

[Ogaday]

I found a crappy bug in my code and I've got some commits in the works so hopefully I'll make some pushes today

[Ogaday]

I'm a lot happier with the code now. There is probably some repetition and doubtless some mistakes have slipped through, but I think we can refactor that / optimise it later.

I did remove some functionality from the client class, for now, but I will focus on restoring it. Also as a priority / items to keep track of are:

• Argument validation as mentioned by @odinuv above.
• Increase endpoint implementation. There are a lot of endpoints, some are simple, some are more complicated.
• Improve parameters passed to the load_tables method of Workspaces.
• Add error handling to the base Endpoint request methods.
• Increase & improve test coverage (as always).

I also tried to bring the existing bucket tests in line with the "new" architecture, but I don't know if they'll work on Travis or not.

[pocin]

This is a fairly hidden gotcha. Instead of the finally write else. Otherwise the raised exception will be silenced by the finally.

This should work as expected

        try:
            r.raise_for_status()
        except requests.HTTPError:
            # Handle different error codes
            raise
        else:
            return r.json()

try this in your interpreter to see what I mean

class Resp:
    foo = 'bar'

def test(r):
    try:
        print(1/0)
    except:
        raise
    finally:
        return r.foo

r = Resp()

print(test(r)) #will print 'bar', but you would expect ZeroDivisionError

[Ogaday]

Thanks, that is nasty! I wrote a test for it and hopefully fixed it.

[pocin]

same as above, just marking it

[odinuv]

I'm merging this so that I can make changes to it, I also made issues of the checklist (those I can imagine being 'delivered')