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

Add exposedmethod decorator and documentation (Resolves #292) #307

Closed
wants to merge 1 commit into from
Closed

Conversation

comrumino
Copy link
Collaborator

No description provided.

@coldfix
Copy link
Contributor

coldfix commented Nov 2, 2018

Hi, I don't think patching Server is the correct approach because services are not only exposed by servers, and because this means that you modify the class later than it was constructed, potentially many times. I would prefer to either

  • provide a class decorator that patches the class directly
  • use a metaclass for Service

(even if both of these methods are not directly compatible with a runtime-dependent exposed_prefix)

@comrumino
Copy link
Collaborator Author

Thanks for the feedback. I managed to keep support for the runtime-dependent exposed_prefix 😃.

@coldfix
Copy link
Contributor

coldfix commented Nov 8, 2018

Hey, I haven't forgotten about this PR, I will take some time to review. Another point is that I would prefer to have policy proxies (*) in the long run, if possible compatible with the notation introduced in this PR. Maybe the class decorator should be renamed to something like rpyc.with_policy. Thinking further, the policy wrapper object won't even need a class decorator and look much cleaner from a user perspective. I'll play around with policy wrappers and get back to you in a week or so.

(*) My basic idea is that rpyc.Connection won't access the "real" object directly anymore, but will interact with it through a proxy that basically does all the handle_XXX stuff in the rpyc protocol, i.e. getattr/setattr/delattr/operators/... This proxy would be free to enforce arbitrary custom access policies on a very granular level, i.e. you can have different policies for different objects of the same type in the same connection, or even different views of the same object. There are two approaches that will have to be evaluated: a policy with handle_XXX methods versus seemless wrapper objects that behave exactly the same as the target by implementing the corresponding dunder methods while handle_XXX stays in Connection. (The former will typically need instanciating less proxies, while the latter allows to "cut the middle man" and access objects directly and makes it easier to have different policy views on the same object)

@comrumino
Copy link
Collaborator Author

Agreed, the name rpyc.service isn't great. Hiding the class decorator function would be a better API decision (but definitely requires some overhauling elsewhere to work). Maybe the destination of this PR should be a different branch? I'd be happy to change the destination if you would like.

@coldfix
Copy link
Contributor

coldfix commented Nov 8, 2018

Nevermind all the micro-comments, I think the solution should do exactly the same as suggested in:

#70 (comment)

With one exception: replace the delattr(cls, k) line with another setattr(cls, k, v.func).

rpyc/utils/__init__.py Show resolved Hide resolved
rpyc/utils/__init__.py Show resolved Hide resolved
rpyc/utils/__init__.py Outdated Show resolved Hide resolved
rpyc/utils/__init__.py Outdated Show resolved Hide resolved
rpyc/utils/__init__.py Outdated Show resolved Hide resolved
rpyc/utils/__init__.py Outdated Show resolved Hide resolved
rpyc/utils/__init__.py Outdated Show resolved Hide resolved
rpyc/utils/__init__.py Outdated Show resolved Hide resolved
@comrumino
Copy link
Collaborator Author

comrumino commented Nov 13, 2018

Nevermind all the micro-comments, I think the solution should do exactly the same as suggested in:

#70 (comment)

With one exception: replace the delattr(cls, k) line with another setattr(cls, k, v.func).

The current commit should behave exactly as desired, but it also supports exposed classes that have exposed methods and any non-standard exposed_prefix (which is why it's a bit longer). lmk, if you'd like the non-standard exposed_prefix functionality or not---I see no harm in keeping it

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.

2 participants