Skip to content
This repository has been archived by the owner on Jan 9, 2018. It is now read-only.

Design intent of Action, CallAction, CompleteCallAction #28

Open
trumpetinc opened this issue Feb 18, 2014 · 3 comments
Open

Design intent of Action, CallAction, CompleteCallAction #28

trumpetinc opened this issue Feb 18, 2014 · 3 comments
Labels
Milestone

Comments

@trumpetinc
Copy link
Collaborator

I had some thoughts on this part of the design:

I think that the ideal end-user case would be to use an annotation based system, backed by a class implementation that handles the most complex case.

So I'm thinking that we want the CompleteCallAction interface, but not support the CallAction (or Action) interface at all.

If someone wants to have something more like CallAction, then that should be possible to create by implementing a wrapper class that implements CompleteCallAction, but calls the action code however they need to.

I propose that this be the only interface supported by DefaultRPCManager:

public interface CompleteCallAction{
public T execute(WampSession session, WampCallMessage msg) throws Exception;
}

then, we can focus on an implementation of CompleteCallAction that uses annotations to do dynamic method invocation.

Or, if we don't want to do annotations just yet, we can do this::

public interface SimpleCallAction{
public T execute(Object ... params) throws Exception;
}

public class SimpleCallActionWrapper implements CompleteCallAction{
private final SimpleCallAction simple;

private SimpleCallActionWrapper(SimpleCallAction<T> simple){
          this.simple = simple;
    }

    public static <T> SimpleCallActionWrapper<T> wrap(SimpleCallAction<T> simple){
         return new SimpleCallActionWrapper<T>(simple);
    }

public T execute(WampSession session, WampCallMessage msg) throws Exception{
         return simple.execute(Utils.objectArrayForArguments(msg.getArguments()));
    }        

}

I think it would be better to add wrapper logic to the builder that is constructing the DefaultRPCManager than have if(action instanceof CallAction) in the DefaultRPCManager#onMessage() method.

@ghetolay
Copy link
Owner

First DefaultRPCManager should handle Action instead of CallAction on ActionMapping and it should be 2 addAction() method. One with CallAction argument and one with CompleteCallAction. I'll change that.

We could do what you suggest but what's the benefit of it ?
I don't think the "else if" economy on onMessage() covers the complexity of the wrapper creation and execution.

Also it's CallAction and CompleteCallAction instead of SimpleCallAction and CallAction cause I would rather name CallAction the one user will most use and I really think users will majoritarily use the simple CallAction system and both are essential.

@ghetolay ghetolay added this to the JSR356 milestone Feb 19, 2014
@ghetolay ghetolay removed the jsr356 label Feb 19, 2014
@ghetolay ghetolay modified the milestone: JSR356 Feb 19, 2014
@ghetolay
Copy link
Owner

CompleteCallAction is an awful name but I didn't come up with something better :)

@trumpetinc
Copy link
Collaborator Author

I think that the key here is to separate the API into layers, and use naming conventions that indicate the layer. I wrote a little bit about this here: #27

I guess what I'm saying is that there shouldn't be an Action interface at all. It doesn't provide value.

It is better to have DefaultRPCManager work with one type of action, then do the special handling somewhere else. Otherwise, DefaultRCPManager winds up trying to address multiple concerns, and becomes difficult to maintain.

I think a good approach here is to determine the minimum, most powerful interface that the RPCActionHandler (or whatever we call it) needs to have so that DefaultRPCManager can do it's job, then worry about providing simplified interface in a different level of the API.

It seems like the following is what we need:

RPCCallResult handleCall(String callId, WampCallMessage msg)

I'm not even sure that it should throw CallException (I'm pretty sure that it shouldn't).

This does push a lot of responsibility down into the RPCActionHandler, but that is OK at this level of the API. We can simplify at a higher level.

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

No branches or pull requests

2 participants