StdObjectMapperFactory Expected Behavior

[Steve Hill]

Hi All,

First - thanks so much for the work on Ektorp. It has really allowed me to hit the ground running and produce something valuable for my project quickly. Its familiar interface makes CouchDb a viable option for folks with Java backgrounds.

I have a question about the expected behavior of StdObjectMapperFactory#setObjectMapper. On the surface, it seems like this would support global config of a given ObjectMapper. In practice, this doesn't always happen.

There are a few related, but distinct items here:

1. The upgrade from 1.3.0 to 1.4.x had a regression that resulted in null values no longer being ignored on serialization.
1. This confused me until I dug into the code
2. The bug resulted in the tutorial being broken, which impacted at least one person - https://github.com/helun/Ektorp/issues/230
3. A fix for restoring this behavior alone is in https://github.com/helun/Ektorp/pull/233
2. Configuration Responsibility - 
1. YannROBERT made a great point: "IMO It's up to the application developer to configure this on a per class basis. Ektorp should not enforce this for every classes." https://github.com/helun/Ektorp/pull/233#issuecomment-83498432
2. I think that's a very valid point, it seems outside of the responsibility of the ObjectMapperFactory to configure the ObjectMapper when a developer has the option to pass in a configured ObjectMapper.

I'd really enjoy contributing the code here, but being new to the project, I want to make sure I'm on the same page as the more established members of the community:

• Does it make sense to fix the non-null regression in isolation?
• If the StdObjectMapperFactory should respect the given ObjectMapper in all cases:
• should we update that class (making a potentially breaking change)?
• or should we introduce a new ObjectMapperFactory implementation and leave the current one unchanged? (proposal below)

Background

There are 2 methods on the ObjectMapperFactory interface:

[1] ObjectMapper createObjectMapper();

[2] ObjectMapper createObjectMapper(CouchDbConnector);

In the current implementation of StdObjectMapperFactory, the variable set by calling setObjectMapper is only used by #1. #2 always creates

a new ObjectMapper, ignoring the variable. I gather this is why the implementation of #1 is synchronized, while #2 is not.

[1] is called from:

• new StdCouchDbInstance(HttpClient)
  
• new StdCouchDbInstance(HttpClient, ObjectMapperFactory)
  
• new ViewQuery()
  

[2] is called from:

• new StdCouchDbConnector(String, CouchDbInstance)
  
• new StdCouchDbConnector(String, CouchDbInstance, ObjectMapperFactory)
  

In the application I've been writing, I'm always using a CouchDbConnector or an extension of CouchDbRepositorySupport<T>, which depends on a CouchDbConnector. So any global configuration I do, for example making DateTime serialize and deserialize from an ISO date string instead of the

default bean-serialization, doesn't take effect here.

Proposal

In my application I've reimplemented the interface to respect the given ObjectMapper in both situations. I think this could be beneficial to others.

```

public class ImmutableObjectMapperFactory implements ObjectMapperFactory {

    private final ObjectMapper objectMapper;

    public ImmutableObjectMapperFactory() {

        this(new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL));

    }

    public ImmutableObjectMapperFactory(ObjectMapper objectMapper) {

        this.objectMapper = objectMapper;

    }

    @Override public ObjectMapper createObjectMapper() {

        return objectMapper;

    }

    @Override public ObjectMapper createObjectMapper(CouchDbConnector connector) {

        ObjectMapper copy = objectMapper.copy();

        copy.registerModule(new EktorpJacksonModule(connector, copy));

        return copy;

    }

}

```

I like this for a few reasons:

1. The passed-in, configured object mapper is always used.
   
2. synchronization is now unnecessary because the mapper is immutable. 
   
3. Default ignore-null configuration is still included if you call the parameterless constructor, so the tutorial would remain working unchanged. 
4. The EktorpJacksonModule is still added only for the instances used by the connector, which seems to mostly exist for the @DocumentReferences annotation.
   

Best,

Steve

[Yann ROBERT]

Hi Steve

I think the default ObjectMapperFactory implementation used by Ektorp should not have any effects on Serialization behavior.
It should just use the default Jackson serialization behavior.

Fact that Ektorp did that in the past was, IMO, a mistake.
A regression made that Ektorp does not configure the Serialization behavior anymore. I think this is fortunate.

Since users of newer versions of Ektorp (and newer version of Jackson) are probably relying the fact that Ektorp does not set serialization behavior, this should not be changed. It already breaks by accident for users upgrading from an old version, we should not break it on purpose for users upgrading from newer versions.

Now, following the questions you raised, I think it should be made simple for a developer to configure Ektorp to use an implementation of the interface that do set serialization behavior, it he wants to for a specific application.
In particular it would make the job of upgrading from an older version simpler (for instance you would not have to mess with annotating all of your classes with @JsonInclude(JsonInclude.Include.NON_NULL) like I suggested in PR #233)
In that regard, Ektorp already provides a way to pass an alternative implementation of the interface. (except in ViewQuery.DEFAULT_MAPPER but it's minor)

Now regarding your proposal.

I like your implementation of the interface.
I dislike the fact StdObjectMapperFactory is mutable. Your implementation fixes that.
But it's setting serialization behavior (regarding inclusion of nulls). Maybe we could had your implementation to Ektorp (renamed and not used). But we would need a variant that does not set the serialization behavior at all, that will be used by default in replacement of StdObjectMapperFactory.
StdObjectMapperFactory should remain unchanged (maybe marked deprecated) in case there are users out there that reference this class directly. [It would be good to known if there are any people out there that are actually using class StdObjectMapperFactory (and it's setters) in there code?]

To a larger extent, I think the interface ObjectMapperFactory is too complex :
method ```ObjectMapper createObjectMapper(CouchDbConnector connector)``` should not have been there in the first place, it should have been inlined at the only location that is using it.
I'd like to see the ObjectMapperFactory interface splitted in 2. Maybe we should do that in a major Ektorp release?

Any thoughts Henrik?

- Yann

[Henrik Lundgren]

StdObjectMapperFactory has always been a small headache. The awkwardness of the code is mainly a legacy from the time when jackson did not support modules, so the whole thing should be refactored now when it is easier to integrate with jackson.

But the main priority should be not to break existing code.

Since auto setting Include non null is the existing default behavior this cannot change.

If anyone has other needs then it should be easy override this.

Regarding a new major release, I'm thinking of making a non-backwards compatible version based on Java 8 with multiple modules with different levels of abstraction and convenience.

Here's some modules that ought to exist:

    1. couchdb-api module that only encapsulates the urls and requests and response types that couchdb supports.

    2. Jackson-mapping support

    3. Classic Apache-http connector

I would drop support for @DocumentReferences since it is mostly a source of problems.

I would also drop support for Spring, it is easy enough to support it yourself when you need it.

I for one will need other connectors such as a non-blocking connector for vert-x, but this will tie in very hard to the vert-x api so it must be a stand alone module.

The vert-x connector would probably just use the couch-db api module and jackson.

//Henrik

--
You received this message because you are subscribed to the Google Groups "ektorp-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ektorp-discus...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.