castor
  1. castor
  2. CASTOR-1696

Source generator creates Vector reference setter from schema when types=j2

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.0.5
    • Fix Version/s: 1.1 M1
    • Component/s: XML code generator
    • Labels:
      None
    • Environment:
      Windows XP Pro
    • Number of attachments :
      2

      Description

      Upgraded to 1.0.5 from 1.0M4. Based on the following example snippet:

      <xs:complexType name="GenClass">
      <xs:complexContent>
      <xs:extension base="BaseGen">
      <xs:sequence>
      <xs:element name="widgets" type="Widget" minOccurs="0"
      maxOccurs="unbounded" />
      </xs:sequence>
      </xs:extension>
      </xs:complexContent>
      </xs:complexType>

      The following setters are generated (with Java 5 support turned on):

      public void setWidgets(Widget[] vWidgetsArray)
      public void setWidgets(java.util.List<Widget> vWidgetsList)
      public void setWidgetsList(java.util.Vector<Widget> WidgetsVector)

      whereas the following were generated in 1.0M4:

      public void setWidgets(Widget[] WidgetsArray)
      public void setWidgets(java.util.List widgetsCollection)
      public void setWidgetsList(java.util.List widgetsCollection)

        Activity

        Hide
        Werner Guttmann added a comment -

        Why would you need - in j2 mode - a second setter with identical signature ? And as mentioned on the mailing list, this method has a particular purpose: to allow the member to be populated by passing a Vector to it. Now, having said that, one could argue that the name itself is not really intuitive ...,

        Show
        Werner Guttmann added a comment - Why would you need - in j2 mode - a second setter with identical signature ? And as mentioned on the mailing list, this method has a particular purpose: to allow the member to be populated by passing a Vector to it. Now, having said that, one could argue that the name itself is not really intuitive ...,
        Hide
        Ralf Joachim added a comment -

        If the setWidgetsList() at previous releases took a List as argument in j2 mode I think we should change that back from Vector to List. In addition it does not make sense to have 2 methods (setWidgets() and setWidgetsList()) with the same List parameter. I therefore think we should declare the setWidgetsList() as deprecated asap and remove it on the long term.

        Show
        Ralf Joachim added a comment - If the setWidgetsList() at previous releases took a List as argument in j2 mode I think we should change that back from Vector to List. In addition it does not make sense to have 2 methods (setWidgets() and setWidgetsList()) with the same List parameter. I therefore think we should declare the setWidgetsList() as deprecated asap and remove it on the long term.
        Hide
        Edward Kuns added a comment -

        I agree with Ralf. And something that takes a List will accept a Vector anyway, right?

        Show
        Edward Kuns added a comment - I agree with Ralf. And something that takes a List will accept a Vector anyway, right?
        Hide
        Werner Guttmann added a comment -

        Okay, will make the method name more 'flexible's as requested, but at the same time make this method 'deprecated' (if that's achievable with the code generator at all).

        Show
        Werner Guttmann added a comment - Okay, will make the method name more 'flexible's as requested, but at the same time make this method 'deprecated' (if that's achievable with the code generator at all).
        Hide
        Werner Guttmann added a comment -

        Looks like the damage happened between versions 6202 and 6214 of CollectionInfo(J2), when basically all 'method creation' methods moved into CollectionInfo only without taking into consideration that the implementation of createAccessMethods() used different JParameter objects (siffernetly tpyed) for creating calling the createSetCollection() methods.

        Show
        Werner Guttmann added a comment - Looks like the damage happened between versions 6202 and 6214 of CollectionInfo(J2), when basically all 'method creation' methods moved into CollectionInfo only without taking into consideration that the implementation of createAccessMethods() used different JParameter objects (siffernetly tpyed) for creating calling the createSetCollection() methods.
        Hide
        Werner Guttmann added a comment -

        After a few more tests, here's what I found.

        Currently, we create the following methods for 'j1':

        public void setWidgets(int index, java.lang.String vWidgets)
        public void setWidgets(java.lang.String[] vWidgetsArray)
        public void setWidgets(java.util.Vector vWidgetsList)
        public void setWidgetsAsReference(java.util.Vector WidgetsVector)

        and for 'j2':

        public void setWidgets(int index, java.lang.String vWidgets)
        public void setWidgets(java.lang.String[] vWidgetsArray)
        public void setWidgets(java.util.List vWidgetsList)
        public void setWidgetsAsReference(java.util.Vector WidgetsVector)

        Whilst the first three methods get generated as expected, the last one seems to be wrong. I will make the changes for this method to behave according to the otehr ones, i.e not ignoring the field info factory used.

        Show
        Werner Guttmann added a comment - After a few more tests, here's what I found. Currently, we create the following methods for 'j1': public void setWidgets(int index, java.lang.String vWidgets) public void setWidgets(java.lang.String[] vWidgetsArray) public void setWidgets(java.util.Vector vWidgetsList) public void setWidgetsAsReference(java.util.Vector WidgetsVector) and for 'j2': public void setWidgets(int index, java.lang.String vWidgets) public void setWidgets(java.lang.String[] vWidgetsArray) public void setWidgets(java.util.List vWidgetsList) public void setWidgetsAsReference(java.util.Vector WidgetsVector) Whilst the first three methods get generated as expected, the last one seems to be wrong. I will make the changes for this method to behave according to the otehr ones, i.e not ignoring the field info factory used.
        Hide
        Werner Guttmann added a comment -

        Just wondering whether it would be acceptable for the parameter of the setWidgetsAsReference to carry a suffix that actually represents the actual java.util.* class type, i.e. 'Vector', 'List', 'Collection', etc., and not just 'Collection' for all Java 2 collection types ?

        Show
        Werner Guttmann added a comment - Just wondering whether it would be acceptable for the parameter of the setWidgetsAsReference to carry a suffix that actually represents the actual java.util.* class type, i.e. 'Vector', 'List', 'Collection', etc., and not just 'Collection' for all Java 2 collection types ?
        Hide
        Werner Guttmann added a comment -

        Initial patch for review.

        Show
        Werner Guttmann added a comment - Initial patch for review.
        Hide
        Ralf Joachim added a comment -

        It would easy review if you could attach a snipped of the generated code for such issues.
        I wonder why the method is called setWidgetsAsReference() instead of setWidgetList().
        It seams you have not found an easy way to define deprecation for a generated method.

        Show
        Ralf Joachim added a comment - It would easy review if you could attach a snipped of the generated code for such issues. I wonder why the method is called setWidgetsAsReference() instead of setWidgetList(). It seams you have not found an easy way to define deprecation for a generated method.
        Hide
        Edward Kuns added a comment -

        I think the most general solution would be to have a method (for J2) that accepts Collection and to have a method that returns Collection. That is, why not:

        public void setWidgets(java.util.Collection vWidgetsList)

        if we can truly handle Collection, then we don't care what kind of collection they pass in and we don't have to worry about making a plethora of differently-named methods to handle different intputs. Then we could get rid of the "as reference" (because what does that mean?) method entirely.

        Is there any reason (other than possible backwards compatibility) that we need the method

        public void setWidgetsAsReference(java.util.Vector WidgetsVector)

        if we already have the setWidgets that takes Collection? If we need it for backwards compatiblity, then how about it just is a proxy that immediately passes control to setWidgets(java.util.Collection)?

        Show
        Edward Kuns added a comment - I think the most general solution would be to have a method (for J2) that accepts Collection and to have a method that returns Collection. That is, why not: public void setWidgets(java.util.Collection vWidgetsList) if we can truly handle Collection, then we don't care what kind of collection they pass in and we don't have to worry about making a plethora of differently-named methods to handle different intputs. Then we could get rid of the "as reference" (because what does that mean?) method entirely. Is there any reason (other than possible backwards compatibility) that we need the method public void setWidgetsAsReference(java.util.Vector WidgetsVector) if we already have the setWidgets that takes Collection? If we need it for backwards compatiblity, then how about it just is a proxy that immediately passes control to setWidgets(java.util.Collection)?
        Hide
        Werner Guttmann added a comment -

        Ralf, the method has been deliberately been renamed to reflect the fact that you are injecting a reference (as opposed to constructing a new collection) s part of quite some big refactoring a few weeks ago.

        Show
        Werner Guttmann added a comment - Ralf, the method has been deliberately been renamed to reflect the fact that you are injecting a reference (as opposed to constructing a new collection) s part of quite some big refactoring a few weeks ago.
        Hide
        Edward Kuns added a comment -

        Werner, are you saying that the setXXXXXAsReference **REPLACES** whatever collection is currently held with the one being passed in? If so, I would suggest another renaming to make it more obvious that values previously in the collection will be lost.

        Show
        Edward Kuns added a comment - Werner, are you saying that the setXXXXXAsReference ** REPLACES ** whatever collection is currently held with the one being passed in? If so, I would suggest another renaming to make it more obvious that values previously in the collection will be lost.
        Hide
        Werner Guttmann added a comment -

        Based upon the comments that will be generated ....

        /** Sets the value of 'widgetList' by setting it to the given Vector. No type checking is performed. */

        and the code generated as follows

        this.widgetList = widgetList;

        this is what happens.

        Show
        Werner Guttmann added a comment - Based upon the comments that will be generated .... /** Sets the value of 'widgetList' by setting it to the given Vector. No type checking is performed. */ and the code generated as follows this.widgetList = widgetList; this is what happens.
        Hide
        Werner Guttmann added a comment -

        All others either use Collection.addAll() or Collection.add() to copy elements in to the target collection (after a Collection.clear(), that is).

        Show
        Werner Guttmann added a comment - All others either use Collection.addAll() or Collection.add() to copy elements in to the target collection (after a Collection.clear(), that is).
        Hide
        Edward Kuns added a comment -

        I now understand much better what your questions are. You are asking ONLY about the method

        setWidgetsAsReference(java.util.Vector WidgetsVector)

        correct? What is the value of this method to someone who is using Castor? It's more efficient than doing Collection.clear() then .addAll(), but other than this, is there value added by this method? (I imagine this is why Ralf suggests getting rid of this method altogether.)

        As long as we generate this method, we definitely want the method parameter type to be the same as the type of the collection used. (So we will not need type verification or casting and there'll be no risk of class cast exception.)

        Other than this, I cannot really think of a more meaningful name than setXXXXXXAsReference.

        Show
        Edward Kuns added a comment - I now understand much better what your questions are. You are asking ONLY about the method setWidgetsAsReference(java.util.Vector WidgetsVector) correct? What is the value of this method to someone who is using Castor? It's more efficient than doing Collection.clear() then .addAll(), but other than this, is there value added by this method? (I imagine this is why Ralf suggests getting rid of this method altogether.) As long as we generate this method, we definitely want the method parameter type to be the same as the type of the collection used. (So we will not need type verification or casting and there'll be no risk of class cast exception.) Other than this, I cannot really think of a more meaningful name than setXXXXXXAsReference.
        Hide
        Werner Guttmann added a comment -

        Fine by me. I'll add add the @deprecated comment, and make sure that the method is correctly typed, according to what the user specified either when calling the SourceGenerator or through the binding file (as per the recently added new collection type support).

        Show
        Werner Guttmann added a comment - Fine by me. I'll add add the @deprecated comment, and make sure that the method is correctly typed, according to what the user specified either when calling the SourceGenerator or through the binding file (as per the recently added new collection type support).
        Hide
        Werner Guttmann added a comment -

        Whilst we are actually talking about things related to collections, does anybody have a few what methods should be generated for Map-based collection types ?

        Show
        Werner Guttmann added a comment - Whilst we are actually talking about things related to collections, does anybody have a few what methods should be generated for Map-based collection types ?
        Hide
        Werner Guttmann added a comment -

        Edward, let me just make sure that I understand you correctly. Above, you state ...

        > As long as we generate this method, we definitely want the method parameter type to be the same as
        > the type of the collection used. (So we will not need type verification or casting and there'll be no risk of class cast exception.)

        My original idea was to bring this in line with the FieldInfoFactory used when creating the SourceGenerator instance ('j1', 'j2'). SO it could either be

        public void setWidgetsAsReference(java.util.Vector widgetsVector), or
        public void setWidgetsAsReference(java.util.List widgetsList)

        Having said that, you might have noted that users are now able to override the J2 collection type used for a collection member, by e.g. using java.util.Set, java.util.SortedSet et alias. How am I meant to be reading your statement (quoted above) with regards to this question ?

        Show
        Werner Guttmann added a comment - Edward, let me just make sure that I understand you correctly. Above, you state ... > As long as we generate this method, we definitely want the method parameter type to be the same as > the type of the collection used. (So we will not need type verification or casting and there'll be no risk of class cast exception.) My original idea was to bring this in line with the FieldInfoFactory used when creating the SourceGenerator instance ('j1', 'j2'). SO it could either be public void setWidgetsAsReference(java.util.Vector widgetsVector), or public void setWidgetsAsReference(java.util.List widgetsList) Having said that, you might have noted that users are now able to override the J2 collection type used for a collection member, by e.g. using java.util.Set, java.util.SortedSet et alias. How am I meant to be reading your statement (quoted above) with regards to this question ?
        Hide
        Edward Kuns added a comment -

        If users override the J2 collection type, then our best course of action is to use THAT type in the signature. That way we expose a clear API and we avoid the risk of incompatible types or of a class cast exception.

        Show
        Edward Kuns added a comment - If users override the J2 collection type, then our best course of action is to use THAT type in the signature. That way we expose a clear API and we avoid the risk of incompatible types or of a class cast exception.

          People

          • Assignee:
            Werner Guttmann
            Reporter:
            Dhruva Reddy
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

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