OpenJPA
  1. OpenJPA
  2. OPENJPA-245

Attach NEW and auto-increment identity

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.6, 0.9.7
    • Fix Version/s: 1.0.3, 1.1.0
    • Component/s: jpa
    • Labels:
      None
    • Environment:
      jdk1.5.0_11, Win XP, Fedora Core 6, Postgres 8.1 (on Fedora)

      Description

      According to documentation (1.2 Attach Behavior), when an entity instance is NEW (never detached):

      • If neither of the above cases apply, OpenJPA will check to see if an instance with the same primary key values exists in the database. If so, the object is considered detached. Otherwise, it is considered new.

      This doesn't work for me - a new record in database is created on commit instead of updating the existing one. The "regular" case - detach/modify/attach works fine - the existing record is updated.

      It is very easy to reproduce - just create a new instance of an entity, assign an already existing primary key, call em.merge() and commit. A new record will be created in database, with new, auto-generated primary key.

      I stumbled on this trying to implement a web service that uses OpenJPA-based backend. When servicing an "update" request, the web service instantiates a NEW object (by performing XML de-serialization) and calls em.merge to update the entity. A new record gets created instead of updating an existing one.

      ------------ Entity class (START) ------------------------------

      package exaple;

      public class Consumer implements java.io.Serializable {

      private long id;

      public long getId()

      { return this.id; }

      public void setId(long id)

      { this.id = id; }

      private java.lang.String firstName;

      public java.lang.String getFirstName()

      { return this.firstName; }

      public void setFirstName(java.lang.String firstName)

      { this.firstName = firstName; }

      private java.lang.String lastName;

      public java.lang.String getLastName()

      { return this.lastName; }

      public void setLastName(java.lang.String lastName)

      { this.lastName = lastName; }

      ------------ Entity class (END) ------------------------------
      ------------ persistence.xml (START) ------------------------------
      <?xml version="1.0" encoding="UTF-8"?>
      <persistence xmlns="http://java.sun.com/xml/ns/persistence" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="1.0">

      <persistence-unit name="example" transaction-type="RESOURCE_LOCAL">

      <provider>org.apache.openjpa.persistence.PersistenceProviderImpl</provider>

      <!-- We must enumerate each entity in the persistence unit -->
      <class>example.Consumer</class>

      <properties>

      <property name="openjpa.jdbc.DBDictionary" value="postgres"/>
      <property name="openjpa.ConnectionDriverName" value="org.postgresql.Driver"/>
      <property name="openjpa.ConnectionUserName" value="app_user"/>
      <property name="openjpa.ConnectionPassword" value="app_user"/>
      <property name="openjpa.ConnectionURL" value="jdbc:postgresql://localhost/alikic"/>
      <property name="openjpa.Log" value="DefaultLevel=WARN,SQL=TRACE"/>

      </properties>
      </persistence-unit>

      </persistence>
      ------------ persistence.xml (END) ------------------------------
      ------------ orm.xml (START) ------------------------------
      <entity-mappings xmlns="http://java.sun.com/xml/ns/persistence/orm"
      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xsi:schemaLocation="http://java.sun.com/xml/ns/persistence/orm orm_1_0.xsd"
      version="1.0">
      <entity class="example.Consumer">
      <attributes>
      <id name="id">
      <generated-value strategy="IDENTITY"/>
      </id>
      <basic name="firstName">
      <column name="first_name"/>
      </basic>
      <basic name="lastName">
      <column name="last_name"/>
      </basic>
      </attributes>
      </entity>
      </entity-mappings>
      ------------ orm.xml (END) ------------------------------

      1. TestMerge.zip
        4 kB
        Pinaki Poddar
      2. OPENJPA-245.patch.txt
        7 kB
        Michael Dick
      3. OPENJPA-245.patch
        4 kB
        Patrick Linskey

        Activity

        Aleksandar Likic created issue -
        Hide
        Pinaki Poddar added a comment -

        merge() to identify that the the merged entity instance is not truely new but a version of a instance that is already persistent, the entity requires a version field.
        If the merged instance was a clone created out of serialization and desrialization, then OpenJPA will maintain the required bits to identify the difference between a truly new instance and an instance that has been detached.

        Show
        Pinaki Poddar added a comment - merge() to identify that the the merged entity instance is not truely new but a version of a instance that is already persistent, the entity requires a version field. If the merged instance was a clone created out of serialization and desrialization, then OpenJPA will maintain the required bits to identify the difference between a truly new instance and an instance that has been detached.
        Hide
        Pinaki Poddar added a comment -

        Attached a test case that demonstates merge() behaviour of entities with or without a version field and when the merged instance is a clone or a copy. A clone is created by serialization+deserialization of the original persistent instance. A copy is created by Java new() operator and then fields values are set equal to the original persistent instance (including the primary key field).

        Thus we have four scenarios:
        1. Unversioned + Copy
        2. Unversioned + Clone
        3. Versioned + Copy
        4. Versioned + Clone

        The first case originated this issue (at least that's what I gathered from the description). The first case behaves 'rationally but non-intutively'. It creates a new database record rather than updating the existing one. The merge() operation does not see a versioned field, treats the argument instance as a new instance, assigns a new identity (ignoring what the application has set).
        If the entity were using application identity, one would have encountered a DuplicateKeyException of some sort, I presume but not tested here.

        The rest three cases update the existing record. In the third case, the user application has to copy the version field for merge() to treat the merged instance 'correctly' i.e. as a update rather than an insert of a new record. This seems to be the solution to the original use cse – add a version field and copy the version field to the merged instance.

        Show
        Pinaki Poddar added a comment - Attached a test case that demonstates merge() behaviour of entities with or without a version field and when the merged instance is a clone or a copy. A clone is created by serialization+deserialization of the original persistent instance. A copy is created by Java new() operator and then fields values are set equal to the original persistent instance (including the primary key field). Thus we have four scenarios: 1. Unversioned + Copy 2. Unversioned + Clone 3. Versioned + Copy 4. Versioned + Clone The first case originated this issue (at least that's what I gathered from the description). The first case behaves 'rationally but non-intutively'. It creates a new database record rather than updating the existing one. The merge() operation does not see a versioned field, treats the argument instance as a new instance, assigns a new identity (ignoring what the application has set). If the entity were using application identity, one would have encountered a DuplicateKeyException of some sort, I presume but not tested here. The rest three cases update the existing record. In the third case, the user application has to copy the version field for merge() to treat the merged instance 'correctly' i.e. as a update rather than an insert of a new record. This seems to be the solution to the original use cse – add a version field and copy the version field to the merged instance.
        Pinaki Poddar made changes -
        Field Original Value New Value
        Attachment TestMerge.zip [ 12358445 ]
        Hide
        Pinaki Poddar added a comment -

        From openjpa documentation (ref: 1.2. Attach Behavior)

        " * If the instance was detached and detached state is enabled, OpenJPA will use the detached state to determine the object's version and primary key values. In addition, this state will tell OpenJPA which fields were loaded at the time of detach, and in turn where to expect changes. Loaded detached fields with null values will set the attached instance's corresponding fields to null.

        • If the instance has a Version field, OpenJPA will consider the object detached if the version field has a non-default value, and new otherwise.
        • If neither of the above cases apply, OpenJPA will check to see if an instance with the same primary key values exists in the database. If so, the object is considered detached. Otherwise, it is considered new."

        The implementaion does not seem to adhere to the last statement above. This is observed by the original scenario described in this issue as well as the attached testcase testMergeUnversionedNewObjectCreatesNewRecord().

        AttachStrategy implementaion is responsible for merge(). For a newly created entity instance even if it carries id of a persistent record – the attach strategy selected (by AttachManager.getStrategy() method) is VersionAttachStrategy. Now, VersionAttachStrategy determines whether the instance to be attached is new by the following logic:

        boolean isNew = !broker.isDetached(pc); // VersionAttachStrategy.java:70

        which turns out to be true in this case.

        With the current implementation, VersionAttachStrategy does not detect that a) the instance to be attached is carrying a primary key equal to a pre-existing data record and b) hence it should be an update and not an insert. However, it passes the instance as PNEW further downstream to be flushed (the instance is still carrying a primary key value same as the one set by the application and a record with the same key exists). However, as the identity field is annotated with GenerateValue.IDENTITY – the PNEW instance gets stored without a duplicate key exception and its primary key in the database is auto incremented. That the primary key value assigned by the user application is ignored and a new value is assigned can also be verified (see the testcase).

        This is the flow for a new instance being merged in a context which is not managing another instance with the same primary key. If the new instance carrying an existing key was merged to a EntityManager that already is managing another instance, because the attach strategy still considered the to be merged instance as PNEW – a EntityExistsException is raised by the object management kernel even before attampting a flush to the database.

        Show
        Pinaki Poddar added a comment - From openjpa documentation (ref: 1.2. Attach Behavior) " * If the instance was detached and detached state is enabled, OpenJPA will use the detached state to determine the object's version and primary key values. In addition, this state will tell OpenJPA which fields were loaded at the time of detach, and in turn where to expect changes. Loaded detached fields with null values will set the attached instance's corresponding fields to null. If the instance has a Version field, OpenJPA will consider the object detached if the version field has a non-default value, and new otherwise. If neither of the above cases apply, OpenJPA will check to see if an instance with the same primary key values exists in the database. If so, the object is considered detached. Otherwise, it is considered new." The implementaion does not seem to adhere to the last statement above. This is observed by the original scenario described in this issue as well as the attached testcase testMergeUnversionedNewObjectCreatesNewRecord(). AttachStrategy implementaion is responsible for merge(). For a newly created entity instance even if it carries id of a persistent record – the attach strategy selected (by AttachManager.getStrategy() method) is VersionAttachStrategy. Now, VersionAttachStrategy determines whether the instance to be attached is new by the following logic: boolean isNew = !broker.isDetached(pc); // VersionAttachStrategy.java:70 which turns out to be true in this case. With the current implementation, VersionAttachStrategy does not detect that a) the instance to be attached is carrying a primary key equal to a pre-existing data record and b) hence it should be an update and not an insert. However, it passes the instance as PNEW further downstream to be flushed (the instance is still carrying a primary key value same as the one set by the application and a record with the same key exists). However, as the identity field is annotated with GenerateValue.IDENTITY – the PNEW instance gets stored without a duplicate key exception and its primary key in the database is auto incremented. That the primary key value assigned by the user application is ignored and a new value is assigned can also be verified (see the testcase). This is the flow for a new instance being merged in a context which is not managing another instance with the same primary key. If the new instance carrying an existing key was merged to a EntityManager that already is managing another instance, because the attach strategy still considered the to be merged instance as PNEW – a EntityExistsException is raised by the object management kernel even before attampting a flush to the database.
        Hide
        Pinaki Poddar added a comment -

        An entity instance x which has been created by new() operator and carrying a existing id (say idx) set by the user application is treated as detached by merge() when the entity class is annotated with @DetachState(enabled=false). Then merge() updates the existing database record with current state of x. If it is not annotated with @DetachState(enabled=false), the instance x is trated as a new instance, and if the entity is using auto-generated identity, a new database record is created, ignoring the id set by the application. However, if the entitymanager doing the merge() is already managing an instance with the same identity idx then an EntityExists exception is raised.

        The generated enhanced code differs based on @DetachedState setting.
        So if the application has changed this setting, the entity must be enhanced again.

        Show
        Pinaki Poddar added a comment - An entity instance x which has been created by new() operator and carrying a existing id (say idx) set by the user application is treated as detached by merge() when the entity class is annotated with @DetachState(enabled=false). Then merge() updates the existing database record with current state of x. If it is not annotated with @DetachState(enabled=false), the instance x is trated as a new instance, and if the entity is using auto-generated identity, a new database record is created, ignoring the id set by the application. However, if the entitymanager doing the merge() is already managing an instance with the same identity idx then an EntityExists exception is raised. The generated enhanced code differs based on @DetachedState setting. So if the application has changed this setting, the entity must be enhanced again.
        Michael Dick made changes -
        Assignee Michael Dick [ mikedd ]
        Hide
        Michael Dick added a comment -

        I'm still seeing this problem on 1.0.x, and trunk. Attaching a patch for 1.0.1. I believe this resolves the problem. I'll commit to trunk and 1.0.x shortly.

        Show
        Michael Dick added a comment - I'm still seeing this problem on 1.0.x, and trunk. Attaching a patch for 1.0.1. I believe this resolves the problem. I'll commit to trunk and 1.0.x shortly.
        Michael Dick made changes -
        Attachment OPENJPA-245.patch.txt [ 12373786 ]
        Michael Dick made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Patrick Linskey made changes -
        Status Resolved [ 5 ] Reopened [ 4 ]
        Resolution Fixed [ 1 ]
        Hide
        Patrick Linskey added a comment -

        I believe that the changes made for this issue will circumvent some of the more intelligent behavior in more-sophisticated attach strategies, which are used when the instance being attached is an enhanced type that was previously detached (vs. a newly-created instance).

        The attached patch moves the changes to VersionAttachManager, which is invoked when other strategies can't be found.

        We should probably come up with some more complex test scenarios involving graphs of newly-created instances and mixed graphs.

        Show
        Patrick Linskey added a comment - I believe that the changes made for this issue will circumvent some of the more intelligent behavior in more-sophisticated attach strategies, which are used when the instance being attached is an enhanced type that was previously detached (vs. a newly-created instance). The attached patch moves the changes to VersionAttachManager, which is invoked when other strategies can't be found. We should probably come up with some more complex test scenarios involving graphs of newly-created instances and mixed graphs.
        Patrick Linskey made changes -
        Attachment OPENJPA-245.patch [ 12374063 ]
        Hide
        Patrick Linskey added a comment -

        Resolved with r615317.

        Show
        Patrick Linskey added a comment - Resolved with r615317.
        Patrick Linskey made changes -
        Resolution Fixed [ 1 ]
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 1.1.0 [ 12312344 ]
        Michael Dick made changes -
        Fix Version/s 1.0.3 [ 12312969 ]
        Donald Woods made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Michael Dick
            Reporter:
            Aleksandar Likic
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development