castor
  1. castor
  2. CASTOR-1932

Split ClassDescriptorResolver to be more expandable for future needs

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2
    • Component/s: XML
    • Labels:
      None
    • Number of attachments :
      5

      Description

      As preparation for the JAXB implementation of Castor we need to have an extendable ClassDescriptorResolver.

      The idea is to create one command for each resolve path currently implemented and then sum all commands up in a Castor XML strategy.

      1. castor-1932.txt
        111 kB
        Joachim Grüneis
      2. castor-1932-20070610.txt
        90 kB
        Joachim Grüneis
      3. castor-1932-20070701.txt
        97 kB
        Joachim Grüneis
      4. castor-1932-20070725.txt
        102 kB
        Joachim Grüneis
      5. patch-C1932-20070728.txt
        110 kB
        Ralf Joachim

        Activity

        Hide
        Joachim Grüneis added a comment -

        I split up XMLClassDescriptorResolver as follows:

        XMLClassDescriptorResolver will have the same interface as today and will know which strategy to use and how to instantiate the strategy.

        ResolveStrategy interface that describes what information a strategy has to provide to XMLClassDescriptorResolver - it is more or less the same as cdr had been before, but now cdr has another purpose.

        CastorXmlStrategy which implements ResolveStrategy and is the implementation of todays Castor strategy to resolve class descriptors. The strategy handles features like caching...

        ResolveCommand interface that describes what a resolve command needs to provide.

        All resolve steps of XMLClassDescriptorResolverImpl are now implementations of ResolveCommand.

        Show
        Joachim Grüneis added a comment - I split up XMLClassDescriptorResolver as follows: XMLClassDescriptorResolver will have the same interface as today and will know which strategy to use and how to instantiate the strategy. ResolveStrategy interface that describes what information a strategy has to provide to XMLClassDescriptorResolver - it is more or less the same as cdr had been before, but now cdr has another purpose. CastorXmlStrategy which implements ResolveStrategy and is the implementation of todays Castor strategy to resolve class descriptors. The strategy handles features like caching... ResolveCommand interface that describes what a resolve command needs to provide. All resolve steps of XMLClassDescriptorResolverImpl are now implementations of ResolveCommand.
        Hide
        Werner Guttmann added a comment -

        So where's the patch ?

        Show
        Werner Guttmann added a comment - So where's the patch ?
        Hide
        Joachim Grüneis added a comment -

        My path includes the following changes against HEAD:

        org.exolab.castor.tools.MappingTool
        replaced a reference to XMLClassDescriptorResolverImp with XMLClassDescriptorResolver

        org.exolab.castor.xml.XMLConstants
        add two new contants for XMLClassDescriptorResolver

        org.exolab.castor.xml.util.XMLClassDescriptorResolverImpl
        is now more or less a facade for the CastorXMLStrategy and in future for other resolving strategies yet to come

        org.exolab.castor.xml.util.ResolverStrategy
        interface of the resolve strategies

        org.exolab.castor.xml.util.ResolverCommand
        interface of resolve commands

        org.exolab.castor.xml.util.resolvers.*
        the resolve commands extracted from the old XMLClassDescriptorResolverImpl implementation

        Show
        Joachim Grüneis added a comment - My path includes the following changes against HEAD: org.exolab.castor.tools.MappingTool replaced a reference to XMLClassDescriptorResolverImp with XMLClassDescriptorResolver org.exolab.castor.xml.XMLConstants add two new contants for XMLClassDescriptorResolver org.exolab.castor.xml.util.XMLClassDescriptorResolverImpl is now more or less a facade for the CastorXMLStrategy and in future for other resolving strategies yet to come org.exolab.castor.xml.util.ResolverStrategy interface of the resolve strategies org.exolab.castor.xml.util.ResolverCommand interface of resolve commands org.exolab.castor.xml.util.resolvers.* the resolve commands extracted from the old XMLClassDescriptorResolverImpl implementation
        Hide
        Joachim Grüneis added a comment -

        Some toughts about my implementation.

        It only splits the 'old' XMLClassDescriptorResolverImpl into multiple classes to have a fine separation of concerns. The concerns of XMLClassDescriptorResolverImpl are:

        • be a facade to the outside world
        • own the descriptor cache which is a list of already resolved classes and also the place where a strategy puts it's results
        • know which strategies are there, instantiate the strategy and call it whenever a descriptor is not at hand
        • provide the configuration items the strategies need
          Concerns of a Strategy:
        • knows which commands (and in which order) are called to resolve a class
          Concerns of a Command
        • is a single method how a class is resolved
          . a tricky point about this is, that commands might resolve multiple classes when requested for a single class...

        There are some points about my current implementation which I'm unsure of - but I couldn't resist to try these things...
        1) The configurations and how they are handled...
        First it is a question if these configurations should be handled in such a local style and second if having the configurations that strongly typed is a clever idea. I implemented it that way to get a feeling about this solution... now I would throw it away and replace it with a much more relaxed system where all configuration items are found in some tree/map like structure by names and if a required configuration is not set: write a message and die silently...
        2) currently it is totally hard wired (the strategy and the commands)
        Next step will be to relax which strategies are instantiated and which commands are contained... based on the new context stuff
        3) the addClass and addPackage stuff is currently disabled
        I will create an own strategy for this stuff... yet to come

        Show
        Joachim Grüneis added a comment - Some toughts about my implementation. It only splits the 'old' XMLClassDescriptorResolverImpl into multiple classes to have a fine separation of concerns. The concerns of XMLClassDescriptorResolverImpl are: be a facade to the outside world own the descriptor cache which is a list of already resolved classes and also the place where a strategy puts it's results know which strategies are there, instantiate the strategy and call it whenever a descriptor is not at hand provide the configuration items the strategies need Concerns of a Strategy: knows which commands (and in which order) are called to resolve a class Concerns of a Command is a single method how a class is resolved . a tricky point about this is, that commands might resolve multiple classes when requested for a single class... There are some points about my current implementation which I'm unsure of - but I couldn't resist to try these things... 1) The configurations and how they are handled... First it is a question if these configurations should be handled in such a local style and second if having the configurations that strongly typed is a clever idea. I implemented it that way to get a feeling about this solution... now I would throw it away and replace it with a much more relaxed system where all configuration items are found in some tree/map like structure by names and if a required configuration is not set: write a message and die silently... 2) currently it is totally hard wired (the strategy and the commands) Next step will be to relax which strategies are instantiated and which commands are contained... based on the new context stuff 3) the addClass and addPackage stuff is currently disabled I will create an own strategy for this stuff... yet to come
        Hide
        Joachim Grüneis added a comment -

        the patch - Ta Ta

        Show
        Joachim Grüneis added a comment - the patch - Ta Ta
        Hide
        Joachim Grüneis added a comment -

        I have reworked my path. Mainly I removed loads of interfaces that ensure the correct configuration and introduced properties instead. Have fun with it.

        Show
        Joachim Grüneis added a comment - I have reworked my path. Mainly I removed loads of interfaces that ensure the correct configuration and introduced properties instead. Have fun with it.
        Hide
        Ralf Joachim added a comment -

        Hi Joachim,

        wouldn't it be better to change ResolverCommand to define the following 2 methods:

        public Map resolve(String className, Map p) throws ResolverException;
        public Map resolve(Class clazz, Map p) throws ResolverException;

        In addition I would create a abstract class AbstractClassNameResolver that implements:

        public final Map resolve(Class clazz, Map p) throws ResolverException {
        if (clazz == null)

        { String message = "No class to resolve specified"; _log.warn(message); throw new IllegalArgumentException(message); }

        return resolve(clazz.getName(), p);
        }

        This should remove quite some duplicated code.

        Show
        Ralf Joachim added a comment - Hi Joachim, wouldn't it be better to change ResolverCommand to define the following 2 methods: public Map resolve(String className, Map p) throws ResolverException; public Map resolve(Class clazz, Map p) throws ResolverException; In addition I would create a abstract class AbstractClassNameResolver that implements: public final Map resolve(Class clazz, Map p) throws ResolverException { if (clazz == null) { String message = "No class to resolve specified"; _log.warn(message); throw new IllegalArgumentException(message); } return resolve(clazz.getName(), p); } This should remove quite some duplicated code.
        Hide
        Ralf Joachim added a comment -

        In addition I wonder if the check for a classloader that happens at every resolve method of ResolverCommand's can also be extracted to a central place.

        I'm also not a fan of inner classes or interfaces like DiscriptorCache. Any reasons why you hidded that interface?

        Show
        Ralf Joachim added a comment - In addition I wonder if the check for a classloader that happens at every resolve method of ResolverCommand's can also be extracted to a central place. I'm also not a fan of inner classes or interfaces like DiscriptorCache. Any reasons why you hidded that interface?
        Hide
        Ralf Joachim added a comment -

        Having said that I haven't looked at all details yet.

        Show
        Ralf Joachim added a comment - Having said that I haven't looked at all details yet.
        Hide
        Joachim Grüneis added a comment -

        Hello Ralf,

        my first implementation was a like what you suggest (ResolveCommand.resolve(String)) - and it failed a huge lot of tests... now I know why and I will, for sure, do some refactoring.

        But before doing refactoring I want to know if:
        *) the way of handing down configuration (in form of properties) is ok
        *) the split of XMLClassDescriptorResolver / Strategy / Command is ok

        As for inner interfaces: in JAXB I will need them, so I toyed around with them... and also I think it a nice way to say "this is what ResolveStrategy needs to be provided for it but no one else is probably intereseted in it - it is no interface of interest for Castor itself"

        Show
        Joachim Grüneis added a comment - Hello Ralf, my first implementation was a like what you suggest (ResolveCommand.resolve(String)) - and it failed a huge lot of tests... now I know why and I will, for sure, do some refactoring. But before doing refactoring I want to know if: *) the way of handing down configuration (in form of properties) is ok *) the split of XMLClassDescriptorResolver / Strategy / Command is ok As for inner interfaces: in JAXB I will need them, so I toyed around with them... and also I think it a nice way to say "this is what ResolveStrategy needs to be provided for it but no one else is probably intereseted in it - it is no interface of interest for Castor itself"
        Hide
        Ralf Joachim added a comment -

        I'll take a closer look at the 2 points from above when separation of JDO modul got finished.

        Show
        Ralf Joachim added a comment - I'll take a closer look at the 2 points from above when separation of JDO modul got finished.
        Hide
        Ralf Joachim added a comment -

        @inner interfaces/inner classes:

        I never would use inner interfaces as I frequently find myself searching for them. The only exception for me is if one have been lazy enough to use such constructs in a public specification (e.g. in JAXB).

        About the same applies for inner classes with the exception of simple value objects without additional responsibilities (e.g. Map.Entry).

        You defined DescriptorCache interface in ResolverStrategy while its implementation is in XMLClassDescriptorResolverImpl. The interface is used at CastorXMLStrategy as intended but in addition it is used by XMLClassDescriptorResolverImpl which does not implement ResolverStrategy. Having said that I would throw away the interface in that case and move DescriptorCacheImpl out of XMLClassDescriptorResolverImpl.

        @split of XMLClassDescriptorResolver / Strategy / Command:
        very good

        @handling of configuration:
        As XMLClassDescriptorResolverImpl is responsible for instantiating ResolverStrategies as well as ResolverCommands we should pass a Configuration instance (the one I'm working at at another issue) from XMLClassDescriptorResolverImpl to each strategy and command. Each of these classes can hold a reference to this Configuration if it needs to access some of its properties.

        I don't think CastorXMLResolver should offer get/setProperties methods nor is there a need to pass the Configuration to each Command.resolve() method call (at least as far as I understand the code ATM). I also would not put things like a DescriptorCache or MappingLoader instance into the Configuration. The keys of the properties in the Configuration should be defined as constants at one place (e.g. in an XMLConfiguration class). This omits typos and allows for easier search of places where they are used with a bit of more work to maintain them.

        Show
        Ralf Joachim added a comment - @inner interfaces/inner classes: I never would use inner interfaces as I frequently find myself searching for them. The only exception for me is if one have been lazy enough to use such constructs in a public specification (e.g. in JAXB). About the same applies for inner classes with the exception of simple value objects without additional responsibilities (e.g. Map.Entry). You defined DescriptorCache interface in ResolverStrategy while its implementation is in XMLClassDescriptorResolverImpl. The interface is used at CastorXMLStrategy as intended but in addition it is used by XMLClassDescriptorResolverImpl which does not implement ResolverStrategy. Having said that I would throw away the interface in that case and move DescriptorCacheImpl out of XMLClassDescriptorResolverImpl. @split of XMLClassDescriptorResolver / Strategy / Command: very good @handling of configuration: As XMLClassDescriptorResolverImpl is responsible for instantiating ResolverStrategies as well as ResolverCommands we should pass a Configuration instance (the one I'm working at at another issue) from XMLClassDescriptorResolverImpl to each strategy and command. Each of these classes can hold a reference to this Configuration if it needs to access some of its properties. I don't think CastorXMLResolver should offer get/setProperties methods nor is there a need to pass the Configuration to each Command.resolve() method call (at least as far as I understand the code ATM). I also would not put things like a DescriptorCache or MappingLoader instance into the Configuration. The keys of the properties in the Configuration should be defined as constants at one place (e.g. in an XMLConfiguration class). This omits typos and allows for easier search of places where they are used with a bit of more work to maintain them.
        Hide
        Joachim Grüneis added a comment -

        @inner interfaces / inner classes

        During the implementation of this patch I started to like inner interfaces. Lets look onto ResolverStrategy.DescriptorCache:

        • the name is ugly (maybe even a bit misleading for the current purpose) but here I took what had been there before...
        • with this interface the ResolverStrategy states that it requires a place to put the descriptor found to and the outside world should provide it
          The other approach would be to have 'classic' interface, but I see the following disadvantages in it:
        • we would create an interface, which is only used by very few classes... I would like to keep this 'more private'...
        • for me it is the ResolverStrategy that wants to have a place to write its results to - so the interface is specific for this class

        Beside of this arguments: I will remove the inner interface if it is required...

        @ResolverStrategy.DescriptorCache

        I will rename the interface to something like ResolverResults - any ideas?

        @handling of configuration

        The current configuration stuff is a bit fuzzy as I wanted to wait for your configuration stuff

        Show
        Joachim Grüneis added a comment - @inner interfaces / inner classes During the implementation of this patch I started to like inner interfaces. Lets look onto ResolverStrategy.DescriptorCache: the name is ugly (maybe even a bit misleading for the current purpose) but here I took what had been there before... with this interface the ResolverStrategy states that it requires a place to put the descriptor found to and the outside world should provide it The other approach would be to have 'classic' interface, but I see the following disadvantages in it: we would create an interface, which is only used by very few classes... I would like to keep this 'more private'... for me it is the ResolverStrategy that wants to have a place to write its results to - so the interface is specific for this class Beside of this arguments: I will remove the inner interface if it is required... @ResolverStrategy.DescriptorCache I will rename the interface to something like ResolverResults - any ideas? @handling of configuration The current configuration stuff is a bit fuzzy as I wanted to wait for your configuration stuff
        Hide
        Joachim Grüneis added a comment -

        I've overworked the patch a lot... the 'main' changes are:
        *) introduced ResolverPackageCommand package are different then classes and with XMLClassDescriptorCacheImple.addPackage we have the need for pure package stuff
        *) I did it!! Now only the class name is required to be handed down to strategy and commands... the class loaders had been beasts but I've tamed them
        *) there are now abstract commands to ensure a maximum of NOT copied coded
        maybe I've missed something in my list... but I think these are the high lights.

        Show
        Joachim Grüneis added a comment - I've overworked the patch a lot... the 'main' changes are: *) introduced ResolverPackageCommand package are different then classes and with XMLClassDescriptorCacheImple.addPackage we have the need for pure package stuff *) I did it!! Now only the class name is required to be handed down to strategy and commands... the class loaders had been beasts but I've tamed them *) there are now abstract commands to ensure a maximum of NOT copied coded maybe I've missed something in my list... but I think these are the high lights.
        Hide
        Joachim Grüneis added a comment -

        As for configuration: this needs to change as soon as the new configuration stuff is available.

        Show
        Joachim Grüneis added a comment - As for configuration: this needs to change as soon as the new configuration stuff is available.
        Hide
        Ralf Joachim added a comment -

        Haven't had enough time to look at the new patch in more details but what I recognized are some warnings of ResolverException being not declared in ResolverCommands while it is declared to be thrown at one of the resolve methods. So I wonder if you intended it to be a checked or unchecked exception.

        Show
        Ralf Joachim added a comment - Haven't had enough time to look at the new patch in more details but what I recognized are some warnings of ResolverException being not declared in ResolverCommands while it is declared to be thrown at one of the resolve methods. So I wonder if you intended it to be a checked or unchecked exception.
        Hide
        Joachim Grüneis added a comment -

        Corrected problem with not declared exception.

        Show
        Joachim Grüneis added a comment - Corrected problem with not declared exception.
        Hide
        Ralf Joachim added a comment -

        Final patch with some minor changes at Joachim's original code.

        Show
        Ralf Joachim added a comment - Final patch with some minor changes at Joachim's original code.

          People

          • Assignee:
            Joachim Grüneis
            Reporter:
            Joachim Grüneis
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: