Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 535998 WLS 12.2.1.2 DOES NOT PROPERLY IMPLEMENT JSR 303 ISSUE #185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bug 535998 WLS 12.2.1.2 DOES NOT PROPERLY IMPLEMENT JSR 303 ISSUE #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e9fd36
Bug 535998 WLS 12.2.1.2 DOES NOT PROPERLY IMPLEMENT JSR 303 ISSUE
sureshbn Jul 19, 2018
11e7e9a
Bug 535998 WLS 12.2.1.2 DOES NOT PROPERLY IMPLEMENT JSR 303 ISSUE
sureshbn Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...clipse.persistence.core/src/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4333,6 +4333,13 @@ public void registerNewObjectForPersist(Object newObject, Map visitedObjects) {
//if object is deleted and a create is issued on the that object
// then the object must be transitioned back to existing and not deleted
this.undeleteObject(newObject);
} else {
if (descriptor.getEventManager().hasAnyEventListeners()) {
DescriptorEvent event = new DescriptorEvent(newObject);
event.setEventCode(DescriptorEventManager.PreUpdateEvent);
event.setSession(this);
descriptor.getEventManager().executeEvent(event);
}
}
descriptor.getObjectBuilder().cascadeRegisterNewForCreate(newObject, this, visitedObjects);
// After any cascade persists and assigning any sequence numbers,
Expand Down
27 changes: 27 additions & 0 deletions ...src/org/eclipse/persistence/testing/tests/jpa/beanvalidation/BeanValidationJunitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public static Test suite() {
suite.addTest(new BeanValidationJunitTest("testRemoveWithInvalidData"));
suite.addTest(new BeanValidationJunitTest("testTraversableResolverPreventsLoadingOfLazyRelationships"));
suite.addTest(new BeanValidationJunitTest("testTraversableResolverPreventsTraversingRelationshipMultipleTimes"));
suite.addTest(new BeanValidationJunitTest("testPesistWithInvalidAndModifiedBeanData"));
}
return suite;
}
Expand Down Expand Up @@ -342,6 +343,32 @@ public void testTraversableResolverPreventsLoadingOfLazyRelationships() {
assertTrue( "Lazy field should not be instantiated because of validation", !isInstantiated(employee, "projects", project) );
assertTrue( "Lazy field should not be instantiated because of validation", !isInstantiated(employee, "managedProject", project) );
}

public void testPesistWithInvalidAndModifiedBeanData() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a different test than BeanValidationJunitTest.testUpdateWithInvalidData()? They seem to be trying to test the same thing, but testUpdateWithInvalidData doesn't seem to be using callbacks and is instead expecting validation on transaction commit. Is this right?

eclipselink/jpa/eclipselink.jpa.test/src/org/eclipse/persistence/testing/tests/jpa/beanvalidation/BeanValidationJunitTest.java

Lines 252 to 258 in b87d11a

try {
// Update it with invalid value
beginTransaction(em);
Employee e = em.find(Employee.class, EMPLOYEE_PK);
e.setName(invalidName);
commitTransaction(em);
} catch (RuntimeException e) {

According to 3.6 of the spec:

These lifecycle validation events occur immediately after the point at which all the 
PrePersist, PreUpdate, and PreRemove lifecycle callback method invocations respectively 
have been complete

I'm wondering if this means testUpdateWithInvalidData is wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw a javadoc somewhere saying that current behaviour is expected and intended by the spec - prepersist is called on the first persist call and not during transaction commit, so subsequent updates to the entity, is is done here are not validated as some people would expect. I just don't know, where I saw that comment :-/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here it is: https://github.com/eclipse-ee4j/eclipselink/blob/master/jpa/org.eclipse.persistence.jpa/src/org/eclipse/persistence/internal/jpa/metadata/listeners/BeanValidationListener.java#L74

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukasj I see! I think there is a misconception then that validation occurs on transaction commit. After reading the spec, it seems pretty clear that validation occurs on the EntityManager operation invocations. However, I searched around online for some examples and it seems transaction commit is being expected by users. Given the comment you found, it certainly sounds like the current spec design has issues that we may want to address in the future.

For example, here is a Hibernate example I found. It seems the example they are using is expecting transaction commit. Perhaps Hibernate is not adhering to the spec? Idk.


EntityManager em = createEntityManager();
boolean gotConstraintViolations = false;
String invalidName = getFilledStringOfLength(Employee.NAME_MAX_SIZSE + 1);
String validName = getFilledStringOfLength(Employee.NAME_MAX_SIZSE - 1);

// Persist an object with valid value
beginTransaction(em);
Employee e1 = new Employee(100, validName, validName, 1337);
em.persist(e1);

// (b) now we demonstrate how eclipse link fails to invoke the JSR validations on entity
// when we modify the entity during the transaction with invalid value
try {
e1.setName(invalidName);
e1.setSurname(invalidName);
em.persist(e1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No em.merge(e1)? The entity is already managed at this point. I ran a quick local test and I don't see EclipseLink calling @PrePersist twice when there are multiple persists for the same managed entity. I can verify that with EclipseLink's implementation as well. Take a look at org.eclipse.persistence.internal.sessions.UnitOfWorkImpl.registerNotRegisteredNewObjectForPersist:

   /**
     * INTERNAL:
     * Called only by registerNewObjectForPersist method,
     * and only if newObject is not already registered.
     * Could be overridden in subclasses.
     */

If the object being persisted is already registered, we don't execute the PrePersistEvent.

}catch (ConstraintViolationException e) {
assertTrue("Transaction not marked for roll back when ConstraintViolation is thrown", getRollbackOnly(em)) ;
gotConstraintViolations =true;
}
assertTrue("Did not get Constraint Violation while persisting invalid data ", gotConstraintViolations);
}


/**
* Strategy:
Expand Down