google / rejoiner

Generates a unified GraphQL schema from gRPC microservices and other Protobuf sources
https://google.github.io/rejoiner/
Apache License 2.0
3.67k stars 139 forks source link

Usage from Kotlin #65

Open chrischandler25 opened 5 years ago

chrischandler25 commented 5 years ago

Hi.

Thanks for creating this great tool. I've been using it in a Kotlin application, which has been really simple and worked brilliantly for my requirement.

I have run into a little problem though. I am hoping to remove a field, from one of the GRPC types. I followed your documentation and examples, but think this may fall over in a difference between Kotlin and java. Kotlin by default creates a property getter when a read only var is declared. It looks to me as though you expect only field adding @SchemaModification annotations to be associated to methods, so it's reporting the following exception:

Jan 13, 2019 10:31:15 PM com.google.inject.internal.MessageProcessor visit INFO: An exception was caught and reported. Message: java.lang.NoSuchMethodException: com.google.api.graphql.rejoiner.SchemaModification.getDescriptor() java.lang.RuntimeException: java.lang.NoSuchMethodException: com.google.api.graphql.rejoiner.SchemaModification.getDescriptor() at com.google.api.graphql.rejoiner.SchemaModule.configure(SchemaModule.java:228)

It is possible to annotate the variable definition with @JvmField which will prevent the getter creation and use a Java field, but then the @SchemaModification annotation seems to no longer be applied to the field.

I appreciate that this is a problem of using rejoiner from a language other than the intended, but wondered if you might be able to suggest any different approach to remove the field?

Thanks again Chris

siderakis commented 5 years ago

Can you please add a code snippet that results in the error.

siderakis commented 5 years ago

BTW someone created an Kotlin example at https://github.com/dbaggett/medallion

chrischandler25 commented 5 years ago

The Kotlin example is useful to see, although unfortunately doesn't utilise the remove field functionality.

My first attempt was:

class AuthServiceSchemaModule : SchemaModule() {
    @SchemaModification
    val removeRegisterUserRemoteAddr = Type
        .find(RegisterUserRequest.getDescriptor())
        .removeField("remoteAddr")!!

This results in the error: java.lang.NoSuchMethodException: com.google.api.graphql.rejoiner.SchemaModification.getDescriptor() My Java is basic, but I think I can see why this is happening.

Line 228 in SchemaModule.java I think expects the only @SchemaModification annotations on a method to be for adding fields. As Kotlin creates a getter behind the scenes this is conflicting with the intentions of SchemaModule.

The decompiled Java code of this approach is:

public final class AuthServiceSchemaModule extends SchemaModule {
   @NotNull
   private final TypeModification removeRegisterUserRemoteAddr;

   /** @deprecated */
   // $FF: synthetic method
   @SchemaModification
   public static void removeRegisterUserRemoteAddr$annotations() {
   }

   @NotNull
   public final TypeModification getRemoveRegisterUserRemoteAddr() {
      return this.removeRegisterUserRemoteAddr;
   }

Another thing I tried was adding the @JvmField annotation, which tells the compiler to use a java field. The problem here is that it appears to have created a field, but rather than assign the value inline it has done it from a constructor:

public final class AuthServiceSchemaModule extends SchemaModule {
   @JvmField
   @NotNull
   public final TypeModification removeRegisterUserRemoteAddr;

   /** @deprecated */
   // $FF: synthetic method
   @SchemaModification
   public static void removeRegisterUserRemoteAddr$annotations() {
   }

   public AuthServiceSchemaModule() {
      TypeModification var10001 = Type.find((GenericDescriptor)RegisterUserRequest.getDescriptor()).removeField("remoteAddr");
      if (var10001 == null) {
         Intrinsics.throwNpe();
      }

      this.removeRegisterUserRemoteAddr = var10001;
   }

This approach results in the same error. Any ideas would be greatly appreciated.

Regards Chris

chrischandler25 commented 5 years ago

I've got a little closer to a solution with this. It turns out that by prefixing the annotation with @field the compiler will ensure that the annotation is attached to the java backing field.

The generated code now looks more sensible, as follows:

public final class AuthServiceSchemaModule extends SchemaModule {
   @SchemaModification
   @JvmField
   @NotNull
   public final TypeModification removeRegisterUserRemoteAddr;

