More efficient fix for OPENJPA-245.

git-svn-id: https://svn.apache.org/repos/asf/openjpa/branches/1.1.x@646082 13f79535-47bb-0310-9956-ffa450edef68

openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java

@@ -118,7 +118,10 @@ public class PCEnhancer {
     public static final int ENHANCE_INTERFACE = 2 << 1;
     public static final int ENHANCE_PC = 2 << 2;
 
-    private static final String PRE = "pc";
+    public static final String PRE = "pc";
+    public static final String ISDETACHEDSTATEDEFINITIVE = PRE 
+        + "isDetachedStateDefinitive";
+
     private static final Class PCTYPE = PersistenceCapable.class;
     private static final String SM = PRE + "StateManager";
     private static final Class SMTYPE = StateManager.class;
@@ -2999,12 +3002,27 @@ public class PCEnhancer {
      */
     private void addIsDetachedMethod()
         throws NoSuchMethodException {
-        // public boolean pcIsDetached ()
+        // public boolean pcIsDetached()
         BCMethod method = _pc.declareMethod(PRE + "IsDetached",
             Boolean.class, null);
         method.makePublic();
         Code code = method.getCode(true);
-        writeIsDetachedMethod(code);
+        boolean needsDefinitiveMethod = writeIsDetachedMethod(code);
+        code.calculateMaxStack();
+        code.calculateMaxLocals();
+        if (!needsDefinitiveMethod) 
+            return;
+
+        // private boolean pcIsDetachedStateDefinitive()
+        //   return false;
+        // auxilliary enhancers may change the return value of this method
+        // if their specs consider detached state definitive
+        method = _pc.declareMethod(ISDETACHEDSTATEDEFINITIVE, boolean.class,
+            null);
+        method.makePrivate();
+        code = method.getCode(true);
+        code.constant().setValue(false);
+        code.ireturn();
         code.calculateMaxStack();
         code.calculateMaxLocals();
     }
@@ -3012,14 +3030,17 @@ public class PCEnhancer {
     /**
      * Creates the body of the pcIsDetached() method to determine if an
      * instance is detached.
+     *
+     * @return true if we need a pcIsDetachedStateDefinitive method, false
+     * otherwise
      */
-    private void writeIsDetachedMethod(Code code)
+    private boolean writeIsDetachedMethod(Code code)
         throws NoSuchMethodException {
         // not detachable: return Boolean.FALSE
         if (!_meta.isDetachable()) {
             code.getstatic().setField(Boolean.class, "FALSE", Boolean.class);
             code.areturn();
-            return;
+            return false;
         }
 
         // if (sm != null)
@@ -3067,7 +3088,7 @@ public class PCEnhancer {
                 ifins.setTarget(target);
                 notdeser.setTarget(target);
                 code.areturn();
-                return;
+                return false;
             }
         }
 
@@ -3077,9 +3098,9 @@ public class PCEnhancer {
         if (notdeser != null)
             notdeser.setTarget(target);
 
-        // allow users with version fields to manually construct a "detached"
+        // allow users with version or auto-assigned pk fields to manually 
-        // instance, so check version before taking into account non-existent
+        // construct a "detached" instance, so check these before taking into 
-        // detached state
+        // account non-existent detached state
 
         // consider detached if version is non-default
         FieldMetaData version = _meta.getVersionField();
@@ -3094,41 +3115,13 @@ public class PCEnhancer {
             ifins.setTarget(code.getstatic().setField(Boolean.class, "FALSE",
                 Boolean.class));
             code.areturn();
-            return;
+            return false;
-        }
-
-        // no detached state: if instance uses detached state and it's not
-        // synthetic or the instance is not serializable or the state isn't
-        // transient, must not be detached
-        if (state == null
-            && (!ClassMetaData.SYNTHETIC.equals(_meta.getDetachedState())
-            || !Serializable.class.isAssignableFrom(_meta.getDescribedType())
-            || !_repos.getConfiguration().getDetachStateInstance().
-            isDetachedStateTransient())) {
-            // return Boolean.FALSE
-            code.getstatic().setField(Boolean.class, "FALSE", Boolean.class);
-            code.areturn();
-            return;
-        }
-
-        // no detached state: if instance uses detached state (and must be
-        // synthetic and transient in serializable instance at this point),
-        // not detached if state not set to DESERIALIZED
-        if (state == null) {
-            // if (pcGetDetachedState () == null) // instead of DESERIALIZED
-            //     return Boolean.FALSE;
-            loadManagedInstance(code, false);
-            code.invokevirtual().setMethod(PRE + "GetDetachedState",
-                Object.class, null);
-            ifins = code.ifnonnull();
-            code.getstatic().setField(Boolean.class, "FALSE", Boolean.class);
-            code.areturn();
-            ifins.setTarget(code.nop());
         }
 
         // consider detached if auto-genned primary keys are non-default
         ifins = null;
         JumpInstruction ifins2 = null;
+        boolean hasAutoAssignedPK = false;
         if (state != Boolean.TRUE
             && _meta.getIdentityType() == ClassMetaData.ID_APPLICATION) {
             // for each pk field:
@@ -3162,13 +3155,65 @@ public class PCEnhancer {
             }
         }
 
-        // give up; we just don't know
+        // create artificial target to simplify
-        target = code.constant().setNull();
+        target = code.nop();
         if (ifins != null)
             ifins.setTarget(target);
         if (ifins2 != null)
             ifins2.setTarget(target);
+
+        // if has auto-assigned pk and we get to this point, must have default
+        // value, so must be new instance
+        if (hasAutoAssignedPK) {
+            code.getstatic().setField(Boolean.class, "FALSE", Boolean.class);
             code.areturn();
+            return false;
+        }
+
+        // if detached state is not definitive, just give up now and return
+        // null so that the runtime will perform a DB lookup to determine
+        // whether we're detached or new
+        code.aload().setThis();
+        code.invokespecial().setMethod(ISDETACHEDSTATEDEFINITIVE, boolean.class,
+            null);
+        ifins = code.ifne();
+        code.constant().setNull();
+        code.areturn();
+        ifins.setTarget(code.nop());
+
+        // no detached state: if instance uses detached state and it's not
+        // synthetic or the instance is not serializable or the state isn't
+        // transient, must not be detached
+        if (state == null
+            && (!ClassMetaData.SYNTHETIC.equals(_meta.getDetachedState())
+            || !Serializable.class.isAssignableFrom(_meta.getDescribedType())
+            || !_repos.getConfiguration().getDetachStateInstance().
+            isDetachedStateTransient())) {
+            // return Boolean.FALSE
+            code.getstatic().setField(Boolean.class, "FALSE", Boolean.class);
+            code.areturn();
+            return true;
+        }
+
+        // no detached state: if instance uses detached state (and must be
+        // synthetic and transient in serializable instance at this point),
+        // not detached if state not set to DESERIALIZED
+        if (state == null) {
+            // if (pcGetDetachedState () == null) // instead of DESERIALIZED
+            //     return Boolean.FALSE;
+            loadManagedInstance(code, false);
+            code.invokevirtual().setMethod(PRE + "GetDetachedState",
+                Object.class, null);
+            ifins = code.ifnonnull();
+            code.getstatic().setField(Boolean.class, "FALSE", Boolean.class);
+            code.areturn();
+            ifins.setTarget(code.nop());
+        }
+
+        // give up; we just don't know
+        code.constant().setNull();
+        code.areturn();
+        return true;
     }
 
     /**

openjpa-kernel/src/main/java/org/apache/openjpa/kernel/VersionAttachStrategy.java

@@ -69,19 +69,12 @@ class VersionAttachStrategy
     public Object attach(AttachManager manager, Object toAttach,
         ClassMetaData meta, PersistenceCapable into, OpenJPAStateManager owner,
         ValueMetaData ownerMeta, boolean explicit) {
-
-        // VersionAttachStrategy is invoked in the case where no more
-        // intelligent strategy could be found; let's be more lenient
-        // about new vs. detached record determination.
-        if (into == null)
-            into = findFromDatabase(manager, toAttach);
-
         BrokerImpl broker = manager.getBroker();
         PersistenceCapable pc = ImplHelper.toPersistenceCapable(toAttach,
             meta.getRepository().getConfiguration());
 
         boolean embedded = ownerMeta != null && ownerMeta.isEmbeddedPC();
-        boolean isNew = !broker.isDetached(pc) && into == null;
+        boolean isNew = !broker.isDetached(pc);
         Object version = null;
         StateManagerImpl sm;
 

openjpa-kernel/src/main/resources/org/apache/openjpa/kernel/localizer.properties

@@ -116,7 +116,9 @@ not-managed: The given instance "{0}" is not managed by this context.
 trans-not-managed: This broker is not configured to use managed transactions.
 bad-detached-op: You cannot perform operation {0} on detached object "{1}". \
 	This operation only applies to managed objects.
-persist-detached: Attempt to persist detached object "{0}".
+persist-detached: Attempt to persist detached object "{0}".  If this is a new \
+  instance, make sure any versino and/or auto-generated primary key fields are \
+  null/default when persisting.
 null-value: The field "{0}" of instance "{1}" contained a null value; \
 	the metadata for this field specifies that nulls are illegal.
 change-identity: Attempt to change a primary key field of an instance that \

openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/detachment/TestAttachConstructedCopy.java

@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.    
+ */
+package org.apache.openjpa.persistence.detachment;
+
+import javax.persistence.EntityManager;
+
+import junit.textui.TestRunner;
+import org.apache.openjpa.persistence.test.SingleEMFTestCase;
+
+/**
+ * Test that manually constructing instances with existing primary key values
+ * and attaching them works.
+ *
+ * @author Abe White
+ */
+public class TestAttachConstructedCopy
+    extends SingleEMFTestCase {
+
+    public void setUp() {
+        setUp(Record.class);
+    }
+
+    public void testAttachConstructedCopyWithGeneratedPKAndNoVersion() {
+        Record record = new Record();
+        record.setContent("orig");
+
+        EntityManager em = emf.createEntityManager();
+        em.getTransaction().begin();
+        em.persist(record);
+        em.getTransaction().commit();
+        em.close();
+        int id = record.getId();
+
+        Record copy = new Record();
+        copy.setId(id);
+        copy.setContent("new");
+
+        em = emf.createEntityManager();
+        em.getTransaction().begin(); 
+        record = em.merge(copy);
+        assertTrue(record != copy);
+        assertEquals("new", record.getContent());
+        em.getTransaction().commit();
+        em.close();
+
+        em = emf.createEntityManager();
+        record = em.find(Record.class, id);
+        assertEquals("new", record.getContent());
+        em.close();
+    }
+
+    public static void main(String[] args) {
+        TestRunner.run(TestAttachConstructedCopy.class);
+    }
+}
+

openjpa-project/src/doc/manual/ref_guide_remote.xml

@@ -173,7 +173,10 @@ attached instance's corresponding fields to null.
                     <para>
 If the instance has a <literal>Version</literal> field,
 OpenJPA will consider the object detached if the version field has a non-default
-value, and new otherwise.
+value, and new otherwise.  Similarly, if the instance has 
+<literal>GeneratedValue</literal> primary key fields, OpenJPA will consider the
+object detached if any of these fields have non-default values, and new 
+otherwise.
                     </para>
                     <para>
 When attaching null fields in these cases, OpenJPA cannot distinguish between a