spring-framework icon indicating copy to clipboard operation
spring-framework copied to clipboard

Fix the issue in MappingJackson2MessageConverter where deserializatio…

Open cooperlyt opened this issue 1 year ago • 2 comments

Fix the issue in MappingJackson2MessageConverter where deserialization returns null values for POJOs that do not have @JsonView annotations

When a POJO does not have the JsonView annotation set, and Jackson's DEFAULT_VIEW_INCLUSION is false (the default value in Spring framework #16793), deserialization fails and returns an object with all properties empty.

I added a test in MappingJackson2MessageConverterTests that will fail before I fix it.

	@Test
	public void fromMessageToMessageWithPojoClass(){
		// #16793 https://github.com/spring-projects/spring-framework/issues/16793
		//ObjectMapper objectMapper = JsonMapper.builder().configure(MapperFeature.DEFAULT_VIEW_INCLUSION, false).build();
		MappingJackson2MessageConverter converter = new MappingJackson2MessageConverter();
		String payload = "{\"string\":\"foo\"}";
		Message<?> message = MessageBuilder.withPayload(payload.getBytes(StandardCharsets.UTF_8)).build();
		Object actual = converter.fromMessage(message, MyBean.class, MyBean.class);
		assertThat(actual).isInstanceOf(MyBean.class);
		assertThat(((MyBean) actual).getString()).isEqualTo("foo");
	}

PS: Discussion with jackson-databind issues.

cooperlyt avatar Oct 16 '24 02:10 cooperlyt

@cooperlyt Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla avatar Oct 16 '24 02:10 pivotal-cla

@cooperlyt Thank you for signing the Contributor License Agreement!

pivotal-cla avatar Oct 16 '24 02:10 pivotal-cla

Could you please share the original use case that ended-up passing this MyBean.class hint to the converter?

sdeleuze avatar Oct 21 '24 09:10 sdeleuze

I Use Spring Cloud Stream connect to RabbitMQ, send a Json message, that happened on customer

cooperlyt avatar Oct 21 '24 12:10 cooperlyt

I would need a reproducer since I am not sure the use case is valid, it could be a Spring Cloud Stream bug. Please attach a zip archive or comment with a link to a repository that allow to reproduce this use case.

sdeleuze avatar Oct 21 '24 12:10 sdeleuze

the case need a RabbitMQ, Can I provide a docker-compose.yml

cooperlyt avatar Oct 21 '24 13:10 cooperlyt

Sure docker-compose.yml is perfectly fine, it will allow me to reproduce locally.

sdeleuze avatar Oct 21 '24 13:10 sdeleuze

A test case Project

just run ./gradlew test test will be fails.

add spring.jackson.mapper.default-view-inclusion = true test will be pass

RabbitMQ docker-compose.yml

cooperlyt avatar Oct 21 '24 15:10 cooperlyt

Thanks for the reproducer, I will have a look.

sdeleuze avatar Oct 23 '24 15:10 sdeleuze

Looks like https://github.com/spring-cloud/spring-cloud-function is using the conversionHint parameter as a way to pass the itemType as a Class here which is not supported by MappingJackson2MessageConverter or other org.springframework.messaging.converter.MessageConverter as far as I can tell.

@olegz Can you elaborate on this? I think I understand the intent, but that's not a supported use case so far unless I am mistaken.

sdeleuze avatar Oct 23 '24 16:10 sdeleuze

Consider the following Function

public Function<List<Person>, Integer> function() 

with the following payload as input

[{"name":"julien"},{"name":"ricky"},{"name":"bubbles"}]

If we don't provide/use conversionHint the List will come in as List<Map> instead of List<Person>

So, aside form passing a targetClass as Class to convertFromInternal(Message<?> message, Class<?> targetClass, @Nullable Object conversionHint) we need to pass ParameterisedType as hint and eventually invoke ObjectMapper.mapper.readValue((String) json, constructType) where constructType is ParameterisedType, thus ensuring successful conversion

olegz avatar Oct 24 '24 08:10 olegz

And with regard to the original issue, i don't understand why @cooperlyt needs to register custom message converter - CustomMessageMarshallingConverter. Within SCF/Stream stack we provide JsonMessageConverter that is capable of converting complex types Can you please elaborate?

@sdeleuze feel free to push this issue to s-c-stream/function if you determine it's on us.

olegz avatar Oct 24 '24 08:10 olegz

@cooperlyt Can you please elaborate on what @olegz asked?

sdeleuze avatar Oct 25 '24 17:10 sdeleuze

Sorry for the late reply.

Let me explain the reasons:

First, after Spring Boot 3.3.X, if I don't register a custom JSON converter, it won't be applicable to any other converters. For example, in my sample program, if I remove the registration of CustomMessageMarshallingConverter, it will throw an error saying a binary array cannot be converted to an object. I'm not sure why JsonMessageConverter is not effective.

Second, I need to make some custom settings to the ObjectMapper, such as registering some custom modules. @sdeleuze @olegz

cooperlyt avatar Oct 26 '24 11:10 cooperlyt

I checked the code of JsonMessageConverter, and it seems that it does not have the ability to deserialize from JSON byte[] to an object . Additionally, starting from Spring Boot 3.3.X, it appears that the original JSON conversion done in the Stream RabbitMQ module has been deprecated, with the output fixed as byte[] and handed over to the converter to handle the deserialization of the payload. but JsonMessageConverter does not have the capability to handle this, I had to use MappingJackson2MessageConverter to convert my message payload.

To solve this problem, it seems there are two options: one is to enable MappingJackson2MessageConverter to correctly handle the deserialization of the payload (which is the goal of my PR), and the other is to give JsonMessageConverter the ability to deserialize JSON byte[] to an object!

Am I misunderstanding anything? @olegz

cooperlyt avatar Oct 26 '24 12:10 cooperlyt

@olegz I am open to evolve MappingJackson2MessageConverter to do what you need (element type resolution), just for now what your try to do with the API is not supported. Please let me know if we should add support for type resolution on MappingJackson2MessageConverter to make Spring Cloud Stream using it if that make sense.

sdeleuze avatar Oct 28 '24 08:10 sdeleuze

@sdeleuze I would love if such support was provided at the core and somewhat surprised that it has never came up especially in the context of Spring Messaging Wondering what @artembilan thinks of that as well as I suspect there is something to that extent in spring-integration as well, but i may be wrong.

olegz avatar Oct 28 '24 09:10 olegz

@cooperlyt I don't understand what you mean when you say it does not have the ability to deserialize from JSON byte[] to an object as it is exactly what it does and all our payloads in stream/function come as byte[] and there are applications and tests to validated that, so please clarify. . . If you can provide a reproducible sample we can certainly take a look and see if any improvements are necessary.

Also, customisations to ObjectMapper . . . you can certainly do it via standard boot mechanisms

But as I said, a reproducible sample would help

olegz avatar Oct 28 '24 09:10 olegz

In my sample program, I remove my custom CustomMessageMarshallingConverter, an exception occurs

expectation "expectNext(MyBean(string=foo))" failed (expected value: MyBean(string=foo); actual value: [B@20cccf6)

no deserialization converter is applied. Did I miss something? @olegz

cooperlyt avatar Oct 28 '24 17:10 cooperlyt

@cooperlyt Indeed there was an issue in spring-cloud-function 4.1.3 with the way we create ObjectMapper that is used by JsonMessageConverter, but it has been addressed, so please upgrade your dependencies to

extra["springCloudVersion"] = "2023.0.4-SNAPSHOT"

. . . and you should be fine. I just tested it with your project

olegz avatar Oct 29 '24 10:10 olegz

@cooperlyt we'll have a 4.1.4 release on November 12

olegz avatar Oct 29 '24 11:10 olegz

@cooperlyt Please test with the version shared by Oleg and confirm I can close this PR unmerged.

sdeleuze avatar Oct 29 '24 14:10 sdeleuze

Thanks I tested the new version, and it works well. Since the issue has been resolved, my PR is no longer needed. Please close it.

cooperlyt avatar Oct 29 '24 23:10 cooperlyt