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

#1074 - Add option to register additional media types for HAL and HAL-FORMS. #1077

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gregturn
Copy link
Contributor

Inside HalConfiguration and HalFormsConfiguration, add a "wither" to support registering additional mediatypes. This makes it possible for Spring Boot to create a bean that will yield HAL when clients ask for application/json.

NOTE: This is NOT implemented in Collection+JSON, UBER, or any other mediatypes, and has not been captured as an interface that all custom mediatypes would pick up. So far, it's custom for HAL and HAL-FORMS. If there is a big draw, we can look at extracting some type of interface. But for now, community feedback would be nice before going down that road.

…-FORMS.

Inside HalConfiguration and HalFormsConfiguration, add a "wither" to support registering additional mediatypes. This makes it possible for Spring Boot to create a bean that will yield HAL when clients ask for application/json.

NOTE: This is NOT implemented in Collection+JSON, UBER, or any other mediatypes, and has not been captured as an interface that all custom mediatypes would pick up. So far, it's custom for HAL and HAL-FORMS. If there is a big draw, we can look at extracting some type of interface. But for now, community feedback would be nice before going down that road.
@gregturn gregturn requested a review from odrotbohm September 16, 2019 15:26
@gregturn
Copy link
Contributor Author

@wilkinsona Perhaps you'd better care to review it here?

@wilkinsona
Copy link
Member

Thanks, Greg. This seems to work nicely for Spring Boot's purposes with MVC: wilkinsona/spring-boot@3522870. Unfortunately, I have not yet had time to test it with WebFlux due to the ongoing problems there (#1080).

@wilkinsona
Copy link
Member

Any chance of this landing in snapshots soon? If it lands at the last minute, the couple of days between HATEOAS's release and Boot's release is likely to be filled with work picking up everyone's releases and the updates to Boot's HATEOS auto-configuration won't make it in.

@odrotbohm
Copy link
Member

As indicated in the Slack conversation, I don’t think that this should be resolved on the HMC/Codec level as without further changes in MVC/WebFlux, their defaulting might cause invalid representations selected.

If – as suggested – the media type aliasing is handled by the core framework we don’t need Boot to reach out to all HMCs and tweak their configuration and the HMC implementations don’t need to expose API to do that in the first place.

@wilkinsona
Copy link
Member

Sorry, I missed that in the Slack conversation.

their defaulting might cause invalid representations

I don't think I understand why this is the case. What circumstances will lead to an invalid representation being served?

What I'd like to be able to do is to have application/hal+json served in response to a request that accepts application/json. That seems reasonable to me as HAL is JSON just in a specific format. Perhaps the solution here is too broad as it allows any media type to be added and that's what you mean by creating the possibility of an invalid representation being served?

We're a bit stuck between a rock and a hard place now. Without some sort of answer for #1074, I think we're going to run out of time and auto-configuration for Spring HATEOAS's WebFlux won't make it into Boot 2.2. The alternative is to rework the spring.hateoas.use-hal-as-default-json-media-type property to make it clear that it's servlet specific. Unfortunately, I've already spent all the time I had available and then some on getting to this point so that's likely to be a step too far.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 20, 2019

I don’t know. Seems like a simple option that allows Boot support.

  1. If/when Framework offers a better option we can deprecate this
  2. It doesn’t break anyone currently since it’s an opt-in choice.
  3. Pushes things forward in a positive way

I don’t want waiting for perfect to obstruct us from something that’s good.

@wilkinsona
Copy link
Member

I am inclined to agree, @gregturn. Your proposed changes don't do anything by default in Spring HATEOAS and Boot will have a property to control its behaviour. In the absence of a change in Framework – which isn't going to happen at this late stage in 5.2's development – this proposal or something similar feels like a reasonable and pragmatic compromise to me.

@odrotbohm
Copy link
Member

odrotbohm commented Sep 23, 2019

Again, I am not opposed to introduce the suggested changes if we end up deciding that that's necessary. After the Slack discussion, I don't think it is. And I don't think we should introduce API we might have to deprecate and eventually remove if it isn't necessary. Especially not if we're already in RC days before the scheduled GA release. There had been plenty of time, hypermedia on WebFlux is not the most pressing issue in the first place, i.e. I see no need to rush things in a few days before GA.

As far as I understood, there's some defaulting in Spring Framework, that adds a catch all path to requests for mediatypes matching application/*+json . Now imagine we have HAL FORMS (application/prs.hal-forms+json) registered but the client asks for Collection+JSON (application/vnd.collection+json). HAL Forms wouldn't be a direct match, but the fallback to a simple match to application/*+json would still select it and return HAL FORMS when the client asked for Collection JSON.

I think a first step we should take is to decide where the discussion is actually supposed to be held because we're now between slack, a ticket and a PR and the conversation just gets dropped in the middle of it and is then taken over by others in a different spot so that there's quite a bit of work and overhead induced by reiteration, re-discussion. We shouldn't try to resolve this with with a "Oh yeah, let's merge this real quick.". I'm pretty sure an approach like that would never be an option for Boot itself. I don't see why we should act any different here.

Let's please step back to discuss #1074 and #1073 as they seem very related and get back to discuss implementation approaches when we have agreed on whether Spring HATEOAS is the place that actually is the place where the fix needs to be implemented.

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

Successfully merging this pull request may close these issues.

3 participants