MyFaces Core
  1. MyFaces Core
  2. MYFACES-2945

Make a way to get the FacesConfig from a provider

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 2.0.3
    • Component/s: SPI
    • Labels:
      None

      Description

      Currently, MyFaces startup listener will parse the all the faces configuration files and sort them on each startup time, and it will be better to do it once in the deployment time, and get those data structure instances from a provider. One possible way is to make those FacesConfig class serializable.

      1. MYFACES-2945-FacesConfigurationMerger.patch
        113 kB
        Jakob Korherr
      2. MYFACES-2945-3.patch
        205 kB
        Leonardo Uribe
      3. MYFACES-2945-2.patch
        147 kB
        Leonardo Uribe
      4. GeronimoFacesConfigurationMerger.java
        2 kB
        Jakob Korherr

        Issue Links

          Activity

          Hide
          Ivan added a comment -

          Just some ideas for the integration improvment, please help to give some comments.
          a. Create a new SPI provider and a provider factory, might name FacesConfigurationProvider/FacesConfigurationProviderFactory, which has a method
          FacesConfig getFacesConfig(ExternalContext context);
          This method is designed to get the final combined the FacesConfig instance, which including application resource configuration + annotation + default web application resource
          And move those logic about merge/annotaiton scanning from FacesConfigurator and AnnotationConfigurator to the default implementation provider
          b. Make some classes in the org.apache.myfaces.config.impl.digester.elements package serializable, this might be optional...
          From the integration side, the third-party container could implement their own FacesConfigurationProvider, they might return the FacesConfig object directly from their own structure.
          c. I saw some codes in the ***ProviderFactory is commented out,
          //AnnotationProviderFactory instance = (AnnotationProviderFactory) ctx.getApplicationMap().get(FACTORY_KEY);
          //if (instance != null)
          //

          { // return instance; //}

          Did it cause any issue ? Or I hope to recover them, as it will be more easier to set other implemtations for other containers. The commons-discovery package might not work correctly in OSGI environment. as they will try to search META-INF/services folder, and this way sometimes might not work correctly in OSGi environment.
          thanks.

          Show
          Ivan added a comment - Just some ideas for the integration improvment, please help to give some comments. a. Create a new SPI provider and a provider factory, might name FacesConfigurationProvider/FacesConfigurationProviderFactory, which has a method FacesConfig getFacesConfig(ExternalContext context); This method is designed to get the final combined the FacesConfig instance, which including application resource configuration + annotation + default web application resource And move those logic about merge/annotaiton scanning from FacesConfigurator and AnnotationConfigurator to the default implementation provider b. Make some classes in the org.apache.myfaces.config.impl.digester.elements package serializable, this might be optional... From the integration side, the third-party container could implement their own FacesConfigurationProvider, they might return the FacesConfig object directly from their own structure. c. I saw some codes in the ***ProviderFactory is commented out, //AnnotationProviderFactory instance = (AnnotationProviderFactory) ctx.getApplicationMap().get(FACTORY_KEY); //if (instance != null) // { // return instance; //} Did it cause any issue ? Or I hope to recover them, as it will be more easier to set other implemtations for other containers. The commons-discovery package might not work correctly in OSGI environment. as they will try to search META-INF/services folder, and this way sometimes might not work correctly in OSGi environment. thanks.
          Hide
          Jakob Korherr added a comment -

          The suggestion seems ok to me. We will start working on that soon!

          Show
          Jakob Korherr added a comment - The suggestion seems ok to me. We will start working on that soon!
          Hide
          Leonardo Uribe added a comment -

          I'll provide an analisys and a patch for this one soon.

          Show
          Leonardo Uribe added a comment - I'll provide an analisys and a patch for this one soon.
          Hide
          Leonardo Uribe added a comment -

          Hi

          Here we have different problems to handle, so below there is a list of them:

          1. commons-discovery package might not work correctly in OSGi environment: OSGi does not provide a Thread Context Class Loader (TCCL) by default, but as explained before (MYFACES-2944), the container must provide a TCCL to make JSF work in OSGi. In theory, if the TCCL interface is correct, this should not be a problem, but anyway it could be good to provide an interface that can be configured using an application scope key to locate SPI interfaces. Right now we are using commons-discovery for LifecycleProvider and others Providers under org.apache.myfaces.spi interface, but we have a different code on FacesConfigurator, following JSF 2.0 spec section 11.2.6.1 FactoryFinder. Before solve this one we must unify the code that load SPI interfaces from /META-INF/services in one way or another (use commons discovery or a custom code).

          2. How to prevent parse faces-config.xml files more times than necessary?. I think this is the objective why it is required an interface to handle this issue, right? From the server container point of view, it could be good to parse all stuff (faces-config.xml, META-INF/services and JSF annotations) just once and save it, so if the web application is undeployed and deployed again, the initialization time will be faster.

          In this issue I suppose we are only interested in (2) but in some way it is related to (1). To understand clearly which options do we have, it is necessary to take into account JSF 2.0 spec section 11, specially section 11.2.6.1 and 11.4. The relevant points are resumed below (the idea is be very detailed on this part, otherwise it is easy to get lost):

          1.2.6.1 FactoryFinder

          "... For a given factory class name, a corresponding implementation class is searched for based on the following algorithm. Items are listed in order of decreasing search precedence ...", that means the if there is configuration on the first items on the list, that one takes precedence over the later ones.

          "...
          1. If a default JavaServer Faces configuration file (/WEB-INF/faces-config.xml) is bundled into the web application, and it contains a factory entry of the given factory class name, that factory class is used.
          2. If the JavaServer Faces configuration resource(s) named by the javax.faces.CONFIG_FILES ServletContext init parameter (if any) contain any factory entries of the given factory class name, those factories are used, with the last one taking precedence.
          3. If there are any META-INF/faces-config.xml resources bundled any JAR files in the web ServletContext's resource paths, the factory entries of the given factory class name in those files are used, with the last one taking precedence.
          4. If a META-INF/services/

          {factory-class-name} resource is visible to the web application class loader for the calling application (typically as a result of being present in the manifest of a JAR file), its first line is read and assumed to be the name of the factory implementation class to use.
          5. If none of the above steps yield a match, the JavaServer Faces implementation specific class is used.
          ..."

          11.4 Application Startup Behavior

          ".. At application startup time, before any requests are processed, the JSF implementation must process zero or more application configuration resources, located according as follows:

          Make a list of all of the application configuration resources found using the following algorithm:

          - Search for all resources that match either "META-INF/faces-config.xml" or end with ".facesconfig.xml" directly in the "META-INF" directory. Each resource that matches that expression must be considered an application configuration resource.

          - Check for the existence of a context initialization parameter named javax.faces.CONFIG_FILES. If it exists, treat it as a comma-delimited list of context relative resource paths (starting with a "/"), and add each of the specfied resources to the list.

          Let this list be known as applicationConfigurationResources for discussion. Also, check for the existence of a web application configuration resource named "/WEB-INF/faces-config.xml", and refer to this as applicationFacesConfig for discussion, but do not put it in the list. When parsing the application configuration resources, the implementation must ensure that applicationConfigurationResources are parsed before applicationFacesConfig..."

          11.5.1 Requirements for scanning of classes for annotations

          "...
          - If the <faces-config> element in the WEB-INF/faces-config.xml file contains metadata-complete attribute whose value is "true", the implementation must not perform annotation scanning on any classes except for those classes provided by the implementation itself. Otherwise, continue as follows.
          - If the runtime discovers a conflict between an entry in the Application Configuration Resources and an annotation, the entry in the Application Configuration Resources takes precedence.
          - All classes in WEB-INF/classes must be scanned.
          - For every jar in the application's WEB-INF/lib directory, if the jar contains a "META-INF/faces-config.xml" file or a file that matches the regular expression ".*\.faces-config.xml" (even an empty one), all classes in that jar must be scanned. ..."

          Based on the previous documentation, the ordering to load JSF configuration is this:

          - Look JSF standard faces-config.xml file (in myfaces case META-INF/standard-faces-config.xml).
          - Look JSF Factory class configuration under META-INF/services/{factory-class-name}

          .

          • Look JSF annotation on the classpath (jars and WEB-INF/classes). (org.apache.myfaces.spi.AnnotationProvider delegates how JSF annotations are scanned).
          • Look for META-INF/faces-config.xml and META-INF/*.faces-config.xml resources on classpath (org.apache.myfaces.spi.FacesConfigResourceProvider delegates how these config files are found if it is necessary to the server container, for example if it use a custom way to load resources like JBoss AS 6 (jndi://...) ). This resources are
            added as applicationConfigurationResources.
          • Look for resources under javax.faces.CONFIG_FILES. This resources are added as applicationConfigurationResources.
          • Sort all applicationConfigurationResources using rules on JSF 2.0 section 11.4.7
          • Look for /WEB-INF/faces-config.xml file.

          And finally the configuration information are feed in this way (from lower to upper priority)

          • META-INF/standard-faces-config.xml
          • META-INF/services/ {factory-class-name}
          • Annotations if and only if /WEB-INF/faces-config.xml <metadata-complete> is set to true.
          • Sorted ApplicationConfigurationResources.
          • /WEB-INF/faces-config.xml

          I hope with the previous explanation, it becomes clear the steps required to create the configuration information. These steps are done by FacesConfigurator.

          Now, if the problem is "cache" configuration information there are two possible options:

          1. Cache the final information of all previous process. That means if in a project a dependency is changed, of a relate file is, all previous algorithm must be done again.
          2. Cache each configuration bundle separately. That means if a dependency is added to the project on the server, we only need to parse the additional files and all final configuration will be calculated each time the web application is deployed, but in this case it will be a lot faster than parse all files over and over.

          The option (1) (note this is the one proposed here) looks the most easy one, but I checked the current algorithm and I notice it requires some major changes. The first problem is the "container" used to "sum" all information is not:

          org.apache.myfaces.config.impl.digester.elements.FacesConfig

          It is

          org.apache.myfaces.config.impl.digester.FacesConfigDispenser<FacesConfig>

          The first step is change FacesConfigDispenser from interface to abstract class. But one problem is FacesConfigurator does not implement the algorithm exactly as the theorical algorithm exposed here says. So we need to fix that. After that, we need to "hide" all methods on FacesConfigDispenser that start with "feed" word, because it is a detail that we don't want to expose in a SPI interface. All previous points can be done, but it will take some time to complete them.

          The option (2) is in theory more flexible, but requires to think more about how the interface should looks like, and do a similar work to the one exposed before. Anyway, do those changes will take more time than I expected.

          Suggestions are welcome.

          Show
          Leonardo Uribe added a comment - Hi Here we have different problems to handle, so below there is a list of them: 1. commons-discovery package might not work correctly in OSGi environment: OSGi does not provide a Thread Context Class Loader (TCCL) by default, but as explained before ( MYFACES-2944 ), the container must provide a TCCL to make JSF work in OSGi. In theory, if the TCCL interface is correct, this should not be a problem, but anyway it could be good to provide an interface that can be configured using an application scope key to locate SPI interfaces. Right now we are using commons-discovery for LifecycleProvider and others Providers under org.apache.myfaces.spi interface, but we have a different code on FacesConfigurator, following JSF 2.0 spec section 11.2.6.1 FactoryFinder. Before solve this one we must unify the code that load SPI interfaces from /META-INF/services in one way or another (use commons discovery or a custom code). 2. How to prevent parse faces-config.xml files more times than necessary?. I think this is the objective why it is required an interface to handle this issue, right? From the server container point of view, it could be good to parse all stuff (faces-config.xml, META-INF/services and JSF annotations) just once and save it, so if the web application is undeployed and deployed again, the initialization time will be faster. In this issue I suppose we are only interested in (2) but in some way it is related to (1). To understand clearly which options do we have, it is necessary to take into account JSF 2.0 spec section 11, specially section 11.2.6.1 and 11.4. The relevant points are resumed below (the idea is be very detailed on this part, otherwise it is easy to get lost): 1.2.6.1 FactoryFinder "... For a given factory class name, a corresponding implementation class is searched for based on the following algorithm. Items are listed in order of decreasing search precedence ...", that means the if there is configuration on the first items on the list, that one takes precedence over the later ones. "... 1. If a default JavaServer Faces configuration file (/WEB-INF/faces-config.xml) is bundled into the web application, and it contains a factory entry of the given factory class name, that factory class is used. 2. If the JavaServer Faces configuration resource(s) named by the javax.faces.CONFIG_FILES ServletContext init parameter (if any) contain any factory entries of the given factory class name, those factories are used, with the last one taking precedence. 3. If there are any META-INF/faces-config.xml resources bundled any JAR files in the web ServletContext's resource paths, the factory entries of the given factory class name in those files are used, with the last one taking precedence. 4. If a META-INF/services/ {factory-class-name} resource is visible to the web application class loader for the calling application (typically as a result of being present in the manifest of a JAR file), its first line is read and assumed to be the name of the factory implementation class to use. 5. If none of the above steps yield a match, the JavaServer Faces implementation specific class is used. ..." 11.4 Application Startup Behavior ".. At application startup time, before any requests are processed, the JSF implementation must process zero or more application configuration resources, located according as follows: Make a list of all of the application configuration resources found using the following algorithm: - Search for all resources that match either "META-INF/faces-config.xml" or end with ".facesconfig.xml" directly in the "META-INF" directory. Each resource that matches that expression must be considered an application configuration resource. - Check for the existence of a context initialization parameter named javax.faces.CONFIG_FILES. If it exists, treat it as a comma-delimited list of context relative resource paths (starting with a "/"), and add each of the specfied resources to the list. Let this list be known as applicationConfigurationResources for discussion. Also, check for the existence of a web application configuration resource named "/WEB-INF/faces-config.xml", and refer to this as applicationFacesConfig for discussion, but do not put it in the list. When parsing the application configuration resources, the implementation must ensure that applicationConfigurationResources are parsed before applicationFacesConfig..." 11.5.1 Requirements for scanning of classes for annotations "... - If the <faces-config> element in the WEB-INF/faces-config.xml file contains metadata-complete attribute whose value is "true", the implementation must not perform annotation scanning on any classes except for those classes provided by the implementation itself. Otherwise, continue as follows. - If the runtime discovers a conflict between an entry in the Application Configuration Resources and an annotation, the entry in the Application Configuration Resources takes precedence. - All classes in WEB-INF/classes must be scanned. - For every jar in the application's WEB-INF/lib directory, if the jar contains a "META-INF/faces-config.xml" file or a file that matches the regular expression ".*\.faces-config.xml" (even an empty one), all classes in that jar must be scanned. ..." Based on the previous documentation, the ordering to load JSF configuration is this: - Look JSF standard faces-config.xml file (in myfaces case META-INF/standard-faces-config.xml). - Look JSF Factory class configuration under META-INF/services/{factory-class-name} . Look JSF annotation on the classpath (jars and WEB-INF/classes). (org.apache.myfaces.spi.AnnotationProvider delegates how JSF annotations are scanned). Look for META-INF/faces-config.xml and META-INF/*.faces-config.xml resources on classpath (org.apache.myfaces.spi.FacesConfigResourceProvider delegates how these config files are found if it is necessary to the server container, for example if it use a custom way to load resources like JBoss AS 6 (jndi://...) ). This resources are added as applicationConfigurationResources. Look for resources under javax.faces.CONFIG_FILES. This resources are added as applicationConfigurationResources. Sort all applicationConfigurationResources using rules on JSF 2.0 section 11.4.7 Look for /WEB-INF/faces-config.xml file. And finally the configuration information are feed in this way (from lower to upper priority) META-INF/standard-faces-config.xml META-INF/services/ {factory-class-name} Annotations if and only if /WEB-INF/faces-config.xml <metadata-complete> is set to true. Sorted ApplicationConfigurationResources. /WEB-INF/faces-config.xml I hope with the previous explanation, it becomes clear the steps required to create the configuration information. These steps are done by FacesConfigurator. Now, if the problem is "cache" configuration information there are two possible options: 1. Cache the final information of all previous process. That means if in a project a dependency is changed, of a relate file is, all previous algorithm must be done again. 2. Cache each configuration bundle separately. That means if a dependency is added to the project on the server, we only need to parse the additional files and all final configuration will be calculated each time the web application is deployed, but in this case it will be a lot faster than parse all files over and over. The option (1) (note this is the one proposed here) looks the most easy one, but I checked the current algorithm and I notice it requires some major changes. The first problem is the "container" used to "sum" all information is not: org.apache.myfaces.config.impl.digester.elements.FacesConfig It is org.apache.myfaces.config.impl.digester.FacesConfigDispenser<FacesConfig> The first step is change FacesConfigDispenser from interface to abstract class. But one problem is FacesConfigurator does not implement the algorithm exactly as the theorical algorithm exposed here says. So we need to fix that. After that, we need to "hide" all methods on FacesConfigDispenser that start with "feed" word, because it is a detail that we don't want to expose in a SPI interface. All previous points can be done, but it will take some time to complete them. The option (2) is in theory more flexible, but requires to think more about how the interface should looks like, and do a similar work to the one exposed before. Anyway, do those changes will take more time than I expected. Suggestions are welcome.
          Hide
          Ivan added a comment - - edited

          First, I have to admit I am not familar with the whole MyFaces arch, just my two cents
          1. Yes, commons-discovery should not work correctly in OSGI, while Geronimo provides a BundleClassloader, it will help to search all the bundles wired to the application, and it should be why it works now. But, for those SPI, I do not like to find them by commons-discovery or any other search strategy, as those configurations should be provided by the Geronimo (e.g. Geronimo needs to know those annotated managed bean classes, and create jndi context for them if required), and on the runtime, no one knows what resource could be find anywhere, currently, I used an 'urgly' way to directly configure them in the cache of commons-discovery, and I hope that there is better way to configure it.
          2. I would prefer the option 2.
          To cache all the configurations from server runtime and application is a bit overcharged. I think that it is only required to cache the final result when the application is deployed, if the dependency of the application is changed, it needs to re-deploy the application, as many things might be changed, not only for JSF. I feel that the interface mentioned by you or other members on the mail list is OK, a method like FacesConfig getApplicationFacesConfig() should be enough.
          Also, how about creating a builder class ? And, move those sort algorithm there from the FacesConfiguator, and for FacesConfigurator, it directly call the getFacesConfig method to get the final result and configure the final Facotory to the JSF runtime.
          Thanks.

          Show
          Ivan added a comment - - edited First, I have to admit I am not familar with the whole MyFaces arch, just my two cents 1. Yes, commons-discovery should not work correctly in OSGI, while Geronimo provides a BundleClassloader, it will help to search all the bundles wired to the application, and it should be why it works now. But, for those SPI, I do not like to find them by commons-discovery or any other search strategy, as those configurations should be provided by the Geronimo (e.g. Geronimo needs to know those annotated managed bean classes, and create jndi context for them if required), and on the runtime, no one knows what resource could be find anywhere, currently, I used an 'urgly' way to directly configure them in the cache of commons-discovery, and I hope that there is better way to configure it. 2. I would prefer the option 2. To cache all the configurations from server runtime and application is a bit overcharged. I think that it is only required to cache the final result when the application is deployed, if the dependency of the application is changed, it needs to re-deploy the application, as many things might be changed, not only for JSF. I feel that the interface mentioned by you or other members on the mail list is OK, a method like FacesConfig getApplicationFacesConfig() should be enough. Also, how about creating a builder class ? And, move those sort algorithm there from the FacesConfiguator, and for FacesConfigurator, it directly call the getFacesConfig method to get the final result and configure the final Facotory to the JSF runtime. Thanks.
          Hide
          Jakob Korherr added a comment -

          Thanks for all this information, Leo

          For the commons-discovery problem I would suggest to "copy" the code from ServiceLoader from the Java 6 Runtime [1]. As I've seen so far, many other open source projects that run on Java 5 did the same (e.g. Arquillian), thus it seems to be a common technique at the moment. In this way we could get rid of commons-discovery once and for all, because in future versions of JSF which run only on Java 6, we can just replace our copied version of ServiceLoader with the version from Java 6.

          Furthermore we could create a MyFacesServiceLoader which uses this ServiceLoader class, but also has a way to add Service entries programmatically. Thus Geronimo would be able to add its Service implementations to that class before MyFaces starts and it will be guaranteed that those Service impls will be used.

          Ivan, it is not clear to me which option you prefer. You wrote "I would prefer the option 2", but in the text afterwards you explain why option 1 (Cache the final information of all previous process) is better. So what do you actually prefer for Geronimo?
          IMO option 1 is the way to go, because it's easier and moreover, on option 2 there are a lot of things which can (and will) go wrong, because it could be really complex if you think of the whole Java EE enviroment.

          For the OSGi related problems, please take a look at MYFACES-2976. IMHO a separate MyFaces OSGI bundle is the way to go (actually, Mojarra does exactly the same). Of course, this OSGi bundle will be created automatically by maven out of the myfaces-api and myfaces-impl artifacts and there is no need duplicate code.

          [1] http://download.oracle.com/javase/6/docs/api/java/util/ServiceLoader.html

          Show
          Jakob Korherr added a comment - Thanks for all this information, Leo For the commons-discovery problem I would suggest to "copy" the code from ServiceLoader from the Java 6 Runtime [1] . As I've seen so far, many other open source projects that run on Java 5 did the same (e.g. Arquillian), thus it seems to be a common technique at the moment. In this way we could get rid of commons-discovery once and for all, because in future versions of JSF which run only on Java 6, we can just replace our copied version of ServiceLoader with the version from Java 6. Furthermore we could create a MyFacesServiceLoader which uses this ServiceLoader class, but also has a way to add Service entries programmatically. Thus Geronimo would be able to add its Service implementations to that class before MyFaces starts and it will be guaranteed that those Service impls will be used. Ivan, it is not clear to me which option you prefer. You wrote "I would prefer the option 2", but in the text afterwards you explain why option 1 (Cache the final information of all previous process) is better. So what do you actually prefer for Geronimo? IMO option 1 is the way to go, because it's easier and moreover, on option 2 there are a lot of things which can (and will) go wrong, because it could be really complex if you think of the whole Java EE enviroment. For the OSGi related problems, please take a look at MYFACES-2976 . IMHO a separate MyFaces OSGI bundle is the way to go (actually, Mojarra does exactly the same). Of course, this OSGi bundle will be created automatically by maven out of the myfaces-api and myfaces-impl artifacts and there is no need duplicate code. [1] http://download.oracle.com/javase/6/docs/api/java/util/ServiceLoader.html
          Hide
          Ivan added a comment -

          I mean the option 1

          Show
          Ivan added a comment - I mean the option 1
          Hide
          Leonardo Uribe added a comment -

          Attached to this issue there is a possible patch implementing option "2". This is not a final solution, it requires some cleanup.

          In few words, the patch does two things:

          1. Convert all interfaces on org.apache.myfaces.config.element to abstract classes that implements Serializable interface.
          2. Provide an interface like this:

          public abstract class FacesConfigurationProvider

          { public abstract FacesConfig getStandardFacesConfig(ExternalContext ectx); public abstract FacesConfig getMetaInfServicesFacesConfig(ExternalContext ectx); public abstract FacesConfig getAnnotationsFacesConfig(ExternalContext ectx, boolean metadataComplete); public abstract List<FacesConfig> getClassloaderFacesConfig(ExternalContext ectx); public abstract List<FacesConfig> getContextSpecifiedFacesConfig(ExternalContext ectx); public abstract FacesConfig getWebAppFacesConfig(ExternalContext ectx); }

          The advantage of this interface is that it allows better customization, but the disadvantage is maybe it goes too far and that kind of detail level is not necessary. Maybe keep things simple and just provide a Serializable FacesConfigDispenser (requires some changes here) is enough. But if that so, an interface with a method like this

          Serializable getFacesConfig(ExternalContext ectx)

          could be the way to go. In theory it is not necessary to expose FacesConfig or FacesConfigDispenser on the method, because the intention is just cache this value.

          Suggestions are welcome.

          Show
          Leonardo Uribe added a comment - Attached to this issue there is a possible patch implementing option "2". This is not a final solution, it requires some cleanup. In few words, the patch does two things: 1. Convert all interfaces on org.apache.myfaces.config.element to abstract classes that implements Serializable interface. 2. Provide an interface like this: public abstract class FacesConfigurationProvider { public abstract FacesConfig getStandardFacesConfig(ExternalContext ectx); public abstract FacesConfig getMetaInfServicesFacesConfig(ExternalContext ectx); public abstract FacesConfig getAnnotationsFacesConfig(ExternalContext ectx, boolean metadataComplete); public abstract List<FacesConfig> getClassloaderFacesConfig(ExternalContext ectx); public abstract List<FacesConfig> getContextSpecifiedFacesConfig(ExternalContext ectx); public abstract FacesConfig getWebAppFacesConfig(ExternalContext ectx); } The advantage of this interface is that it allows better customization, but the disadvantage is maybe it goes too far and that kind of detail level is not necessary. Maybe keep things simple and just provide a Serializable FacesConfigDispenser (requires some changes here) is enough. But if that so, an interface with a method like this Serializable getFacesConfig(ExternalContext ectx) could be the way to go. In theory it is not necessary to expose FacesConfig or FacesConfigDispenser on the method, because the intention is just cache this value. Suggestions are welcome.
          Hide
          Leonardo Uribe added a comment -

          @ServiceLoader: I have checked it and the problem is we can't "wrap" different providers like we do right now with commons-discovery. But maybe we can add some custom code that simplify this stuff, removing commons-discovery dependency, based on the code that provides ServiceLoader on apache harmony.

          Show
          Leonardo Uribe added a comment - @ServiceLoader: I have checked it and the problem is we can't "wrap" different providers like we do right now with commons-discovery. But maybe we can add some custom code that simplify this stuff, removing commons-discovery dependency, based on the code that provides ServiceLoader on apache harmony.
          Hide
          Leonardo Uribe added a comment -

          In MYFACES-2945-3.patch there is a proposal for the alternative 1. In few words, the idea is create a new abstract class called FacesConfigData and change FacesConfigurationProvider with this interface:

          public abstract class FacesConfigurationProvider

          { public abstract FacesConfigData getFacesConfigData(ExternalContext ectx); }

          After some attempts, the structure of the solution is not too different from the previous patch, because after all in DefaultFacesConfigurationProvider it is necessary to put all previous code plus ordering and feeding of FacesConfig files.

          It requires some cleanup, remove some duplicate code on FacesConfigurator, and do some test but it looks good.

          It could be good to know what do you think guys about this solution. If no objections I'll commit an updated of this solution soon.

          Show
          Leonardo Uribe added a comment - In MYFACES-2945 -3.patch there is a proposal for the alternative 1. In few words, the idea is create a new abstract class called FacesConfigData and change FacesConfigurationProvider with this interface: public abstract class FacesConfigurationProvider { public abstract FacesConfigData getFacesConfigData(ExternalContext ectx); } After some attempts, the structure of the solution is not too different from the previous patch, because after all in DefaultFacesConfigurationProvider it is necessary to put all previous code plus ordering and feeding of FacesConfig files. It requires some cleanup, remove some duplicate code on FacesConfigurator, and do some test but it looks good. It could be good to know what do you think guys about this solution. If no objections I'll commit an updated of this solution soon.
          Hide
          Jakob Korherr added a comment -

          I agree with this approach. Also I took a brief look at the patch and it looks very good. However, I haven't tried it yet.

          But generally I'd say +1 for committing it!

          Show
          Jakob Korherr added a comment - I agree with this approach. Also I took a brief look at the patch and it looks very good. However, I haven't tried it yet. But generally I'd say +1 for committing it!
          Hide
          Leonardo Uribe added a comment -

          After discuss this one on dev list, it seems better to combine these the two solutions.

          The reason why we should include the additional methods to retrieve FacesConfig files is the container could need to parse faces-config.xml files. In Geronimo case, it was used jaxb for that purpose.

          I did some additional tests and we require some fixes, to allow wrapping the default implementation, adding some wrapper classes like FacesConfigurationProviderWrapper and AnnotationProviderWrapper.

          Show
          Leonardo Uribe added a comment - After discuss this one on dev list, it seems better to combine these the two solutions. The reason why we should include the additional methods to retrieve FacesConfig files is the container could need to parse faces-config.xml files. In Geronimo case, it was used jaxb for that purpose. I did some additional tests and we require some fixes, to allow wrapping the default implementation, adding some wrapper classes like FacesConfigurationProviderWrapper and AnnotationProviderWrapper.
          Hide
          Leonardo Uribe added a comment - - edited

          Comment from Jakob Korherr (Issue MYFACES-2998 which is duplicate from here)

          Currently the FacesConfigurationProvider provides the following methods to get the FacesConfig data from different sources:

          • getStandardFacesConfig()
          • getMetaInfServicesFacesConfig()
          • getAnnotationsFacesConfig()
          • getClassloaderFacesConfig()
          • getContextSpecifiedFacesConfig()
          • getWebAppFacesConfig()

          Unfortunately it also provides a method that should combine all these data: getFacesConfigData(). This method is here due to refactorings, but IMHO this is the last place where it should be put, because it gets "its own implementation" via its Factory and then calls all of the above methods on it. I know this was introduced to support wrapping the default impl, but it really is very, very ugly.

          Because of the fact that the faces-config merging (and ordering) is a very complex task and there is only one "right" way to do it (per JSF 2.0 spec), this must not be done by a custom SPI implementation. This code should be provided by MyFaces only.

          Thus IMO the right way to solve this problem is to move all merging and ordering code into its own, MyFaces-specific class. This class (and not the SPI) should provide the getFacesConfigData() method. This means that in FacesConfigurator.configure() we should first get the FacesConfigurationProvider SPI impl and then pass it to FacesConfigMerger.getFacesConfigData().

          NOTE that with this approach custom FacesConfigurationProvider impls (like in Geronimo) do not have to deal with this complex merging and ordering stuff and their life is a lot easier!

          Show
          Leonardo Uribe added a comment - - edited Comment from Jakob Korherr (Issue MYFACES-2998 which is duplicate from here) Currently the FacesConfigurationProvider provides the following methods to get the FacesConfig data from different sources: getStandardFacesConfig() getMetaInfServicesFacesConfig() getAnnotationsFacesConfig() getClassloaderFacesConfig() getContextSpecifiedFacesConfig() getWebAppFacesConfig() Unfortunately it also provides a method that should combine all these data: getFacesConfigData(). This method is here due to refactorings, but IMHO this is the last place where it should be put, because it gets "its own implementation" via its Factory and then calls all of the above methods on it. I know this was introduced to support wrapping the default impl, but it really is very, very ugly. Because of the fact that the faces-config merging (and ordering) is a very complex task and there is only one "right" way to do it (per JSF 2.0 spec), this must not be done by a custom SPI implementation. This code should be provided by MyFaces only. Thus IMO the right way to solve this problem is to move all merging and ordering code into its own, MyFaces-specific class. This class (and not the SPI) should provide the getFacesConfigData() method. This means that in FacesConfigurator.configure() we should first get the FacesConfigurationProvider SPI impl and then pass it to FacesConfigMerger.getFacesConfigData(). NOTE that with this approach custom FacesConfigurationProvider impls (like in Geronimo) do not have to deal with this complex merging and ordering stuff and their life is a lot easier!
          Hide
          Jakob Korherr added a comment -

          If no objections, I will commit the patch on MYFACES-2998 soon!

          Show
          Jakob Korherr added a comment - If no objections, I will commit the patch on MYFACES-2998 soon!
          Hide
          Jakob Korherr added a comment -

          Just for reference: related discussion on the mailing-list: http://www.mail-archive.com/dev@myfaces.apache.org/msg50248.html

          Show
          Jakob Korherr added a comment - Just for reference: related discussion on the mailing-list: http://www.mail-archive.com/dev@myfaces.apache.org/msg50248.html
          Hide
          Jakob Korherr added a comment -

          Patch for a FacesConfigurationMerger SPI as discussed on the mailing list.

          Show
          Jakob Korherr added a comment - Patch for a FacesConfigurationMerger SPI as discussed on the mailing list.
          Hide
          Jakob Korherr added a comment -

          With the patch MYFACES-2945-FacesConfigurationMerger.patch the following is possible for Geronimo:

          public class GeronimoFacesConfigurationMerger extends FacesConfigurationMerger
          {

          private FacesConfigurationMerger myfacesProvider;

          public GeronimoFacesConfigurationMerger(FacesConfigurationMerger myfacesProvider)

          { this.myfacesProvider = myfacesProvider; }

          @Override
          public FacesConfigData getFacesConfigData(FacesConfigurationProvider facesConfigProvider, ExternalContext ectx)
          {
          FacesConfigData facesConfigData = getCachedData(ectx);

          if (facesConfigData == null)

          { // merge the data using MyFaces' FacesConfigurationMerger impl facesConfigData = myfacesProvider.getFacesConfigData(facesConfigProvider, ectx); // cache it cacheFacesConfigData(facesConfigData, ectx); }

          return facesConfigData;
          }

          private FacesConfigData getCachedData(ExternalContext ectx)

          { // TODO get cached FacesConfigData instance return null; }

          private void cacheFacesConfigData(FacesConfigData facesConfigData, ExternalContext ectx)

          { // TODO cache data }

          }

          If no objections, I will commit this patch soon!

          Show
          Jakob Korherr added a comment - With the patch MYFACES-2945 -FacesConfigurationMerger.patch the following is possible for Geronimo: public class GeronimoFacesConfigurationMerger extends FacesConfigurationMerger { private FacesConfigurationMerger myfacesProvider; public GeronimoFacesConfigurationMerger(FacesConfigurationMerger myfacesProvider) { this.myfacesProvider = myfacesProvider; } @Override public FacesConfigData getFacesConfigData(FacesConfigurationProvider facesConfigProvider, ExternalContext ectx) { FacesConfigData facesConfigData = getCachedData(ectx); if (facesConfigData == null) { // merge the data using MyFaces' FacesConfigurationMerger impl facesConfigData = myfacesProvider.getFacesConfigData(facesConfigProvider, ectx); // cache it cacheFacesConfigData(facesConfigData, ectx); } return facesConfigData; } private FacesConfigData getCachedData(ExternalContext ectx) { // TODO get cached FacesConfigData instance return null; } private void cacheFacesConfigData(FacesConfigData facesConfigData, ExternalContext ectx) { // TODO cache data } } If no objections, I will commit this patch soon!
          Hide
          Leonardo Uribe added a comment -

          I think it is not necessary to pass FacesConfigurationProvider as param for getFacesConfigData, because in theory we don't do anything with it before pass it and wrappers will not do anything with it later. I think it looks good to load it using FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalContext).

          Show
          Leonardo Uribe added a comment - I think it is not necessary to pass FacesConfigurationProvider as param for getFacesConfigData, because in theory we don't do anything with it before pass it and wrappers will not do anything with it later. I think it looks good to load it using FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalContext).
          Hide
          Jakob Korherr added a comment -

          Hehe. I actually had it this way first, but then - when writing the code in FacesConfigurator - it felt kinda weird to get the whole FacesConfigData only from the **Merger SPI without even touching the **Provider SPI.

          For me it made more sence to get the **Provider first and then to pass it to the **Merger, so that you see on FacesConfigurator that there are acutally two SPIs involved.

          However, I am also ok to do it the other way. WDYT after this little explanation? Please tell me your final opinion and I'll commit an appropriate version of the proposed patch!

          Show
          Jakob Korherr added a comment - Hehe. I actually had it this way first, but then - when writing the code in FacesConfigurator - it felt kinda weird to get the whole FacesConfigData only from the **Merger SPI without even touching the **Provider SPI. For me it made more sence to get the **Provider first and then to pass it to the **Merger, so that you see on FacesConfigurator that there are acutally two SPIs involved. However, I am also ok to do it the other way. WDYT after this little explanation? Please tell me your final opinion and I'll commit an appropriate version of the proposed patch!
          Hide
          Leonardo Uribe added a comment -

          I think it is better do not include it as parameter and instead add some comment on the javadoc, saying the information to take into account is retrieved from FacesConfigurationProvider. The fact of use FacesConfigurationProvider is more an implementation detail than a requeriment, so if somebody wants to do not use it is valid usage.

          Show
          Leonardo Uribe added a comment - I think it is better do not include it as parameter and instead add some comment on the javadoc, saying the information to take into account is retrieved from FacesConfigurationProvider. The fact of use FacesConfigurationProvider is more an implementation detail than a requeriment, so if somebody wants to do not use it is valid usage.
          Hide
          Jakob Korherr added a comment -

          As discussed, I just committed a modified version of the proposed patch (MYFACES-2945-FacesConfigurationMerger.patch).

          The FacesConfigurationMerger SPI now does not get the FacesConfigurationProvider directly via parameter, but retrieves it via FacesConfigurationProviderFactory if needed.

          I'll also attach a modified version of the GeronimoFacesConfigurationMerger implementation that I used for testing just as a reference to this issue.

          If the Geronimo team is happy with this solution, we can resolve this issue!

          Show
          Jakob Korherr added a comment - As discussed, I just committed a modified version of the proposed patch ( MYFACES-2945 -FacesConfigurationMerger.patch). The FacesConfigurationMerger SPI now does not get the FacesConfigurationProvider directly via parameter, but retrieves it via FacesConfigurationProviderFactory if needed. I'll also attach a modified version of the GeronimoFacesConfigurationMerger implementation that I used for testing just as a reference to this issue. If the Geronimo team is happy with this solution, we can resolve this issue!
          Hide
          Jakob Korherr added a comment -

          GeronimoFacesConfigurationMerger SPI impl used for testing.

          Show
          Jakob Korherr added a comment - GeronimoFacesConfigurationMerger SPI impl used for testing.

            People

            • Assignee:
              Leonardo Uribe
              Reporter:
              Ivan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development