Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3-RC2
    • Fix Version/s: 4.0-beta1
    • Component/s: Runtime
    • Labels:
      None
    • Environment:
      Linux

      Description

      I've a "complicated" JOIN statement that features some custom column like "COUNT(a) AS ca".

      Torque runtime fails on the

      Criteria::addAscendingOrderByColumn("ca")

      because it expects a full qualified table.columnName which is not the case for the example "ca" column.

      The problem can be fixed by adapting the following functions (3.3-RC2):
      src/java/org/apache/torque/util/SQLBuilder.java
      removeSQLFunction(final String name) : name must not necessarily contain '.' or '*" thus replacing the first thrown exception with a "return name" fixes that issue
      processOrderBy(...) : in the for loop, strippedColumnName must not contain '.' hence replacing the first thrown exception with "orderByColumn.add(orderByColumn); break;" fixes the issue

      This are quick fixes and I guess you want to reconsider whether you want to implement it that way. After these two changes, it was working correctly though.

        Activity

        Thomas Fox made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Thomas Fox made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 4.0 [ 12312102 ]
        Resolution Fixed [ 1 ]
        Hide
        Thomas Fox added a comment -

        fixed, see org.apache.torque.sql.SqlBuilderTest.testOrderByWithAsColumn()

        Show
        Thomas Fox added a comment - fixed, see org.apache.torque.sql.SqlBuilderTest.testOrderByWithAsColumn()
        Hide
        Thomas Fox added a comment -

        I'm afraid you have to check out from SVN and use maven to build the project.
        Did this work for you ?

        Show
        Thomas Fox added a comment - I'm afraid you have to check out from SVN and use maven to build the project. Did this work for you ?
        Hide
        Stefan Birrer added a comment -

        Thanks Thomas,

        How do I best get to the jar with the patches?

        Do I have to check out from CVS? Or is there a simpler way.

        thanks
        stefan


        CSA, Metis Enterprise Technologies LLC
        http://www.neokastblog.com

        Show
        Stefan Birrer added a comment - Thanks Thomas, How do I best get to the jar with the patches? Do I have to check out from CVS? Or is there a simpler way. thanks stefan – CSA, Metis Enterprise Technologies LLC http://www.neokastblog.com
        Thomas Fox made changes -
        Field Original Value New Value
        Assignee Thomas Fischer [ tfischer ]
        Hide
        Thomas Fox added a comment -

        I did not understand that the change in processOrderBy was the main point. I was under the impression that you needed both changes for your query to work.

        I have made the requested change to processOrderBy. Could you please check whether this works as you expected ?
        The change in removeSQLFunction will not be made, instead the problem will be resolved by improved column definitions after 3.3.

        Show
        Thomas Fox added a comment - I did not understand that the change in processOrderBy was the main point. I was under the impression that you needed both changes for your query to work. I have made the requested change to processOrderBy. Could you please check whether this works as you expected ? The change in removeSQLFunction will not be made, instead the problem will be resolved by improved column definitions after 3.3.
        Hide
        Stefan Birrer added a comment -

        I see your point, so you basically mean this:

        F(a) AS a.ca

        should return a.ca, right?

        So, I totally agree on your point about the removeSQLFunction() and it's
        contract. You don't want to change that.

        However, you may want to change processOrderBy() to fulfill it's contract of
        ordering by a given column name. Currently it only supports columns that map
        to an table column and don't support aggregation columns as in (COUNT(a) as
        ca) which is at least supported in mysql.

        To problem with this bug is that you can't work around b/c of the exception
        being thrown at runtime. So the only way to solve is to modify the
        processOrderBy(). The reason I touched removeSQLFunction is just to avoid it
        to throw, conceptually the following lines have the same purposes (and this
        is bad style I know but just to illustrate):

        orderByColumn = "ca"; // from the example above

        try

        { strippedColumnName = removeSQLFunction(orderByColumn); }

        catch (TorqueException e)

        { // this is currently a failure scenario while indeed it could be an aggregated column name strippedColumnName = orderByColumn; }


        CSA, Metis Enterprise Technologies LLC
        http://www.neokastblog.com

        Show
        Stefan Birrer added a comment - I see your point, so you basically mean this: F(a) AS a.ca should return a.ca, right? So, I totally agree on your point about the removeSQLFunction() and it's contract. You don't want to change that. However, you may want to change processOrderBy() to fulfill it's contract of ordering by a given column name. Currently it only supports columns that map to an table column and don't support aggregation columns as in (COUNT(a) as ca) which is at least supported in mysql. To problem with this bug is that you can't work around b/c of the exception being thrown at runtime. So the only way to solve is to modify the processOrderBy(). The reason I touched removeSQLFunction is just to avoid it to throw, conceptually the following lines have the same purposes (and this is bad style I know but just to illustrate): orderByColumn = "ca"; // from the example above try { strippedColumnName = removeSQLFunction(orderByColumn); } catch (TorqueException e) { // this is currently a failure scenario while indeed it could be an aggregated column name strippedColumnName = orderByColumn; } – CSA, Metis Enterprise Technologies LLC http://www.neokastblog.com
        Hide
        Thomas Fox added a comment -

        I have realized that, but maybe I have been unclear in my answer. I'll try again.
        The change you propose to removeSQLFunction() violates the contract of removeSQLFunction(), namely to remove the sql function whenever possible. And I see no simple way to comply with the contract if no dot or star is present.

        Show
        Thomas Fox added a comment - I have realized that, but maybe I have been unclear in my answer. I'll try again. The change you propose to removeSQLFunction() violates the contract of removeSQLFunction(), namely to remove the sql function whenever possible. And I see no simple way to comply with the contract if no dot or star is present.
        Hide
        Stefan Birrer added a comment -

        It sounds reasonable to postpone fixing that bug in a later full release.

        However, I think you misinterpret my proposed fixes. In fact, they do not
        change any existing "good" behavior. The only thing I proposed below is to
        replace two exceptions that make a previous failure path a new "good" path
        to account for this use case.

        Let me know if I need to be more verbose in my explanations. I can also
        submit a patched file that shows my changes.

        stefan


        CSA, Metis Enterprise Technologies LLC
        http://www.neokastblog.com

        Show
        Stefan Birrer added a comment - It sounds reasonable to postpone fixing that bug in a later full release. However, I think you misinterpret my proposed fixes. In fact, they do not change any existing "good" behavior. The only thing I proposed below is to replace two exceptions that make a previous failure path a new "good" path to account for this use case. Let me know if I need to be more verbose in my explanations. I can also submit a patched file that shows my changes. stefan – CSA, Metis Enterprise Technologies LLC http://www.neokastblog.com
        Hide
        Thomas Fox added a comment -

        I do not believe that the proposed change to removeSQLFunction() will have the desired results. One still would want to remove any sql functions from the table name even if it does not contain a dot (because if one does not, this would mean that table names may not be found in the table Map, leading to errors in creating the sql statement which are not attributed easily to this).
        The dot is an easy method to determine where the column name sits. If this would not be used, one would have to do a sophisticated parsing of the column string. And I'd like to avoid different algorithms for "table name containing a dot" and "table name not containing a dot".

        I am reluctant to change the algorithm of removeSQLFunction() between release candidates, this may induce subtle errors in user's code. After 3.3 is out, it is planned to replace strings for table names by structured objects, so this kind of problem can be solved more easily then.

        Show
        Thomas Fox added a comment - I do not believe that the proposed change to removeSQLFunction() will have the desired results. One still would want to remove any sql functions from the table name even if it does not contain a dot (because if one does not, this would mean that table names may not be found in the table Map, leading to errors in creating the sql statement which are not attributed easily to this). The dot is an easy method to determine where the column name sits. If this would not be used, one would have to do a sophisticated parsing of the column string. And I'd like to avoid different algorithms for "table name containing a dot" and "table name not containing a dot". I am reluctant to change the algorithm of removeSQLFunction() between release candidates, this may induce subtle errors in user's code. After 3.3 is out, it is planned to replace strings for table names by structured objects, so this kind of problem can be solved more easily then.
        Stefan Birrer created issue -

          People

          • Assignee:
            Thomas Fox
            Reporter:
            Stefan Birrer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development