ContractVerifierObjectMapper - special support for Avro #2405
Conversation
relates to spring-cloudgh-2402 Signed-off-by: Emanuel Trandafir <emanueltrandafir1993@gmail.com>
closes spring-cloudgh-2402 Signed-off-by: Emanuel Trandafir <emanueltrandafir1993@gmail.com>
etrandafir93
force-pushed
the
contracte_verifier_avro
branch
from
1445195 to
558624b
Compare
| <dependency> | ||
| <groupId>org.apache.avro</groupId> | ||
| <artifactId>avro</artifactId> | ||
| <version>1.12.1</version> |
There was a problem hiding this comment.
Move this to a property
| public ContractVerifierObjectMapper() { | ||
| this.objectMapper = new JsonMapper(); | ||
| public ContractVerifierObjectMapper(JsonMapper mapper) { | ||
| this.objectMapper = usesAvro() ? ignoreAvroFields(mapper) : mapper; |
There was a problem hiding this comment.
I'm wondering about one thing though. Now we have Avro. What if in the future we will need to support more things? I don't like that we're leaking Avro in this class.
I think this code should be moved to NoOpContractVerifierAutoConfiguration and have a conditional on class there, where in case of avro being present would create a proper ContractVerifierObjectMapper.
WDYT?
There was a problem hiding this comment.
@marcingrzejszczak yes, it makes sense. I've re-arranged things a bit, now.
Can you please take a look?
etrandafir93
force-pushed
the
contracte_verifier_avro
branch
from
3a6ae0f to
9001803
Compare
closes spring-cloudgh-2402 Signed-off-by: Emanuel Trandafir <emanueltrandafir1993@gmail.com>
etrandafir93
force-pushed
the
contracte_verifier_avro
branch
from
9001803 to
a74018f
Compare
| return new ContractVerifierObjectMapper(mapper); | ||
| } | ||
|
|
||
| @Bean |
There was a problem hiding this comment.
I would wrap these Avro related classes in a nested AvroContractVerifierObjectMapperConfiguration
There was a problem hiding this comment.
I've created the new -nested - config class now
closes spring-cloudgh-2402 Signed-off-by: Emanuel Trandafir <emanueltrandafir1993@gmail.com>
| @Bean | ||
| @ConditionalOnMissingBean | ||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnClass(name = "org.apache.avro.specific.SpecificRecordBase") |
There was a problem hiding this comment.
Since it's a cass you can add this as an optional dependency and then normally reference it. Then same below - you could use the class reference instead of doing Class.forName()
There was a problem hiding this comment.
I've changed it now, it looks much better 👍🏼
It won't fail because of the SpecificRecordBase import if we don't load the nested config class, right?
There was a problem hiding this comment.
Exactly, since we have textual class on conditional on class here the class will not be loaded. If the class is missing the rest won't be read.
closes spring-cloudgh-2402 Signed-off-by: Emanuel Trandafir <emanueltrandafir1993@gmail.com>
| @Bean | ||
| @ConditionalOnMissingBean | ||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnClass(name = "org.apache.avro.specific.SpecificRecordBase") |
There was a problem hiding this comment.
Exactly, since we have textual class on conditional on class here the class will not be loaded. If the class is missing the rest won't be read.
3 participants
Some Avro-specific fields will make the ContractVerifierObjectMapper when it tries to map the actual intercepted message to JSON, before performing the relevant assertions on it.
Ignoring these fields will fix the issue, and it won't affect the JSON assertions.
closes #2402