Skip to content

Commit

Permalink
Fix #2482: Warn about invalid Java migrations
Browse files Browse the repository at this point in the history
  • Loading branch information
Alex Tercete committed Sep 11, 2019
1 parent ce44005 commit ba24c3c
Show file tree
Hide file tree
Showing 10 changed files with 66 additions and 52 deletions.
9 changes: 6 additions & 3 deletions flyway-core/src/main/java/org/flywaydb/core/Flyway.java
Expand Up @@ -23,6 +23,7 @@
import org.flywaydb.core.api.configuration.FluentConfiguration;
import org.flywaydb.core.api.logging.Log;
import org.flywaydb.core.api.logging.LogFactory;
import org.flywaydb.core.api.migration.JavaMigration;
import org.flywaydb.core.api.resolver.MigrationResolver;
import org.flywaydb.core.internal.callback.CallbackExecutor;
import org.flywaydb.core.internal.callback.DefaultCallbackExecutor;
Expand Down Expand Up @@ -371,7 +372,7 @@ public Void execute(MigrationResolver migrationResolver,
* @return A new, fully configured, MigrationResolver instance.
*/
private MigrationResolver createMigrationResolver(ResourceProvider resourceProvider,
ClassProvider classProvider,
ClassProvider<JavaMigration> classProvider,
SqlScriptExecutorFactory sqlScriptExecutorFactory,
SqlScriptFactory sqlScriptFactory) {
return new CompositeMigrationResolver(resourceProvider, classProvider, configuration,
Expand Down Expand Up @@ -410,12 +411,14 @@ private MigrationResolver createMigrationResolver(ResourceProvider resourceProvi


final ResourceProvider resourceProvider;
ClassProvider classProvider;
ClassProvider<JavaMigration> classProvider;
if (!scannerRequired && configuration.isSkipDefaultResolvers() && configuration.isSkipDefaultCallbacks()) {
resourceProvider = NoopResourceProvider.INSTANCE;
//noinspection unchecked
classProvider = NoopClassProvider.INSTANCE;
} else {
Scanner scanner = new Scanner(
Scanner<JavaMigration> scanner = new Scanner<>(
JavaMigration.class,
Arrays.asList(configuration.getLocations()),
configuration.getClassLoader(),
configuration.getEncoding()
Expand Down
Expand Up @@ -20,12 +20,11 @@
/**
* A facility to obtain classes.
*/
public interface ClassProvider {
public interface ClassProvider<I> {
/**
* Retrieve all classes which implement this interface.
* Retrieve all classes which implement the specified interface.
*
* @param implementedInterface The interface the matching classes should implement.
* @return The non-abstract classes that were found.
*/
<I> Collection<Class<? extends I>> getClasses(Class<I> implementedInterface);
Collection<Class<? extends I>> getClasses();
}
Expand Up @@ -25,7 +25,7 @@ public enum NoopClassProvider implements ClassProvider {
INSTANCE;

@Override
public <I> Collection<Class<? extends I>> getClasses(Class<I> implementedInterface) {
public Collection<Class<?>> getClasses() {
return Collections.emptyList();
}
}
Expand Up @@ -17,6 +17,7 @@

import org.flywaydb.core.api.FlywayException;
import org.flywaydb.core.api.configuration.Configuration;
import org.flywaydb.core.api.migration.JavaMigration;
import org.flywaydb.core.api.resolver.Context;
import org.flywaydb.core.api.resolver.MigrationResolver;
import org.flywaydb.core.api.resolver.ResolvedMigration;
Expand Down Expand Up @@ -62,7 +63,7 @@ public class CompositeMigrationResolver implements MigrationResolver {
* @param customMigrationResolvers Custom Migration Resolvers.
*/
public CompositeMigrationResolver(ResourceProvider resourceProvider,
ClassProvider classProvider,
ClassProvider<JavaMigration> classProvider,
Configuration configuration,
SqlScriptExecutorFactory sqlScriptExecutorFactory,
SqlScriptFactory sqlScriptFactory,
Expand Down
Expand Up @@ -36,7 +36,7 @@ public class ScanningJavaMigrationResolver implements MigrationResolver {
/**
* The Scanner to use.
*/
private final ClassProvider classProvider;
private final ClassProvider<JavaMigration> classProvider;

/**
* The configuration to inject (if necessary) in the migration classes.
Expand All @@ -49,7 +49,7 @@ public class ScanningJavaMigrationResolver implements MigrationResolver {
* @param classProvider The class provider.
* @param configuration The configuration to inject (if necessary) in the migration classes.
*/
public ScanningJavaMigrationResolver(ClassProvider classProvider, Configuration configuration) {
public ScanningJavaMigrationResolver(ClassProvider<JavaMigration> classProvider, Configuration configuration) {
this.classProvider = classProvider;
this.configuration = configuration;
}
Expand All @@ -58,7 +58,7 @@ public ScanningJavaMigrationResolver(ClassProvider classProvider, Configuration
public List<ResolvedMigration> resolveMigrations(Context context) {
List<ResolvedMigration> migrations = new ArrayList<>();

for (Class<?> clazz : classProvider.getClasses(JavaMigration.class)) {
for (Class<?> clazz : classProvider.getClasses()) {
JavaMigration javaMigration = ClassUtils.instantiate(clazz.getName(), configuration.getClassLoader());
migrations.add(new ResolvedJavaMigration(javaMigration));
}
Expand Down
Expand Up @@ -31,18 +31,19 @@
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;

/**
* Scanner for Resources and Classes.
*/
public class Scanner implements ResourceProvider, ClassProvider {
public class Scanner<I> implements ResourceProvider, ClassProvider<I> {
private static final Log LOG = LogFactory.getLog(Scanner.class);

private final List<LoadableResource> resources = new ArrayList<>();
private final List<Class<?>> classes = new ArrayList<>();
private final List<Class<? extends I>> classes = new ArrayList<>();

public Scanner(Collection<Location> locations, ClassLoader classLoader, Charset encoding
public Scanner(Class<I> implementedInterface, Collection<Location> locations, ClassLoader classLoader, Charset encoding



Expand All @@ -59,9 +60,9 @@ public Scanner(Collection<Location> locations, ClassLoader classLoader, Charset
if (location.isFileSystem()) {
resources.addAll(fileSystemScanner.scanForResources(location));
} else {
ResourceAndClassScanner resourceAndClassScanner = android
? new AndroidScanner(classLoader, encoding, location)
: new ClassPathScanner(classLoader, encoding, location);
ResourceAndClassScanner<I> resourceAndClassScanner = android
? new AndroidScanner<>(implementedInterface, classLoader, encoding, location)
: new ClassPathScanner<>(implementedInterface, classLoader, encoding, location);
resources.addAll(resourceAndClassScanner.scanForResources());
classes.addAll(resourceAndClassScanner.scanForClasses());
}
Expand Down Expand Up @@ -100,22 +101,12 @@ public Collection<LoadableResource> getResources(String prefix, String... suffix
}

/**
* Scans the classpath for concrete classes under the specified package implementing this interface.
* Scans the classpath for concrete classes under the specified package implementing the specified interface.
* Non-instantiable abstract classes are filtered out.
*
* @param implementedInterface The interface the matching classes should implement.
* @return The non-abstract classes that were found.
*/
public <I> Collection<Class<? extends I>> getClasses(Class<I> implementedInterface) {
List<Class<? extends I>> result = new ArrayList<>();
for (Class<?> clazz : classes) {
if (!implementedInterface.isAssignableFrom(clazz)) {
continue;
}

//noinspection unchecked
result.add((Class<? extends I>) clazz);
}
return result;
public Collection<Class<? extends I>> getClasses() {
return Collections.unmodifiableCollection(classes);
}
}
Expand Up @@ -37,16 +37,18 @@
/**
* Class & resource scanner for Android.
*/
public class AndroidScanner implements ResourceAndClassScanner {
public class AndroidScanner<I> implements ResourceAndClassScanner<I> {
private static final Log LOG = LogFactory.getLog(AndroidScanner.class);

private final Context context;

private final Class<I> implementedInterface;
private final ClassLoader clazzLoader;
private final Charset encoding;
private final Location location;

public AndroidScanner(ClassLoader clazzLoader, Charset encoding, Location location) {
public AndroidScanner(Class<I> implementedInterface, ClassLoader clazzLoader, Charset encoding, Location location) {
this.implementedInterface = implementedInterface;
this.clazzLoader = clazzLoader;
this.encoding = encoding;
this.location = location;
Expand Down Expand Up @@ -74,10 +76,10 @@ public Collection<LoadableResource> scanForResources() {
}

@Override
public Collection<Class<?>> scanForClasses() {
public Collection<Class<? extends I>> scanForClasses() {
String pkg = location.getPath().replace("/", ".");

List<Class<?>> classes = new ArrayList<>();
List<Class<? extends I>> classes = new ArrayList<>();
String sourceDir = context.getApplicationInfo().sourceDir;
DexFile dex = null;
try {
Expand All @@ -86,7 +88,7 @@ public Collection<Class<?>> scanForClasses() {
while (entries.hasMoreElements()) {
String className = entries.nextElement();
if (className.startsWith(pkg)) {
Class<?> clazz = ClassUtils.loadClass(className, clazzLoader);
Class<? extends I> clazz = ClassUtils.loadClass(implementedInterface, className, clazzLoader);
if (clazz != null) {
classes.add(clazz);
}
Expand Down
Expand Up @@ -39,9 +39,10 @@
/**
* ClassPath scanner.
*/
public class ClassPathScanner implements ResourceAndClassScanner {
public class ClassPathScanner<I> implements ResourceAndClassScanner<I> {
private static final Log LOG = LogFactory.getLog(ClassPathScanner.class);

private final Class<I> implementedInterface;
/**
* The ClassLoader for loading migrations on the classpath.
*/
Expand Down Expand Up @@ -70,7 +71,8 @@ public class ClassPathScanner implements ResourceAndClassScanner {
*
* @param classLoader The ClassLoader for loading migrations on the classpath.
*/
public ClassPathScanner(ClassLoader classLoader, Charset encoding, Location location) {
public ClassPathScanner(Class<I> implementedInterface, ClassLoader classLoader, Charset encoding, Location location) {
this.implementedInterface = implementedInterface;
this.classLoader = classLoader;
this.location = location;

Expand All @@ -87,14 +89,17 @@ public Collection<LoadableResource> scanForResources() {
}

@Override
public Collection<Class<?>> scanForClasses() {
public Collection<Class<? extends I>> scanForClasses() {
LOG.debug("Scanning for classes at " + location);

List<Class<?>> classes = new ArrayList<>();
List<Class<? extends I>> classes = new ArrayList<>();

for (LoadableResource resource : resources) {
if (resource.getAbsolutePath().endsWith(".class")) {
Class<?> clazz = ClassUtils.loadClass(toClassName(resource.getAbsolutePath()), classLoader);
Class<? extends I> clazz = ClassUtils.loadClass(
implementedInterface,
toClassName(resource.getAbsolutePath()),
classLoader);
if (clazz != null) {
classes.add(clazz);
}
Expand Down
Expand Up @@ -22,7 +22,7 @@
/**
* Scanner for both resources and classes.
*/
public interface ResourceAndClassScanner {
public interface ResourceAndClassScanner<I> {
/**
* Scans the classpath for resources under the configured location.
*
Expand All @@ -31,10 +31,10 @@ public interface ResourceAndClassScanner {
Collection<LoadableResource> scanForResources();

/**
* Scans the classpath for concrete classes under the specified package implementing this interface.
* Scans the classpath for concrete classes under the specified package implementing the specified interface.
* Non-instantiable abstract classes are filtered out.
*
* @return The non-abstract classes that were found.
*/
Collection<Class<?>> scanForClasses();
Collection<Class<? extends I>> scanForClasses();
}
Expand Up @@ -117,31 +117,44 @@ public static boolean isPresent(String className, ClassLoader classLoader) {
/**
* Loads the class with this name using the class loader.
*
* @param className The name of the class to load.
* @param classLoader The ClassLoader to use.
* @param implementedInterface The interface the class is expected to implement.
* @param className The name of the class to load.
* @param classLoader The ClassLoader to use.
* @return the newly loaded class or {@code null} if it could not be loaded.
*/
public static Class<?> loadClass(String className, ClassLoader classLoader) {
public static <I> Class<? extends I> loadClass(Class<I> implementedInterface, String className, ClassLoader classLoader) {
try {
Class<?> clazz = classLoader.loadClass(className);

if (!implementedInterface.isAssignableFrom(clazz)) {
return null;
}

if (Modifier.isAbstract(clazz.getModifiers()) || clazz.isEnum() || clazz.isAnonymousClass()) {
LOG.debug("Skipping non-instantiable class: " + className);
return null;
}

clazz.getDeclaredConstructor().newInstance();
LOG.debug("Found class: " + className);
return clazz;
//noinspection unchecked
return (Class<? extends I>) clazz;
} catch (Throwable e) {
Throwable rootCause = ExceptionUtils.getRootCause(e);
LOG.debug("Skipping " + className + " (" + e.getClass().getSimpleName() + "): " + e.getMessage()
+ (rootCause == e ? "" :
" caused by " + rootCause.getClass().getSimpleName() + ": " + rootCause.getMessage()
+ " at " + ExceptionUtils.getThrowLocation(rootCause)));
LOG.warn("Skipping " + className + ": " + formatThrowable(e) + (
rootCause == e
? ""
: " caused by " + formatThrowable(rootCause)
+ " at " + ExceptionUtils.getThrowLocation(rootCause)
));
return null;
}
}

private static String formatThrowable(Throwable e) {
return "(" + e.getClass().getSimpleName() + ": " + e.getMessage() + ")";
}

/**
* Retrieves the physical location on disk of this class.
*
Expand Down

0 comments on commit ba24c3c

Please sign in to comment.