Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for client stub constructor injection #749

Merged
merged 14 commits into from
Oct 17, 2022

Conversation

xJoeWoo
Copy link
Contributor

@xJoeWoo xJoeWoo commented Sep 19, 2022

This starter is lack of support for Constructor Injection of client stubs. CI is quite a common usage in Spring projects(especially projects with Kotlin) to make service class neat and easy to test.

In old ways, multiple @GrpcClientBeans must be used on configuration class to register client stubs as Spring Bean. So that client stubs can be injected as a bean.

This PR implements a new way for simpler CI. Example

The basic implement mechanism are:

  1. beanName field was added to @GrpcClient, and make this annotation able to mark on constructor parameters;
  2. Search all beans for all constructor parameters annotated with @GrpcClient, by GrpcClientConstructorInjectBeanFactoryPostProcessor. Collect grpc client config together with parameter type.
  3. Build a client stub bean registrations class GrpcClientConstructorInjection then register it to bean factory;
  4. Registering client stub beans in existing post processing class GrpcClientBeanPostProcessor, by GrpcClientBeanPostProcessor#initGrpClientConstructorInjects().

Maybe BREAKING CHANGES:

  1. Names of client stub beans must not the same if they are declared by @GrpcClientBean, even they have same class. This PR changes it to allow registering same name client stub bean if they have same class. Source here

Also, Kotlin was introduced for Constructor Injection test.

Signed-off-by: xJoeWoo <xjoewoo@gmail.com>
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this feature is on my wish-list since a long time.
I have to think about some subtile implications such as having two beans with the same type but different names. I'm not sure whether spring resolves them correctly.

@xJoeWoo
Copy link
Contributor Author

xJoeWoo commented Sep 19, 2022

I didn't evaluate the full impact to interceptor or other stub processing steps. Maybe some researches need to be made by maintainers.

@ST-DDT ST-DDT added the enhancement A feature request or improvement label Sep 19, 2022
@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 19, 2022

Please run gradle spotlessApply and commit the changes.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 20, 2022

Did you commit all changes from spotlessApply?

Signed-off-by: xJoeWoo <xjoewoo@gmail.com>
@xJoeWoo
Copy link
Contributor Author

xJoeWoo commented Sep 20, 2022

Did you commit all changes from spotlessApply?

Yep

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 20, 2022

The tests are failing.

Signed-off-by: xJoeWoo <xjoewoo@gmail.com>
@xJoeWoo
Copy link
Contributor Author

xJoeWoo commented Sep 20, 2022

The tests are failing.

I missed that. Also, I ran test task in root gradle project and every test seemed ok except test server bind port conflicts.

Signed-off-by: xJoeWoo <xjoewoo@gmail.com>
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have an in depth look at this and some extended testing this weekend.

There are a couple of things that I'm not sure about yet:

  • Should we merge the PostProcessors? IMO they are very similar as they work on the same annotation and even exchange data. I'm not sure which implications that might have an the initGrpClientConstructorInjects() method though.
  • I'm not sure whether/how spring determines that the GrpcClient Bean that is created for a constructor parameter is actually intended to be used for that parameter (if there are multiple parameters with the same class).
  • whether there is a way to do this without adding a bean name to it
  • whether there is a way to support bean factory methods/do this in a more generic way

This a very "technical" PR that challenges my knowledge about Spring's internals and thus it might take a few days to review.
Once again, thanks for this PR.

@xJoeWoo
Copy link
Contributor Author

xJoeWoo commented Sep 21, 2022

It's my pleasure to help this project better 😄. I'm not quite an expert on Spring, maybe my ideas are not the perfect solution.

  • The two PostProcessors perform their actions on different phases of launching spring. The bean factory postprocessor collects stubs requirements by constructors, before any beans are initialized. Otherwise due to missing bean corresponding to constructor parameter, the missing bean exception will be thrown when create a bean. The original processor runs after bean initialization, and reusing bean register codes from @GrpcClientBean. The initGrpClientConstructorInjects() method can be moved to first one processor but register codes will be repeated or another util class/reference to the other processor is needed.

  • Multi same classes bean injection must specify which bean should be use. Usually we use @qualifier(old way) or parameter name(new way, param name is same as bean name) to bind a specific bean to parameter. Examples can be found in GrpcClient#beanName, creating a grpc stub bean with specific bean name, and make the name of constructor parameter same as bean name.

  • It would be nice if spring provides some kind of definitions/configurers that can let us pass/bind a specific instance to constructor parameter. However I'm lack of acknowledges about such tools 😂. So the existed client stub bean method is selected.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 26, 2022

I had a look at this and I noticed two shortcomings that needs to be addressed.

  • The bean name must match the constructor parameter name, and doing that can be confusing. To avoid accidental invalid bean assignments, the bean name should be derived from the parameter name and cannot be specified manually.
  • The bean registration has to be unequivocal meaning that only if the bean (with the same name) was created by this library and it was created using the exact same configuration (annotation), then it is allowed. Otherwise a BeanCreationException should be thrown (or bean name conflict or whatever matches best...). It should be clearly documented in the @GrpcClient bean that if it is used in a constructor, then it will create a bean with the name of the parameter.

I also considered explicitly setting the constructor argument values on the bean definition (there is a setter for the value), maybe this works and it isn't required to create a named bean anymore. I didn't have time to test it, but I would like to evaluate it before we merge this solution.

…ues instead of creating bean

Signed-off-by: xJoeWoo <xjoewoo@gmail.com>
@xJoeWoo
Copy link
Contributor Author

xJoeWoo commented Sep 27, 2022

I realized ConstructorArgumentValues in BeanDefinition was the tool I looked for binding client stub to constructor parameter. beanName field on @GrpcClient had removed, and no beans will be created on latest commit.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Sep 27, 2022

Awesome, this looks very promising. I will review this in the next few days.

@ST-DDT ST-DDT added this to the 2.14.0 milestone Oct 4, 2022
@ST-DDT ST-DDT requested a review from yidongnan October 4, 2022 20:47
@ST-DDT ST-DDT mentioned this pull request Oct 7, 2022
Copy link
Collaborator

@yidongnan yidongnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yidongnan yidongnan merged commit 948f66a into grpc-ecosystem:master Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature request or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants