From 502e39f3974b03308e7a2a76f55b5b6dcfbaac93 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 18 Jun 2017 11:24:20 +0200 Subject: [PATCH] #48 Extract cyclic dependency check into a handler, fix optional request type --- .../AllInstancesAnnotationHandler.java | 2 +- .../ch/jalu/injector/InjectorBuilder.java | 2 + .../java/ch/jalu/injector/InjectorImpl.java | 164 +++++++++++------- .../injector/context/ObjectIdentifier.java | 8 + .../injector/context/ResolutionContext.java | 27 +++ .../CyclicDependenciesDetector.java | 50 ++++++ .../handlers/instantiation/Resolution.java | 6 +- .../instantiation/StandardInjection.java | 3 +- .../provider/ProviderHandlerImpl.java | 6 +- .../ch/jalu/injector/utils/InjectorUtils.java | 15 ++ .../ch/jalu/injector/InjectorImplTest.java | 17 -- .../instantiation/StandardInjectionTest.java | 9 - 12 files changed, 215 insertions(+), 94 deletions(-) create mode 100644 injector/src/main/java/ch/jalu/injector/handlers/dependency/CyclicDependenciesDetector.java diff --git a/injector-extras/src/main/java/ch/jalu/injector/extras/handlers/AllInstancesAnnotationHandler.java b/injector-extras/src/main/java/ch/jalu/injector/extras/handlers/AllInstancesAnnotationHandler.java index cfb2826..5b6da0a 100644 --- a/injector-extras/src/main/java/ch/jalu/injector/extras/handlers/AllInstancesAnnotationHandler.java +++ b/injector-extras/src/main/java/ch/jalu/injector/extras/handlers/AllInstancesAnnotationHandler.java @@ -82,7 +82,7 @@ public Object instantiateWith(Object... values) { } @Override - public boolean isNewlyCreated() { + public boolean isInstantiation() { return true; } } diff --git a/injector/src/main/java/ch/jalu/injector/InjectorBuilder.java b/injector/src/main/java/ch/jalu/injector/InjectorBuilder.java index e342c51..28f317d 100644 --- a/injector/src/main/java/ch/jalu/injector/InjectorBuilder.java +++ b/injector/src/main/java/ch/jalu/injector/InjectorBuilder.java @@ -1,6 +1,7 @@ package ch.jalu.injector; import ch.jalu.injector.handlers.Handler; +import ch.jalu.injector.handlers.dependency.CyclicDependenciesDetector; import ch.jalu.injector.handlers.dependency.FactoryDependencyHandler; import ch.jalu.injector.handlers.dependency.SavedAnnotationsHandler; import ch.jalu.injector.handlers.dependency.SingletonStoreDependencyHandler; @@ -45,6 +46,7 @@ public static List createDefaultHandlers(String rootPackage) { new FactoryDependencyHandler(), new SingletonStoreDependencyHandler(), // Instantiation provider + new CyclicDependenciesDetector(), new DefaultInjectionProvider(rootPackage), // PostConstruct new PostConstructMethodInvoker())); diff --git a/injector/src/main/java/ch/jalu/injector/InjectorImpl.java b/injector/src/main/java/ch/jalu/injector/InjectorImpl.java index 92ee9c6..6027006 100644 --- a/injector/src/main/java/ch/jalu/injector/InjectorImpl.java +++ b/injector/src/main/java/ch/jalu/injector/InjectorImpl.java @@ -15,15 +15,14 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import static ch.jalu.injector.context.StandardResolutionType.REQUEST_SCOPED; import static ch.jalu.injector.context.StandardResolutionType.REQUEST_SCOPED_IF_HAS_DEPENDENCIES; import static ch.jalu.injector.context.StandardResolutionType.SINGLETON; import static ch.jalu.injector.utils.InjectorUtils.checkNotNull; +import static ch.jalu.injector.utils.InjectorUtils.containsNullValue; import static ch.jalu.injector.utils.InjectorUtils.firstNotNull; import static ch.jalu.injector.utils.InjectorUtils.rethrowException; @@ -59,12 +58,12 @@ public void register(Class clazz, T object) { @Override public void provide(Class clazz, Object object) { checkNotNull(clazz, "Provided annotation may not be null"); - for (Handler handler : config.getHandlers()) { - try { + try { + for (Handler handler : config.getHandlers()) { handler.onAnnotation(clazz, object); - } catch (Exception e) { - rethrowException(e); } + } catch (Exception e) { + rethrowException(e); } } @@ -103,12 +102,12 @@ public Collection retrieveAllOfType(Class clazz) { public void registerProvider(Class clazz, Provider provider) { checkNotNull(clazz, "Class may not be null"); checkNotNull(provider, "Provider may not be null"); - for (Handler handler : config.getHandlers()) { - try { + try { + for (Handler handler : config.getHandlers()) { handler.onProvider(clazz, provider); - } catch (Exception e) { - rethrowException(e); } + } catch (Exception e) { + rethrowException(e); } } @@ -116,12 +115,12 @@ public void registerProvider(Class clazz, Provider provider) public > void registerProvider(Class clazz, Class

providerClass) { checkNotNull(clazz, "Class may not be null"); checkNotNull(providerClass, "Provider class may not be null"); - for (Handler handler : config.getHandlers()) { - try { + try { + for (Handler handler : config.getHandlers()) { handler.onProviderClass(clazz, providerClass); - } catch (Exception e) { - rethrowException(e); } + } catch (Exception e) { + rethrowException(e); } } @@ -131,13 +130,18 @@ public InjectorConfig getConfig() { @SuppressWarnings("unchecked") private T resolve(ResolutionType resolutionType, Class clazz) { - return (T) resolveObject( - new ResolutionContext(this, new ObjectIdentifier(resolutionType, clazz)), - new HashSet<>()); + return (T) resolveContext( + new ResolutionContext(this, new ObjectIdentifier(resolutionType, clazz))); } + /** + * Returns the object as defined by the given context. + * + * @param context the context to resolve the object for + * @return the resolved object, {@code null} if the context specifies it is optional and some criteria is not met + */ @Nullable - private Object resolveObject(ResolutionContext context, Set> traversedClasses) { + protected Object resolveContext(ResolutionContext context) { // TODO #49: Convert singleton store to a Handler impl. if (context.getIdentifier().getResolutionType() == StandardResolutionType.SINGLETON) { Object knownSingleton = objects.get(context.getIdentifier().getTypeAsClass()); @@ -147,34 +151,77 @@ private Object resolveObject(ResolutionContext context, Set> traversedC } Resolution resolution = findResolutionOrFail(context); + if (isContextChildOfOptionalRequest(context) && resolution.isInstantiation()) { + return null; + } - traversedClasses.add(context.getIdentifier().getTypeAsClass()); - validateInjectionHasNoCircularDependencies(resolution, traversedClasses); + Object[] resolvedDependencies = resolveDependencies(context, resolution); + if (containsNullValue(resolvedDependencies)) { + throwForUnexpectedNullDependency(context); + return null; + } - for (ObjectIdentifier identifier : resolution.getDependencies()) { - if (traversedClasses.contains(identifier.getTypeAsClass())) { - throw new InjectorException("Found cyclic dependency - already traversed '" - + identifier.getTypeAsClass() + "' (full traversal list: " + traversedClasses + ")"); - } + Object object = runPostConstructHandlers(resolution.instantiateWith(resolvedDependencies), context, resolution); + if (resolution.isInstantiation() && context.getIdentifier().getResolutionType() == SINGLETON) { + register((Class) context.getOriginalIdentifier().getTypeAsClass(), object); } + return object; + } - List dependencies = resolution.getDependencies(); - Object[] resolvedDependencies = dependencies.stream() - .map(identifier -> new ResolutionContext(this, identifier)) - .map(dependencyContext -> resolveObject(dependencyContext, new HashSet<>(traversedClasses))) - .toArray(); - Object object = resolution.instantiateWith(resolvedDependencies); - - if (resolution.isNewlyCreated()) { - object = runPostConstructHandlers(object, context, resolution); - if (context.getIdentifier().getResolutionType() == StandardResolutionType.SINGLETON) { - register((Class) context.getOriginalIdentifier().getTypeAsClass(), object); + /** + * Resolves the dependencies as defined by the given resolution. + * If a dependency is resolved to {@code null}, the process is aborted and the remaining dependencies + * are not resolved. + * + * @param context the resolution context + * @param resolution the resolution whose dependencies should be provided + * @return array with the dependencies, in the same order as given by the resolution + */ + protected Object[] resolveDependencies(ResolutionContext context, Resolution resolution) { + final int totalDependencies = resolution.getDependencies().size(); + final Object[] resolvedDependencies = new Object[totalDependencies]; + + int index = 0; + for (ObjectIdentifier dependencyId : resolution.getDependencies()) { + Object dependency = resolveContext(context.createChildContext(dependencyId)); + if (dependency == null) { + break; } + resolvedDependencies[index] = dependency; + ++index; } - return object; + return resolvedDependencies; + } + + /** + * Called when a resolved dependency is null, this method may throw an exception in the cases when this + * should not happen. If this method does not throw an exception, null is returned from {@link #resolveContext}. + * + * @param context the resolution context + */ + protected void throwForUnexpectedNullDependency(ResolutionContext context) { + if (context.getIdentifier().getResolutionType() == REQUEST_SCOPED_IF_HAS_DEPENDENCIES + || isContextChildOfOptionalRequest(context)) { + // Situation where null may occur, so throw no exception + return; + } + throw new InjectorException("Found null returned as dependency while resolving '" + + context.getIdentifier() + "'"); + } + + private static boolean isContextChildOfOptionalRequest(ResolutionContext context) { + return !context.getParents().isEmpty() + && context.getParents().get(0).getIdentifier().getResolutionType() == REQUEST_SCOPED_IF_HAS_DEPENDENCIES; } - private Resolution findResolutionOrFail(ResolutionContext context) { + /** + * Calls the defined handlers and returns the first {@link Resolution} that is returned based on + * the provided resolution context. Throws an exception if no handler returned a resolution. + * + * @param context the context to find the resolution for + * @return the resolution + */ + protected Resolution findResolutionOrFail(ResolutionContext context) { try { for (Handler handler : config.getHandlers()) { Resolution resolution = handler.resolve(context); @@ -203,32 +250,29 @@ private Resolution findResolutionOrFail(ResolutionContext context) { + "require the default constructor"); } - private T runPostConstructHandlers(T instance, ResolutionContext context, Resolution resolution) { - T object = instance; - for (Handler handler : config.getHandlers()) { - try { - object = firstNotNull(handler.postProcess(object, context, resolution), object); - } catch (Exception e) { - rethrowException(e); - } - } - return object; - } - /** - * Validates that none of the dependencies' types are present in the given collection - * of traversed classes. This prevents circular dependencies. + * Invokes the handler's post construct method when appropriate. Returns the object as returned by the + * handlers, which may be different from the provided one. * - * @param resolution the resolution method to get the dependencies from - * @param traversedClasses the collection of traversed classes + * @param instance the object that was resolved + * @param context the resolution context + * @param resolution the resolution used to get the object + * @param the object's type + * @return the object to use (as post construct methods may change it) */ - private static void validateInjectionHasNoCircularDependencies(Resolution resolution, - Set> traversedClasses) { - for (ObjectIdentifier identifier : resolution.getDependencies()) { - if (traversedClasses.contains(identifier.getTypeAsClass())) { - throw new InjectorException("Found cyclic dependency - already traversed '" - + identifier.getTypeAsClass() + "' (full traversal list: " + traversedClasses + ")"); + protected T runPostConstructHandlers(T instance, ResolutionContext context, Resolution resolution) { + if (!resolution.isInstantiation()) { + return instance; + } + + T object = instance; + try { + for (Handler handler : config.getHandlers()) { + object = firstNotNull(handler.postProcess(object, context, resolution), object); } + } catch (Exception e) { + rethrowException(e); } + return object; } } diff --git a/injector/src/main/java/ch/jalu/injector/context/ObjectIdentifier.java b/injector/src/main/java/ch/jalu/injector/context/ObjectIdentifier.java index 87d6236..d94d98a 100644 --- a/injector/src/main/java/ch/jalu/injector/context/ObjectIdentifier.java +++ b/injector/src/main/java/ch/jalu/injector/context/ObjectIdentifier.java @@ -23,6 +23,9 @@ public ObjectIdentifier(ResolutionType resolutionType, Type type, Annotation... this.annotations = Arrays.asList(annotations); } + /** + * @return the resolution type (scope) requested for the object + */ public ResolutionType getResolutionType() { return resolutionType; } @@ -69,4 +72,9 @@ public Class getTypeAsClass() { public List getAnnotations() { return annotations; } + + @Override + public String toString() { + return "ObjId[type=" + type + ", annotations=" + annotations + "]"; + } } diff --git a/injector/src/main/java/ch/jalu/injector/context/ResolutionContext.java b/injector/src/main/java/ch/jalu/injector/context/ResolutionContext.java index cdbe75a..460197e 100644 --- a/injector/src/main/java/ch/jalu/injector/context/ResolutionContext.java +++ b/injector/src/main/java/ch/jalu/injector/context/ResolutionContext.java @@ -3,6 +3,9 @@ import ch.jalu.injector.Injector; import ch.jalu.injector.exceptions.InjectorException; +import java.util.ArrayList; +import java.util.List; + /** * Resolution context: contains data about the object that is requested, such as identifying * information about the object to retrieve or construct and the context in which it is being @@ -13,7 +16,14 @@ public class ResolutionContext { private final Injector injector; private final ObjectIdentifier originalIdentifier; private ObjectIdentifier identifier; + private List parents = new ArrayList<>(); + /** + * Creates a new resolution context with no predecessors. + * + * @param injector the injector + * @param identifier the identifier of the object to create + */ public ResolutionContext(Injector injector, ObjectIdentifier identifier) { this.injector = injector; this.originalIdentifier = identifier; @@ -35,6 +45,10 @@ public ObjectIdentifier getIdentifier() { return identifier; } + public List getParents() { + return parents; + } + /** * Sets the class to instantiate an object of. * @@ -48,4 +62,17 @@ public void setIdentifier(ObjectIdentifier identifier) { + "' is not a child of original class '" + originalIdentifier.getTypeAsClass() + "'"); } } + + /** + * Creates a context for the given identifier with this context as parent. + * + * @param identifier the identifier to create a context for + * @return the child context + */ + public ResolutionContext createChildContext(ObjectIdentifier identifier) { + ResolutionContext child = new ResolutionContext(injector, identifier); + child.parents.addAll(this.parents); + child.parents.add(this); + return child; + } } diff --git a/injector/src/main/java/ch/jalu/injector/handlers/dependency/CyclicDependenciesDetector.java b/injector/src/main/java/ch/jalu/injector/handlers/dependency/CyclicDependenciesDetector.java new file mode 100644 index 0000000..6688497 --- /dev/null +++ b/injector/src/main/java/ch/jalu/injector/handlers/dependency/CyclicDependenciesDetector.java @@ -0,0 +1,50 @@ +package ch.jalu.injector.handlers.dependency; + +import ch.jalu.injector.context.ObjectIdentifier; +import ch.jalu.injector.context.ResolutionContext; +import ch.jalu.injector.exceptions.InjectorException; +import ch.jalu.injector.handlers.Handler; +import ch.jalu.injector.handlers.instantiation.Resolution; + +import javax.annotation.Nullable; +import java.lang.reflect.Type; +import java.util.HashSet; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Detects cycles in the dependencies based on the context's parents. + * This handler should come at the start of the chain so it can stop it with an appropriate error message. + * If not stopped by this handler, cyclic dependencies will cause a StackOverflowException. + */ +public class CyclicDependenciesDetector implements Handler { + + @Override + public Resolution resolve(ResolutionContext context) { + ObjectIdentifier duplicateIdentifier = findRepeatedIdentifier(context); + if (duplicateIdentifier != null) { + String traversalList = buildParentsList(context); + throw new InjectorException("Found cyclic dependency' - already traversed '" + duplicateIdentifier + + "' (full traversal list: " + traversalList + " -> " + context.getIdentifier() + ")"); + } + return null; + } + + @Nullable + private static ObjectIdentifier findRepeatedIdentifier(ResolutionContext context) { + Set types = new HashSet<>(); + types.add(context.getIdentifier().getType()); + for (ResolutionContext parent : context.getParents()) { + if (!types.add(parent.getIdentifier().getType())) { + return parent.getIdentifier(); + } + } + return null; + } + + private static String buildParentsList(ResolutionContext context) { + return context.getParents().stream() + .map(ctx -> ctx.getIdentifier().getType().getTypeName()) + .collect(Collectors.joining(" -> ")); + } +} diff --git a/injector/src/main/java/ch/jalu/injector/handlers/instantiation/Resolution.java b/injector/src/main/java/ch/jalu/injector/handlers/instantiation/Resolution.java index cd3534a..f48dd94 100644 --- a/injector/src/main/java/ch/jalu/injector/handlers/instantiation/Resolution.java +++ b/injector/src/main/java/ch/jalu/injector/handlers/instantiation/Resolution.java @@ -32,13 +32,13 @@ public interface Resolution { T instantiateWith(Object... values); /** - * Returns whether this resolution will return a newly created object or not. + * Returns whether this resolution will instantiate an object or not. * Certain resolutions simply need to retrieve an existing object; this method - * returns {@code true} in other cases when the requested object has to be instantiated. + * returns {@code true} when the requested object has to be instantiated. * * @return true if a newly created object will be returned, false if the object already exists */ - default boolean isNewlyCreated() { + default boolean isInstantiation() { return false; } } diff --git a/injector/src/main/java/ch/jalu/injector/handlers/instantiation/StandardInjection.java b/injector/src/main/java/ch/jalu/injector/handlers/instantiation/StandardInjection.java index 7fff658..ad83fec 100644 --- a/injector/src/main/java/ch/jalu/injector/handlers/instantiation/StandardInjection.java +++ b/injector/src/main/java/ch/jalu/injector/handlers/instantiation/StandardInjection.java @@ -56,7 +56,6 @@ public List getDependencies() { @Override public T instantiateWith(Object... values) { // Check no null values & correct size - InjectorUtils.checkNoNullValues(values); InjectorUtils.checkArgument(values.length == constructor.getParameterTypes().length + fields.size(), "Number of values does not correspond to the expected number"); @@ -73,7 +72,7 @@ public T instantiateWith(Object... values) { } @Override - public boolean isNewlyCreated() { + public boolean isInstantiation() { return true; } diff --git a/injector/src/main/java/ch/jalu/injector/handlers/provider/ProviderHandlerImpl.java b/injector/src/main/java/ch/jalu/injector/handlers/provider/ProviderHandlerImpl.java index fe28aac..0927a3e 100644 --- a/injector/src/main/java/ch/jalu/injector/handlers/provider/ProviderHandlerImpl.java +++ b/injector/src/main/java/ch/jalu/injector/handlers/provider/ProviderHandlerImpl.java @@ -96,7 +96,7 @@ public T instantiateWith(Object... values) { } @Override - public boolean isNewlyCreated() { + public boolean isInstantiation() { return true; } @@ -124,6 +124,7 @@ public List getDependencies() { } @Override + @SuppressWarnings("unchecked") public T instantiateWith(Object... values) { InjectorUtils.checkArgument(values.length == 1 && providerClass.isInstance(values[0]), "Expected one dependency of type " + providerClass); @@ -131,7 +132,7 @@ public T instantiateWith(Object... values) { } @Override - public boolean isNewlyCreated() { + public boolean isInstantiation() { return true; } @@ -148,6 +149,7 @@ public List getDependencies() { } @Override + @SuppressWarnings("unchecked") public Provider instantiateWith(Object... values) { InjectorUtils.checkArgument(values.length == 1 && providerClass.isInstance(values[0]), "Expected one dependency of type " + providerClass); diff --git a/injector/src/main/java/ch/jalu/injector/utils/InjectorUtils.java b/injector/src/main/java/ch/jalu/injector/utils/InjectorUtils.java index 0ddcec0..9a76516 100644 --- a/injector/src/main/java/ch/jalu/injector/utils/InjectorUtils.java +++ b/injector/src/main/java/ch/jalu/injector/utils/InjectorUtils.java @@ -31,6 +31,21 @@ public static void checkNoNullValues(T... arr) { } } + /** + * Returns whether the given array contains a null value or not. + * + * @param arr the array to process + * @return true if at least one entry is null, false otherwise + */ + public static boolean containsNullValue(Object... arr) { + for (Object o : arr) { + if (o == null) { + return true; + } + } + return false; + } + public static void checkArgument(boolean expression, String errorMessage) { if (!expression) { throw new InjectorException(errorMessage); diff --git a/injector/src/test/java/ch/jalu/injector/InjectorImplTest.java b/injector/src/test/java/ch/jalu/injector/InjectorImplTest.java index 24181a2..5aaf3b6 100644 --- a/injector/src/test/java/ch/jalu/injector/InjectorImplTest.java +++ b/injector/src/test/java/ch/jalu/injector/InjectorImplTest.java @@ -23,7 +23,6 @@ import ch.jalu.injector.samples.StaticFieldInjection; import ch.jalu.injector.samples.inheritance.Child; import org.junit.Before; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -287,20 +286,6 @@ public void shouldUseCustomInstantiation() { assertThat(example, not(nullValue())); } - @Test - @Ignore - public void shouldThrowIfNoInstantiationMethodsAreAvailable() { - // given - List handlers = null; // TODO #48: fix getAllHandlersExceptInstantiationProviders(); - Injector injector = new InjectorBuilder().addHandlers(handlers).create(); - - // expect - exceptionCatcher.expect("You did not register any instantiation methods"); - - // when - injector.getSingleton(CustomInstantiationExample.class); - } - @Test public void shouldForwardToAnnotationValueHandlers() throws Exception { // given @@ -395,7 +380,6 @@ public void shouldInstantiateForAllAvailableDependencies() { } @Test - @Ignore // TODO #48: Fix createIfHasDependencies feature public void shouldNotInstantiateForMissingDependencies() { // given / when GammaService gammaService = injector.createIfHasDependencies(GammaService.class); @@ -420,7 +404,6 @@ public void shouldInstantiateForAllAvailableDependenciesAndAnnotations() { } @Test - @Ignore // TODO #48: Fix createIfHasDependencies feature public void shouldReturnNullForMissingDependency() { // given injector.provide(Size.class, 2809375); diff --git a/injector/src/test/java/ch/jalu/injector/handlers/instantiation/StandardInjectionTest.java b/injector/src/test/java/ch/jalu/injector/handlers/instantiation/StandardInjectionTest.java index 24cb3a3..e44e9b9 100644 --- a/injector/src/test/java/ch/jalu/injector/handlers/instantiation/StandardInjectionTest.java +++ b/injector/src/test/java/ch/jalu/injector/handlers/instantiation/StandardInjectionTest.java @@ -118,15 +118,6 @@ public void shouldInstantiateClassWithConstructorInjection() { assertThat(gammaService, not(nullValue())); } - @Test(expected = InjectorException.class) - public void shouldThrowForNullValue() { - // given - Resolution injection = provider.safeGet(ClassWithAnnotations.class); - - // when / then - injection.instantiateWith(-112, null, 12L); - } - @Test(expected = InjectorException.class) public void shouldThrowUponInstantiationError() { // given