Skip to content

Experiment: Detect link methods that do NOT return resources #629

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
wants to merge 1 commit into from

Conversation

gregturn
Copy link
Contributor

@gregturn gregturn commented Sep 18, 2017

>>> DO NOT MERGE <<<

Detect endpoints that do NOT return classes assignable to ResourceSupport.

Question: Do we post a warning, or actually break?

@gregturn
Copy link
Contributor Author

@olivergierke I wrote some code that detects if the Spring MVC endpoints return type is NOT a ResourceSupport/Resource/Resources type, because it will probably yield the wrong JSON. I've seen a couple issues recently that track back to improperly built endpoints.

In this coding, it logs a warning AND throws a RuntimeException. I imagine the warning is good, but perhaps we need to drop the exception? Or perhaps we need an option to toggle the exception?

This looks like a keen mechanism to warn users that they've misconstructed their controllers.

Detect endpoints that do NOT return classes assignable to `ResourceSupport`.

Question: Do we post a warning, or actually break?
@gregturn gregturn force-pushed the feature/non-resources branch from 606d9b2 to 36a58a7 Compare September 18, 2017 20:03
@odrotbohm
Copy link
Member

odrotbohm commented Sep 19, 2017

@olivergierke I wrote some code that detects if the Spring MVC endpoints return type is NOT a ResourceSupport/Resource/Resources type, because it will probably yield the wrong JSON.

"Wrong JSON"? What problem are we actually solving here? When creating links, we must not care whether the resource we point to is hypermedia enabled or not. How would I point to a controller method serving a PDF? Also, note that it's very common to use HttpEntity<?> or ResponseEntity<?> as wrapper for the actual payloads.

@gregturn
Copy link
Contributor Author

The problem is that some users of Spring HATEOAS have controllers returning List<Resource<Item>>, which the HAL serializers won't render as valid JSON. These people should return Resources<Resource<Item>> instead (or some other type that is either ResourceSupport, Resource, or Resources).

By catching these scenarios, we'll stop getting issues opened like #532, that turned out to simply be a bad return type for Spring HATEOAS.

@gregturn
Copy link
Contributor Author

I think part of this is due to ResourceAssemblerSupport having toResources returning a List (an issue patched by #591), and people using that as their controller return type.

Seems like a simple thing we can detect and either warn or fail fast for the user.

@odrotbohm
Copy link
Member

Hm, I see. Maybe I am just not getting why this has anything to do with link creation? These are completely orthogonal issues, right? Wrapping an HttpEntity into a Resource is completely backwards as if at all, the Resource is the payload of the HttpEntity. But then again, there's no reason we should remove the ability to point to arbitrary controller methods.

The problem of List<Resource<Item>> still persists even with that change. If you don't point to that controller method it will still be used and render the "invalid" JSON. TBCH, that's not even invalid, because it might be a deliberate decision to simply render an array of Resource instances so that the individual elements carry hypermedia elements, but the wrapping array does not. So there are very valid reasons to use both variants. Maybe it's just a matter of documenting this aspect slightly better?

@gregturn
Copy link
Contributor Author

The reason I picked link creation as the point of detection is because it appeared the only place to sniff out the return type of a Spring MVC controller.

Well perhaps my check isn't sophisticated enough. If it's a Spring MVC response container, check its payload type?

And your point about being intentionally done by the end user is why I speculated this might best be a logged warning, not an exception, perhaps with an option to disable?

When I say option to disable, perhaps an annotation that signals "I know what I'm doing" on the endpoint? @SupressResourceValidation or something?

@gregturn
Copy link
Contributor Author

Related to #690

@gregturn
Copy link
Contributor Author

Closed in light of #416.

@gregturn gregturn closed this Dec 20, 2018
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.

2 participants