Applied design & implementation level refactoring techniques to improve code readability & maintainability #2517

Taran05 · 2023-04-04T13:53:34Z

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our [issue tracker](https://github.
com/spring-projects/spring-data-elasticsearch/issues). Add the issue number to the Closes #issue-number line below
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Closes #issue-number

pivotal-cla · 2023-04-04T13:53:40Z

@Taran05 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla · 2023-04-04T13:56:01Z

@Taran05 Thank you for signing the Contributor License Agreement!

sothawo

This PR does not fix anything or add value to the code. It rather removes some compiler safety and reverts previous changes.

Don't commit formatting changes that are not aligned with the linked code style rules.

When running the tests, InetSocketAddressParserUnitTests fails.

And, before submitting a pull request, create an issue explaining the problem that you want to address and how you plan to solve it.

sothawo · 2023-04-04T16:09:18Z

src/main/java/org/springframework/data/elasticsearch/client/ClientConfiguration.java

+	 * @return the function for configuring a WebClient.
+	 */
+	Function<WebClient, WebClient> getWebClientConfigurer();
+


This reverts the change from #2513 reintroducing the since 4.3 unused method for configuring the WebClient. Why?

sothawo · 2023-04-04T16:10:42Z

src/main/java/org/springframework/data/elasticsearch/client/ClientConfiguration.java

@@ -329,7 +335,7 @@ default TerminalClientConfigurationBuilder withSocketTimeout(long millis) {
 		 * @param proxy a proxy formatted as String {@literal host:port}.
 		 * @return the {@link TerminalClientConfigurationBuilder}.
 		 */
-		TerminalClientConfigurationBuilder withProxy(String proxy);
+		TerminalClientConfigurationBuilder setProxy(String proxy);


all the methods on this builder class (and probably almost all builders in this project) use the with...() naming. Why should this be changed?

sothawo · 2023-04-04T16:14:02Z

src/main/java/org/springframework/data/elasticsearch/client/ClientConfigurationBuilder.java

@@ -71,7 +73,6 @@
 	public MaybeSecureClientConfigurationBuilder connectedTo(String... hostAndPorts) {

 		Assert.notEmpty(hostAndPorts, "At least one host is required");
-


We separate the Assert.* blocks with a newline from the rest of the code. One leading and one trailing. There might be places where such a newline is missing, but here the formatting is alright.

sothawo · 2023-04-04T16:14:26Z

src/main/java/org/springframework/data/elasticsearch/client/ClientConfigurationBuilder.java

@@ -59,6 +60,7 @@
 	private @Nullable String password;
 	private @Nullable String pathPrefix;
 	private @Nullable String proxy;
+	private final Function<WebClient, WebClient> webClientConfigurer = Function.identity();


reintroducing removed code

sothawo · 2023-04-04T16:15:48Z

src/main/java/org/springframework/data/elasticsearch/client/ClientConfigurationBuilder.java

@@ -91,7 +92,7 @@ public MaybeSecureClientConfigurationBuilder connectedTo(InetSocketAddress... en
 	}

 	@Override
-	public MaybeSecureClientConfigurationBuilder withProxy(String proxy) {
+	public MaybeSecureClientConfigurationBuilder setProxy(String proxy) {


see my other comment in ClientConfiguration

sothawo · 2023-04-04T16:30:51Z

src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchClients.java


 		TransportOptions.Builder transportOptionsBuilder = transportOptions != null ? transportOptions.toBuilder()
 				: new RestClientOptions(RequestOptions.DEFAULT).toBuilder();

 		ContentType jsonContentType = Version.VERSION == null ? ContentType.APPLICATION_JSON
 				: ContentType.create("application/vnd.elasticsearch+json",
-						new BasicNameValuePair("compatible-with", String.valueOf(Version.VERSION.major())));
+				new BasicNameValuePair("compatible-with", String.valueOf(Version.VERSION.major())));


formatting change not compatible with code formatting guidelines

sothawo · 2023-04-04T16:32:47Z

src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchClients.java

+
+	/**
+	 * Provides the {@link ElasticsearchClient} to be used.
+	 *
+	 * @param restClient the low level RestClient to use
+	 * @return ElasticsearchClient instance
+	 */
+	@Bean
+	public static ElasticsearchClient elasticsearchClient(RestClient restClient) {
+
+		Assert.notNull(restClient, "restClient must not be null");
+
+		return ElasticsearchClients.createImperative(restClient, transportOptions());
+	}
+
+	public static TransportOptions transportOptions() {
+		return new RestClientOptions(RequestOptions.DEFAULT);
+	}
+


putting a @Bean method here makes no sense, as this is a final utility class containing static methods and not a Spring configuration class that could provide beans. Bean setup of all relevant beans is done in the org.springframework.data.elasticsearch.client.elc.ElasticsearchConfiguration derived classes.

sothawo · 2023-04-04T16:35:19Z

src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchConfiguration.java

-		Assert.notNull(restClient, "restClient must not be null");
-
-		return ElasticsearchClients.createImperative(restClient, transportOptions());
+		return ElasticsearchClients.elasticsearchClient(restClient);


see my comment in ElasticsearchClients about creation of beans. In addition to that, we cannot remove the transportOptions() method from this abstract class, as it might be overridden by the user in a derived implementation.

sothawo · 2023-04-04T16:35:35Z

src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchConfiguration.java

@@ -81,7 +79,7 @@ public ElasticsearchClient elasticsearchClient(RestClient restClient) {
 	 */
 	@Bean(name = { "elasticsearchOperations", "elasticsearchTemplate" })
 	public ElasticsearchOperations elasticsearchOperations(ElasticsearchConverter elasticsearchConverter,
-			ElasticsearchClient elasticsearchClient) {
+														   ElasticsearchClient elasticsearchClient) {


formatting change not compatible with code formatting guidelines

sothawo · 2023-04-04T17:05:49Z

src/main/java/org/springframework/data/elasticsearch/client/elc/ElasticsearchTemplate.java

@@ -159,7 +160,6 @@ public <T> List<MultiGetItem<T>> multiGet(Query query, Class<T> clazz, IndexCoor
 				.collect(Collectors.toList());
 	}

-	@Override


Why should we remove the @Override annotations on these methods? This would remove the ability of the compiler to check if we are really implementing or overloading a method instead of overloading it.

Otherwise only irrelevant formatting changes

Taran05 added 3 commits April 4, 2023 10:15

Applied implementation & design level refactoring techniques

4e78c63

Passed some test cases & updated the java socs specifictions

d07422a

Added my name in headers of the files edited

f9cd6b9

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 4, 2023

sothawo reviewed Apr 4, 2023

View reviewed changes

sothawo closed this Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Applied design & implementation level refactoring techniques to improve code readability & maintainability #2517

Applied design & implementation level refactoring techniques to improve code readability & maintainability #2517

Uh oh!

Taran05 commented Apr 4, 2023 •

edited

Loading

Uh oh!

pivotal-cla commented Apr 4, 2023

Uh oh!

pivotal-cla commented Apr 4, 2023

Uh oh!

sothawo left a comment

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

sothawo Apr 4, 2023

Uh oh!

Uh oh!

		@@ -71,7 +73,6 @@
		public MaybeSecureClientConfigurationBuilder connectedTo(String... hostAndPorts) {

		Assert.notEmpty(hostAndPorts, "At least one host is required");

Applied design & implementation level refactoring techniques to improve code readability & maintainability #2517

Applied design & implementation level refactoring techniques to improve code readability & maintainability #2517

Uh oh!

Conversation

Taran05 commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pivotal-cla commented Apr 4, 2023

Uh oh!

pivotal-cla commented Apr 4, 2023

Uh oh!

sothawo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Taran05 commented Apr 4, 2023 •

edited

Loading