Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError in user code is hidden by library #25

Closed
deadbeef404 opened this issue Mar 18, 2015 · 8 comments
Closed

TypeError in user code is hidden by library #25

deadbeef404 opened this issue Mar 18, 2015 · 8 comments
Assignees

Comments

@deadbeef404
Copy link

Can I please draw your attention to jsonrpc/manager.py in __get_responses where parameters are validated for a method.
Due to the way that parameters are validated, a TypeError is raised when the method is given incorrect parameters, and thus, the following line is executed

output = response(error=JSONRPCInvalidParams()._data)

The exception handling around this doesn't account for the fact that a developer's code within that method can potentially throw a TypeError. In this case, the real issue is hidden and the following error is returned:

{u'jsonrpc': u'2.0', u'id': 0, u'error': {u'message': u'Invalid params', u'code': -32602}}

I've just lost some time investigating why I was getting this error, and it would be great to avoid inflicting that upon someone else.

@pavlov99
Copy link
Owner

@deadbeef404 sure, let me see. I agree, TypeError inside the method would throw 'Invalid params' exception.

@pavlov99 pavlov99 self-assigned this Mar 19, 2015
@pavlov99
Copy link
Owner

@deadbeef404 let us solve this issue from the business side.
Technically, it is impossible to distinguish TypeError because of invalid parameters and TypeError thrown from the function itself. Unfortunately, json-rpc could not do much about it, it does not know function it calls. What is possible to do:

  1. Catch TypeError from inside the function and throw JSONRPCDispatchException
    https://github.com/pavlov99/json-rpc/blob/master/jsonrpc/exceptions.py#L172
    From library side, need to document it better.
  2. I could add more information to error's data field, as implemented for general exceptions: https://github.com/pavlov99/json-rpc/blob/master/jsonrpc/manager.py#L109-114
    Would it solve your issue?
  3. Use *args, **kwargs for functions and accepts/returns decorators https://wiki.python.org/moin/PythonDecoratorLibrary#Type_Enforcement_.28accepts.2Freturns.29
    But this is the least desirable way.

Let me know what do you think about it.

@pavlov99
Copy link
Owner

I have implemented body message to TypeError (InvalidParams). https://github.com/pavlov99/json-rpc/blob/master/jsonrpc/manager.py#L106-110

It gives more information about the error: https://github.com/pavlov99/json-rpc/blob/master/jsonrpc/tests/test_manager.py#L82-91

Versionadded: 1.8.4

@deadbeef404
Copy link
Author

This change is in improvement as it would at lease preserve some information. There is the slight issue though: this is still is returning the wrong error code for the exception, and thus not matching the specification.

In regards to your point "it does not know what function it calls", well, it does. It has a reference to it to which it passes *request.args, **request.kwargs.

Taking this into account, the most ideal way would be to actually perform some type of argument matching by accessing method.func_code.co_varnames to inspect the call before it happens. Actually, this argument matching approach, if implemented, would best be run after the TypeError occurs, that way the code would not add the upfront overhead on every call, just those that have a TypeError already occurring.
Have you considered this approach?

@deadbeef404
Copy link
Author

For a better approach than the simple co_varnames, this seems useful: http://stackoverflow.com/a/197053/945856

@pavlov99
Copy link
Owner

@deadbeef404 point taken. I did not want to play with code inspection, but seems we could not avoid it. To follow specification is important and I agree with you that current error type is not correct in case of TypeError inside the function. Let me play with the code and add suggested functionality.

@pavlov99
Copy link
Owner

@deadbeef404 , updated code https://github.com/pavlov99/json-rpc/blob/master/jsonrpc/manager.py#L117-120
Version 1.9.0 has been released.

@deadbeef404
Copy link
Author

Thank you for your hard work! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants