Skip to content

Create RestClient bean from a RestHighLevelClient if such a bean has been contributed by the application. #17488

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Create RestClient bean from a RestHighLevelClient if such a bean has been contributed by the application. #17488

wants to merge 1 commit into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Jul 11, 2019

see gh-17464

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 11, 2019
bclozel
bclozel reviewed Jul 11, 2019
View reviewed changes
...a/org/springframework/boot/autoconfigure/elasticsearch/rest/RestClientAutoConfiguration.java Outdated

@Bean
@ConditionalOnMissingBean
public RestClient restClient(RestHighLevelClient restHighLevelClient) {
Copy link
Member

Choose a reason for hiding this comment

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

There's already another bean method named for RestClient with the same name. Wouldn't that create issues with bean overriding?
Maybe we could change the existing bean method and inject ObjectProvider<RestHighLevelClient> and do everything there (return the client from the high level if it exists or create a new one from the builder). What fo you think (I've quickly looked at your PR, so I might haved missed something)?

@bclozel bclozel added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 11, 2019
@bclozel bclozel added this to the 2.1.x milestone Jul 11, 2019
@bclozel bclozel mentioned this pull request Jul 11, 2019
Closed
@nosan
Copy link
Contributor Author

nosan commented Jul 12, 2019

Thank you, @bclozel

There's already another bean method named for RestClient with the same name. Wouldn't that create issues with bean overriding?

Everything is ok, at least I added a test to check this.

Maybe we could change the existing bean method and inject ObjectProvider

We could not change the existing bean method as RestHighLevelClient and RestClient are present in different jars.
Use ObjectProvider<RestHighLevelClient> instead of RestHighLevelClient bean is a good idea 👍,
at least it is a guarantee that configuration will be applied only if there is a unique bean of RestHighLevelClient. (I added a test to check this as well)

PR has been updated.

snicoll pushed a commit that referenced this pull request Aug 1, 2019
@nosan @snicoll
Create RestClient from a RestHighLevelClient if available
0e87b9c 
See gh-17488
snicoll added a commit that referenced this pull request Aug 1, 2019
@snicoll
Polish "Create RestClient from a RestHighLevelClient if available"
fb0fccf 
This significantly rework the auto-configuration to reflect the order
in which things are expected. Rather than keeping a conceptual cycle
between the builder and the two inner classes that are processed first,
the configuration is now split in three parts:

* The builder that is required and common
* The configuration when the HighLevelClient is available
* The RestClient configuration when that's not the case

See gh-17488
@snicoll snicoll mentioned this pull request Aug 1, 2019
Closed
@snicoll snicoll modified the milestones: 2.1.x, 2.1.7 Aug 1, 2019
@snicoll snicoll closed this in a76aaab Aug 1, 2019
@snicoll
Copy link
Member

snicoll commented Aug 1, 2019

Thanks again for the pr @nosan.

Everything is ok, at least I added a test to check this.

It is ok indeed as they are conditions to avoid creating it. However restClient is quite generic and I think we should rename it. I've created #17725

@nosan the first test in your PR has an invalid assertion. I fix it to assert that the rest client is the expected one and, of course, it failed since I've changed the assertion. The test failed as things were not created in the expected order.

Unrelated to your PR but there was a conceptual cycle as the inner class (processed first) required the RestClientBuilder that is provided by the auto-configuration class itself. I've polished the auto-configuration to avoid this cycle and provide an explicit order.

@snicoll snicoll self-assigned this Aug 1, 2019
@nosan
Copy link
Contributor Author

nosan commented Aug 1, 2019

Thank you so much, @snicoll

the first test in your PR has an invalid assertion. I fix it to assert that the rest client is the expected one and, of course, it failed since I've changed the assertion. The test failed as things were not created in the expected order.

Sorry about that 😕

@nosan nosan deleted the gh-17464 branch August 1, 2019 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bclozel bclozel bclozel left review comments

Assignees

@snicoll snicoll

Labels
type: bug A general bug
Projects
None yet
Milestone
2.1.7
Development

Successfully merging this pull request may close these issues.
4 participants
@nosan @snicoll @bclozel @spring-projects-issues