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

f:metadata and f:view behavior #5105

Closed
volosied opened this issue May 26, 2022 · 16 comments
Closed

f:metadata and f:view behavior #5105

volosied opened this issue May 26, 2022 · 16 comments
Labels

Comments

@volosied
Copy link
Contributor

volosied commented May 26, 2022

Describe the bug

I've found implementation differences between Mojarra and MyFaces, and I'd like to know what behavior is correct?

The specification documents say that f:metadata "must be a child of the <f:view>". When it's not a child of f:view, should an exception be thrown?

Mojarra doesn't throw any exceptions, but MyFaces does.

jakarta.faces.view.facelets.TagException: /view01.xhtml at line 5 and column 19 <f:metadata> Parent UIComponent j_id_2 should be instance of UIViewRoot

Tested on Mojarra 3.0.0 and MyFaces 3.0.1.

To Reproduce

Steps to reproduce the behavior:

  1. Deploy both apps in Tomcat 10
  2. Go to http://localhost:8080/metadata-app-mojarra/view01.xhtml
  3. Go to http://localhost:8080/metadata-app-myfaces/view01.xhtml

Expected behavior

Should Mojarra throw a similar exception or should MyFaces ignore this exception? Or should the spec doc be updated?

Screenshots
MyFaces Exception:
image

Both wars attached.
Wars.zip

Thanks!

@volosied
Copy link
Contributor Author

Some previous discussion:

@melloware
Copy link
Contributor

I personally think its should throw an exception.

@codylerum
Copy link
Contributor

Definitely an exception over silently failing.

@pizzi80
Copy link
Contributor

pizzi80 commented May 27, 2022

+1 for exception

Questions for the future (probably must be discussed in Faces API)

  1. why f:metadata must be inside f:view?
    it would be techically possible to put it at the same level of h:head or h:body ?

  2. why f:view exists? if it's an "xhtml" (or any other mapped extension) page
    it's an f:view ... we'are using jsf, it's quite normal that every page is a "faces view"

@melloware
Copy link
Contributor

@pizzi80 f:viewis needed still so you can do statelessless which I use on all my 404, 403 pages etc. See: https://balusc.omnifaces.org/2013/02/stateless-jsf.html

@pizzi80
Copy link
Contributor

pizzi80 commented May 27, 2022

yes, all these attributes could be moved to an ipothetic html tag...

Probably the f:metadata has no sense at all in terms of JSF

As stated many time during the last 10+ years JSF is a framework generating HTML, for example:

JSF is a MVC framework generating HTML, not some kind of a REST web service framework.
You're essentially abusing JSF > as a web service. Your concrete problem is simply caused by placing tags and so on > in the view file yourself.
If you really insist, then you can always achieve this by using ui:composition instead of . You also need to make
sure that the right content type of application/json is been used, this defaults in JSF namely to text/html.

https://stackoverflow.com/questions/10982762/how-to-generate-json-response-from-jsf

In this way it would be possible to create a template or a page in a simpler and more linear way, for example:

<h:html xmlns:c="http://xmlns.jcp.org/jsp/jstl/core"
        xmlns:j="http://xmlns.jcp.org/jsf"
        xmlns:f="http://xmlns.jcp.org/jsf/core"
        xmlns:h="http://xmlns.jcp.org/jsf/html"
        xmlns:ui="http://xmlns.jcp.org/jsf/facelets"
        xmlns:pt="http://xmlns.jcp.org/jsf/passthrough"		
        xmlns:o="http://omnifaces.org/ui"
        lang="en" 
        locale="en" 
        transient="true" >

  <f:viewParam name="id"/>
  <f:importConstants type="org.omnifaces.util.Faces" />
  <c:set var="prod" value="#{conf.production}" scope="application" />

  <h:head>
    <title>Hello</title>
  </h:head>
  <h:body>
    <h1>Hello</h1>
  </h:body>
</h:html>

@melloware
Copy link
Contributor

@pizzi80 that sounds like a JSF spec change and we shouldn't pollute this specific topic with it since this is about the "current" spec and what we should do. You could probably open a Faces Spec ticket.

@pizzi80
Copy link
Contributor

pizzi80 commented May 27, 2022

created

jakartaee/faces#1683

@melloware
Copy link
Contributor

I think the difference with two implementations is that MyFaces extends TagHandler so it can check...

        if (! (parent instanceof UIViewRoot) )
        {
            throw new TagException(this.tag, "Parent UIComponent "+parent.getId()+" should be instance of UIViewRoot");
        }

But in Mojarra its not a TagHandler so checking for the parent won't be as straighforward as far as I can tell.

@tandraschko
Copy link
Contributor

also related to: jakartaee/faces#1466

@melloware
Copy link
Contributor

Just saw this PR: #5179

@volosied
Copy link
Contributor Author

Should myfaces also be updated to log a warning, instead of throwing an exception?

@melloware
Copy link
Contributor

@volosied for consistency i think it should and even log the exact error text of Mojarra so if people look it up on stack overflow etc.?

@BalusC
Copy link
Contributor

BalusC commented Nov 25, 2022

The warning log is merely for backwards compatibilty of Mojarra itself because the spec does still not say what should happen when the f:metadata is not a direct child of f:view. There are basically 3 options:

  1. ignore f:metadata and log warning
  2. use f:metadata but log warning
  3. throw exception

For Faces.next we can set it in stone.

The first technical question is: why would it ever harm if f:metadata is not a direct child of f:view? One of the answers is that the f:metadata should reside in the view associated with viewId and not in a template (include/decorate) or composite as that would be prone to end up in confusing/unintentional behavior when used in multiple views/locations. One of the ways to guarantee consistent and predictable behavior that is that it must be a direct child of f:view.

@mnriem
Copy link
Contributor

mnriem commented Sep 19, 2023

@BalusC Should this one be closed in favor of jakartaee/faces#1683

@BalusC
Copy link
Contributor

BalusC commented Sep 23, 2023

Not the same issue. But on second thought current Mojarra ticket should be closed and a new Faces ticket must be opened where we set in stone what exactly Faces should do during compile when f:metadata is found to not be a direct child of f:view. So hereby: jakartaee/faces#1849

@BalusC BalusC closed this as completed Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants