facebookarchive / swift

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

Service implementor cannot extend multiple ThriftServices interfaces. #170

Closed aching closed 10 years ago

aching commented 10 years ago

I have a class that implements two different interfaces that are both annotated as ThriftService objects. When passing an instance of this class to ThriftServiceProcessor, it will fail due to implementing multiple services. This is useful functionality, per se, to limit clients from having access to all the methods. Can we simply remove the check here?

Caused by: java.lang.IllegalStateException: service JobStatsStore extends multiple services at com.google.common.base.Preconditions.checkState(Preconditions.java:150) at com.facebook.swift.service.metadata.ThriftServiceMetadata.(ThriftServiceMetadata.java:87) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:75) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:63)

andrewcox commented 10 years ago

You can, you just need to annotate your final class with @ThriftService (it searches the tree for the proper annotation to find the name of the service from the annotation, and if you have two parent classes with @ThriftService annotations, you need to add the annotation on the derived class to resolve the conflict).

That said, swift also supports passing multiple objects when creating the processor, which is often a cleaner model than inheriting multiple service interfaces.

From: Avery Ching notifications@github.com<mailto:notifications@github.com> Reply-To: facebook/swift reply@reply.github.com<mailto:reply@reply.github.com> Date: Thursday, January 30, 2014 at 1:49 PM To: facebook/swift swift@noreply.github.com<mailto:swift@noreply.github.com> Subject: [swift] Service implementor cannot extend multiple ThriftServices interfaces. (#170)

I have a class that implements two different interfaces that are both annotated as ThriftService objects. When passing an instance of this class to ThriftServiceProcessor, it will fail due to implementing multiple services. This is useful functionality, per se, to limit clients from having access to all the methods. Can we simply remove the check here?

Caused by: java.lang.IllegalStateException: service JobStatsStore extends multiple services at com.google.common.base.Preconditions.checkState(Preconditions.java:150) at com.facebook.swift.service.metadata.ThriftServiceMetadata.(ThriftServiceMetadata.java:87) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:75) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:63)

— Reply to this email directly or view it on GitHubhttps://urldefense.proofpoint.com/v1/url?u=https://github.com/facebook/swift/issues/170&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=sJ61cr6u3yYT6%2FsOZjgMEw%3D%3D%0A&m=f1eCT5rbncGaIE7COUaKsmAkU8WqNiQtOR8zU7rsn3M%3D%0A&s=0687c789c10b4904e5241e91596f2c47a4b0314c88d6958ec942c49d584a0939.

aching commented 10 years ago

Thanks for your reply Andrew. If I am understanding you correctly, you are suggesting:

@ThriftService public interface ServiceA

@ThriftService public interface ServiceB

@ThriftService public class ServiceImpl implements ServiceA, ServiceB

ThriftServiceProcessor tsp = new ThriftServiceProcessor(...,..., new ServiceImpl());

This doesn't work either unfortunately.

Caused by: java.lang.IllegalStateException: service JobStatsStore extends multiple services at com.google.common.base.Preconditions.checkState(Preconditions.java:150) at com.facebook.swift.service.metadata.ThriftServiceMetadata.(ThriftServiceMetadata.java:87) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:75) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:63)

One reason to do this is to only expose a subset of the methods available on the service to one client and a subset to another client.

andrewcox commented 10 years ago

Ok, found the change that caused this. This used to work (I even remember fixing it) as long as you have @ThriftService annotation on your handler class.

The problem is that this only works if you’re concerned with swift and no other implementations of Thrift. When we added a generator that takes Swift classes and generates .thrift files, it could support this because thrift IDL does not support multiple inheritance, and some of this code for the generator crept into the path for actually running a service.

We could fix this, but anyway, if you write your service in swift, you’ll probably eventually want to produce .thrift files so other language clients can call you, and you won’t be able to do it if you have this kind of service hierarchy.

The right thing to do long-term would be to change thrift IDL to support this, and then lift the restriction from swift since it would be fine to have multiple parents.

Meanwhile, a workaround to get this going in swift is to implement each subset as a separate class, and pass an instance of each of these classes to the ThriftServiceProcessor constructor.

From: Avery Ching notifications@github.com<mailto:notifications@github.com> Reply-To: facebook/swift reply@reply.github.com<mailto:reply@reply.github.com> Date: Thursday, January 30, 2014 at 4:15 PM To: facebook/swift swift@noreply.github.com<mailto:swift@noreply.github.com> Cc: Andrew Cox andrewcox@fb.com<mailto:andrewcox@fb.com> Subject: Re: [swift] Service implementor cannot extend multiple ThriftServices interfaces. (#170)

Thanks for your reply Andrew. If I am understanding you correctly, you are suggesting:

@ThriftService public interface ServiceA

@ThriftService public interface ServiceB

@ThriftService public class ServiceImpl implements ServiceA, ServiceB

ThriftServiceProcessor tsp = new ThriftServiceProcessor(...,..., new ServiceImpl());

This doesn't work either unfortunately.

Caused by: java.lang.IllegalStateException: service JobStatsStore extends multiple services at com.google.common.base.Preconditions.checkState(Preconditions.java:150) at com.facebook.swift.service.metadata.ThriftServiceMetadata.(ThriftServiceMetadata.java:87) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:75) at com.facebook.swift.service.ThriftServiceProcessor.(ThriftServiceProcessor.java:63)

One reason to do this is to only expose a subset of the methods available on the service to one client and a subset to another client.

— Reply to this email directly or view it on GitHubhttps://urldefense.proofpoint.com/v1/url?u=https://github.com/facebook/swift/issues/170%23issuecomment-33749994&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=sJ61cr6u3yYT6%2FsOZjgMEw%3D%3D%0A&m=pPimib%2BpG7eKOc%2FenifacMYos07rTdHY%2B3KJFc16oVQ%3D%0A&s=0b283029ec2264769a6c1ba7494230783377b81760511a5721b765e7f9bc6d0f.

aching commented 10 years ago

Thanks for your response. I guess the workaround will have to do for now.

andrewcox commented 10 years ago

I've changed this so that you can do this (have a class that extends two @ThriftService interfaces), as long as the class itself has a @ThriftService annotation. You just won't be able to use swift2thrift to generate thrift IDL for that implementation class (however, you can still use swift2thrift to generate thrift IDL for the two interfaces the class extends).

vicmosin commented 9 years ago

Is there any way to extend generic interfaces?

@ThriftService
public interface BaseService<T> {

    @ThriftMethod
    T save(T object);
}

This one fails for example...

dain commented 9 years ago

To encode and decode in Thrift, we need to know the concrete type. IIRC Swift will walk the type hierarchy to resolve the base type, but it must resolve to a concrete type, which this example does not.

vicmosin commented 9 years ago

Do you mean in generally? Or only regarding my example? Take a look at my question please: http://stackoverflow.com/questions/30760377/use-generics-with-thriftservice

andrewcox commented 9 years ago

We don't support generic parameters on a service definition.