Add support for creating regex and subnet-based ReadFrom instances from a single string #3016

nosan · 2024-10-15T12:31:32Z

Before this commit, it was not possible to use ReadFrom.valueOf for subnet and regex types. This commit introduces support for these types, as well as the use of names in underscore format

tishun

Awesome, thanks for the contribution!

And thanks for the tests. I just have a few of notes.

src/main/java/io/lettuce/core/ReadFrom.java

tishun · 2024-10-15T14:50:42Z

src/main/java/io/lettuce/core/ReadFrom.java

+                return subnet(value.split(","));
+            }
+            if (type.equalsIgnoreCase("regex")) {
+                return regex(Pattern.compile(value));


Possibly throws PatternSyntaxException

If we want to avoid expanding the contract (this is a runtime exception, but assuming existing applications could be catching the IllegalArgumentsException it might constitute a breach of contract to them) we should perhaps wrap the PatternSyntaxException inside a IllegalArgumentsException? What do you think?

that makes sense! My bad. fixed

src/main/java/io/lettuce/core/ReadFrom.java

nosan · 2024-10-15T15:05:05Z

Thank you, @tishun

PR has been updated

nosan · 2024-10-15T15:40:01Z

I didn't add support for the kebab-case format, but it might be worth considering. For instance, order-sensitive-regex is more readable than orderSensitiveRegex. However, as an alternative, users can opt for ORDER_SENSITIVE_REGEX

nosan · 2024-10-15T16:48:36Z

I'm not sure that ReadFrom.valueOf(...) needs to support order-sensitive-regex. For me, it seems like an unnecessary overhead. For more complex configurations like this, Spring Boot already provides LettuceClientConfigurationBuilderCustomizer.

What do you think, @wilkinsona?

wilkinsona · 2024-10-15T17:26:51Z

I'd defer to @tishun and @mp911de on that one. If use of an order-sensitive regex is at least as common as a "normal" regex, I think it would be good for valueOf to support them both.

mp911de · 2024-10-16T13:33:09Z

I didn't add support for the kebab-case format, but it might be worth considering. For instance, order-sensitive-regex is more readable than orderSensitiveRegex. However, as an alternative, users can opt for ORDER_SENSITIVE_REGEX

In doubt, I suggest defaulting to non-order-sensitive to simplify the problem space. Once a future requirement comes along that asks for order-sensitive regex (the same could be for order-sensitive subnet) we can still sort that issue out later.

For now, I suggest including the changes that fit in nicely and where we know that we like their design.

tishun · 2024-10-16T13:41:29Z

Would you consider adding support for the following formats as well? ANY_REPLICA, any-replica?

Actually IMHO we should go with the kebab-case (only, without snake_case) instead.

The Spring Boot application property names are already following this case and it would be mixing of styles if we use a third case (we are already using camelCase so we can't fully avoid mixing styles)
It seems that other projects have adopted the same case for the values, at least based on a very quick search (see for example this result)

tishun · 2024-10-16T13:44:21Z

In doubt, I suggest defaulting to non-order-sensitive to simplify the problem space. Once a future requirement comes along that asks for order-sensitive regex (the same could be for order-sensitive subnet) we can still sort that issue out later.

For now, I suggest including the changes that fit in nicely and where we know that we like their design.

@mp911de you were discussing if we should support order-sensitive pattern and not the case here, right?
If so - I also agree we should start with the non-order-sensitive and we can extend the contract if we decide we need to.

nosan · 2024-10-16T14:18:33Z

Thanks, @mp911de and @tishun

I replaced the snake_case format with the kebab-case format.
I removed support for order-sensitive-regex

nosan · 2024-10-16T14:27:34Z

@tishun
Still, it’s worth considering support for snake_case as well or instead of kebab-case, as it is more familiar and commonly used for constant names (e.g., in enums).

Even the initial request in Spring Boot expected the snake_case format: spring-projects/spring-boot#42576 (comment)

tishun · 2024-10-16T15:57:13Z

@tishun Still, it’s worth considering support for snake_case as well or instead of kebab-case, as it is more familiar and commonly used for constant names (e.g., in enums).

Even the initial request in Spring Boot expected the snake_case format: spring-projects/spring-boot#42576 (comment)

Oh ... I see. Haven't considered that.
I've asked the original author of the issue about their use case.
Not entirely sure they are describing a case where they convert between enums.

But the enum conversion still makes sense to be honest - if another enum has these values (for some reason) we would simply convert based on the name (and not have to have another string representation in the alternative enum).

Perhaps we need to support all three then.

I will agree to both solutions (CamelCase + kebab-case and CamelCase + kebab-case + snake_case).
If we go with the first one we can extend the contract later if needed, but it is a relatively easy change to support from the start.

nosan · 2024-10-16T16:05:12Z

Added support for all three formats.

nosan · 2024-10-16T16:47:59Z

@tishun

I read this comment spring-projects/spring-boot#42576 (comment)

And actually, it makes sense. Initially, the scope of this PR was to support regex and subnet types.

Adding additional formats like snake_case and kebab-case should not be included here at all.

In Spring Boot it will be possible to use the following format:

spring.data.redis.lettuce.read-from=upstreamPreferred
spring.data.redis.lettuce.read-from=subnet:...

So, I propose to revert everything that is related to format changes and just add support for regex and subnet.

Supporting order-sensitive regex has diverted me from my original focus. My apologies.

UPDATE: I reverted the changes related to the formats. They can always be added later if necessary.

…om a single string redis#3013 Before this commit, it was not possible to use ReadFrom.valueOf(name) for subnet and regex types. This commit introduces a new syntax subnet:192.168.0.0/16,2001:db8:abcd:0000::/52 and regex:.*region-1.* respectively

tishun · 2024-10-17T14:01:02Z

@tishun

I read this comment spring-projects/spring-boot#42576 (comment)

And actually, it makes sense. Initially, the scope of this PR was to support regex and subnet types.

Adding additional formats like snake_case and kebab-case should not be included here at all.

In Spring Boot it will be possible to use the following format:

spring.data.redis.lettuce.read-from=upstreamPreferred spring.data.redis.lettuce.read-from=subnet:...

So, I propose to revert everything that is related to format changes and just add support for regex and subnet.

Supporting order-sensitive regex has diverted me from my original focus. My apologies.

UPDATE: I reverted the changes related to the formats. They can always be added later if necessary.

I agree. Thanks for working on this!

nosan · 2024-10-18T17:54:41Z

Thank you very much, @tishun

nosan mentioned this pull request Oct 15, 2024

Add support for creating regex- and subnet-based ReadFrom instances from a single string #3013

Closed

tishun requested changes Oct 15, 2024

View reviewed changes

nosan requested a review from tishun October 15, 2024 15:10

nosan force-pushed the gh-3013 branch 2 times, most recently from 9a4c11b to d41760c Compare October 15, 2024 15:31

nosan mentioned this pull request Oct 15, 2024

Add spring.redis.lettuce.read-from property for configuring LettuceClientConfiguration's read from setting spring-projects/spring-boot#42576

Closed

nosan force-pushed the gh-3013 branch 3 times, most recently from 8d3d3d0 to 17f9b1f Compare October 15, 2024 19:03

nosan force-pushed the gh-3013 branch from 17f9b1f to 6c201e6 Compare October 16, 2024 14:17

nosan force-pushed the gh-3013 branch from 6c201e6 to 613a92f Compare October 16, 2024 16:05

nosan force-pushed the gh-3013 branch from 613a92f to 986547d Compare October 16, 2024 17:44

tishun approved these changes Oct 18, 2024

View reviewed changes

tishun added this to the 6.5.0.RELEASE milestone Oct 18, 2024

tishun added the type: feature A new feature label Oct 18, 2024

tishun merged commit 8fc5a3f into redis:main Oct 18, 2024
5 checks passed

nosan mentioned this pull request Oct 18, 2024

Add spring.data.redis.lettuce.read-from property spring-projects/spring-boot#42588

Open

nosan deleted the gh-3013 branch October 19, 2024 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for creating regex and subnet-based ReadFrom instances from a single string #3016

Add support for creating regex and subnet-based ReadFrom instances from a single string #3016

nosan commented Oct 15, 2024 •

edited by tishun

Loading

tishun left a comment

tishun Oct 15, 2024

nosan Oct 15, 2024

nosan commented Oct 15, 2024

nosan commented Oct 15, 2024

nosan commented Oct 15, 2024

wilkinsona commented Oct 15, 2024

mp911de commented Oct 16, 2024

tishun commented Oct 16, 2024 •

edited

Loading

tishun commented Oct 16, 2024 •

edited

Loading

nosan commented Oct 16, 2024 •

edited

Loading

nosan commented Oct 16, 2024 •

edited

Loading

tishun commented Oct 16, 2024

nosan commented Oct 16, 2024

nosan commented Oct 16, 2024 •

edited

Loading

tishun commented Oct 17, 2024

nosan commented Oct 18, 2024

Add support for creating regex and subnet-based ReadFrom instances from a single string #3016

Add support for creating regex and subnet-based ReadFrom instances from a single string #3016

Conversation

nosan commented Oct 15, 2024 • edited by tishun Loading

tishun left a comment

Choose a reason for hiding this comment

tishun Oct 15, 2024

Choose a reason for hiding this comment

nosan Oct 15, 2024

Choose a reason for hiding this comment

nosan commented Oct 15, 2024

nosan commented Oct 15, 2024

nosan commented Oct 15, 2024

wilkinsona commented Oct 15, 2024

mp911de commented Oct 16, 2024

tishun commented Oct 16, 2024 • edited Loading

tishun commented Oct 16, 2024 • edited Loading

nosan commented Oct 16, 2024 • edited Loading

nosan commented Oct 16, 2024 • edited Loading

tishun commented Oct 16, 2024

nosan commented Oct 16, 2024

nosan commented Oct 16, 2024 • edited Loading

tishun commented Oct 17, 2024

nosan commented Oct 18, 2024

nosan commented Oct 15, 2024 •

edited by tishun

Loading

tishun commented Oct 16, 2024 •

edited

Loading

tishun commented Oct 16, 2024 •

edited

Loading

nosan commented Oct 16, 2024 •

edited

Loading

nosan commented Oct 16, 2024 •

edited

Loading

nosan commented Oct 16, 2024 •

edited

Loading