   public AuthServiceSchemaModule() {
      TypeModification var10001 = Type.find((GenericDescriptor)RegisterUserRequest.getDescriptor()).removeField("remoteAddr");
      if (var10001 == null) {
         Intrinsics.throwNpe();
      }

      this.removeRegisterUserRemoteAddr = var10001;
   }
}

This still doesn't seem to have any effect however. I wonder if I'm misunderstanding the concept of removing fields.

I have a field for the remote users IP address in my GRPC service, which I want to populate from the servlet request rather than have supplied by the GraphQL called. My aim is to remove this field from the GraphQL schema.

I am on the wrong track here?

Chris

siderakis commented 5 years ago

Does removeRegisterUserRemoteAddr have to be assigned a value in the constructor? I need to check, but maybe something runs in the SchemaModule constructor before it’s assigned.

Sent from my iPhone

On Jan 14, 2019, at 4:22 PM, chrischandler25 notifications@github.com wrote:

removeRegisterUserRemoteAddr

chrischandler25 commented 5 years ago

I can't find any way to have the value assigned inline, but it was suggested to me that the bytecode is likely the same. They seemed to think that in Java when values are assigned inline a default constructor is created by the compiler for the assignment.

siderakis commented 5 years ago

Do you know why it's casting to GenericDescriptor? I wonder if you could look at the fields of TypeModification if you set a breakpoint.

As a temporary workaround could you include a Java SchemaModule that includes removeRegisterUserRemoteAddr?

siderakis commented 5 years ago

Ohh, can you try Type.find("RegisterUserRequest")....

chrischandler25 commented 5 years ago

I'm not sure about the cast, but don't think it would make any difference.

I had tried using a string for the type name in addition to the getDescriptor approach with no noticeable difference. They both result in a RemoveFieldByName object with the values:

fieldNameToRemove = "remoteAddr"
typeName = "RegisterUserRequest"

I've also tried using the following Java code:

public class AuthServiceSchemaExtModule extends SchemaModule {
    @SchemaModification
    TypeModification removeRegisterUserRemoteAddr = Type
            .find("RegisterUserRequest")
            .removeField("remoteAddr");

    @SchemaModification
    TypeModification removeAuthenticateUserRemoteAddr = Type
            .find(AuthenticateUserRequest.getDescriptor())
            .removeField("remoteAddr");
}

But even that appears to have no effect.

I tried to follow your schema provider test -https://github.com/google/rejoiner/blob/7f18f8c6056cdb2f2cf54237b17856f08c794df4/rejoiner/src/test/java/com/google/api/graphql/rejoiner/SchemaProviderModuleTest.java I'm a little confused though as even the comment suggests it may not work?

// TODO: this should be empty, currently type modifications only apply to types // annotated with ExtraTypes.

chrischandler25 commented 5 years ago

I've been looking at this a little further as I'd love to be able to get it working.

Stepping through the modifyTypes method of the ProtoRegistry.java class, I notice that for my two defined type RegisterUserRequest and RegisterUserResponse, 2 additional mapping types have been created, Input_RegisterUserRequest, and Input_RegisterUserResponse.

The field removals are actually applied to my defined types (without the Input_ prefix), however the line

if (mapping.get(key) instanceof GraphQLObjectType) {

skips the modification code for each of the Input types. It seems that the Input types are the ones exposed in the schema (for input values), so it makes some sense that the expected fields aren't being removed. Should an Input type be produced for the return types as it seems to be (although this isn't causing any issue)? I'd probably have to put in a fair amount of effort to understand why the Input types are generated, but was hoping you may be able to point me in the right direction?

Regards Chris