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.

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 544202 - Fix CacheKey.protectedForeignKeys update #369

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* Copyright (c) 1998, 2019 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2018 IBM Corporation. All rights reserved.
* Copyright (c) 1998, 2020 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2020 IBM Corporation. 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
Expand All @@ -27,6 +27,8 @@
// - 499335: Multiple embeddable fields can't reference same object
// 02/20/2018-2.7 Will Dazey
// - 529602: Added support for CLOBs in DELETE statements for Oracle
// 04/19/2020-3.0 Alexandre Jacob
// - 544202: Fixed refresh of foreign key values when cacheKey was invalidated
package org.eclipse.persistence.internal.descriptors;

import java.io.Serializable;
Expand Down Expand Up @@ -1041,6 +1043,15 @@ protected Object buildObject(boolean returnCacheKey, ObjectBuildingQuery query,

FetchGroup fetchGroup = query.getExecutionFetchGroup(concreteDescriptor);

// 544202: cachedForeignKeys should be refreshed when cacheKey was invalidated
if (domainWasMissing || shouldRetrieveBypassCache
|| cacheKey.getInvalidationState() == CacheKey.CACHE_KEY_INVALID) {

if (isProtected && (cacheKey != null)) {
cacheForeignKeyValues(databaseRow, cacheKey, session);
}
}

if (domainWasMissing || shouldRetrieveBypassCache) {
cacheHit = false;
if (domainObject == null || shouldStoreBypassCache) {
Expand Down Expand Up @@ -1069,9 +1080,6 @@ protected Object buildObject(boolean returnCacheKey, ObjectBuildingQuery query,
cacheKey.setObject(domainObject);
}
concreteObjectBuilder.buildAttributesIntoObject(domainObject, cacheKey, databaseRow, query, joinManager, fetchGroup, false, session);
if (isProtected && (cacheKey != null)) {
cacheForeignKeyValues(databaseRow, cacheKey, session);
}
if (shouldMaintainCache && !shouldStoreBypassCache) {
// Set the fetch group to the domain object, after built.
if ((fetchGroup != null) && concreteDescriptor.hasFetchGroupManager()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
/*
* Copyright (c) 2020 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
*/

Copy link
Member

Choose a reason for hiding this comment

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

can you follow the convention used in other files wrt formatting of license header? I do not think this is going to pass the copyright check - verify by running mvn -Pcoding-standards glassfish-copyright-mave-plugin:check or mvn -Pcoding-standards glassfish-copyright-mave-plugin:copyright (the latter will show wrong files but won't fail the build in case of errors) - note that the check may take up to 2hours to finish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @lukasj, thank you for your attention, I fixed the copyright as you suggested.
I ran:
mvn -X -Pcoding-standards glassfish-copyright:copyright -pl foundation/org.eclipse.persistence.core
And can confirm it ends with a BUILD SUCCESS and no reported errors.

I rebased master on my branch in order to be up to date

Copy link
Contributor Author

@ajacob ajacob May 1, 2020

Choose a reason for hiding this comment

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

Updated PR's description. Everything was already in bugzilla + test case but it's better to sum up here for future reference

// Contributors:
// 04/19/2020-3.0 Alexandre Jacob
// - 544202: Fixed refresh of foreign key values when cacheKey was invalidated

package org.eclipse.persistence.jpa.test.sharedcache;

import static org.junit.Assert.assertEquals;

import org.eclipse.persistence.jpa.JpaEntityManager;
import org.eclipse.persistence.jpa.test.framework.DDLGen;
import org.eclipse.persistence.jpa.test.framework.Emf;
import org.eclipse.persistence.jpa.test.framework.EmfRunner;
import org.eclipse.persistence.jpa.test.framework.Property;
import org.eclipse.persistence.jpa.test.sharedcache.model.Student;
import org.eclipse.persistence.jpa.test.sharedcache.model.Teacher;
import org.eclipse.persistence.sessions.Session;
import org.eclipse.persistence.sessions.SessionProfiler;
import org.eclipse.persistence.tools.profiler.PerformanceMonitor;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

import jakarta.persistence.EntityManager;
import jakarta.persistence.EntityManagerFactory;
import jakarta.persistence.EntityTransaction;

@RunWith(EmfRunner.class)
public class TestSharedCacheWithNonCacheable {

@Emf(createTables = DDLGen.DROP_CREATE, classes = { Teacher.class, Student.class },
properties = {
@Property(name = "eclipselink.logging.level", value = "FINE"),
@Property(name = "eclipselink.logging.parameters", value = "true"),
@Property(name = "eclipselink.cache.shared.default", value = "false"),
@Property(name = "eclipselink.profiler", value = "PerformanceMonitor")
})
private EntityManagerFactory emf;

private PerformanceMonitor performanceMonitor;

@Before
public void setUp() {
Session session = ((JpaEntityManager) emf.createEntityManager()).getServerSession();
performanceMonitor = (PerformanceMonitor) session.getProfiler();

// We populate DB with 2 students and 1 teacher
Student student1 = new Student(1, "Picasso");
Student student2 = new Student(2, "Pablo");

Teacher teacher = new Teacher(1, "Foo", student1);

final EntityManager em = this.emf.createEntityManager();
final EntityTransaction transaction = em.getTransaction();

transaction.begin();
em.persist(student1);
em.persist(student2);
em.persist(teacher);
transaction.commit();

em.close();
}

@Test
public void testSharedCacheWithNonCacheable() throws Exception {
// 1 : We want to get our Teacher 1. He is supposed to be in the shared cache.
// The Student linked to our Teacher should not be in the cache. (@Noncacheable)
{
Teacher teacher = this.findTeacher(1);

assertEquals(teacher.getId(), 1);
assertEquals(teacher.getName(), "Foo");

// Our Teacher IS in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 1L); // + 1

Student student = teacher.getStudent();

assertEquals(student.getId(), 1); // trigger lazy loading of our Student

// Our Student is NOT in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 1L); // no change
}

// 2 : We change our Teacher 1 in native SQL. The name and the linked student are changed.
{
EntityManager em = emf.createEntityManager();

EntityTransaction transaction = em.getTransaction();

transaction.begin();
em.createNativeQuery("update TEACHER set NAME = ?, STUDENT_ID = ? where ID = ?")
.setParameter(1, "Bar")
.setParameter(2, 2)
.setParameter(3, 1)
.executeUpdate();
transaction.commit();

em.close();
}

// 3 : We want to get our Teacher 1 ONE MORE TIME. He is still supposed to be in the shared cache.
// The Student linked to our Teacher should not be in the cache. (@Noncacheable)
{
Teacher teacher = this.findTeacher(1);

assertEquals(teacher.getId(), 1);
assertEquals(teacher.getName(), "Foo");

// Our Teacher IS in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 2L); // + 1

Student student = teacher.getStudent();

assertEquals(student.getId(), 1); // trigger lazy loading of our Student

// Our Student is NOT in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 2L); // no change
}

// 4 : Now we clear shared cache.
{
emf.getCache().evict(Teacher.class);
}

// 5 : We want to get our Teacher 1 for the THIRD TIME. He is not in the shared cache anymore (invalidated)
// Data should reflect our update from (2)
{
Teacher teacher = this.findTeacher(1);

assertEquals(teacher.getId(), 1);
assertEquals(teacher.getName(), "Bar"); // Updated

// Our Teacher is NOT in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 2L); // no change

Student student = teacher.getStudent();

assertEquals(student.getId(), 2); // trigger lazy loading of our Student

// Our Student is NOT in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 2L); // no change
}

// 6 : We want to get our Teacher 1 for the FOURTH TIME. He is back in the shared.
// Data should reflect our update from (2)
{
Teacher teacher = this.findTeacher(1);

assertEquals(teacher.getId(), 1);
assertEquals(teacher.getName(), "Bar"); // Updated

// Our Teacher IS in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 3L); // + 1

Student student = teacher.getStudent();

// Before correction of bug 544202 this value was 1 because CacheKey.protectedForeignKeys was never updated
assertEquals(student.getId(), 2); // trigger lazy loading of our Student

// Our Student is NOT in shared cache
assertEquals(performanceMonitor.getOperationTime(SessionProfiler.CacheHits), 3L); // no change
}
}

private Teacher findTeacher(int id) {
final EntityManager em = this.emf.createEntityManager();
Teacher teacher = em.find(Teacher.class, id);
teacher.getStudent().getId();
em.close();

return teacher;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright (c) 2020 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:
// 04/19/2020-3.0 Alexandre Jacob
// - 544202: Fixed refresh of foreign key values when cacheKey was invalidated

package org.eclipse.persistence.jpa.test.sharedcache.model;

import jakarta.persistence.Entity;
import jakarta.persistence.Id;

@Entity
public class Student {

@Id
private int id;

private String name;

public Student() {

}

public Student(int id, String name) {
this.id = id;
this.name = name;
}

public int getId() {
return id;
}

public void setId(int id) {
this.id = id;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright (c) 2020 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:
// 04/19/2020-3.0 Alexandre Jacob
// - 544202: Fixed refresh of foreign key values when cacheKey was invalidated

package org.eclipse.persistence.jpa.test.sharedcache.model;

import org.eclipse.persistence.annotations.Cache;
import org.eclipse.persistence.annotations.Noncacheable;
import org.eclipse.persistence.config.CacheIsolationType;

import jakarta.persistence.Cacheable;
import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import jakarta.persistence.ManyToOne;

@Entity
@Cacheable(value = true)
@Cache(isolation = CacheIsolationType.SHARED)
public class Teacher {

@Id
private int id;

private String name;

@Noncacheable
@ManyToOne
private Student student;

public Teacher() {

}

public Teacher(int id, String name, Student student) {
this.id = id;
this.name = name;
this.student = student;
}

public int getId() {
return id;
}

public void setId(int id) {
this.id = id;
}

public String getName() {
return name;
}

public void setName(String name) {
this.name = name;
}

public Student getStudent() {
return student;
}

public void setStudent(Student student) {
this.student = student;
}
}