10 Replies Latest reply on Dec 11, 2008 9:52 PM by jim.ma

    Wise API reviewing

    maeste

      Hi folks,
      I've started a branch for WISE-34 where I'm reviewing our API to make it more general (in particular to support JAX-RS and WS-*).
      To have an idea of what I have in mind you can take a look to WISE-34 issue and its subtasks.

      I have just made a first commit where I have reviewed our WS-* API, and of course of our current implementation of wsse and MTOM (ok, it isn't a WS-* and in fact I called new api package wsextension because these APIs are intended to support all extension, not only WS-*).

      I haven't yet written unit tests and javadocs (it's late night here now, I'l try to do something tomorrow morning), but you can already take a look ad give me your feedback.
      The basic idea is to use a generic interface to add all ws-* to an endpoint and delegate to a visitor the real operation of enabling WS-*. Of course the instance of visitor implementation is injected by IOC (JBoss MC) to support different implementation (aka native, metro etc).
      Also configurations (eventually needed by single WS-*) are injected by IOC to make easier to enable WS-* by pure configuration files (near zero code approach mainly, but not only, for ESB).


      Any comments are more than welcome.

        • 1. Re: Wise API reviewing
          jim.ma

          I mostly like your idea and implementation . What I like is using same common API to enable WS-Security , MTOM , WS-Addressing and WS-RM. What I do not make sure I like is it mixed the the wise core runtime configuration with user configuration for enable WS-*.

          When user uses or integrate Wise , he or she will import the jars related to wise runtime .
          The jboss-beans.xml will be packaged into jar file. We should not expect user unzip that jar and modify the META-INF/jboss-beans.xml to enable WS-security. If it's feasible, we should provide a separated configuration file for user to add WS-* configuration .

          I see you introduced new class WSExtensionVisitor , but I do not find out where it will visit the WSEndpoint class. Now the behavior is WSSecurityEnabler/MTOMEnabler invoking the related method in WSExtensionVisitor and the method in WSExtensionVistor is responsible for do the real job to enable WS-*. So I say WSExtensionVistor does not really visit the WSEndpoint . Do you plan to modify this API to do something like this :

           public class WSExtensionVistor {
           public void visitWSSecurity() {
           ....
           }
          
           public void visitWSRM() {
           ....
           }
          
           public void visitWSAddressing() {
           ....
           }
          
           public void visit(WSEndpoint point ) {
           visitWSSecurity();
           visitWSRM();
           visitWSAddressing();
           }
          
           }
          


          And another thing I think we can refactor the NativeSecurityConfig class. I suggest to use a property map to store the user configuration instead of using get and set method for a specific property . When we need other property key value pairs we should add other get/set methods . It is not easily extendible, right ?

          Just my 2 cents. Correct me if I missed something .

          Jim

          • 2. Re: Wise API reviewing
            maeste

             

            "jim.ma" wrote:
            I mostly like your idea and implementation . What I like is using same common API to enable WS-Security , MTOM , WS-Addressing and WS-RM. What I do not make sure I like is it mixed the the wise core runtime configuration with user configuration for enable WS-*.

            When user uses or integrate Wise , he or she will import the jars related to wise runtime .
            The jboss-beans.xml will be packaged into jar file. We should not expect user unzip that jar and modify the META-INF/jboss-beans.xml to enable WS-security. If it's feasible, we should provide a separated configuration file for user to add WS-* configuration .

            I don't agree here. jboss-beans.xml is the only config file where isr put their own configurations like "keepsource", "verbose" WS-* related and so on. They are specific "user space" configuration that final user have to set up for his own needs. I think wise-core.jar must not include jboss-beans.xml into its jar, but we have to ask users to put their own jboss-beans.xml as part of their project (you are right here we have to explain that better in our user manual).
            IOW jboss-beans.xml is substituting wise-core.properties, the former way to define user's configurations.

            I see you introduced new class WSExtensionVisitor , but I do not find out where it will visit the WSEndpoint class. Now the behavior is WSSecurityEnabler/MTOMEnabler invoking the related method in WSExtensionVisitor and the method in WSExtensionVistor is responsible for do the real job to enable WS-*. So I say WSExtensionVistor does not really visit the WSEndpoint .

            Ok you are right it isn't a perfect visitor pattern. In fact I started to write this code thinking to just add WSExtensions to endpoint and implement a "visitAll" method which would scan the WSExtension collection to call enable and so visit all the element (the extension). But it was one more call in charge of users, so I decide to "visit" single elements during addition...I know it's a border line implementation of visitor pattern (or perhaps it isn't a visitor at all), but I think it could work for us and keep it simpler. Maybe we can rename this class? Or maybe not. Opinions?


            And another thing I think we can refactor the NativeSecurityConfig class. I suggest to use a property map to store the user configuration instead of using get and set method for a specific property . When we need other property key value pairs we should add other get/set methods . It is not easily extendible, right ?

            Well, but we really need to make NativeSecurityConfig easily extensible (if jbossws sec will not change, I think it will not ever change too), loosing part of its easy readability? I made a value object here because I think it's better to have code easy to read and understand and easy to IOC (and it is in general my style, but I'd like to discuss about). With a prperty map users have to know valid value for keys while a value Object auto explain itself. As said it's just a matter of style, no problem for me to change it if you can see real advantages somewhere.



            Just my 2 cents. Correct me if I missed something .
            Jim

            Thanks for all good points.

            • 3. Re: Wise API reviewing
              maeste

              Oki I've just committed unit tests and javadoc of WS-* new APIs.
              Please have a look and give me your feedback.

              • 4. Re: Wise API reviewing
                jim.ma

                 

                "maeste" wrote:

                I don't agree here. jboss-beans.xml is the only config file where isr put their own configurations like "keepsource", "verbose" WS-* related and so on. They are specific "user space" configuration that final user have to set up for his own needs. I think wise-core.jar must not include jboss-beans.xml into its jar, but we have to ask users to put their own jboss-beans.xml as part of their project (you are right here we have to explain that better in our user manual).
                IOW jboss-beans.xml is substituting wise-core.properties, the former way to define user's configurations.

                Maybe we need to split the configuration for wise-core (for example , the WSDynamicClientFactory configuration) in a separated file . You see user who only want to use wise to call a web service does not need to know this.

                "maeste" wrote:

                Ok you are right it isn't a perfect visitor pattern. In fact I started to write this code thinking to just add WSExtensions to endpoint and implement a "visitAll" method which would scan the WSExtension collection to call enable and so visit all the element (the extension). But it was one more call in charge of users, so I decide to "visit" single elements during addition...I know it's a border line implementation of visitor pattern (or perhaps it isn't a visitor at all), but I think it could work for us and keep it simpler. Maybe we can rename this class? Or maybe not. Opinions?

                I am disposed to rename it if it is not real a visitor pattern class.


                "maeste" wrote:

                Well, but we really need to make NativeSecurityConfig easily extensible (if jbossws sec will not change, I think it will not ever change too), loosing part of its easy readability? I made a value object here because I think it's better to have code easy to read and understand and easy to IOC (and it is in general my style, but I'd like to discuss about). With a prperty map users have to know valid value for keys while a value Object auto explain itself. As said it's just a matter of style, no problem for me to change it if you can see real advantages somewhere.

                If I remembered correctly, we also can inject a map or list into a bean container. Yes, it's just matter of style, they both can achieve same thing. If use get/set style, there are many fields needs to be added to this class to support various style WS-Security. Use a property map can simplify it.


                • 5. Re: Wise API reviewing
                  maeste

                   

                  "jim.ma" wrote:

                  Maybe we need to split the configuration for wise-core (for example , the WSDynamicClientFactory configuration) in a separated file . You see user who only want to use wise to call a web service does not need to know this.

                  Yep, even if it is the only config (at the moment) users don't care. It shouldn't be a big problem for users. Maybe we can split config in future.


                  I am disposed to rename it if it is not real a visitor pattern class.

                  Oki, I'm fine with. I haven't in mind any pattern closer to this implementation, so a name like EnablerImplDelegate is fine for you? Something better in mind?


                  If I remembered correctly, we also can inject a map or list into a bean container. Yes, it's just matter of style, they both can achieve same thing. If use get/set style, there are many fields needs to be added to this class to support various style WS-Security. Use a property map can simplify it.

                  Yes you can inject Map. With "various style WS-Security" do you mena different implementation (native, cxf, metro)?
                  I would prefer to have different config class for different style of WS-Security, it keep it simple to understand also injection. With a map used for all style it's harder for users to understand which key/value pair have to inject. With multiple classes named NativeWSSecurityConfig, CXFSecurityConfig, MetroSecurityConfig it's easier for users to understand what they are injecting and javadoc full document the valid field out of the box.



                  • 6. Re: Wise API reviewing
                    jim.ma

                     

                    "maeste" wrote:

                    Oki, I'm fine with. I haven't in mind any pattern closer to this implementation, so a name like EnablerImplDelegate is fine for you? Something better in mind?

                    I think it is a good name for it.
                    "maeste" wrote:

                    Yes you can inject Map. With "various style WS-Security" do you mena different implementation (native, cxf, metro)?
                    I would prefer to have different config class for different style of WS-Security, it keep it simple to understand also injection. With a map used for all style it's harder for users to understand which key/value pair have to inject. With multiple classes named NativeWSSecurityConfig, CXFSecurityConfig, MetroSecurityConfig it's easier for users to understand what they are injecting and javadoc full document the valid field out of the box.

                    No. I mean the number ways to validate the user , for example :Username/Password, UsernameToken, X509 Certificates and Kerberos ,etc.


                    • 7. Re: Wise API reviewing
                      maeste

                       

                      "jim.ma" wrote:

                      Oki, I'm fine with. I haven't in mind any pattern closer to this implementation, so a name like EnablerImplDelegate is fine for you? Something better in mind?

                      oki I'll do it during my luch break https://jira.jboss.org/jira/browse/WISE-84

                      No. I mean the number ways to validate the user , for example :Username/Password, UsernameToken, X509 Certificates and Kerberos ,etc.

                      But it stay in xml config file (the default for native). This IOC class purpose is only to give Wise client URL to config file, keystore, truststore, and config name to use. Specific config continue to stay in xml config, I don't think wise should wrap (at least at this stage) stack specific config files.

                      • 8. Re: Wise API reviewing
                        jim.ma

                         

                        "maeste" wrote:

                        But it stay in xml config file (the default for native). This IOC class purpose is only to give Wise client URL to config file, keystore, truststore, and config name to use. Specific config continue to stay in xml config, I don't think wise should wrap (at least at this stage) stack specific config files.

                        Actually what I talked before is all about JbossWS native , not statck specific configuration . The keystore , truststore and other configuration can be all wrote in jboss-wsse-client.xml. There is no restriction for what can be defined in config file and what can be defined in NativeSecurityConfig class. What I plan to do in the future (maybe after 1.0 release) is remove the jboss-wsse-client.xml configuration file and generate the jboss-wsse-client.xml at the background with the information provided by NativeSecurityConfig class. That will be more flexible and easier to enable WS-Security. So NativeSecurityConfig should provide get/set methods for all configuration items defined in jboss-ws-security.xsd. That's why I'd rather like a property map to store these value pairs.


                        • 9. Re: Wise API reviewing
                          asoldano

                          My 2 cents on the ws-security configuration being discussed here; if the Wise client wsse configuration (currently NativeSecurityConfig) is going to contain all the configuration items currently found in jboss-ws-security.xsd (http://fisheye.jboss.org/browse/JBossWS/stack/native/trunk/modules/core/src/main/resources/schema/jboss-ws-security_1_0.xsd?r=7167), well I don't think a map is a good solution, at least it's not the most elegant one. There're a lot of complexType even having children with the same names. As a matter of fact that xsd is currently mapped to a hierarchy of classes in JBossWS.

                          • 10. Re: Wise API reviewing
                            jim.ma

                             

                            "alessio.soldano@jboss.com" wrote:
                            My 2 cents on the ws-security configuration being discussed here; if the Wise client wsse configuration (currently NativeSecurityConfig) is going to contain all the configuration items currently found in jboss-ws-security.xsd (http://fisheye.jboss.org/browse/JBossWS/stack/native/trunk/modules/core/src/main/resources/schema/jboss-ws-security_1_0.xsd?r=7167), well I don't think a map is a good solution, at least it's not the most elegant one. There're a lot of complexType even having children with the same names. As a matter of fact that xsd is currently mapped to a hierarchy of classes in JBossWS.

                            Well, It's the problem of which one is more extendible and flexible. If use property map, we can avoid add or modify the get/set methods when the jboss-ws-security.xsd is modified or updated. If it is not a big problem for us, both are OK for me.