Shift deletion of records of extended objects from SQLStatementRemove to ClassMolder

Hi Ralf,

I went through the stuff related to shifting extended part of code. I found the following method in classmolder in which it is using _persistence.delete statement. Is this the place where we can move the extended refactored code snippet?

  /**
     * Delete an object of the base type from the persistence storage.
     * All object to be deleted by this method will be <tt>markDelete</tt>
     * before it method is called.
     *
     * @param tx - transaction in action
     * @param oid - the object identity of the target object
     */
    public void delete(final TransactionContext tx, final OID oid) throws PersistenceException {

        resetResolvers();

        Identity ids = oid.getIdentity();

        for (int i = 0; i < _fhs.length; i++) {
            if (_fhs[i].isManyToMany()) {
                _fhs[i].getRelationLoader().deleteRelation(
                        tx.getConnection(oid.getMolder().getLockEngine()), ids);
            }
        }

        _persistence.delete(tx.getConnection(oid.getMolder().getLockEngine()), ids);    <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

        // All field along the extend path will be deleted by transaction
        // However, everything off the path must be deleted by ClassMolder.

        Vector<ClassMolder> extendPath = new Vector<ClassMolder>();
        ClassMolder base = this;
        while (base != null) {
            extendPath.add(base);
            base = base._extends;
        }

        base = _depends;
        while (base != null) {
            if (base._extendent != null) {
                for (int i = 0; i < base._extendent.size(); i++) {
                    if (extendPath.contains(base._extendent.get(i))) {
                        // NB: further INVESTIGATION
                    }
                }
            }

            base = base._extends;
        }

    }

Thanks.

Yes.

Thanks for the comment. The place where I can move code is just after the following statement inside ClassMolder delete method
_persistence.delete(tx.getConnection(oid.getMolder().getLockEngine()), ids);

But I am getting problem in understanding how to access the sqlengine here and call the delete method. We have to move the following code from SQLStatementRemove to ClassMolder

if (extended != null)

And the parameter should be [tx.getConnection(oid.getMolder().getLockEngine()), ids] ?

Thanks.

I think I got little bit idea how we can add the code there. I did something like this

_persistence.delete(tx.getConnection(oid.getMolder().getLockEngine()), ids); // Original statement inside delete method of ClassMolder

//After that I did something like this

ClassMolder extended = this.getExtends();
// Must delete record in parent table last.
// All other dependents have been deleted before.
if (extended != null)

Is that fine? If it is, then I will make a patch today so that you can review the changes.

Thanks

Looks fine to me.

This is the patch for this issue. Please use this in conjuncton with all the patches for the subtasks of issue CASTOR-2702.

Any Comments or Suggestions.

Thanks.

This patch is not self-contained as the changes to SQLStatementRemove are missing.

In addition the original code recursively deleted all records of the extend hierarchy while the patch does only delete the record of the class directly extended. In other words if you have a hierarchy C extends B extends A, remove C would have called remove B and this would have called remove A subsequently. After the patch a call to remove C would only remove B record but not A record.

Just trying to follow this as much as I can (with time currently severly limited due to a huge go live in 10 days). I just wonder whether it would make sense to commit the smallest patch units possible ? I guess soon Lukas and I will be reviewing and refactoring JDO code as well, and I think we should try to minimize potential overlaps as much as possible.

I do see that this issue is a sub-task of CASTOR-2702, and I guess you guys have planned to go for a single patch that accumulates many like this one.

What do you think ?

Hi Werner, I don't think there will be much overlay between the work of Ahmad and Lukas as they work on different ends of Castor. At the begining I like Ahmad to work with very small pieces to get him familiar with procedures and give me an impression of his skills. As soon as I have enough faith in his experience and he is familiar with procedures we'll go for bigger chunks. In addition there will be quite some work at new classes where we need to touch current codebase only when integrating them.

Hi Ralf,

I see the issue of recursive deletion. I don't know whether we can use recursive approach anymore because ClassMolder is at the top level in a heirarchy than SQLStatmentRemove. So I think we can loop through the hiearchy. The stuff I was imagining should be like:

ClassMolder extended = this.getExtends();
Connection con = tx.getConnection(oid.getMolder().getLockEngine());

while (extended != null)

I don't know whether this is fine or not. By the way how we can test this thing?

Regards, Ahmad

In this patch I included both ClassMolder & SQLStatementRemove classes. I imagine self containment of patches means that I include both the classes in this patch.

Regards, Ahmad

Self-containment of patch indeed means that all changes required to fix a task is included in the patch. As you have to change SQLStatementRemove and ClassMolder to fix this issue changes to both classes should be contained in the patch. But in addition it does also mean that the patch should not contain changes of other tasks. In your patch you also included changes to castor-2704 and castor-2705 which should not be the case. Having said that parts of the changes to one of these foreign issues is not okay.

As you see at the example of the recursive removal of the extended records and my explanation in a former comment there are quite some reasons why I decided to take such small steps. The more mistakes you make and the longer you do not follow this rules the longer we have to continue with such small iterations.

Please prepare a new patch that only includes the changes to the issue at hand and no change to one of the other issues including release notes as I showed you at castor-2706.

Ralf, I thin initially you are correct; but I am 100% positive that half way through Lukas' work he'll have to e.g. touch ClassMolder as well. But for the time being we should be safe.

Final Patch with Release info.

Regards, Ahmad

While fixing some tab and idention issues at Ahmad's patch I recognized some areas where code could be improved a bit and decided to go for them. Final patch attached.