Skip to content

HATEOAS: ResponseEntity<Resource> from @ExceptionHandler method not converted to HAL #8174

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

Closed
mvitz opened this issue Feb 2, 2017 · 16 comments
Labels
for: external-project For an external project and not something we can fix status: declined A suggestion or change that we don't feel we should currently apply

Comments

@mvitz
Copy link
Contributor

mvitz commented Feb 2, 2017

Given a controller like https://github.com/mvitz/spring-boot-hateoas-autoconfiguration-bug/blob/master/src/main/java/de/mvitz/spring/hateoas/server/ExceptionController.java with a @ExceptionHandler annotated method the response is not converted to valid HAL, see:

curl -i localhost:8080/exception -HAccept:application/hal+json
HTTP/1.1 400
Content-Type: application/hal+json;charset=UTF-8
Transfer-Encoding: chunked
Date: Thu, 02 Feb 2017 00:05:02 GMT
Connection: close

{"links":[{"rel":"self","href":"http://google.de"}],"message":"Foo"}

Maybe I am (again) missing some little thing here but I think given the example controller I should expect to receive a response with valid HAL.

Issue occurs with Spring-Boot 1.4.4.RELEASE and 1.5.1.RELEASE as well.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 2, 2017
@bclozel
Copy link
Member

bclozel commented Feb 2, 2017

I'm not sure this is supported by Spring HATOAS in the first, nor that representing an exception instance as a HAL resource makes sense. As far as I know, Boot does nothing special in that area.

Could you open this issue against the original project? (actually, is this a duplicate?)
Thanks!

@bclozel bclozel closed this as completed Feb 2, 2017
@bclozel bclozel added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 2, 2017
@snicoll
Copy link
Member

snicoll commented Feb 2, 2017

@mvitz I second that, please consider creating an issue in the original project first unless you have a strong indication that this is auto-configuration related.

@mvitz
Copy link
Contributor Author

mvitz commented Feb 2, 2017

@snicoll for me (and I would consider myself as an advanced spring user) it's not always obvious which component is responsible for such stuff.

As a user I would expect that if I use the HATEOAS auto configuration and HAL is working from controller methods it works in an exception handler method as well. I never expected that this issue would be outside of configuration.

Using HATEOAS without Boot requires me to Annotate some Configuration with @EnableHypermedia and after that everything works. How am I supposed to not think this has something to do with auto configuration.

@blcozel
I had the same thought "does it make sense" but I think that should be the decision of the applications developer

I'm sorry if I cause additional work for this team but I think you should see this as a positive thing. I'm trying to help and improve your work and do not stop using it silently. Additionally I would like to thank you for all the good work.

@snicoll
Copy link
Member

snicoll commented Feb 2, 2017

We obviously see your (and any report in genral) as a positive thing. But it isn't obvious from your report that this is working without Spring Boot. Actually you've done all that work but you didn't mention it!

@snicoll snicoll reopened this Feb 2, 2017
@snicoll snicoll added status: waiting-for-triage An issue we've not yet triaged and removed status: invalid An issue that we don't feel is valid labels Feb 2, 2017
@bclozel
Copy link
Member

bclozel commented Feb 2, 2017

@mvitz

Using HATEOAS without Boot requires me to Annotate some Configuration with @EnableHypermedia and after that everything works.

Do you mean that you got this very use case to work when using the @EnableHypermedia annotation? In that case, @snicoll is right, this could be indeed a Spring Boot issue.

@mvitz
Copy link
Contributor Author

mvitz commented Feb 2, 2017

Yes. If I add @EnableHypermedia this use case works. After some digging it this works because with the annotation the main Jackson object mapper is changed. With only the autoconfiguration present an additional mapper is created.

@wilkinsona
Copy link
Member

I've reproduced the problem, and also reproduced it working when adding @EnableHypermedia. Thanks for the sample, @mvitz.

@wilkinsona wilkinsona added priority: normal type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 2, 2017
@wilkinsona wilkinsona added this to the 1.4.5 milestone Feb 2, 2017
@wilkinsona
Copy link
Member

wilkinsona commented Feb 2, 2017

When you use @EnableHypermedia, it registers its _halObjectMapper bean before Boot has auto-configured its ObjectMapper. This switches off the ObjectMapper auto-configuration and _halObjectMapper becomes the context's one and only ObjectMapper. It's this ObjectMapper that's then used by the auto-configured MappingJackson2HttpMessageConverter so all JSON responses are using the HAL ObjectMapper.

