From 7cece296b3e9183aae9dd49abfa235612ab26036 Mon Sep 17 00:00:00 2001 From: "A. Abram White" Date: Tue, 8 Apr 2008 21:29:19 +0000 Subject: [PATCH] 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 --- .../apache/openjpa/enhance/PCEnhancer.java | 127 ++++++++++++------ .../openjpa/kernel/VersionAttachStrategy.java | 9 +- .../openjpa/kernel/localizer.properties | 6 +- .../detachment/TestAttachConstructedCopy.java | 72 ++++++++++ .../src/doc/manual/ref_guide_remote.xml | 5 +- 5 files changed, 167 insertions(+), 52 deletions(-) create mode 100755 openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/detachment/TestAttachConstructedCopy.java diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java b/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java index 52b4572a1..5b056f928 100644 --- a/openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCEnhancer.java +++ b/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" - // instance, so check version before taking into account non-existent - // detached state + // allow users with version or auto-assigned pk fields to manually + // construct a "detached" instance, so check these before taking into + // 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; - } - - // 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()); + return false; } // 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 - target = code.constant().setNull(); + // create artificial target to simplify + 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; } /** diff --git a/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/VersionAttachStrategy.java b/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/VersionAttachStrategy.java index 7fa741f7b..c01dcb2fa 100644 --- a/openjpa-kernel/src/main/java/org/apache/openjpa/kernel/VersionAttachStrategy.java +++ b/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; diff --git a/openjpa-kernel/src/main/resources/org/apache/openjpa/kernel/localizer.properties b/openjpa-kernel/src/main/resources/org/apache/openjpa/kernel/localizer.properties index cfeac3d4f..5c8d73732 100644 --- a/openjpa-kernel/src/main/resources/org/apache/openjpa/kernel/localizer.properties +++ b/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 \ @@ -396,4 +398,4 @@ cant-serialize-pessimistic-broker: Serialization not allowed for brokers with \ an active datastore (pessimistic) transaction. cant-serialize-connected-broker: Serialization not allowed for brokers with \ an active connection to the database. -no-interface-metadata: No metadata was found for managed interface {0}. \ No newline at end of file +no-interface-metadata: No metadata was found for managed interface {0}. diff --git a/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/detachment/TestAttachConstructedCopy.java b/openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/detachment/TestAttachConstructedCopy.java new file mode 100755 index 000000000..1af43571a --- /dev/null +++ b/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); + } +} + diff --git a/openjpa-project/src/doc/manual/ref_guide_remote.xml b/openjpa-project/src/doc/manual/ref_guide_remote.xml index 0320be631..0ed8ca611 100644 --- a/openjpa-project/src/doc/manual/ref_guide_remote.xml +++ b/openjpa-project/src/doc/manual/ref_guide_remote.xml @@ -173,7 +173,10 @@ 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. +value, and new otherwise. Similarly, if the instance has +GeneratedValue primary key fields, OpenJPA will consider the +object detached if any of these fields have non-default values, and new +otherwise. When attaching null fields in these cases, OpenJPA cannot distinguish between a