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

What is a proper response when an unregistered media type is request? #1073

Closed
gregturn opened this issue Sep 5, 2019 · 10 comments
Closed

Comments

@gregturn
Copy link
Contributor

gregturn commented Sep 5, 2019

If only HAL is registered via @EnableHypermediaSupport, and the client requests Collection+JSON via an Accept header, Spring Framework will fall back to it's handler for application/*-json.

Is this proper behavior? If not, what is?

This is a continuation of the discussion from #1064.

@gregturn
Copy link
Contributor Author

I’m not sure what the proper response here is.

Is the idea is to throw an exception when asking for a hypermedia type not registered but to fallback to that catchall handler?

Maybe HATEOAS needs a handler registered after all others that if it gets hit and the requested type is hypermedia mediatype it throw an exception. But if the requested mediatype is NOT one of ours it lets it go.

Not sure if any of this is blocking for 1.0 GA.

@wilkinsona
Copy link
Member

I'm not sure if it is either. My concern is that, depending on what the answer is, breaking changes may be required to implement it. It feels like the sort of thing that should be resolved before the restrictions of reaching 1.0 come into play.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 18, 2019

When I register this Encoder after all others, it properly fails if you ask for an unregistered Spring HATEOAS media type.

private static class NonEncoder implements Encoder<Object> {

	private final List<MimeType> hypermediaMediaTypes;

	public NonEncoder(List<MimeType> hypermediaMediaTypes) {
		this.hypermediaMediaTypes = hypermediaMediaTypes;
	}

	@Override
	public boolean canEncode(ResolvableType elementType, MimeType mimeType) {
		return mimeType == null
				|| hypermediaMediaTypes.stream().anyMatch(mediaType -> mediaType.isCompatibleWith(mimeType));
	}

	@Override
	public Flux<DataBuffer> encode(Publisher<?> inputStream, DataBufferFactory bufferFactory,
			ResolvableType elementType, MimeType mimeType, Map<String, Object> hints) {
		return Flux.error(new MediaTypeNotSupportedStatusException(mimeType + " support was never registered!"));
	}

	@Override
	public DataBuffer encodeValue(Object value, DataBufferFactory bufferFactory, ResolvableType valueType, MimeType mimeType, Map<String, Object> hints) {
		throw new MediaTypeNotSupportedStatusException(mimeType + " support was never registered!");
	}

	@Override
	public List<MimeType> getEncodableMimeTypes() {
		return hypermediaMediaTypes;
	}
}

@wilkinsona
Copy link
Member

The javadoc for canEncode says that it returns "whether the encoder supports the given source element type and the MIME type". Returning true and then failing in encode and encodeValue because the encoder does not, in fact, support the MIME type doesn't seem to honour that contract.

@gregturn
Copy link
Contributor Author

I want it to match if the MimeType is one of our hypermedia mime types. But then blow up on encoding.

This is the one after the “real” handlers. Hence, if you register UBER and then request HAL, it is skipped by UBER but picked up by this one, and generates the exception.

Probably need a better name to communicate it’s purpose. And need a decoder equivalent. And probably an MVC equivalent.

@gregturn
Copy link
Contributor Author

If you ask for something else like Actuator’s MimeType, that should bypass this extra encoder too.

@wilkinsona
Copy link
Member

wilkinsona commented Sep 18, 2019

I understand the purpose, but it feels like an abuse of the Encoder contract to me. I'm concerned about unwanted side-effects that may occur in a large app. I don't think that you can guarantee that your Encoder will always be last. If another Encoder is added after it that can handle a hypermedia MIME type, the encoder proposed here will prevent it from being called.

Have you talked to anyone on the Framework team about this? I know that @bclozel was interested in the use case, for example.

@gregturn
Copy link
Contributor Author

gregturn commented Sep 18, 2019

No i haven’t chatted with them directly.

If it’s this risky I doubt the ability to add this to 1.0.

@bclozel
Copy link
Member

bclozel commented Sep 18, 2019

In general, Spring Framework with the following rules:

  • a codec should be registered against a MIME type only if can actually handle it (so canEncode should only return true if, besides an unpredictable runtime error happens, it should handle the encoding properly).
  • content negotiation is handled in a way that the Framework selects the best available media type (if any compatible), or responds with an HTTP client error

I agree that the registered application/*+json media type in Framework can be an issue. This registration is generally useful because developers can use custom media types and add meaning to the representation (versioning for example). Technically, media types with the +json suffix mean that they can be processed as regular JSON. It's a fallback, and clients should fail if this is not something they can process.

In any case, if a client asks for application/something+json to a Spring web application, the application might respond with application/json, even if Spring HATEOAS is not involved at all. I think we should keep this behavior consistent and only reconsider this in Framework if necessary.

@gregturn
Copy link
Contributor Author

Based on all comments I’m closing this as works as designed.

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

No branches or pull requests

3 participants