Skip to content
This repository has been archived by the owner on Jul 19, 2020. It is now read-only.

Support View annotation on Controller methods returning non void types (except Viewable) #63

Closed
mvcbot opened this issue Aug 7, 2015 · 9 comments
Milestone

Comments

@mvcbot
Copy link

mvcbot commented Aug 7, 2015

Original issue MVC_SPEC-51 created by beryozkin_sergey:

@Controller
@View("book.jsp")
public Book getBook() {}
{code}

I.e, if it is non-void, non-Viewable response with View then the response is treated as the content to be processed by the view engine.
 
The simplest and safe way to bind Book to HTTP request attribute is to assume a convention that a class name (Book.class.getName()) is used as a key. 
Additionally (or alternatively) an extra optional attribute can be added to @View:
{code:java}
@Controller
@View(value="book.jsp", name="book")
public Book getBook() {}
{code}

Furthermore the above style can also support:

@Controller
@View("book.jsp")
@Produces({"application/xml", "application/json", "text/html"})
public Book getBook() {}

given that it is very typical in JAX-RS to have a single method with multiple Produces values.

I think this style is very important to support because it is more natural to write than having to use a @nAmed sync such as Greetings in the spec example or dealing with Models directly, in both cases the application directly having to be aware of the MVC semantics.

IMHO it is not difficult to support it at a spec level. It is also another good example why a strict CDI binding requirement may need to be relaxed and limited to a subset of styles.

@mvcbot
Copy link
Author

mvcbot commented Aug 7, 2015

Comment by beryozkin_sergey:

Please assign to Santiago

@mvcbot
Copy link
Author

mvcbot commented Aug 7, 2015

Comment by Santiago Pericas-Geertsen:

It is certainly possible to consider using @view this way. However, MVC is now "reserving" the return type for view (or Viewable). In fact, the current behavior in the latest spec is to call toString() on a returned object to get a view path. Having said that, perhaps there should be a new Viewable constructor that takes a name/model pair to avoid the need for Models in that case.

As for using a controller to return non-HTML representations, it's better to separate that into two: a resource and a controller at this point.

@mvcbot
Copy link
Author

mvcbot commented Aug 7, 2015

Comment by beryozkin_sergey:

The whole idea of this proposal is for users be able to avoid dealing directly with all the (JSP/etc) paths and MVC API (Viewable) directly. I'm not sure what is wrong with such an approach, at the very least it can be considered a reasonable design practice... I'm not against giving users a direct/fine-grained control but a less 'intrusive' option is not bad at all ?

You mentioned the current API calls toString() but specifying a rule that if @view is available on a method returning a type (non-void, non-Viewable) then it is a not a resource path but the content can work ? It would simply relax the current statement that View is ignored for non-void responses without introducing any contradiction into the text. It would integrate well with all the spec ?

Please ignore my reference to CDI here.

@mvcbot
Copy link
Author

mvcbot commented Aug 11, 2015

Comment by Christian Kaltepoth:

Let me explain the reason why I suggested to call toString() on all controller results which doesn't match any of the specified types (String, Viewable, etc). The reason for that was that this allows to implement typesafe ways of specifying which view to render. You could for example return enums from the controller and implement toString() accordingly to return the view corresponding to the specific enum value.

However, I also see that providing some other default behavior could make sense. What we are currently talking about is something like this, right?

{quote}
If there is a @View on the controller method/class and the controller method returns a result of some other type (!= Viewable, String, Response), the result is stored in the model (using some default key like model) and the view specified via @View is rendered.
{quote}

I see that this may also be a reasonable default behavior. BUT it basically prevents to implement typesafe navigation as I described before.

@mvcbot
Copy link
Author

mvcbot commented Aug 11, 2015

Comment by Santiago Pericas-Geertsen:

I generally like the suggestion, but I think we need to be careful not to add more complexity in the form of rules that developers would need to understand that don't really add new capabilities to the API. Sometimes adding more ways of doing something is not necessarily a good thing. Having said that, if we don't need name in @view, and use of a default name as suggested, that may be a little better.

@mvcbot
Copy link
Author

mvcbot commented Aug 11, 2015

Comment by Christian Kaltepoth:

I agree that we should avoid too much complexity.

If we add support for something like this, we shouldn't support customizing the name. I see no good reason for this feature. We should choose one of these approaches to generate the name:

  • Either use a fixed name like model
  • Or create a name from the class name using the same rules that are used by @Named by default.

@mvcbot
Copy link
Author

mvcbot commented Aug 11, 2015

Comment by beryozkin_sergey:

Hi Christian, Santiago

Thanks for the comments, I agree having an extra View name attribute adds to the complexity (it can always be reviewed later on if needed should users start asking).
Defaulting to a class name appears to be very safe. I hope users will like it in general, in addition to being able to do a fine-grained control of MVC flows

Cheers, Sergey

@mvcbot mvcbot added this to the 1.0-pr milestone Apr 26, 2017
@chkal chkal removed this from the 1.0-pr milestone Dec 18, 2017
@chkal chkal added this to the Future milestone Feb 11, 2018
@chkal
Copy link
Contributor

chkal commented Feb 11, 2018

I don't think that we can provide this feature in version 1.0. So moving this to "Future". But we should make sure that something like this can be implemented later without breaking backwards compatibility.

@chkal
Copy link
Contributor

chkal commented Jul 19, 2020

Moved to jakartaee/mvc#33

@chkal chkal closed this as completed Jul 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants