Extra annotation for gRPC clients/stubs integration with spring context

[a-simeshin]

Context

Closes #463

There is a rather inconvenient problem described in https://github.com/yidongnan/grpc-spring-boot-starter/issues/463
@GrpcClient is not a @Bean, so we are encouraged to wrap it in a standard bean creation for quality of life.

@Configuration
public class MyCustomGrpcBeanConfigh {

    @GrpcClient("client")
    private MyStub myStub;

    @Bean
    MyExternalBean myExternalBean() {
        return new MyExternalBean(myStub);
    }

}

@Configuration
public class MyAnotherCustomGrpcBeanConfig {

    @Bean
    MyAnotherExternalBean myAnotherExternalBean(@Autiwired MyExternalBean myExternalBean) {
        return new MyAnotherExternalBean (myExternalBean.getStub());
    }

}

TO BE

@Configuration
public class MyCustomGrpcBeanConfigh {

    @GrpcClient("client")
    private MyStub myStub;

}

@Configuration
public class MyAnotherCustomGrpcBeanConfig {

    @Bean
    MyAnotherExternalBean myAnotherExternalBean(@Autiwired MyStub myStub) {
        return new MyAnotherExternalBean (myStub);
    }

}

TO DO

• register client as bean after injection section into BeanPostProcessor
• invoke autowire manually via AutowireCapableBeanFactory
• make sure the bean is created in context, @Autowired and @Qualifier are available for method and field injection
• proof it with unit test

[ST-DDT]

I really like the idea of your change, but IMO the @GrpcClient annotation in its current state supports multiple specific variations for a single client (name). Creating a bean for every @GrpcClient annotation would break existing code (duplicate bean definition) or behave randomly (e.g. due to the random bean initialization order).
How about adding a new annotation for that.

/**
 * Annotation that can be added to `@Configuration` classes to create GrpcClient beans in the ApplicationContext.
 */
@Target(CLASS)
@Repeatable
public @interface GrpcClientBean {

    /** The type of the bean to create. */
    Class<?> clazz();

    /** The name of the bean to create. If empty, a name will be generated automatically based on the bean class and the client name. */
    String beanName() default "";

    /** The client definition used to create the bean. */
    GrpcClient definition();

}

(Not sure about the property names yet)

Usage:

@Configuration
@GrpcClientBean(clazz = SimpleServiceStub.class, definition = @GrpcClient("foobar"))
public class GrpcBeanConfig {

    MyAnotherExternalBean myExternalBean(SimpleServiceStub foobarSimpleServiceStub) {
        return new MyAnotherExternalBean (foobarSimpleServiceStub);
    }

}

I'm not sure whether we should add a scope option as the stubs themselves are immutable?

[ST-DDT]

See also https://github.com/yidongnan/grpc-spring-boot-starter/issues/463
( I plan to refactor the bean post processor bean a bit to use a GrpcStubInjector to split the stub generation from the stub injection, this would hardly have any impact on the implementation of this feature )

[a-simeshin]

I really like the idea of your change, but IMO the @GrpcClient bean in its current state supports multiple specific variations for a single client (name).

I thought about this problem. It seems to me that a good solution would be to declare the bean name as @GrpcClient value + class name of the stub, because it is impossible by means of this library to create two identical types of stubs for one client name.

Unfortunately, this idea has failed.
I still don't understand why, when registering a bean name as name + injectionType.getSimpleName(), then it cannot be found in the context by @Qualifier, but when registering a bean name as Introspector.decapitalize(injectionType.getSimpleName()), everything goes well.
@ST-DDT, сould you be so kind to tell me why this is happening? It just blows my mind :(

Creating a bean for every @GrpcClient annotation would break existing code (duplicate bean definition) or behave randomly (e.g. due to the ranom bean initialization order).

That is why "soft" registration of the bean is proposed with try catch and only warn. Without this, indeed, even in the examples of unit tests, it was clear that about 50% would break without proper softness. It seems to me that this solution should not break anything in existing projects, and tests confirm this, while allowing you to use all the advantages of @Bean in new projects.

How about adding a new annotation for that.

To be honest, I don't like this solution, because for me all the beauty of this project is in 1 simple annotation that will do everything for you, then we will have more and more. On the other hand, we will be sure that nothing will break.
Anyway, @ST-DDT, I will trust your opinion and wait for your comment below. Will do it as you say

[ST-DDT]

I really like the idea of your change, but IMO the @GrpcClient bean in its current state supports multiple specific variations for a single client (name).

I thought about this problem. It seems to me that a good solution would be to declare the bean name as @GrpcClient value + class name of the stub, because it is impossible by means of this library to create two identical types of stubs for one client name.

Unfortunately, this idea has failed.
I still don't understand why, when registering a bean name as name + injectionType.getSimpleName(), then it cannot be found in the context by @Qualifier, but when registering a bean name as Introspector.decapitalize(injectionType.getSimpleName()), everything goes well.
@ST-DDT, сould you be so kind to tell me why this is happening? It just blows my mind :(

After some debugging I managed to get it to work. In my case I either forgot to update one of the two @Qualifier annotations or had typos in them (other character case etc).

I modified the code a bit to make it easier to debug:

final String beanName = annotation.value() + injectionType.getSimpleName();
// final String beanName = Introspector.decapitalize(injectionType.getSimpleName());
try {
	final ConfigurableListableBeanFactory beanFactory =
			((ConfigurableApplicationContext) this.applicationContext).getBeanFactory();
	final Set<String> names = Sets.newHashSet(beanFactory.getBeanNamesIterator());
	beanFactory.registerSingleton(beanName, value);
	final Set<String> names2 = Sets.newHashSet(beanFactory.getBeanNamesIterator());
	names2.removeAll(names);
	log.warn("New Names: {}", names2);

	this.applicationContext.getAutowireCapableBeanFactory().autowireBean(value);
} catch (final Exception e) {
	log.warn("Could not register and autowire bean: {}", beanName, e);
}

IMO distinguishing them by client name is a requirement, especially since the following is potentially frequently used (especially if spread over multiple classes/libraries):

@GrpcClient("test")
TestServiceGrpc.TestServiceBlockingStub blockingStub;

@GrpcClient("fallback")
TestServiceGrpc.TestServiceBlockingStub fallbackBlockingStub;

Creating a bean for every @GrpcClient annotation would break existing code (duplicate bean definition) or behave randomly (e.g. due to the ranom bean initialization order).

That is why "soft" registration of the bean is proposed with try catch and only warn. Without this, indeed, even in the examples of unit tests, it was clear that about 50% would break without proper softness. It seems to me that this solution should not break anything in existing projects, and tests confirm this, while allowing you to use all the advantages of @Bean in new projects.

You are right. Your soft registration does not break any existing code, but it writes plenty of warnings to the logs.
(That is if you have used @GrpcClient SomeStub in more than one class.)
If I just run the tests, I get 265 of these warnings. Sure the test code does not reflect real code, but this huge number still indicates, that there is an issue there (84 with client disambiguation).

How about adding a new annotation for that.

To be honest, I don't like this solution, because for me all the beauty of this project is in 1 simple annotation that will do everything for you, then we will have more and more. On the other hand, we will be sure that nothing will break.

Thanks for sharing your opinion. I summarized the benefits of each variant below, please comment, if I forgot something.

Pro OneForAll:

@GrpcClient("client")
private MyStub myStub;

• Single annotation for all purposes
• Easy to define

Pro ExtraAnnotation:

@GrpcClientBean(clazz = MyStub.class, definition = @GrpcClient("client"))

• Unambiguous
• Allows declaring multiple beans of a single type
• Allows customization of bean names
• No need to define a field just to create a bean
• Easy to define

From this summary, I still prefer the ExtraAnnotation variant, but that's just my opinion.
@yidongnan , @Shinigami92 what do you think would be the best way here? (feel free to propose other solutions as well)

[ST-DDT]

I consulted with @Shinigami92 and we came to the conclusion, that the unambiguity of the bean definition is more important than having only a single annotation for all.

Please change your PR to create an extra annotation for grpc clients/stubs that will be added to the application context.

[a-simeshin]

I consulted with @Shinigami92 and we came to the conclusion, that the unambiguity of the bean definition is more important than having only a single annotation for all.

Please change your PR to create an extra annotation for grpc clients/stubs that will be added to the application context.

Thank you very much for the research after reanalysis, the solution with additional annotation looks better for me now. I`ll start ASAP on this week

Before starting, I propose to have your agreement with solution, that we should add annotation, which add to the context a bean

/**
 * Annotation that can be added to `@Configuration` classes to create GrpcClient beans in the ApplicationContext.
 */
@Target(CLASS)
@Repeatable
public @interface GrpcClientBean {

    /** The type of the bean to create. */
    Class<?> clazz();

    /** The name of the bean to create. If empty, a name will be generated automatically based on the bean class and the client name. */
    String beanName() default "";

    /** The client definition used to create the bean. */
    GrpcClient definition();

}

But there are several questions that are not entirely clear to me:

1. If the bean name is not specified, then should I create a bean with name GrpcClient.value() + clazz.getSimpleName() ?
   A: Yep it`s in the text of annotation. Im blind
2. Should I call manually autowireBean(beanValue) after bin creation?
3. Do I need to create a separate BeanPostProcessor class that will work only with this annotation, separately from @GrpcClient ?
4. Should I check, that class with annotation @GrpcBeanClient also annotated with @Configuration? Or we can offer a softer solution?
5. How a situation should be handled when an annotated field with @GrpcClient already declared manually in the class with annotation @GrpcBeanClient ?

[ST-DDT]

I`ll start ASAP on this week

Awesome. No need to rush though. Have a nice weekend!

2. Should I call manually autowireBean(beanValue) after bin creation?

I'm not sure about that. Will the beans get autowired if not called?
IMO it is not that important for now, as the @grpcClient injections aren't autowired right now either.

3. Do I need to create a separate BeanPostProcessor class that will work only with this annotation, separately from @GrpcClient ?

No, IMO the existing post processor is a good match for it -> Add this new feature to it.
It will get trimmed a bit when the injector will be split into a separate class (separate/future PR).

4. Should I check, that class with annotation @GrpcBeanClient also annotated with @Configuration? Or we can offer a softer solution?

That's a good question. My first thought was we should check for the @GrpcBeanClient annotation, and if it isn't an annotated with @Configuration, then we should throw an exception. While it is probably not necessary to add the restriction, I hope to establish some good coding practices with that.

5. How a situation should be handled when an annotated field with @GrpcClient already declared manually in the class with annotation @GrpcBeanClient ?

IMO we should not add any additional limitations on these, so it will be possible to to have both @GrpcClientBean on the class itself and @GrpcClient on its fields. That way you use the client injection inside a config to create special beans that will then be exposed as new beans. IMO a project should use only one of the two variants, but that's probably a very harsh restriction, especially during the migration phase.

---

Any suggestions regarding field names? I'm really not sure which names would be best.

• clazz or type or beanType
• beanName or name
• definition or client

[a-simeshin]

Any suggestions regarding field names? I'm really not sure which names would be best.

• clazz or type or beanType
• beanName or name
• definition or client

I think that simple and concise names just awesome. So clazz and client would be nice, but beanName allows to avoid misunderstandings with the name in the annotation @GrpcClient, because it`s very likely that someone has already created a bean with the client's name in real projects (even me).

[a-simeshin]

@ST-DDT would you be so kind to code review again? GrpcClientBeanInjectionTest with example of usage, also you were definitely right, living without beans hassle and warns in the log is much more better.
ATM no changes in docs, ill add after final agreement with code.

[ST-DDT]

Looks good to me, I have just some small suggestions regarding the code readability.

[a-simeshin]

@ST-DDT would you be so kind to final review?

[ST-DDT]

Sorry, for the delay with the review. I'm currently quite busy so I can only do a fast pass. I hope that I have more time next weekend.

[ST-DDT]

Final review before merge. 👍

[a-simeshin]

@ST-DDT it seems that we have a flaky test - MetricCollectingInterceptorTest > testMetricsFailedCall() failed in different builds with just md changes:

• https://github.com/yidongnan/grpc-spring-boot-starter/pull/580/checks?check_run_id=3635062828
• https://github.com/yidongnan/grpc-spring-boot-starter/pull/580/checks?check_run_id=3635074122
• https://github.com/yidongnan/grpc-spring-boot-starter/pull/580/checks?check_run_id=3635118686

Have I to commit something to rerun build or there is more another "handy" way?
Should I create issue and try to fix and different PR?

[ST-DDT]

@ST-DDT it seems that we have a flaky test - MetricCollectingInterceptorTest > testMetricsFailedCall() failed in different builds with just md changes:

I am aware of that test, I already improved it multiple times to "fail less", but the error still prevails and I'm not sure why. Currently I get a ~1% chance of a failure in my local builds. Sometimes I suspect that it only happens at specific times.

Have I to commit something to rerun build or there is more another "handy" way?

I have a re-run button in travis, that I can use to execute the checks again. Not sure, whether you have it as well.

Should I create issue and try to fix and different PR?

Feel free to have a try (once this one is merged).

[ST-DDT]

Thanks for your contribution!

[ST-DDT]

Please try do move the @GrpcClientBean to its own config and order RatingOrchestrationConfig after that config.

It looks like I need to improve the initialization/bean declaration order somehow.

---

EDIT: Please try removing @Service from the service, as this might interfere with bean initialization order. (Or order it after the RatingOrchestrationConfig)