Cloning a GenericEntity is fine, and if you pass it to entity engine
methods it works as expected.
-Adrian
On 4/27/2013 4:20 PM, Paul Foxworthy wrote:
Hi Adrian,
I totally agree that GenericPK isn't pulling its weight. I think the point
of the class is just to communicate "hey, this map happens to just contain
primary key fields". GenericEntity already has an isPrimaryKey method, so if
you need to know you can find out, and we could achieve the same effect with
good variable names.
I think GenericEntity was intended as a pseudo abstract base class for
GenericPK and GenericValue, even though the abstract keyword isn't there.
There would need to be some code changes to really make it abstract, but I
think not a lot. If GenericPK goes, yes, we should just unify GenericEntity
and GenericValue.
There might be one reason to have two classes. GenericValue is the class
that tracks original values and is persisted by the Delegator, so you could
argue that GenericEntity should be immutable once populated and only
GenericValue should have the option to be mutable. That would be a
significant change from the current design, and I think of limited
usefulness.
You said "changes intended for the original GenericEntity instance are never
made". But the same problem would arise if I cloned a GenericEntity instance
and made changes to the clone and not the original. So even if there was
only one class, I could bring on the same problem. Is cloning a
GenericEntity a bad thing to do? The class implements Cloneable at present.
Cheers
Paul Foxworthy
Adrian Crum-3 wrote
I found some flaws in the entity engine while working on the entity
cache bugs.
The main problem is with the GenericEntity class and its subclasses -
GenericPK and GenericValue. When you get a GenericPK instance from
GenericEntity or GenericValue, a new GenericPK instance is created and
initialized with values from the GenericEntity (GenericValue) instance.
There is no "connection" from the new GenericPK instance to the
GenericEntity that created it. Then you use the GenericPK instance in
some entity engine method calls. Some of those method calls might cast
GenericPK to GenericEntity. In either case, the methods treat the
GenericPK as if it was the same instance as the GenericEntity instance
that created it - but it isn't, it's a copy with no connection to the
GenericEntity instance that created it. Consequently, changes intended
for the original GenericEntity instance are never made.
I can't see a reason for having the two GenericEntity subclasses.
GenericPK is a GenericEntity with a subset of the entity's fields (the
PK fields), otherwise the two are identical. GenericValue is a
GenericEntity with a few convenience methods and a copy of the original
field values. From my perspective, it would be better if GenericEntity
included all of GenericValue's features (I can't see why it shouldn't) -
eliminating the need for GenericValue.
So, the entity engine API is a bit muddled in this area and it is
causing some implementation problems.
Another problem I found was in the
AbstractEntityConditionCache.storeHook method(s). This method attempts
to perform "smart" cache clearing by analyzing all of the cached
conditions to see if they match the GenericEntity (or one of its
subclasses) that is being operated on. Any condition matches are cleared
from the cache. From my perspective, the overhead caused by all of this
analysis cancels any benefit from performing fine-grained cache clearing
- but let's assume I'm wrong and carry on...
If you paid close attention to the previous paragraphs, you should be
able to see immediately why these methods don't work. If I pass a
GenericPK instance, it isn't going to match the cached conditions
because it contains a subset of the fields contained in the conditions.
If I pass a GenericValue instance, it compares the *current* field
values with the cached conditions. Oops, they won't match if any of the
GenericValue fields were modified. It would be better to compare the
*original* field values (GenericValue has them) with the cached
conditions. The code could be modified to fix that problem but that
doesn't help GenericEntity - which doesn't contain a copy of the
original field values.
In revision 1476296 I modified GenericDelegator to always clear the
cache *after* the entity operation - so the cache will be loaded with
the changed values. With that revision, comparing GenericValue to the
cached conditions will always fail - because its original values are
lost. I bypassed using the AbstractEntityConditionCache.storeHook method
to work around that.
So, there needs to be some entity engine API changes. I have some ideas,
but I will wait for comments before proceeding further.
-Adrian
-----
--
Coherent Software Australia Pty Ltd
http://www.coherentsoftware.com.au/
Bonsai ERP, the all-inclusive ERP system
http://www.bonsaierp.com.au/
--
View this message in context:
http://ofbiz.135035.n4.nabble.com/Entity-engine-design-implementation-problems-tp4640922p4640929.html
Sent from the OFBiz - Dev mailing list archive at Nabble.com.
