castor
  1. castor
  2. CASTOR-778

"Useless" code in ClassMolder.delete

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.5.3
    • Fix Version/s: 0.9.9 M1
    • Component/s: JDO
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: PC
    • Number of attachments :
      2

      Description

      I found some code in ClassMolder.delete that isn't doing anything other then to
      burn cpu-cycles and the patch also have some other XXX's that points to some
      unused/empty methods that perhaps could be removed.

      1. patch
        3 kB
        Stein M. Hugubakken
      2. patch.20050725.txt
        6 kB
        Werner Guttmann

        Issue Links

          Activity

          Hide
          Stein M. Hugubakken added a comment -

          Created an attachment (id=518)
          Proposed patch

          Show
          Stein M. Hugubakken added a comment - Created an attachment (id=518) Proposed patch
          Hide
          Werner Guttmann added a comment -

          Reassigned to myself, added Thomas to CC.

          Show
          Werner Guttmann added a comment - Reassigned to myself, added Thomas to CC.
          Hide
          Werner Guttmann added a comment -

          The following comment from ClassMOlder.deleteExtend()

          /**

          • It method is called by delete to delete the extended object of the base
          • type from the Persistence.
            *
            */

          got me thinking a little bit more. Could it be that this is code intended to
          work with polymorhism ? It would be absolutely brilliant if somebody working on
          Castor years back could share his or her knowledge with us on this very subject.

          Show
          Werner Guttmann added a comment - The following comment from ClassMOlder.deleteExtend() /** It method is called by delete to delete the extended object of the base type from the Persistence. * */ got me thinking a little bit more. Could it be that this is code intended to work with polymorhism ? It would be absolutely brilliant if somebody working on Castor years back could share his or her knowledge with us on this very subject.
          Hide
          Werner Guttmann added a comment -

          Actually, just realised what the commented code fragment is about. Imagine a
          hierarchy of ObjectA and ObjectB where ObjectB extends ObjectA. In the case
          where we are trying to remove an instance of ObjectB, everything works fine. In
          the case where we are trying to remove an instance of ObjectA, things do NOT
          work at the moment and tangling table entries in the table to which ObjectB is
          mapped are the result.

          Imho, the commented calls to deleteExtend() need to be uncommented, i.e.
          reactivated. But before even thinking about this, I'd appreciate any comments
          (as always).

          Show
          Werner Guttmann added a comment - Actually, just realised what the commented code fragment is about. Imagine a hierarchy of ObjectA and ObjectB where ObjectB extends ObjectA. In the case where we are trying to remove an instance of ObjectB, everything works fine. In the case where we are trying to remove an instance of ObjectA, things do NOT work at the moment and tangling table entries in the table to which ObjectB is mapped are the result. Imho, the commented calls to deleteExtend() need to be uncommented, i.e. reactivated. But before even thinking about this, I'd appreciate any comments (as always).
          Hide
          Werner Guttmann added a comment -

          Actually, after some more thinking, it seems like deleteExtend() does not yield
          the desired results, as it covers dependent objects only. Imho, one needs to
          write code that deals with the extend hierarchy in both directions. At the
          moment, only one direction (towards the base class of the hierarchy) is covered
          completely.

          Show
          Werner Guttmann added a comment - Actually, after some more thinking, it seems like deleteExtend() does not yield the desired results, as it covers dependent objects only. Imho, one needs to write code that deals with the extend hierarchy in both directions. At the moment, only one direction (towards the base class of the hierarchy) is covered completely.
          Hide
          Bruce Snyder (bugzilla) added a comment -

          I have come across this before myself, but I never took the time to understand
          it. I think that this method should be made to work beyond only dependent
          objects, but there may be a reason why it was left in this state. Maybe Thomas
          knows.

          Show
          Bruce Snyder (bugzilla) added a comment - I have come across this before myself, but I never took the time to understand it. I think that this method should be made to work beyond only dependent objects, but there may be a reason why it was left in this state. Maybe Thomas knows.
          Hide
          Thomas Yip added a comment -

          I think Werner was right on what the code suppose to do, and why the code
          doesn't work.

          Show
          Thomas Yip added a comment - I think Werner was right on what the code suppose to do, and why the code doesn't work.
          Hide
          Werner Guttmann added a comment -

          Great. Anybody willing (and able) to work with me on this ?

          Show
          Werner Guttmann added a comment - Great. Anybody willing (and able) to work with me on this ?
          Hide
          Werner Guttmann added a comment -

          Well, well .. . Rather than looking for peers joining me on this, let me try
          to get my head around the following question. With the above scenario, where we
          have ObjectBs extending ObjectA, is the following statement correct?

          "If we delete an instanec of ObjectA, we have to delete all instances of ObjectB
          that share the same identity."

          Opinions ?

          Show
          Werner Guttmann added a comment - Well, well .. . Rather than looking for peers joining me on this, let me try to get my head around the following question. With the above scenario, where we have ObjectBs extending ObjectA, is the following statement correct? "If we delete an instanec of ObjectA, we have to delete all instances of ObjectB that share the same identity." Opinions ?
          Hide
          Werner Guttmann added a comment -

          Thomas, it seems like the code already tries to take care of DEPEND relations,
          etc. when trying to delete an instance of ObjectB, as there is code that seems
          to walk the complete object graph in terms of dependent objects.

          My question is about the sequence of events when deleting an instance of ObjectB
          as discussed above. Right now, the current code seems to want to ..

          a) delete the instance of ObjectB (SQLEngine.delete())
          b) delete the instance of ObjectA (SQLEngine.delete())
          c) ???
          d) ???

          Can you please fill in c) and d) for me, using lines 2288 to 2315. It looks like
          d) is about finding all classes that extend ObjectB (directly or indirectly),
          and delete them as well. But what is d) about ?

          Show
          Werner Guttmann added a comment - Thomas, it seems like the code already tries to take care of DEPEND relations, etc. when trying to delete an instance of ObjectB, as there is code that seems to walk the complete object graph in terms of dependent objects. My question is about the sequence of events when deleting an instance of ObjectB as discussed above. Right now, the current code seems to want to .. a) delete the instance of ObjectB (SQLEngine.delete()) b) delete the instance of ObjectA (SQLEngine.delete()) c) ??? d) ??? Can you please fill in c) and d) for me, using lines 2288 to 2315. It looks like d) is about finding all classes that extend ObjectB (directly or indirectly), and delete them as well. But what is d) about ?
          Hide
          Werner Guttmann added a comment -

          And before I get the most essential question, hw come the calls to
          deleteExtend() have been commented in the first place ? Is this for a particular
          reason ?

          Show
          Werner Guttmann added a comment - And before I get the most essential question, hw come the calls to deleteExtend() have been commented in the first place ? Is this for a particular reason ?
          Hide
          Thomas Yip added a comment -

          I would need to inspect the code in detail, before I can
          figure it out. Will come back to you soon, hopefully in
          a day or two.

          Show
          Thomas Yip added a comment - I would need to inspect the code in detail, before I can figure it out. Will come back to you soon, hopefully in a day or two.
          Hide
          Bruce Snyder (bugzilla) added a comment -

          Werner, I notice that you added some logging for the case where the extendPath
          contains the directly extending class. Did you make any progress on this?

          Show
          Bruce Snyder (bugzilla) added a comment - Werner, I notice that you added some logging for the case where the extendPath contains the directly extending class. Did you make any progress on this?
          Hide
          Werner Guttmann added a comment -

          Not really, as things are pretty complex. I'll have to look at some code to see
          whether I have done anything at all, or just analysed current behaviour.

          Show
          Werner Guttmann added a comment - Not really, as things are pretty complex. I'll have to look at some code to see whether I have done anything at all, or just analysed current behaviour.
          Hide
          Werner Guttmann added a comment -

          Based upon my work to add support for polymorphism, I think that this code really is not used at all, and hence should/could be removed.

          Show
          Werner Guttmann added a comment - Based upon my work to add support for polymorphism, I think that this code really is not used at all, and hence should/could be removed.
          Hide
          Werner Guttmann added a comment -

          Used to be BugZilla bug 1636.

          Show
          Werner Guttmann added a comment - Used to be BugZilla bug 1636.
          Hide
          Werner Guttmann added a comment -

          Will prepare and commit a patch for removal of bespoke code when committing issue CASTOR-1018.

          Show
          Werner Guttmann added a comment - Will prepare and commit a patch for removal of bespoke code when committing issue CASTOR-1018 .
          Hide
          Werner Guttmann added a comment -

          Patch as discussed above, basically removing ClassMolder.deleteExtend() and all (commented) references to it.

          Show
          Werner Guttmann added a comment - Patch as discussed above, basically removing ClassMolder.deleteExtend() and all (commented) references to it.
          Hide
          Ralf Joachim added a comment -

          As deleteExtend() is never called I think we are on the save side to delete that code. Therefore go ahead and commit.

          Show
          Ralf Joachim added a comment - As deleteExtend() is never called I think we are on the save side to delete that code. Therefore go ahead and commit.

            People

            • Assignee:
              Werner Guttmann
              Reporter:
              Stein M. Hugubakken
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 15 minutes
                15m