Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve HttpMessageConverters javadoc #15027

Closed
liudaomanbu opened this issue Oct 31, 2018 · 15 comments

Comments

Projects
None yet
5 participants
@liudaomanbu
Copy link

commented Oct 31, 2018

I use the spring-boot-starter-parent 1.5.17.RELEASE.

I create my config extends WebMvcConfigurerAdapter,and overwrite the extendMessageConverters method.

I see the List<HttpMessageConverter<?>> has two MappingJackson2HttpMessageConverters and two StringHttpMessageConverters.

@Configuration
public class TestConfig extends WebMvcConfigurerAdapter {

  @Override
  public void extendMessageConverters(List<HttpMessageConverter<?>> converters) {
    System.out.println(converters);
  }
}

image

I try to register my TestDeserializer to jackson.
But this is only the first one to take effect.

This is the first one.
image
This is the second one.
image

I think is maybe a bug?
If it is not a bug,someone can explain?

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

I try to debug the reason and found the order of production two MappingJackson2HttpMessageConverters.

Firstly, in org.springframework.boot.autoconfigure.web.JacksonHttpMessageConvertersConfiguration, the mappingJackson2HttpMessageConverter method create a new MappingJackson2HttpMessageConverter and register to Spring container.

@Configuration
class JacksonHttpMessageConvertersConfiguration {

	@Configuration
	@ConditionalOnClass(ObjectMapper.class)
	@ConditionalOnBean(ObjectMapper.class)
	@ConditionalOnProperty(name = HttpMessageConvertersAutoConfiguration.PREFERRED_MAPPER_PROPERTY, havingValue = "jackson", matchIfMissing = true)
	protected static class MappingJackson2HttpMessageConverterConfiguration {

		@Bean
		@ConditionalOnMissingBean(value = MappingJackson2HttpMessageConverter.class, ignoredType = {
				"org.springframework.hateoas.mvc.TypeConstrainedMappingJackson2HttpMessageConverter",
				"org.springframework.data.rest.webmvc.alps.AlpsJsonHttpMessageConverter" })
		public MappingJackson2HttpMessageConverter mappingJackson2HttpMessageConverter(
				ObjectMapper objectMapper) {
			return new MappingJackson2HttpMessageConverter(objectMapper);
		}

	}
//......
}

Secondly, in org.springframework.boot.autoconfigure.web.HttpMessageConvertersAutoConfiguration, the messageConverters method create a HttpMessageConverters object.

@Configuration
@ConditionalOnClass(HttpMessageConverter.class)
@AutoConfigureAfter({ GsonAutoConfiguration.class, JacksonAutoConfiguration.class })
@Import({ JacksonHttpMessageConvertersConfiguration.class,
		GsonHttpMessageConvertersConfiguration.class })
public class HttpMessageConvertersAutoConfiguration {
	//......
	@Bean
	@ConditionalOnMissingBean
	public HttpMessageConverters messageConverters() {
		return new HttpMessageConverters((this.converters != null) ? this.converters
				: Collections.<HttpMessageConverter<?>>emptyList());
	}
	//......
}

And the constructor param this.converters has a StringHttpMessageConverter object and the last MappingJackson2HttpMessageConverter object.

Thirdly, in org.springframework.boot.autoconfigure.web.HttpMessageConverters constructor, excute the getDefaultConverters method get a List that has a new StringHttpMessageConverter object and a new MappingJackson2HttpMessageConverter object.
And the getCombinedConverters not remove the repeated objects.

public class HttpMessageConverters implements Iterable<HttpMessageConverter<?>> {
	public HttpMessageConverters(boolean addDefaultConverters,
			Collection<HttpMessageConverter<?>> converters) {
		List<HttpMessageConverter<?>> combined = getCombinedConverters(converters,
				addDefaultConverters ? getDefaultConverters()
						: Collections.<HttpMessageConverter<?>>emptyList());
		combined = postProcessConverters(combined);
		this.converters = Collections.unmodifiableList(combined);
	}
}

image
image

