OpenJPA
  1. OpenJPA
  2. OPENJPA-1787

Bean validation fails merging a new entity

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.1, 2.1.0, 2.2.0
    • Fix Version/s: 2.1.1, 2.2.0
    • Component/s: jpa, kernel
    • Labels:
      None

      Description

      The bean validation is not working correctly

      If you try to merge a new entity.

      EntityManager em = entityManagerFactory.createEntityManager();
      Person person = new Person();
      person.setName("Oliver"); // Employee.name is annotated @NotNull
      person = em.merge(person);

      you get a ConstraintValidationException, although name is set.

      1. OPENJPA-1787_jUnits_2.1.x.patch
        10 kB
        Jeremy Bauer
      2. OPENJPA-1787_2.1.x.patch
        3 kB
        Jeremy Bauer
      3. testcase-openjpa-1787.tar
        3 kB
        Oliver Ringel
      4. com.example.TestEmployeeDAO.txt
        5 kB
        Oliver Ringel
      5. openjpa-1787.tar
        7 kB
        Oliver Ringel

        Activity

        Oliver Ringel created issue -
        Hide
        Oliver Ringel added a comment -

        Here is a testcase.

        Show
        Oliver Ringel added a comment - Here is a testcase.
        Oliver Ringel made changes -
        Field Original Value New Value
        Attachment openjpa-1787.tar [ 12454309 ]
        Hide
        Donald Woods added a comment -

        Can you provide more details about your application and environment? You marked affects 2.0.1 and 2.1.0, so which jars are you using? The openjpa-2.x.x.jar does not include the BVAL API or implementation. If you grab the latest openjpa-all-2.1.0-SNAPSHOT.jar then that includes the Apache Geronimo Validation API and Apache Bean Validation implementation.

        Show
        Donald Woods added a comment - Can you provide more details about your application and environment? You marked affects 2.0.1 and 2.1.0, so which jars are you using? The openjpa-2.x.x.jar does not include the BVAL API or implementation. If you grab the latest openjpa-all-2.1.0-SNAPSHOT.jar then that includes the Apache Geronimo Validation API and Apache Bean Validation implementation.
        Hide
        Donald Woods added a comment -

        So after looking at your example, I'm not sure why you opened a JIRA....
        You're using Hibernate Validator 4.0.2.GA, which we support.
        The Employee.name is marked with @NotNull.
        But in your code snippet above, you are setting the Name, so what is the behavior you're expecting? @NotNull should only throw an exception if you didn't call setName() before trying to merge/persist the entity.....

        Show
        Donald Woods added a comment - So after looking at your example, I'm not sure why you opened a JIRA.... You're using Hibernate Validator 4.0.2.GA, which we support. The Employee.name is marked with @NotNull. But in your code snippet above, you are setting the Name, so what is the behavior you're expecting? @NotNull should only throw an exception if you didn't call setName() before trying to merge/persist the entity.....
        Hide
        Oliver Ringel added a comment -

        OK, I think I described the issue not clear enough.

        As you said right, @NotNull should only throw an exception, if name is not set.
        The problem is, that I get an exception (javax.validation.ConstraintViolationException),
        although I called setName().

        Using persist instead of merge is working as expected. I added added test method to
        demonstrate this.

        Show
        Oliver Ringel added a comment - OK, I think I described the issue not clear enough. As you said right, @NotNull should only throw an exception, if name is not set. The problem is, that I get an exception (javax.validation.ConstraintViolationException), although I called setName(). Using persist instead of merge is working as expected. I added added test method to demonstrate this.
        Hide
        Donald Woods added a comment -

        Can you attach your stack trace? I just ran into a scenario where an entity not using Validation is causing the Apache Bean Validation provider to throw an exception due to entity.hashCode() throwing a NPE....

        Show
        Donald Woods added a comment - Can you attach your stack trace? I just ran into a scenario where an entity not using Validation is causing the Apache Bean Validation provider to throw an exception due to entity.hashCode() throwing a NPE....
        Hide
        Oliver Ringel added a comment -

        Here is my test output.

        Show
        Oliver Ringel added a comment - Here is my test output.
        Oliver Ringel made changes -
        Attachment com.example.TestEmployeeDAO.txt [ 12454528 ]
        Oliver Ringel made changes -
        Affects Version/s 2.2.0 [ 12315910 ]
        Environment Ubuntu 10.04, Spring 3.0.3
        Description The bean validation is not working correctly, if you try to merge not existing entity.

                Employee employee = new Employee();
                employee.setName("Oliver"); // Employee.name is annotated @NotNull
                employee = em.merge(employee);

        em.persist and em.merge together with a managed entity is working as expected.

        The bean validation is not working correctly

        If you try to merge a new entity.

                EntityManager em = entityManagerFactory.createEntityManager();
                Person person = new Person();
                person.setName("Oliver"); // Employee.name is annotated @NotNull
                person = em.merge(person);

        you get a ConstraintValidationException, although name is set.

        Priority Major [ 3 ] Critical [ 2 ]
        Component/s jpa [ 12311304 ]
        Component/s kernel [ 12311302 ]
        Component/s validation [ 12312869 ]
        Hide
        Oliver Ringel added a comment -

        A new simple testcase without dependencies to spring, etc.

        Show
        Oliver Ringel added a comment - A new simple testcase without dependencies to spring, etc.
        Oliver Ringel made changes -
        Attachment testcase-openjpa-1787.tar [ 12472438 ]
        Hide
        Oliver Ringel added a comment -

        I finally found the place in the source code where the data gets lost.

        If you merge a new entity the BrokerImpl first tries to attach the entity via the AttachManager.
        The AttachManager uses an AttachStrategy to persist the new entity.

        Here ist the code snippet from the org.apache.openjpa.kernel.AttachStrategy class with my comments

        protected StateManagerImpl persist(AttachManager manager,
        PersistenceCapable pc, ClassMetaData meta, Object appId,
        boolean explicit) {
        PersistenceCapable newInstance;

        if (!manager.getCopyNew())
        newInstance = pc; <--- calling this would fix the issue
        else if (appId == null)

        { newInstance = pc.pcNewInstance(null, false); <--- but this is called, pc.pcNewInstance returns an new instance, person.name is set to null }

        else
        newInstance = pc.pcNewInstance(null, appId, false);

        return (StateManagerImpl) manager.getBroker().persist <-- this calls finally BrokerImpl.persistInternal(...) and fails because person.name is null
        (newInstance, appId, explicit, manager.getBehavior());
        }

        For my testcases this issue could be fixed by replacing the first if statement with "if (manager.getCopyNew())".
        I'm absolutely not sure, if it is really that easy. I guess not.

        Maybe another good place to fix the problem is EntityManagerImpl.merge() which also sets the AttachManager's copyNew flag.

        Please could someone with more insight into openjpa verify this solution.
        Thanks.

        Show
        Oliver Ringel added a comment - I finally found the place in the source code where the data gets lost. If you merge a new entity the BrokerImpl first tries to attach the entity via the AttachManager. The AttachManager uses an AttachStrategy to persist the new entity. Here ist the code snippet from the org.apache.openjpa.kernel.AttachStrategy class with my comments protected StateManagerImpl persist(AttachManager manager, PersistenceCapable pc, ClassMetaData meta, Object appId, boolean explicit) { PersistenceCapable newInstance; if (!manager.getCopyNew()) newInstance = pc; <--- calling this would fix the issue else if (appId == null) { newInstance = pc.pcNewInstance(null, false); <--- but this is called, pc.pcNewInstance returns an new instance, person.name is set to null } else newInstance = pc.pcNewInstance(null, appId, false); return (StateManagerImpl) manager.getBroker().persist <-- this calls finally BrokerImpl.persistInternal(...) and fails because person.name is null (newInstance, appId, explicit, manager.getBehavior()); } For my testcases this issue could be fixed by replacing the first if statement with "if (manager.getCopyNew())". I'm absolutely not sure, if it is really that easy. I guess not. Maybe another good place to fix the problem is EntityManagerImpl.merge() which also sets the AttachManager's copyNew flag. Please could someone with more insight into openjpa verify this solution. Thanks.
        Hide
        Michael Dick added a comment -

        The main problem with your proposed solution is that it would change the way merge works. EntityManager.merge creates a new copy of the entity and it's the copy of the entity that becomes part of the persistence context - not the entity you passed in. Your change would eliminate the copy.

        I think the real issue here is that the data hasn't been copied into the new instance before validation occurs. I'm not familiar with this code path, and I haven't had a chance to try your testcase yet, but I'd start by looking at the code where we copy the fields into a new instance.

        Show
        Michael Dick added a comment - The main problem with your proposed solution is that it would change the way merge works. EntityManager.merge creates a new copy of the entity and it's the copy of the entity that becomes part of the persistence context - not the entity you passed in. Your change would eliminate the copy. I think the real issue here is that the data hasn't been copied into the new instance before validation occurs. I'm not familiar with this code path, and I haven't had a chance to try your testcase yet, but I'd start by looking at the code where we copy the fields into a new instance.
        Hide
        Oliver Ringel added a comment -

        Hi Michael,
        thank you for your answer. I thought that it would not be so easy to solve this issue. Unfortunately.

        I agree with you that the main problem here is that a copy action is missing and I guess AttachStrategy.persist is the right place to add some kind of copy functionality.

        I started looking into the source code of the enhanced class from the test. PersistenceCapable declares a public method to copy fields (pcCopyFields(...)).
        Unfortunately you can't use in AttachStrategy.persist, because the entity to persist has no statemanager at this point (an exception InvalidStateException will be the result).

        My suggestion is to modify the class enhancement and remove the StateManager check in pcCopyFields (I don't see the need for this check).
        Afterwards you can use pcCopyFields in AttachStrategy.persist by adding something like

        ...
        if (manager.getCopyNew())

        { int[] fields = new int[meta.getFields().length]; for (int i = 0; i < fields.length; i++) fields[i] = i; newInstance.pcCopyFields(pc, fields); }

        ...

        This solution works for my testcase.

        As an alternative you can modify (or add an additional) newInstance to copy the data.

        I have no real experience with OpenJPA. Perhaps there is a much better solution.

        Show
        Oliver Ringel added a comment - Hi Michael, thank you for your answer. I thought that it would not be so easy to solve this issue. Unfortunately. I agree with you that the main problem here is that a copy action is missing and I guess AttachStrategy.persist is the right place to add some kind of copy functionality. I started looking into the source code of the enhanced class from the test. PersistenceCapable declares a public method to copy fields (pcCopyFields(...)). Unfortunately you can't use in AttachStrategy.persist, because the entity to persist has no statemanager at this point (an exception InvalidStateException will be the result). My suggestion is to modify the class enhancement and remove the StateManager check in pcCopyFields (I don't see the need for this check). Afterwards you can use pcCopyFields in AttachStrategy.persist by adding something like ... if (manager.getCopyNew()) { int[] fields = new int[meta.getFields().length]; for (int i = 0; i < fields.length; i++) fields[i] = i; newInstance.pcCopyFields(pc, fields); } ... This solution works for my testcase. As an alternative you can modify (or add an additional) newInstance to copy the data. I have no real experience with OpenJPA. Perhaps there is a much better solution.
        Hide
        Rick Curtis added a comment -

        Can you post a stack trace of the problem?

        Show
        Rick Curtis added a comment - Can you post a stack trace of the problem?
        Jeremy Bauer made changes -
        Assignee Jeremy Bauer [ techhusky ]
        Hide
        Oliver Ringel added a comment -

        Hi Rick,

        this is the stack trace from my testcase.

        2004 OpenJpaTestcase TRACE [main] openjpa.Runtime - An exception occurred while ending the transaction. This exception will be re-thrown.
        <openjpa-2.2.0-SNAPSHOT-r422266:1078811 fatal store error> org.apache.openjpa.util.StoreException: The transaction cannot be committed, because it was already marked for rollback only. The transaction will be rolled back instead. The cause of the rollback-only status is reported in the embedded stack.
        at org.apache.openjpa.kernel.LocalManagedRuntime.commit(LocalManagedRuntime.java:89)
        at org.apache.openjpa.kernel.BrokerImpl.commit(BrokerImpl.java:1497)
        at org.apache.openjpa.kernel.DelegatingBroker.commit(DelegatingBroker.java:933)
        at org.apache.openjpa.persistence.EntityManagerImpl.commit(EntityManagerImpl.java:565)
        at testcase.openjpa.OpenJpaTest.testValidation(OpenJpaTest.java:24)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
        at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
        at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
        at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:76)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:236)
        at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:59)
        at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:115)
        at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:102)
        at org.apache.maven.surefire.Surefire.run(Surefire.java:180)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:350)
        at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:1021)
        Caused by: javax.validation.ConstraintViolationException: A validation constraint failure occurred for class "testcase.openjpa.Person".
        at org.apache.openjpa.persistence.validation.ValidatorImpl.validate(ValidatorImpl.java:282)
        at org.apache.openjpa.validation.ValidatingLifecycleEventManager.fireEvent(ValidatingLifecycleEventManager.java:122)
        at org.apache.openjpa.kernel.BrokerImpl.fireLifecycleEvent(BrokerImpl.java:790)
        at org.apache.openjpa.kernel.BrokerImpl.persistInternal(BrokerImpl.java:2606)
        at org.apache.openjpa.kernel.BrokerImpl.persist(BrokerImpl.java:2544)
        at org.apache.openjpa.kernel.AttachStrategy.persist(AttachStrategy.java:95)
        at org.apache.openjpa.kernel.VersionAttachStrategy.attach(VersionAttachStrategy.java:102)
        at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:251)
        at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:104)
        at org.apache.openjpa.kernel.BrokerImpl.attach(BrokerImpl.java:3433)
        at org.apache.openjpa.kernel.DelegatingBroker.attach(DelegatingBroker.java:1214)
        at org.apache.openjpa.persistence.EntityManagerImpl.merge(EntityManagerImpl.java:873)
        at testcase.openjpa.OpenJpaTest.testValidation(OpenJpaTest.java:22)
        ... 26 more

        Show
        Oliver Ringel added a comment - Hi Rick, this is the stack trace from my testcase. 2004 OpenJpaTestcase TRACE [main] openjpa.Runtime - An exception occurred while ending the transaction. This exception will be re-thrown. <openjpa-2.2.0-SNAPSHOT-r422266:1078811 fatal store error> org.apache.openjpa.util.StoreException: The transaction cannot be committed, because it was already marked for rollback only. The transaction will be rolled back instead. The cause of the rollback-only status is reported in the embedded stack. at org.apache.openjpa.kernel.LocalManagedRuntime.commit(LocalManagedRuntime.java:89) at org.apache.openjpa.kernel.BrokerImpl.commit(BrokerImpl.java:1497) at org.apache.openjpa.kernel.DelegatingBroker.commit(DelegatingBroker.java:933) at org.apache.openjpa.persistence.EntityManagerImpl.commit(EntityManagerImpl.java:565) at testcase.openjpa.OpenJpaTest.testValidation(OpenJpaTest.java:24) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:76) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184) at org.junit.runners.ParentRunner.run(ParentRunner.java:236) at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:59) at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:115) at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:102) at org.apache.maven.surefire.Surefire.run(Surefire.java:180) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:350) at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:1021) Caused by: javax.validation.ConstraintViolationException: A validation constraint failure occurred for class "testcase.openjpa.Person". at org.apache.openjpa.persistence.validation.ValidatorImpl.validate(ValidatorImpl.java:282) at org.apache.openjpa.validation.ValidatingLifecycleEventManager.fireEvent(ValidatingLifecycleEventManager.java:122) at org.apache.openjpa.kernel.BrokerImpl.fireLifecycleEvent(BrokerImpl.java:790) at org.apache.openjpa.kernel.BrokerImpl.persistInternal(BrokerImpl.java:2606) at org.apache.openjpa.kernel.BrokerImpl.persist(BrokerImpl.java:2544) at org.apache.openjpa.kernel.AttachStrategy.persist(AttachStrategy.java:95) at org.apache.openjpa.kernel.VersionAttachStrategy.attach(VersionAttachStrategy.java:102) at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:251) at org.apache.openjpa.kernel.AttachManager.attach(AttachManager.java:104) at org.apache.openjpa.kernel.BrokerImpl.attach(BrokerImpl.java:3433) at org.apache.openjpa.kernel.DelegatingBroker.attach(DelegatingBroker.java:1214) at org.apache.openjpa.persistence.EntityManagerImpl.merge(EntityManagerImpl.java:873) at testcase.openjpa.OpenJpaTest.testValidation(OpenJpaTest.java:22) ... 26 more
        Hide
        Jeremy Bauer added a comment -

        I tracked down the source of the problem. When merging a new entity, OpenJPA persists a new "empty" entity, sets (or retrieves) an ID for the new entity, and then merges in the object passed in. The problem is that the during the persist of the empty entity, a PRE_PERSIST lifecycle event gets fired, causing validation to occur. I've found that not only is validation occurring prematurely, but OpenJPA violates the JPA specification for the PrePersist callback as well.

        <jpa 1.0 & 2.0 spec - section 3.5.2>
        For entities to which the merge operation has been applied and causes the creation of newly managed instances, the PrePersist callback methods will be invoked for the managed instance after the entity state has been copied to it.
        <jpa 1.0 & 2.0 spec - section 3.5.2/>

        By adding this simple callback to the Person class, it showed that a PrePersist callback occurred before the entity was fully populated.
        class Person {
        ...

        @PrePersist
        public void prePersist()

        { System.out.println("PrePersist getName()=" + this.getName()); }

        }

        Result: PrePersist getName()=null

        I think the solution will be to disable this callback for the case where a new entity is created specifically for the purposes of a merge. Working on a fix...

        Show
        Jeremy Bauer added a comment - I tracked down the source of the problem. When merging a new entity, OpenJPA persists a new "empty" entity, sets (or retrieves) an ID for the new entity, and then merges in the object passed in. The problem is that the during the persist of the empty entity, a PRE_PERSIST lifecycle event gets fired, causing validation to occur. I've found that not only is validation occurring prematurely, but OpenJPA violates the JPA specification for the PrePersist callback as well. <jpa 1.0 & 2.0 spec - section 3.5.2> For entities to which the merge operation has been applied and causes the creation of newly managed instances, the PrePersist callback methods will be invoked for the managed instance after the entity state has been copied to it. <jpa 1.0 & 2.0 spec - section 3.5.2/> By adding this simple callback to the Person class, it showed that a PrePersist callback occurred before the entity was fully populated. class Person { ... @PrePersist public void prePersist() { System.out.println("PrePersist getName()=" + this.getName()); } } Result: PrePersist getName()=null I think the solution will be to disable this callback for the case where a new entity is created specifically for the purposes of a merge. Working on a fix...
        Hide
        Jeremy Bauer added a comment -

        Attaching patch for 2.1.x release that prevents the persist path from firing a premature pre-persist event on new entities produced in a merge operation. I am seeing some jUnit failures that test order/look schema related rather than related to this change. I am looking into those failures and producing additional test cases.

        Show
        Jeremy Bauer added a comment - Attaching patch for 2.1.x release that prevents the persist path from firing a premature pre-persist event on new entities produced in a merge operation. I am seeing some jUnit failures that test order/look schema related rather than related to this change. I am looking into those failures and producing additional test cases.
        Jeremy Bauer made changes -
        Attachment OPENJPA-1787_2.1.x.patch [ 12473363 ]
        Hide
        Jeremy Bauer added a comment -

        Attached jUnits for inclusion in the bean validation integration module.

        Show
        Jeremy Bauer added a comment - Attached jUnits for inclusion in the bean validation integration module.
        Jeremy Bauer made changes -
        Attachment OPENJPA-1787_jUnits_2.1.x.patch [ 12473408 ]
        Hide
        Jeremy Bauer added a comment -

        Committed code and jUnit patches to trunk. I was able to get clean local and server based builds today. I'm not sure what was up yesterday.

        Show
        Jeremy Bauer added a comment - Committed code and jUnit patches to trunk. I was able to get clean local and server based builds today. I'm not sure what was up yesterday.
        Hide
        Oliver Ringel added a comment -

        Perfect. You fixed the issue. Both my test case as well as my real project are working now. Thank you very much.

        BTW. I found something probably wrong in the ValidatingLifecycleEventManager

        ...
        @Override
        public boolean hasUpdateListeners(Object source, ClassMetaData meta) {
        if (_validator == null)

        { return super.hasUpdateListeners(source, meta); }

        return _validator.validating(source, LifecycleEvent.BEFORE_PERSIST) || <--- LifecycleEvent.BEFORE_UPDATE
        super.hasUpdateListeners(source, meta);
        }

        @Override
        public boolean hasPersistListeners(Object source, ClassMetaData meta) {
        if (_validator == null)

        { return super.hasPersistListeners(source, meta); }

        return _validator.validating(source, LifecycleEvent.BEFORE_UPDATE) || <--- LifecycleEvent.BEFORE_PERSIST
        super.hasPersistListeners(source, meta);
        }
        ...

        Although it has no effect for my testcase, it looks not correct. Maybe you verify this.

        Show
        Oliver Ringel added a comment - Perfect. You fixed the issue. Both my test case as well as my real project are working now. Thank you very much. BTW. I found something probably wrong in the ValidatingLifecycleEventManager ... @Override public boolean hasUpdateListeners(Object source, ClassMetaData meta) { if (_validator == null) { return super.hasUpdateListeners(source, meta); } return _validator.validating(source, LifecycleEvent.BEFORE_PERSIST) || <--- LifecycleEvent.BEFORE_UPDATE super.hasUpdateListeners(source, meta); } @Override public boolean hasPersistListeners(Object source, ClassMetaData meta) { if (_validator == null) { return super.hasPersistListeners(source, meta); } return _validator.validating(source, LifecycleEvent.BEFORE_UPDATE) || <--- LifecycleEvent.BEFORE_PERSIST super.hasPersistListeners(source, meta); } ... Although it has no effect for my testcase, it looks not correct. Maybe you verify this.
        Hide
        Jeremy Bauer added a comment -

        Great! I'm glad it fixed the problem. I just checked the code into the 2.1.x stream as well.

        Yes, the has*Listeners methods in the VLEM do look suspect. Thanks for reporting it. I'll do some verification and will open a new JIRA if it turns out to be a bug. I'm about 99% certain it is... Both pre-persist and pre-update validation are enabled by default. So, the bug will not surface unless non-default validation groups are specified. Looks like we are missing a test variation or three.

        Show
        Jeremy Bauer added a comment - Great! I'm glad it fixed the problem. I just checked the code into the 2.1.x stream as well. Yes, the has*Listeners methods in the VLEM do look suspect. Thanks for reporting it. I'll do some verification and will open a new JIRA if it turns out to be a bug. I'm about 99% certain it is... Both pre-persist and pre-update validation are enabled by default. So, the bug will not surface unless non-default validation groups are specified. Looks like we are missing a test variation or three.
        Jeremy Bauer made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.1.1 [ 12316191 ]
        Fix Version/s 2.2.0 [ 12315910 ]
        Resolution Fixed [ 1 ]
        Hide
        Jeremy Bauer added a comment -

        It looks like the hasPersistListeners and hasUpdateListeners methods in the VLEM are implemented simply for the sake of consistency and, as far as I can tell, are not actually called within OpenJPA. Event type checking is all within the fireEvent method itself and testing shows it to work as expected. The hasDeleteListeners method (which is implemented correctly) does get called by JDBCStoreQuery for specific in-memory operations. For the sake of consistency, and in the event that an external plugin may be calling these methods (they are public), I'll check in the simple code fix and some additional test cases that further verify validation is working as expected, based on the event type and specified validation group (or lack thereof). This JIRA will be used to make these changes.

        Show
        Jeremy Bauer added a comment - It looks like the hasPersistListeners and hasUpdateListeners methods in the VLEM are implemented simply for the sake of consistency and, as far as I can tell, are not actually called within OpenJPA. Event type checking is all within the fireEvent method itself and testing shows it to work as expected. The hasDeleteListeners method (which is implemented correctly) does get called by JDBCStoreQuery for specific in-memory operations. For the sake of consistency, and in the event that an external plugin may be calling these methods (they are public), I'll check in the simple code fix and some additional test cases that further verify validation is working as expected, based on the event type and specified validation group (or lack thereof). This JIRA will be used to make these changes.
        Jeremy Bauer made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Jeremy Bauer added a comment -

        Committed has*Listeners fix + additional testcases to trunk under rev 1082259. I don't think this additional change necessarily needs to be backported since it doesn't appear to cause any real problem, but it certainly could be.

        Show
        Jeremy Bauer added a comment - Committed has*Listeners fix + additional testcases to trunk under rev 1082259. I don't think this additional change necessarily needs to be backported since it doesn't appear to cause any real problem, but it certainly could be.
        Jeremy Bauer made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Albert Lee added a comment -

        Close issue in preparation for 2.2.0 release.

        Show
        Albert Lee added a comment - Close issue in preparation for 2.2.0 release.
        Albert Lee made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Roger Keays added a comment -

        Did this bug fix make it into 2.2.0?

        I have exactly the same problem using @Basic(optional=false)

        Show
        Roger Keays added a comment - Did this bug fix make it into 2.2.0? I have exactly the same problem using @Basic(optional=false)
        Hide
        Roger Keays added a comment -

        Or more accurately, when the merge cascades more than one relationship. i.e. the non-null field is

        Invoice.Customer.Contact.name

        and I call

        merge(invoice)

        The Customer validates okay, but validation annotations on the Contact all fail because the fields are null.

        Show
        Roger Keays added a comment - Or more accurately, when the merge cascades more than one relationship. i.e. the non-null field is Invoice.Customer.Contact.name and I call merge(invoice) The Customer validates okay, but validation annotations on the Contact all fail because the fields are null.
        Hide
        Kevin Sutter added a comment -

        According to Albert's comment on 02/02/2012 and the svn commit messages, this problem was resolved in the 2.2.0 release. Are you still experiencing a similar problem with 2.2.0? Or, were you just asking?

        Show
        Kevin Sutter added a comment - According to Albert's comment on 02/02/2012 and the svn commit messages, this problem was resolved in the 2.2.0 release. Are you still experiencing a similar problem with 2.2.0? Or, were you just asking?
        Hide
        Roger Keays added a comment -

        I have this problem in 2.2.0

        <dependency>
        <groupId>org.apache.openjpa</groupId>
        <artifactId>openjpa</artifactId>
        <version>2.2.0</version>
        <scope>compile</scope>
        </dependency>

        Show
        Roger Keays added a comment - I have this problem in 2.2.0 <dependency> <groupId>org.apache.openjpa</groupId> <artifactId>openjpa</artifactId> <version>2.2.0</version> <scope>compile</scope> </dependency>
        Hide
        Roger Keays added a comment -

        I've hit this bug again with cascade merging. Validations on the nested object fail because the fields are all null. Maybe the fireEvents flag isn't properly handled when cascading?

        Show
        Roger Keays added a comment - I've hit this bug again with cascade merging. Validations on the nested object fail because the fields are all null. Maybe the fireEvents flag isn't properly handled when cascading?
        Hide
        Roger Keays added a comment -

        I've opened OPENJPA-2279 and attached a failing unit test for the problem I described.

        Show
        Roger Keays added a comment - I've opened OPENJPA-2279 and attached a failing unit test for the problem I described.

          People

          • Assignee:
            Jeremy Bauer
            Reporter:
            Oliver Ringel
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development