Refactoring GenericDelegator - Chain or Responsibility and Decorator

[Mansour]

Hello all,
I am looking at the code for the GenericDelegator. We have 2913 line
of code in one class !!!
This class handles a lot of functions and I think it's the most
critical to the application. These functions can
be distributed among more classes, providing better maintainability
and extensibility. I can see the best pattern to
be used here are chain of responsibilities and decorator, where each
class provide a defined set of functionality and implement the
Delegator interface.

For example, currently this class handles caching, configurations,
model Reading, encryption, logging, and the main function of accessing
data.
It's true that the interface it implements is huge (1217 line of code)
but mostly comments and documentation. CoR design pattern will allow
easier
unit testing, and better maintainability and allow to extend. So let's say:

GenericDelegator implements Delegator (Interfaces with the external world )
 ===> passes calls to SQLQueriesDelegator (build the sql through
implementing findXXX())
   =====> passes calls to LoggingDelegator
     =====> passes calls to ModuleReaderDelegator
      ======> MultiTenantDelegator
       ========> CachingDelegator
        ==========> EncryptionDelegator
         ============> etc

Each delegator does some work and calls the next one in chain until
the final functionality is achieved. Of course customization will be
easier.
For example, if I want to swap the current caching implementation with
something like memcahe or ecache, I have to change in many places in
the single 2913 line-of-code class !! With CoR all I do is swap the
current CachingDelegator class with my own. Let's say I want to access
the file system
or jcr nodes in the same unified way I access the db. In this case all
I need to do is add some thing like "RequestRoutingDelegator", and
JCRDelegator.
The RequestRouting decides how to access the entity in hand and based
on that uses either the SQLDelegator or JCRDelegator.
Those who did xml processing with SAX2 filters (CoR) or worked with
java IO streams (Decorator Pattern) will appreciate the concept of
separating functionality. An example in JavaIO, if one needs to ecrypt
a stream,  unzip the input, or zip the output .... etc. All these is
done though decorator pattern.

The whole chain can be created and constructed in the factory method
(or using a container config in the future), so existing applications
should not see any changes, as all of these delegators will be hidden
by package visibility (except for the first one in the chain).
In the factory method (hopefully in the future called through a
service locator), we can do something like:

Delegator encryptionDelegator = new EncryptionDelegator();
Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
.....
..... // may be do some configuration as we dont want every delegator
to configure itself by reading a file or looking up a
    // of a property, in case we used a container in the future.
....
Delegator realOne = new GenericDelegator();

static methods can be moved where needed and ensure there's not
duplication of functionality. At some point we may consider
making them instance methods.

To avoid DRY, we can create a class (implement Delegator) with default
functionality of calling the child (next) delegator.
Then just extend it and override whatever we need. This way we keep
the code minimal, and clean.

At the end, assuming no advantage gained, who wants to maintain 2913
line of code with some lines more than 100 character wide ??!!

I am not sure about the amount of work it takes to finalize it, but I
can see some difficulties. For example, assuming two different methods
belongs to two different layers, need one of the static methods in the
GenericDelegator. This will lead to duplicate code, we are going to
call static methods in other classes, and creating a mess again.
Another issue is when an object needs to be accessed in different
layers. This means we need to keep a reference for that object in
multiple places. What if it changed in one of the layers ? For
example, ModelReader is accessed in cloneDelegator and in
decryptField. Both of these methods belongs to different Delegators.
cloneDelegator goes into GenericDelegator, and decryptField goes into
EncryptionDelegator. What if the model reference changed in one of
them ?? Caution needed !

Another problem is when the order of calls in GeneralDelegator is not
consistent. For example, in one method we accessed the cache then we
did the logging. In another method we did the opposite, we logged,
then we accessed the cache. When done in layers, it will be harder to
do this.

Another alternative is something close to the composite pattern, where
we have Helper classes with a subset of Delegator implementation. The
GenericDelegator class, instantiates them and keeps a reference to
each of them. Then calls the appropriate method. This really simple,
but less
sexy, as extending the functionality still requires looking between
the lines. It's just breaking the HUGE class into smaller classes.

Not sure if I missed something. Other patterns could fit better. If
someone captured something, please let us know. Ofbiz team can decide
what path to follow.

Since this component is critical to the application, some may prefer a
more senior resource on the team and more skilled
with the internals of ofbiz, to handle this task. However, I don't
have a problem giving it a try. This task may sound small, but I don't
know what is a head of us. Someone with deeper knowledge about the
entity engine can comment.
Alternatively, one of the commiters, can adopt this task, and do the
cleaning as needed. For example, every while and then revisit this
huge class, and
break separate the functionality into the corresponding classes.

I didn't want to open this subject now, as I am aware of the cleaning
being performed by the team. However, if we can not do it now, and we
agreed about the path, it can be added to JIRA for future reference.

Please discuss and share knowledge.

[Jacopo Cappellato-4]

Mansour,

it is interesting.
Maybe it would be easier if we simplify the existing classes (and application code that uses them) to use only a subset of the methods available (some of them are already deprecated); the refactoring based on design patterns will be easier at that point.

Jacopo

On Mar 26, 2012, at 11:25 PM, Mansour Al Akeel wrote:

> Hello all,
> I am looking at the code for the GenericDelegator. We have 2913 line
> of code in one class !!!
> This class handles a lot of functions and I think it's the most
> critical to the application. These functions can
> be distributed among more classes, providing better maintainability
> and extensibility. I can see the best pattern to
> be used here are chain of responsibilities and decorator, where each
> class provide a defined set of functionality and implement the
> Delegator interface.
>
> For example, currently this class handles caching, configurations,
> model Reading, encryption, logging, and the main function of accessing
> data.
> It's true that the interface it implements is huge (1217 line of code)
> but mostly comments and documentation. CoR design pattern will allow
> easier
> unit testing, and better maintainability and allow to extend. So let's say:
>
> GenericDelegator implements Delegator (Interfaces with the external world )
> ===> passes calls to SQLQueriesDelegator (build the sql through
> implementing findXXX())
>   =====> passes calls to LoggingDelegator
>     =====> passes calls to ModuleReaderDelegator
>      ======> MultiTenantDelegator
>       ========> CachingDelegator
>        ==========> EncryptionDelegator
>         ============> etc
>
> Each delegator does some work and calls the next one in chain until
> the final functionality is achieved. Of course customization will be
> easier.
> For example, if I want to swap the current caching implementation with
> something like memcahe or ecache, I have to change in many places in
> the single 2913 line-of-code class !! With CoR all I do is swap the
> current CachingDelegator class with my own. Let's say I want to access
> the file system
> or jcr nodes in the same unified way I access the db. In this case all
> I need to do is add some thing like "RequestRoutingDelegator", and
> JCRDelegator.
> The RequestRouting decides how to access the entity in hand and based
> on that uses either the SQLDelegator or JCRDelegator.
> Those who did xml processing with SAX2 filters (CoR) or worked with
> java IO streams (Decorator Pattern) will appreciate the concept of
> separating functionality. An example in JavaIO, if one needs to ecrypt
> a stream,  unzip the input, or zip the output .... etc. All these is
> done though decorator pattern.
>
> The whole chain can be created and constructed in the factory method
> (or using a container config in the future), so existing applications
> should not see any changes, as all of these delegators will be hidden
> by package visibility (except for the first one in the chain).
> In the factory method (hopefully in the future called through a
> service locator), we can do something like:
>
> Delegator encryptionDelegator = new EncryptionDelegator();
> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
> .....
> ..... // may be do some configuration as we dont want every delegator
> to configure itself by reading a file or looking up a
>    // of a property, in case we used a container in the future.
> ....
> Delegator realOne = new GenericDelegator();
>
> static methods can be moved where needed and ensure there's not
> duplication of functionality. At some point we may consider
> making them instance methods.
>
> To avoid DRY, we can create a class (implement Delegator) with default
> functionality of calling the child (next) delegator.
> Then just extend it and override whatever we need. This way we keep
> the code minimal, and clean.
>
> At the end, assuming no advantage gained, who wants to maintain 2913
> line of code with some lines more than 100 character wide ??!!
>
> I am not sure about the amount of work it takes to finalize it, but I
> can see some difficulties. For example, assuming two different methods
> belongs to two different layers, need one of the static methods in the
> GenericDelegator. This will lead to duplicate code, we are going to
> call static methods in other classes, and creating a mess again.
> Another issue is when an object needs to be accessed in different
> layers. This means we need to keep a reference for that object in
> multiple places. What if it changed in one of the layers ? For
> example, ModelReader is accessed in cloneDelegator and in
> decryptField. Both of these methods belongs to different Delegators.
> cloneDelegator goes into GenericDelegator, and decryptField goes into
> EncryptionDelegator. What if the model reference changed in one of
> them ?? Caution needed !
>
> Another problem is when the order of calls in GeneralDelegator is not
> consistent. For example, in one method we accessed the cache then we
> did the logging. In another method we did the opposite, we logged,
> then we accessed the cache. When done in layers, it will be harder to
> do this.
>
> Another alternative is something close to the composite pattern, where
> we have Helper classes with a subset of Delegator implementation. The
> GenericDelegator class, instantiates them and keeps a reference to
> each of them. Then calls the appropriate method. This really simple,
> but less
> sexy, as extending the functionality still requires looking between
> the lines. It's just breaking the HUGE class into smaller classes.
>
> Not sure if I missed something. Other patterns could fit better. If
> someone captured something, please let us know. Ofbiz team can decide
> what path to follow.
>
> Since this component is critical to the application, some may prefer a
> more senior resource on the team and more skilled
> with the internals of ofbiz, to handle this task. However, I don't
> have a problem giving it a try. This task may sound small, but I don't
> know what is a head of us. Someone with deeper knowledge about the
> entity engine can comment.
> Alternatively, one of the commiters, can adopt this task, and do the
> cleaning as needed. For example, every while and then revisit this
> huge class, and
> break separate the functionality into the corresponding classes.
>
> I didn't want to open this subject now, as I am aware of the cleaning
> being performed by the team. However, if we can not do it now, and we
> agreed about the path, it can be added to JIRA for future reference.
>
> Please discuss and share knowledge.

[Adrian Crum-3]

I agree that the decorator pattern could improve the Delegator
implementation.

Some time ago I had suggested using the decorator pattern to support DB
localization - which would eliminate the need for the clunky
LocalizedMap interface/implementations - but there wasn't much interest
in it.

-Adrian

On 3/26/2012 10:25 PM, Mansour Al Akeel wrote:

> Hello all,
> I am looking at the code for the GenericDelegator. We have 2913 line
> of code in one class !!!
> This class handles a lot of functions and I think it's the most
> critical to the application. These functions can
> be distributed among more classes, providing better maintainability
> and extensibility. I can see the best pattern to
> be used here are chain of responsibilities and decorator, where each
> class provide a defined set of functionality and implement the
> Delegator interface.
>
> For example, currently this class handles caching, configurations,
> model Reading, encryption, logging, and the main function of accessing
> data.
> It's true that the interface it implements is huge (1217 line of code)
> but mostly comments and documentation. CoR design pattern will allow
> easier
> unit testing, and better maintainability and allow to extend. So let's say:
>
> GenericDelegator implements Delegator (Interfaces with the external world )
>   ===>  passes calls to SQLQueriesDelegator (build the sql through
> implementing findXXX())
>     =====>  passes calls to LoggingDelegator
>       =====>  passes calls to ModuleReaderDelegator
>        ======>  MultiTenantDelegator
>         ========>  CachingDelegator
>          ==========>  EncryptionDelegator
>           ============>  etc
>
> Each delegator does some work and calls the next one in chain until
> the final functionality is achieved. Of course customization will be
> easier.
> For example, if I want to swap the current caching implementation with
> something like memcahe or ecache, I have to change in many places in
> the single 2913 line-of-code class !! With CoR all I do is swap the
> current CachingDelegator class with my own. Let's say I want to access
> the file system
> or jcr nodes in the same unified way I access the db. In this case all
> I need to do is add some thing like "RequestRoutingDelegator", and
> JCRDelegator.
> The RequestRouting decides how to access the entity in hand and based
> on that uses either the SQLDelegator or JCRDelegator.
> Those who did xml processing with SAX2 filters (CoR) or worked with
> java IO streams (Decorator Pattern) will appreciate the concept of
> separating functionality. An example in JavaIO, if one needs to ecrypt
> a stream,  unzip the input, or zip the output .... etc. All these is
> done though decorator pattern.
>
> The whole chain can be created and constructed in the factory method
> (or using a container config in the future), so existing applications
> should not see any changes, as all of these delegators will be hidden
> by package visibility (except for the first one in the chain).
> In the factory method (hopefully in the future called through a
> service locator), we can do something like:
>
> Delegator encryptionDelegator = new EncryptionDelegator();
> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
> .....
> ..... // may be do some configuration as we dont want every delegator
> to configure itself by reading a file or looking up a
>      // of a property, in case we used a container in the future.
> ....
> Delegator realOne = new GenericDelegator();
>
> static methods can be moved where needed and ensure there's not
> duplication of functionality. At some point we may consider
> making them instance methods.
>
> To avoid DRY, we can create a class (implement Delegator) with default
> functionality of calling the child (next) delegator.
> Then just extend it and override whatever we need. This way we keep
> the code minimal, and clean.
>
> At the end, assuming no advantage gained, who wants to maintain 2913
> line of code with some lines more than 100 character wide ??!!
>
> I am not sure about the amount of work it takes to finalize it, but I
> can see some difficulties. For example, assuming two different methods
> belongs to two different layers, need one of the static methods in the
> GenericDelegator. This will lead to duplicate code, we are going to
> call static methods in other classes, and creating a mess again.
> Another issue is when an object needs to be accessed in different
> layers. This means we need to keep a reference for that object in
> multiple places. What if it changed in one of the layers ? For
> example, ModelReader is accessed in cloneDelegator and in
> decryptField. Both of these methods belongs to different Delegators.
> cloneDelegator goes into GenericDelegator, and decryptField goes into
> EncryptionDelegator. What if the model reference changed in one of
> them ?? Caution needed !
>
> Another problem is when the order of calls in GeneralDelegator is not
> consistent. For example, in one method we accessed the cache then we
> did the logging. In another method we did the opposite, we logged,
> then we accessed the cache. When done in layers, it will be harder to
> do this.
>
> Another alternative is something close to the composite pattern, where
> we have Helper classes with a subset of Delegator implementation. The
> GenericDelegator class, instantiates them and keeps a reference to
> each of them. Then calls the appropriate method. This really simple,
> but less
> sexy, as extending the functionality still requires looking between
> the lines. It's just breaking the HUGE class into smaller classes.
>
> Not sure if I missed something. Other patterns could fit better. If
> someone captured something, please let us know. Ofbiz team can decide
> what path to follow.
>
> Since this component is critical to the application, some may prefer a
> more senior resource on the team and more skilled
> with the internals of ofbiz, to handle this task. However, I don't
> have a problem giving it a try. This task may sound small, but I don't
> know what is a head of us. Someone with deeper knowledge about the
> entity engine can comment.
> Alternatively, one of the commiters, can adopt this task, and do the
> cleaning as needed. For example, every while and then revisit this
> huge class, and
> break separate the functionality into the corresponding classes.
>
> I didn't want to open this subject now, as I am aware of the cleaning
> being performed by the team. However, if we can not do it now, and we
> agreed about the path, it can be added to JIRA for future reference.
>
> Please discuss and share knowledge.

