Change TokenEndpoint renderer to View #25

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@jricher
Contributor

jricher commented Apr 5, 2012

Cleaned up version of previous pull request. Caching of output now turned off via the MappingJacksonJsonView bean configuration, to be spec compliant. Vestigial code removed.

Unit tests still not in here, but if this code works well enough we'll submit them in a separate pull request.

@dsyer

This comment has been minimized.

Show comment Hide comment
@dsyer

dsyer Apr 17, 2012

Owner

I'm not totally in love with this yet, even though there are no serious test failures (I had to modify the one that checks the cache headers). There's no support out of the box for XML formatting (if the client asks for "Accept: application/xml") unless I missed something. Can you see a nice way to provide that?

Or maybe even make this viewName controller optional, and make the old style the default? We could have a single method that does the business logic, and two controllers (the old and the new) either (but not both) of which can be instantiated by choice.

Owner

dsyer commented Apr 17, 2012

I'm not totally in love with this yet, even though there are no serious test failures (I had to modify the one that checks the cache headers). There's no support out of the box for XML formatting (if the client asks for "Accept: application/xml") unless I missed something. Can you see a nice way to provide that?

Or maybe even make this viewName controller optional, and make the old style the default? We could have a single method that does the business logic, and two controllers (the old and the new) either (but not both) of which can be instantiated by choice.

@jricher

This comment has been minimized.

Show comment Hide comment
@jricher

jricher Apr 17, 2012

Contributor

Since this returns a view name, and not a view object, it's up to a view name resolver to determine which view to use to do the actual rendering. One of the simplest is to use a BeanNameViewResolver, which this code instantiates in the definition parser in this patch, but there are other ways to go about it. Spring has a built-in ContentNegotiatingViewNameResolver for handling just that; you'd just have to wire it in to the right places and get the views attached to it in the right spots. (Side note: One issue I've had with the CNVNR in the past is that it tries to resolve everything that comes to it, which makes it impossible to use alongside of JSPs without some extra tooling to keep it in its place. But it does handle the accept headers and form parameters for view types very well for you.)

The main thing is that the view name can be mapped to an arbitrary View object based on some other code and functionality in the future.

Contributor

jricher commented Apr 17, 2012

Since this returns a view name, and not a view object, it's up to a view name resolver to determine which view to use to do the actual rendering. One of the simplest is to use a BeanNameViewResolver, which this code instantiates in the definition parser in this patch, but there are other ways to go about it. Spring has a built-in ContentNegotiatingViewNameResolver for handling just that; you'd just have to wire it in to the right places and get the views attached to it in the right spots. (Side note: One issue I've had with the CNVNR in the past is that it tries to resolve everything that comes to it, which makes it impossible to use alongside of JSPs without some extra tooling to keep it in its place. But it does handle the accept headers and form parameters for view types very well for you.)

The main thing is that the view name can be mapped to an arbitrary View object based on some other code and functionality in the future.

@dsyer

This comment has been minimized.

Show comment Hide comment
@dsyer

dsyer Apr 17, 2012

Owner

OK, I believe that in principle. I would like to see it in practice since it's not about "in the future" - I don't want to regress to a state that doesn't support simultaneous JSON and XML rendering. If CNVR is part of the solution here then I'm probably a little uncomfortable with the hard-coded BeanNameViewResolver that we are adding in this patch. It seems like having 2 controllers might not be a bad idea - one that works out of the box, and one that requires a view resolver to be configured by the user.

Aside: I don't believe that the ContentNegotiatingViewResolver requires anything fancy to render JSPs (it wouldn't be very popular if it did). I'm sure there are some pretty standard usages of it, and I would be happy to recommend them to SECOAUTH users, but preferably as an optional customization.

Owner

dsyer commented Apr 17, 2012

OK, I believe that in principle. I would like to see it in practice since it's not about "in the future" - I don't want to regress to a state that doesn't support simultaneous JSON and XML rendering. If CNVR is part of the solution here then I'm probably a little uncomfortable with the hard-coded BeanNameViewResolver that we are adding in this patch. It seems like having 2 controllers might not be a bad idea - one that works out of the box, and one that requires a view resolver to be configured by the user.

Aside: I don't believe that the ContentNegotiatingViewResolver requires anything fancy to render JSPs (it wouldn't be very popular if it did). I'm sure there are some pretty standard usages of it, and I would be happy to recommend them to SECOAUTH users, but preferably as an optional customization.

@jricher

This comment has been minimized.

Show comment Hide comment
@jricher

jricher Apr 17, 2012

Contributor

OK, two controllers might make sense here, so long there was an easy way to switch between them at configuration time. I wasn't particularly comfortable with hardcoding the BeanNameViewResolver into the config parser either, but that was the only way I could think of to make this work out of the box with the existing configuration and parser setup. I think that we want people to be able to use the same style configuration file that's in there today to get a simple configuration, but that with a couple additions you could get extended or customized functionality. I also didn't want to duplicate the rest of the functionality in different controllers.

Aside: The problems I've had in the past with CNVR stem from it being set up with a "default" view. Since the InternalResourceViewResovler also has a "default" view, both of these were vying for the right to serve all view names. IRVR has the added problem of not knowing whether or not its resources actually exist until after it's been served. I dug around the Internet but wasn't able to find a clean way around it so I built a compound view resolver that dispatched based on view name prefixes. It's entirely plausible that I was just Doing It Wrong, and I'd be glad to see a better solution. Is there a way to bind a ViewResolver to a particular controller? I'm not aware of one, but it would help with the configuration here because we could limit it to a particular subset.

Contributor

jricher commented Apr 17, 2012

OK, two controllers might make sense here, so long there was an easy way to switch between them at configuration time. I wasn't particularly comfortable with hardcoding the BeanNameViewResolver into the config parser either, but that was the only way I could think of to make this work out of the box with the existing configuration and parser setup. I think that we want people to be able to use the same style configuration file that's in there today to get a simple configuration, but that with a couple additions you could get extended or customized functionality. I also didn't want to duplicate the rest of the functionality in different controllers.

Aside: The problems I've had in the past with CNVR stem from it being set up with a "default" view. Since the InternalResourceViewResovler also has a "default" view, both of these were vying for the right to serve all view names. IRVR has the added problem of not knowing whether or not its resources actually exist until after it's been served. I dug around the Internet but wasn't able to find a clean way around it so I built a compound view resolver that dispatched based on view name prefixes. It's entirely plausible that I was just Doing It Wrong, and I'd be glad to see a better solution. Is there a way to bind a ViewResolver to a particular controller? I'm not aware of one, but it would help with the configuration here because we could limit it to a particular subset.

@dsyer

This comment has been minimized.

Show comment Hide comment
@dsyer

dsyer Apr 12, 2013

Owner

This was an interesting discussion, but I don't think it's going to lead to a merge. HttpMessageConverter should be fine for rendering machine responses, and we've all been using it now for a while. If you want to revisit, just ping me.

Owner

dsyer commented Apr 12, 2013

This was an interesting discussion, but I don't think it's going to lead to a merge. HttpMessageConverter should be fine for rendering machine responses, and we've all been using it now for a while. If you want to revisit, just ping me.

@dsyer dsyer closed this Apr 12, 2013

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