From cea10af33c0a9c005e252d3fc2b7f55db56a986f Mon Sep 17 00:00:00 2001 From: Pavol Mederly Date: Sun, 12 Apr 2015 08:05:49 +0200 Subject: [PATCH] Fix for MID-1917 (user photo disappearing) --- .../midpoint/repo/sql/UserPhotoTest.java | 183 ++++++++++++++++++ .../resources/photo/t001-add-employeeType.xml | 29 +++ .../resources/photo/t002-remove-photo.xml | 28 +++ .../resources/photo/t003-re-add-photo.xml | 29 +++ .../resources/photo/t004-change-photo.xml | 29 +++ .../src/test/resources/photo/user.xml | 35 ++++ .../repo/sql/SqlRepositoryServiceImpl.java | 51 ++++- .../midpoint/repo/sql/data/common/RUser.java | 6 +- 8 files changed, 388 insertions(+), 2 deletions(-) create mode 100644 repo/repo-sql-impl-test/src/test/java/com/evolveum/midpoint/repo/sql/UserPhotoTest.java create mode 100644 repo/repo-sql-impl-test/src/test/resources/photo/t001-add-employeeType.xml create mode 100644 repo/repo-sql-impl-test/src/test/resources/photo/t002-remove-photo.xml create mode 100644 repo/repo-sql-impl-test/src/test/resources/photo/t003-re-add-photo.xml create mode 100644 repo/repo-sql-impl-test/src/test/resources/photo/t004-change-photo.xml create mode 100644 repo/repo-sql-impl-test/src/test/resources/photo/user.xml diff --git a/repo/repo-sql-impl-test/src/test/java/com/evolveum/midpoint/repo/sql/UserPhotoTest.java b/repo/repo-sql-impl-test/src/test/java/com/evolveum/midpoint/repo/sql/UserPhotoTest.java new file mode 100644 index 00000000000..6d6b4692c04 --- /dev/null +++ b/repo/repo-sql-impl-test/src/test/java/com/evolveum/midpoint/repo/sql/UserPhotoTest.java @@ -0,0 +1,183 @@ +/* + * Copyright (c) 2010-2015 Evolveum + * + * Licensed 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 com.evolveum.midpoint.repo.sql; + +import com.evolveum.midpoint.prism.PrismObject; +import com.evolveum.midpoint.prism.delta.ObjectDelta; +import com.evolveum.midpoint.prism.util.PrismTestUtil; +import com.evolveum.midpoint.schema.DeltaConvertor; +import com.evolveum.midpoint.schema.GetOperationOptions; +import com.evolveum.midpoint.schema.MidPointPrismContextFactory; +import com.evolveum.midpoint.schema.RetrieveOption; +import com.evolveum.midpoint.schema.SelectorOptions; +import com.evolveum.midpoint.schema.result.OperationResult; +import com.evolveum.midpoint.util.exception.ObjectNotFoundException; +import com.evolveum.midpoint.util.exception.SchemaException; +import com.evolveum.midpoint.xml.ns._public.common.api_types_3.ObjectModificationType; +import com.evolveum.midpoint.xml.ns._public.common.common_3.UserType; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.ContextConfiguration; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import java.io.File; +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; + +import static org.testng.AssertJUnit.assertEquals; +import static org.testng.AssertJUnit.fail; + +/** + * @author mederly + */ +@ContextConfiguration(locations = {"../../../../../ctx-test.xml"}) +@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) +public class UserPhotoTest extends BaseSQLRepoTest { + + private static final File TEST_DIR = new File("src/test/resources/photo"); + private static final File USER_FILE = new File(TEST_DIR, "user.xml"); + private static final File T001_ADD_EMPLOYEE_TYPE = new File(TEST_DIR, "t001-add-employeeType.xml"); + private static final File T002_REMOVE_PHOTO = new File(TEST_DIR, "t002-remove-photo.xml"); + private static final File T003_RE_ADD_PHOTO = new File(TEST_DIR, "t003-re-add-photo.xml"); + private static final File T004_CHANGE_PHOTO = new File(TEST_DIR, "t004-change-photo.xml"); + + private String userOid; + + @BeforeClass + public void beforeClass() throws Exception { + super.beforeClass(); + + PrismTestUtil.resetPrismContext(MidPointPrismContextFactory.FACTORY); + } + + @Test + public void test010Add() throws Exception { + OperationResult result = new OperationResult(UserPhotoTest.class.getName() + ".test010Add"); + + PrismObject user = PrismTestUtil.parseObject(USER_FILE); + userOid = repositoryService.addObject(user, null, result); + + checkObject(userOid, USER_FILE, result); + checkObjectNoPhoto(userOid, USER_FILE, result); + } + + @Test + public void test020ModifyUser() throws Exception { + OperationResult result = new OperationResult(UserPhotoTest.class.getName() + ".test020ModifyUser"); + + ObjectDelta delta = parseDelta(userOid, T001_ADD_EMPLOYEE_TYPE); + + repositoryService.modifyObject(UserType.class, userOid, delta.getModifications(), result); + checkObject(userOid, USER_FILE, result, delta); + } + + @Test + public void test030RemovePhoto() throws Exception { + OperationResult result = new OperationResult(UserPhotoTest.class.getName() + ".test030RemovePhoto"); + + ObjectDelta delta1 = parseDelta(userOid, T001_ADD_EMPLOYEE_TYPE); + ObjectDelta delta2 = parseDelta(userOid, T002_REMOVE_PHOTO); + + repositoryService.modifyObject(UserType.class, userOid, delta2.getModifications(), result); + checkObject(userOid, USER_FILE, result, delta1, delta2); + } + + @Test + public void test040ReAddPhoto() throws Exception { + OperationResult result = new OperationResult(UserPhotoTest.class.getName() + ".test040ReAddPhoto"); + + ObjectDelta delta1 = parseDelta(userOid, T001_ADD_EMPLOYEE_TYPE); + ObjectDelta delta2 = parseDelta(userOid, T002_REMOVE_PHOTO); + ObjectDelta delta3 = parseDelta(userOid, T003_RE_ADD_PHOTO); + + repositoryService.modifyObject(UserType.class, userOid, delta3.getModifications(), result); + checkObject(userOid, USER_FILE, result, delta1, delta2, delta3); + } + + @Test + public void test050ChangePhoto() throws Exception { + OperationResult result = new OperationResult(UserPhotoTest.class.getName() + ".test050ReplacePhoto"); + + ObjectDelta delta1 = parseDelta(userOid, T001_ADD_EMPLOYEE_TYPE); + ObjectDelta delta2 = parseDelta(userOid, T002_REMOVE_PHOTO); + ObjectDelta delta3 = parseDelta(userOid, T003_RE_ADD_PHOTO); + ObjectDelta delta4 = parseDelta(userOid, T004_CHANGE_PHOTO); + + repositoryService.modifyObject(UserType.class, userOid, delta4.getModifications(), result); + checkObject(userOid, USER_FILE, result, delta1, delta2, delta3, delta4); + } + + /** + * Checks that after removing a user the photo is removed as well. + */ + @Test + public void test099DeleteUser() throws Exception { + OperationResult result = new OperationResult(UserPhotoTest.class.getName() + ".test099DeleteUser"); + + repositoryService.deleteObject(UserType.class, userOid, result); + + PrismObject user = PrismTestUtil.parseObject(USER_FILE); + user.asObjectable().setJpegPhoto(null); + String oid = repositoryService.addObject(user, null, result); + assertEquals("Oid was changed", userOid, oid); + + checkObject(userOid, user, true, result); // there should be no photo there + } + + protected ObjectDelta parseDelta(String oid, File file) throws SchemaException, IOException { + ObjectModificationType modification = PrismTestUtil.parseAtomicValue(file, ObjectModificationType.COMPLEX_TYPE); + ObjectDelta delta = DeltaConvertor.createObjectDelta(modification, UserType.class, prismContext); + delta.setOid(oid); + return delta; + } + + private void checkObject(String oid, File file, OperationResult result) throws SchemaException, IOException, ObjectNotFoundException { + PrismObject expected = PrismTestUtil.parseObject(file); + checkObject(oid, expected, true, result); + } + + private void checkObjectNoPhoto(String oid, File file, OperationResult result) throws SchemaException, IOException, ObjectNotFoundException { + PrismObject expected = PrismTestUtil.parseObject(file); + expected.asObjectable().setJpegPhoto(null); + checkObject(oid, expected, false, result); + } + + private void checkObject(String oid, PrismObject expected, boolean loadPhoto, OperationResult result) throws ObjectNotFoundException, SchemaException { + Collection> options; + if (loadPhoto) { + options = Arrays.asList(SelectorOptions.create(UserType.F_JPEG_PHOTO, GetOperationOptions.createRetrieve(RetrieveOption.INCLUDE))); + } else { + options = null; + } + PrismObject real = repositoryService.getObject(UserType.class, oid, options, result); + ObjectDelta delta = expected.diff(real); + System.out.println("Expected object = \n" + expected.debugDump()); + System.out.println("Real object in repo = \n" + real.debugDump()); + System.out.println("Difference = \n" + delta.debugDump()); + if (!delta.isEmpty()) { + fail("Objects are not equal.\n*** Expected:\n" + expected.debugDump() + "\n*** Got:\n" + real.debugDump() + "\n*** Delta:\n" + delta.debugDump()); + } + } + + private void checkObject(String oid, File file, OperationResult result, ObjectDelta... appliedDeltas) throws SchemaException, IOException, ObjectNotFoundException { + PrismObject expected = PrismTestUtil.parseObject(file); + for (ObjectDelta delta : appliedDeltas) { + delta.applyTo(expected); + } + checkObject(oid, expected, true, result); + } +} diff --git a/repo/repo-sql-impl-test/src/test/resources/photo/t001-add-employeeType.xml b/repo/repo-sql-impl-test/src/test/resources/photo/t001-add-employeeType.xml new file mode 100644 index 00000000000..e8a53c4bf6f --- /dev/null +++ b/repo/repo-sql-impl-test/src/test/resources/photo/t001-add-employeeType.xml @@ -0,0 +1,29 @@ + + + + + + 48174916-4137-9830-4442-654354808381 + + add + c:employeeNumber + en1234 + + diff --git a/repo/repo-sql-impl-test/src/test/resources/photo/t002-remove-photo.xml b/repo/repo-sql-impl-test/src/test/resources/photo/t002-remove-photo.xml new file mode 100644 index 00000000000..a2d77c947be --- /dev/null +++ b/repo/repo-sql-impl-test/src/test/resources/photo/t002-remove-photo.xml @@ -0,0 +1,28 @@ + + + + + + 48174916-4137-9830-4442-654354808381 + + replace + jpegPhoto + + diff --git a/repo/repo-sql-impl-test/src/test/resources/photo/t003-re-add-photo.xml b/repo/repo-sql-impl-test/src/test/resources/photo/t003-re-add-photo.xml new file mode 100644 index 00000000000..c7ff8845eb9 --- /dev/null +++ b/repo/repo-sql-impl-test/src/test/resources/photo/t003-re-add-photo.xml @@ -0,0 +1,29 @@ + + + + + + 48174916-4137-9830-4442-654354808381 + + replace + jpegPhoto + /9j/4AAQSkZJRgABAQEAYABgAAD/2wBDAAIBAQIBAQICAgICAgICAwUDAwMDAwYEBAMFBwYHBwcGBwcICQsJCAgKCAcHCg0KCgsMDAwMBwkODw0MDgsMDAz/2wBDAQICAgMDAwYDAwYMCAcIDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAz/wAARCAAgACADASIAAhEBAxEB/8QAHwAAAQUBAQEBAQEAAAAAAAAAAAECAwQFBgcICQoL/8QAtRAAAgEDAwIEAwUFBAQAAAF9AQIDAAQRBRIhMUEGE1FhByJxFDKBkaEII0KxwRVS0fAkM2JyggkKFhcYGRolJicoKSo0NTY3ODk6Q0RFRkdISUpTVFVWV1hZWmNkZWZnaGlqc3R1dnd4eXqDhIWGh4iJipKTlJWWl5iZmqKjpKWmp6ipqrKztLW2t7i5usLDxMXGx8jJytLT1NXW19jZ2uHi4+Tl5ufo6erx8vP09fb3+Pn6/8QAHwEAAwEBAQEBAQEBAQAAAAAAAAECAwQFBgcICQoL/8QAtREAAgECBAQDBAcFBAQAAQJ3AAECAxEEBSExBhJBUQdhcRMiMoEIFEKRobHBCSMzUvAVYnLRChYkNOEl8RcYGRomJygpKjU2Nzg5OkNERUZHSElKU1RVVldYWVpjZGVmZ2hpanN0dXZ3eHl6goOEhYaHiImKkpOUlZaXmJmaoqOkpaanqKmqsrO0tba3uLm6wsPExcbHyMnK0tPU1dbX2Nna4uPk5ebn6Onq8vP09fb3+Pn6/9oADAMBAAIRAxEAPwD9/KKKKACiiigAooooAKKKKAP/2Q== + + diff --git a/repo/repo-sql-impl-test/src/test/resources/photo/t004-change-photo.xml b/repo/repo-sql-impl-test/src/test/resources/photo/t004-change-photo.xml new file mode 100644 index 00000000000..add2007d4a2 --- /dev/null +++ b/repo/repo-sql-impl-test/src/test/resources/photo/t004-change-photo.xml @@ -0,0 +1,29 @@ + + + + + + 48174916-4137-9830-4442-654354808381 + + replace + jpegPhoto + /9j/4AAQSkZJRgABAQEAYABgAAD/2wBDAAIBAQIBAQICAgICAgICAwUDAwMDAwYEBAMFBwYHBwcGBwcICQsJCAgKCAcHCg0KCgsMDAwMBwkODw0MDgsMDAz/2wBDAQICAgMDAwYDAwYMCAcIDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAz/wAARCAAEAAQDASIAAhEBAxEB/8QAHwAAAQUBAQEBAQEAAAAAAAAAAAECAwQFBgcICQoL/8QAtRAAAgEDAwIEAwUFBAQAAAF9AQIDAAQRBRIhMUEGE1FhByJxFDKBkaEII0KxwRVS0fAkM2JyggkKFhcYGRolJicoKSo0NTY3ODk6Q0RFRkdISUpTVFVWV1hZWmNkZWZnaGlqc3R1dnd4eXqDhIWGh4iJipKTlJWWl5iZmqKjpKWmp6ipqrKztLW2t7i5usLDxMXGx8jJytLT1NXW19jZ2uHi4+Tl5ufo6erx8vP09fb3+Pn6/8QAHwEAAwEBAQEBAQEBAQAAAAAAAAECAwQFBgcICQoL/8QAtREAAgECBAQDBAcFBAQAAQJ3AAECAxEEBSExBhJBUQdhcRMiMoEIFEKRobHBCSMzUvAVYnLRChYkNOEl8RcYGRomJygpKjU2Nzg5OkNERUZHSElKU1RVVldYWVpjZGVmZ2hpanN0dXZ3eHl6goOEhYaHiImKkpOUlZaXmJmaoqOkpaanqKmqsrO0tba3uLm6wsPExcbHyMnK0tPU1dbX2Nna4uPk5ebn6Onq8vP09fb3+Pn6/9oADAMBAAIRAxEAPwD9/KKKKAP/2Q== + + diff --git a/repo/repo-sql-impl-test/src/test/resources/photo/user.xml b/repo/repo-sql-impl-test/src/test/resources/photo/user.xml new file mode 100644 index 00000000000..ba870899478 --- /dev/null +++ b/repo/repo-sql-impl-test/src/test/resources/photo/user.xml @@ -0,0 +1,35 @@ + + + + + guybrushhh + + Guybrush Threepwood + guybrush threepwood + + + Guybrush + guybrush + + + Threepwood + threepwood + + /9j/4AAQSkZJRgABAQEAYABgAAD/2wBDAAIBAQIBAQICAgICAgICAwUDAwMDAwYEBAMFBwYHBwcGBwcICQsJCAgKCAcHCg0KCgsMDAwMBwkODw0MDgsMDAz/2wBDAQICAgMDAwYDAwYMCAcIDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAz/wAARCAAgACADASIAAhEBAxEB/8QAHwAAAQUBAQEBAQEAAAAAAAAAAAECAwQFBgcICQoL/8QAtRAAAgEDAwIEAwUFBAQAAAF9AQIDAAQRBRIhMUEGE1FhByJxFDKBkaEII0KxwRVS0fAkM2JyggkKFhcYGRolJicoKSo0NTY3ODk6Q0RFRkdISUpTVFVWV1hZWmNkZWZnaGlqc3R1dnd4eXqDhIWGh4iJipKTlJWWl5iZmqKjpKWmp6ipqrKztLW2t7i5usLDxMXGx8jJytLT1NXW19jZ2uHi4+Tl5ufo6erx8vP09fb3+Pn6/8QAHwEAAwEBAQEBAQEBAQAAAAAAAAECAwQFBgcICQoL/8QAtREAAgECBAQDBAcFBAQAAQJ3AAECAxEEBSExBhJBUQdhcRMiMoEIFEKRobHBCSMzUvAVYnLRChYkNOEl8RcYGRomJygpKjU2Nzg5OkNERUZHSElKU1RVVldYWVpjZGVmZ2hpanN0dXZ3eHl6goOEhYaHiImKkpOUlZaXmJmaoqOkpaanqKmqsrO0tba3uLm6wsPExcbHyMnK0tPU1dbX2Nna4uPk5ebn6Onq8vP09fb3+Pn6/9oADAMBAAIRAxEAPwD9/KKKKACiiigAooooAKKKKAP/2Q== + diff --git a/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/SqlRepositoryServiceImpl.java b/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/SqlRepositoryServiceImpl.java index 2c28d8324a3..6ce509fb82d 100644 --- a/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/SqlRepositoryServiceImpl.java +++ b/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/SqlRepositoryServiceImpl.java @@ -1237,8 +1237,29 @@ private void modifyObjectAttempt(Class type, String oi Collection lookupTableModifications = filterLookupTableModifications(type, modifications); + // JpegPhoto (RUserPhoto) is a special kind of entity. First of all, it is lazily loaded, because photos are really big. + // Each RUserPhoto naturally belongs to one RUser, so it would be appropriate to set orphanRemoval=true for user-photo + // association. However, this leads to a strange problem when merging in-memory RUser object with the database state: + // If in-memory RUser object has no photo associated (because of lazy loading), then the associated RUserPhoto is deleted. + // + // To prevent this behavior, we've set orphanRemoval to false. Fortunately, the remove operation on RUser + // seems to be still cascaded to RUserPhoto. What we have to implement ourselves, however, is removal of RUserPhoto + // _without_ removing of RUser. In order to know whether the photo has to be removed, we have to retrieve + // its value, apply the delta (e.g. if the delta is a DELETE VALUE X, we have to know whether X matches current + // value of the photo), and if the resulting value is empty, we have to manually delete the RUserPhoto instance. + // + // So the first step is to retrieve the current value of photo - we obviously do this only if the modifications + // deal with the jpegPhoto property. + Collection> options; + boolean containsUserPhotoModification = UserType.class.equals(type) && containsPhotoModification(modifications); + if (containsUserPhotoModification) { + options = Arrays.asList(SelectorOptions.create(UserType.F_JPEG_PHOTO, GetOperationOptions.createRetrieve(RetrieveOption.INCLUDE))); + } else { + options = null; + } + // get object - PrismObject prismObject = getObject(session, type, oid, null, true); + PrismObject prismObject = getObject(session, type, oid, options, true); // apply diff if (LOGGER.isTraceEnabled()) { LOGGER.trace("OBJECT before:\n{}", new Object[]{prismObject.debugDump()}); @@ -1251,6 +1272,11 @@ private void modifyObjectAttempt(Class type, String oi if (LOGGER.isTraceEnabled()) { LOGGER.trace("OBJECT after:\n{}", prismObject.debugDump()); } + + // Continuing the photo treatment: should we remove the (now obsolete) user photo? + // We have to test prismObject at this place, because updateFullObject (below) removes photo property from the prismObject. + boolean shouldPhotoBeRemoved = containsUserPhotoModification && ((UserType) prismObject.asObjectable()).getJpegPhoto() == null; + // merge and update object LOGGER.trace("Translating JAXB to data type."); RObject rObject = createDataObjectFromJAXB(prismObject, PrismIdentifierGenerator.Operation.MODIFY); @@ -1265,6 +1291,15 @@ private void modifyObjectAttempt(Class type, String oi getClosureManager().updateOrgClosure(originalObject, modifications, session, oid, type, OrgClosureManager.Operation.MODIFY, closureContext); } + // JpegPhoto cleanup: As said before, if a user has to have no photo (after modifications are applied), + // we have to remove the photo manually. + if (shouldPhotoBeRemoved) { + Query query = session.createQuery("delete RUserPhoto where ownerOid = :oid"); + query.setParameter("oid", prismObject.getOid()); + query.executeUpdate(); + LOGGER.trace("User photo for {} was deleted", prismObject.getOid()); + } + LOGGER.trace("Before commit..."); session.getTransaction().commit(); LOGGER.trace("Committed!"); @@ -1318,6 +1353,20 @@ private Collection filterLookupTable return tableDelta; } + private boolean containsPhotoModification(Collection modifications) { + ItemPath photoPath = new ItemPath(UserType.F_JPEG_PHOTO); + for (ItemDelta delta : modifications) { + ItemPath path = delta.getPath(); + if (path.isEmpty()) { + throw new UnsupportedOperationException("User cannot be modified via empty-path modification"); + } else if (photoPath.isSubPathOrEquivalent(path)) { // actually, "subpath" variant should not occur + return true; + } + } + + return false; + } + private void cleanupClosureAndSessionAndResult(final OrgClosureManager.Context closureContext, final Session session, final OperationResult result) { if (closureContext != null) { getClosureManager().cleanUpAfterOperation(closureContext, session); diff --git a/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/data/common/RUser.java b/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/data/common/RUser.java index 8b65b0e2761..7beb89fd2c0 100644 --- a/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/data/common/RUser.java +++ b/repo/repo-sql-impl/src/main/java/com/evolveum/midpoint/repo/sql/data/common/RUser.java @@ -201,7 +201,11 @@ public boolean isHasPhoto() { return hasPhoto; } - @OneToMany(mappedBy = "owner", orphanRemoval = true) + // setting orphanRemoval = false prevents: + // (1) deletion of photos for RUsers that have no photos fetched (because fetching is lazy) + // (2) even querying of m_user_photo table on RUser merge + // (see comments in SqlRepositoryServiceImpl.modifyObjectAttempt) + @OneToMany(mappedBy = "owner", orphanRemoval = false) @Cascade({org.hibernate.annotations.CascadeType.ALL}) public Set getJpegPhoto() { if (jpegPhoto == null) {