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

Non-existent view with non-standard mapping incorrectly throws ViewHandlingStrategyNotFoundException instead of returning 404 #5120

Closed
BalusC opened this issue Jun 19, 2022 · 2 comments
Milestone

Comments

@BalusC
Copy link
Contributor

BalusC commented Jun 19, 2022

Discovered in omnifaces/omnifaces@9d1ebaf

Have an additional mapping on FacesServlet using an URL pattern which does not match Facelets default suffix:

	<servlet-mapping>
		<servlet-name>facesServlet</servlet-name>
		<url-pattern>*.xhtml</url-pattern>
		<url-pattern>*.jsf</url-pattern>
	</servlet-mapping>

Opening non-existent view on an URL pattern which coincidentally matches Facelets default suffix of .xhtml such as /foo.xhtml correctly returns 404.

However, opening non-existent view on an URL pattern which does not match Facelets default suffix of .xhtml such as /foo.jsf incorrectly throws ViewHandlingStrategyNotFoundException instead of returning 404.

com.sun.faces.application.view.ViewHandlingStrategyNotFoundException
	java.base/java.util.Optional.orElseThrow(Optional.java:408)
	com.sun.faces.application.view.ViewHandlingStrategyManager.getStrategy(ViewHandlingStrategyManager.java:57)
	com.sun.faces.application.view.ViewDeclarationLanguageFactoryImpl.getViewDeclarationLanguage(ViewDeclarationLanguageFactoryImpl.java:45)
	com.sun.faces.application.view.MultiViewHandler.getViewDeclarationLanguage(MultiViewHandler.java:407)
	jakarta.faces.application.ViewHandlerWrapper.getViewDeclarationLanguage(ViewHandlerWrapper.java:335)
	jakarta.faces.application.ViewHandlerWrapper.getViewDeclarationLanguage(ViewHandlerWrapper.java:335)
	com.sun.faces.lifecycle.RestoreViewPhase.execute(RestoreViewPhase.java:179)
	com.sun.faces.lifecycle.Phase.doPhase(Phase.java:72)
	com.sun.faces.lifecycle.RestoreViewPhase.doPhase(RestoreViewPhase.java:95)
	com.sun.faces.lifecycle.LifecycleImpl.execute(LifecycleImpl.java:159)
	jakarta.faces.webapp.FacesServlet.executeLifecyle(FacesServlet.java:691)
	jakarta.faces.webapp.FacesServlet.service(FacesServlet.java:449)

It worked fine in Mojarra 4.0.0-M1.

The bug was introduced during 45848de
The ViewHandler#deriveLogicalViewId() is specified to allow returning non-existent view IDs. However during abovementioned work it's converting view ID using MultiViewHandler#convertViewId() which by itself does:

            if (vdl.viewExists(context, convertedViewId)) {
                // RELEASE_PENDING (rlubke,driscoll) cache the lookup
                return convertedViewId;
            }

This check is in hindsight superflous and should be removed. The convertViewId() method is nowhere else used. The ViewHandler#deriveViewId() already does that check by itself.

arjantijms added a commit that referenced this issue Jun 19, 2022
Fix #5120: no need to doublecheck if view exists when invoked via ViewHandler#deriveLogicalViewId() instead of deriveViewId()
@arjantijms arjantijms reopened this Mar 11, 2023
@arjantijms
Copy link
Contributor

With this change, a TCK test now fails.

/**
* @testName: viewHandlerDeriveLogicalViewIDTest

* @assertion_ids: JSF:JAVADOC:368; JSF:JAVADOC:2533

* @test_Strategy: Validate that we Derive and return the viewId from the
* current request. (with no physical view)

* @since: 2.2
*/
public void viewHandlerDeriveLogicalViewIDTest(HttpServletRequest request, HttpServletResponse responsethrows ServletException, IOException {

    PrintWriter out = response.getWriter();
    String viewId"/viewId";
    FacesContext context = getFacesContext();
    ViewHandler vh = getApplication().getViewHandler();
    
    // Get viewId
    String derivedId = vh.deriveLogicalViewId(context, viewId);
    
    if (!viewId.equals(derivedId)) {
     out.println(JSFTestUtil.FAIL + [JSFTestUtil.NL](http://jsftestutil.nl/)"Unexpectecd value from ViewHandler.deriveViewId()" + [JSFTestUtil.NL](http://jsftestutil.nl/)"Execpted: " + viewId + [JSFTestUtil.NL](http://jsftestutil.nl/) + "Received: " + derivedId);else {
        out.println(JSFTestUtil.PASS);
    }
}// End viewHandlerDeriveLogicalViewIDTest

Note that the referenced assertion_ids are seemingly bogus.

@arjantijms
Copy link
Contributor

CC @BalusC

arjantijms added a commit to arjantijms/mojarra that referenced this issue Mar 14, 2023
Signed-off-by: Arjan Tijms <arjan.tijms@gmail.com>
arjantijms added a commit to arjantijms/mojarra that referenced this issue Mar 14, 2023
This fixes the TCK failure introduced by issue eclipse-ee4j#5120 and pr eclipse-ee4j#5121

Signed-off-by: Arjan Tijms <arjan.tijms@omnifish.ee>
arjantijms added a commit that referenced this issue Mar 14, 2023
Fix for derivePhysicalViewId broken in #5120
@BalusC BalusC closed this as completed May 20, 2023
@BalusC BalusC added this to the 4.0.2 milestone Jul 22, 2023
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

No branches or pull requests

2 participants