customize object mapper from the object mapper factory

[kxmas]

Address https://github.com/microsoft/spring-data-cosmosdb/issues/380 and maintains ZonedDateTimeDeserializer customization.

[msftclas]

All CLA requirements met.

[Incarnation-p-lee]

Could you please fix some CI reported issue ? So we can start the review process.

[kxmas]

Could you please fix some CI reported issue ? So we can start the review process.

I cleaned up the imports, but I'm not sure what to do about the failing CI build. This is a minor change that didn't affect the connection settings

[kushagraThapar]

@kxmas The problem with customizing object mapper in ObjectMapperFactory is in case spring instantiates the object mapper bean externally, you loose all the customizations.

In the latest master, you will see, I have now customized objectMapper bean in the constructor of MappingDocumentDbConverter. That seems to be working fine in both the cases. (when spring instantiates, or we instantiate from ObjectMapperFactory. If you agree, please close this PR.

[kxmas]

hi @kushagraThapar,

It's still broken in the sense that if I instantiate my own objectMapper bean, the constructor in MappingDocumentDbConverter is registering a converter that could possibly override the converter that I already configured, along with other settings that I explicitly configured.

The customizations the MappingDocumentDbConverter should only be applied if the a new ObjectMapper is instantiated.

The problem with customizing object mapper in ObjectMapperFactory is in case spring instantiates the object mapper bean externally, you loose all the customizations.

But that's a feature! If I'm taking the time to customize the object mapper, this project shouldn't stomp on my customizations.

[kushagraThapar]

@kxmas , How are you specifying the object mapper? Are you using @Qualifier(Constants.OBJECTMAPPER_BEAN_NAME) property to instantiate your mapper and then relying on spring to autowire it ?

If that's the case, I think I understand your issue now, and it makes sense to me.

[kxmas]

@kxmas , How are you specifying the object mapper? Are you using @qualifier(Constants.OBJECTMAPPER_BEAN_NAME) property to instantiate your mapper and then relying on spring to autowire it ?

If that's the case, I think I understand your issue now, and it makes sense to me.

Yes, I was creating a objectmapper with that bean name and letting spring autowire it.

I'm glad it makes sense now :)

[kushagraThapar]

@kxmas thanks for explanation. It makes sense now. I just started owning this repo, so still exploring it :) Your PR makes sense. I have one question regarding the canDeserialize call.

Also, would it be possible for you to update some of the tests like DocumentDbTemplateIT, DocumentDbTemplatePartitionIT.java, ReactiveCosmosTemplateIT.java, ReactiveCosmosTemplatePartitionIT.java, etc.

They instantiate objectMapper directly, like: new ObjectMapper(). And then pass the objectMapper instance to MappingDocumentDbConverter. If you can please remove that instantiation and just pass null, so that MappingDocumentDbConverter uses ObjectMapperFactory to get objectMapper instance. That way we will be able to test this change in our CI.

[kxmas]

Yeah, I can probably find some time to that.

[kushagraThapar]

Yeah, I can probably find some time to that.

Thanks!

[kxmas]

Tests have been updated.

[kushagraThapar]

Tests have been updated.

Thank you @kxmas for updating the tests, I looked at the code and it looks good. One question I have though is, as you mentioned that JavaTimeModule should handle the ZonedDateTimeDeserializer, why remove the ZonedDataTimeDeserializerTest ? Shouldn't we keep that test and update it to use Java Time Module ?

[kxmas]

Tests have been updated.

Thank you @kxmas for updating the tests, I looked at the code and it looks good. One question I have though is, as you mentioned that JavaTimeModule should handle the ZonedDateTimeDeserializer, why remove the ZonedDataTimeDeserializerTest ? Shouldn't we keep that test and update it to use Java Time Module ?

No, the tests were just exercising the deserializer code that was also deleted. It doesn't make sense to test Jackson's deserialization code in this project. The Jackson2 repo is the place for that.

There's a case to be made to adding a ZonedDateTime to the integration tests though.

[kushagraThapar]

Tests have been updated.

Thank you @kxmas for updating the tests, I looked at the code and it looks good. One question I have though is, as you mentioned that JavaTimeModule should handle the ZonedDateTimeDeserializer, why remove the ZonedDataTimeDeserializerTest ? Shouldn't we keep that test and update it to use Java Time Module ?

No, the tests were just exercising the deserializer code that was also deleted. It doesn't make sense to test Jackson's deserialization code in this project. The Jackson2 repo is the place for that.

There's a case to be made to adding a ZonedDateTime to the integration tests though.

I see, I am just skeptical about JavaTimeModule handling the zonedDateTime deserialization. Can we please add just one test case for that in this PR? I can add more tests later.

[kushagraThapar]

/azp run

[azure-pipelines[bot]]

No pipelines are associated with this pull request.

[kushagraThapar]

/azp run

[azure-pipelines[bot]]

No pipelines are associated with this pull request.

[kxmas]

I see, I am just skeptical about JavaTimeModule handling the zonedDateTime deserialization. Can we please add just one test case for that in this PR? I can add more tests later.

Why would you be a skeptic if you can see the code? :)

Anyway, I updated the ZonedDateTimeDeserializerTest.

[kushagraThapar]

I see, I am just skeptical about JavaTimeModule handling the zonedDateTime deserialization. Can we please add just one test case for that in this PR? I can add more tests later.

Why would you be a skeptic if you can see the code? :)

Anyway, I updated the ZonedDateTimeDeserializerTest.

lol, completely my bad. I apologize for my negligence.

Thank you for taking extra time to update the tests, I will get this in after running additional tests.

[kushagraThapar]

/azp run

[azure-pipelines[bot]]

No pipelines are associated with this pull request.

[kushagraThapar]

/azp run

[azure-pipelines[bot]]

No pipelines are associated with this pull request.