From 13f72ed1663714949f8e95f02652686ffecdaef2 Mon Sep 17 00:00:00 2001 From: Jonathan Gallimore Date: Fri, 13 Sep 2024 11:37:46 +0100 Subject: [PATCH 1/2] Don't validate if the only change is to a @Version attribute Signed-off-by: Jonathan Gallimore --- .../queries/DatabaseQueryMechanism.java | 1 + .../BeanValidationTableCreator.java | 49 ++++++ .../models/jpa/beanvalidation/Task.java | 62 +++++++ .../main/resources/META-INF/persistence.xml | 1 + .../BeanValidationJunitTest.java | 166 ++++++++++++++++++ .../listeners/BeanValidationListener.java | 7 +- 6 files changed, 285 insertions(+), 1 deletion(-) create mode 100644 jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/queries/DatabaseQueryMechanism.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/queries/DatabaseQueryMechanism.java index 6939d096ff7..2d7742ba204 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/queries/DatabaseQueryMechanism.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/queries/DatabaseQueryMechanism.java @@ -1081,6 +1081,7 @@ && isExpressionQueryMechanism()) {// this is not a hand-coded call (custom SQL e // PERF: Avoid events if no listeners. if (eventManager.hasAnyEventListeners()) { DescriptorEvent event = new DescriptorEvent(DescriptorEventManager.PreUpdateWithChangesEvent, writeQuery); + event.setChangeSet(changeSet); eventManager.executeEvent(event); // PreUpdateWithChangesEvent listeners may have altered the object - should recalculate the change set. diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java index a5216a0dae3..47b3dbfb5d3 100644 --- a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java @@ -22,6 +22,7 @@ public BeanValidationTableCreator() { setName("BeanValidationEmployeeProject"); addTableDefinition(buildProjectTable()); + addTableDefinition(buildTaskTable()); addTableDefinition(buildEmployeeTable()); addTableDefinition(buildEmployeeProjectTable()); } @@ -53,6 +54,54 @@ public TableDefinition buildProjectTable() { return table; } + public TableDefinition buildTaskTable() { + TableDefinition table = new TableDefinition(); + table.setName("CMP3_BV_TASK"); + + FieldDefinition fieldID = new FieldDefinition(); + fieldID.setName("ID"); + fieldID.setTypeName("NUMBER"); + fieldID.setSize(19); + fieldID.setSubSize(0); + fieldID.setIsPrimaryKey(true); + fieldID.setIsIdentity(true); + fieldID.setShouldAllowNull(false); + table.addField(fieldID); + + FieldDefinition fieldVersion = new FieldDefinition(); + fieldVersion.setName("VERSION"); + fieldVersion.setTypeName("NUMBER"); + fieldVersion.setSize(19); + fieldVersion.setSubSize(0); + fieldVersion.setIsPrimaryKey(false); + fieldVersion.setIsIdentity(false); + fieldVersion.setShouldAllowNull(false); + table.addField(fieldVersion); + + FieldDefinition fieldName = new FieldDefinition(); + fieldName.setName("NAME"); + fieldName.setTypeName("VARCHAR"); + fieldName.setSize(20); + fieldName.setShouldAllowNull(true); + fieldName.setIsPrimaryKey(false); + fieldName.setUnique(false); + fieldName.setIsIdentity(false); + table.addField(fieldName); + + FieldDefinition fieldPriority = new FieldDefinition(); + fieldPriority.setName("PRIORITY"); + fieldPriority.setTypeName("NUMBER"); + fieldPriority.setSize(19); + fieldPriority.setSubSize(0); + fieldPriority.setIsPrimaryKey(false); + fieldPriority.setIsIdentity(false); + fieldPriority.setShouldAllowNull(false); + table.addField(fieldPriority); + + return table; + } + + public TableDefinition buildEmployeeTable() { TableDefinition table = new TableDefinition(); table.setName("CMP3_BV_EMPLOYEE"); diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java new file mode 100644 index 00000000000..861dc1ef1e8 --- /dev/null +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2009, 2022 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0, + * or the Eclipse Distribution License v. 1.0 which is available at + * http://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause + */ + +// Contributors: + +package org.eclipse.persistence.testing.models.jpa.beanvalidation; + +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.Column; +import jakarta.persistence.Version; +import jakarta.validation.constraints.NotNull; + +@Entity(name="CMP3_BV_TASK") +public class Task { + + @Id + private int id; + + @Version + private int version; + + @NotNull + private String name; + + @Column + private int priority; + + public Task() {} + + + public int getId() { + return id; + } + + public String getName() { + return name; + } + + public void setName(final String name) { + this.name = name; + } + + + public int getPriority() { + return priority; + } + + + public void setPriority(final int priority) { + this.priority = priority; + } +} diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml index bc783787a7a..85500550d4b 100644 --- a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml @@ -21,6 +21,7 @@ org.eclipse.persistence.testing.models.jpa.beanvalidation.Employee org.eclipse.persistence.testing.models.jpa.beanvalidation.Project org.eclipse.persistence.testing.models.jpa.beanvalidation.Address + org.eclipse.persistence.testing.models.jpa.beanvalidation.Task CALLBACK diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/test/java/org/eclipse/persistence/testing/tests/jpa/beanvalidation/BeanValidationJunitTest.java b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/test/java/org/eclipse/persistence/testing/tests/jpa/beanvalidation/BeanValidationJunitTest.java index 66fbb884875..87df56fdea0 100644 --- a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/test/java/org/eclipse/persistence/testing/tests/jpa/beanvalidation/BeanValidationJunitTest.java +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/test/java/org/eclipse/persistence/testing/tests/jpa/beanvalidation/BeanValidationJunitTest.java @@ -14,8 +14,12 @@ // Marcel Valovy package org.eclipse.persistence.testing.tests.jpa.beanvalidation; +import java.util.Vector; + import jakarta.persistence.EntityManager; import jakarta.persistence.EntityManagerFactory; +import jakarta.persistence.LockModeType; +import jakarta.persistence.RollbackException; import jakarta.persistence.TypedQuery; import jakarta.validation.ConstraintViolation; import jakarta.validation.ConstraintViolationException; @@ -23,11 +27,13 @@ import junit.framework.TestSuite; import org.eclipse.persistence.logging.SessionLog; import org.eclipse.persistence.mappings.ForeignReferenceMapping; +import org.eclipse.persistence.sessions.DatabaseRecord; import org.eclipse.persistence.testing.framework.jpa.junit.JUnitTestCase; import org.eclipse.persistence.testing.models.jpa.beanvalidation.Address; import org.eclipse.persistence.testing.models.jpa.beanvalidation.BeanValidationTableCreator; import org.eclipse.persistence.testing.models.jpa.beanvalidation.Employee; import org.eclipse.persistence.testing.models.jpa.beanvalidation.Project; +import org.eclipse.persistence.testing.models.jpa.beanvalidation.Task; import java.util.ArrayList; import java.util.Arrays; @@ -62,6 +68,9 @@ public static Test suite() { suite.addTest(new BeanValidationJunitTest("testTraversableResolverPreventsLoadingOfLazyRelationships")); suite.addTest(new BeanValidationJunitTest("testTraversableResolverPreventsTraversingRelationshipMultipleTimes")); suite.addTest(new BeanValidationJunitTest("testValidateChangedData")); + suite.addTest(new BeanValidationJunitTest("testPessimisticLockWithInvalidData")); + suite.addTest(new BeanValidationJunitTest("testPessimisticLockUpdateObjectWithInvalidData")); + suite.addTest(new BeanValidationJunitTest("testPessimisticLockUpdateObjectWithValidData")); return suite; } @@ -390,6 +399,163 @@ public void testValidateChangedData() { closeEntityManager(em); } } + + /** + * Strategy: + * 1. Update an Employee and related project to trigger validation on it + * 2. Find the object using its primary key, with a pessimistic lock + * 3. Do not change the object + * 4. End the transaction + * 5. The version field should be incremented in the database, but no other changes + * 6. No validation exceptions should be thrown + */ + public void testPessimisticLockWithInvalidData() throws Exception { + try { + getDatabaseSession().executeNonSelectingSQL("insert into CMP3_BV_TASK values (900, 1, NULL, 1)"); + } catch (Throwable t) { + getDatabaseSession().getSessionLog().logThrowable(SessionLog.WARNING, t); + } + clearCache(); + Map props = new HashMap<>(); + props.put("eclipselink.weaving", "false"); + EntityManagerFactory factory = getEntityManagerFactory(props); + EntityManager em = factory.createEntityManager(); + try { + beginTransaction(em); + + Task task = em.find(Task.class, 900, LockModeType.PESSIMISTIC_FORCE_INCREMENT); + + commitTransaction(em); + + Vector resultSet = getDatabaseSession().executeSQL("select * from CMP3_BV_TASK where ID=900"); + assertEquals(1, resultSet.size()); + + final DatabaseRecord dr = (DatabaseRecord) resultSet.firstElement(); + assertEquals(900L, dr.get("ID")); + assertEquals(2L, dr.get("VERSION")); // should be incremented by the pessimistic lock + assertNull(dr.get("NAME")); // should be unchanged + assertEquals(1L, dr.get("PRIORITY")); // should be unchanged + + } catch (RuntimeException ex) { + if (isTransactionActive(em)) { + rollbackTransaction(em); + } + throw ex; + } finally { + closeEntityManager(em); + } + } + + /** + * Strategy: + * 1. Update an Employee and related project to trigger validation on it + * 2. Find the object using its primary key, with a pessimistic lock + * 3. Change the object with still invalid data + * 4. End the transaction + * 5. The version field should be incremented in the database, but no other changes + * 6. A validation exception should be thrown + */ + public void testPessimisticLockUpdateObjectWithInvalidData() throws Exception { + try { + getDatabaseSession().executeNonSelectingSQL("insert into CMP3_BV_TASK values (901, 1, NULL, 1)"); + } catch (Throwable t) { + getDatabaseSession().getSessionLog().logThrowable(SessionLog.WARNING, t); + } + clearCache(); + Map props = new HashMap<>(); + props.put("eclipselink.weaving", "false"); + boolean gotConstraintViolations = false; + EntityManagerFactory factory = getEntityManagerFactory(props); + EntityManager em = factory.createEntityManager(); + try { + beginTransaction(em); + + Task task = em.find(Task.class, 901, LockModeType.PESSIMISTIC_FORCE_INCREMENT); + task.setPriority(2); + + commitTransaction(em); + } catch (ConstraintViolationException e) { + assertTrue("Transaction not marked for roll back when ConstraintViolation is thrown", getRollbackOnly(em)); + Set> constraintViolations = e.getConstraintViolations(); + ConstraintViolation constraintViolation = constraintViolations.iterator().next(); + Object invalidValue = constraintViolation.getInvalidValue(); + System.out.println(invalidValue); + gotConstraintViolations = true; + } catch (RollbackException e) { + e.printStackTrace(); + final ConstraintViolationException cve = (ConstraintViolationException) e.getCause(); + Set> constraintViolations = cve.getConstraintViolations(); + ConstraintViolation constraintViolation = constraintViolations.iterator().next(); + assertEquals("must not be null", constraintViolation.getMessage()); + gotConstraintViolations = true; + } finally { + if (isTransactionActive(em)) { + rollbackTransaction(em); + } + closeEntityManager(em); + } + + assertTrue("Did not get Constraint Violation while persisting invalid data ", gotConstraintViolations); + + Vector resultSet = getDatabaseSession().executeSQL("select * from CMP3_BV_TASK where ID=901"); + assertEquals(1, resultSet.size()); + + final DatabaseRecord dr = (DatabaseRecord) resultSet.firstElement(); + assertEquals(901L, dr.get("ID")); + assertEquals(1L, dr.get("VERSION")); // should be unchanged + assertNull(dr.get("NAME")); // should be unchanged + assertEquals(1L, dr.get("PRIORITY")); // should be unchanged + } + + /** + * Strategy: + * 1. Update an Employee and related project to trigger validation on it + * 2. Find the object using its primary key, with a pessimistic lock + * 3. Change the object to have valid data + * 4. End the transaction + * 5. The version field should be incremented in the database along with the other changes + * 6. No validation exceptions should be thrown + */ + public void testPessimisticLockUpdateObjectWithValidData() throws Exception { + try { + getDatabaseSession().executeNonSelectingSQL("insert into CMP3_BV_TASK values (902, 1, NULL, 1)"); + } catch (Throwable t) { + getDatabaseSession().getSessionLog().logThrowable(SessionLog.WARNING, t); + } + clearCache(); + Map props = new HashMap<>(); + props.put("eclipselink.weaving", "false"); + boolean gotConstraintViolations = false; + EntityManagerFactory factory = getEntityManagerFactory(props); + EntityManager em = factory.createEntityManager(); + try { + beginTransaction(em); + + Task task = em.find(Task.class, 902, LockModeType.PESSIMISTIC_FORCE_INCREMENT); + task.setPriority(2); + task.setName("Do some work"); + + commitTransaction(em); + } catch (ConstraintViolationException e) { + gotConstraintViolations = true; + } finally { + if (isTransactionActive(em)) { + rollbackTransaction(em); + } + closeEntityManager(em); + } + + assertFalse("Got Constraint Violation while persisting valid data ", gotConstraintViolations); + + Vector resultSet = getDatabaseSession().executeSQL("select * from CMP3_BV_TASK where ID=902"); + assertEquals(1, resultSet.size()); + + final DatabaseRecord dr = (DatabaseRecord) resultSet.firstElement(); + assertEquals(902L, dr.get("ID")); + assertEquals(2L, dr.get("VERSION")); // should be incremented by the pessimistic lock + assertEquals("Do some work", dr.get("NAME")); // new value + assertEquals(2L, dr.get("PRIORITY")); // new value + } //--------------------Helper Methods ---------------// private boolean isInstantiated(Object entityObject, String attributeName, org.eclipse.persistence.sessions.Project project) { diff --git a/jpa/org.eclipse.persistence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metadata/listeners/BeanValidationListener.java b/jpa/org.eclipse.persistence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metadata/listeners/BeanValidationListener.java index 769f33e0b4c..8056ac7fe62 100644 --- a/jpa/org.eclipse.persistence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metadata/listeners/BeanValidationListener.java +++ b/jpa/org.eclipse.persistence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metadata/listeners/BeanValidationListener.java @@ -40,9 +40,11 @@ import org.eclipse.persistence.descriptors.ClassDescriptor; import org.eclipse.persistence.descriptors.DescriptorEvent; import org.eclipse.persistence.descriptors.DescriptorEventAdapter; +import org.eclipse.persistence.descriptors.DescriptorEventManager; import org.eclipse.persistence.descriptors.FetchGroupManager; import org.eclipse.persistence.internal.localization.ExceptionLocalization; import org.eclipse.persistence.internal.security.PrivilegedAccessHelper; +import org.eclipse.persistence.internal.sessions.ObjectChangeSet; import org.eclipse.persistence.internal.sessions.UnitOfWorkImpl; import org.eclipse.persistence.mappings.DatabaseMapping; import org.eclipse.persistence.mappings.ForeignReferenceMapping; @@ -93,7 +95,10 @@ public void aboutToUpdate(DescriptorEvent event) { // preUpdate is also generated for deleted objects that were modified in this UOW. // Do not perform preUpdate validation for such objects as preRemove would have already been called. if(!unitOfWork.isObjectDeleted(source)) { - validateOnCallbackEvent(event, "preUpdate", groupPreUpdate); + ObjectChangeSet changeSet = event.getChangeSet(); + if (changeSet != null && (changeSet.isNew() || (changeSet.getChanges() != null && changeSet.getChanges().size() > 0))) { + validateOnCallbackEvent(event, "preUpdate", groupPreUpdate); + } } } From 2a564a53b66f5a86bb7209bc19af6b6663a91afe Mon Sep 17 00:00:00 2001 From: Jonathan Gallimore Date: Fri, 4 Oct 2024 15:20:37 +0100 Subject: [PATCH 2/2] Adjusted following feedback: * Copyright headers * Formatting/spacing * Remove exception block from test case and attempt to make the test case clearer Signed-off-by: Jonathan Gallimore --- .../BeanValidationTableCreator.java | 2 +- .../models/jpa/beanvalidation/Task.java | 13 +++++------- .../main/resources/META-INF/persistence.xml | 2 +- .../BeanValidationJunitTest.java | 21 +++++++------------ 4 files changed, 15 insertions(+), 23 deletions(-) diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java index 47b3dbfb5d3..91bc833e9ea 100644 --- a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/BeanValidationTableCreator.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2022 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2009, 2024 Oracle and/or its affiliates. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java index 861dc1ef1e8..63becfc4d34 100644 --- a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/java/org/eclipse/persistence/testing/models/jpa/beanvalidation/Task.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2022 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2009, 2024 Oracle and/or its affiliates. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -24,20 +24,19 @@ public class Task { @Id - private int id; + private int id; @Version - private int version; + private int version; @NotNull - private String name; + private String name; @Column - private int priority; + private int priority; public Task() {} - public int getId() { return id; } @@ -50,12 +49,10 @@ public void setName(final String name) { this.name = name; } - public int getPriority() { return priority; } - public void setPriority(final int priority) { this.priority = priority; } diff --git a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml index 85500550d4b..5f999f24b88 100644 --- a/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml +++ b/jpa/eclipselink.jpa.testapps/jpa.test.beanvalidation/src/main/resources/META-INF/persistence.xml @@ -1,6 +1,6 @@