facebookarchive / swift

An annotation-based Java library for creating Thrift serializable types and services.
Apache License 2.0
900 stars 297 forks source link

Want method to specific service methods via interfaces instead of classes #239

Closed allComputableThings closed 9 years ago

allComputableThings commented 9 years ago

Sometimes a single service object may implement multiple service APIs for different consumers (e.g. web client vs. internal client).

@ThriftService
public interface MyWebClientService {
    @ThriftMethod
    public User getUser(User id)

    @ThriftMethod
    public LoginToken login(String user, String password)
}

@ThriftService
public interface MyInternalService {
    @ThriftMethod
    public User getUser(User id)

    @ThriftMethod
    public SecureData getSecureData()
}

public class MyServiceImpl implements MyInternalService, MyUserService {
    public User getUser(User id) { .... }
    public LoginToken login(String user, String password) { ... }
    public User getUser(User id) { ... }
    public SecureData getSecureData() { ... }
}

Would like to export different API views of MyServiceImpl implements internally and to web clients.

Is this already possible?

dain commented 9 years ago

How do you know if a client is internal or "web"? I can think of two ways, different ports, which should be possible, but will require manual configuration of the system (i.e., not guice). The other way is with a client identifier in a header or some parameter, and in this case you will just need some if statements in your code.

allComputableThings commented 9 years ago

No if-statements are required. Its totally up to whichever code takes the TProcessor and turns it into a service. I already have code that works like this:

TProcessor clientProcessor = makeClientProcessor();
TProcessor internalProcessor = makeInternalProcessor();

startWebSeverThread( 8080, clientProcessor, "/thrift/",  ... other params );
startThriftTCPSeverThread( 9080, clientProcessor );

So the services are securely separated by ports. (Security it provided by a firewall, load balancers and cloud security groups).

Additionally, we can also provide secure access as follows:

startWebSeverThread( 8080, clientProcessor, "/thrift/",
                                      internalProcessor, "/thriftinternal/", 
                                      ... other params );

Here our web server checks for security tokens in the HTTP request before passing the request to the server. In fact our current internal services work like this (as REST APIs). I'd like to replace them with a thrift APIs (in order to automatically force keeping our client code in sync with the available server API)

We don't use guice.

allComputableThings commented 9 years ago

Is the problems here technical or ideological?

I don't mind working on this (if there aren't large technical barriers).

dain commented 9 years ago

@andrewcox did you mean to close this issue?

dain commented 9 years ago

@stuz5000 did you try extracting an interface and it didn't work? I would guess the code walks the super classes and interfaces looking for @ThriftMethod annotated methods. If it doesn't, it should be a quick fix to add.

andrewcox commented 9 years ago

@dain sorry re-opening. I closed because I think your answer is adequate (use two ports, or use if statements). It seems @stuz5000 is looking for two ports, and this is totally doable. So, @stuz5000 is the work we are talking about adding guice support for this or what?

allComputableThings commented 9 years ago

We don't use guice. Broadly, what I'm trying to do is take a Scala&Java interfaces that are published variously as available as REST and JSONRPC (both TCP and HTTP) network points, and additionally export them as Thrift APIs in order to force client code to be in sync with the Java interface (or force a compile error if not). We define our APIs as a set of (name, Class, T) triples:

internalService =  [ (serviceName, serviceInterface, serviceObject),
                               (...), .... ]
clientService =  [ (serviceNameX, serviceInterfaceY, serviceObjectZ),
                               (...), .... ]

... and publish them as REST, JSONRPC, and (ideally) as Thrift interfaces.

I can't do this with ThriftServiceProcessor -- it only accepts a service object, not a service object and an interface Class.

With some modifications, I've modified ThriftServiceProcessor to take an interface and pass it to ThriftServiceMetadata, builds the interface -- works. My frankenclass ignores the Thrift annotations, so I not sure it makes sense to push it to the main repository. Perhaps a better workaround is it to have ThriftServiceProcessor take ThriftServiceMetadata as a constructor parameter.

allComputableThings commented 9 years ago

Just out of interest -- what's the benefit of ThriftServiceProcessor dealing with futures internally? (I understand the benefit of futures on the client side, but not the server where the request is typically all processed within a single thread.)

dain commented 9 years ago

So I just looked at the code for ThriftServiceProcessor. It looks like you can implement an annotated thrift service interface and Swift will work exactly as if you had placed the annotations directly on the implementing class. So just make sure that serviceObject and serviceObjectz implement serviceInterface. Is there something else you need? If so, can you write a full example (in Java) that shows what you want?

As for why may want to have futures on the server, some servers are doing async work and don't what to hold a thread. For example, a thrift server may call another thrift server using an async client. If so, you don't have to hold on to the service thread while waiting for the second server to finish the request.

allComputableThings commented 9 years ago

Because ThriftServiceProcessor accepts only objects, I can't create two services -- one with an InternalClientAPI and one with a WebClientAPI, from a single service object.

@ThriftService
public interface WebClientAPI{
   @ThriftMethod
   public Info getInfo();
}

@ThriftService
public interface InternalClientAPI
{
   @ThriftMethod
   publicvoid secretMethod();
}

public class ServiceImpl implement InternalClientAPI, WebClientAPI{
  public void secretMethod() {...}
  public Info getInfo();
}

serviceObject = new ServiceImpl();
TProcessor internalTProcessor = createService(serviceObject, InternalClientAPI.class);
TProcessor webTProcessor = createService(serviceObject, WebClientAPI.class);

ThriftServiceProcessor take no Class interface in its constructor -- only the full concrete Objects.

I almost have a workaround here: https://github.com/stuz5000/swift

Note it also makes @ThriftService and @ThriftMethod annotations optional (with a flag passed to Thrift2Swift). Because we can define an interface independently of the service object, the interface itself is usually sufficient to specify the service (these annotations are redundant and burdensome if working with an existing project with many RPC APIs specified as java interfaces). Adding @ThriftService, @ThriftMethod annotations gives the original behavior and allows customization.

dain commented 9 years ago

This doesn't seem like a great idea. Why not just have two service objects, one that implements WebClientAPI and one that implements InternalClientAPI? If they need shared state, just put that into a third instance and share it with the internal and web client.

If you really, really, really one one instance, just do something like this:

serviceObject = new ServiceImpl();
TProcessor internalTProcessor = createService(serviceObject, InternalClientAPI.class);
TProcessor webTProcessor = createService(new WebClientAPI() {
    public Info getInfo() { serviceObject.getInfo() };
});

The compiler will enforce the anonymous inner class has all of the delegates, and with self respecting IDE it should be trivial to keep the code synchronized.

Anyway, I don't see us adding a feature that allows a client to only export some methods from an instance. We are really more focused on making it easy to compose services that are split into logical chunks with a different objects for each one. This is in the other direction.