Cayenne
  1. Cayenne
  2. CAY-811

Meaningful identity columns: user provided PK values are ignored

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.2 branch, 2.0 branch, 3.0
    • Fix Version/s: Short term future
    • Component/s: Core Library
    • Labels:
      None

      Description

      I found it when testing on 3.0, although I suspect this is a problem on 2.0 and 1.2 as well. When a meaningful PK is set by the user and is also mapped as a DB-generated PK, Cayenne incorrectly overrides user value. There are two places where this must be fixed:

      1. DataDomainInsertBucket (I will commit the fix shortly)
      2. InsertBatchQueryBuilder (this is a bit more hairy - as the batch syntax will be affected depending on whether user provided a value or not)

        Activity

        Hide
        Andrus Adamchik added a comment -

        BTW, we already had a unit test for that: org.apache.cayenne.access.IdentityColumnsTest, although the error it throws is a bit obscure - ClassCastException caused by generated key to be of a different type

        Show
        Andrus Adamchik added a comment - BTW, we already had a unit test for that: org.apache.cayenne.access.IdentityColumnsTest, although the error it throws is a bit obscure - ClassCastException caused by generated key to be of a different type
        Hide
        Andrus Adamchik added a comment -

        Will remove the unit test until this is fixed. Here is the test source code to reinstate it later:

        public void testMeaningfulPK() throws Exception

        { DataContext context = createDataContext(); MeaningfulGeneratedColumnTestEntity o = (MeaningfulGeneratedColumnTestEntity) context .newObject(MeaningfulGeneratedColumnTestEntity.class); o.setName("o1"); o.setGeneratedColumn(new Integer(33333)); context.commitChanges(); assertNotNull(o.getGeneratedColumn()); assertEquals(new Integer(33333), o.getObjectId().getIdSnapshot().get( MeaningfulGeneratedColumnTestEntity.GENERATED_COLUMN_PK_COLUMN)); }
        Show
        Andrus Adamchik added a comment - Will remove the unit test until this is fixed. Here is the test source code to reinstate it later: public void testMeaningfulPK() throws Exception { DataContext context = createDataContext(); MeaningfulGeneratedColumnTestEntity o = (MeaningfulGeneratedColumnTestEntity) context .newObject(MeaningfulGeneratedColumnTestEntity.class); o.setName("o1"); o.setGeneratedColumn(new Integer(33333)); context.commitChanges(); assertNotNull(o.getGeneratedColumn()); assertEquals(new Integer(33333), o.getObjectId().getIdSnapshot().get( MeaningfulGeneratedColumnTestEntity.GENERATED_COLUMN_PK_COLUMN)); }
        Hide
        Tore Halset added a comment -

        Testing on Derby (using batch insert) only the value 0 (new Integer(0)) will lead to the wrong value.

        This did work perfectly for me under 2.0.2, but not after upgrading to 3.0M1

        Show
        Tore Halset added a comment - Testing on Derby (using batch insert) only the value 0 (new Integer(0)) will lead to the wrong value. This did work perfectly for me under 2.0.2, but not after upgrading to 3.0M1
        Hide
        Tore Halset added a comment -

        I do not fully understand

        "2. InsertBatchQueryBuilder (this is a bit more hairy - as the batch syntax will be affected depending on whether user provided a value or not)"

        If there is an insert on a table with meaningful primary key (primary key mapped to java side), then null as the primary key from the java side is not allowed? Or what?

        Show
        Tore Halset added a comment - I do not fully understand "2. InsertBatchQueryBuilder (this is a bit more hairy - as the batch syntax will be affected depending on whether user provided a value or not)" If there is an insert on a table with meaningful primary key (primary key mapped to java side), then null as the primary key from the java side is not allowed? Or what?
        Hide
        Andrus Adamchik added a comment -

        > "2. InsertBatchQueryBuilder (this is a bit more hairy - as the batch syntax will be affected depending on whether user provided a value or not)"

        > If there is an insert on a table with meaningful primary key (primary key mapped to java side), then null as the primary key from the java side is not allowed? Or what?

        PreparedStatement syntax has to be different (if we want to rely on DB autoincrement, we must omit a PK parameter from PreparedStatement);

        • User supplied PK:

        INSERT INTO ARTIST (ID, NAME) VALUES (?, ?)

        • DB generated PK:

        INSERT INTO ARTIST (NAME) VALUES

        Show
        Andrus Adamchik added a comment - > "2. InsertBatchQueryBuilder (this is a bit more hairy - as the batch syntax will be affected depending on whether user provided a value or not)" > If there is an insert on a table with meaningful primary key (primary key mapped to java side), then null as the primary key from the java side is not allowed? Or what? PreparedStatement syntax has to be different (if we want to rely on DB autoincrement, we must omit a PK parameter from PreparedStatement); User supplied PK: INSERT INTO ARTIST (ID, NAME) VALUES (?, ?) DB generated PK: INSERT INTO ARTIST (NAME) VALUES
        Hide
        Tore Halset added a comment -

        Agree on that one, but my issue is with a meaningful primary key that has PK Strategy "Default", not "Database-Generated". The column is included correctly in the insert statement, but the value is not what the user inserted, but a 200-type value from cayenne.

        Will have to dig some more...

        Show
        Tore Halset added a comment - Agree on that one, but my issue is with a meaningful primary key that has PK Strategy "Default", not "Database-Generated". The column is included correctly in the insert statement, but the value is not what the user inserted, but a 200-type value from cayenne. Will have to dig some more...
        Hide
        Tore Halset added a comment -

        This small patch fixes my issue (int meaningful primary key not marked as generated). Is it the same as your 1) in the description above? I would love to commit it, but I do not know why the special handling of a Number with the intValue 0 was added?

        Index: framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/DataDomainInsertBucket.java
        ===================================================================
        — framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/DataDomainInsertBucket.java (revision 562071)
        +++ framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/DataDomainInsertBucket.java (working copy)
        @@ -144,11 +144,8 @@
        .readPropertyDirectly(object);

        if (value != null) {

        • // treat numeric zero values as nulls requiring generation
        • if (!(value instanceof Number && ((Number) value).intValue() == 0)) { - idMap.put(dbAttrName, value); - continue; - }

          + idMap.put(dbAttrName, value);
          + continue;
          }
          }

        Show
        Tore Halset added a comment - This small patch fixes my issue (int meaningful primary key not marked as generated). Is it the same as your 1) in the description above? I would love to commit it, but I do not know why the special handling of a Number with the intValue 0 was added? Index: framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/DataDomainInsertBucket.java =================================================================== — framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/DataDomainInsertBucket.java (revision 562071) +++ framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/DataDomainInsertBucket.java (working copy) @@ -144,11 +144,8 @@ .readPropertyDirectly(object); if (value != null) { // treat numeric zero values as nulls requiring generation if (!(value instanceof Number && ((Number) value).intValue() == 0)) { - idMap.put(dbAttrName, value); - continue; - } + idMap.put(dbAttrName, value); + continue; } }

          People

          • Assignee:
            Andrus Adamchik
            Reporter:
            Andrus Adamchik
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development