MyFaces Core
  1. MyFaces Core
  2. MYFACES-2986

Provide an interface to override how to find spi interfaces

    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: JSR-314, SPI
    • Labels:
      None

      Description

      This is the last step to solve MYFACES-2944 and MYFACES-2945 problem related to OSGi and SPI. Now it is possible to call

      ServiceLoaderFinderFactory.setServiceLoaderFinder(ExternalContext ectx, ServiceLoaderFinder slp)

      or

      ServiceLoaderFinderFactory.setServiceLoaderFinder(ServletContext ctx, ServiceLoaderFinder slp)

      Just before initialization to set a ServiceLoaderFinder that will be used later to locate SPI interfaces. In this way, it is possible to provide a code that looks SPI interfaces using OSGi bundles.

      1. MYFACES-2986-2.patch
        52 kB
        Leonardo Uribe

        Issue Links

          Activity

          Hide
          Leonardo Uribe added a comment -

          If no objections, I'll commit this code soon. Note the proposed patch removes commons-discovery dependency.

          Show
          Leonardo Uribe added a comment - If no objections, I'll commit this code soon. Note the proposed patch removes commons-discovery dependency.
          Hide
          Jakob Korherr added a comment -

          The patch looks good! It is very great that we can finally get rid of commons-discovery!

          Two comments:

          1) I don't really like the name "ServiceLoaderFinder". This would suggest a class which finds "ServiceLoaders", but it actually finds ServiceProviders. Thus I'd like to propose to change the name to "ServiceProviderFinder".

          2) the patch always uses the first SPI class from the List:

          if (classList != null && !classList.isEmpty())

          { return ClassUtils.newInstance(classList.get(0)); }

          I think it would be better here to throw an Exception if there is more than one fitting SPI implementation, because right now it is not really clear which one will be used in this case ( --> does the default ServiceLoaderFinder always return the service classes in the same order?).

          WDYT?

          Show
          Jakob Korherr added a comment - The patch looks good! It is very great that we can finally get rid of commons-discovery! Two comments: 1) I don't really like the name "ServiceLoaderFinder". This would suggest a class which finds "ServiceLoaders", but it actually finds ServiceProviders. Thus I'd like to propose to change the name to "ServiceProviderFinder". 2) the patch always uses the first SPI class from the List: if (classList != null && !classList.isEmpty()) { return ClassUtils.newInstance(classList.get(0)); } I think it would be better here to throw an Exception if there is more than one fitting SPI implementation, because right now it is not really clear which one will be used in this case ( --> does the default ServiceLoaderFinder always return the service classes in the same order?). WDYT?
          Hide
          Jakob Korherr added a comment -

          ..some comments to the patch (which was committed a bit too fast)..

          Show
          Jakob Korherr added a comment - ..some comments to the patch (which was committed a bit too fast)..
          Hide
          Leonardo Uribe added a comment -

          1) Ok, use ServiceProviderFinder sounds better, so I commited the necessary changes.

          2) Well, in theory in the classpath there will not be multiple SPI implementations, because the idea is each server container provide its own implementations (note LifecycleProvider is an exception to this rule). Pick the first one is the same as commons-discovery does. About the ordering problem, we can't do too much, because it is a problem related to SPI itself. But anyway, we always provide two ways to override a SPI provider. For example, if someone wants to override AnnotationProvider it can create a custom one under this key:

          META-INF/services/org.apache.myfaces.spi.AnnotationProvider

          or he/she can override the factory to define a custom ordering or something else:

          META-INF/services/org.apache.myfaces.spi.AnnotationProviderFactory

          But maybe we should log an informative message indicating there are two possible SPI providers, instead prevent initialize myfaces.

          Show
          Leonardo Uribe added a comment - 1) Ok, use ServiceProviderFinder sounds better, so I commited the necessary changes. 2) Well, in theory in the classpath there will not be multiple SPI implementations, because the idea is each server container provide its own implementations (note LifecycleProvider is an exception to this rule). Pick the first one is the same as commons-discovery does. About the ordering problem, we can't do too much, because it is a problem related to SPI itself. But anyway, we always provide two ways to override a SPI provider. For example, if someone wants to override AnnotationProvider it can create a custom one under this key: META-INF/services/org.apache.myfaces.spi.AnnotationProvider or he/she can override the factory to define a custom ordering or something else: META-INF/services/org.apache.myfaces.spi.AnnotationProviderFactory But maybe we should log an informative message indicating there are two possible SPI providers, instead prevent initialize myfaces.
          Hide
          Jakob Korherr added a comment -

          1) OK, great! But please also change DefaultServiceLoaderFinder to DefaultServiceProviderFinder (I think you forgot this one in your last commit)

          2) Yes, in theory there should only be (maximum) one SPI impl per SPI, however a user may misconfigure his classpath or copy/paste the wrong code and thus get two SPI impls for one SPI. In this case it is not clear what happens, so we should really throw an exception (at least when running in development mode).

          Show
          Jakob Korherr added a comment - 1) OK, great! But please also change DefaultServiceLoaderFinder to DefaultServiceProviderFinder (I think you forgot this one in your last commit) 2) Yes, in theory there should only be (maximum) one SPI impl per SPI, however a user may misconfigure his classpath or copy/paste the wrong code and thus get two SPI impls for one SPI. In this case it is not clear what happens, so we should really throw an exception (at least when running in development mode).
          Hide
          Leonardo Uribe added a comment -

          1) Ok, I committed the necessary changes.

          2) Unfortunately in this point we only have an ExternalContext instance, so we can't check for project stage. I added a descriptive warning message indicating what's happening and the action taken. I think is the best we can do. Anyway, I already restored the code to override a Factory (allow AnnotationProviderFactory.setAnnotationProviderFactory ) so in the worst case users could create a custom initializer and call this method if something is not well configured.

          Show
          Leonardo Uribe added a comment - 1) Ok, I committed the necessary changes. 2) Unfortunately in this point we only have an ExternalContext instance, so we can't check for project stage. I added a descriptive warning message indicating what's happening and the action taken. I think is the best we can do. Anyway, I already restored the code to override a Factory (allow AnnotationProviderFactory.setAnnotationProviderFactory ) so in the worst case users could create a custom initializer and call this method if something is not well configured.
          Hide
          Jakob Korherr added a comment -

          Ok, good solution

          Show
          Jakob Korherr added a comment - Ok, good solution

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development