Add User token to HTTP Header #31

travelist · 2017-05-29T20:23:39Z

No description provided.

travelist · 2017-05-31T21:26:43Z

Error seems to come from pip install with python 2.6 environment.
Not sure how to fix it since havent change requirements.txt. any clue?

kosukemori

I can't find doc about python's version neither.
@victorfonsec4 @marcsolanas
Which version do you developed?

kosukemori · 2017-05-31T21:39:43Z

zeus/client.py

@@ -23,14 +23,17 @@

 class ZeusClient(MetricsInterface, LogsInterface, AlertsInterface,
                 TrigAlertsInterface):
-
    """Zeus Client class, implementing wrapper methods for the
    Zeus API.

    """

    def __init__(self, user_token, server):
        self.token = user_token


@travelist
user_token is duplicated in self.token and in header, but for compatibly, it's necessary, right?
(Just asking)

they says user_token but this was external_token. header change is necessary to support for user token, meanwhile external token is used in url path.

def __init__(self, user_token, server): # => user_token is eiter external token or user token... # agreed that variable name is a bit misleading,,, pass

travelist · 2017-05-31T21:59:19Z

IMHO, these things might be worth considering

In stead of using Interface (mixed-in? seems these classes have concrete implementations?), better to add methods dynamically to the class definition
- https://stackoverflow.com/a/17930262/4126250
- example: https://github.com/googlemaps/google-maps-services-python/blob/master/googlemaps/client.py#L365
Serializing and deserializing should be done by one place instead of doing it in each implementation

it would be helpful to know what is the current implementation design.

victorfonsec4 · 2017-05-31T22:13:10Z

I'm using python 2.7.1, I think this was the version they used when creating this project, not sure though

victorfonsec4 · 2017-05-31T22:14:19Z

tests/test_zeus_client.py

@@ -68,23 +81,22 @@ def test_validate_dates(self):
    @mock.patch('zeus.interfaces.rest.requests')
    def test_post_empty_log(self, mock_requests):
        logs = []
+        url = '{}/logs/{}/ZeusTest'.format(FAKE_SERVER, FAKE_TOKEN)


use urljoin

I'm not planning to refactor using urljoin. because

urlparse.urljoin('/abc/', 'def') # => /abc/def urlparse.urljoin('/abc', 'def') # => '/def' urlparse.urljoin('abc', 'def') # => 'def' urlparse.urljoin('abc', '/def') # => '/def' urlparse.urljoin('/abc', '/def') # => '/def' urlparse.urljoin('/abc/', '/def') # => '/def'

So

# From url = '{}/logs/{}/ZeusTest'.format(FAKE_SERVER, FAKE_TOKEN) # To (not really tested though) url = urlparse.urljoin(FAKE_SERVER, '/logs/') url = urlparse.urljoin(url, FAKE_TOKEN + '/') url = urlparse.urljoin(url, ZeusTest)

looks bit redundant... (or am i missing something...?)

edit: nvm, looking into other solutions

Ok this one might be more appropriate:

>>> import posixpath >>> urlparse.urljoin('http://www.fake.com', posixpath.join('logs', 'token', 'test')) 'http://www.fake.com/logs/token/test' >>> urlparse.urljoin('http://www.fake.com', posixpath.join('/logs/', 'token/', 'test/')) 'http://www.fake.com/logs/token/test/' >>> urlparse.urljoin('http://www.fake.com', posixpath.join('/logs/', 'token', 'test/')) 'http://www.fake.com/logs/token/test/' >>> urlparse.urljoin('http://www.fake.com', posixpath.join('/logs', 'token', 'test')) 'http://www.fake.com/logs/token/test' >>> urlparse.urljoin('http://www.fake.com/', posixpath.join('/logs', 'token', 'test')) 'http://www.fake.com/logs/token/test'

posixpath.join('a/', '/b', 'c') # => '/b/c' posixpath.join('a', '/b/', '/c') # => '/c' posixpath.join('a/', '/b') # => '/b'

If there are no strong preference, i think format() might be good enough? how do you think

format has the same problems and also adds the problem with the base url, and with urls ending with backslash, it's way more brittle than urljoin

We need to know the behavior of urlparse.urljoin or posixpath.join functions, isn't it just adding complexity (and it does not fix the slash problem). We could not expect the behavior when i wrote https://github.com/CiscoZeus/zeus_api/pull/125 for example.

# easy to debug url = '{}/logs/{}/ZeusTest'.format(FAKE_SERVER, FAKE_TOKEN) urlparse.urljoin('http://www.fake.com/', posixpath.join('logs', '/token/', '/test')) # => what will be (without executing this code).

In addition, I've just googled python client github, figuring out most of OSS does just + or format.

google-cloud-python

kubernetes-incubator

(these are just random, not 100% sure if we investigate more github repos.)

if you still think it's better to use urljoin, I'm okay to change. so LMK.

path join functions usually treat paths starting with a forward slash as coming from the root path, so if you do join with something starting with a slash it's as if it started from the root again.

I have run into problems before using +, especially when it depends on url given by a user

I still can not get the point (either way we need to validate user input such as the following example). Meanwhile figured out:

urlparse.urljoin('localhost', posixpath.join('logs', 'token', 'test')) # => 'logs/token/test' urlparse.urljoin('localhost:8000', posixpath.join('logs', 'token', 'test')) # => 'logs/token/test' urlparse.urljoin('localhost:8000/', posixpath.join('logs', 'token', 'test')) # => 'logs/token/test'

(user needs to set http, or https or we need to check)

Don't you think just simply do formatting good enough?

url = '{}/logs/{}/test'.format(host, token) # => 'localhost/logs/token/test' # => 'localhost:8000/logs/token/test' # => 'http://localhost:8000/logs/token/test'

if user doesn't set http or https, requests library will fail anyway...

i;m only saying this because i had bug in the past that were caused by the client adding an extra / at the end of the url:

http://localhost:8000 works
http://localhost:8000/ doesn't work

The error message that arises from that is non obvious about the forward slash and will take time to track when it happens, i'm very wary of handling urls or paths manually because of that.

victorfonsec4 · 2017-05-31T22:26:04Z

It's a interface in the design pattern sense not in the java sense. Maybe a better name is appropriate.

I agree serializing would be better in one place

travelist · 2017-05-31T22:37:13Z

Ideally and personally, I want to make all interface (mix-in) classes into just functions and put these functions to the method of the Client class dynamically. (the same as the example code above.)

Hence mix-in feature or multiple inheritance in our client code seems just adding complexity without much benefit?? (I'm missing something..??) Meanwhile, problem is, if these interface define the same self.something, it's overwritten implicitly but we want to write self.base_url = '/metrics or /alerts in each interface for example.

also, we can remove RestClient, and ZeusClient should take over the responsibility to serialize, deserialize, request and error handling (actually this is a bit depends on though).

(anyway, not really plan to do refactor because of time constraint.... anyone's help is very appreciated..)

BTW, we also need to update Java client... so if there are some good strategy, please LMK (i will search for it by myself as well... but i have almost 0 experience with Java. any suggestions are helpful).

victorfonsec4 · 2017-05-31T23:44:51Z

Yeah the inheritance is bad, the thing about using bare methods is that we would be passing repeated information around like the token. Maybe make the interfaces as objects of the ZeusClient and then use functions that redirect the calls to each interface?

Not sure about putting the RestClient inside ZeusClient since each interface depends on it and ZeusClient depends on the interfaces

travelist · 2017-06-01T00:08:51Z

In my opinion, RestClient is redundant unless it become super class of (current) interface classes.

Personally ZeusClient should implement serialize/deserialise...etc as private methods by itself, then it gives more readability.

. Maybe make the interfaces as objects of the ZeusClient and then use functions that redirect the calls to each interface?

I guess you are thinking dependency-injection pattern right? not sure yet whether it's beneficial or not because our interface is not optional, not plan to be replaced by alternative (ZeusClient always has to have [Metrics|Log|...]Interface).

Meanwhile what i'm saying is:

From this example: https://stackoverflow.com/a/17930262/4126250

class A(object):
    pass

## NOTE: cls argument will be class A object, like self.
def func(cls):
    print 'I am a class method'
    # instead of self.something, we can write cls.something to access object value

A.func = classmethod(func)

the_name = 'other_func' 
setattr(A, the_name, classmethod(func))

Real example is like bellow:

gmaps = googlemaps.Client(key='Add Your Key here')

# Not necessary to pass 'key' or token to methods.
geocode_result = gmaps.geocode('1600 Amphitheatre Parkway, Mountain View, CA')

victorfonsec4 · 2017-06-01T00:26:03Z

Ah okay, its wrapping the functions from the interfaces and passing the client, with the rest functions, as an argument. Yeah that makes sense

coveralls · 2017-06-01T18:59:10Z

Coverage decreased (-1.7%) to 93.659% when pulling 82a40d8 on add-user-token-feature into 8c3b774 on develop.

travelist · 2017-06-01T19:16:12Z

removed test with python2.6.

tokwii · 2017-06-03T19:53:25Z

@travelist @victorfonsec4 Can I use this test Ingestion through the API?

travelist · 2017-06-03T19:56:14Z

@tokwii yes! no problem!

coveralls · 2017-06-04T01:09:18Z

Coverage decreased (-12.6%) to 82.759% when pulling e2fdf2f on add-user-token-feature into 8c3b774 on develop.

coveralls · 2017-06-04T01:13:12Z

Coverage decreased (-12.6%) to 82.759% when pulling 618aa19 on add-user-token-feature into 8c3b774 on develop.

travelist · 2017-06-04T03:14:50Z

reviewable. not planning to refactor all of the part of the code. Hope code is readable enough.

travelist · 2017-06-05T21:59:38Z

@victorfonsec4 Thanks for the review!:)

Add User token to HTTP Header

4b98bcf

travelist force-pushed the add-user-token-feature branch 2 times, most recently from ff6b92c to 684d7f7 Compare May 30, 2017 03:52

travelist added the wip label May 31, 2017

travelist force-pushed the add-user-token-feature branch 4 times, most recently from 9477804 to e2373f0 Compare May 31, 2017 19:55

travelist removed the wip label May 31, 2017

travelist requested a review from kosukemori May 31, 2017 21:26

kosukemori reviewed May 31, 2017

View reviewed changes

victorfonsec4 reviewed May 31, 2017

View reviewed changes

travelist requested a review from jjanicechen May 31, 2017 22:52

Update test code for Bucket-Name Header

82a40d8

travelist force-pushed the add-user-token-feature branch from e2373f0 to 82a40d8 Compare June 1, 2017 18:57

Update for how to build url using urlparse and posixpath

12ffe70

travelist force-pushed the add-user-token-feature branch from 6417b5e to e2fdf2f Compare June 4, 2017 01:08

Remove mixed-in interfaces

618aa19

travelist force-pushed the add-user-token-feature branch from e2fdf2f to 618aa19 Compare June 4, 2017 01:11

victorfonsec4 approved these changes Jun 5, 2017

View reviewed changes

travelist merged commit c1e3416 into develop Jun 5, 2017

travelist deleted the add-user-token-feature branch June 5, 2017 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add User token to HTTP Header #31

Add User token to HTTP Header #31

travelist commented May 29, 2017 •

edited

travelist commented May 31, 2017

kosukemori left a comment

kosukemori May 31, 2017 •

edited

travelist May 31, 2017 •

edited

travelist commented May 31, 2017 •

edited

victorfonsec4 commented May 31, 2017

victorfonsec4 May 31, 2017

travelist May 31, 2017 •

edited

victorfonsec4 May 31, 2017 •

edited

victorfonsec4 May 31, 2017

travelist Jun 1, 2017 •

edited

victorfonsec4 Jun 1, 2017 •

edited

travelist Jun 1, 2017 •

edited

victorfonsec4 Jun 1, 2017

travelist Jun 2, 2017 •

edited

victorfonsec4 Jun 2, 2017

victorfonsec4 commented May 31, 2017

travelist commented May 31, 2017 •

edited

victorfonsec4 commented May 31, 2017

travelist commented Jun 1, 2017 •

edited

victorfonsec4 commented Jun 1, 2017

coveralls commented Jun 1, 2017

travelist commented Jun 1, 2017

tokwii commented Jun 3, 2017

travelist commented Jun 3, 2017

coveralls commented Jun 4, 2017

coveralls commented Jun 4, 2017

travelist commented Jun 4, 2017 •

edited

travelist commented Jun 5, 2017

Add User token to HTTP Header #31

Add User token to HTTP Header #31

Conversation

travelist commented May 29, 2017 • edited

travelist commented May 31, 2017

kosukemori left a comment

Choose a reason for hiding this comment

kosukemori May 31, 2017 • edited

Choose a reason for hiding this comment

travelist May 31, 2017 • edited

Choose a reason for hiding this comment

travelist commented May 31, 2017 • edited

victorfonsec4 commented May 31, 2017

victorfonsec4 May 31, 2017

Choose a reason for hiding this comment

travelist May 31, 2017 • edited

Choose a reason for hiding this comment

victorfonsec4 May 31, 2017 • edited

Choose a reason for hiding this comment

victorfonsec4 May 31, 2017

Choose a reason for hiding this comment

travelist Jun 1, 2017 • edited

Choose a reason for hiding this comment

victorfonsec4 Jun 1, 2017 • edited

Choose a reason for hiding this comment

travelist Jun 1, 2017 • edited

Choose a reason for hiding this comment

victorfonsec4 Jun 1, 2017

Choose a reason for hiding this comment

travelist Jun 2, 2017 • edited

Choose a reason for hiding this comment

victorfonsec4 Jun 2, 2017

Choose a reason for hiding this comment

victorfonsec4 commented May 31, 2017

travelist commented May 31, 2017 • edited

victorfonsec4 commented May 31, 2017

travelist commented Jun 1, 2017 • edited

victorfonsec4 commented Jun 1, 2017

coveralls commented Jun 1, 2017

travelist commented Jun 1, 2017

tokwii commented Jun 3, 2017

travelist commented Jun 3, 2017

coveralls commented Jun 4, 2017

coveralls commented Jun 4, 2017

travelist commented Jun 4, 2017 • edited

travelist commented Jun 5, 2017

travelist commented May 29, 2017 •

edited

kosukemori May 31, 2017 •

edited

travelist May 31, 2017 •

edited

travelist commented May 31, 2017 •

edited

travelist May 31, 2017 •

edited

victorfonsec4 May 31, 2017 •

edited

travelist Jun 1, 2017 •

edited

victorfonsec4 Jun 1, 2017 •

edited

travelist Jun 1, 2017 •

edited

travelist Jun 2, 2017 •

edited

travelist commented May 31, 2017 •

edited

travelist commented Jun 1, 2017 •

edited

travelist commented Jun 4, 2017 •

edited