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.
