Skip to content

misc updates #95

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

misc updates #95

wants to merge 6 commits into from

Conversation

wonderbeyond
Copy link

@wonderbeyond wonderbeyond commented Jul 11, 2018

Some misc updates when I apply json-rpc to latest two projects. Just for your review.

  • Support custom json encoder for serializing response data
  • Support registering global decorators for jsonrpc methods
  • Support custom method prefix for Dispatcher.add_class
  • Support explicitly exporting subset of class methods with Dispatcher.add_class
  • Support using Dispatcher.add_method as a decorator generator
$ tox -e py27,py35,py36 
  py27: commands succeeded
  py35: commands succeeded
  py36: commands succeeded
  congratulations :)

Some use cases from my real projects

from awesome.json import JSONEncoder
jsonrpc_api = JSONRPCAPI(json_encoder=JSONEncoder)
from awesome.auth.decorators import login_required
dispatcher.register_decorator(login_required)
@dispatcher.add_class
class CellAPI(APISet):
    rpc_exports = ['detail', 'create', 'update']
    rpc_method_prefix = 'Cell'
    ### ...

- Support custom json encoder for serializing response data
- Support registering global decorators for jsonrpc methods
- Support custom method prefix for Dispatcher.add_class
- Support explicitly exporting subset of class methods with Dispatcher.add_class
Below is the code to reveal this bug:

    @dispatcher.add_class
    class ProcessOrderAPI(APISet):
	rpc_exports = ['query', 'detail', 'start', 'update']
	rpc_method_prefix = 'ProcessOrder'
	Model = ProcessOrder

	def update(self, object, attrs):
	    # NOTE HERE: Since add_class retuns None previously,
	    # So the 1st argument of super() is None
	    return super(ProcessOrderAPI, self).update(object, attrs)
@pavlov99
Copy link
Owner

Hi, @wonderbeyond

Thank you for the PR and sorry for coming back late.

After closer inspection of your code, I've noticed that there are quite many features there. Some of them I completely support, others - not.

Using custom json serializer is convenient, I'm on the same page.

Three other features I'd like to discuss:

  1. using decorators such as login_required is convenient, however, they could be added outside of dispatcher:
@dispatcher.add_method
@login_required
def my_handler():
    pass
  1. rpc_method_prefix already exists in method registration:
@dispatcher.add_method(prefix='Cell')
def my_method():
    pass

There is no support for prefix in case of class yet but this would be a proper place for prefix handler:

@dispatcher.add_class(prefix='Cell')
class CellAPI:
    pass
  1. Could you please elaborate on your use case with rpc_exports? At the moment all of the class methods (except methods starting with '_') are added to dispatcher.

@pavlov99
Copy link
Owner

One more thing: dispatcher should not know about any hooks. They could be implemented in manager.

@wonderbeyond
Copy link
Author

using decorators such as login_required is convenient, however, they could be added outside of dispatcher:

However I want a way for global decorator for all rpc handlers.

rpc_method_prefix already exists in method registration:

I see no prefix parameter of Dispatcher.add_method: https://github.com/pavlov99/json-rpc/blob/master/jsonrpc/dispatcher.py#L67

There is no support for prefix in case of class yet but this would be a proper place for prefix handler:

Yes, I think your style is better.

Could you please elaborate on your use case with rpc_exports? At the moment all of the class methods (except methods starting with '_') are added to dispatcher.

In my practice, I define a base class naming APISet:

image

This class acts as a template, provides common CURD methods, which a subclass has no need to override in general. Give the the right Model attr, and it just works.

image

For security considerations, I only export detail and list (i.e. readonly methods) as default. The subclass can make specific choices. It can also override or define new methods and make them exportable.

@wonderbeyond
Copy link
Author

@pavlov99

One more thing: dispatcher should not know about any hooks. They could be implemented in manager.

I've tried, but I found JSONRPCResponseManager was intended to be a static class, which only has classmethods.

Code sample from README:

@Request.application
def application(request):
    # Dispatcher is dictionary {<method_name>: callable}
    dispatcher["echo"] = lambda s: s
    dispatcher["add"] = lambda a, b: a + b

    response = JSONRPCResponseManager.handle(
        request.data, dispatcher)
    return Response(response.json, mimetype='application/json')

I found no elegant way to bind extra data to the manager. Can we make some change?

@S1983tt
Copy link

S1983tt commented Oct 20, 2022

4c13c48

1 similar comment
@S1983tt
Copy link

S1983tt commented Oct 20, 2022

4c13c48

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

Successfully merging this pull request may close these issues.

3 participants