Add Thorntail support

[juangon]

So, after release of 2.3.0.Final, I can push this pull request.

There are some changes I want to highlight:

• In Extensionless Mapping, there is a ServletContexListener (MappingInit), that makes usage of FacesContext.getCurrentInstance(). Depending if JSF Mojarra's own ServletContextListener (ConfigureListener), where all environment is setup, is invoked before project's custom ServletContextListener, this will work or will throw a NullPointerException. In order to avoid this, I had to order both listeners explicitely in web.xml, so JSF initialization comes first always.
• In Servlet Mapping test: I made two changes that now makes test failing in Payara: -For DEFAULT servlet, match value should be empty, instead of the wrong requested resource. This seems clear in javadoc examples here: https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletMapping.html -For CONTEXT_ROOT servlet, javadoc table example was clear too, except there were some wrong statement in getPattern for DEFAULT servlet, but this should be fixed under: https://github.com/eclipse-ee4j/servlet-api/issues/233

/cc @Ladicek

[Ladicek]

Looking good to me!

[juangon]

Hi @arjantijms . I guess you are waiting for https://github.com/eclipse-ee4j/servlet-api/issues/233 in order to merge this?

[arjantijms]

@juangon That was indeed the idea. I guess this PR and that one should be able to be merged soon.

Thanks again for looking at this!

[arjantijms]

https://github.com/eclipse-ee4j/servlet-api/pull/234 has been merged, fixing https://github.com/eclipse-ee4j/servlet-api/issues/233

[arjantijms]

@juangon I did spot one potential issue with the PR though:

com.sun.faces.config.ConfigureListener

This is only valid for Mojarra, and not for MyFaces. You could try if a system event listener to do the mapping in works better.

For an example see:

• https://github.com/omnifaces/omnifaces/blob/master/src/main/java/org/omnifaces/ApplicationProcessor.java
• https://github.com/omnifaces/omnifaces/blob/master/src/main/resources/META-INF/faces-config.xml#L34

[juangon]

Thanks for pointing that out. Yes I saw it too when implementing but, as servers that works on these tests are all using Mojarra I didn't consider that necessary, althought it should be the right fix indeed.

Will check using System even listeners rigth now.

Thanks!

[juangon]

@arjantijms doesn't work. It isn't possible to add mappings or any servlet registration programatically outside a ServletContext listener:

[arjantijms]

@juangon the failure is probably not exactly due to that, as the PostConstructApplicationEvent is thrown from a ServletContextListener (in Mojarra, it's the ConfigureListener that throws it).

The problem is probably that the ConfigureListener itself is programmatically registered (in FacesInitialiser), and that for some reason things like getServletRegistrations and getVirtualServer are not allowed to be called from programmatically registered listeners.

[arjantijms]

Also interesting here is that StandardContext in GF and Payara contains special exception code for Mojarra:

 if (t instanceof ServletContextListener) {
            ServletContextListener proxy = (ServletContextListener) t;
            if (isProgrammatic) {
                proxy = new RestrictedServletContextListener(
                    (ServletContextListener) t);
            }
            // Always add the JSF listener as the first element,
            // see GlassFish Issue 2563 for details
            boolean isFirst =
                "com.sun.faces.config.ConfigureListener".equals(
                    t.getClass().getName());
            if (isFirst) {
                contextListeners.add(0, proxy);
            } else {
                contextListeners.add(proxy);
            }
            if (!added) {
                added = true;
            }
        }

Referenced issue is https://github.com/eclipse-ee4j/glassfish/issues/2563

[arjantijms]

@juangon after thinking about this a little, the following should work for the case the JSF configure/init listener is last

Collect the registrations in a statically declared listener:

@WebListener
public class MappingInit implements ServletContextListener {

    @Override
    public void contextInitialized(ServletContextEvent sce) {
        sce.getServletContext().setAttribute("mappings", sce.getServletContext().getServletRegistrations());
    }

}

Then use them in the JSF system event listener:

    @Override
    public void processEvent(SystemEvent event) {
        try {
            ServletContext servletContext = (ServletContext) FacesContext.getCurrentInstance().getExternalContext().getContext();

             FacesContext context = FacesContext.getCurrentInstance();

             Map<String, ? extends ServletRegistration> servletRegistrations = (Map<String, ? extends ServletRegistration> ) servletContext.getAttribute("mappings");
             if (servletRegistrations == null) {
                 return;
             }

             servletRegistrations
                   .values()
                   .stream()
                   .filter(e -> e.getClassName().equals(FacesServlet.class.getName()))
                   .findAny()
                   .ifPresent(
                       reg -> context.getApplication()
                                     .getViewHandler()
                                     .getViews(context, "/", RETURN_AS_MINIMAL_IMPLICIT_OUTCOME)
                                     .forEach(e -> reg.addMapping(e)));

        }
        catch (Exception | LinkageError e) {
            throw e;
        }
    }

This should only still take into account the situation that the JSF listener is first, which can be handled by having a regular context listener still there, that checks if the JSF system event listener hasn't run already.

[juangon]

@arjantijms wow, "interesting" hack for making it first listener in Glassfish/Payara:-).

About your suggestion, thanks for that! It now works in Thorntail but not using Payara, as it can't found the new mappings. This is caused mainly by ServletRegistration Map obtained from the ServletContext attribute in SystemEvent being null

[juangon]

@arjantijms I pushed changes for your JSF suggestion here: https://github.com/juangon/javaee8-samples/tree/thorntail_JSF . There you can see it doesn't work in Payara due to mappings being empty. It works in Thorntail. If you need anything from me, just tell me. Thanks!!

[arjantijms]

There you can see it doesn't work in Payara due to mappings being empty. It works in Thorntail.

My suggestion would be too have the regular static listener that does the mappings there and keep a flag in the application attributes to check if mappings have already been applied. If mappings have been applied AND JSF has been initialised, the listener can go ahead and do its regular work (as it does in master now), if not it does nothing.

That would, I think, work for both cases.

[juangon]

Thanks @arjantijms ! Now it works, although looks a bit "hacky".

Just pushed all changes.

[arjantijms]

@juangon thank you too!

Hopefully I can add something to the JSF 3.0 spec or Servlet.Next spec to make this less hacky in the future, but for EE 8 it seems that this is the way then.

Payara is of course failing now, so I should fix that too in Payara.

Thx again!