Torque
  1. Torque
  2. TORQUE-113

doDelete with invalid column should throw exception, not delete all rows

    Details

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

      Description

      The following (incorrect) client code should cause an exception to be thrown. Instead of an exception, all rows in table T1 are deleted.

      T1Peer.doDelete(new Criteria().add(T2Peer.COL, 2));
      

      This code appeared in a project I am working on. The intent was to delete rows from table T2.

      Before the fix for TORQUE-93, this code had the intended effect becaue T1Peer.doDelete(criteria) just passed the criteria object to BasePeer.doDelete(criteria). Since the only reference BasePeer had to a table was table T2 in the criteria, only rows in table T2 were deleted.

      Now that T1Peer.doDelete(criteria) calls BasePeer.doDelete(criteria, TABLE_NAME) instead, the test if (crit.containsKey(key)) in BasePeer.processTables() always fails. This leads to an empty where clause, causing all rows in table T1 to be deleted.

      Expect: All Criterion objects in a Criteria should be used in the final where clause. If not, an exception should be thrown.

        Activity

        Hide
        Thomas Fox added a comment -

        The column will be added correctly into the SQL query but the resulting sql query is incorrect and thus the database will throw an error.

        Show
        Thomas Fox added a comment - The column will be added correctly into the SQL query but the resulting sql query is incorrect and thus the database will throw an error.
        Hide
        Thomas Fox added a comment -

        I do not see a good reason for normalizing column names. Taking the exact case from the schema is what one would expect; it offers full flexibility; and it provides consistent case handling over column names, table names and database names.

        Show
        Thomas Fox added a comment - I do not see a good reason for normalizing column names. Taking the exact case from the schema is what one would expect; it offers full flexibility; and it provides consistent case handling over column names, table names and database names.
        Hide
        CG Monroe added a comment -

        I think part of the underlying problems people are seeing might be due to the Torque-44. This was released in 3.3-RC1 in Nov 2006. It basically causes the DB id strings (tables, names, etc), to use the exact same case as is defined in the XML.

        Good news is that there is a new generator option, torque.deprecated.uppercasePeer which can be set to true to revert to the original behaviour.

        Doesn't fix this problem, but might be a quick way around it.

        Carl... you should take a look at the Map objects generated with the record objects. All the information defined in XML is now available there. Using these would be a much more generic way to find your primary keys.

        As to case insensitivity, yes there are a lot of issues around this. But there may also be some low hanging areas we could improve on. One area I've thought about doing this is to "normalize" the key names used in the Map objects. E.g., change the add and find methods to convert the keys to all one case. This is in line with the SQL standard and would only effect non-standard case sensitive situation where someone has two tables or columns with the same name but with different case. (Which, IMHO, is REALLY bad schema design...)

        Another place is the getByName methods in the record objects. It wouldn't be hard to do some normalization here as well.

        Show
        CG Monroe added a comment - I think part of the underlying problems people are seeing might be due to the Torque-44. This was released in 3.3-RC1 in Nov 2006. It basically causes the DB id strings (tables, names, etc), to use the exact same case as is defined in the XML. Good news is that there is a new generator option, torque.deprecated.uppercasePeer which can be set to true to revert to the original behaviour. Doesn't fix this problem, but might be a quick way around it. Carl... you should take a look at the Map objects generated with the record objects. All the information defined in XML is now available there. Using these would be a much more generic way to find your primary keys. As to case insensitivity, yes there are a lot of issues around this. But there may also be some low hanging areas we could improve on. One area I've thought about doing this is to "normalize" the key names used in the Map objects. E.g., change the add and find methods to convert the keys to all one case. This is in line with the SQL standard and would only effect non-standard case sensitive situation where someone has two tables or columns with the same name but with different case. (Which, IMHO, is REALLY bad schema design...) Another place is the getByName methods in the record objects. It wouldn't be hard to do some normalization here as well.
        Hide
        Carl Manaster added a comment -

        Thanks, Brendan; I will look into that.

        Show
        Carl Manaster added a comment - Thanks, Brendan; I will look into that.
        Hide
        Brendan Miller added a comment -

        You might have a look at TableMaps. They provide, for each table, a object view of the schema including table name, column names, Java names for each, which is primary key, etc.

        Show
        Brendan Miller added a comment - You might have a look at TableMaps. They provide, for each table, a object view of the schema including table name, column names, Java names for each, which is primary key, etc.
        Hide
        Carl Manaster added a comment -

        I should have mentioned that the code that was generating the wrong-case string constants was doing so generically, with a mix of introspection and "manual" string manipulation - building "table.TABLE_ID" from "table", basically. I'm not sure why it was doing the case conversion, but it was and it used to work. I don't think I know of a way to get the PK from a table via introspection; for more direct (non-generic) queries we of course use the Peer class constants.

        ... OK, saying I don't know how made me think, and of course it's pretty simple:

        /**

        • @param table
        • @return the name of the primary key field
        • @throws Exception
          */
          public static String getIdField(String table) throws Exception { String idFieldName = table.toUpperCase() + "_ID"; Class<?> peer = getPeerClass(table); Field field = peer.getField(idFieldName); return (String) field.get(null); }

        Which perhaps suggests where that whole toUpperCase() business came from in the first place. In case anyone missed it, this function of course relies on the primary key being named table.table_id, but that's a common enough convention.

        Show
        Carl Manaster added a comment - I should have mentioned that the code that was generating the wrong-case string constants was doing so generically, with a mix of introspection and "manual" string manipulation - building "table.TABLE_ID" from "table", basically. I'm not sure why it was doing the case conversion, but it was and it used to work. I don't think I know of a way to get the PK from a table via introspection; for more direct (non-generic) queries we of course use the Peer class constants. ... OK, saying I don't know how made me think, and of course it's pretty simple: /** @param table @return the name of the primary key field @throws Exception */ public static String getIdField(String table) throws Exception { String idFieldName = table.toUpperCase() + "_ID"; Class<?> peer = getPeerClass(table); Field field = peer.getField(idFieldName); return (String) field.get(null); } Which perhaps suggests where that whole toUpperCase() business came from in the first place. In case anyone missed it, this function of course relies on the primary key being named table.table_id, but that's a common enough convention.
        Hide
        Brendan Miller added a comment -

        For this reason, we always use the Peer class constants. It is a good way to a) only have one string literal defined in the codebase for each column and b) a good way to let the compiler tell you when someone changed a column name in the schema, but didn't update code using the column names.

        Nonetheless, it might be wise to add a note in some documentation somewhere about the behavior. I do think that Criteria validation, if already being performed by ProcessTables should throw an Exception and refuse to delete records over deleting all records.

        Show
        Brendan Miller added a comment - For this reason, we always use the Peer class constants. It is a good way to a) only have one string literal defined in the codebase for each column and b) a good way to let the compiler tell you when someone changed a column name in the schema, but didn't update code using the column names. Nonetheless, it might be wise to add a note in some documentation somewhere about the behavior. I do think that Criteria validation, if already being performed by ProcessTables should throw an Exception and refuse to delete records over deleting all records.
        Hide
        Thomas Fox added a comment -

        Regarding case insensitivity: It is a lot of work to make Torque aware whether the underlying db is case sensitive or not. One example is mysql: On Windows it is case insensitive, on linux Database names and table names are case sensitve, but column names are not.

        The thing which has changed is that in earlier Torque versions, the column name constant used in the Peer class was case-converted, now it uses the exact case from the schema. So if the constants from the Peer class are used (which is the recommended way) , everything will work, but if own column name constants are created, things may not work, depending on the case of the column names in the schema..

        Show
        Thomas Fox added a comment - Regarding case insensitivity: It is a lot of work to make Torque aware whether the underlying db is case sensitive or not. One example is mysql: On Windows it is case insensitive, on linux Database names and table names are case sensitve, but column names are not. The thing which has changed is that in earlier Torque versions, the column name constant used in the Peer class was case-converted, now it uses the exact case from the schema. So if the constants from the Peer class are used (which is the recommended way) , everything will work, but if own column name constants are created, things may not work, depending on the case of the column names in the schema..
        Hide
        Carl Manaster added a comment -

        I'll chime in with a "me, too" on this - today I fixed a bug in code that had been working previously - deleting just one record - but that, with 3.3, started deleting all records, because the field name's case was wrong. Since our database is case-insensitive, or since our configuration is case-insensitive, the SQL would be fine, regardless of case. But now that BasePeer.ProcessTables() is asking the criteria to look up by exact string key, it fails if the case is wrong and comes up with an empty WHERE clause, deleting all records. I agree that it would be better to throw an exception in this case than to delete all records, although ideally (to my mind), torque would honor the case sensitivity of the underlying database.

        I should say: I don't know how it worked before 3.3; I'm just assuming that something different was going on in ProcessTables(), because that's clearly where it is building the WHERE clause and clearly where it is failing. But it's possible that the criteria was converting all its keys to uppercase or something - I don't know, and don't want to send the maintainer on a wild goose chase. It's just where the problem appears to be, to me.

        Show
        Carl Manaster added a comment - I'll chime in with a "me, too" on this - today I fixed a bug in code that had been working previously - deleting just one record - but that, with 3.3, started deleting all records, because the field name's case was wrong. Since our database is case-insensitive, or since our configuration is case-insensitive, the SQL would be fine, regardless of case. But now that BasePeer.ProcessTables() is asking the criteria to look up by exact string key, it fails if the case is wrong and comes up with an empty WHERE clause, deleting all records. I agree that it would be better to throw an exception in this case than to delete all records, although ideally (to my mind), torque would honor the case sensitivity of the underlying database. I should say: I don't know how it worked before 3.3; I'm just assuming that something different was going on in ProcessTables(), because that's clearly where it is building the WHERE clause and clearly where it is failing. But it's possible that the criteria was converting all its keys to uppercase or something - I don't know, and don't want to send the maintainer on a wild goose chase. It's just where the problem appears to be, to me.
        Hide
        Julian Zinn added a comment -

        See also TORQUE-112.

        Show
        Julian Zinn added a comment - See also TORQUE-112 .

          People

          • Assignee:
            Thomas Fox
            Reporter:
            Julian Zinn
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development