Closed DeltaIII closed 1 year ago
I moved this ticket into Spring Data Redis as it is a module-specific concern. We have already quite a few converters for JSR-310 types, however we don't have ones for Offset[Date]Time
.
One more aspect here: JdkSerializationRedisSerializer
isn't actually related to CustomConversions
, it would be good if you shared some code in which case you're experiencing null
values so that we understand your context.
Since you're a bit familiar with our code, do you want to submit a pull request so that we can ship a fix with the next release?
FTR: I was able to produce a similar problem (I suppose) by simply adding a OffsetDateTime
(persistent) property to the Person
class from the Spring Data Examples, Spring Data Redis Repositories example, and setting a few OffsetDateTime
values for the "Starks" (e.g. "arya").
Modified Person
class:
class Person {
private OffsetDateTime lastAccessed;
// other persistent properties
Person lastAccessed(OffsetDateTime lastAccessed) {
this.lastAccessed = lastAccessed;
return this;
}
}
Modified PersonRepositoryTest
class:
class PersonRepositoryTests {
private Person arya = new Person("arya", "stark", Gender.FEMALE).lastAccessed(OffsetDateTime.now());
// ...
}
Then I simply ran and debugged the findBySingleProperty()
test case method.
I don't necessarily agree that we need "custom" SD Converters
for java.time
types, particularly since java.time
types are java.io.Serializable
, and the Spring Data Redis JdkSerializationRedisSerializer
(used by default in the RedisTemplate
, which is used by the SD (Redis/KeyValue) Repository infrastructure) should be able serialize OffsetDateTime
values to a byte[]
for the persistent property to be mapped to Redis by the MappingRedisConverter
and other supporting Repository infrastructure classes.
Of course, it would need to be given a chance to serialize and persist java.time
values from persistent entities in the first place, but seemingly, the MappingRedisConverter.writeToBucket(..)
expects there to be a registered, "custom" Conversion for a property of type OffsetDateTime
.
The execution path for a persistent property of type OffsetDateTime
on a persistent entity (such as the Person
class in the examples), as witnessed from the debugger (with source references to Spring Data Redis (only), ignoring Spring Data KeyValue), follows:
0) From the SD KeyValue Repository infrastruture (on which SD Redis bulids), the SimpleKeyValueRepository.save(entity)
method is called
1) Calls SD KeyValue KeyValueTemplate.insert(objectToInsert)
2) RedisKeyValueTemplate.insert(id, objectToInsert)
3) Calls (back to) SD KeyValue KeyValueTemplate.insert(id, objectToInsert)
4) Calls RedisKeyValueAdapter.put(id, item, keyspace)
5) Calls MappingRedisConverter.write(source, sink:RedisData)
6) Calls MappingRedisConverter.writeInternal(..)
7) Then fails to write the OffsetDateTime
since there is no registered, "custom" Conversion in MappingRedisConverter.writeToBucket(..)
There should NOT need to be a "custom" Conversion since the OffsetDateTime
, or java.time
types in general, are "simple" store types that can be serialized to a byte[]
(by SD Redis's JdkSerializationRedisSerializer
) and stored in the Redis hash for a Person
instance, AFAICT.
I have only started investigating this issue so I am still scratching the surface.
On closer inspection, this may be a simple fix, by simply registering additional java.time
Converters
for OffsetDateTime
and similar, missing java.time
types (e.g. OffsetTime
), as necessary in SD Redis's Jsr310Converters
class (see here).
Although, since Java's OffsetDateTime
(and OffsetTime
) have been part of the JDK since Java 8 (for example; see @since
tag), but were not added to SD Redis's Jsr310Converters
class along with other java.time
types on creation, I am now curious why?
I doubt they were simply "forgotten".
We never provided Zoned or Offset variants of temporal types on a broader basis because in typical stores working with dates, such a field requires two components (time, offset/zone) to be properly constructed without resorting to the system timezone.
In Redis, since everything is a byte array, we weren't asked to provide converters and so they never got added in the first place.
Hi, I'm facing the same problem of serialisation of an OffsetDateTime. Is there any workaround using configuration with RedisCustomConversions or RedisTemplate ? Like many i tried to implement my proper RedisTemplate with its custom serializer (ObjectMapper with JavaTimeModule from jackson-datatype-jsr310 or my custom one) but never consumed on sending.
Here an example of the configuration i've done :
Configuration:
@Slf4j
@Configuration
public class CustomRedisConfiguration {
@Bean
public RedisConnectionFactory redisConnectionFactory() {
var redisURI = RedisURI.builder()
.withDatabase(0)
.withHost("localhost")
.withPort(6379)
.build();
RedisConfiguration redisConfiguration = LettuceConnectionFactory.createRedisConfiguration(redisURI);
return new LettuceConnectionFactory(redisConfiguration);
}
@Bean
public RedisCustomConversions redisCustomConversions(OffsetDateTimeToBytesConverter offsetToBytes) {
return new RedisCustomConversions(Collections.singletonList(offsetToBytes));
}
@Primary
@Bean
RedisTemplate<String, TestOffsetDateTime> redisTemplate(RedisConnectionFactory connectionFactory) {
RedisTemplate<String, TestOffsetDateTime> redisTemplate = new RedisTemplate<>();
redisTemplate.setConnectionFactory(connectionFactory);
redisTemplate.setKeySerializer(new StringRedisSerializer());
redisTemplate.setValueSerializer(jackson2JsonRedisSerializer());
redisTemplate.afterPropertiesSet();
return redisTemplate;
}
@Bean
public Jackson2JsonRedisSerializer<TestOffsetDateTime> jackson2JsonRedisSerializer() {
var serializer = new Jackson2JsonRedisSerializer<>(TestOffsetDateTime.class);
ObjectMapper om = new ObjectMapper();
om.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
om.registerModule(new JavaTimeModule());
om.registerModule(new SimpleModule().addSerializer(OffsetDateTime.class, new OffsetDateTimeSerializer()));
serializer.setObjectMapper(om);
return serializer;
}
}
Serializer:
@Component
public class OffsetDateTimeSerializer extends JsonSerializer<OffsetDateTime> {
private static final Logger log = LoggerFactory.getLogger(OffsetDateTimeSerializer.class);
public OffsetDateTimeSerializer() {
}
public void serialize(OffsetDateTime offsetDateTime, JsonGenerator jsonGenerator, SerializerProvider serializerProvider) throws IOException {
if (offsetDateTime == null) {
throw new IOException("OffsetDateTime argument is null.");
} else {
log.info("LOG> in serializer {}", offsetDateTime);
jsonGenerator.writeString(DateTimeFormatter.ISO_OFFSET_DATE_TIME.format(offsetDateTime));
}
}
}
Converter:
@Component
@WritingConverter
public class OffsetDateTimeToBytesConverter implements Converter<OffsetDateTime, byte[]> {
@Override
public byte[] convert(final OffsetDateTime source) {
return source.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME).getBytes();
}
}
Object to send:
@Data
@Builder
@AllArgsConstructor
@NoArgsConstructor
public class TestOffsetDateTime {
public String string;
@JsonFormat(
shape = JsonFormat.Shape.STRING,
pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSXXX"
)
public OffsetDateTime offsetDateTime;
public LocalDateTime localDateTime;
public Date date;
}
Publisher:
public RecordId test() {
var test = TestOffsetDateTime.builder()
.string("test")
.localDateTime(LocalDateTime.now())
.offsetDateTime(OffsetDateTime.now())
.date(new Date())
.build();
ObjectRecord<String, TestOffsetDateTime> record = StreamRecords.newRecord()
.ofObject(test)
.withStreamKey("test-stream");
RecordId r = this.redisOperations.opsForStream().add(record);
log.info("LOG> recordId: {}", r);
return r;
}
@mp911de - Then, I guess 1 question I have is, why are we converting ZonedDateTime
to/from byte[]
arrays (here).
I get that it is just a ZonedDateTime
to String
to byte[]
conversion and back, but we could do the same for OffsetDateTime
/ OffsetTime
values as well.
Yeah, I would like to have these as well. Am 16. Aug. 2023, 19:32 +0200 schrieb John Blum @.***>:
@mp911de - Then, I guess 1 question I have is, why are we de/serializing ZonedDateTime, here? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
@DeltaIII & @lduffman -
I explored more on this issue today, and while Spring Data Redis currently does not support either java.time.OffsetDateTime
or java.time.OffsetTime
types presently, there is a workaround.
I have demonstrated this in my redis-experiments project, within the spring-data-redis-experiments module, and specifically the RedisRepositoryWithEntityHavingOffsetDateTimePropertyIntegrationTests class.
You can see that my User
application entity type (source) contains both OffsetDateTime
and OffsetTime
properties (here).
Additionally, I have "customized" the SD (Redis) Repository infrastructure configuration, and specifically the "RedisCustomConversions
" that is registered as a bean in the Spring ApplicationContext
(container) when using the SD Redis Repository infrastructure.
This allows you to add additional Converters
required by your Spring (Boot) Data Redis applications.
First, I defined custom Converters
for the java.time
types: OffsetDateTime
and OffsetTime
. This is not unlike how Spring Data RedisJsr310Converters
handles conversion for many of the java.time
types already (see here).
Then, I simply replace the RedisCustomConversions
bean with a new instance that registers all the framework provided converters in addition to my application-specific Converters
using a Spring BeanPostProcessor
(see here).
Alternatively, you could simply use the ZonedDateTime
type on your application entities rather that OffsetDateTime
(or OffsetTime
) and properties of these java.time
types will be properly handled.
See #2681.
Thank you very much 😄
Any idea when this would be in a release?
Check out the release calendar at https://calendar.spring.io/. As of now, the next round of releases is scheduled for tomorrow.
@jxblum Thank you very much for your response and investigation. I didn't succeed to use OffsetDateTime with redis streams (no repositories in my case) but just using ZonedDateTime type will do the trick for the moment :)
When using
spring-data-redis
andRedisSerializer.java()
I noticed that a field of typeOffsetTime
wasn't being serialised even though it implementsSerializable
. The root cause is thatSimpleTypeHolder
assumes that any class that starts withjava.time
is a simple type (which in turn meansCustomConversions.isSimpleType(OffsetTime.class) == true
). This breaks the assumption thatCustomConversions
has a converter for the type as per the javadoc:Expected behaviour:
customConversions.isSimpleType
returns false if there are no converters forOffsetDateTime
orOffsetTime
Actual behaviour:
customConversions.isSimpleType(OffsetDateTime.class) == true
even if there are no converters forOffsetDateTime
(and forOffsetTime
Relevant Code links: https://github.com/spring-projects/spring-data-commons/blob/541f0ced32f3f8b2143ea5b793a489828567ff13/src/main/java/org/springframework/data/mapping/model/SimpleTypeHolder.java#L158
https://github.com/spring-projects/spring-data-commons/blob/541f0ced32f3f8b2143ea5b793a489828567ff13/src/main/java/org/springframework/data/convert/CustomConversions.java#L190