If you don't use @EnableHypermedia, then JacksonAutoConfiguration gets a chance to create its @Primary ObjectMapper and it's this one, that knows nothing about HAL, that's used by the auto-configured MappingJackson2HttpMessageConverter. Generally speaking, this isn't a problem as Spring HATEOAS's Jackson2ModuleRegisteringBeanPostProcessor post-processes all RequestMappingHandlerAdapter beans and customises their HTTP message converters, adding a TypeConstrainedMappingJackson2HttpMessageConverter that does understand HAL to the start of the list.

It doesn't work for the error response in the sample as the ExceptionHandlerExceptionResolver that's used hasn't had the above-described conversion applied to its HTTP message converters. I'll see if there's something we can do to work around this in Spring Boot, but I think this really ought to be fixed in Spring HATEOAS as you could easily get into a similar situation without using Boot.

/cc @olivergierke

@wilkinsona
Copy link
Member

There's no easy way that I can see for us to create or lay our hands on a TypeConstrainedMappingJackson2HttpMessageConverter to add to the list of converters used by the ExceptionHandlerExceptionResolver. Unfortunately that means that this needs to be fixed in Spring HATEOAS and it is, I think, a duplicate of the issue that @bclozel linked to above.

@olivergierke Can you please take a look at this?

@mvitz
Copy link
Contributor Author

mvitz commented Feb 2, 2017

Thanks for all your work up to this point.

@wilkinsona wilkinsona added the status: blocked An issue that's blocked on an external project change label Feb 8, 2017
@odrotbohm
Copy link
Member

I think we're gonna have to investigate how to register the HttpMessageConverters in a way that the Spring MVC infrastructure picks them up more globally rather than post processing already existing beans. In case of HandlerExceptionResolver they're actually buried in a composite implementation. I guess we have to take a step back and take a fresh look to rethink the way we register the HttpMessageConverters we want to deploy.

Spring HATEOAS is on Spring 4.3 in the meantime and quite a few new MVC customization APIs have been introduced since we started that rather convoluted implementation. I think we might be able to get away with something dramatically simpler and less error prone.

It might also mean that a bit of the setup in Spring Data REST has to be revisited as part of that slightly weird design (inspecting existing beans and leniently registering HMARs in them) is to make sure Spring Data REST can actually extend and replace a couple of components set up in Spring HATEOAS. I have a call scheduled with @rstoyanchev early next week. After that we probably have a clearer picture of what we can do for Spring Data REST 2.0 and then adapt Spring HATEOAS accordingly.

@philwebb philwebb modified the milestones: 1.4.6, 1.4.5 Mar 1, 2017
@snicoll
Copy link
Member

snicoll commented Apr 7, 2017

@olivergierke where is this going to be implemented. I am asking this issue is flagged for 1.4.x atm.

@philwebb
Copy link
Member

philwebb commented Mar 21, 2018

It looks like we can't fix this without a Spring HATEOAS change. I'm going to close this one for now. If we get an update in Spring HATEOAS we'll reconsider.

@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed priority: normal status: blocked An issue that's blocked on an external project change type: bug A general bug labels Mar 21, 2018
@wilkinsona wilkinsona added the for: external-project For an external project and not something we can fix label Mar 21, 2018
@odrotbohm
Copy link
Member

We've just pushed some significant configuration changes to Spring HATEOAS (both on master as well on the bugfix branch for the upcoming 0.25.0) in the context of spring-projects/spring-hateoas#719 and spring-projects/spring-hateoas#723. I've adapted Spring Data REST to work with these changes (coming in Lovelace) and verified our Spring Data examples work with these changes on Boot 2.0.3. Find the details of what has changed in the tickets I linked above.

I'd like to encourage everyone who brought up issues in this ticket to try to upgrade to Spring HATEOAS 0.25.0.BUILD-SNAPSHOT (it doesn't contain any major changes but the ones just described), give it a spin and report problems you (still see) in spring-projects/spring-hateoas#719.

@mvitz
Copy link
Contributor Author

mvitz commented Jun 22, 2018

Thanks, when upgrading the example to Spring Boot 2.0.3 and Hateoas 0.25.0.BUILD-SNAPSHOT everything works as expected.

@odrotbohm
Copy link
Member

Thanks for confirming, Michael! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

7 participants