castor
  1. castor
  2. CASTOR-1147

CTF test case TC28 (jdo.Collections) fails when set-method points to a "addItem" method.

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Blocker Blocker
    • Resolution: Unresolved
    • Affects Version/s: 0.9.7 M1
    • Fix Version/s: None
    • Component/s: JDO
    • Labels:
      None
    • Number of attachments :
      1

      Description

      jdo.TestColAdd has a field item (ArrayList of jdo.Item), for which in the mapping we declare the method "addItem" to be used through the field mapping (as per set-method). As a aresult of this, TC28 fails at least in one place during rollback in a call to FieldMolder.setValue(). It fails simply because it tries to call TestColAdd.setItem(ArrayList) ... which happens not to be there.

        Issue Links

          Activity

          Hide
          Werner Guttmann added a comment -

          Initial patch, proposing a resolution where in FieldMolder we additionally check for a setXX(9 method and if we cannot find one, throw an exception.

          Show
          Werner Guttmann added a comment - Initial patch, proposing a resolution where in FieldMolder we additionally check for a setXX(9 method and if we cannot find one, throw an exception.
          Hide
          Werner Guttmann added a comment -

          FWIT, tests run successfulyl on mySQL, but I honestly cannot predict whether above patch will introduce any side effects of unforeseen nature .. .

          Show
          Werner Guttmann added a comment - FWIT, tests run successfulyl on mySQL, but I honestly cannot predict whether above patch will introduce any side effects of unforeseen nature .. .
          Hide
          Ralf Joachim added a comment -

          Have you tested if still the add method is used as default and not the set method that should be used as fallback only? There is at minium the side effect that users that worked with add method only (e.g. gregory) now are required to also implement a set method.

          According to the side effects you mentioned I think we only should comment out the rollback part of the new test that fails.

          Show
          Ralf Joachim added a comment - Have you tested if still the add method is used as default and not the set method that should be used as fallback only? There is at minium the side effect that users that worked with add method only (e.g. gregory) now are required to also implement a set method. According to the side effects you mentioned I think we only should comment out the rollback part of the new test that fails.
          Hide
          Werner Guttmann added a comment -

          Based upon the commens above, I'll revert the addition by Greg, and we'll revist the problem later on, e.g. for 0.9.9.

          Show
          Werner Guttmann added a comment - Based upon the commens above, I'll revert the addition by Greg, and we'll revist the problem later on, e.g. for 0.9.9.
          Hide
          Werner Guttmann added a comment -

          Addition reverted as indicated above.

          Show
          Werner Guttmann added a comment - Addition reverted as indicated above.
          Hide
          Gregory Block added a comment -

          So, where to begin on this?

          The ability to handle and manage getters and setters is rife with trouble, at the moment.

          • Having an addXXX() method does not remove the requirement of a setXXX() method
          • In theory get/set is anathema to the add/iterate model; in addition, the current model, which allows for 'arbitrary' mix-and-match doesn't actually work out.
          • addXXX() is all well and good, but when it comes to revert the object from cache, we're dependent on setXXX()'s presence.

          One of two things is going to need to happen in order for this to be normalised.

          1) We dump 'arbitrary' get/set/add combinations, and move to one of two handler pairs: It's either a getCollection/setCollection handler, which handles getting and setting of the complete collection, or an add/iterate handler which supports adding, iterating, and calling remove() on the iterator.

          2) We start allowing the calling of private or protected methods, so that API which is critical to not make public can be created on the objects, but refused as public API.

          Option 1) has the benefit of cleaning up a great deal of logic, in theory, across the board - method discovery becomes method-pair discovery, and it provides a nice place to push code for handling the brains of getter/setter handling.

          Option 2) has the benefit, if widely accepted, of allowing people to designate non-public API as accessible by Castor, to allow for developers to lock down their API for other developers on objects without requiring expensive copy operations on these objects.

          Status quo is, IMO, not an option.

          Show
          Gregory Block added a comment - So, where to begin on this? The ability to handle and manage getters and setters is rife with trouble, at the moment. Having an addXXX() method does not remove the requirement of a setXXX() method In theory get/set is anathema to the add/iterate model; in addition, the current model, which allows for 'arbitrary' mix-and-match doesn't actually work out. addXXX() is all well and good, but when it comes to revert the object from cache, we're dependent on setXXX()'s presence. One of two things is going to need to happen in order for this to be normalised. 1) We dump 'arbitrary' get/set/add combinations, and move to one of two handler pairs: It's either a getCollection/setCollection handler, which handles getting and setting of the complete collection, or an add/iterate handler which supports adding, iterating, and calling remove() on the iterator. 2) We start allowing the calling of private or protected methods, so that API which is critical to not make public can be created on the objects, but refused as public API. Option 1) has the benefit of cleaning up a great deal of logic, in theory, across the board - method discovery becomes method-pair discovery, and it provides a nice place to push code for handling the brains of getter/setter handling. Option 2) has the benefit, if widely accepted, of allowing people to designate non-public API as accessible by Castor, to allow for developers to lock down their API for other developers on objects without requiring expensive copy operations on these objects. Status quo is, IMO, not an option.
          Hide
          Ralf Joachim added a comment -

          I think with accesing protected or private methods or properties we can work around the problem seen but IMO its only a workaround. Therfore i suggest to discuss that at another issue. The real problem is the crapy access to collections. Like you, i think we should introduce different strategies to access collections but IMO there are 3 stategies to implement and they also depend on the collection type.

          a) If we have getCollection() and setCollection() methods we can don't have the requirement for the getter to return the original collection. This is especially required if collection type is Iterator or Enumeration.

          b) The third posibility will be used if we have getCollection(), addObject() and removeObject() methods. This also not requires the getter to return the original collection. and gives the user the possiblility to add costum code e.g to ensure that bidirectional relations are consistent.

          c) With most of the collections only a getCollection() method that returns a reference to the original collection is enough. We can use the collections add() and remove() methods to modify it in this case. Care should be taken that the getCollection() method not returns a copy or clone of the original collection as this will break things here. Having said that I recognized some weeks ago that castor currently also has problems when the getCollection() method returns a copy of the original collection.

          There are qiute some more strategies but IMO they are all variations of the 3 described above. That's also true for the iterator one you mentioned.

          Show
          Ralf Joachim added a comment - I think with accesing protected or private methods or properties we can work around the problem seen but IMO its only a workaround. Therfore i suggest to discuss that at another issue. The real problem is the crapy access to collections. Like you, i think we should introduce different strategies to access collections but IMO there are 3 stategies to implement and they also depend on the collection type. a) If we have getCollection() and setCollection() methods we can don't have the requirement for the getter to return the original collection. This is especially required if collection type is Iterator or Enumeration. b) The third posibility will be used if we have getCollection(), addObject() and removeObject() methods. This also not requires the getter to return the original collection. and gives the user the possiblility to add costum code e.g to ensure that bidirectional relations are consistent. c) With most of the collections only a getCollection() method that returns a reference to the original collection is enough. We can use the collections add() and remove() methods to modify it in this case. Care should be taken that the getCollection() method not returns a copy or clone of the original collection as this will break things here. Having said that I recognized some weeks ago that castor currently also has problems when the getCollection() method returns a copy of the original collection. There are qiute some more strategies but IMO they are all variations of the 3 described above. That's also true for the iterator one you mentioned.
          Hide
          Ralf Joachim added a comment -

          Patch has been reverted due to problem described at CASTOR-1147

          Show
          Ralf Joachim added a comment - Patch has been reverted due to problem described at CASTOR-1147

            People

            • Assignee:
              Unassigned
              Reporter:
              Werner Guttmann
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Time Tracking

                Estimated:
                Original Estimate - 2 hours
                2h
                Remaining:
                Time Spent - 1 hour Remaining Estimate - 1 hour
                1h
                Logged:
                Time Spent - 1 hour Remaining Estimate - 1 hour
                1h