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

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          2m 16s 1 Leonardo Uribe 01/Dec/10 02:15
          Patch Available Patch Available Resolved Resolved
          14h 1 Leonardo Uribe 01/Dec/10 16:15
          Resolved Resolved Reopened Reopened
          17m 43s 1 Jakob Korherr 01/Dec/10 16:33
          Reopened Reopened Resolved Resolved
          1h 59m 1 Leonardo Uribe 01/Dec/10 18:32
          Resolved Resolved Closed Closed
          12d 6h 28m 1 Leonardo Uribe 14/Dec/10 01:01
          Leonardo Uribe made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Leonardo Uribe made changes -
          Fix Version/s 2.0.3 [ 12315976 ]
          Fix Version/s 2.0.3-SNAPSHOT [ 12315349 ]
          Jakob Korherr made changes -
          Component/s SPI [ 12314112 ]
          Hide
          Jakob Korherr added a comment -

          Ok, good solution

          Show
          Jakob Korherr added a comment - Ok, good solution
          Leonardo Uribe made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          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 -

          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, 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.
          Jakob Korherr made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          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
          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?
          Leonardo Uribe made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 2.0.3-SNAPSHOT [ 12315349 ]
          Resolution Fixed [ 1 ]
          Leonardo Uribe made changes -
          Attachment MYFACES-2986-1.patch [ 12465019 ]
          Leonardo Uribe made changes -
          Attachment MYFACES-2986-2.patch [ 12465021 ]
          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.
          Leonardo Uribe made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Leonardo Uribe made changes -
          Attachment MYFACES-2986-1.patch [ 12465019 ]
          Leonardo Uribe made changes -
          Link This issue is related to MYFACES-2944 [ MYFACES-2944 ]
          Leonardo Uribe made changes -
          Field Original Value New Value
          Link This issue is related to MYFACES-2945 [ MYFACES-2945 ]
          Leonardo Uribe created issue -

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development