Merge branch '33-too-many-auditlogs-for-sets' into 'master'

Resolve "Too many AuditLogs for Sets"

Closes #33

See merge request genesys-pgr/application-blocks!45

auditlog/src/main/java/org/genesys/blocks/auditlog/component/AuditTrailInterceptor.java

@@ -427,7 +427,7 @@ public class AuditTrailInterceptor extends EmptyInterceptor implements Initializ
 			deleted = convertEntityId(deleted);
 		}
 
-		LOG.trace("prev=" + deleted.toString() + " curr=" + null);
+		LOG.trace("prev={} curr=null", deleted);
 		recordDelete(pc.getOwner(), (Long) key, propertyName, deleted.toString(), referencedEntity);
 	}
 
@@ -466,8 +466,10 @@ public class AuditTrailInterceptor extends EmptyInterceptor implements Initializ
 		Collection<?> previous = null;
 		final Serializable snap = pc.getStoredSnapshot();
 		if (snap instanceof Map<?, ?>) {
+			// LOG.trace("Snap keys: {}", ((Map) snap).keySet());
+			// LOG.trace("Snap vals: {}", ((Map) snap).values());
 			final Map<?, ?> snapMap = (Map<?, ?>) snap;
-			previous = snapMap.values();
+			previous = snapMap.keySet();
 		} else if (snap instanceof Collection<?>) {
 			final Collection<?> snapList = (Collection<?>) snap;
 			previous = snapList;
@@ -484,8 +486,29 @@ public class AuditTrailInterceptor extends EmptyInterceptor implements Initializ
 			remaining = convertEntityId(remaining);
 		}
 
-		LOG.trace("prev=" + previous + " curr=" + remaining);
-		recordChange(pc.getOwner(), (Long) key, propertyName, previous.toString(), remaining.toString(), referencedEntity);
+		LOG.trace("prev={} curr={}", previous, remaining);
+		recordChange(pc.getOwner(), (Long) key, propertyName, collectionToStringSorted(previous), collectionToStringSorted(remaining), referencedEntity);
+	}
+
+	
+	/**
+	 * Produce a string representation of the collection. Set elements are sorted,
+	 * other collection types return `#toString`.
+	 *
+	 * @param collection the collection
+	 * @return the string
+	 */
+	private String collectionToStringSorted(Collection<?> collection) {
+		if (collection == null) {
+			return null;
+		}
+		if (collection instanceof Set<?>) {
+			LOG.trace("Converting to sorted list {} -> {}", collection, collection.stream().sorted().map(Object::toString).collect(Collectors.joining(", ", "[", "]")));
+			return collection.stream().sorted().map(Object::toString).collect(Collectors.joining(", ", "[", "]"));
+		} else {
+			LOG.trace("Not sorting {}: {}", collection.getClass(), collection);
+		}
+		return collection.toString();
 	}
 
 	/**
@@ -501,7 +524,7 @@ public class AuditTrailInterceptor extends EmptyInterceptor implements Initializ
 		// Field
 		final Field field = ReflectionUtils.findField(class1, propertyName);
 		if (field != null) {
-			LOG.trace("Found field: " + field + "\n\ttype=" + field.getType() + "\n\tgeneric=" + field.getGenericType() + "\n\tgenericTN=" + field.getGenericType().getTypeName());
+			LOG.trace("Found field: {}\n\ttype={}\n\tgeneric={}\n\tgenericTN={}", field, field.getType(), field.getGenericType(), field.getGenericType().getTypeName());
 			final ResolvableType t = ResolvableType.forField(field, class1);
 			if (t.hasGenerics()) {
 				LOG.trace("\tResoved={} returning={}", t.toString(), t.resolveGeneric(0));
@@ -516,7 +539,7 @@ public class AuditTrailInterceptor extends EmptyInterceptor implements Initializ
 		try {
 			final Method method = class1.getMethod("get" + StringUtils.capitalize(propertyName));
 			if (method != null) {
-				LOG.trace("Didn't find field, found the method: " + method.getReturnType().getTypeParameters()[0]);
+				LOG.trace("Didn't find field, found the method: {}", method.getReturnType().getTypeParameters()[0]);
 			}
 		} catch (SecurityException | NoSuchMethodException e) {
 			e.printStackTrace();
@@ -785,12 +808,12 @@ public class AuditTrailInterceptor extends EmptyInterceptor implements Initializ
 		if (getter != null) {
 			final Object result = getter.invoke(entity);
 			if (result == null) {
-				LOG.trace(entity + " is transient, has " + methodName + " == null");
+				LOG.trace("{} is transient, has {} == null", entity, methodName);
 				return true;
 			} else if (result instanceof Number) {
 				final Number r = (Number) result;
 				if (r.longValue() < 0) {
-					LOG.trace(entity + " is transient, has " + methodName + " = " + result + " < 0");
+					LOG.trace("{} is transient, has {} = {} < 0", entity, methodName, result);
 					return true;
 				} else {
 					// LOG.trace(entity + " is not transient, has " + methodName + " = " + result +

auditlog/src/test/java/org/genesys/blocks/auditlog/service/AuditTrailServiceTest.java

@@ -242,15 +242,15 @@ public class AuditTrailServiceTest extends ServiceTest {
 		assertThat(lastLog.getPreviousState(), is("[1, 2, 3]"));
 		assertThat(lastLog.getNewState(), is("[2, 3, 4]"));
 
-		entity.getSet().clear();
+		entity.getSet().remove("piper");
 		entity = exampleAuditedEntityService.save(entity);
 		// printAuditLogs(entity);
 		assertThat(listAuditLogs(entity), hasSize(2));
 		lastLog = lastAuditLog(entity);
 		assertThat(lastLog.getAction(), is(AuditAction.UPDATE));
 		assertThat(lastLog.getPropertyName(), is("set"));
-		assertThat(lastLog.getPreviousState(), is("[zebra, banana, piper]"));
-		assertThat(lastLog.getNewState(), is("[]"));
+		assertThat(lastLog.getPreviousState(), is("[banana, piper, zebra]"));
+		assertThat(lastLog.getNewState(), is("[banana, zebra]"));
 
 		entity.getSet().clear();
 		entity.getSet().add("c++");
@@ -260,7 +260,7 @@ public class AuditTrailServiceTest extends ServiceTest {
 		lastLog = lastAuditLog(entity);
 		assertThat(lastLog.getAction(), is(AuditAction.UPDATE));
 		assertThat(lastLog.getPropertyName(), is("set"));
-		assertThat(lastLog.getPreviousState(), is("[]"));
+		assertThat(lastLog.getPreviousState(), is("[banana, zebra]"));
 		assertThat(lastLog.getNewState(), is("[c++]"));
 
 		assertThat(entity.getList(), hasSize(3));
@@ -274,7 +274,7 @@ public class AuditTrailServiceTest extends ServiceTest {
 		lastLog = lastAuditLog(entity);
 		assertThat(lastLog.getAction(), is(AuditAction.UPDATE));
 		assertThat(lastLog.getPropertyName(), is("set"));
-		assertThat(lastLog.getPreviousState(), is("[]"));
+		assertThat(lastLog.getPreviousState(), is("[banana, zebra]"));
 		assertThat(lastLog.getNewState(), is("[c++]"));
 
 	}