[Mansour]

Jacopo,

I didn't know about the deprecated methods, and I didn't want to open
the subject of changing methods, because I don't what methods are used
a lot in the applications code, and don't want to introduce a lot of
change. However, in my opinion, I don't see a need for something like:

public void clearCacheLine(GenericPK primaryKey);

I am not sure if an application programmer will be interested in
something like this. If I have to design Delegator class, I will
restrict the methods only to those that offer some sort of
functionality to the application programmer, and hide everything else.
However, to avoid breaking any existing application code, I suggested
only a refactor. However you suggestion is useful, and would make
things easier, and give a ofbiz entity engine a cleaner code base.

Definitely, I am supporting this idea. If we are going to do a
refactor, we might as well, remove deprecated methods, and replace
methods in the application code, that uses those deprecated calls.

So the starting point to tackle this task with minimal risk would be,
decide and remove deprecated methods from Delegator interface, then
clean the applications code and replace those calls.

Is that ok ? Do you see a need for a fork ?




On Tue, Mar 27, 2012 at 6:12 AM, Adrian Crum
<[hidden email]> wrote:

> I agree that the decorator pattern could improve the Delegator
> implementation.
>
> Some time ago I had suggested using the decorator pattern to support DB
> localization - which would eliminate the need for the clunky LocalizedMap
> interface/implementations - but there wasn't much interest in it.
>
> -Adrian
>
>
> On 3/26/2012 10:25 PM, Mansour Al Akeel wrote:
>>
>> Hello all,
>> I am looking at the code for the GenericDelegator. We have 2913 line
>> of code in one class !!!
>> This class handles a lot of functions and I think it's the most
>> critical to the application. These functions can
>> be distributed among more classes, providing better maintainability
>> and extensibility. I can see the best pattern to
>> be used here are chain of responsibilities and decorator, where each
>> class provide a defined set of functionality and implement the
>> Delegator interface.
>>
>> For example, currently this class handles caching, configurations,
>> model Reading, encryption, logging, and the main function of accessing
>> data.
>> It's true that the interface it implements is huge (1217 line of code)
>> but mostly comments and documentation. CoR design pattern will allow
>> easier
>> unit testing, and better maintainability and allow to extend. So let's
>> say:
>>
>> GenericDelegator implements Delegator (Interfaces with the external world
>> )
>>  ===>  passes calls to SQLQueriesDelegator (build the sql through
>> implementing findXXX())
>>    =====>  passes calls to LoggingDelegator
>>      =====>  passes calls to ModuleReaderDelegator
>>       ======>  MultiTenantDelegator
>>        ========>  CachingDelegator
>>         ==========>  EncryptionDelegator
>>          ============>  etc
>>
>> Each delegator does some work and calls the next one in chain until
>> the final functionality is achieved. Of course customization will be
>> easier.
>> For example, if I want to swap the current caching implementation with
>> something like memcahe or ecache, I have to change in many places in
>> the single 2913 line-of-code class !! With CoR all I do is swap the
>> current CachingDelegator class with my own. Let's say I want to access
>> the file system
>> or jcr nodes in the same unified way I access the db. In this case all
>> I need to do is add some thing like "RequestRoutingDelegator", and
>> JCRDelegator.
>> The RequestRouting decides how to access the entity in hand and based
>> on that uses either the SQLDelegator or JCRDelegator.
>> Those who did xml processing with SAX2 filters (CoR) or worked with
>> java IO streams (Decorator Pattern) will appreciate the concept of
>> separating functionality. An example in JavaIO, if one needs to ecrypt
>> a stream,  unzip the input, or zip the output .... etc. All these is
>> done though decorator pattern.
>>
>> The whole chain can be created and constructed in the factory method
>> (or using a container config in the future), so existing applications
>> should not see any changes, as all of these delegators will be hidden
>> by package visibility (except for the first one in the chain).
>> In the factory method (hopefully in the future called through a
>> service locator), we can do something like:
>>
>> Delegator encryptionDelegator = new EncryptionDelegator();
>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
>> .....
>> ..... // may be do some configuration as we dont want every delegator
>> to configure itself by reading a file or looking up a
>>     // of a property, in case we used a container in the future.
>> ....
>> Delegator realOne = new GenericDelegator();
>>
>> static methods can be moved where needed and ensure there's not
>> duplication of functionality. At some point we may consider
>> making them instance methods.
>>
>> To avoid DRY, we can create a class (implement Delegator) with default
>> functionality of calling the child (next) delegator.
>> Then just extend it and override whatever we need. This way we keep
>> the code minimal, and clean.
>>
>> At the end, assuming no advantage gained, who wants to maintain 2913
>> line of code with some lines more than 100 character wide ??!!
>>
>> I am not sure about the amount of work it takes to finalize it, but I
>> can see some difficulties. For example, assuming two different methods
>> belongs to two different layers, need one of the static methods in the
>> GenericDelegator. This will lead to duplicate code, we are going to
>> call static methods in other classes, and creating a mess again.
>> Another issue is when an object needs to be accessed in different
>> layers. This means we need to keep a reference for that object in
>> multiple places. What if it changed in one of the layers ? For
>> example, ModelReader is accessed in cloneDelegator and in
>> decryptField. Both of these methods belongs to different Delegators.
>> cloneDelegator goes into GenericDelegator, and decryptField goes into
>> EncryptionDelegator. What if the model reference changed in one of
>> them ?? Caution needed !
>>
>> Another problem is when the order of calls in GeneralDelegator is not
>> consistent. For example, in one method we accessed the cache then we
>> did the logging. In another method we did the opposite, we logged,
>> then we accessed the cache. When done in layers, it will be harder to
>> do this.
>>
>> Another alternative is something close to the composite pattern, where
>> we have Helper classes with a subset of Delegator implementation. The
>> GenericDelegator class, instantiates them and keeps a reference to
>> each of them. Then calls the appropriate method. This really simple,
>> but less
>> sexy, as extending the functionality still requires looking between
>> the lines. It's just breaking the HUGE class into smaller classes.
>>
>> Not sure if I missed something. Other patterns could fit better. If
>> someone captured something, please let us know. Ofbiz team can decide
>> what path to follow.
>>
>> Since this component is critical to the application, some may prefer a
>> more senior resource on the team and more skilled
>> with the internals of ofbiz, to handle this task. However, I don't
>> have a problem giving it a try. This task may sound small, but I don't
>> know what is a head of us. Someone with deeper knowledge about the
>> entity engine can comment.
>> Alternatively, one of the commiters, can adopt this task, and do the
>> cleaning as needed. For example, every while and then revisit this
>> huge class, and
>> break separate the functionality into the corresponding classes.
>>
>> I didn't want to open this subject now, as I am aware of the cleaning
>> being performed by the team. However, if we can not do it now, and we
>> agreed about the path, it can be added to JIRA for future reference.
>>
>> Please discuss and share knowledge.

[Mansour]

Thank you Adrian.
I don't think a lot of developers will see an immediate gain. But in
the long run, it will be appreciated.
It allows us to introduce new functionality.

For example, in one of my previous emails, I pointed out the need
for a Record Level Security, like the one offered by Oracle DB and
recently by postgresql. Where the loged in user
have specific access to the data. The same principal, is used in most
NoSQL data bases.
My plan was to have an xml config file next to the entities (ie,
acl.xml), and restrict access based to entities I need.
The whole idea is, to appends some query parameters to calls to DB,
giving the application programmer a nice and
clean way to work with records.

When I looked into the current code, I didn't want to touch it. So, I
thought a bout writing a new JDBC (decorator)
driver that wraps the calls, to the DB. But then,
I don't see this is wise. Since the JBDC driver will have to read
application data, it is part of the framework. It can
be done as a hack, but entity engine is more
appropriate for this. Plus I may have some issues with caching. If the
cache is checked before sending a call to the DB, then
it may return results, a user should not see !

So, yes the pattern will make it easier for us, and application
programers will find the resulting functionality that can be
introduced useful.





On Tue, Mar 27, 2012 at 6:12 AM, Adrian Crum
<[hidden email]> wrote:

> I agree that the decorator pattern could improve the Delegator
> implementation.
>
> Some time ago I had suggested using the decorator pattern to support DB
> localization - which would eliminate the need for the clunky LocalizedMap
> interface/implementations - but there wasn't much interest in it.
>
> -Adrian
>
>
> On 3/26/2012 10:25 PM, Mansour Al Akeel wrote:
>>
>> Hello all,
>> I am looking at the code for the GenericDelegator. We have 2913 line
>> of code in one class !!!
>> This class handles a lot of functions and I think it's the most
>> critical to the application. These functions can
>> be distributed among more classes, providing better maintainability
>> and extensibility. I can see the best pattern to
>> be used here are chain of responsibilities and decorator, where each
>> class provide a defined set of functionality and implement the
>> Delegator interface.
>>
>> For example, currently this class handles caching, configurations,
>> model Reading, encryption, logging, and the main function of accessing
>> data.
>> It's true that the interface it implements is huge (1217 line of code)
>> but mostly comments and documentation. CoR design pattern will allow
>> easier
>> unit testing, and better maintainability and allow to extend. So let's
>> say:
>>
>> GenericDelegator implements Delegator (Interfaces with the external world
>> )
>>  ===>  passes calls to SQLQueriesDelegator (build the sql through
>> implementing findXXX())
>>    =====>  passes calls to LoggingDelegator
>>      =====>  passes calls to ModuleReaderDelegator
>>       ======>  MultiTenantDelegator
>>        ========>  CachingDelegator
>>         ==========>  EncryptionDelegator
>>          ============>  etc
>>
>> Each delegator does some work and calls the next one in chain until
>> the final functionality is achieved. Of course customization will be
>> easier.
>> For example, if I want to swap the current caching implementation with
>> something like memcahe or ecache, I have to change in many places in
>> the single 2913 line-of-code class !! With CoR all I do is swap the
>> current CachingDelegator class with my own. Let's say I want to access
>> the file system
>> or jcr nodes in the same unified way I access the db. In this case all
>> I need to do is add some thing like "RequestRoutingDelegator", and
>> JCRDelegator.
>> The RequestRouting decides how to access the entity in hand and based
>> on that uses either the SQLDelegator or JCRDelegator.
>> Those who did xml processing with SAX2 filters (CoR) or worked with
>> java IO streams (Decorator Pattern) will appreciate the concept of
>> separating functionality. An example in JavaIO, if one needs to ecrypt
>> a stream,  unzip the input, or zip the output .... etc. All these is
>> done though decorator pattern.
>>
>> The whole chain can be created and constructed in the factory method
>> (or using a container config in the future), so existing applications
>> should not see any changes, as all of these delegators will be hidden
>> by package visibility (except for the first one in the chain).
>> In the factory method (hopefully in the future called through a
>> service locator), we can do something like:
>>
>> Delegator encryptionDelegator = new EncryptionDelegator();
>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
>> .....
>> ..... // may be do some configuration as we dont want every delegator
>> to configure itself by reading a file or looking up a
>>     // of a property, in case we used a container in the future.
>> ....
>> Delegator realOne = new GenericDelegator();
>>
>> static methods can be moved where needed and ensure there's not
>> duplication of functionality. At some point we may consider
>> making them instance methods.
>>
>> To avoid DRY, we can create a class (implement Delegator) with default
>> functionality of calling the child (next) delegator.
>> Then just extend it and override whatever we need. This way we keep
>> the code minimal, and clean.
>>
>> At the end, assuming no advantage gained, who wants to maintain 2913
>> line of code with some lines more than 100 character wide ??!!
>>
>> I am not sure about the amount of work it takes to finalize it, but I
>> can see some difficulties. For example, assuming two different methods
>> belongs to two different layers, need one of the static methods in the
>> GenericDelegator. This will lead to duplicate code, we are going to
>> call static methods in other classes, and creating a mess again.
>> Another issue is when an object needs to be accessed in different
>> layers. This means we need to keep a reference for that object in
>> multiple places. What if it changed in one of the layers ? For
>> example, ModelReader is accessed in cloneDelegator and in
>> decryptField. Both of these methods belongs to different Delegators.
>> cloneDelegator goes into GenericDelegator, and decryptField goes into
>> EncryptionDelegator. What if the model reference changed in one of
>> them ?? Caution needed !
>>
>> Another problem is when the order of calls in GeneralDelegator is not
>> consistent. For example, in one method we accessed the cache then we
>> did the logging. In another method we did the opposite, we logged,
>> then we accessed the cache. When done in layers, it will be harder to
>> do this.
>>
>> Another alternative is something close to the composite pattern, where
>> we have Helper classes with a subset of Delegator implementation. The
>> GenericDelegator class, instantiates them and keeps a reference to
>> each of them. Then calls the appropriate method. This really simple,
>> but less
>> sexy, as extending the functionality still requires looking between
>> the lines. It's just breaking the HUGE class into smaller classes.
>>
>> Not sure if I missed something. Other patterns could fit better. If
>> someone captured something, please let us know. Ofbiz team can decide
>> what path to follow.
>>
>> Since this component is critical to the application, some may prefer a
>> more senior resource on the team and more skilled
>> with the internals of ofbiz, to handle this task. However, I don't
>> have a problem giving it a try. This task may sound small, but I don't
>> know what is a head of us. Someone with deeper knowledge about the
>> entity engine can comment.
>> Alternatively, one of the commiters, can adopt this task, and do the
>> cleaning as needed. For example, every while and then revisit this
>> huge class, and
>> break separate the functionality into the corresponding classes.
>>
>> I didn't want to open this subject now, as I am aware of the cleaning
>> being performed by the team. However, if we can not do it now, and we
>> agreed about the path, it can be added to JIRA for future reference.
>>
>> Please discuss and share knowledge.

[Adrian Crum-3]

On 3/27/2012 2:41 PM, Mansour Al Akeel wrote:

> Thank you Adrian.
> I don't think a lot of developers will see an immediate gain. But in
> the long run, it will be appreciated.
> It allows us to introduce new functionality.
>
> For example, in one of my previous emails, I pointed out the need
> for a Record Level Security, like the one offered by Oracle DB and
> recently by postgresql. Where the loged in user
> have specific access to the data. The same principal, is used in most
> NoSQL data bases.
> My plan was to have an xml config file next to the entities (ie,
> acl.xml), and restrict access based to entities I need.
> The whole idea is, to appends some query parameters to calls to DB,
> giving the application programmer a nice and
> clean way to work with records.

If you want to restrict access to data based on a user login, then the
access control should be configured in the database - not in XML files.
I am not convinced that the type of security you describe should be
handled by the delegator. In most cases, that type of access control is
included in a broader set of business rules - so it should be
implemented in services.

-Adrian

[Jacques Le Roux]

From: "Mansour Al Akeel" <[hidden email]>
> Jacopo,
>
> I didn't know about the deprecated methods, and I didn't want to open
> the subject of changing methods, because I don't what methods are used
> a lot in the applications code, and don't want to introduce a lot of
> change. However, in my opinion, I don't see a need for something like:
>
> public void clearCacheLine(GenericPK primaryKey);

It's used in GenericDelegator.removeByPrimaryKey() and GenericDelegator.removeAll()
 

> I am not sure if an application programmer will be interested in
> something like this. If I have to design Delegator class, I will
> restrict the methods only to those that offer some sort of
> functionality to the application programmer, and hide everything else.
> However, to avoid breaking any existing application code, I suggested
> only a refactor. However you suggestion is useful, and would make
> things easier, and give a ofbiz entity engine a cleaner code base.
>
> Definitely, I am supporting this idea. If we are going to do a
> refactor, we might as well, remove deprecated methods, and replace
> methods in the application code, that uses those deprecated calls.
>
> So the starting point to tackle this task with minimal risk would be,
> decide and remove deprecated methods from Delegator interface, then
> clean the applications code and replace those calls.
>
> Is that ok ? Do you see a need for a fork ?

A branch would be safer

Jacques

>
>
>
> On Tue, Mar 27, 2012 at 6:12 AM, Adrian Crum
> <[hidden email]> wrote:
>> I agree that the decorator pattern could improve the Delegator
>> implementation.
>>
>> Some time ago I had suggested using the decorator pattern to support DB
>> localization - which would eliminate the need for the clunky LocalizedMap
>> interface/implementations - but there wasn't much interest in it.
>>
>> -Adrian
>>
>>
>> On 3/26/2012 10:25 PM, Mansour Al Akeel wrote:
>>>
>>> Hello all,
>>> I am looking at the code for the GenericDelegator. We have 2913 line
>>> of code in one class !!!
>>> This class handles a lot of functions and I think it's the most
>>> critical to the application. These functions can
>>> be distributed among more classes, providing better maintainability
>>> and extensibility. I can see the best pattern to
>>> be used here are chain of responsibilities and decorator, where each
>>> class provide a defined set of functionality and implement the
>>> Delegator interface.
>>>
>>> For example, currently this class handles caching, configurations,
>>> model Reading, encryption, logging, and the main function of accessing
>>> data.
>>> It's true that the interface it implements is huge (1217 line of code)
>>> but mostly comments and documentation. CoR design pattern will allow
>>> easier
>>> unit testing, and better maintainability and allow to extend. So let's
>>> say:
>>>
>>> GenericDelegator implements Delegator (Interfaces with the external world
>>> )
>>> ===> passes calls to SQLQueriesDelegator (build the sql through
>>> implementing findXXX())
>>> =====> passes calls to LoggingDelegator
>>> =====> passes calls to ModuleReaderDelegator
>>> ======> MultiTenantDelegator
>>> ========> CachingDelegator
>>> ==========> EncryptionDelegator
>>> ============> etc
>>>
>>> Each delegator does some work and calls the next one in chain until
>>> the final functionality is achieved. Of course customization will be
>>> easier.
>>> For example, if I want to swap the current caching implementation with
>>> something like memcahe or ecache, I have to change in many places in
>>> the single 2913 line-of-code class !! With CoR all I do is swap the
>>> current CachingDelegator class with my own. Let's say I want to access
>>> the file system
>>> or jcr nodes in the same unified way I access the db. In this case all
>>> I need to do is add some thing like "RequestRoutingDelegator", and
>>> JCRDelegator.
>>> The RequestRouting decides how to access the entity in hand and based
>>> on that uses either the SQLDelegator or JCRDelegator.
>>> Those who did xml processing with SAX2 filters (CoR) or worked with
>>> java IO streams (Decorator Pattern) will appreciate the concept of
>>> separating functionality. An example in JavaIO, if one needs to ecrypt
>>> a stream, unzip the input, or zip the output .... etc. All these is
>>> done though decorator pattern.
>>>
>>> The whole chain can be created and constructed in the factory method
>>> (or using a container config in the future), so existing applications
>>> should not see any changes, as all of these delegators will be hidden
>>> by package visibility (except for the first one in the chain).
>>> In the factory method (hopefully in the future called through a
>>> service locator), we can do something like:
>>>
>>> Delegator encryptionDelegator = new EncryptionDelegator();
>>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
>>> .....
>>> ..... // may be do some configuration as we dont want every delegator
>>> to configure itself by reading a file or looking up a
>>> // of a property, in case we used a container in the future.
>>> ....
>>> Delegator realOne = new GenericDelegator();
>>>
>>> static methods can be moved where needed and ensure there's not
>>> duplication of functionality. At some point we may consider
>>> making them instance methods.
>>>
>>> To avoid DRY, we can create a class (implement Delegator) with default
>>> functionality of calling the child (next) delegator.
>>> Then just extend it and override whatever we need. This way we keep
>>> the code minimal, and clean.
>>>
>>> At the end, assuming no advantage gained, who wants to maintain 2913
>>> line of code with some lines more than 100 character wide ??!!
>>>
>>> I am not sure about the amount of work it takes to finalize it, but I
>>> can see some difficulties. For example, assuming two different methods
>>> belongs to two different layers, need one of the static methods in the
>>> GenericDelegator. This will lead to duplicate code, we are going to
>>> call static methods in other classes, and creating a mess again.
>>> Another issue is when an object needs to be accessed in different
>>> layers. This means we need to keep a reference for that object in
>>> multiple places. What if it changed in one of the layers ? For
>>> example, ModelReader is accessed in cloneDelegator and in
>>> decryptField. Both of these methods belongs to different Delegators.
>>> cloneDelegator goes into GenericDelegator, and decryptField goes into
>>> EncryptionDelegator. What if the model reference changed in one of
>>> them ?? Caution needed !
>>>
>>> Another problem is when the order of calls in GeneralDelegator is not
>>> consistent. For example, in one method we accessed the cache then we
>>> did the logging. In another method we did the opposite, we logged,
>>> then we accessed the cache. When done in layers, it will be harder to
>>> do this.
>>>
>>> Another alternative is something close to the composite pattern, where
>>> we have Helper classes with a subset of Delegator implementation. The
>>> GenericDelegator class, instantiates them and keeps a reference to
>>> each of them. Then calls the appropriate method. This really simple,
>>> but less
>>> sexy, as extending the functionality still requires looking between
>>> the lines. It's just breaking the HUGE class into smaller classes.
>>>
>>> Not sure if I missed something. Other patterns could fit better. If
>>> someone captured something, please let us know. Ofbiz team can decide
>>> what path to follow.
>>>
>>> Since this component is critical to the application, some may prefer a
>>> more senior resource on the team and more skilled
>>> with the internals of ofbiz, to handle this task. However, I don't
>>> have a problem giving it a try. This task may sound small, but I don't
>>> know what is a head of us. Someone with deeper knowledge about the
>>> entity engine can comment.
>>> Alternatively, one of the commiters, can adopt this task, and do the
>>> cleaning as needed. For example, every while and then revisit this
>>> huge class, and
>>> break separate the functionality into the corresponding classes.
>>>
>>> I didn't want to open this subject now, as I am aware of the cleaning
>>> being performed by the team. However, if we can not do it now, and we
>>> agreed about the path, it can be added to JIRA for future reference.
>>>
>>> Please discuss and share knowledge.
>

[Mansour]

This true. But to reuse db connection we use a pool, so we connect to
the DB with one user, and
reuse that connection. Based on that, I don't see how can we configure
the DB to do so. And configurations
wouldn't be portable. So if I am working on some client code, and
configured these ACL rules with derby, then
when deployed to production, I have to find a way to configure MySql
DB to do these rules.

We do this already by checking permissions in the services:
<if-has-permission permission="ORDERMGR" action="_VIEW"/>

But this is just another way to apply this on the whole entities. This
doesn't always work as expected.
For example, the project manager component, shows projects that the
user doesn't belong to, and should not see.
/org/ofbiz/project/ProjectPermissionServices.xml:
  <not><if-has-permission permission="PROJECTMGR_ROLE_VIEW"/></not>

The solution comes with ofbiz, is ProjectPermissionServices, which is
fine, but for each application needs a different set
of security rules, and may implement its own security services to
provide some set of this rules.
What is the reason of creating a security services script for each
component. Keep in mind that most of these permissions applies to the
screen engine, or to the services. If we want to do a record level
security we need to bring the whole set of records from the DB, Then
iterate over them to see which one we are allowed to access for the
current user. Adding it into the delegator will make this transparent
to application users. The user need to edit acl.xml to reflect
security on entities.

IMO your point is separation of concerns. Something like "Let the
delegator do the data access, and services select what data it needs".
And I agree about this, but from different perspective,  we can say
"this is the data available for this user". and therefore it's part of
the business rules. Not sure if my point is clear here.

The concept has been done with different database access technology,
like JPA http://jpasecurity.sourceforge.net/

After all, I see your point, and I will reconsider this option and
give it another thought.

For now, we agree on the need to refactor Delegator, and we are
looking for a plan to minimize the risk involved.
What are you thoughts ?



On Tue, Mar 27, 2012 at 9:48 AM, Adrian Crum
<[hidden email]> wrote:

> On 3/27/2012 2:41 PM, Mansour Al Akeel wrote:
>>
>> Thank you Adrian.
>> I don't think a lot of developers will see an immediate gain. But in
>> the long run, it will be appreciated.
>> It allows us to introduce new functionality.
>>
>> For example, in one of my previous emails, I pointed out the need
>> for a Record Level Security, like the one offered by Oracle DB and
>> recently by postgresql. Where the loged in user
>> have specific access to the data. The same principal, is used in most
>> NoSQL data bases.
>> My plan was to have an xml config file next to the entities (ie,
>> acl.xml), and restrict access based to entities I need.
>> The whole idea is, to appends some query parameters to calls to DB,
>> giving the application programmer a nice and
>> clean way to work with records.
>
>
> If you want to restrict access to data based on a user login, then the
> access control should be configured in the database - not in XML files. I am
> not convinced that the type of security you describe should be handled by
> the delegator. In most cases, that type of access control is included in a
> broader set of business rules - so it should be implemented in services.
>
> -Adrian
>

[Mansour]

yep,
anywhere away from the current code.


On Tue, Mar 27, 2012 at 10:44 AM, Jacques Le Roux
<[hidden email]> wrote:

> From: "Mansour Al Akeel" <[hidden email]>
>
>> Jacopo,
>>
>> I didn't know about the deprecated methods, and I didn't want to open
>> the subject of changing methods, because I don't what methods are used
>> a lot in the applications code, and don't want to introduce a lot of
>> change. However, in my opinion, I don't see a need for something like:
>>
>> public void clearCacheLine(GenericPK primaryKey);
>
>
> It's used in GenericDelegator.removeByPrimaryKey() and
> GenericDelegator.removeAll()
>

This usage is inside the delegator itself so it can be wrapped, but
from applications it is used only.
./product/src/org/ofbiz/product/product/ProductEvents.java
Additional components under "framework" needs it.

Will need to plan before we start and create a branch.

>
>> I am not sure if an application programmer will be interested in
>> something like this. If I have to design Delegator class, I will
>> restrict the methods only to those that offer some sort of
>> functionality to the application programmer, and hide everything else.
>> However, to avoid breaking any existing application code, I suggested
>> only a refactor. However you suggestion is useful, and would make
>> things easier, and give a ofbiz entity engine a cleaner code base.
>>
>> Definitely, I am supporting this idea. If we are going to do a
>> refactor, we might as well, remove deprecated methods, and replace
>> methods in the application code, that uses those deprecated calls.
>>
>> So the starting point to tackle this task with minimal risk would be,
>> decide and remove deprecated methods from Delegator interface, then
>> clean the applications code and replace those calls.
>>
>> Is that ok ? Do you see a need for a fork ?
>
>
> A branch would be safer
>
> Jacques
>
>
>>
>>
>>
>> On Tue, Mar 27, 2012 at 6:12 AM, Adrian Crum
>> <[hidden email]> wrote:
>>>
>>> I agree that the decorator pattern could improve the Delegator
>>> implementation.
>>>
>>> Some time ago I had suggested using the decorator pattern to support DB
>>> localization - which would eliminate the need for the clunky LocalizedMap
>>> interface/implementations - but there wasn't much interest in it.
>>>
>>> -Adrian
>>>
>>>
>>> On 3/26/2012 10:25 PM, Mansour Al Akeel wrote:
>>>>
>>>>
>>>> Hello all,
>>>> I am looking at the code for the GenericDelegator. We have 2913 line
>>>> of code in one class !!!
>>>> This class handles a lot of functions and I think it's the most
>>>> critical to the application. These functions can
>>>> be distributed among more classes, providing better maintainability
>>>> and extensibility. I can see the best pattern to
>>>> be used here are chain of responsibilities and decorator, where each
>>>> class provide a defined set of functionality and implement the
>>>> Delegator interface.
>>>>
>>>> For example, currently this class handles caching, configurations,
>>>> model Reading, encryption, logging, and the main function of accessing
>>>> data.
>>>> It's true that the interface it implements is huge (1217 line of code)
>>>> but mostly comments and documentation. CoR design pattern will allow
>>>> easier
>>>> unit testing, and better maintainability and allow to extend. So let's
>>>> say:
>>>>
>>>> GenericDelegator implements Delegator (Interfaces with the external
>>>> world
>>>> )
>>>> ===> passes calls to SQLQueriesDelegator (build the sql through
>>>> implementing findXXX())
>>>> =====> passes calls to LoggingDelegator
>>>> =====> passes calls to ModuleReaderDelegator
>>>> ======> MultiTenantDelegator
>>>> ========> CachingDelegator
>>>> ==========> EncryptionDelegator
>>>> ============> etc
>>>>
>>>> Each delegator does some work and calls the next one in chain until
>>>> the final functionality is achieved. Of course customization will be
>>>> easier.
>>>> For example, if I want to swap the current caching implementation with
>>>> something like memcahe or ecache, I have to change in many places in
>>>> the single 2913 line-of-code class !! With CoR all I do is swap the
>>>> current CachingDelegator class with my own. Let's say I want to access
>>>> the file system
>>>> or jcr nodes in the same unified way I access the db. In this case all
>>>> I need to do is add some thing like "RequestRoutingDelegator", and
>>>> JCRDelegator.
>>>> The RequestRouting decides how to access the entity in hand and based
>>>> on that uses either the SQLDelegator or JCRDelegator.
>>>> Those who did xml processing with SAX2 filters (CoR) or worked with
>>>> java IO streams (Decorator Pattern) will appreciate the concept of
>>>> separating functionality. An example in JavaIO, if one needs to ecrypt
>>>> a stream, unzip the input, or zip the output .... etc. All these is
>>>> done though decorator pattern.
>>>>
>>>> The whole chain can be created and constructed in the factory method
>>>> (or using a container config in the future), so existing applications
>>>> should not see any changes, as all of these delegators will be hidden
>>>> by package visibility (except for the first one in the chain).
>>>> In the factory method (hopefully in the future called through a
>>>> service locator), we can do something like:
>>>>
>>>> Delegator encryptionDelegator = new EncryptionDelegator();
>>>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
>>>> .....
>>>> ..... // may be do some configuration as we dont want every delegator
>>>> to configure itself by reading a file or looking up a
>>>> // of a property, in case we used a container in the future.
>>>> ....
>>>> Delegator realOne = new GenericDelegator();
>>>>
>>>> static methods can be moved where needed and ensure there's not
>>>> duplication of functionality. At some point we may consider
>>>> making them instance methods.
>>>>
>>>> To avoid DRY, we can create a class (implement Delegator) with default
>>>> functionality of calling the child (next) delegator.
>>>> Then just extend it and override whatever we need. This way we keep
>>>> the code minimal, and clean.
>>>>
>>>> At the end, assuming no advantage gained, who wants to maintain 2913
>>>> line of code with some lines more than 100 character wide ??!!
>>>>
>>>> I am not sure about the amount of work it takes to finalize it, but I
>>>> can see some difficulties. For example, assuming two different methods
>>>> belongs to two different layers, need one of the static methods in the
>>>> GenericDelegator. This will lead to duplicate code, we are going to
>>>> call static methods in other classes, and creating a mess again.
>>>> Another issue is when an object needs to be accessed in different
>>>> layers. This means we need to keep a reference for that object in
>>>> multiple places. What if it changed in one of the layers ? For
>>>> example, ModelReader is accessed in cloneDelegator and in
>>>> decryptField. Both of these methods belongs to different Delegators.
>>>> cloneDelegator goes into GenericDelegator, and decryptField goes into
>>>> EncryptionDelegator. What if the model reference changed in one of
>>>> them ?? Caution needed !
>>>>
>>>> Another problem is when the order of calls in GeneralDelegator is not
>>>> consistent. For example, in one method we accessed the cache then we
>>>> did the logging. In another method we did the opposite, we logged,
>>>> then we accessed the cache. When done in layers, it will be harder to
>>>> do this.
>>>>
>>>> Another alternative is something close to the composite pattern, where
>>>> we have Helper classes with a subset of Delegator implementation. The
>>>> GenericDelegator class, instantiates them and keeps a reference to
>>>> each of them. Then calls the appropriate method. This really simple,
>>>> but less
>>>> sexy, as extending the functionality still requires looking between
>>>> the lines. It's just breaking the HUGE class into smaller classes.
>>>>
>>>> Not sure if I missed something. Other patterns could fit better. If
>>>> someone captured something, please let us know. Ofbiz team can decide
>>>> what path to follow.
>>>>
>>>> Since this component is critical to the application, some may prefer a
>>>> more senior resource on the team and more skilled
>>>> with the internals of ofbiz, to handle this task. However, I don't
>>>> have a problem giving it a try. This task may sound small, but I don't
>>>> know what is a head of us. Someone with deeper knowledge about the
>>>> entity engine can comment.
>>>> Alternatively, one of the commiters, can adopt this task, and do the
>>>> cleaning as needed. For example, every while and then revisit this
>>>> huge class, and
>>>> break separate the functionality into the corresponding classes.
>>>>
>>>> I didn't want to open this subject now, as I am aware of the cleaning
>>>> being performed by the team. However, if we can not do it now, and we
>>>> agreed about the path, it can be added to JIRA for future reference.
>>>>
>>>> Please discuss and share knowledge.
>>
>>
>

[Mansour]

Jacopo,
I am still thinking about your suggestion to change the Delegator
interface and use only subset of the methods.
If you have some thought ready, please share them and discuss.

If you have some plan to introduce these changes, then let me know about them.

I think we will have to change many things anyway if we need to refactor.
It's not like what I initially thought, just moving code around will be enough.



On Tue, Mar 27, 2012 at 3:40 AM, Jacopo Cappellato
<[hidden email]> wrote:

> Mansour,
>
> it is interesting.
> Maybe it would be easier if we simplify the existing classes (and application code that uses them) to use only a subset of the methods available (some of them are already deprecated); the refactoring based on design patterns will be easier at that point.
>
> Jacopo
>
> On Mar 26, 2012, at 11:25 PM, Mansour Al Akeel wrote:
>
>> Hello all,
>> I am looking at the code for the GenericDelegator. We have 2913 line
>> of code in one class !!!
>> This class handles a lot of functions and I think it's the most
>> critical to the application. These functions can
>> be distributed among more classes, providing better maintainability
>> and extensibility. I can see the best pattern to
>> be used here are chain of responsibilities and decorator, where each
>> class provide a defined set of functionality and implement the
>> Delegator interface.
>>
>> For example, currently this class handles caching, configurations,
>> model Reading, encryption, logging, and the main function of accessing
>> data.
>> It's true that the interface it implements is huge (1217 line of code)
>> but mostly comments and documentation. CoR design pattern will allow
>> easier
>> unit testing, and better maintainability and allow to extend. So let's say:
>>
>> GenericDelegator implements Delegator (Interfaces with the external world )
>> ===> passes calls to SQLQueriesDelegator (build the sql through
>> implementing findXXX())
>>   =====> passes calls to LoggingDelegator
>>     =====> passes calls to ModuleReaderDelegator
>>      ======> MultiTenantDelegator
>>       ========> CachingDelegator
>>        ==========> EncryptionDelegator
>>         ============> etc
>>
>> Each delegator does some work and calls the next one in chain until
>> the final functionality is achieved. Of course customization will be
>> easier.
>> For example, if I want to swap the current caching implementation with
>> something like memcahe or ecache, I have to change in many places in
>> the single 2913 line-of-code class !! With CoR all I do is swap the
>> current CachingDelegator class with my own. Let's say I want to access
>> the file system
>> or jcr nodes in the same unified way I access the db. In this case all
>> I need to do is add some thing like "RequestRoutingDelegator", and
>> JCRDelegator.
>> The RequestRouting decides how to access the entity in hand and based
>> on that uses either the SQLDelegator or JCRDelegator.
>> Those who did xml processing with SAX2 filters (CoR) or worked with
>> java IO streams (Decorator Pattern) will appreciate the concept of
>> separating functionality. An example in JavaIO, if one needs to ecrypt
>> a stream,  unzip the input, or zip the output .... etc. All these is
>> done though decorator pattern.
>>
>> The whole chain can be created and constructed in the factory method
>> (or using a container config in the future), so existing applications
>> should not see any changes, as all of these delegators will be hidden
>> by package visibility (except for the first one in the chain).
>> In the factory method (hopefully in the future called through a
>> service locator), we can do something like:
>>
>> Delegator encryptionDelegator = new EncryptionDelegator();
>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
>> .....
>> ..... // may be do some configuration as we dont want every delegator
>> to configure itself by reading a file or looking up a
>>    // of a property, in case we used a container in the future.
>> ....
>> Delegator realOne = new GenericDelegator();
>>
>> static methods can be moved where needed and ensure there's not
>> duplication of functionality. At some point we may consider
>> making them instance methods.
>>
>> To avoid DRY, we can create a class (implement Delegator) with default
>> functionality of calling the child (next) delegator.
>> Then just extend it and override whatever we need. This way we keep
>> the code minimal, and clean.
>>
>> At the end, assuming no advantage gained, who wants to maintain 2913
>> line of code with some lines more than 100 character wide ??!!
>>
>> I am not sure about the amount of work it takes to finalize it, but I
>> can see some difficulties. For example, assuming two different methods
>> belongs to two different layers, need one of the static methods in the
>> GenericDelegator. This will lead to duplicate code, we are going to
>> call static methods in other classes, and creating a mess again.
>> Another issue is when an object needs to be accessed in different
>> layers. This means we need to keep a reference for that object in
>> multiple places. What if it changed in one of the layers ? For
>> example, ModelReader is accessed in cloneDelegator and in
>> decryptField. Both of these methods belongs to different Delegators.
>> cloneDelegator goes into GenericDelegator, and decryptField goes into
>> EncryptionDelegator. What if the model reference changed in one of
>> them ?? Caution needed !
>>
>> Another problem is when the order of calls in GeneralDelegator is not
>> consistent. For example, in one method we accessed the cache then we
>> did the logging. In another method we did the opposite, we logged,
>> then we accessed the cache. When done in layers, it will be harder to
>> do this.
>>
>> Another alternative is something close to the composite pattern, where
>> we have Helper classes with a subset of Delegator implementation. The
>> GenericDelegator class, instantiates them and keeps a reference to
>> each of them. Then calls the appropriate method. This really simple,
>> but less
>> sexy, as extending the functionality still requires looking between
>> the lines. It's just breaking the HUGE class into smaller classes.
>>
>> Not sure if I missed something. Other patterns could fit better. If
>> someone captured something, please let us know. Ofbiz team can decide
>> what path to follow.
>>
>> Since this component is critical to the application, some may prefer a
>> more senior resource on the team and more skilled
>> with the internals of ofbiz, to handle this task. However, I don't
>> have a problem giving it a try. This task may sound small, but I don't
>> know what is a head of us. Someone with deeper knowledge about the
>> entity engine can comment.
>> Alternatively, one of the commiters, can adopt this task, and do the
>> cleaning as needed. For example, every while and then revisit this
>> huge class, and
>> break separate the functionality into the corresponding classes.
>>
>> I didn't want to open this subject now, as I am aware of the cleaning
>> being performed by the team. However, if we can not do it now, and we
>> agreed about the path, it can be added to JIRA for future reference.
>>
>> Please discuss and share knowledge.
>

[Mansour]

From Delegator.java

 139     public GenericDelegator cloneDelegator();
 140
 141     public GenericDelegator cloneDelegator(String delegatorName);


This has to be changed. Delegator is an interface, and it has a
reference to its implementation "GenericDelegator".

Anyone knows of anything that might break, or a reason not to change this ?

I will open an issue, for this.



On Tue, Mar 27, 2012 at 4:02 PM, Mansour Al Akeel
<[hidden email]> wrote:

> Jacopo,
> I am still thinking about your suggestion to change the Delegator
> interface and use only subset of the methods.
> If you have some thought ready, please share them and discuss.
>
> If you have some plan to introduce these changes, then let me know about them.
>
> I think we will have to change many things anyway if we need to refactor.
> It's not like what I initially thought, just moving code around will be enough.
>
>
>
> On Tue, Mar 27, 2012 at 3:40 AM, Jacopo Cappellato
> <[hidden email]> wrote:
>> Mansour,
>>
>> it is interesting.
>> Maybe it would be easier if we simplify the existing classes (and application code that uses them) to use only a subset of the methods available (some of them are already deprecated); the refactoring based on design patterns will be easier at that point.
>>
>> Jacopo
>>
>> On Mar 26, 2012, at 11:25 PM, Mansour Al Akeel wrote:
>>
>>> Hello all,
>>> I am looking at the code for the GenericDelegator. We have 2913 line
>>> of code in one class !!!
>>> This class handles a lot of functions and I think it's the most
>>> critical to the application. These functions can
>>> be distributed among more classes, providing better maintainability
>>> and extensibility. I can see the best pattern to
>>> be used here are chain of responsibilities and decorator, where each
>>> class provide a defined set of functionality and implement the
>>> Delegator interface.
>>>
>>> For example, currently this class handles caching, configurations,
>>> model Reading, encryption, logging, and the main function of accessing
>>> data.
>>> It's true that the interface it implements is huge (1217 line of code)
>>> but mostly comments and documentation. CoR design pattern will allow
>>> easier
>>> unit testing, and better maintainability and allow to extend. So let's say:
>>>
>>> GenericDelegator implements Delegator (Interfaces with the external world )
>>> ===> passes calls to SQLQueriesDelegator (build the sql through
>>> implementing findXXX())
>>>   =====> passes calls to LoggingDelegator
>>>     =====> passes calls to ModuleReaderDelegator
>>>      ======> MultiTenantDelegator
>>>       ========> CachingDelegator
>>>        ==========> EncryptionDelegator
>>>         ============> etc
>>>
>>> Each delegator does some work and calls the next one in chain until
>>> the final functionality is achieved. Of course customization will be
>>> easier.
>>> For example, if I want to swap the current caching implementation with
>>> something like memcahe or ecache, I have to change in many places in
>>> the single 2913 line-of-code class !! With CoR all I do is swap the
>>> current CachingDelegator class with my own. Let's say I want to access
>>> the file system
>>> or jcr nodes in the same unified way I access the db. In this case all
>>> I need to do is add some thing like "RequestRoutingDelegator", and
>>> JCRDelegator.
>>> The RequestRouting decides how to access the entity in hand and based
>>> on that uses either the SQLDelegator or JCRDelegator.
>>> Those who did xml processing with SAX2 filters (CoR) or worked with
>>> java IO streams (Decorator Pattern) will appreciate the concept of
>>> separating functionality. An example in JavaIO, if one needs to ecrypt
>>> a stream,  unzip the input, or zip the output .... etc. All these is
>>> done though decorator pattern.
>>>
>>> The whole chain can be created and constructed in the factory method
>>> (or using a container config in the future), so existing applications
>>> should not see any changes, as all of these delegators will be hidden
>>> by package visibility (except for the first one in the chain).
>>> In the factory method (hopefully in the future called through a
>>> service locator), we can do something like:
>>>
>>> Delegator encryptionDelegator = new EncryptionDelegator();
>>> Delegator cachingDel = new CachingDelegator( encryptiongDelegaort);
>>> .....
>>> ..... // may be do some configuration as we dont want every delegator
>>> to configure itself by reading a file or looking up a
>>>    // of a property, in case we used a container in the future.
>>> ....
>>> Delegator realOne = new GenericDelegator();
>>>
>>> static methods can be moved where needed and ensure there's not
>>> duplication of functionality. At some point we may consider
>>> making them instance methods.
>>>
>>> To avoid DRY, we can create a class (implement Delegator) with default
>>> functionality of calling the child (next) delegator.
>>> Then just extend it and override whatever we need. This way we keep
>>> the code minimal, and clean.
>>>
>>> At the end, assuming no advantage gained, who wants to maintain 2913
>>> line of code with some lines more than 100 character wide ??!!
>>>
>>> I am not sure about the amount of work it takes to finalize it, but I
>>> can see some difficulties. For example, assuming two different methods
>>> belongs to two different layers, need one of the static methods in the
>>> GenericDelegator. This will lead to duplicate code, we are going to
>>> call static methods in other classes, and creating a mess again.
>>> Another issue is when an object needs to be accessed in different
>>> layers. This means we need to keep a reference for that object in
>>> multiple places. What if it changed in one of the layers ? For
>>> example, ModelReader is accessed in cloneDelegator and in
>>> decryptField. Both of these methods belongs to different Delegators.
>>> cloneDelegator goes into GenericDelegator, and decryptField goes into
>>> EncryptionDelegator. What if the model reference changed in one of
>>> them ?? Caution needed !
>>>
>>> Another problem is when the order of calls in GeneralDelegator is not
>>> consistent. For example, in one method we accessed the cache then we
>>> did the logging. In another method we did the opposite, we logged,
>>> then we accessed the cache. When done in layers, it will be harder to
>>> do this.
>>>
>>> Another alternative is something close to the composite pattern, where
>>> we have Helper classes with a subset of Delegator implementation. The
>>> GenericDelegator class, instantiates them and keeps a reference to
>>> each of them. Then calls the appropriate method. This really simple,
>>> but less
>>> sexy, as extending the functionality still requires looking between
>>> the lines. It's just breaking the HUGE class into smaller classes.
>>>
>>> Not sure if I missed something. Other patterns could fit better. If
>>> someone captured something, please let us know. Ofbiz team can decide
>>> what path to follow.
>>>
>>> Since this component is critical to the application, some may prefer a
>>> more senior resource on the team and more skilled
>>> with the internals of ofbiz, to handle this task. However, I don't
>>> have a problem giving it a try. This task may sound small, but I don't
>>> know what is a head of us. Someone with deeper knowledge about the
>>> entity engine can comment.
>>> Alternatively, one of the commiters, can adopt this task, and do the
>>> cleaning as needed. For example, every while and then revisit this
>>> huge class, and
>>> break separate the functionality into the corresponding classes.
>>>
>>> I didn't want to open this subject now, as I am aware of the cleaning
>>> being performed by the team. However, if we can not do it now, and we
>>> agreed about the path, it can be added to JIRA for future reference.
>>>
>>> Please discuss and share knowledge.
>>