fix HashMap deserialization

[mfilatov]

@badgerwithagun I found that code in PR #952

    Map<String, String> session = null;
    if (sessionNode != null && !sessionNode.isNull()) {
      Map<String, String> sessTmp = new HashMap<>();
      sessionNode.forEachEntry((key, value) -> sessTmp.put(key, value.asText()));
      session = sessTmp;
    }

works incorrectly for "session":{"java.util.HashMap":{"traceId":"0000","spanId":"00","traceFlags":"00"}}

We will have following in result Map:
"java.util.HashMap" -> ""

[mfilatov]

This fix does not cover all cases.
Possible cause is that we have two different result of serialization for Invocation.

1. Code with HasMap

    var mdc = new HashMap<>();
    mdc.put("REQUEST-ID", "someRequestId");

    var session = new HashMap<>();
    session.put("SESSION-ID", "someSessionId");

    var invocation = new Invocation(CLASS_NAME, METHOD_NAME, parameterTypes, args, mdc, session));

Serialization result
{"className":"foo",............"mdc":{"java.util.HashMap":{"REQUEST-ID":"someRequestId"}},"session":{"java.util.HashMap":{"SESSION-ID":"someSessionId"}}

2. Code with Map.of()

    var mdc = Map.of("REQUEST-ID", "someRequestId");

    var session = Map.of("SESSION-ID", "someSessionId");

    var invocation = new Invocation(CLASS_NAME, METHOD_NAME, parameterTypes, args, mdc, session);

Serialization result
{"className":"foo",............"mdc":{"REQUEST-ID":"someRequestId"},"session":{"SESSION-ID":"someSessionId"}

[mfilatov]

@badgerwithagun what do you think about it?
I added a test in commit to demonstrate this behavior.

[badgerwithagun]

Thanks @mfilatov . I originally had it like this and the tests blew up. I've enabled for the PR, let's see if it was just me....

[badgerwithagun]

@mfilatov I can't find your comment about copying the response from extractSession. Seems to have disappeared.. I think we should also do that by the way. You're quite right; just for defensive programming reasons in a multi-threaded environment.

[mfilatov]

@badgerwithagun You're probably talking about this:

  private TransactionOutboxEntry newEntry(
      Class<?> clazz,
      String methodName,
      Class<?>[] params,
      Object[] args,
      String uniqueRequestId,
      String topic) {

    var invocation =
        new Invocation(
            instantiator.getName(clazz),
            methodName,
            params,
            args,
            serializeMdc && (MDC.getMDCAdapter() != null) ? MDC.getCopyOfContextMap() : null,
            listener.extractSession() == null ? null : new HashMap<>(listener.extractSession()));

I was trying to get around the serialization issue and wasn't thinking about multi-threading at the time :)

[badgerwithagun]

Nice.

Build is failing btw

[mfilatov]

@badgerwithagun I removed the tests from the commit, that I added earlier, added session map copying and fixed code formatting.
But then the test com.gruelbox.transactionoutbox.jackson.TestTransactionOutboxEntrySerialization#testWithSessionAndMdc will fail.

[mfilatov]

I fixed the test in the commit, but I'm not sure it's a good idea.

[mfilatov]

@badgerwithagun I think this is the final version, but I've prepared two more options just in case.

[mfilatov]

@badgerwithagun Two more alternative options #962 and #963

[badgerwithagun]

Hey @mfilatov - feel free to close the other two.

I'm uncomfortable with the hard reliance on HashMap. It feels like any Map type should work here. Is that possible?

[mfilatov]

Hi @badgerwithagun
About listener.extractSession() == null ? null : new HashMap<>(listener.extractSession())
I was trying to prevent using the final implementation of the Map (for example static final class MapN<K,V>), because there is this option NON_FINAL in the code of method TransactionOutboxJacksonModule#typeResolver.

  public static TypeResolverBuilder<?> typeResolver() {
    return new ObjectMapper.DefaultTypeResolverBuilder(
            NON_FINAL,
            BasicPolymorphicTypeValidator.builder().allowIfBaseType(Object.class).build())
        .init(JsonTypeInfo.Id.CLASS, null)
        .inclusion(JsonTypeInfo.As.WRAPPER_OBJECT);
  }

That is why we have different serialization behavior for final and non-final implementations of Map.
If this PR can be accepted in its current form, I will be glad, but I want to draw your attention to the following:

1. TestTransactionOutboxEntrySerialization
   Replaced the use of Map.of with HashMap.
2. If the TransactionOutboxListener#extractSession method returns the final implementation of Map, we will get an error later during deserialization.

Also that's why I've prepared two other solutions to this issue.

[mfilatov]

Hi @badgerwithagun
If this PR is not applicable, then I can prepare additional options.

[badgerwithagun]

I intend to merge this for now as others are starting to report the same issue and would like to try this solution

Just waiting for the build to run green

[badgerwithagun]

Some test failures @mfilatov

[mfilatov]

@badgerwithagun fixed
The same issue with Map.of()

[badgerwithagun]

@mfilatov I think we need to support Map.of() in extractSession; it's a public API and being restrictive about the Map types we support is going to cause all sorts of problems.

For the time being, how about we just take whatever is returned from extractSession and copy it into a hashmap internally (then revert the change to this test)

[mfilatov]

I have reverted back my old changes in TransactionOutboxImpl and TestJacksonSerializer.java
@badgerwithagun please take a look

P.S.: but maybe you meant something else

[mfilatov]

@badgerwithagun Could you please run the build to check that everything is ok?