11 Replies Latest reply on May 23, 2010 11:22 AM by kenglxn

    SHRINKWRAP-163: Making the Archive name optional

    alrubinger

      Ken's submitted a patch to:

       

      https://jira.jboss.org/browse/SHRINKWRAP-163

       

      Some review.

       

      1) JavaDocs.  As noted in Ken's TODOs, yep, we'll put these in before commit.

       

      2) ExtensionType:

         @Override
         public String toString()
         {
            return "." + extension;
         }
      

       

      The "." can be a final static char.

       

      3) ConfigurationBuilder / Configuration:

      public ConfigurationBuilder assignableExtensionTypeMapping(final Map<Class<?>, ExtensionType> assignableExtensionTypeMapping)

       

      Can we find a cleaner name?  "getExtensionMapping"?

       

      4) ShrinkWrap/ArchiveFactory:

      @Deprecated
         public static <T extends Assignable> T create(final String archiveName, final Class<T> type)
      

       

      Let's note in the JavaDocs why it's deprecated, and point to the new stuff.

       

      5) ShrinkWrapTestCase:

      @Test
         public void shouldCreateArchiveWithCorrectExtensionBasedOnType() throws Exception
         {
            JavaArchive javaArchive = ShrinkWrap.create(JavaArchive.class);
            Assert.assertTrue("JavaArchive should have proper extension: " + ExtensionType.JAR, javaArchive.getName().endsWith(ExtensionType.JAR.toString()));
      
            WebArchive webArchive = ShrinkWrap.create(WebArchive.class);
            Assert.assertTrue("WebArchive should have proper extension: " + ExtensionType.WAR, webArchive.getName().endsWith(ExtensionType.WAR.toString()));
      
            EnterpriseArchive enterpriseArchive = ShrinkWrap.create(EnterpriseArchive.class);
            Assert.assertTrue("EnterpriseArchive should have proper extension: " + ExtensionType.EAR, enterpriseArchive.getName().endsWith(ExtensionType.EAR.toString()));
      
            ResourceAdapterArchive resourceAdapterArchive = ShrinkWrap.create(ResourceAdapterArchive.class);
            Assert.assertTrue("ResourceAdapterArchive should have proper extension: " + ExtensionType.RAR, resourceAdapterArchive.getName().endsWith(ExtensionType.RAR.toString()));
         }
      

       

      If we honor Kent Beck fully, these checks may each get their own test.

       

      6) ArchiveFactory:

      public <T extends Assignable> T create(final Class<T> type)
            throws IllegalArgumentException
         {
            // Precondition checks
            if (type == null)
            {
               throw new IllegalArgumentException("Type must be specified");
            }
      
            String archiveName = UUID.randomUUID().toString();
      
            ExtensionType extensionType = configuration.getAssignableExtensionTypeMapping().get(type);
            if (extensionType != null)
            {
               archiveName += extensionType;
            }
      
            return create(type, archiveName);
         }
      

       

      I try to avoid logic like this.  If there's a mapping, we append to the name.  Else we don't.  I'd probably opt for an unchecked exception saying something like "The current configuration has no mapping for type X, unable to determine extension.  Either add a mapping via Configuration.getExtensionMapping or manually assign a name."  Thoughts?

       

      7) Test Coverage

       

      Coburtura shows we're missing tests for:

       

      ShrinkWrap:

      public static <T extends Assignable> T create(final Class<T> type, final String archiveName)
            throws IllegalArgumentException

      public static <T extends Assignable> T create(final Class<T> type, final String archiveName, final ExtensionType extensionType)
            throws IllegalArgumentException

      ArchiveFactory:

      public <T extends Assignable> T create(final Class<T> type, final String archiveName, final ExtensionType extensionType)
            throws IllegalArgumentException

       

      Otherwise real nice.

       

      S,

      ALR

        • 1. Re: SHRINKWRAP-163: Making the Archive name optional
          aslak

          I haven't been following this to closely, but I've read up on the jira issue.

           

          Just one little clearification;

           

          The output of the issue is to be able to crate a 'no-named' archive, where 'no-name' is a random generated name. I can see the benefit of  only specifying the type as a shortcut (I'm a bit unclear on the use case outside a temporary-throwaway archive tho). So the Spec Interface(WebArchive etc) to  ExtensionType mapping is needed to add the appropriate extension to the generated name.

           

          But why expose  ExtensionType as part of the API? I don't understand the benefit or usecase for:

           

          {code:java}

          public static <T extends Assignable> T create(final Class<T> type, final String archiveName, final ExtensionType extensionType)
                throws IllegalArgumentException

          {code}

           

          I would say it creates confusion to what archiveName is suppose to be. And since you can create a Archive with only Type and it figures out the ExtensionType, why can't I create a Archive with a Type and archiveName and it figures it out for me there as well.

           

          ?

           

          And a comment to the implementation:

           

          The Type to ExtentionType mapping is hardcoded in the ConfigurationBuilder for the Types we provide. Shouldn't we provide a SPI to auto load other peoples mapping as well. So a user can write the following without having to go to the full length v:

           

          {code:java}

          ShrinkWrap.create(ThirdPartyArchive.class)

          {code}

          • 2. Re: SHRINKWRAP-163: Making the Archive name optional
            kenglxn

            All good points. I'll adjust implementation according to these points.

             

            in regards to 6) i agree as well. Updating to:

             

            {code:java}

            public <T extends Assignable> T create(final Class<T> type)

             

                  throws IllegalArgumentException

             

               {

             

                  // Precondition checks

             

                  if (type == null)

             

                  {

             

                     throw new IllegalArgumentException("Type must be specified");

             

                  }

             

                  ExtensionType extensionType = configuration.getAssignableExtensionTypeMapping().get(type);

             

                        if (extensionType != null)

             

                        {

             

                             throw new IllegalArgumentException("The current configuration has no mapping for type X, unable to determine extension.  Either add a mapping via Configuration.getExtensionMapping or manually assign a name.");

             

                        }

             

                  String archiveName = UUID.randomUUID().toString();
             

             

                  return create(type, archiveName);

             

               }

            {code}

            Also, Aslak has a good point in regards to extending the ExtensionMapping.

            I played around a bit, and made a test where i append a new ExtensionType to the mapping, but I get an exception:

             

             

            {code:java}

            ExtensionType customExtensionType = new ExtensionType("mar");

             

            ShrinkWrap.getDefaultDomain().getConfiguration().getAssignableExtensionTypeMapping().put(MemoryMapArchive.class , customExtensionType);

             

            MemoryMapArchive mar = ShrinkWrap.create(MemoryMapArchive.class);

            {code}

            Gives the following:

             

            {code}

            java.lang.RuntimeException: Could not find any mapping for extension org.jboss.shrinkwrap.spi.MemoryMapArchive

             

                at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.findExtensionImpl(ServiceExtensionLoader.java:180)

                at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.loadExtension(ServiceExtensionLoader.java:148)

                at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.createFromLoadExtension(ServiceExtensionLoader.java:136)

                at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.load(ServiceExtensionLoader.java:72)

                at org.jboss.shrinkwrap.impl.base.ArchiveBase.as(ArchiveBase.java:329)

                at org.jboss.shrinkwrap.api.ArchiveFactory.create(ArchiveFactory.java:140)

                at org.jboss.shrinkwrap.api.ArchiveFactory.create(ArchiveFactory.java:112)

                at org.jboss.shrinkwrap.api.ShrinkWrap.create(ShrinkWrap.java:172)

                at org.jboss.shrinkwrap.impl.base.ShrinkWrapTestCase.shouldCreateArchiveWithCorrectExtensionBasedOnType(ShrinkWrapTestCase.java:206)

             

            ...

             

            Caused by: java.lang.RuntimeException: No extension implementation found for org.jboss.shrinkwrap.spi.MemoryMapArchive, please verify classpath or add a extensionOverride

            {code}

            • 3. Re: SHRINKWRAP-163: Making the Archive name optional
              aslak

              Gives the following:

               

              java.lang.RuntimeException: Could not find any mapping for extension org.jboss.shrinkwrap.spi.MemoryMapArchive    at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.findExtensionImpl(ServiceExtensionLoader.java:180)    at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.loadExtension(ServiceExtensionLoader.java:148)    at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.createFromLoadExtension(ServiceExtensionLoader.java:136)    at org.jboss.shrinkwrap.impl.base.ServiceExtensionLoader.load(ServiceExtensionLoader.java:72)    at org.jboss.shrinkwrap.impl.base.ArchiveBase.as(ArchiveBase.java:329)    at org.jboss.shrinkwrap.api.ArchiveFactory.create(ArchiveFactory.java:140)    at org.jboss.shrinkwrap.api.ArchiveFactory.create(ArchiveFactory.java:112)    at org.jboss.shrinkwrap.api.ShrinkWrap.create(ShrinkWrap.java:172)    at org.jboss.shrinkwrap.impl.base.ShrinkWrapTestCase.shouldCreateArchiveWithCorrectExtensionBasedOnType(ShrinkWrapTestCase.java:206)

              ...

              Caused by: java.lang.RuntimeException: No extension implementation found for org.jboss.shrinkwrap.spi.MemoryMapArchive, please verify classpath or add a extensionOverride

               

              MemoryMapArchive is not a loadable type in the Assignable fashion. It's missing the MemoryMapArchive SPI file to define a implementation and is really only meant to be a internal class. The Type extension model to day works like this: you give it a Interface and it tries to find the implementation based on the standard Java SPI model, looking for a file in classpath under /META-INF/services/Interface that contain the implementation class name.

               

              But that brings up a interesting point, we're missing the ability to just create a Archive, it has to be wrapped in some Type.

               

              {code:java}

              Archive<?> archive = ShrinkWrap.create(Archive.class, "archive.zip");

              {code}

               

              Maybe even default to Archive when nothing is given:

               

              {code:java}

              Archive<?> archive = ShrinkWrap.create();

              {code}

              • 4. Re: SHRINKWRAP-163: Making the Archive name optional
                kenglxn

                Aslak Knutsen wrote:

                MemoryMapArchive is not a loadable type in the Assignable fashion.

                Missed that. But I was trying to just supply an Archive that wasn't in the mapping without a lot of effort .

                 

                Essentially what I want is to test the use case where a user suplies his own type that is not in the mapping.

                • 5. Re: SHRINKWRAP-163: Making the Archive name optional
                  kenglxn

                  just a little copy paste from #jbosstesting irc dialog:

                   

                  (01:59:45 PM) kenglxn: aslak, just wondered about some input on http://community.jboss.org/message/544367#544367
                  (01:59:52 PM) kenglxn: if you have a minute
                  (01:59:59 PM) aslak: sure
                  (02:00:40 PM) aslak: create a Interface that extends Archive
                  (02:01:19 PM) aslak: create a implementation of that interface that extends AssignableBase
                  (02:01:59 PM) aslak: create a constructor on that implementation that takes a Archive<?> or any other extension, like JavaArchive
                  (02:02:44 PM) aslak: create a file under META-INF/services/fully-qualified-internface-name that container the fully-qualified-implementation-nmae
                  (02:03:34 PM) aslak: based on that, you should be able to do both, ShrinkWrap.create(MyType) and archive.as(MyType)
                  (02:04:32 PM) kenglxn: aslak: sweet
                  (02:05:04 PM) aslak: it's probably easier to let your implementation extend ContainerBase then AssignableBase, then your don't have to implement all the Archive interface delegations
                  (02:06:09 PM) kenglxn: aslak, this sounds like a case that will need to be well documented
                  (02:06:28 PM) aslak: all that is really needed is for the implementation to extend the interface and implement Assignable, both AssignableBase and ContianerBase help out with some of the basic plumming
                  (02:06:46 PM) aslak: yea
                  (02:06:49 PM) kenglxn: I see
                  (02:07:30 PM) kenglxn: so from a users perspective, we should probably have som doc for: "I want to supply my own archive type"
                  (02:07:44 PM) kenglxn: that describes what you just said
                  (02:08:05 PM) kenglxn: If we create a testcase that does this, then it is somewhat documented there
                  (02:08:10 PM) kenglxn: what do you think?
                  (02:09:32 PM) kenglxn: aslak, do you see this falling in under SW-163?
                  (02:10:08 PM) kenglxn: or perhaps it is a new task, Allow users to supply their own archivetype
                  (02:12:41 PM) aslak: users can supply their own archive interface today, it's just very well documented. so a documentation task on sure
                  (02:14:21 PM) kenglxn: aslak, ok, and I'll just add a testcase for it under 163
                  (02:14:27 PM) aslak: i would say that since SHRINKWRAP-163 provide ExtensionType, it could provide support for custom archive types as welll. but i hvae no problem splitting it in two
                  (02:14:28 PM) jbossbot: [SHRINKWRAP-163] Make archive name optional in ShrinkWrap#create() [Coding In Progress, Major, Ken Gullaksen] https://jira.jboss.org/browse/SHRINKWRAP-163
                  (02:15:00 PM) aslak: and that should be, _not_ very well documented..
                  (02:15:11 PM) kenglxn: yea, i figured
                  (02:15:32 PM) aslak: i think we have a general 'create doc/reference guide' doc task
                  (02:16:14 PM) aslak: there is a doc module as well, just not much in it. just some structure i started at some point
                  (02:18:54 PM) aslak: kenglxn, what testcase were you going to add?
                  (02:19:54 PM) kenglxn: aslak, so the ExtensionTypeMapping was implemented to help us determine the ExtensionType for the ShrinkWrap.create(type) only. And then we throw an exeption if the mapping is not found. So if a user supplies their own type, then they also need to do the steps you mentioned here. Thus I figure perhaps I should add a testcase that do those steps, to ensure it all goes like we expect
                  (02:20:22 PM) kenglxn: aslak,v  something like userShouldBeAbleToSupplyOwnType
                  (02:20:40 PM) aslak: kenglxn, aa yes
                  (02:22:35 PM) kenglxn: sorry not ExtensionTypeMapping like a class, but the field assignableExtensionTypeMapping which will be changed to extensionTypeMapping.
                  (02:22:51 PM) kenglxn: in the Configuration and ConfigurationBuilder
                  (02:23:29 PM) kenglxn: aslak, so I mean the ExtensionType class and assignableExtensionTypeMapping field, just to clear up
                  • 6. Re: SHRINKWRAP-163: Making the Archive name optional
                    alrubinger

                    Aslak Knutsen wrote:

                    But why expose  ExtensionType as part of the API? I don't understand the benefit or usecase for:

                     

                    public static <T extends Assignable> T create(final Class<T> type, final String archiveName, final ExtensionType extensionType)      throws IllegalArgumentException
                    

                    I've been wondering this myself.  Last Ken and I spoke we chose to leave it in, but I don't think it really makes sense.  Unless Dan (who opened the ticket) has a usecase, I say nix this method, and tuck away ExtensionType as package-private or something in the API.

                     

                    Aslak Knutsen wrote:

                    The Type to ExtentionType mapping is hardcoded in the ConfigurationBuilder for the Types we provide. Shouldn't we provide a SPI to auto load other peoples mapping as well. So a user can write the following without having to go to the full length v:

                     

                    ShrinkWrap.create(ThirdPartyArchive.class)
                    

                     

                     

                    Just a note that the default mappings are hardcoded, but mutable.

                     

                    configuration.getExtensionMappings().add(...);

                     

                    We could do some mechanism similar to the current "extensionOverrides" thing (another name we should likely improve).

                     

                    S,

                    ALR

                    • 7. Re: SHRINKWRAP-163: Making the Archive name optional
                      alrubinger

                      Ken Gullaksen wrote:

                      in regards to 6) i agree as well. Updating to:

                       

                      public <T extends Assignable> T create(final Class<T> type)
                       
                            throws IllegalArgumentException
                       
                         {
                       
                      ...
                       
                      ...              throw new IllegalArgumentException("The current configuration has no mapping for type X, unable to determine extension.  Either add a mapping via Configuration.getExtensionMapping or manually assign a name.");
                       
                         }
                      

                       

                      "Type X" in the exception message is actually the variable Class<T> "type" in the real patch now, right?

                       

                      S,

                      ALR

                      • 8. Re: SHRINKWRAP-163: Making the Archive name optional
                        kenglxn

                        Andrew Rubinger wrote:

                        "Type X" in the exception message is actually the variable Class<T> "type" in the real patch now, right?

                         

                        S,

                        ALR

                        Of course

                        • 9. Re: SHRINKWRAP-163: Making the Archive name optional
                          kenglxn

                          Ok, I'll nix the create with the argument for ExtensionType.

                           

                          Aslak and I had a dialog where I was wondering on the use case where a user supplies their own Archive type.

                          This probably needs to be documented, but I figure a testcase for it is supported is a start.

                           

                          So in this case I just need some clarification.

                           

                          When ShrinkWrap.create(UserSuppliedArchive.class) is invoked, it will throw an Exception when trying to lookup the ExtensionType, unless a user has added their own.

                          Like:

                           

                          configuration.getExtensionMappings().add(...);

                           

                          In this case the user will supply a new ExtensionType("zap") for the UserSuppliedArchive.class as key.

                           

                          WDYT?

                          • 10. Re: SHRINKWRAP-163: Making the Archive name optional
                            aslak

                            When ShrinkWrap.create(UserSuppliedArchive.class) is invoked, it will throw an Exception when trying to lookup the ExtensionType, unless a user has added their own.

                            Like:

                             

                            configuration.getExtensionMappings().add(...);

                             

                             

                            In this case the user will supply a new ExtensionType("zap") for the UserSuppliedArchive.class as key.

                             

                            WDYT?

                            When a User use an ArchiveType provided by FrameworkX he should use the FrameworkXArchive in the same way he would a ShrinkWrap provided one. Same goes for the FrameworkX developer, he should be able to provide the user with the same experience as ShrinkWrap developers can.

                             

                            We have the possibility with ExtensionLoader to *override* the default defined ones. These are default dynamically loaded based on metadata provided by the developer. The only case for this is if the User wants to provide a different implementation for e.g. JavaArchive.

                            But that is not the same as the user having to provide the ExtensionTypeMappings him self if he wants to use the FrameworkXArchive.

                             

                            ExtensionTypeMappings should be metadata provided along side the ArchiveType definition/implementation.

                            • 11. Re: SHRINKWRAP-163: Making the Archive name optional
                              kenglxn

                              Ok.

                              Then I'll nix the ShrinkWrap.create(type, archiveName, extensionType) and hide ExtensionType as package private as well.

                              I'll submit a patch with the new stuff very soon