Improve JSON-less 404 error readability and prevention with client library

[ianballou]

This code aims to stop the Unexpected error: No JSON object could be decoded error mentioned in #910:

• The error is caused by 404 pages without JSON hitting json.loads().
• If json.loads() fails, print out the response content rather than throwing the error above.
• Since this isn't pretty, the following target 404's:
  • Characters that get passed to the API will be limited to numbers, letters, and $ - _ . + ! * ' ( ) ,
  • The client library checks object names per-call for characters that don't match this.
• If the client library detects bad characters, it will print something like: Error: Projects may not contain: ['/', '&'] to alert the user of the illegal characters that they entered.

I anticipate that the tests will take a while to write since I am introducing changes to the majority of the client library calls. Thus, I'm introducing this code now so I can get feedback before writing the tests.

Note: some commands, such as switch_register, have not been implemented in the client library yet. These will get checks for reserved characters when they are implemented.

[zenhack]

A few comments.

[zenhack]

We could probably do a better job of factoring this out. maybe:

check_reserved('Owner', owner)

with a suitable definition of check_reserved.

We could go a bit further and implement a decorator so we could do something like:

@check_reserved_chars('network', 'owner', 'access', slashes_ok=['net_id'])
def create(self, network, owner, access, net_id):
    # Implementation unchanged from master.

It might also make sense to survey the arguments and see what the proportion of arguments to be checked vs. not checked; depending, we may want to specify arguments not to check, rather than the other way around.

[ianballou]

I like the decorator solution, I'm going to work on that.

[naved001]

@Izhmash let me know when you do the decorator solution, I'll review this after that.

[ianballou]

@zenhack how would you relate the arguments in create() to the strings in the decorator? Since the arguments are passed as *args the only way I can see to relate them would be by their order which might not be robust enough. It would work out if the arguments were changed to be keyworded but that would mean editing the original function.

Edit: In case the decorator solution does start getting too complicated, I think it might be better to clean up the calls with a new definition of check_reserved() like you mentioned above.

[xuhang57]

I am not sure whether this is right but: what you need is a function that checking the error, for example

def decorator(argument):
    def real_decorator(function):
        def wrapper(*args, **kwargs):
            funny_stuff()
            something_with_argument(argument)
            function(*args, **kwargs)
            more_funny_stuff()
        return wrapper
    return real_decorator

https://stackoverflow.com/questions/5929107/decorators-with-parameters

[xuhang57]

And

@check_reserved_chars(arguments)
class Network(ClientBase):

should work so you don't have to put the decorator in every function.

[ianballou]

@xuhang57 thanks for the input. The code you presented above definitely fits what I had in mind. The issue lies with relating the non-keyworded arguments in the decorator to the arguments in the methods such as create() so we can choose what gets checked.

The only way that I could see to relate (for example) the string 'network' in the decorator to the network passed to the method would be by location since network names have no standard in HIL. That, then, becomes complicated when considering that some of the objects will allow slashes with the slashes_ok flag above.

In the meantime I'm going to work on factoring out my original code and present a solution without decorators.

[zenhack]

The inspect module may be of interest:

https://docs.python.org/2/library/inspect.html

from there, you can do something like:

argspec = inspect.getargspec(f)
def wrapper(*args):
    for argname, argval in zip(argspec.args, args):
        if argname not in slashes_ok:
            check(argval)

Maybe adding some similar logic for kwargs.

[zenhack]

1. I don't think most of these need escaping when inside a character class. The dash can go at the end of the list to avoid it being interpreted as special. Escaping the single quote can be avoided by using double quotes to delmit the string as a whole.
2. Generally speaking, use r"raw strings" for regexes, so you don't have to worry about what needs to be escaped in a python string as well.

[ianballou]

Ok, I'll work on that.

[ianballou]

Thanks for the tip, c1fea67 addresses this.

[ianballou]

Just wanted to note that I'm back working for the new semester so I'll have activity on here soon.

[ianballou]

My latest commit cleans up the methods for checking for reserved characters.

[naved001]

you could just do a single return after the if-else block

[ianballou]

Fixed

[naved001]

existence

[ianballou]

Fixed

[ianballou]

The tests for this are in progress now.

[ianballou]

I've setup decorators/wrappers to check the illegal characters now, so the client functions should remain otherwise unedited.

[ianballou]

I've addressed the requests and added a first pass at the tests. Did I address your requests correctly @zenhack and @naved001 ?

[xuhang57]

LGTM

[zenhack]

It seems like the vast majority of our parameters need checking; maybe instead of listing the ones to check in each call to the decorator, we should list the ones not to check?

so:

@check_reserved_chars(dont_check=['foo'], slashes_ok=['bar'])
...

[zenhack]

This if/else can just be outer_kwargs.get('slashes_ok', [])

[zenhack]

Took me a second to work out why we were chopping off the first element; probably should add a comment about that.

[zenhack]

Could simplify this to:

check_reserved(argname, argval, slashes_ok=argname in slashes_ok)

[zenhack]

You've got a mismatch between the name you're passing to the decorator and the actual name of the parameter. Similar thing in a bunch of places below.

[ianballou]

I figured there were some places where the variable names wouldn't want to get out. I put 'node' there because otherwise in the error reporting it would say 'node_name' to the user. Is that preferable?

[ianballou]

To settle this I'm just going to use argspec and match the variable names directly.

[naved001]

I don't have any other comments to make on this at this point, @zenhack has already made some good points.

[naved001]

well, except for this line. There's a lot going on here, would be nice if we split it to make it more readable. But I won't block the PR on this.

[ianballou]

I might be wrong, but I think this is one of those cases where the list comprehension is actually less complex than if I didn't use a one-liner. I can add a comment for clarification if that helps.

[naved001]

actually, don't worry about it.

…

On Tue, Feb 6, 2018 at 3:28 PM, Ian Ballou ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In hil/client/base.py <#926 (comment)>: > e = json.loads(response.content) raise FailedAPICallException( error_type=e['type'], message=e['msg'], ) + # Catching responses that do not return JSON + except ValueError: + return response.content + + +def _find_reserved(string, slashes_ok=False): + """Returns a list of illegal characters in a string""" + if slashes_ok: + p = r"[^A-Za-z0-9 /$_.+!*'(),-]+" + else: + p = r"[^A-Za-z0-9 $_.+!*'(),-]+" + return list(x for l in re.findall(p, string) for x in l) I might be wrong, but I think this is one of those cases where the list comprehension is actually less complex than if I didn't use a one-liner. I can add a comment for clarification if that helps. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#926 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATLmqB-yO1cFIPjvQmgM3ubT6papKBBCks5tSLXugaJpZM4RD9GS> .

[ianballou]

@zenhack I believe I've addressed all of your requests in these two commits

[zenhack]

Gripes about the one function, but this is almost ready I think.

[zenhack]

It took me >1 min to figure out how this worked (I think partially just because there was a lot of seeking involved in parsing the comprehension). What about:

1. Change the patterns to remove the +
2. list(set(re.findall(p, string)))

Also, I don't like the fact that we have two nearly identical regexes. One alternative:

p = # regex without the /
result = # expression with re.findall
if not slashes_ok and '/' in string:
    result.append('/')

[ianballou]

@zenhack my code now should reflect your comments. My logic is slightly different from what you recommended, but works just as it did before.

[zenhack]

This should probably not include the slash I think? Otherwise the slashes will end up in the result regardless of the value of slashes_ok. Would you also add a test that catches this?

[ianballou]

I thought that result shouldn't check slashes by default so I put one in my "allowed characters". Then, if slashes aren't allowed but there is one in the string, I need to add a slash to my list of unique illegal characters, result. My goal is for result to have all the characters that are in string but not in p.

My tests all failed when I did remove the slash from p, but when I put it back everything worked as it should. Tomorrow I'll double check that and write a test to check for a proper response.

[zenhack]

Ah, no you're totally right; I had things backwards in my head; forgot that the regex was being inverted, so would match allowed characters rather than banned characters.

[zenhack]

LGTM