42576

Comment From: wilkinsona

Thanks, @nosan. Unfortunately, when I looked at this yesterday, I mistakenly thought that ReadFrom was an enum. The changes proposed here demonstrate that's not the case. Sorry for any wasted effort here but I'm not sure that I like having three properties (read-from.type, read-from.cidr-notations, and read-from.pattern). A lot of the time, only type will be needed and the other two are only needed when type has a particular value.

I wonder if we could still use a single property. The values that map directly to a ReadFrom constant are straightforward but we'd need some special handling for subnet and regex. Perhaps we could support something like subnet:notationOne,notationTwo,notationThree and regex:pattern? This support could perhaps be implemented as a @ConfigurationPropertiesBinding converter and the property could be typed as a ReadFrom instance.

I don't want to waste any more of your time (apologies again for the time possibly already wasted) so please don't do anything about the above until we've had a chance to discuss this as a team.

Comment From: nosan

Thanks, @wilkinsona. No problem at all.

Comment From: nosan


@Bean
@ConfigurationPropertiesBinding
StringToReadFromConverter stringToReadFromConverter() {
   return new StringToReadFromConverter();
}


static class StringToReadFromConverter implements GenericConverter {

        private static final Set<ConvertiblePair> CONVERTIBLE_TYPES;

        static {
            CONVERTIBLE_TYPES = Set.of(new ConvertiblePair(String.class, ReadFrom.class));
        }

        @Override
        public Set<ConvertiblePair> getConvertibleTypes() {
            return CONVERTIBLE_TYPES;
        }

        @Override
        public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor targetType) {
            if (source == null) {
                return null;
            }
            String value = source.toString();
            int index = value.indexOf(':');
            if (index != -1) {
                String type = value.substring(0, index);
                if (type.equalsIgnoreCase("subnet")) {
                    return ReadFrom.subnet(StringUtils.commaDelimitedListToStringArray(value.substring(index + 1)));
                }
                if (type.equalsIgnoreCase("regex")) {
                    return ReadFrom.regex(Pattern.compile(value.substring(index + 1)));
                }
            }
            return ReadFrom.valueOf(value);
        }

    }

in that case, it will be possible to use the following syntax:

spring.data.redis.lettuce.read-from=regex:192.*
spring.data.redis.lettuce.read-from=subnet:192.12.128.0/32,192.168.255.255/32
spring.data.redis.lettuce.read-from=any

Comment From: nosan

Is it ok to use ReadFrom field type from io.lettuce.core in RedisProperties.Lettuce properties?

/**
* Defines from which Redis nodes data is read.
*/
private ReadFrom readFrom;

Comment From: nosan

perhaps it would be beneficial to reach out to someone from the Lettuce team to explore the possibility of adding support for the regex and subnet types in the ReadFrom.valueOf(...) method

At the moment:

```java

//ReadFrom valueOf(String name)

    if (name.equalsIgnoreCase("subnet")) {
        throw new IllegalArgumentException("subnet must be created via ReadFrom#subnet");
    }

    if (name.equalsIgnoreCase("regex")) {
        throw new IllegalArgumentException("regex must be created via ReadFrom#regex");
    }

```

Comment From: wilkinsona

perhaps it would be beneficial to reach out to someone from the Lettuce team

I think that's a good idea. WDYT, @mp911de, about supporting subnet:… and regex:… or similar syntax with ReadFrom.valueOf to ease property-based configuration?

Comment From: mp911de

I like the idea a lot, especially flexibility introduced with regex: and subnet: variants is pretty exciting.

Comment From: wilkinsona

Thanks, @mp911de. I've opened https://github.com/redis/lettuce/issues/3013 for further consideration.

Comment From: nosan

@wilkinsona https://github.com/redis/lettuce/pull/3016 It's been merged and will be available in Lettuce 6.5.0.RELEASE. Meanwhile, I've updated the PR. Could I kindly ask you to review it once again, please?

I have the following questions: - How can I correctly map kebab-case values? ReadFrom.valueOf supports case-insensitive names (e.g., upstreampreferred). I used readFrom.replaceAll("-", "") - What values should I use for regex and subnet in additional-spring-configuration-metadata.json?

Comment From: philwebb

How can I correctly map kebab-case values? ReadFrom.valueOf supports case-insensitive names (e.g., upstreampreferred). I used readFrom.replaceAll("-", "")

I think we can use the same logic as in LenientObjectToEnumConverterFactory. I've done that in https://github.com/spring-projects/spring-boot/commit/bea7704b09f2782e60a8490e4e266e9ed9b4db62

What values should I use for regex and subnet in additional-spring-configuration-metadata.json?

I think the ones you've added are correct.

Comment From: nosan

Thank you, @philwebb I cherry-pick your commit 👍

Comment From: wilkinsona

I wonder if we should hold off on documenting regex: and subnet: in the metadata until we're using Lettuce 6.5 by default. The feature's still useful before then and if someone using Lettuce 6.4 wants to configure a regex or subnet read from, they can always use a customizer till Lettuce 6.5 is out and they've overridden the version or we've upgraded.

Comment From: nosan

I wonder if we should hold off on documenting regex: and subnet: in the metadata until we're using Lettuce 6.5 by default. The feature's still useful before then and if someone using Lettuce 6.4 wants to configure a regex or subnet read from, they can always use a customizer till Lettuce 6.5 is out and they've overridden the version or we've upgraded.

It depends on when it gets merged :) If you think it's ready to be merged, I can remove subnet: and regex: from the metadata.

Comment From: nosan

Another approach could be to temporarily add support for regex: and subnet: in Spring Boot, and then remove it once version 6.5.0 is released.

Comment From: wilkinsona

We discussed this today and decided that we're going to err on the side of caution and wait till Lettuce 6.5 is out before merging anything. Hopefully that'll be in the Boot 3.5 timeframe.

Comment From: nosan

Thank you for your feedback, @wilkinsona. Feel free to reach out if any further work is required on this.

Comment From: nosan

6.5.0.RELEASE has been released. I bumped the version to 6.5.0 and uncommented tests for regex and subnet.

Comment From: philwebb

Thanks @nosan. We're going to wait for 3.5 to merge this one. The version bump is a bit risky post RC

Comment From: nosan

Thanks, @philwebb. Should I revert bump version commit, meanwhile?

I know that you have a separate workflow for dependencies upgrade. So maybe, this version bump shouldn’t be included here.

Comment From: philwebb

No it's fine. We'll just deal with that nearer the time.

SpringBoot Add spring.data.redis.lettuce.read-from property

42576