Torque
  1. Torque
  2. TORQUE-108

Criteria addJoin causes incorrect SQL to be generated when optional schema references are in use (Oracle)

    Details

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

      Description

      I previously wrote about this to torque-user in Oct 2007.

      In a schema definition that includes torque.dsfactory.programs.schema, writing the following

      Criteria crit = new Criteria();
      crit.addJoin(TransactionPeer.ORDER_ID, OrderPeer.ID);
      crit.add(TransactionPeer.ACCT_ID, account.getID());
      List<Order> orders = OrderPeer.doSelect(crit);

      generates

      SELECT <..ORDERS columns..> FROM TRANSACTION, ORDERS, DBSCHEMA.ORDERS, DBSCHEMA.TRANSACTION
      WHERE TRANSACTION.ORDER_ID=ORDERS.ID AND TRANSACTION.ACCT_ID= ?

      Upon examining the Torque code, it appears that SQLBuilder.processJoins does not add the full table names, while SQLBuilder.processCriterions does.
      Shouldn't they both add the full table names?

      1. joinbuilder.patch
        2 kB
        Brendan Miller
      2. JoinBuilderTest.java
        3 kB
        Brendan Miller

        Activity

        Hide
        Brendan Miller added a comment -

        This makes JoinBuilder use the full table name including the optional schema reference name.

        Show
        Brendan Miller added a comment - This makes JoinBuilder use the full table name including the optional schema reference name.
        Hide
        Scott Eade added a comment -

        Would it be possible to provide a test case for this.

        Show
        Scott Eade added a comment - Would it be possible to provide a test case for this.
        Hide
        Brendan Miller added a comment -

        Here are some test cases for JoinBuilder. The first test should run fine on the current source. The second test illustrates the use of a schema name on the database and will fail without my previously-submitted patch. Both tests run fine with the previous patch.

        Show
        Brendan Miller added a comment - Here are some test cases for JoinBuilder. The first test should run fine on the current source. The second test illustrates the use of a schema name on the database and will fail without my previously-submitted patch. Both tests run fine with the previous patch.
        Hide
        Ralph Weires added a comment - - edited

        I have the exact same problem using a MSSQL database with a custom schema (instead of the default 'dbo' schema). The SQL generated for queries without joins works fine, but as soon as I add a join (addJoin), the generated SQL becomes invalid, in the same way as already described by Brendan above.

        I checked the latest SVN trunk and saw that the patch provided by Brendan is not included there yet, so is there a chance this patch might become integrated anytime soon?

        Show
        Ralph Weires added a comment - - edited I have the exact same problem using a MSSQL database with a custom schema (instead of the default 'dbo' schema). The SQL generated for queries without joins works fine, but as soon as I add a join (addJoin), the generated SQL becomes invalid, in the same way as already described by Brendan above. I checked the latest SVN trunk and saw that the patch provided by Brendan is not included there yet, so is there a chance this patch might become integrated anytime soon?
        Hide
        Thomas Fox added a comment -

        fixed. See org.apache.torque.sql.SqlBuilderTest.testOrderByWithDefaultSchema() and org.apache.torque.sql.SqlBuilderTest.testInnerJoinImplicitWithDefaultSchema()

        Show
        Thomas Fox added a comment - fixed. See org.apache.torque.sql.SqlBuilderTest.testOrderByWithDefaultSchema() and org.apache.torque.sql.SqlBuilderTest.testInnerJoinImplicitWithDefaultSchema()
        Hide
        Petr Bodnar added a comment -

        Hi, I have a new finding: I tried the attached patch on 3.3.x and it doesn't work (possibly brakes it) when aliases are used for joining - as aliases don't have any scheme, they are not properly added to the query with this patch... Is this problem fixed in Torque 4.0?

        Show
        Petr Bodnar added a comment - Hi, I have a new finding: I tried the attached patch on 3.3.x and it doesn't work (possibly brakes it) when aliases are used for joining - as aliases don't have any scheme, they are not properly added to the query with this patch... Is this problem fixed in Torque 4.0?
        Hide
        Thomas Fox added a comment -

        This problem is fixed in 4.0. I am not sure whether the attached patch was used for the fix, so no idea whether it is working to patch 3.3 or not.

        Show
        Thomas Fox added a comment - This problem is fixed in 4.0. I am not sure whether the attached patch was used for the fix, so no idea whether it is working to patch 3.3 or not.
        Hide
        Petr Bodnar added a comment -

        Thanks, confirmed for 4.x trunk, the problematic scenario is not quite covered by a unit test, so I needed to write my own one...

        The second thing is this patch doesn't really affect the error with aliases, i. e. "<table> <schema>.<alias>" is wrongly added to the from clause instead of "<schema>.<table> <alias>" with or without the patch, in 3.3.

        BTW It's a pity that one needs to wrap every single column or expression into ColumnImpl in 4.0, but that's for quite another ticket...

        Show
        Petr Bodnar added a comment - Thanks, confirmed for 4.x trunk, the problematic scenario is not quite covered by a unit test, so I needed to write my own one... The second thing is this patch doesn't really affect the error with aliases, i. e. "<table> <schema>.<alias>" is wrongly added to the from clause instead of "<schema>.<table> <alias>" with or without the patch, in 3.3 . BTW It's a pity that one needs to wrap every single column or expression into ColumnImpl in 4.0, but that's for quite another ticket...
        Hide
        Thomas Fox added a comment -

        If you want, can you provide your test so it can be added to the torte test suite ? Please reopen the ticket if you do so.
        As for the ColumnImpl, the reason why this was introdiced is that if you do criteria.add("test","value") there is no way of telling whteher "test" and "value" are table columns or verbatim values. ColumnImpl provides a clean distiction: all ColumnImpl objects are columns, and all String fields are verbatim values.

        Show
        Thomas Fox added a comment - If you want, can you provide your test so it can be added to the torte test suite ? Please reopen the ticket if you do so. As for the ColumnImpl, the reason why this was introdiced is that if you do criteria.add("test","value") there is no way of telling whteher "test" and "value" are table columns or verbatim values. ColumnImpl provides a clean distiction: all ColumnImpl objects are columns, and all String fields are verbatim values.
        Hide
        Petr Bodnar added a comment -

        Hi, when looking at the tests again, I think that the existing testInnerJoinImplicitWithAliasAndDefaultSchema is already sufficient - the only real difference is that I tested with outer join which is not that important difference from the Torque's architecture perspective, I guess.

        Ad ColumnImpl - I can see why this was introduced, but I still don't think, from the perspective of an API user, it was really necessary to remove the possibility of putting just String (and internally calling new ColumnImpl(sqlExpression)) where it logically means just a column name in most of the cases (compare with e. g. Hibernate's criteria API simplicity...).

        Show
        Petr Bodnar added a comment - Hi, when looking at the tests again, I think that the existing testInnerJoinImplicitWithAliasAndDefaultSchema is already sufficient - the only real difference is that I tested with outer join which is not that important difference from the Torque's architecture perspective, I guess. Ad ColumnImpl - I can see why this was introduced, but I still don't think, from the perspective of an API user, it was really necessary to remove the possibility of putting just String (and internally calling new ColumnImpl(sqlExpression) ) where it logically means just a column name in most of the cases (compare with e. g. Hibernate's criteria API simplicity...).

          People

          • Assignee:
            Thomas Fox
            Reporter:
            Brendan Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development