EventStore / EventStoreDB-Client-Java

Official Asynchronous Java 8+ Client Library for EventStoreDB 20.6+
https://eventstore.com
Apache License 2.0
63 stars 20 forks source link

Add serialization customization configs #147

Closed jvera-inviu closed 2 years ago

jvera-inviu commented 2 years ago

Address some issues regarding serialization using Jackson mapper. Fixes https://github.com/EventStore/EventStoreDB-Client-Java/issues/136 https://github.com/EventStore/EventStoreDB-Client-Java/issues/117

This PR adds the possibility to add a configuration file to define Jackson Mapper specific configurations and supported features. If none is provided that basic version will be used. In addition, there is a programatic way of override configurations implementing this interface and configuring it as a System property. (-DJacksonObjectMapperFactory = yourClass)

YoEight commented 2 years ago

Hey @jvera-inviu, why did you close your PR?

jvera-inviu commented 2 years ago

Hey @YoEight, it is not completed yet, I need to add some tests. I miss-clicked because I tried to add it first as a PR to my fork. But it is almost done.

jvera-inviu commented 2 years ago

@YoEight I think it is done, could you take a look? Thanks!

YoEight commented 2 years ago

@jvera-inviu I will do it as soon as I got some free time. Thanks for your work!

oskardudycz commented 2 years ago

@jvera-inviu, although I agree that it'd be great to support that out of the box, and what you did is valid, I also believe that's a bit too many dependencies for the client library. This solution would be a decent one for the application, as it's easier and more predictable on what dependencies you have and manage them. In the general availability client (like this one) the more package you're dependent on, the more permutations for potential conflict other users may have. It may not be easy to manage that if someone already has them installed. Conflicts in peer dependencies are hard to track.

What I'd suggest is to use the approach I did in my samples, have a look here: https://github.com/oskardudycz/EventSourcing.JVM/blob/main/samples/event-sourcing-esdb-simple/src/main/java/io/eventdriven/ecommerce/core/serialization/EventSerializer.java.

package io.eventdriven.ecommerce.core.serialization;

import com.eventstore.dbclient.EventData;
import com.eventstore.dbclient.EventDataBuilder;
import com.eventstore.dbclient.ResolvedEvent;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import io.eventdriven.ecommerce.core.events.EventTypeMapper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.Optional;
import java.util.UUID;

public final class EventSerializer {
  private static final Logger logger = LoggerFactory.getLogger(EventSerializer.class);
  public static final ObjectMapper mapper =
    new JsonMapper()
      .registerModule(new JavaTimeModule())
      .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
      .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);

  public static EventData serialize(Object event) {
    try {
      return EventDataBuilder.json(
        UUID.randomUUID(),
        EventTypeMapper.toName(event.getClass()),
        mapper.writeValueAsBytes(event)
      ).build();
    } catch (JsonProcessingException e) {
      throw new RuntimeException(e);
    }
  }

  public static <Event> Optional<Event> deserialize(ResolvedEvent resolvedEvent) {
    var eventClass = EventTypeMapper.toClass(resolvedEvent.getEvent().getEventType());

    if (eventClass.isEmpty())
      return Optional.empty();

    return deserialize(eventClass.get(), resolvedEvent);
  }

  public static <Event> Optional<Event> deserialize(Class<Event> eventClass, ResolvedEvent resolvedEvent) {
    try {

      var result = mapper.readValue(resolvedEvent.getEvent().getEventData(), eventClass);

      if (result == null)
        return Optional.empty();

      return Optional.of(result);
    } catch (IOException e) {
      logger.warn("Error deserializing event %s".formatted(resolvedEvent.getEvent().getEventType()), e);
      return Optional.empty();
    }
  }
}

It's already enabled in the current version by wrapping serialisation in your mapper and putting bytes directly. I'd suggest to use a similar approach instead of extending our library, as in the long term, having so many dependencies will lead to dependency management conflicts for users and maintenance issues.

I think that this is quite straightforward approach, @jvera-inviu thoughts?

guiwoda-inviu commented 2 years ago

@oskardudycz your approach solves deserialization of RecordedEvents, but JsonMapper instances are being created on several other places as well. Is there any reason to have internal, non-configurable mapper instances? Or should the library allow every place where there's serdes needs to be configurable by the application?

We could replace this complex, configuration-driven approach with a more lightweight SPI based one, so that applications can provide some sort of JsonMapperFactory implementation, with a default one being set by the library for backwards compatibility.

(I work with jvera, we are on a team pushing the implementation of an event-driven architecture with eventstore as the backbone 😇 )

YoEight commented 2 years ago

After a lengthy discussion on this topic with the team, we decided to not go forward with that PR. Our next step will consist of removing all methods that do (de)serialization on behalf of the user. From now on, the library will only expose raw bytes and let the user decide how they want to deserialize their JSON payload.

Thank you @guiwoda-inviu again for your work!

guiwoda-inviu commented 2 years ago

@YoEight no problem! Is there any way in which we can help you with this? We are also working on some changes for the Akka persistence plugin that are also related to serialization. Maybe we can chat, see if there's a way to coordinate contribution?