Tuscany
  1. Tuscany
  2. TUSCANY-1465

Allow passing ResultDescriptor for dynamic Commands

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Java-DAS-Next
    • Fix Version/s: Java-DAS-Next
    • Component/s: Java DAS RDB
    • Labels:
      None

      Description

      http://www.mail-archive.com/tuscany-dev@ws.apache.org/msg19886.html
      Action for the issue discussed in above mail.

      1. 1465.patch
        25 kB
        Amita Vadhavkar
      2. 1465.patch
        31 kB
        Amita Vadhavkar

        Activity

        Hide
        Amita Vadhavkar added a comment -

        Added new attribute <xsd:attribute name="columnIndex" type="xsd:int" default="-1"/>
        added to ResultDescriptor in config.
        Some conversation on below thread-
        http://www.mail-archive.com/tuscany-user@ws.apache.org/msg01506.html

        Show
        Amita Vadhavkar added a comment - Added new attribute <xsd:attribute name="columnIndex" type="xsd:int" default="-1"/> added to ResultDescriptor in config. Some conversation on below thread- http://www.mail-archive.com/tuscany-user@ws.apache.org/msg01506.html
        Hide
        Adriano Crestani added a comment -
        • On ReadCommandImpl.setResultDescriptor(List) you are storing the list directly, however I think the list should be copied(also the elements), because the way it is the user can modify the Command's ResultDescriptor List or the ResultDescriptor itself anywhere else on users'code without notify the Command.
        • The ResultDescriptorSorter.sortList(List) is returning 3 different ways:
          1. When the List parameter is null, the method returns null. I think it's ok
          2. When the List is empty, the method returns the same List object
          3. Otherwise, it's returning a new List object containing the ResultDescriptors sorted by the index

        I think on 2 the List returned should be a new empty List and not the parameter List. OR the 3 could return the same List but with the objects sorted. I think it's important to define the method logic, if it will always return the same List passed on parameter or return a new List.

        • Why ResultDescriptor cannot be made Comparable?
        • Is there any reason not to name the method List Command.set/getResultDescriptor to set/getResultDescriptors? Because it returns a List of ResultDescriptors, and not a single one.

        Maybe we could use some pattern here, as addResultDescriptor(ResultDescriptor), removeResultDescriptor(ResultDescriptor), and getResultDescriptor(int).

        • Why the ResultDescriptor Lists paremeters and returns cannot be defined as List<ResultDescriptor>?

        I applied the patch on my trunk and it passed on all mvn tests, I think it`s ok. I just want to know your thougths about my questions above : )

        Regards,
        Adriano Crestani

        Show
        Adriano Crestani added a comment - On ReadCommandImpl.setResultDescriptor(List) you are storing the list directly, however I think the list should be copied(also the elements), because the way it is the user can modify the Command's ResultDescriptor List or the ResultDescriptor itself anywhere else on users'code without notify the Command. The ResultDescriptorSorter.sortList(List) is returning 3 different ways: 1. When the List parameter is null, the method returns null. I think it's ok 2. When the List is empty, the method returns the same List object 3. Otherwise, it's returning a new List object containing the ResultDescriptors sorted by the index I think on 2 the List returned should be a new empty List and not the parameter List. OR the 3 could return the same List but with the objects sorted. I think it's important to define the method logic, if it will always return the same List passed on parameter or return a new List. Why ResultDescriptor cannot be made Comparable? Is there any reason not to name the method List Command.set/getResultDescriptor to set/getResultDescriptors? Because it returns a List of ResultDescriptors, and not a single one. Maybe we could use some pattern here, as addResultDescriptor(ResultDescriptor), removeResultDescriptor(ResultDescriptor), and getResultDescriptor(int). Why the ResultDescriptor Lists paremeters and returns cannot be defined as List<ResultDescriptor>? I applied the patch on my trunk and it passed on all mvn tests, I think it`s ok. I just want to know your thougths about my questions above : ) Regards, Adriano Crestani
        Hide
        Amita Vadhavkar added a comment -

        Hi , am checking for your comments for 1464 and 1465, will reply/give improved patch for both in a day.
        Please do not commit any of these.
        Regards
        Amita

        Show
        Amita Vadhavkar added a comment - Hi , am checking for your comments for 1464 and 1465, will reply/give improved patch for both in a day. Please do not commit any of these. Regards Amita
        Hide
        ant elder added a comment -

        I wonder if this is case where "be liberal in what we accept, conservative in what we do" might apply? If a patch has been tried and it looks ok and all tests pass but there's just a some questions on approach i think it can be good to just apply it and then incrementally develop things further once its in.

        Show
        ant elder added a comment - I wonder if this is case where "be liberal in what we accept, conservative in what we do" might apply? If a patch has been tried and it looks ok and all tests pass but there's just a some questions on approach i think it can be good to just apply it and then incrementally develop things further once its in.
        Hide
        Adriano Crestani added a comment -

        I agree with you ant, I applied the patches from JIRA 1465 and 1464 separately and both passed on maven tests. Then I applied both on the same trunk, but the tests failed, so I couldn't commit

        These errors are commented on https://issues.apache.org/jira/browse/TUSCANY-1464

        Regards,
        Adraino Crestani

        Show
        Adriano Crestani added a comment - I agree with you ant, I applied the patches from JIRA 1465 and 1464 separately and both passed on maven tests. Then I applied both on the same trunk, but the tests failed, so I couldn't commit These errors are commented on https://issues.apache.org/jira/browse/TUSCANY-1464 Regards, Adraino Crestani
        Hide
        Amita Vadhavkar added a comment -

        1) The way ResultDescriptor can be replaced on a Command is thru set method. And in this, I am
        replacing the resultSetShape for the Command (way to notify) as well. In forming ResultSetShape,
        the list gets sorted. So the sorting will happen once for each new call to setResultDescriptor().
        So, where will be the problem?

        2) Done. (always return a new ArrayList from sorter)

        3) ResultDescriptor is generated code, so directly can not add comparable interface to it.
        Another way will be extend ResultDescriptor implement Comparable in this extension and
        use this as wrapper over ResultDescriptor internal to DAS. But as the present requirement
        is just a simple sorting of index, avoided it and used directly ResultDescriptor. Any suggestion?

        4) renamed to set/getResultDescriptors(). Also added addResultDescriptor(ResultDescriptor), removeResultDescriptor(ResultDescriptor), and getResultDescriptor(int) and used in a test case.
        Please see below, if it looks OK, so I will submit patch based on this.
        /**

        • Add/replace based on index (>=0)embedded in resultDescriptor else add at end
        • @param resultDescriptor
          */
          void addResultDescriptor(ResultDescriptor resultDescriptor);

        /**

        • remove ResultDescriptor at given index(>=0) and return same. If not
        • present return null. For -ve index, return null
        • @param index
        • @return
          */
          ResultDescriptor removeResultDescriptor(int index);

        /**

        • Remove resultDescriptor only if matched for index(>=0), name, type, schema
        • name and table name and return same, else return null For -ve index, ignore
        • index and if unique match for rest of the attriutes, remove/return, if multiple
        • matches found, throw RuntimeException
        • @param resultDescriptor
        • @return
          */
          ResultDescriptor removeResultDescriptor(ResultDescriptor resultDescriptor);

        /**

        • Return resultDescriptor if exact match for index(>=0) found
        • else return null;
        • @param index
        • @return
          */
          ResultDescriptor getResultDescriptor(int index);

        5) List<Type> , is JDK5 feature and recently we resolved JIRA-1237 to ensure compatibility with JDK1.4
        so avoided use of JDK5.

        6) The test fail is the test case code problem ..please see..from 1464 last comment -

        {............I took a clean base from trunk and applied 1464 and 1465 to it. The problem was in the test cases of 1465 and I am creating new patch for it. (As 1464 is now catching absence of PK from select, the 2 cases were failing in 1465, as in converter column name should match table (dbms) column name and can not be aby arbitrary names) .............}
        Show
        Amita Vadhavkar added a comment - 1) The way ResultDescriptor can be replaced on a Command is thru set method. And in this, I am replacing the resultSetShape for the Command (way to notify) as well. In forming ResultSetShape, the list gets sorted. So the sorting will happen once for each new call to setResultDescriptor(). So, where will be the problem? 2) Done. (always return a new ArrayList from sorter) 3) ResultDescriptor is generated code, so directly can not add comparable interface to it. Another way will be extend ResultDescriptor implement Comparable in this extension and use this as wrapper over ResultDescriptor internal to DAS. But as the present requirement is just a simple sorting of index, avoided it and used directly ResultDescriptor. Any suggestion? 4) renamed to set/getResultDescriptors(). Also added addResultDescriptor(ResultDescriptor), removeResultDescriptor(ResultDescriptor), and getResultDescriptor(int) and used in a test case. Please see below, if it looks OK, so I will submit patch based on this. /** Add/replace based on index (>=0)embedded in resultDescriptor else add at end @param resultDescriptor */ void addResultDescriptor(ResultDescriptor resultDescriptor); /** remove ResultDescriptor at given index(>=0) and return same. If not present return null. For -ve index, return null @param index @return */ ResultDescriptor removeResultDescriptor(int index); /** Remove resultDescriptor only if matched for index(>=0), name, type, schema name and table name and return same, else return null For -ve index, ignore index and if unique match for rest of the attriutes, remove/return, if multiple matches found, throw RuntimeException @param resultDescriptor @return */ ResultDescriptor removeResultDescriptor(ResultDescriptor resultDescriptor); /** Return resultDescriptor if exact match for index(>=0) found else return null; @param index @return */ ResultDescriptor getResultDescriptor(int index); 5) List<Type> , is JDK5 feature and recently we resolved JIRA-1237 to ensure compatibility with JDK1.4 so avoided use of JDK5. 6) The test fail is the test case code problem ..please see..from 1464 last comment - {............I took a clean base from trunk and applied 1464 and 1465 to it. The problem was in the test cases of 1465 and I am creating new patch for it. (As 1464 is now catching absence of PK from select, the 2 cases were failing in 1465, as in converter column name should match table (dbms) column name and can not be aby arbitrary names) .............}
        Hide
        Adriano Crestani added a comment -

        Hi Amita,

        1)
        Lets exemplify:

        { //user code List userResultDescriptors = new ArrayList(); userResultDescriptors.add(resultDescriptor1); userResultDescriptors.add(resultDescriptor2); readCommand.setResultDescriptor(userResultDescriptors); userResultDescriptors.add(resultDescriptor3); // [3] }

        //user code

        On ReadCommandImpl:

        public void setResultDescriptor(List resultDescriptor){
        //for null/0 list or -1 columnIndex, throw RuntimeException
        if(resultDescriptor == null || resultDescriptor.size()==0)

        { throw new RuntimeException("Can not accept null or empty ResultDescriptor"); }

        for(int i=0; i<resultDescriptor.size(); i++){
        if( ((ResultDescriptor)resultDescriptor.get).getColumnIndex() <= -1 )

        { throw new RuntimeException("Need >=0 columnIndex sequencing in ResultDescriptor"); }

        }

        this.resultDescriptor = resultDescriptor; // [1]
        this.resultSetShape = new ResultSetShape(resultDescriptor, configWrapper.getConfig()); // [2]
        }

        [1] here the user ResultDescriptor List object is being set as Command ResultDescriptor List, so when the user add the resultDescriptor3 on [3] the Command ResultDescriptor List will also be modified.

        [2] here the user ResultDescriptor List is being copied on ResultSetShape constructor when ResultDescriptorSorter.sortList() method is invoked, as defined on item 2). So, as it is a new copy, when user add the resultDescriptor3 on the list it will not be updated on ResultSetShape, that will continue storing info only about the resultDescriptor1 and 2. So, the readCommandImpl.resultDescriptors and readCommandImpl.resultSetShape will have different information.

        I suggest to copy the user ResultDescriptor List and also its ResultDescriptors, once the user can externaly alter the ResultDescriptor object using its setters and it will not be updated on ResultSetShape.

        It's just all about the ResultSetShape not being updated when ResultDescriptors info are modified externaly. If you want to, you may find a way to keep the ResultSetShape updated, but it all depends on the logic you want to define: if the user can modify the ResultDescriptor externaly after have set the ResultDescriptor on a Command or not.

        4) Sorry for bothering you with names and patterns issues, but as these methods will be used by the user, I think we should be cautiously when defining them. Agreed?

        Show
        Adriano Crestani added a comment - Hi Amita, 1) Lets exemplify: { //user code List userResultDescriptors = new ArrayList(); userResultDescriptors.add(resultDescriptor1); userResultDescriptors.add(resultDescriptor2); readCommand.setResultDescriptor(userResultDescriptors); userResultDescriptors.add(resultDescriptor3); // [3] } //user code On ReadCommandImpl: public void setResultDescriptor(List resultDescriptor){ //for null/0 list or -1 columnIndex, throw RuntimeException if(resultDescriptor == null || resultDescriptor.size()==0) { throw new RuntimeException("Can not accept null or empty ResultDescriptor"); } for(int i=0; i<resultDescriptor.size(); i++){ if( ((ResultDescriptor)resultDescriptor.get ).getColumnIndex() <= -1 ) { throw new RuntimeException("Need >=0 columnIndex sequencing in ResultDescriptor"); } } this.resultDescriptor = resultDescriptor; // [1] this.resultSetShape = new ResultSetShape(resultDescriptor, configWrapper.getConfig()); // [2] } [1] here the user ResultDescriptor List object is being set as Command ResultDescriptor List, so when the user add the resultDescriptor3 on [3] the Command ResultDescriptor List will also be modified. [2] here the user ResultDescriptor List is being copied on ResultSetShape constructor when ResultDescriptorSorter.sortList() method is invoked, as defined on item 2). So, as it is a new copy, when user add the resultDescriptor3 on the list it will not be updated on ResultSetShape, that will continue storing info only about the resultDescriptor1 and 2. So, the readCommandImpl.resultDescriptors and readCommandImpl.resultSetShape will have different information. I suggest to copy the user ResultDescriptor List and also its ResultDescriptors, once the user can externaly alter the ResultDescriptor object using its setters and it will not be updated on ResultSetShape. It's just all about the ResultSetShape not being updated when ResultDescriptors info are modified externaly. If you want to, you may find a way to keep the ResultSetShape updated, but it all depends on the logic you want to define: if the user can modify the ResultDescriptor externaly after have set the ResultDescriptor on a Command or not. 4) Sorry for bothering you with names and patterns issues, but as these methods will be used by the user, I think we should be cautiously when defining them. Agreed?
        Hide
        Amita Vadhavkar added a comment -

        Worked on all comments.

        Removed sorter class from util as anyways, it is too specific for ResultDescriptor
        and ResultDescriptor have meaning only for ReadCommandImpl, used it directly
        in it, so that all copying logic avoided.

        Deep copy provided for ArrayList and elements of it in ReadCommandImpl.

        A new test case to check add/remove methods.

        Naming conventions are good to follow,

        Regards,
        Amita

        Show
        Amita Vadhavkar added a comment - Worked on all comments. Removed sorter class from util as anyways, it is too specific for ResultDescriptor and ResultDescriptor have meaning only for ReadCommandImpl, used it directly in it, so that all copying logic avoided. Deep copy provided for ArrayList and elements of it in ReadCommandImpl. A new test case to check add/remove methods. Naming conventions are good to follow, Regards, Amita
        Hide
        ant elder added a comment -

        Closing because this has been in RESOLVED state for over one year, if it turns out to not be fixed please reopen.

        Show
        ant elder added a comment - Closing because this has been in RESOLVED state for over one year, if it turns out to not be fixed please reopen.

          People

          • Assignee:
            Amita Vadhavkar
            Reporter:
            Amita Vadhavkar
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development