I think the problem may be in this getCombinedConverters method.

	private List<HttpMessageConverter<?>> getCombinedConverters(
			Collection<HttpMessageConverter<?>> converters,
			List<HttpMessageConverter<?>> defaultConverters) {
		List<HttpMessageConverter<?>> combined = new ArrayList<HttpMessageConverter<?>>();
		List<HttpMessageConverter<?>> processing = new ArrayList<HttpMessageConverter<?>>(
				converters);
		for (HttpMessageConverter<?> defaultConverter : defaultConverters) {
			Iterator<HttpMessageConverter<?>> iterator = processing.iterator();
			while (iterator.hasNext()) {
				HttpMessageConverter<?> candidate = iterator.next();
				if (isReplacement(defaultConverter, candidate)) {
					combined.add(candidate);
					iterator.remove();
				}
			}
			combined.add(defaultConverter);
			if (defaultConverter instanceof AllEncompassingFormHttpMessageConverter) {
				configurePartConverters(
						(AllEncompassingFormHttpMessageConverter) defaultConverter,
						converters);
			}
		}
		combined.addAll(0, processing);
		return combined;
	}

This method use the isReplacement method in double cycle to check repeated objects.
But the code don't remove the repeated objects.
For repeated object,the combined first and the exist converter,then add the repeated default converter object.

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

demo.zip
This is the problem demo code.

@bclozel bclozel self-assigned this Oct 31, 2018

@bclozel

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Hello @liudaomanbu,

I think this is by design, see the HttpMessageConverters javadoc:

/**
 * Create a new {@link HttpMessageConverters} instance with the specified converters.
 * @param addDefaultConverters if default converters should be added
 * @param converters converters to be added. Items are added just before any default
 * converter of the same type (or at the front of the list if no default converter is
 * found) The {@link #postProcessConverters(List)} method can be used for further
 * converter manipulation.
 */

There's even a test for this exact behavior. I'm still marking that for team-attention as I'd like to know if there are things we should improve in this area.

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

It is indeed by design. See #1293 for details. One thing that we should do is clarify the javadoc on HttpMessageConverters as one constructor is currently misleading:

/**
* Create a new {@link HttpMessageConverters} instance with the specified additional
* converters.
* @param additionalConverters additional converters to be added. New converters will
* be added to the front of the list, overrides will replace existing items without
* changing the order. The {@link #getConverters()} methods can be used for further
* converter manipulation.
*/
.

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

It is indeed by design. See #1293 for details. One thing that we should do is clarify the javadoc on HttpMessageConverters as one constructor is currently misleading:

spring-boot/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/http/HttpMessageConverters.java

Lines 70 to 77 in 228e054

/**

  • Create a new {@link HttpMessageConverters} instance with the specified additional
  • converters.
  • @param additionalConverters additional converters to be added. New converters will
  • be added to the front of the list, overrides will replace existing items without
  • changing the order. The {@link #getConverters()} methods can be used for further
  • converter manipulation.
    */
    .

Thank you for your answer,I think the javadoc on HttpMessageConverters shoule be fixed.
My understanding of it is that after replace,the default converter should be remove.

But I also have question about the design.
When parsing parameters,only first matched HttpMessageConverter can parsing.
Even if the first matched HttpMessageConverter fails, other matched HttpMessageConverters not get the chance to parsing parameters.
So ,I think the same type HttpMessageConverter have multiple objects is meaningless

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Hello @liudaomanbu,

I think this is by design, see the HttpMessageConverters javadoc:

/**
 * Create a new {@link HttpMessageConverters} instance with the specified converters.
 * @param addDefaultConverters if default converters should be added
 * @param converters converters to be added. Items are added just before any default
 * converter of the same type (or at the front of the list if no default converter is
 * found) The {@link #postProcessConverters(List)} method can be used for further
 * converter manipulation.
 */

There's even a test for this exact behavior. I'm still marking that for team-attention as I'd like to know if there are things we should improve in this area.

Thank you for your answer.

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Even if the first matched HttpMessageConverter fails, other matched HttpMessageConverters not get the chance to parsing parameters.
So ,I think the same type HttpMessageConverter have multiple objects is meaningless

It's only meaningless if an earlier HttpMessageConverter will always match (return true from canRead or canWrite) and therefore prevent a latter converter from being called. That isn't always the case. Different MappingJackson2HttpMessageConverter instances may be configured with different supported media types or may even be subclassed to implement custom canRead or canWrite behaviour. This need to fall back to the default converters was overlooked originally and is the problem that is described in and fixed by #1293.

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Even if the first matched HttpMessageConverter fails, other matched HttpMessageConverters not get the chance to parsing parameters.
So ,I think the same type HttpMessageConverter have multiple objects is meaningless

It's only meaningless if an earlier HttpMessageConverter will always match (return true from canRead or canWrite) and therefore prevent a latter converter from being called. That isn't always the case. Different MappingJackson2HttpMessageConverter instances may be configured with different supported media types or may even be subclassed to implement custom canRead or canWrite behaviour. This need to fall back to the default converters was overlooked originally and is the problem that is described in and fixed by #1293.

Thanks.

@bclozel

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

We're going to improve the javadoc in some places (and maybe the reference doc as well).

@liudaomanbu I'm not sure it would have helped in your case, it seems you went straight to debugging and got surprised by the current behavior. Could you elaborate a bit and explain what you were trying to achieve/configure in the first place? Did you want to configure a specific Jackson option maybe? If you could tell us more about your use case, maybe we could find a better solution here.

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

We're going to improve the javadoc in some places (and maybe the reference doc as well).

@liudaomanbu I'm not sure it would have helped in your case, it seems you went straight to debugging and got surprised by the current behavior. Could you elaborate a bit and explain what you were trying to achieve/configure in the first place? Did you want to configure a specific Jackson option maybe? If you could tell us more about your use case, maybe we could find a better solution here.

I debug for another bug at the beginning.

But the bug is related to HttpMessageConverter and HttpMessageConverters.
In my company framework that based on spring boot, a @Configuration new a HttpMessageConverters object and don't add the existing HttpMessageConverters that in Spring container.

The bug makes all methods to configure Jackson on the document invalid.

After I fix the bug,I found that MappingJackson2HttpMessageConverter is not singleton,and not all MappingJackson2HttpMessageConverters are applied configuration.

I worry that it is also a bug,and the javadoc makes me think it is really a bug.

So i continue to debug.

Your answer is very helpful to me, thank you

@liudaomanbu

This comment has been minimized.

Copy link
Author

commented Oct 31, 2018

Lastly,although wilkinsona's statement is very correct.
But from my point of view,i use the spring-boot-starter-parent framework.
Then the framework create two MappingJackson2HttpMessageConverter object,and always the first to be used.
I think this is strange and a bit puzzling.

@bclozel bclozel changed the title MappingJackson2HttpMessageConverter is not singleton? Improve HttpMessageConverters javadoc Nov 14, 2018

@bclozel bclozel closed this in d84421b Nov 14, 2018

@bclozel bclozel modified the milestones: 2.1.x, 2.1.1 Nov 14, 2018

@kevinjom

This comment has been minimized.

Copy link

commented Apr 18, 2019

Spring boot version: 2.1.4.RELEASE

I am also facing the similar issue, the configured ObjectMapper is not picked up by MappingJackson2HttpMessageConverter.

Then I tried to configure my own MappingJackson2HttpMessageConverter as the document here says I should be able to use my own.

I did a debug, and found the same issue with @liudaomanbu , the one I configured in the container is not the one Spring Boot (MVC) use. See pictures below (look at the object id)

This is the one in the IoC container:
image

This is the one that spring MVC uses to convert the object to response:
image

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@kevinjom Following the clarifications made in this issue, we believe that everything should be working as described in the reference documentation and javadoc. If you do not think that is the case, please open a new issue with a minimal sample that reproduces the behaviour that you are seeing and an description of the behaviour that you expected to see.

@kevinjom

This comment has been minimized.

Copy link

commented Apr 18, 2019

@wilkinsona thanks for replying, I might create a new issue with a demo codebase later. thanks

@kevinjom

This comment has been minimized.

Copy link

commented Apr 21, 2019

Issue found, it's because we are also using @EnableWebMvc, after removed it, the object mapper can be picked up by the conveter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.