`RedisNode` creation from bare hostname assigns default port #3002

LeeHyungGeol · 2024-09-22T08:10:37Z

Summary

This issue arises in the code responsible for separating the host and port from a hostAndPort string, specifically when the string represents an IPv6 address without square brackets or a bare hostname where no port is provided, causing exceptions.

To resolve this, I referenced the ConversionUnitTests test code to assign the characters following the last colon to portString in the case of an IPv6 address without square brackets. For bare hostnames, I used the DEFAULT_PORT from spring-data-redis and assigned it to portString when no port was provided.

https://github.com/spring-projects/spring-data-redis/blob/main/src/test/java/org/springframework/data/redis/connection/convert/ConvertersUnitTests.java#L243

You have read the Spring Data contribution guidelines.
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).

pivotal-cla · 2024-09-22T08:10:39Z

@LeeHyungGeol 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 · 2024-09-22T08:30:50Z

@LeeHyungGeol Thank you for signing the Contributor License Agreement!

mp911de · 2024-09-23T07:13:49Z

src/test/java/org/springframework/data/redis/connection/RedisNodeUnitTests.java

@@ -0,0 +1 @@
+package org.springframework.data.redis.connection;import static org.assertj.core.api.Assertions.assertThat;import static org.junit.jupiter.api.Assertions.assertThrows;import org.junit.jupiter.api.Test;/** * Unit tests for {@link RedisNode}. * I referenced the test code from ConversionUnitTests.java * * @author LeeHyungGeol */public class RedisNodeUnitTests {    private static final int DEFAULT_PORT = 6379;    @Test // IPv4 Address with port    void shouldParseIPv4AddressWithPort() {        RedisNode node = RedisNode.fromString("127.0.0.1:6379");        assertThat(node.getHost()).isEqualTo("127.0.0.1");        assertThat(node.getPort()).isEqualTo(6379);    }    @Test // IPv6 Address with port    void shouldParseIPv6AddressWithPort() {        RedisNode node = RedisNode.fromString("[aaaa:bbbb::dddd:eeee]:6379");        assertThat(node.getHost()).isEqualTo("aaaa:bbbb::dddd:eeee");        assertThat(node.getPort()).isEqualTo(6379);    }    @Test // Bare hostname with port    void shouldParseBareHostnameWithPort() {        RedisNode node = RedisNode.fromString("my.redis.server:6379");        assertThat(node.getHost()).isEqualTo("my.redis.server");        assertThat(node.getPort()).isEqualTo(6379);    }    @Test // Invalid port parsing    void shouldThrowExceptionForInvalidPort() {        assertThrows(IllegalArgumentException.class, () -> {            RedisNode node = RedisNode.fromString("127.0.0.1:invalidPort");        });    }    @Test // Bare IPv6 address (no square brackets)    void shouldParseBareIPv6WithoutPort() {        RedisNode node = RedisNode.fromString("2001:0db8:85a3:0000:0000:8a2e:0370:7334");        assertThat(node.getHost()).isEqualTo("2001:0db8:85a3:0000:0000:8a2e:0370");        assertThat(node.getPort()).isEqualTo(7334);    }    @Test // Bare hostname without port    void shouldParseBareHostnameWithoutPort() {        RedisNode node = RedisNode.fromString("my.redis.server");        assertThat(node.getHost()).isEqualTo("my.redis.server");        assertThat(node.getPort()).isEqualTo(DEFAULT_PORT);    }}


I'm not sure what has happened here. In any case, this test is formatted to not be readable in any way.

@mp911de
I apologize for the issue with the code formatting. I have identified the problem and am currently making corrections. I suspect that the cause of the formatting issue is related to code lint. Therefore, I checked the Spring Data Code Formatting Settings on this page: https://github.com/spring-projects/spring-data-build/tree/main/etc/ide and I am applying those settings.

I am currently trying to apply eclipse-java-google-style.xml as the Eclipse code formatter. Do I need any other formatter?

@mp911de
I have fixed the issue with the code formatting. I would appreciate it if you could review the PR again.

…ttings.

Closes #2928 Original pull request: #3002

Introduce factory method overload accepting the default port so that Sentinel uses the right port. Refine tests. Refine Javadoc. See #2928 Original pull request: #3002

mp911de · 2024-10-10T13:06:05Z

Thank you for your contribution. That's merged and polished now.

Update RedisNode creation from bare hostname should assign default port

59b7535

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 22, 2024

Update RedisNode creation from bare hostname should assign default port

8145291

LeeHyungGeol mentioned this pull request Sep 22, 2024

RedisNode creation from bare hostname should assign default port #2928

Closed

mp911de reviewed Sep 23, 2024

View reviewed changes

mp911de self-assigned this Sep 23, 2024

LeeHyungGeol added 3 commits September 23, 2024 19:33

Update RedisNodeUnitTests code lint to Spring Data Code Formatting Se…

2889a56

…ttings.

Update RedisNodeUnitTests code lint to Intellij Default Settings.

ec2b6c6

Add comment about Apache License & author in the headers of the class.

4a5214e

LeeHyungGeol requested a review from mp911de September 24, 2024 08:03

mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 10, 2024

mp911de changed the title ~~Update RedisNode creation from bare hostname should assign default port~~ RedisNode creation from bare hostname assigns default port Oct 10, 2024

mp911de added this to the 3.4 RC1 (2024.1.0) milestone Oct 10, 2024

mp911de linked an issue Oct 10, 2024 that may be closed by this pull request

RedisNode creation from bare hostname should assign default port #2928

Closed

mp911de pushed a commit that referenced this pull request Oct 10, 2024

RedisNode creation from bare hostname assigns default port.

047e63e

Closes #2928 Original pull request: #3002

mp911de added a commit that referenced this pull request Oct 10, 2024

Polishing.

20ceb4c

Introduce factory method overload accepting the default port so that Sentinel uses the right port. Refine tests. Refine Javadoc. See #2928 Original pull request: #3002

mp911de closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`RedisNode` creation from bare hostname assigns default port #3002

`RedisNode` creation from bare hostname assigns default port #3002

Uh oh!

LeeHyungGeol commented Sep 22, 2024 •

edited

Loading

Uh oh!

pivotal-cla commented Sep 22, 2024

Uh oh!

pivotal-cla commented Sep 22, 2024

Uh oh!

mp911de Sep 23, 2024

Uh oh!

LeeHyungGeol Sep 23, 2024 •

edited

Loading

Uh oh!

LeeHyungGeol Sep 23, 2024

Uh oh!

mp911de commented Oct 10, 2024

Uh oh!

Uh oh!

		@@ -0,0 +1 @@
		package org.springframework.data.redis.connection;import static org.assertj.core.api.Assertions.assertThat;import static org.junit.jupiter.api.Assertions.assertThrows;import org.junit.jupiter.api.Test;/** * Unit tests for {@link RedisNode}. * I referenced the test code from ConversionUnitTests.java * * @author LeeHyungGeol */public class RedisNodeUnitTests { private static final int DEFAULT_PORT = 6379; @Test // IPv4 Address with port void shouldParseIPv4AddressWithPort() { RedisNode node = RedisNode.fromString("127.0.0.1:6379"); assertThat(node.getHost()).isEqualTo("127.0.0.1"); assertThat(node.getPort()).isEqualTo(6379); } @Test // IPv6 Address with port void shouldParseIPv6AddressWithPort() { RedisNode node = RedisNode.fromString("[aaaa:bbbb::dddd:eeee]:6379"); assertThat(node.getHost()).isEqualTo("aaaa:bbbb::dddd:eeee"); assertThat(node.getPort()).isEqualTo(6379); } @Test // Bare hostname with port void shouldParseBareHostnameWithPort() { RedisNode node = RedisNode.fromString("my.redis.server:6379"); assertThat(node.getHost()).isEqualTo("my.redis.server"); assertThat(node.getPort()).isEqualTo(6379); } @Test // Invalid port parsing void shouldThrowExceptionForInvalidPort() { assertThrows(IllegalArgumentException.class, () -> { RedisNode node = RedisNode.fromString("127.0.0.1:invalidPort"); }); } @Test // Bare IPv6 address (no square brackets) void shouldParseBareIPv6WithoutPort() { RedisNode node = RedisNode.fromString("2001:0db8:85a3:0000:0000:8a2e:0370:7334"); assertThat(node.getHost()).isEqualTo("2001:0db8:85a3:0000:0000:8a2e:0370"); assertThat(node.getPort()).isEqualTo(7334); } @Test // Bare hostname without port void shouldParseBareHostnameWithoutPort() { RedisNode node = RedisNode.fromString("my.redis.server"); assertThat(node.getHost()).isEqualTo("my.redis.server"); assertThat(node.getPort()).isEqualTo(DEFAULT_PORT); }}

RedisNode creation from bare hostname assigns default port #3002

RedisNode creation from bare hostname assigns default port #3002

Uh oh!

Conversation

LeeHyungGeol commented Sep 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

pivotal-cla commented Sep 22, 2024

Uh oh!

pivotal-cla commented Sep 22, 2024

Uh oh!

mp911de Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

LeeHyungGeol Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeeHyungGeol Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

mp911de commented Oct 10, 2024

Uh oh!

Uh oh!

`RedisNode` creation from bare hostname assigns default port #3002

`RedisNode` creation from bare hostname assigns default port #3002

LeeHyungGeol commented Sep 22, 2024 •

edited

Loading

LeeHyungGeol Sep 23, 2024 •

edited

Loading