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

#1061 - Reenable default codecs for WebFlux configuration. #1064

Closed
wants to merge 1 commit into from

Conversation

gregturn
Copy link
Contributor

@gregturn gregturn commented Sep 4, 2019

Spring WebFlux now correctly handles custom codecs. This means we can stop disabling them. This change includes more test cases, verifying Spring MVC configuration for hypermedia as well, verifying both Web MVC and WebFlux are properly and consistently configured.

Probably supercedes: #1047

Spring WebFlux now correctly handles custom codecs. This means we can stop disabling them. This change includes more test cases, verifying Spring MVC configuration for hypermedia as well, verifying both Web MVC and WebFlux are properly and consistently configured.

Probably supercedes: #1047
@gregturn gregturn requested a review from odrotbohm September 4, 2019 19:47
@gregturn
Copy link
Contributor Author

gregturn commented Sep 4, 2019

Feedback welcome @wilkinsona.

@gregturn gregturn self-assigned this Sep 4, 2019
@gregturn gregturn added this to the 1.0.0.RC2 milestone Sep 4, 2019
@gregturn gregturn added in: configuration Configuration and setup stack: webflux labels Sep 4, 2019
@wilkinsona
Copy link
Member

Thanks for the fix, @gregturn. +1 for leaving WebFlux's default codecs enabled as disabling them makes WebFlux very cumbersome to use for any endpoints that aren't using Spring HATEOAS.

I am less sure about the fallback to Jackson, even if it is now consistent across MVC and WebFlux. For a request that accepts, say, application/vnd.amundsen-uber+json, it seems wrong to respond with plain JSON that, while legal JSON, is in the wrong format for the requested media type.

I discussed this a bit with @bclozel today and he remarked that it's an interesting use case and one that Framework would like to support. It may be too late in the schedule now for Framework 5.2 and HATEOAS 1.0, but perhaps there's something that can be done without any Framework changes.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 4, 2019

If be curious what the basis is for the wild card matcher? Sounds like it may have been put in to catch a particular situation that is now actually an issue.

@wilkinsona
Copy link
Member

I don't know why it was added originally, but I do know it's useful for any endpoint that returns an application/*+json response that doesn't require any customization of Jackson to serialise correctly. Boot's Actuator is one example.

@wilkinsona
Copy link
Member

It occurred to me overnight that there have been number of Boot issues (spring-projects/spring-boot#2147 is a good starting point) related to this where users expected a request for application/json to return a HAL-formatted response but it did not. That expectation seems reasonable to me as application/hal+json is a form of application/json. The inverse, however, is not the case which is the problem with the fallback for which a test is added in the pull request.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 5, 2019

I think the scope of the issue is limited because I don’t think we have a lot of people asking for, say, Collection+JSON where it wasn’t registered. Which means we can mend that in the next versions.

That being said, my inclination is to remove the wildcard matcher and to instead register handlers as needed. For Boot’s Actuator, you could register one using HATEOAS’s SPI for custom media types.

And when HATEOAS is NOT on the classpath, well you’d have to register that same handler directly with Spring Framework.

This connects with #1052

@wilkinsona
Copy link
Member

I really don't think HATEOAS should do that. While we could do something in Boot's Actuator to work around the breaking change, it would also be a breaking change for any user application with similar endpoints.

@bclozel
Copy link
Member

bclozel commented Sep 5, 2019

I'm not sure I'm following the whole discussion, but the "application/*+json" MIME type is registered by default in Spring Framework because many applications are using a custom type for API versioning. For example:

curl -H "Accept: application/vnd.initializr.v2.1+json" https://start.spring.io

I don't think we should disable that behavior in any case.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 5, 2019

Since the discussion about handling application/**-json is moving off topic of this one, do we need to continue it elsewhere? Or consider it resolved for now?

@wilkinsona
Copy link
Member

Let's move it elsewhere please. I don't think it can be considered resolved and something probably needs to be done before 1.0 GAs and reduces the options available to fix it.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 5, 2019

Resolved via fd70fd9.

@gregturn gregturn closed this Sep 5, 2019
@gregturn gregturn deleted the issue/default-codecs branch September 5, 2019 14:41
@gregturn
Copy link
Contributor Author

gregturn commented Sep 5, 2019

Thanks @wilkinsona @bclozel for your feedback. This issue is now resolved regarding Spring HATEOAS.

@wilkinsona
Copy link
Member

wilkinsona commented Sep 5, 2019

Is there a new HATEOAS issue to discuss the problems with application/*+json handling?

@gregturn
Copy link
Contributor Author

gregturn commented Sep 5, 2019

Frankly, I thought that was a Framework issue to discuss. ;)

@wilkinsona
Copy link
Member

There may be some changes needed in Framework but I think changes will also, if not exclusively, be needed in Spring HATEOAS. IMO, the problem should be tracked here until a plan of attack has been devised. At that point, a Framework issue can be opened if it's needed.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 5, 2019

See #1073 for the continuation of this broader topic. Whether or not this is a Framework issue or what can be found there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants