Simplifying GenericDelegator

[David E Jones]

This has been discussed before, but I thought it might be a good time  
to bring it up again based on Adam Heath's recent additions to the  
GenericDelegator (in revs r585808, r585803, r585802, r585759).

One of the issue with the Entity Engine that has been getting worse  
over the years is method bloat. There are WAY too many variations of  
different methods, IMO. These have been added for convenience, but  
the net effect (that I don't think was expected or even considered)  
is that when looking at the massive list it is tricky to figure out  
which methods to use for what. This is a big problem for people new  
to the Entity Engine, but also a problem for experienced EE users.

In short lately I'm thinking that having so many methods is worse  
than the convenience they offer to make life easier for "lazy"  
coders. Actually, with a decent IDE having lots of parameters isn't  
such a big deal.

This does have the down side of requiring changes to lots of existing  
code to use the fully featured methods instead of the simplified ones  
(most of which just call the full featured ones with default values).

The up side is we end up with a really happy and clean  
GenericDelegator class with only maybe 10-15 methods for different  
general operations, and perhaps even less than that, like 5-10...  
(aside from the cache maintenance methods, and other utility methods,  
many of which we might want to make private, the general goal being  
to simplify the class).

In shorter I think this is getting to be one of those cases where  
less is more...

Any thoughts? Should we start mass-marking methods as deprecated?  
Which ones and in what forms of the methods should we leave?

-David

[Adam Heath-2]

David E Jones wrote:
>  [snip]

Well, I have thoughts on this.  Extend javac.

Basically, add default values to parameter definitions.  Javac would
then auto-generate methods to see each default value.  Of course, this
is something that has to be done outside of ofbiz; I've compiled
openjdk, but I haven't tried to extend it at all.

I've also had the same bad taste with the delegator.  The way you have
you use 2 different methods, based on whether you use caching or not as
always struck me has poor.  I'd rather see that done as a flag/parameter
specifying whether caching should be done.

Another possibly is to use annotations to specify things.  But I haven't
given much thought to that.

And, yet another mention; I plan on adding pure sql querying to the
delegator.  So you can do things like:

delegator.findByQuery("SELECT * FROM Person")

I already have code that can parse sql(it's a jjtree grammar).  I wrote
an xslt that converts *all* entitymodel.xml to sql CREATE statements.
Then I wrote code to parse the resulting sql files, and generate correct
JDBC sql code to handling the normal ofbiz column/table mapping.  I just
haven't integrated it.  My thoughts were to have a framework/entity2,
that talked to the same back-end datastore, so one could use either variant.

Another variant might be this:

Query query = new Query("Person");
query.setSelectFields("partyId", "firstName", "lastName");
query.cacheResults(true);
query.orderBy("lastName", "firstName");
List<GenericValue> people = delegator.runQuery(query);

[Adrian Crum]

David E Jones wrote:
> In short lately I'm thinking that having so many methods is worse  than
> the convenience they offer to make life easier for "lazy"  coders.
> Actually, with a decent IDE having lots of parameters isn't  such a big
> deal.

I'm a huge fan of reducing code. The stark reality is the Apache OFBiz community is having a hard
time maintaining the existing code base, let alone any new/additional code. I think it's a good time
to look at this issue on a broader scale, not just the GenericDelegator.

> The up side is we end up with a really happy and clean  GenericDelegator
> class with only maybe 10-15 methods for different  general operations,
> and perhaps even less than that, like 5-10...  (aside from the cache
> maintenance methods, and other utility methods,  many of which we might
> want to make private, the general goal being  to simplify the class).

Or put them in a separate class that takes a GenericDelegator as an argument.

> In shorter I think this is getting to be one of those cases where  less
> is more...

I agree 100%.

If I hold up both hands can I cast +2 votes?

-Adrian

[Adrian Crum]

David,

I've been thinking about this more. Here are a couple of ideas I came up with:

1. Move the existing delegator cache maintenance methods to a separate interface+class. Add a method
to DelegatorInterface that retrieves an instance of the class for peripheral code to work with. End
result: +1 cache maint class, +1 delegator accessor method, -14 delegator cache maintenance methods.

2. Reduce all of the findByXxxx and findAll methods to a single find method that takes a
FindParameters class. A separate worker class could convert all of the various arguments currently
being sent to the myriad findXxxx methods and condense them into a FindParameters instance that is
passed to the single find method in the delegator. End result: +1 find parameters worker class, -23
delegator methods.

Both ideas simply move code out of the delegator into task-specific worker classes. I don't like the
idea of eliminating the various choices altogether, because that would cause even more code bloat in
the peripheral code.

-Adrian

David E Jones wrote:

>
> This has been discussed before, but I thought it might be a good time  
> to bring it up again based on Adam Heath's recent additions to the  
> GenericDelegator (in revs r585808, r585803, r585802, r585759).
>
> One of the issue with the Entity Engine that has been getting worse  
> over the years is method bloat. There are WAY too many variations of  
> different methods, IMO. These have been added for convenience, but  the
> net effect (that I don't think was expected or even considered)  is that
> when looking at the massive list it is tricky to figure out  which
> methods to use for what. This is a big problem for people new  to the
> Entity Engine, but also a problem for experienced EE users.
>
> In short lately I'm thinking that having so many methods is worse  than
> the convenience they offer to make life easier for "lazy"  coders.
> Actually, with a decent IDE having lots of parameters isn't  such a big
> deal.
>
> This does have the down side of requiring changes to lots of existing  
> code to use the fully featured methods instead of the simplified ones  
> (most of which just call the full featured ones with default values).
>
> The up side is we end up with a really happy and clean  GenericDelegator
> class with only maybe 10-15 methods for different  general operations,
> and perhaps even less than that, like 5-10...  (aside from the cache
> maintenance methods, and other utility methods,  many of which we might
> want to make private, the general goal being  to simplify the class).
>
> In shorter I think this is getting to be one of those cases where  less
> is more...
>
> Any thoughts? Should we start mass-marking methods as deprecated?  Which
> ones and in what forms of the methods should we leave?
>
> -David
>

[Adam Heath-2]

Adrian Crum wrote:

> David,
>
> I've been thinking about this more. Here are a couple of ideas I came up
> with:
>
> 1. Move the existing delegator cache maintenance methods to a separate
> interface+class. Add a method to DelegatorInterface that retrieves an
> instance of the class for peripheral code to work with. End result: +1
> cache maint class, +1 delegator accessor method, -14 delegator cache
> maintenance methods.

I've thought this too.  So all delegator calls would be wrapped,
effectively, and do cache storing.

The code in entity/cache has some algos that could be used for this.
They take their parameters, convert them into a 'key', and use that for
access.

> 2. Reduce all of the findByXxxx and findAll methods to a single find
> method that takes a FindParameters class. A separate worker class could
> convert all of the various arguments currently being sent to the myriad
> findXxxx methods and condense them into a FindParameters instance that
> is passed to the single find method in the delegator. End result: +1
> find parameters worker class, -23 delegator methods.

In effect, that's what already happens; most code eventually calls
findByCondition.

Additionally, this could simplify the sql generating parts too.

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>
>>David,
>>
>>I've been thinking about this more. Here are a couple of ideas I came up
>>with:
>>
>>1. Move the existing delegator cache maintenance methods to a separate
>>interface+class. Add a method to DelegatorInterface that retrieves an
>>instance of the class for peripheral code to work with. End result: +1
>>cache maint class, +1 delegator accessor method, -14 delegator cache
>>maintenance methods.
>
>
> I've thought this too.  So all delegator calls would be wrapped,
> effectively, and do cache storing.
>
> The code in entity/cache has some algos that could be used for this.
> They take their parameters, convert them into a 'key', and use that for
> access.

Yesterday I played around with the code to do this - it was really simple. No, the delegator calls
wouldn't be wrapped... I think. I'm not sure what you mean. I moved all of the DelegatorInterface
ClearAllCacheXxx and ClearCacheXxxx methods to a separate interface and added the accessor method to
the DelegatorInterface. Then I added the cache maint interface implementation to GenericDelegator as
an inner class. The end result was the GenericDelegator code size didn't change - because the outer
class methods were simply moved to the inner class, but the Delegator API was simplified.

[David E Jones]

On Oct 19, 2007, at 9:24 AM, Adam Heath wrote:

> Adrian Crum wrote:
>> 2. Reduce all of the findByXxxx and findAll methods to a single find
>> method that takes a FindParameters class. A separate worker class  
>> could
>> convert all of the various arguments currently being sent to the  
>> myriad
>> findXxxx methods and condense them into a FindParameters instance  
>> that
>> is passed to the single find method in the delegator. End result: +1
>> find parameters worker class, -23 delegator methods.
>
> In effect, that's what already happens; most code eventually calls
> findByCondition.
>
> Additionally, this could simplify the sql generating parts too.

My original thought was to review these (23? wow...) methods and  
deprecate (and later remove) nearly all of them. The main ones I  
think should remain are:

1. a findListIteratorByCondition method with all possible parameters
2. a findByAnd method with all possible parms

... and I think that's about it for the finds. Both would have a new  
parameter called "useCache". The findByAnd is just there for  
convenience as that is a common use case.

The idea of a query parameters object is interesting. If we do  
something like that we should look at some ways of reducing the lines  
of code required to use it. I think that's the only part of it I  
don't like... lots of lines of code.

Whatever the case, I think the most important thing is that we not  
rush this. There are lots of less effective ways to do this (like the  
one we have now...), so the key is to avoid introducing something  
that is just as cumbersome/annoying.

-David

[Jacques Le Roux]

De : "David E Jones" <[hidden email]>

> On Oct 19, 2007, at 9:24 AM, Adam Heath wrote:
>
> > Adrian Crum wrote:
> >> 2. Reduce all of the findByXxxx and findAll methods to a single find
> >> method that takes a FindParameters class. A separate worker class
> >> could
> >> convert all of the various arguments currently being sent to the
> >> myriad
> >> findXxxx methods and condense them into a FindParameters instance
> >> that
> >> is passed to the single find method in the delegator. End result: +1
> >> find parameters worker class, -23 delegator methods.
> >
> > In effect, that's what already happens; most code eventually calls
> > findByCondition.
> >
> > Additionally, this could simplify the sql generating parts too.
>
> My original thought was to review these (23? wow...) methods and
> deprecate (and later remove) nearly all of them. The main ones I
> think should remain are:
>
> 1. a findListIteratorByCondition method with all possible parameters
> 2. a findByAnd method with all possible parms
>
> ... and I think that's about it for the finds. Both would have a new
> parameter called "useCache". The findByAnd is just there for
> convenience as that is a common use case.

+1, Yes I prefer that way, less name easier to understand with parameters in it. Actually, this is why the parameter concept is used
for, isn'it ?

> The idea of a query parameters object is interesting. If we do
> something like that we should look at some ways of reducing the lines
> of code required to use it. I think that's the only part of it I
> don't like... lots of lines of code.

+1. I found Adam's suggestion interesting but a bit verbose (or too near SQL rather). Maybe it was intended, since Adam suppose
everybody know well SQL. Which is false, I'm really bad at it for instance.

> Whatever the case, I think the most important thing is that we not
> rush this. There are lots of less effective ways to do this (like the
> one we have now...), so the key is to avoid introducing something
> that is just as cumbersome/annoying.

There are already good ideas, exchanging about them before implementing anything is reassuring. And maybe we will even find better
or complementing ideas... Sorry I have none, just following for now

Anyway it seems that Adam, who is the most active on this issue, will not have time for it in the next days (weeks, months ?). So we
have gotten time to think, hoping we will not forget to do (I speak mostly for myself here :o)

Jacques

> -David
>
>

[jonwimp]

 > The idea of a query parameters object is interesting. If we do something like
 > that we should look at some ways of reducing the lines of code required to
 > use it. I think that's the only part of it I don't like...  lots of lines of
 > code.

This is a very crucial point in this thread.

A heavily parameterized method is great for genericity. We can call such a method with
programmatically generated parameters. Very consistent, very single-entry-point approach.

However, such a method will require "setting up" of the parameters before calling it.

In comparison, having a few buttons named "chicken", "fish", "pasta" on a microwave oven will
allow me to just press the button for the common scenario I'm facing at the moment. I don't need
to think about how much heat a chicken can take, compared to a fish.

Jonathon

David E Jones wrote:

>
> On Oct 19, 2007, at 9:24 AM, Adam Heath wrote:
>
>> Adrian Crum wrote:
>>> 2. Reduce all of the findByXxxx and findAll methods to a single find
>>> method that takes a FindParameters class. A separate worker class could
>>> convert all of the various arguments currently being sent to the myriad
>>> findXxxx methods and condense them into a FindParameters instance that
>>> is passed to the single find method in the delegator. End result: +1
>>> find parameters worker class, -23 delegator methods.
>>
>> In effect, that's what already happens; most code eventually calls
>> findByCondition.
>>
>> Additionally, this could simplify the sql generating parts too.
>
> My original thought was to review these (23? wow...) methods and
> deprecate (and later remove) nearly all of them. The main ones I think
> should remain are:
>
> 1. a findListIteratorByCondition method with all possible parameters
> 2. a findByAnd method with all possible parms
>
> ... and I think that's about it for the finds. Both would have a new
> parameter called "useCache". The findByAnd is just there for convenience
> as that is a common use case.
>
> The idea of a query parameters object is interesting. If we do something
> like that we should look at some ways of reducing the lines of code
> required to use it. I think that's the only part of it I don't like...
> lots of lines of code.
>
> Whatever the case, I think the most important thing is that we not rush
> this. There are lots of less effective ways to do this (like the one we
> have now...), so the key is to avoid introducing something that is just
> as cumbersome/annoying.
>
> -David
>

[Adrian Crum]

Jonathon -- Improov wrote:

> This is a very crucial point in this thread.
>
> A heavily parameterized method is great for genericity. We can call such
> a method with programmatically generated parameters. Very consistent,
> very single-entry-point approach.
>
> However, such a method will require "setting up" of the parameters
> before calling it.
>
> In comparison, having a few buttons named "chicken", "fish", "pasta" on
> a microwave oven will allow me to just press the button for the common
> scenario I'm facing at the moment. I don't need to think about how much
> heat a chicken can take, compared to a fish.

That's why I was thinking we would still need some kind of worker class that can be called to
prepare the handful of objects we have in hand so they can be used in the single entry point. The
worker class would function as the chicken, fish, and pasta buttons.

The worker class would provide more than just convenience - it would help keep overall code size down.

Let's say we have the one or two methods David proposed, and it takes an average of ten lines of
code to prepare data for those methods. If there are 100 places in the project where that data
preparation is needed, then we've just added 1000 lines of code to the overall project. With a
worker class, we can reduce that to one additional line of code per instance, or 100 lines total.

-Adrian

[jonwimp]

The idea seems to be to pump all entity functions through some generic method. And then have some
"auto-configuring" methods that act as convenient "chicken", "fish", "pasta" buttons.

But isn't that the same as having methods like "findByAnd" pumped through methods like
"findListIteratorByCondition"? That is, isn't the worker class idea already currently implemented?

But I do agree that it's nice to organize the "chicken" "fish" "pasta" buttons into a neat
"control panel" (your worker class), and leave the innards in some class say "BewareInnardClass".

Jonathon

Adrian Crum wrote:

> Jonathon -- Improov wrote:
>> This is a very crucial point in this thread.
>>
>> A heavily parameterized method is great for genericity. We can call
>> such a method with programmatically generated parameters. Very
>> consistent, very single-entry-point approach.
>>
>> However, such a method will require "setting up" of the parameters
>> before calling it.
>>
>> In comparison, having a few buttons named "chicken", "fish", "pasta"
>> on a microwave oven will allow me to just press the button for the
>> common scenario I'm facing at the moment. I don't need to think about
>> how much heat a chicken can take, compared to a fish.
>
> That's why I was thinking we would still need some kind of worker class
> that can be called to prepare the handful of objects we have in hand so
> they can be used in the single entry point. The worker class would
> function as the chicken, fish, and pasta buttons.
>
> The worker class would provide more than just convenience - it would
> help keep overall code size down.
>
> Let's say we have the one or two methods David proposed, and it takes an
> average of ten lines of code to prepare data for those methods. If there
> are 100 places in the project where that data preparation is needed,
> then we've just added 1000 lines of code to the overall project. With a
> worker class, we can reduce that to one additional line of code per
> instance, or 100 lines total.
>
> -Adrian
>
>

[David E Jones]

Yeah, I'm not sure I like having all of these little methods  
ANYWHERE, even in better organized in a special class. No matter how  
you slice it, trying to use 30 largely similar methods is a burden on  
the brain...

The idea of the parameters object or a small number of methods with  
more parameters would be that you wouldn't have so many methods, and  
you could select/use the options you want.

Anyway, please see the attached patch for my first attempt at  
creating a consolidated set of feature-complete methods. There are a  
bunch of changes, so best to focus on the new methods. The other  
changes are just refactoring so the old big set of methods we want to  
deprecate use the new methods.

-David




On Oct 24, 2007, at 3:23 AM, Jonathon -- Improov wrote:

> The idea seems to be to pump all entity functions through some  
> generic method. And then have some "auto-configuring" methods that  
> act as convenient "chicken", "fish", "pasta" buttons.
>
> But isn't that the same as having methods like "findByAnd" pumped  
> through methods like "findListIteratorByCondition"? That is, isn't  
> the worker class idea already currently implemented?
>
> But I do agree that it's nice to organize the "chicken" "fish"  
> "pasta" buttons into a neat "control panel" (your worker class),  
> and leave the innards in some class say "BewareInnardClass".
>
> Jonathon
>
> Adrian Crum wrote:
>> Jonathon -- Improov wrote:
>>> This is a very crucial point in this thread.
>>>
>>> A heavily parameterized method is great for genericity. We can  
>>> call such a method with programmatically generated parameters.  
>>> Very consistent, very single-entry-point approach.
>>>
>>> However, such a method will require "setting up" of the  
>>> parameters before calling it.
>>>
>>> In comparison, having a few buttons named "chicken", "fish",  
>>> "pasta" on a microwave oven will allow me to just press the  
>>> button for the common scenario I'm facing at the moment. I don't  
>>> need to think about how much heat a chicken can take, compared to  
>>> a fish.
>> That's why I was thinking we would still need some kind of worker  
>> class that can be called to prepare the handful of objects we have  
>> in hand so they can be used in the single entry point. The worker  
>> class would function as the chicken, fish, and pasta buttons.
>> The worker class would provide more than just convenience - it  
>> would help keep overall code size down.
>> Let's say we have the one or two methods David proposed, and it  
>> takes an average of ten lines of code to prepare data for those  
>> methods. If there are 100 places in the project where that data  
>> preparation is needed, then we've just added 1000 lines of code to  
>> the overall project. With a worker class, we can reduce that to  
>> one additional line of code per instance, or 100 lines total.
>> -Adrian
>

[Adrian Crum]

Looks great!

+1

David E Jones wrote:

>
> Yeah, I'm not sure I like having all of these little methods  ANYWHERE,
> even in better organized in a special class. No matter how  you slice
> it, trying to use 30 largely similar methods is a burden on  the brain...
>
> The idea of the parameters object or a small number of methods with  
> more parameters would be that you wouldn't have so many methods, and  
> you could select/use the options you want.
>
> Anyway, please see the attached patch for my first attempt at  creating
> a consolidated set of feature-complete methods. There are a  bunch of
> changes, so best to focus on the new methods. The other  changes are
> just refactoring so the old big set of methods we want to  deprecate use
> the new methods.
>
> -David

[Adam Heath-2]

David E Jones wrote:

>
> Yeah, I'm not sure I like having all of these little methods ANYWHERE,
> even in better organized in a special class. No matter how you slice it,
> trying to use 30 largely similar methods is a burden on the brain...
>
> The idea of the parameters object or a small number of methods with more
> parameters would be that you wouldn't have so many methods, and you
> could select/use the options you want.
>
> Anyway, please see the attached patch for my first attempt at creating a
> consolidated set of feature-complete methods. There are a bunch of
> changes, so best to focus on the new methods. The other changes are just
> refactoring so the old big set of methods we want to deprecate use the
> new methods.

Error messages need to be updated; findByPrimaryKey -> findOne,
findByCondition -> findList, etc

An additional method should be added to GenericEntity(and friends), that
mirrors makePKSingle; instead of returning the appropriate
GenericEntity, it returns a map properly constructed with whatever
files.  That way, you don't create a GenericEntity, just to throw it
away immediately.

When introducing a new feature, do not do other unescessary things in
the patch.  Like prefixing unaltered method calls with this.  It makes
it much harder to track down issues when they occur.  Please always run
svn diff and sanity check your changes(for instance, there is a findByOr
that is converted to a this.findByOr, with no other changes on the line).

Why aren't findByAnd and findByOr converted to findList?   Why not just
convert them to conditions?

In findList, EntityListIterator.getCompleteList() will never return
null.  There is no need to check for that.

Transaction processing is wrong.  There are error conditions that will
not cause a rollback to occur; namely, Runtime or Errors.  The proper
way to do this, is to set a flag to true right before the inner block is
done processing, then checking that flag in the finally block, and
calling rollback/commit appropriate.  Ie:

public static <V> V runTransaction(Callable<V> proc) {
        boolean success = false;
        boolean beganTx = false;
        try {
                beganTx = TransactionUtil.begin();
                V result = proc.call();
                success = true;
                return result;
        } finally {
                if (success) {
                        TransactionUtil.commit(beganTx);
                } else {
                        TransactionUtil.rollback(beganTx);
                }
        }
}

The above is an example only.  The pattern is so common, that I've
created a local class for our application, to make this easier.
Ideally, I should send that to ofbiz.

Another nice benefit of the above, is that since the meat of the
algorithm is pushed into Callable.call, you can use the chain pattern to
automatically do transaction processing, caching, or eca stuff.

[David E Jones]

On Oct 25, 2007, at 10:20 AM, Adam Heath wrote:

> David E Jones wrote:
>>
>> Yeah, I'm not sure I like having all of these little methods  
>> ANYWHERE,
>> even in better organized in a special class. No matter how you  
>> slice it,
>> trying to use 30 largely similar methods is a burden on the brain...
>>
>> The idea of the parameters object or a small number of methods  
>> with more
>> parameters would be that you wouldn't have so many methods, and you
>> could select/use the options you want.
>>
>> Anyway, please see the attached patch for my first attempt at  
>> creating a
>> consolidated set of feature-complete methods. There are a bunch of
>> changes, so best to focus on the new methods. The other changes  
>> are just
>> refactoring so the old big set of methods we want to deprecate use  
>> the
>> new methods.
>
> Error messages need to be updated; findByPrimaryKey -> findOne,
> findByCondition -> findList, etc

Yes, good point. Maybe I'll change those a little so they don't have  
this problem.

> An additional method should be added to GenericEntity(and friends),  
> that
> mirrors makePKSingle; instead of returning the appropriate
> GenericEntity, it returns a map properly constructed with whatever
> files.  That way, you don't create a GenericEntity, just to throw it
> away immediately.

This will likely be deprecated anyway.

> When introducing a new feature, do not do other unescessary things in
> the patch.  Like prefixing unaltered method calls with this.  It makes
> it much harder to track down issues when they occur.  Please always  
> run
> svn diff and sanity check your changes(for instance, there is a  
> findByOr
> that is converted to a this.findByOr, with no other changes on the  
> line).

Oh yeah, Mr. Man... why should I? Why shouldn't I clean things up  
while refactoring? When people are submitting things for committers  
to review, great, but if I'm working on code I'll clean up every darn  
thing I see as that is MY responsibility.

If you wanna fight about it get yer dukes up boy! (yeah, them's ma  
faghtin' wouds, I wanna get me in a faght)

If it's too complicated for you to review that's your problem.

> Why aren't findByAnd and findByOr converted to findList?   Why not  
> just
> convert them to conditions?

Why do I care? I'm cleaning up, not optimizing. If we continue on the  
discussed path these will be deprecated anyway. And for myself, I  
have better things to do with my time. Like argue with you... wait  
that seems unproductive, now why am I doing this again?

> In findList, EntityListIterator.getCompleteList() will never return
> null.  There is no need to check for that.

Yes, good point. 17 characters removed!

> Transaction processing is wrong.  There are error conditions that will
> not cause a rollback to occur; namely, Runtime or Errors.  The proper
> way to do this, is to set a flag to true right before the inner  
> block is
> done processing, then checking that flag in the finally block, and
> calling rollback/commit appropriate.  Ie:
>
> public static <V> V runTransaction(Callable<V> proc) {
> boolean success = false;
> boolean beganTx = false;
> try {
> beganTx = TransactionUtil.begin();
> V result = proc.call();
> success = true;
> return result;
> } finally {
> if (success) {
> TransactionUtil.commit(beganTx);
> } else {
> TransactionUtil.rollback(beganTx);
> }
> }
> }
>
> The above is an example only.  The pattern is so common, that I've
> created a local class for our application, to make this easier.
> Ideally, I should send that to ofbiz.
>
> Another nice benefit of the above, is that since the meat of the
> algorithm is pushed into Callable.call, you can use the chain  
> pattern to
> automatically do transaction processing, caching, or eca stuff.

I wouldn't say the transaction processing is "wrong". Sounds like  
it's different from what you expect.

This is how it has been, and it isn't my intention right now to  
change the error handling semantics of these methods. And, BTW, YOU  
should not either unless you discuss it on the list to maybe catch  
some things you missed. This is a fairly material change in error  
handling and might cause dependent code to need changes. Whatever the  
case, I'd rather focus on solving problems, especially since most of  
this code and the techniques in it are pretty well vetted and tested.

-David

[Adam Heath-2]

David E Jones wrote:

>
>> Transaction processing is wrong.  There are error conditions that will
>> not cause a rollback to occur; namely, Runtime or Errors.  The proper
>> way to do this, is to set a flag to true right before the inner block is
>> done processing, then checking that flag in the finally block, and
>> calling rollback/commit appropriate.  Ie:
>>
>> public static <V> V runTransaction(Callable<V> proc) {
>>     boolean success = false;
>>     boolean beganTx = false;
>>     try {
>>         beganTx = TransactionUtil.begin();
>>         V result = proc.call();
>>         success = true;
>>         return result;
>>     } finally {
>>         if (success) {
>>             TransactionUtil.commit(beganTx);
>>         } else {
>>             TransactionUtil.rollback(beganTx);
>>         }
>>     }
>> }
>>
>> The above is an example only.  The pattern is so common, that I've
>> created a local class for our application, to make this easier.
>> Ideally, I should send that to ofbiz.
>>
>> Another nice benefit of the above, is that since the meat of the
>> algorithm is pushed into Callable.call, you can use the chain pattern to
>> automatically do transaction processing, caching, or eca stuff.
>
> I wouldn't say the transaction processing is "wrong". Sounds like it's
> different from what you expect.
>
> This is how it has been, and it isn't my intention right now to change
> the error handling semantics of these methods. And, BTW, YOU should not
> either unless you discuss it on the list to maybe catch some things you
> missed. This is a fairly material change in error handling and might
> cause dependent code to need changes. Whatever the case, I'd rather
> focus on solving problems, especially since most of this code and the
> techniques in it are pretty well vetted and tested.

Consider a NullPointerException being thrown while inside a
try/catch/finally block.  Since the RuntimeException isn't caught, and
the rollback is only called when the listed exceptions are thrown, and
the commit is never called, then the transaction is never closed, and
will leak.

(I'm trying to configure some hardware for a halloween party right now,
so can't respond to your other stuff)

[David E Jones]

On Oct 25, 2007, at 8:48 PM, Adam Heath wrote:

> David E Jones wrote:
>>
>>> Transaction processing is wrong.  There are error conditions that  
>>> will
>>> not cause a rollback to occur; namely, Runtime or Errors.  The  
>>> proper
>>> way to do this, is to set a flag to true right before the inner  
>>> block is
>>> done processing, then checking that flag in the finally block, and
>>> calling rollback/commit appropriate.  Ie:
>>>
>>> public static <V> V runTransaction(Callable<V> proc) {
>>>     boolean success = false;
>>>     boolean beganTx = false;
>>>     try {
>>>         beganTx = TransactionUtil.begin();
>>>         V result = proc.call();
>>>         success = true;
>>>         return result;
>>>     } finally {
>>>         if (success) {
>>>             TransactionUtil.commit(beganTx);
>>>         } else {
>>>             TransactionUtil.rollback(beganTx);
>>>         }
>>>     }
>>> }
>>>
>>> The above is an example only.  The pattern is so common, that I've
>>> created a local class for our application, to make this easier.
>>> Ideally, I should send that to ofbiz.
>>>
>>> Another nice benefit of the above, is that since the meat of the
>>> algorithm is pushed into Callable.call, you can use the chain  
>>> pattern to
>>> automatically do transaction processing, caching, or eca stuff.
>>
>> I wouldn't say the transaction processing is "wrong". Sounds like  
>> it's
>> different from what you expect.
>>
>> This is how it has been, and it isn't my intention right now to  
>> change
>> the error handling semantics of these methods. And, BTW, YOU  
>> should not
>> either unless you discuss it on the list to maybe catch some  
>> things you
>> missed. This is a fairly material change in error handling and might
>> cause dependent code to need changes. Whatever the case, I'd rather
>> focus on solving problems, especially since most of this code and the
>> techniques in it are pretty well vetted and tested.
>
> Consider a NullPointerException being thrown while inside a
> try/catch/finally block.  Since the RuntimeException isn't caught, and
> the rollback is only called when the listed exceptions are thrown, and
> the commit is never called, then the transaction is never closed, and
> will leak.

Actually, the commit is in the finally, so the transaction should  
never leak.

The question, I suppose, is do we consider an NPE something to cause  
a rollback? Historically the answer for the Entity Engine was no.

> (I'm trying to configure some hardware for a halloween party right  
> now,
> so can't respond to your other stuff)

Sounds like fun. From my reply you can probably tell I'm thinking of  
being a fisticuffs redneck. ;)

-David