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

MYFACES-4442: avoid UnsupportedOperationException #292

Merged
merged 1 commit into from Aug 30, 2022

Conversation

pnicolucci
Copy link
Contributor

@pnicolucci pnicolucci commented Aug 29, 2022

Fix for MYFACES-4442: https://issues.apache.org/jira/browse/MYFACES-4442

Current behavior:

  1. MyFacesContainerInitializer.onStartup
    a) Checks clazzes from @HandlesTypes
    b) Checks if a faces-config.xml is present.
    c) If a or b
    - Check if there are current FacesServlet mappings
    - If there is a FacesServlet defined set: FACES_SERVLET_FOUND(true)
    - If there is not a FacesServlet defined, then dynamically add and set: FACES_SERVLET_ADDED_ATTRIBUTE(true)

  2. StartupServletContextListener.contextInitialized
    a) Call FacesInitializer.initFaces(ServletContext)

  3. FacesInitializerImpl.initFaces
    a) If !INITIALIZE_ALWAYS_STANDALONE
    b) Use FacesServletMappingUtils to getFacesServletRegistration
    c) if getFacesServletRegistration is null / 0 then perform other checks
    d) Check MyFacesContainerInitializer.FACES_SERVLET_FOUND
    c) If MyFacesContainerInitializer.FACES_SERVLET_FOUND is null/false then check FACES_SERVLET_ADDED_ATTRIBUTE
    d) If FACES_SERVLET_ADDED_ATTRIBUTE is null / false then put out message: "No mappings of FacesServlet found. Abort
    initializing MyFaces."

Proposed Solution:

  1. MyFacesContainerInitializer.onStartup
    a) set isFacesServletPresent if there is a current FacesServlet defined.
    b) Checks clazzes from @HandlesTypes
    c) Checks if a faces-config.xml is present.
    d) If b or c
    - If there is not a FacesServlet defined (!isFacesServletPresent), then dynamically add and set:
    FACES_SERVLET_ADDED_ATTRIBUTE(true)

  2. StartupServletContextListener.contextInitialized
    a) Call FacesInitializer.initFaces(ServletContext)

  3. FacesInitializerImpl.initFaces
    a) If !INITIALIZE_ALWAYS_STANDALONE
    d) Check MyFacesContainerInitializer.FACES_SERVLET_FOUND
    c) If MyFacesContainerInitializer.FACES_SERVLET_FOUND is null/false then check FACES_SERVLET_ADDED_ATTRIBUTE
    d) If FACES_SERVLET_ADDED_ATTRIBUTE is null / false then put out message: "No mappings of FacesServlet found. Abort
    initializing MyFaces."

Notes:
I don't see any reason why we would need to parse the web.xml or look do another lookup for a FacesServlet in the FacesInitializerImpl when the FacesServlet was already looked up in the MyFacesContainerInitializer. Also if a FacesServlet is not defined and @HandlesTypes classes are passed into onStartup or there is a faces-config.xml available in the application
then the FacesServlet is added dynamically. So for any application that is "Faces enabled" we would either already
have a FacesServlet defined in the web app or we'd try to dynamically add a FacesServlet in onStartup.

To me this pre-Faces 4.0 web.xml parsing and additional lookup seem like old legacy code that perhaps has been around since before ServletContainerInitializers were available (before Servlet 3.0).

This will avoid the UnsupportedOperationException from initFaces for calling getServletRegistrations from a
programmatically added listener: StartupServletContextListener.

@pnicolucci pnicolucci self-assigned this Aug 29, 2022
@pnicolucci pnicolucci marked this pull request as ready for review August 29, 2022 19:24
@tandraschko
Copy link
Member

i like this fix but this doesnt solve https://issues.apache.org/jira/browse/TOMEE-2846, which is actually related to the same root problem

@pnicolucci
Copy link
Contributor Author

@tandraschko I opened https://issues.apache.org/jira/browse/MYFACES-4447 to address automatic extensionless mapping.

This fix at least gets us past the initial initFaces exception and allows me to be unblocked and start investigating other Faces 4.0 issues that we have encountered.

@pnicolucci
Copy link
Contributor Author

So I'll plan to merge this by the end of the day today if no objections to the actual code change here except for we still need to fix it for automatic extensionless mapping which has been broken since 2.3 in this scenario it looks.

@pnicolucci
Copy link
Contributor Author

Opened: to fix extensionless mapping: https://issues.apache.org/jira/browse/MYFACES-4447

Copy link
Contributor

@volosied volosied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I tested a few scenarios with this code and everything looked good.

@pnicolucci pnicolucci merged commit fcaf6d4 into apache:main Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants