Rationalise use of ObjectMapper

[snicoll]

What is the purpose of the change

This commit makes sure that an ObjectMapper is created only if the
regular Spring Boot auto-configuration didn't get a chance to create
one.

In more complex scenario where multiple ObjectMapper instances are
available in the context, a user has to provide an ObjectMapper named
rocketMQMessageObjectMapper.

Brief changelog

• Polish auto-configuration to run after JacksonAutoConfiguration.
• As Jackson is a required dependency removed the inner class and added the condition at class level
• Added tests to demonstrate the use of custom ObjectMapper

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

• Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
• Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
• Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

[snicoll]

I've filed the CLA FWIW

[walking98]

I can see a cycle dependency error with the PR:

The dependencies of some of the beans in the application context form a cycle:

org.springframework.rocketmq.spring.starter.internalRocketMQTransAnnotationProcessor defined in class path resource [org/apache/rocketmq/spring/config/RocketMQAutoConfiguration.class]
↓
transactionHandlerRegistry defined in class path resource [org/apache/rocketmq/spring/config/RocketMQAutoConfiguration.class]
┌─────┐
| rocketMQTemplate defined in class path resource [org/apache/rocketmq/spring/config/RocketMQAutoConfiguration.class]
↑ ↓
| producerApplication
└─────┘

The reproduce steps:

1. mvn clean install (stage the change)

2. add the following bean injection code in rocketmq-spring-boot-samples/rocketmq-produce-demo/src/main/java/org/apache/rocketmq/samples/springboot/ProducerApplication.java
   @bean
   public ObjectMapper objectMapper() {
   return new ObjectMapper();
   }

3. mvn clean package (under rocketmq-spring-boot-samples)

4. java -jar rocketmq-produce-demo/target/rocketmq-produce-demo-0.0.1-SNAPSHOT.jar

The root reason is that the ObjectMapper bean in RocketMQAutoConfiguration is no longer initiated prior to RocketMQTemplate bean injection.

So we still need the @ConditionalOnMissingBean(name = "rocketMQMessageObjectMapper") in rocketMQMessageObjectMapper bean declaration of the RocketMQAutoConfiguration.

[walking98]

should be @ConditionalOnMissingBean(name = "rocketMQMessageObjectMapper")
due to a potential cycle dependency issue. see #6 (comment)

[snicoll]

I think it vastly depends what you want to do as part of the starter and it it still unclear to me. If you want to use the "object mapper" defined in the context, then relying on a named bean by default is the wrong thing to do (as you'd force user to name their unique instance that way).

If you want to make sure that a dedicated ObjectMapper is used for rocketmq, then it should be more clear than just the name.

The suggestion here is the best compromise: if you have a single instance we'll use that. If you have more than one, we'll look for the @Primary one and if that does not exist we'll fallback to one named rocketMQMessageObjectMapper.

[vongosling]

@walking98 I am also confused why we must create a new ObjectMapper here. I also found too many dedicated named instance in older code. I think we should re-think the reusable components in spring components. We really allow many groupId and many topic existing in one instance, to mapping spring concept, how many instance should we allow?

[walking98]

The ideal situation is that there is only exactly one ObjectMapper instance, auto-config needs to create the "rocketMQMessageObjectMapper" if user app does not create. we can not prevent user app to create more than one, if so we just use @primary one instead of creating the "rocketMQMessageObjectMapper".

[snicoll]

@walking98 the cycle has nothing to do with my change (it was broken all along). There is field injection in your spring boot application, something you should never do because you're basically asking for the root source class to be only created when those fields are injected. At the same time, you are creating an ObjectMapper as a method of that bean (so we need to pick that up to apply field injection right after construction).

Please restructure the sample so that the actual sample is in its own dedicated class. Please refrain from using field injection. Let me know if I can assit restructure the sample.

[walking98]

@walking98 the cycle has nothing to do with my change (it was broken all along). There is field injection in your spring boot application, something you should never do because you're basically asking for the root source class to be only created when those fields are injected. At the same time, you are creating an ObjectMapper as a method of that bean (so we need to pick that up to apply field injection right after construction).

Please restructure the sample so that the actual sample is in its own dedicated class. Please refrain from using field injection. Let me know if I can assit restructure the sample.

Sounds like this is a bad usage, so your suggestion is to use the field injection instead of the method injection in the sample code? we must ask user change the code if user has written this sort of method injection?

[snicoll]

your suggestion is to use the field injection instead of the method injection in the sample code?

I am not sure I got that. My suggestion is certainly not to use field injection. At all.

we must ask user change the code if user has written this sort of method injection?

As I've indicated in my previous comment, using field injection on the main context class is a bad practice regardless of this project. To be clear the cycle is the field injection of the template. It will work just fine as long as you're not trying to customize things. By adding an ObjectMapper as a method as you've done here, it is very easy to create the cycle.

Your sample should not be written in your @SpringBootApplication regardless. The configuration should be separate as well to clearly show what is infrastructure and what is the sample itself. Doing this should be done regardless of this issue IMO.

[walking98]

ok, understand. so that is why your test cases can pass because of the other ObjectMapper beans are created out of the main context (@SpringBootApplication)

[snicoll]

That and you're not using field injection in the same class.

[walking98]

LGTM