From 0fcc2dd1affa558be9aa28f91f3a12338066d640 Mon Sep 17 00:00:00 2001 From: Tom Page Date: Fri, 22 Apr 2016 11:47:43 +0100 Subject: [PATCH 1/3] Public API consistency tests. Check that the classes we've declared as AlfrescoPublicApi do not require use of non-public API Alfresco classes. --- pom.xml | 9 +- rm-community/rm-community-repo/pom.xml | 5 + .../api/CommunityPublicAPIUnitTest.java | 42 ++ .../api/PublicAPITestUtil.java | 386 ++++++++++++++++++ 4 files changed, 440 insertions(+), 2 deletions(-) create mode 100644 rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/CommunityPublicAPIUnitTest.java create mode 100644 rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java diff --git a/pom.xml b/pom.xml index dea0feddf9..3b65a8e9c4 100644 --- a/pom.xml +++ b/pom.xml @@ -74,6 +74,11 @@ pom import + + org.reflections + reflections + 0.9.10 + @@ -262,7 +267,7 @@ sql-maven-plugin 1.5 - + org.apache.maven.plugins @@ -272,7 +277,7 @@ V@{project.version} - + org.apache.maven.plugins diff --git a/rm-community/rm-community-repo/pom.xml b/rm-community/rm-community-repo/pom.xml index 0d3dbe1d7d..9f62f72a96 100644 --- a/rm-community/rm-community-repo/pom.xml +++ b/rm-community/rm-community-repo/pom.xml @@ -347,6 +347,11 @@ test + + org.reflections + reflections + test + diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/CommunityPublicAPIUnitTest.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/CommunityPublicAPIUnitTest.java new file mode 100644 index 0000000000..a9f9c282ce --- /dev/null +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/CommunityPublicAPIUnitTest.java @@ -0,0 +1,42 @@ +/* + * #%L + * Alfresco Records Management Module + * %% + * Copyright (C) 2005 - 2016 Alfresco Software Limited + * %% + * This file is part of the Alfresco software. + * - + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is + * provided under the following open source license terms: + * - + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * - + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * - + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + * #L% + */ + +package org.alfresco.module.org_alfresco_module_rm.api; + +import com.google.common.collect.HashMultimap; + +import org.junit.Test; + +public class CommunityPublicAPIUnitTest +{ + @Test + public void testPublicAPIConsistency() + { + HashMultimap, Class> knownBadReferences = HashMultimap.create(); + PublicAPITestUtil.testPublicAPIConsistency("org.alfresco.module.org_alfresco_module_rm", knownBadReferences); + } +} diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java new file mode 100644 index 0000000000..41b59751c0 --- /dev/null +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java @@ -0,0 +1,386 @@ +/* + * #%L + * Alfresco Records Management Module + * %% + * Copyright (C) 2005 - 2016 Alfresco Software Limited + * %% + * This file is part of the Alfresco software. + * - + * If the software was purchased under a paid Alfresco license, the terms of + * the paid license agreement will prevail. Otherwise, the software is + * provided under the following open source license terms: + * - + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * - + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * - + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + * #L% + */ + +package org.alfresco.module.org_alfresco_module_rm.api; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.lang.reflect.AnnotatedElement; +import java.lang.reflect.Constructor; +import java.lang.reflect.Executable; +import java.lang.reflect.Field; +import java.lang.reflect.GenericArrayType; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; +import java.lang.reflect.WildcardType; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import com.google.common.collect.HashMultimap; +import com.google.common.collect.SetMultimap; +import com.google.common.collect.Sets; + +import org.alfresco.api.AlfrescoPublicApi; +import org.apache.commons.lang3.StringUtils; +import org.reflections.Reflections; + +/** + * A utility class to help testing the Alfresco public API. + * + * @author Tom Page + * @since 2.5 + */ +public class PublicAPITestUtil +{ + private static final String ALFRESCO_PACKAGE = "org.alfresco"; + + /** + * Check the consistency of the public API exposed from the given package. For each class in the package that is + * annotated {@link AlfrescoPublicApi}, check that no exposed methods (or fields, constructors, etc.) use + * non-public-API classes from Alfresco. + * + * @param basePackageName The package to check classes within. + * @param knownBadReferences Any references that would cause this test to fail, but which we don't want to change. + * The keys should be public API classes within our code and the values should be the non-public-API + * class that is being referenced. + */ + public static void testPublicAPIConsistency(String basePackageName, SetMultimap, Class> knownBadReferences) + { + Reflections reflections = new Reflections(basePackageName); + Set> publicAPIClasses = reflections.getTypesAnnotatedWith(AlfrescoPublicApi.class, true); + + SetMultimap, Class> referencedFrom = HashMultimap.create(); + Set> referencedClasses = new HashSet<>(); + for (Class publicAPIClass : publicAPIClasses) + { + Set> referencedClassesFromClass = getReferencedClassesFromClass(publicAPIClass, new HashSet<>()); + referencedClassesFromClass.forEach(clazz -> referencedFrom.put(clazz, publicAPIClass)); + + // Remove any references in knownBadReferences and error if an expected reference wasn't found. + if (knownBadReferences.containsKey(publicAPIClass)) + { + for (Class clazz : knownBadReferences.get(publicAPIClass)) + { + assertTrue("Supplied knownBadReferences expects " + clazz + " to be referenced by " + publicAPIClass + + ", but no such error was found", referencedClassesFromClass.remove(clazz)); + } + } + + referencedClasses.addAll(referencedClassesFromClass); + } + + List errorMessages = new ArrayList<>(); + for (Class referencedClass : referencedClasses) + { + if (isInAlfresco(referencedClass) && !isPartOfPublicApi(referencedClass)) + { + Set referencerNames = referencedFrom.get(referencedClass).stream().map(c -> c.getName()) + .collect(Collectors.toSet()); + errorMessages.add(referencedClass.getName() + " <- " + StringUtils.join(referencerNames, ", ")); + } + } + + if (!errorMessages.isEmpty()) + { + System.out.println("Errors found:"); + System.out.println(StringUtils.join(errorMessages, "\n")); + } + + assertEquals("Found references to non-public API classes from public API classes.", Collections.emptyList(), + errorMessages); + } + + /** + * Check if the given class is a part of the Alfresco public API. + * + * @param clazz The class to check. + * @return {@code true} if the given class is annotated with {@link AlfrescoPublicApi}. + */ + private static boolean isPartOfPublicApi(Class clazz) + { + if (clazz.getAnnotation(AlfrescoPublicApi.class) != null) + { + return true; + } + if (clazz.getEnclosingClass() != null) + { + return isPartOfPublicApi(clazz.getEnclosingClass()); + } + return false; + } + + /** + * Get all the classes referenced by the given class, which might be used by an extension. We consider visible + * methods, constructors, fields and inner classes, as well as superclasses and interfaces extended by the class. + * + * @param initialClass The class to analyse. + * @param consideredClasses Classes that have already been considered, and which should not be considered again. If + * the given class has already been considered then an empty set will be returned. This set will be + * updated with the given class. + * @return The set of classes that might be accessible by an extension of this class. + */ + private static Set> getReferencedClassesFromClass(Class initialClass, Set> consideredClasses) + { + Set> referencedClasses = new HashSet<>(); + + if (consideredClasses.add(initialClass)) + { + for (Method method : initialClass.getDeclaredMethods()) + { + if (isVisibleToExtender(method.getModifiers()) && !isDeprecated(method)) + { + referencedClasses.addAll(getClassesFromMethod(method)); + } + } + for (Constructor constructor : initialClass.getDeclaredConstructors()) + { + if (isVisibleToExtender(constructor.getModifiers()) && !isDeprecated(constructor)) + { + referencedClasses.addAll(getClassesFromConstructor(constructor)); + } + } + for (Field field : initialClass.getDeclaredFields()) + { + if (isVisibleToExtender(field.getModifiers()) && !isDeprecated(field)) + { + referencedClasses.addAll(getClassesFromField(field)); + } + } + for (Class clazz : initialClass.getDeclaredClasses()) + { + if (isVisibleToExtender(clazz.getModifiers()) && !isDeprecated(clazz)) + { + referencedClasses.addAll(getReferencedClassesFromClass(clazz, consideredClasses)); + } + } + if (initialClass.getSuperclass() != null && !isDeprecated(initialClass.getSuperclass())) + { + referencedClasses + .addAll(getReferencedClassesFromClass(initialClass.getSuperclass(), consideredClasses)); + } + for (Class clazz : initialClass.getInterfaces()) + { + if (!isDeprecated(clazz)) + { + referencedClasses.addAll(getReferencedClassesFromClass(clazz, consideredClasses)); + } + } + } + return referencedClasses; + } + + /** + * Check if the supplied {@link Executable#getModifiers() modifiers} indicate that an extension can access the + * element. Here we assume that an extension can see public and protected items, but not package protected (or + * private). + * + * @param modifiers The java language modifiers. + * @return {@code true} if the item is visible to an extension. + */ + private static boolean isVisibleToExtender(int modifiers) + { + return Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers); + } + + /** + * Get all classes involved in the signature of the given method. + * + * @param method The method to analyse. + * @return The set of classes. + */ + private static Set> getClassesFromMethod(Method method) + { + Set types = getTypesFromMethod(method); + return getClassesFromTypes(types); + } + + /** + * Get all classes involved in the signature of the given constructor. + * + * @param constructor The constructor to analyse. + * @return The set of classes. + */ + private static Set> getClassesFromConstructor(Constructor constructor) + { + Set types = getTypesFromConstructor(constructor); + return getClassesFromTypes(types); + } + + /** + * Get all classes involved in the type of the supplied field. For example {@code Pair, Integer> foo} + * involves four classes. + * + * @param field The field to look at. + * @return The set of classes. + */ + private static Set> getClassesFromField(Field field) + { + Set types = Sets.newHashSet(field.getGenericType()); + return getClassesFromTypes(types); + } + + /** + * Get all types references by the supplied method signature (i.e. the parameters, return type and exceptions). + * + * @param method The method to analyse. + * @return The set of types. + */ + private static Set getTypesFromMethod(Method method) + { + Set methodTypes = new HashSet<>(); + methodTypes.addAll(Sets.newHashSet(method.getGenericParameterTypes())); + methodTypes.add(method.getGenericReturnType()); + methodTypes.addAll(Sets.newHashSet(method.getGenericExceptionTypes())); + return methodTypes; + } + + /** + * Get all types referenced by the supplied constructor (i.e. the parameters and exceptions). + * + * @param constructor The constructor to analyse. + * @return The set of types. + */ + private static Set getTypesFromConstructor(Constructor constructor) + { + Set methodTypes = new HashSet<>(); + methodTypes.addAll(Sets.newHashSet(constructor.getGenericParameterTypes())); + methodTypes.addAll(Sets.newHashSet(constructor.getGenericExceptionTypes())); + return methodTypes; + } + + /** + * Find all classes that are within the supplied types. For example a {@code Pair, Integer>} contains + * references to four classes. + * + * @param methodTypes The set of types to examine. + * @return The set of classes used to form the given types. + */ + private static Set> getClassesFromTypes(Set methodTypes) + { + Set> methodClasses = new HashSet<>(); + for (Type type : methodTypes) + { + methodClasses.addAll(getClassesFromType(type, new HashSet<>())); + } + return methodClasses; + } + + /** + * Find all classes that are within the supplied type. For example a {@code Pair, Integer>} contains + * references to four classes. + * + * @param type The type to examine. + * @param processedTypes The set of types which have already been processed. If {@code type} is within this set then + * the method returns an empty set, to prevent analysis of the same type multiple times, and to guard + * against circular references. The underlying set is updated with the given type. + * @return The set of classes used to form the given type. + */ + private static Set> getClassesFromType(Type type, Set processedTypes) + { + Set> returnClasses = new HashSet<>(); + + if (processedTypes.add(type)) + { + if (type instanceof ParameterizedType) + { + ParameterizedType parameterizedType = (ParameterizedType) type; + returnClasses.add((Class) parameterizedType.getRawType()); + + for (Type t : parameterizedType.getActualTypeArguments()) + { + returnClasses.addAll(getClassesFromType(t, processedTypes)); + } + } + else if (type instanceof Class) + { + Class clazz = (Class) type; + if (clazz.isArray()) + { + returnClasses.add(clazz.getComponentType()); + } + returnClasses.add(clazz); + } + else if (type instanceof WildcardType) + { + // No-op - Caller can choose what type to use. + } + else if (type instanceof TypeVariable) + { + TypeVariable typeVariable = (TypeVariable) type; + for (Type bound : typeVariable.getBounds()) + { + returnClasses.addAll(getClassesFromType(bound, processedTypes)); + } + } + else if (type instanceof GenericArrayType) + { + GenericArrayType genericArrayType = (GenericArrayType) type; + returnClasses.addAll(getClassesFromType(genericArrayType.getGenericComponentType(), processedTypes)); + } + else + { + throw new IllegalStateException("This test was not written to work with type " + type); + } + } + return returnClasses; + } + + /** + * Check if the element is deprecated. Deprecated elements do not need to be part of the public API, even if they + * are visible to extenders. + * + * @param element The element to check. + * @return {@code true} if the element has been annotated as deprecated. + */ + private static boolean isDeprecated(AnnotatedElement element) + { + return (element.getDeclaredAnnotation(Deprecated.class) != null); + } + + /** + * Check if a class is within org.alfresco, and so whether it could potentially be part of the public API. + * + * @param type The class to check. + * @return {@code true} if this is an Alfresco class. + */ + private static boolean isInAlfresco(Class type) + { + if (type.getPackage() == null) + { + return false; + } + return type.getPackage().getName().startsWith(ALFRESCO_PACKAGE); + } +} From 93143fe5d6a607515154c133630c51a19f6d7cff Mon Sep 17 00:00:00 2001 From: Tom Page Date: Fri, 29 Apr 2016 10:07:04 +0100 Subject: [PATCH 2/3] RM-3345 Ensure the current user is not setting their own clearance. Also add unit tests for admin losing clearance and guest gaining clearance. --- .../test/util/MockAuthenticationUtilHelper.java | 1 + 1 file changed, 1 insertion(+) diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/test/util/MockAuthenticationUtilHelper.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/test/util/MockAuthenticationUtilHelper.java index e7372e5585..5e5308307b 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/test/util/MockAuthenticationUtilHelper.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/test/util/MockAuthenticationUtilHelper.java @@ -99,5 +99,6 @@ public class MockAuthenticationUtilHelper when(mockAuthenticationUtil.getFullyAuthenticatedUser()).thenReturn(fullyAuthenticatedUser); when(mockAuthenticationUtil.getRunAsUser()).thenReturn(fullyAuthenticatedUser); when(mockAuthenticationUtil.getSystemUserName()).thenReturn("system"); + when(mockAuthenticationUtil.getGuestUserName()).thenReturn("guest"); } } From b599e7d102aaaf69a97fc88e834854a1dfac882f Mon Sep 17 00:00:00 2001 From: Tom Page Date: Wed, 4 May 2016 14:34:18 +0100 Subject: [PATCH 3/3] RM-2812 Incompatibility in deprecated methods is still a problem. Remove the checks for deprecation, because deprecated methods/classes may still be used. --- .../api/PublicAPITestUtil.java | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java index 41b59751c0..389a60c9db 100644 --- a/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java +++ b/rm-community/rm-community-repo/unit-test/java/org/alfresco/module/org_alfresco_module_rm/api/PublicAPITestUtil.java @@ -30,7 +30,6 @@ package org.alfresco.module.org_alfresco_module_rm.api; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; -import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Constructor; import java.lang.reflect.Executable; import java.lang.reflect.Field; @@ -159,43 +158,40 @@ public class PublicAPITestUtil { for (Method method : initialClass.getDeclaredMethods()) { - if (isVisibleToExtender(method.getModifiers()) && !isDeprecated(method)) + if (isVisibleToExtender(method.getModifiers())) { referencedClasses.addAll(getClassesFromMethod(method)); } } for (Constructor constructor : initialClass.getDeclaredConstructors()) { - if (isVisibleToExtender(constructor.getModifiers()) && !isDeprecated(constructor)) + if (isVisibleToExtender(constructor.getModifiers())) { referencedClasses.addAll(getClassesFromConstructor(constructor)); } } for (Field field : initialClass.getDeclaredFields()) { - if (isVisibleToExtender(field.getModifiers()) && !isDeprecated(field)) + if (isVisibleToExtender(field.getModifiers())) { referencedClasses.addAll(getClassesFromField(field)); } } for (Class clazz : initialClass.getDeclaredClasses()) { - if (isVisibleToExtender(clazz.getModifiers()) && !isDeprecated(clazz)) + if (isVisibleToExtender(clazz.getModifiers())) { referencedClasses.addAll(getReferencedClassesFromClass(clazz, consideredClasses)); } } - if (initialClass.getSuperclass() != null && !isDeprecated(initialClass.getSuperclass())) + if (initialClass.getSuperclass() != null) { referencedClasses .addAll(getReferencedClassesFromClass(initialClass.getSuperclass(), consideredClasses)); } for (Class clazz : initialClass.getInterfaces()) { - if (!isDeprecated(clazz)) - { - referencedClasses.addAll(getReferencedClassesFromClass(clazz, consideredClasses)); - } + referencedClasses.addAll(getReferencedClassesFromClass(clazz, consideredClasses)); } } return referencedClasses; @@ -357,18 +353,6 @@ public class PublicAPITestUtil return returnClasses; } - /** - * Check if the element is deprecated. Deprecated elements do not need to be part of the public API, even if they - * are visible to extenders. - * - * @param element The element to check. - * @return {@code true} if the element has been annotated as deprecated. - */ - private static boolean isDeprecated(AnnotatedElement element) - { - return (element.getDeclaredAnnotation(Deprecated.class) != null); - } - /** * Check if a class is within org.alfresco, and so whether it could potentially be part of the public API. *