From 1e901aec169393a3d171ec9421163373cb6e9b72 Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Fri, 14 Dec 2018 14:49:50 -0500 Subject: [PATCH 1/5] Extend cycle detection to support set-components. --- .../firebase/components/ComponentRuntime.java | 3 +- ...omponentSorter.java => CycleDetector.java} | 98 +++++++++++++------ ...SorterTest.java => CycleDetectorTest.java} | 69 +++++-------- 3 files changed, 93 insertions(+), 77 deletions(-) rename firebase-common/src/main/java/com/google/firebase/components/{ComponentSorter.java => CycleDetector.java} (58%) rename firebase-common/src/test/java/com/google/firebase/components/{ComponentSorterTest.java => CycleDetectorTest.java} (80%) diff --git a/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java b/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java index 279e798cd64..18dc2612eb5 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java +++ b/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java @@ -53,7 +53,8 @@ public ComponentRuntime( } Collections.addAll(componentsToAdd, additionalComponents); - components = Collections.unmodifiableList(ComponentSorter.sorted(componentsToAdd)); + CycleDetector.detect(componentsToAdd); + components = Collections.unmodifiableList(componentsToAdd); for (Component component : components) { register(component); diff --git a/firebase-common/src/main/java/com/google/firebase/components/ComponentSorter.java b/firebase-common/src/main/java/com/google/firebase/components/CycleDetector.java similarity index 58% rename from firebase-common/src/main/java/com/google/firebase/components/ComponentSorter.java rename to firebase-common/src/main/java/com/google/firebase/components/CycleDetector.java index 2f884acc8d3..a435d6f3a8c 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/ComponentSorter.java +++ b/firebase-common/src/main/java/com/google/firebase/components/CycleDetector.java @@ -15,15 +15,42 @@ package com.google.firebase.components; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -/** Implementation of topological sort. */ -class ComponentSorter { +/** Cycle detector for the {@link Component} dependency graph. */ +class CycleDetector { + private static class Dep { + private final Class anInterface; + private final boolean set; + + private Dep(Class anInterface, boolean set) { + this.anInterface = anInterface; + this.set = set; + } + + @Override + public boolean equals(Object obj) { + if (obj instanceof Dep) { + Dep dep = (Dep) obj; + return dep.anInterface.equals(anInterface) && dep.set == set; + } + return false; + } + + @Override + public int hashCode() { + int h = 1000003; + h ^= anInterface.hashCode(); + h *= 1000003; + h ^= Boolean.valueOf(set).hashCode(); + return h; + } + } + private static class ComponentNode { private final Component component; private final Set dependencies = new HashSet<>(); @@ -63,22 +90,21 @@ boolean isLeaf() { } /** - * Given a list of components, returns a sorted permutation of it. + * Detect a dependency cycle among provided {@link Component}s. * - * @param components Components to sort. - * @return Sorted list of components. + * @param components Components to detect cycle between. * @throws IllegalArgumentException thrown if multiple components implement the same interface. * @throws DependencyCycleException thrown if a dependency cycle between components is detected. */ - static List> sorted(List> components) { + static void detect(List> components) { Set graph = toGraph(components); Set roots = getRoots(graph); - List> result = new ArrayList<>(); + int numVisited = 0; while (!roots.isEmpty()) { ComponentNode node = roots.iterator().next(); roots.remove(node); - result.add(node.getComponent()); + numVisited++; for (ComponentNode dependent : node.getDependencies()) { dependent.removeDependent(node); @@ -88,11 +114,10 @@ static List> sorted(List> components) { } } - // If there is no dependency cycle in the graph, the size of the resulting component list will - // be equal to the original list, meaning that we were able to sort all components. - if (result.size() == components.size()) { - Collections.reverse(result); - return result; + // If there is no dependency cycle in the graph, the number of visited nodes will be equal to + // the original list. + if (numVisited == components.size()) { + return; } // Otherwise there is a cycle. @@ -107,34 +132,49 @@ static List> sorted(List> components) { } private static Set toGraph(List> components) { - Map, ComponentNode> componentIndex = new HashMap<>(components.size()); + Map> componentIndex = new HashMap<>(components.size()); for (Component component : components) { ComponentNode node = new ComponentNode(component); for (Class anInterface : component.getProvidedInterfaces()) { - if (componentIndex.put(anInterface, node) != null) { + Dep cmp = new Dep(anInterface, !component.isValue()); + if (!componentIndex.containsKey(cmp)) { + componentIndex.put(cmp, new HashSet<>()); + } + Set nodes = componentIndex.get(cmp); + if (!nodes.isEmpty() && !cmp.set) { throw new IllegalArgumentException( String.format("Multiple components provide %s.", anInterface)); } + nodes.add(node); } } - for (ComponentNode component : componentIndex.values()) { - for (Dependency dependency : component.getComponent().getDependencies()) { - if (!dependency.isDirectInjection()) { - continue; - } - - ComponentNode depComponent = componentIndex.get(dependency.getInterface()); - // Missing dependencies are skipped for the purposes of the sort as there is no component to - // sort. - if (depComponent != null) { - component.addDependency(depComponent); - depComponent.addDependent(component); + for (Set componentNodes : componentIndex.values()) { + for (ComponentNode node : componentNodes) { + for (Dependency dependency : node.getComponent().getDependencies()) { + if (!dependency.isDirectInjection()) { + continue; + } + + Set depComponents = + componentIndex.get(new Dep(dependency.getInterface(), dependency.isSet())); + if (depComponents == null) { + continue; + } + for (ComponentNode depComponent : depComponents) { + node.addDependency(depComponent); + depComponent.addDependent(node); + } } } } - return new HashSet<>(componentIndex.values()); + HashSet result = new HashSet<>(); + for (Set componentNodes : componentIndex.values()) { + result.addAll(componentNodes); + } + + return result; } private static Set getRoots(Set components) { diff --git a/firebase-common/src/test/java/com/google/firebase/components/ComponentSorterTest.java b/firebase-common/src/test/java/com/google/firebase/components/CycleDetectorTest.java similarity index 80% rename from firebase-common/src/test/java/com/google/firebase/components/ComponentSorterTest.java rename to firebase-common/src/test/java/com/google/firebase/components/CycleDetectorTest.java index b485e206e86..091b7be8b08 100644 --- a/firebase-common/src/test/java/com/google/firebase/components/ComponentSorterTest.java +++ b/firebase-common/src/test/java/com/google/firebase/components/CycleDetectorTest.java @@ -19,15 +19,13 @@ import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; -import java.util.Set; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @RunWith(JUnit4.class) -public class ComponentSorterTest { +public class CycleDetectorTest { private static final ComponentFactory NULL_FACTORY = container -> null; @SuppressWarnings("unchecked") @@ -58,7 +56,7 @@ private interface TestInterface7 {} * ----> 2 <---- */ @Test - public void sort_shouldTopologicallySortComponents1() { + public void detect_shouldNotDetectACycle1() { List> components = Arrays.asList( Component.builder(TestInterface4.class) @@ -75,7 +73,7 @@ public void sort_shouldTopologicallySortComponents1() { .factory(nullFactory()) .build()); - twice(() -> testSort(components)); + twice(() -> detect(components)); } /* @@ -86,7 +84,7 @@ public void sort_shouldTopologicallySortComponents1() { * 6 -> 7 */ @Test - public void sort_shouldTopologicallySortComponents2() { + public void detect_shouldNotDetectACycle2() { List> components = Arrays.asList( Component.builder(TestInterface1.class) @@ -112,7 +110,7 @@ public void sort_shouldTopologicallySortComponents2() { .build(), Component.builder(TestInterface7.class).factory(nullFactory()).build()); - twice(() -> testSort(components)); + twice(() -> detect(components)); } /* @@ -127,7 +125,7 @@ public void sort_shouldTopologicallySortComponents2() { * 5 6 7 */ @Test - public void sort_shouldTopologicallySortComponents3() { + public void detect_shouldNotDetectACycle3() { List> components = Arrays.asList( Component.builder(TestInterface1.class) @@ -152,7 +150,7 @@ public void sort_shouldTopologicallySortComponents3() { Component.builder(TestInterface6.class).factory(nullFactory()).build(), Component.builder(TestInterface7.class).factory(nullFactory()).build()); - twice(() -> testSort(components)); + twice(() -> detect(components)); } /* @@ -164,7 +162,7 @@ public void sort_shouldTopologicallySortComponents3() { * 3 4 5 6 */ @Test - public void sort_shouldTopologicallySortComponents4() { + public void detect_shouldNotDetectACycle4() { List> components = Arrays.asList( Component.builder(TestInterface1.class) @@ -183,7 +181,7 @@ public void sort_shouldTopologicallySortComponents4() { Component.builder(TestInterface6.class).factory(nullFactory()).build(), Component.builder(TestInterface7.class).factory(nullFactory()).build()); - twice(() -> testSort(components)); + twice(() -> detect(components)); } /* @@ -194,7 +192,7 @@ public void sort_shouldTopologicallySortComponents4() { * |_________| */ @Test - public void sort_withDependencyCycle_shouldThrow() { + public void detect_withDependencyCycle_shouldThrow() { List> components = Arrays.asList( Component.builder(TestInterface1.class) @@ -211,7 +209,7 @@ public void sort_withDependencyCycle_shouldThrow() { .build()); try { - ComponentSorter.sorted(components); + CycleDetector.detect(components); fail("Not thrown"); } catch (DependencyCycleException ex) { assertThat(ex.getComponentsInCycle()).containsExactlyElementsIn(components); @@ -227,7 +225,7 @@ public void sort_withDependencyCycle_shouldThrow() { |_Provider_| */ @Test - public void sort_withProviderDependencyCycle_shouldSortCorrectly() { + public void detect_withProviderDependencyCycle_shouldNotThrow() { List> components = Arrays.asList( Component.builder(TestInterface1.class) @@ -243,31 +241,32 @@ public void sort_withProviderDependencyCycle_shouldSortCorrectly() { .factory(nullFactory()) .build()); - ComponentSorter.sorted(components); - twice(() -> testSort(components)); + CycleDetector.detect(components); + twice(() -> detect(components)); } @Test - public void sort_withMultipleComponentsImplementingSameIface_shouldThrow() { + public void detect_withMultipleComponentsImplementingSameIface_shouldThrow() { List> components = Arrays.asList( Component.builder(TestInterface1.class).factory(nullFactory()).build(), Component.builder(TestInterface1.class).factory(nullFactory()).build()); try { - ComponentSorter.sorted(components); + CycleDetector.detect(components); fail(); } catch (IllegalArgumentException ex) { // success. } } - private static void testSort(List> components) { + private static void detect(List> components) { Collections.shuffle(components); - List> sorted = ComponentSorter.sorted(components); - - assertThat(sorted).hasSize(components.size()); - assertAscendingOrder(sorted); + try { + CycleDetector.detect(components); + } catch (DependencyException ex) { + fail(String.format("Unexpected exception thrown: %s", ex)); + } } private static void twice(Runnable runnable) { @@ -275,28 +274,4 @@ private static void twice(Runnable runnable) { runnable.run(); } } - - private static void assertAscendingOrder(List> components) { - Set> seenInterfaces = new HashSet<>(); - Set> allInterfaces = new HashSet<>(); - for (Component component : components) { - allInterfaces.addAll(component.getProvidedInterfaces()); - } - - for (Component component : components) { - for (Dependency dependency : component.getDependencies()) { - if (!dependency.isDirectInjection()) { - continue; - } - Class iface = dependency.getInterface(); - if (allInterfaces.contains(iface) && !seenInterfaces.contains(iface)) { - fail( - String.format( - "Encountered component before its dependency. Component: %s, Dependency: %s", - component, iface)); - } - } - seenInterfaces.addAll(component.getProvidedInterfaces()); - } - } } From 96acdf2648fafe0867255ef459355116ccf61f96 Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Fri, 14 Dec 2018 15:10:42 -0500 Subject: [PATCH 2/5] Add runtime support for set-components. --- .../AbstractComponentContainer.java | 6 ++ .../components/ComponentContainer.java | 7 ++ .../firebase/components/ComponentRuntime.java | 74 ++++++++++++++++-- .../com/google/firebase/components/Lazy.java | 10 ++- .../RestrictedComponentContainer.java | 42 +++++++++- .../components/ComponentRuntimeTest.java | 33 ++++++++ .../google/firebase/components/LazyTest.java | 49 +++++------- .../RestrictedComponentContainerTest.java | 78 ++++++++++++++----- 8 files changed, 237 insertions(+), 62 deletions(-) diff --git a/firebase-common/src/main/java/com/google/firebase/components/AbstractComponentContainer.java b/firebase-common/src/main/java/com/google/firebase/components/AbstractComponentContainer.java index ca38f534b3e..143adf0acc7 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/AbstractComponentContainer.java +++ b/firebase-common/src/main/java/com/google/firebase/components/AbstractComponentContainer.java @@ -15,6 +15,7 @@ package com.google.firebase.components; import com.google.firebase.inject.Provider; +import java.util.Set; abstract class AbstractComponentContainer implements ComponentContainer { @Override @@ -25,4 +26,9 @@ public T get(Class anInterface) { } return provider.get(); } + + @Override + public Set setOf(Class anInterface) { + return setOfProvider(anInterface).get(); + } } diff --git a/firebase-common/src/main/java/com/google/firebase/components/ComponentContainer.java b/firebase-common/src/main/java/com/google/firebase/components/ComponentContainer.java index 85e0d14ba9e..492c2986414 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/ComponentContainer.java +++ b/firebase-common/src/main/java/com/google/firebase/components/ComponentContainer.java @@ -16,6 +16,7 @@ import com.google.android.gms.common.annotation.KeepForSdk; import com.google.firebase.inject.Provider; +import java.util.Set; /** Provides a means to retrieve instances of requested classes/interfaces. */ @KeepForSdk @@ -25,4 +26,10 @@ public interface ComponentContainer { @KeepForSdk Provider getProvider(Class anInterface); + + @KeepForSdk + Set setOf(Class anInterface); + + @KeepForSdk + Provider> setOfProvider(Class anInterface); } diff --git a/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java b/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java index 18dc2612eb5..7b16d832805 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java +++ b/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java @@ -21,8 +21,10 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.Executor; /** @@ -32,8 +34,10 @@ * Component}s via {@link #get(Class)} method. */ public class ComponentRuntime extends AbstractComponentContainer { - private final List> components; + private static final Provider> EMPTY_PROVIDER = Collections::emptySet; + private final Map, Lazy> components = new HashMap<>(); private final Map, Lazy> lazyInstanceMap = new HashMap<>(); + private final Map, Lazy>> lazySetMap = new HashMap<>(); private final EventBus eventBus; /** @@ -54,12 +58,49 @@ public ComponentRuntime( Collections.addAll(componentsToAdd, additionalComponents); CycleDetector.detect(componentsToAdd); - components = Collections.unmodifiableList(componentsToAdd); - for (Component component : components) { + for (Component component : componentsToAdd) { register(component); } validateDependencies(); + + processSetComponents(); + } + + /** Populates lazySetMap to make set components available for consumption via set dependencies. */ + private void processSetComponents() { + Map, Set>> setIndex = new HashMap<>(); + for (Map.Entry, Lazy> entry : components.entrySet()) { + Component component = entry.getKey(); + + // only process set components. + if (component.isValue()) { + continue; + } + + Lazy lazy = entry.getValue(); + + for (Class anInterface : component.getProvidedInterfaces()) { + if (!setIndex.containsKey(anInterface)) { + setIndex.put(anInterface, new HashSet<>()); + } + setIndex.get(anInterface).add(lazy); + } + } + + for (Map.Entry, Set>> entry : setIndex.entrySet()) { + Set> lazies = entry.getValue(); + lazySetMap.put( + entry.getKey(), + new Lazy<>( + () -> { + Set set = new HashSet<>(); + for (Lazy lazy : lazies) { + set.add(lazy.get()); + } + return Collections.unmodifiableSet(set); + })); + } } @Override @@ -69,6 +110,16 @@ public Provider getProvider(Class anInterface) { return (Provider) lazyInstanceMap.get(anInterface); } + @Override + @SuppressWarnings("unchecked") + public Provider> setOfProvider(Class anInterface) { + Lazy> lazy = lazySetMap.get(anInterface); + if (lazy != null) { + return (Provider>) (Provider) lazy; + } + return (Provider>) (Provider) EMPTY_PROVIDER; + } + /** * Initializes all eager components. * @@ -77,10 +128,12 @@ public Provider getProvider(Class anInterface) { *

Note: the method is idempotent. */ public void initializeEagerComponents(boolean isDefaultApp) { - for (Component component : components) { + for (Map.Entry, Lazy> entry : components.entrySet()) { + Component component = entry.getKey(); + Lazy lazy = entry.getValue(); + if (component.isAlwaysEager() || (component.isEagerInDefaultApp() && isDefaultApp)) { - // at least one interface is guarenteed to be provided by a component. - get(component.getProvidedInterfaces().iterator().next()); + lazy.get(); } } @@ -89,15 +142,20 @@ public void initializeEagerComponents(boolean isDefaultApp) { private void register(Component component) { Lazy lazy = - new Lazy<>(component.getFactory(), new RestrictedComponentContainer(component, this)); + new Lazy<>( + () -> component.getFactory().create(new RestrictedComponentContainer(component, this))); + components.put(component, lazy); + if (!component.isValue()) { + return; + } for (Class anInterface : component.getProvidedInterfaces()) { lazyInstanceMap.put(anInterface, lazy); } } private void validateDependencies() { - for (Component component : components) { + for (Component component : components.keySet()) { for (Dependency dependency : component.getDependencies()) { if (dependency.isRequired() && !lazyInstanceMap.containsKey(dependency.getInterface())) { throw new MissingDependencyException( diff --git a/firebase-common/src/main/java/com/google/firebase/components/Lazy.java b/firebase-common/src/main/java/com/google/firebase/components/Lazy.java index 1ff985a4809..20c72aa9196 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/Lazy.java +++ b/firebase-common/src/main/java/com/google/firebase/components/Lazy.java @@ -38,9 +38,8 @@ class Lazy implements Provider { this.instance = instance; } - /** Creates a lazy backed by a {@link ComponentFactory} and {@link ComponentContainer}. */ - Lazy(ComponentFactory factory, ComponentContainer container) { - provider = () -> factory.create(container); + Lazy(Provider provider) { + this.provider = provider; } /** Returns the initialized value. */ @@ -59,7 +58,10 @@ public T get() { } } } - return (T) result; + + @SuppressWarnings("unchecked") + T tResult = (T) result; + return tResult; } @VisibleForTesting diff --git a/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java b/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java index 2365a79af94..0490b9d6251 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java +++ b/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java @@ -28,17 +28,29 @@ final class RestrictedComponentContainer extends AbstractComponentContainer { private final Set> allowedDirectInterfaces; private final Set> allowedProviderInterfaces; + private final Set> allowedSetDirectInterfaces; + private final Set> allowedSetProviderInterfaces; private final Set> allowedPublishedEvents; private final ComponentContainer delegateContainer; RestrictedComponentContainer(Component component, ComponentContainer container) { Set> directInterfaces = new HashSet<>(); Set> providerInterfaces = new HashSet<>(); + Set> setDirectInterfaces = new HashSet<>(); + Set> setProviderInterfaces = new HashSet<>(); for (Dependency dependency : component.getDependencies()) { if (dependency.isDirectInjection()) { - directInterfaces.add(dependency.getInterface()); + if (dependency.isSet()) { + setDirectInterfaces.add(dependency.getInterface()); + } else { + directInterfaces.add(dependency.getInterface()); + } } else { - providerInterfaces.add(dependency.getInterface()); + if (dependency.isSet()) { + setProviderInterfaces.add(dependency.getInterface()); + } else { + providerInterfaces.add(dependency.getInterface()); + } } } if (!component.getPublishedEvents().isEmpty()) { @@ -46,6 +58,8 @@ final class RestrictedComponentContainer extends AbstractComponentContainer { } allowedDirectInterfaces = Collections.unmodifiableSet(directInterfaces); allowedProviderInterfaces = Collections.unmodifiableSet(providerInterfaces); + allowedSetDirectInterfaces = Collections.unmodifiableSet(setDirectInterfaces); + allowedSetProviderInterfaces = Collections.unmodifiableSet(setProviderInterfaces); allowedPublishedEvents = component.getPublishedEvents(); delegateContainer = container; } @@ -54,7 +68,7 @@ final class RestrictedComponentContainer extends AbstractComponentContainer { public T get(Class anInterface) { if (!allowedDirectInterfaces.contains(anInterface)) { throw new IllegalArgumentException( - String.format("Requesting %s is not allowed.", anInterface)); + String.format("Attempting to request an undeclared dependency %s.", anInterface)); } // The container is guaranteed to contain a class keyed with Publisher.class. This is what we @@ -75,11 +89,31 @@ public T get(Class anInterface) { public Provider getProvider(Class anInterface) { if (!allowedProviderInterfaces.contains(anInterface)) { throw new IllegalArgumentException( - String.format("Requesting Provider<%s> is not allowed.", anInterface)); + String.format( + "Attempting to request an undeclared dependency Provider<%s>.", anInterface)); } return delegateContainer.getProvider(anInterface); } + @Override + public Provider> setOfProvider(Class anInterface) { + if (!allowedSetProviderInterfaces.contains(anInterface)) { + throw new IllegalArgumentException( + String.format( + "Attempting to request an undeclared dependency Provider>.", anInterface)); + } + return delegateContainer.setOfProvider(anInterface); + } + + @Override + public Set setOf(Class anInterface) { + if (!allowedSetDirectInterfaces.contains(anInterface)) { + throw new IllegalArgumentException( + String.format("Attempting to request an undeclared dependency Set<%s>.", anInterface)); + } + return delegateContainer.setOf(anInterface); + } + /** * An implementation of {@link Publisher} that is backed by another delegate {@link Publisher} and * restricts publishing to only a set of allowed event types. diff --git a/firebase-common/src/test/java/com/google/firebase/components/ComponentRuntimeTest.java b/firebase-common/src/test/java/com/google/firebase/components/ComponentRuntimeTest.java index 2d33e3f850c..28197e1e377 100644 --- a/firebase-common/src/test/java/com/google/firebase/components/ComponentRuntimeTest.java +++ b/firebase-common/src/test/java/com/google/firebase/components/ComponentRuntimeTest.java @@ -233,4 +233,37 @@ public void container_shouldExposeAllProvidedInterfacesOfAComponent() { assertThat(child).isSameAs(parent); assertThat(child.get()).isSameAs(parent.get()); } + + @Test + public void container_shouldExposeAllRegisteredSetValues() { + ComponentRuntime runtime = + new ComponentRuntime( + EXECUTOR, + Collections.emptyList(), + Component.intoSet(1, Integer.class), + Component.intoSet(2, Integer.class)); + + assertThat(runtime.setOf(Integer.class)).containsExactly(1, 2); + } + + @Test + public void setComponents_shouldParticipateInCycleDetection() { + try { + new ComponentRuntime( + EXECUTOR, + Collections.emptyList(), + Component.builder(ComponentOne.class) + .add(Dependency.setOf(Integer.class)) + .factory(c -> null) + .build(), + Component.intoSet(1, Integer.class), + Component.intoSetBuilder(Integer.class) + .add(Dependency.required(ComponentOne.class)) + .factory(c -> 2) + .build()); + fail("Expected exception not thrown."); + } catch (DependencyCycleException ex) { + // success. + } + } } diff --git a/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java b/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java index 30771296772..a99c9523dce 100644 --- a/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java +++ b/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java @@ -14,14 +14,12 @@ package com.google.firebase.components; -import static com.google.common.truth.Truth.assertThat; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - import com.google.firebase.inject.Provider; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -32,16 +30,18 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @RunWith(JUnit4.class) public final class LazyTest { - private static final ComponentContainer CONTAINER = new EmptyContainer(); @SuppressWarnings("unchecked") - private final ComponentFactory mockFactory = mock(ComponentFactory.class); + private final Provider mockProvider = mock(Provider.class); @Test public void get_whenLazyIsInitializedWithValue_shouldReturnTheValue() { @@ -54,13 +54,13 @@ public void get_whenLazyIsInitializedWithValue_shouldReturnTheValue() { @Test public void get_shouldDelegateToFactory() { Object instance = new Object(); - Lazy lazy = new Lazy<>(mockFactory, CONTAINER); + Lazy lazy = new Lazy<>(mockProvider); - when(mockFactory.create(any())).thenReturn(instance); + when(mockProvider.get()).thenReturn(instance); assertThat(lazy.get()).isSameAs(instance); - verify(mockFactory, times(1)).create(CONTAINER); + verify(mockProvider, times(1)).get(); } @Test @@ -70,8 +70,8 @@ public void get_shouldBeThreadSafe() throws Exception { ExecutorService executor = Executors.newFixedThreadPool(numThreads); - LatchedFactory factory = new LatchedFactory(latch); - Lazy lazy = new Lazy<>(factory, CONTAINER); + LatchedProvider provider = new LatchedProvider(latch); + Lazy lazy = new Lazy<>(provider); List> tasks = new ArrayList<>(numThreads); for (int i = 0; i < numThreads; i++) { @@ -83,7 +83,7 @@ public void get_shouldBeThreadSafe() throws Exception { } List> futures = executor.invokeAll(tasks); - assertThat(factory.instantiationCount.get()).isEqualTo(1); + assertThat(provider.instantiationCount.get()).isEqualTo(1); Set createdInstances = new HashSet<>(); for (Future future : futures) { @@ -92,23 +92,16 @@ public void get_shouldBeThreadSafe() throws Exception { assertThat(createdInstances).hasSize(1); } - private static class EmptyContainer extends AbstractComponentContainer { - @Override - public Provider getProvider(Class anInterface) { - return null; - } - } - - private static class LatchedFactory implements ComponentFactory { + private static class LatchedProvider implements Provider { private final CountDownLatch latch; final AtomicInteger instantiationCount = new AtomicInteger(); - LatchedFactory(CountDownLatch latch) { + LatchedProvider(CountDownLatch latch) { this.latch = latch; } @Override - public Object create(ComponentContainer container) { + public Object get() { // wait for all threads to start and get as close to calling Lazy#get() as possible. uninterruptablyAwait(latch); instantiationCount.incrementAndGet(); diff --git a/firebase-common/src/test/java/com/google/firebase/components/RestrictedComponentContainerTest.java b/firebase-common/src/test/java/com/google/firebase/components/RestrictedComponentContainerTest.java index 9ef02b0e986..6989e440ab8 100644 --- a/firebase-common/src/test/java/com/google/firebase/components/RestrictedComponentContainerTest.java +++ b/firebase-common/src/test/java/com/google/firebase/components/RestrictedComponentContainerTest.java @@ -23,7 +23,9 @@ import com.google.firebase.events.Event; import com.google.firebase.events.Publisher; +import java.util.Collections; import java.util.List; +import java.util.Set; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -35,26 +37,28 @@ public final class RestrictedComponentContainerTest { private final ComponentContainer container = new RestrictedComponentContainer( Component.builder(String.class) - .add(Dependency.required(StringBuilder.class)) - .add(Dependency.requiredProvider(StringBuffer.class)) + .add(Dependency.required(Float.class)) + .add(Dependency.requiredProvider(Double.class)) + .add(Dependency.setOf(Long.class)) + .add(Dependency.setOfProvider(Boolean.class)) .factory(c -> null) .build(), delegate); private final ComponentContainer publishingContainer = new RestrictedComponentContainer( - Component.builder(String.class).publishes(StringBuilder.class).factory(c -> null).build(), + Component.builder(String.class).publishes(Float.class).factory(c -> null).build(), delegate); private final Publisher mockPublisher = mock(Publisher.class); @Test public void get_withAllowedClass_shouldReturnAnInstanceOfThatClass() { - StringBuilder sb = new StringBuilder(); - when(delegate.get(StringBuilder.class)).thenReturn(sb); + Float value = 1.0f; + when(delegate.get(Float.class)).thenReturn(value); - assertThat(container.get(StringBuilder.class)).isSameAs(sb); - verify(delegate).get(StringBuilder.class); + assertThat(container.get(Float.class)).isSameAs(value); + verify(delegate).get(Float.class); } @Test @@ -70,10 +74,10 @@ public void get_withNotAllowedClass_shouldThrow() { @Test public void get_withProviderClass_shouldThrow() { try { - container.get(StringBuffer.class); + container.get(Double.class); fail("Expected exception not thrown."); } catch (IllegalArgumentException ex) { - assertThat(ex.getMessage()).contains("java.lang.StringBuffer"); + assertThat(ex.getMessage()).contains("java.lang.Double"); } } @@ -89,11 +93,11 @@ public void get_withPublisher_shouldThrow() { @Test public void getProvider_withAllowedClass_shouldReturnAnInstanceOfThatClass() { - StringBuffer sb = new StringBuffer(); - when(delegate.getProvider(StringBuffer.class)).thenReturn(new Lazy<>(sb)); + Double value = 3.0d; + when(delegate.getProvider(Double.class)).thenReturn(new Lazy<>(value)); - assertThat(container.getProvider(StringBuffer.class).get()).isSameAs(sb); - verify(delegate).getProvider(StringBuffer.class); + assertThat(container.getProvider(Double.class).get()).isSameAs(value); + verify(delegate).getProvider(Double.class); } @Test @@ -109,10 +113,48 @@ public void getProvider_withNotAllowedClass_shouldThrow() { @Test public void getProvider_withDirectClass_shouldThrow() { try { - container.getProvider(StringBuilder.class); + container.getProvider(Float.class); fail("Expected exception not thrown."); } catch (IllegalArgumentException ex) { - assertThat(ex.getMessage()).contains("java.lang.StringBuilder"); + assertThat(ex.getMessage()).contains("java.lang.Float"); + } + } + + @Test + public void setOf_withAllowedClass_shouldReturnExpectedSet() { + Set set = Collections.emptySet(); + when(delegate.setOf(Long.class)).thenReturn(set); + + assertThat(container.setOf(Long.class)).isSameAs(set); + verify(delegate).setOf(Long.class); + } + + @Test + public void setOf_withNotAllowedClass_shouldThrow() { + try { + container.setOf(List.class); + fail("Expected exception not thrown."); + } catch (IllegalArgumentException ex) { + assertThat(ex.getMessage()).contains("java.util.List"); + } + } + + @Test + public void setOfProvider_withAllowedClass_shouldReturnExpectedSet() { + Set set = Collections.emptySet(); + when(delegate.setOfProvider(Boolean.class)).thenReturn(new Lazy<>(set)); + + assertThat(container.setOfProvider(Boolean.class).get()).isSameAs(set); + verify(delegate).setOfProvider(Boolean.class); + } + + @Test + public void setOfProvider_withNotAllowedClass_shouldThrow() { + try { + container.setOf(List.class); + fail("Expected exception not thrown."); + } catch (IllegalArgumentException ex) { + assertThat(ex.getMessage()).contains("java.util.List"); } } @@ -121,7 +163,7 @@ public void publish_withDeclaredEvent_shouldSucceed() { when(delegate.get(Publisher.class)).thenReturn(mockPublisher); Publisher publisher = publishingContainer.get(Publisher.class); - Event event = new Event<>(StringBuilder.class, new StringBuilder()); + Event event = new Event<>(Float.class, 1f); publisher.publish(event); verify(mockPublisher).publish(event); @@ -132,13 +174,13 @@ public void publish_withUndeclaredEvent_shouldThrow() { when(delegate.get(Publisher.class)).thenReturn(mockPublisher); Publisher publisher = publishingContainer.get(Publisher.class); - Event event = new Event<>(StringBuffer.class, new StringBuffer()); + Event event = new Event<>(Double.class, 1d); try { publisher.publish(event); fail("Expected exception not thrown."); } catch (IllegalArgumentException ex) { - assertThat(ex.getMessage()).contains("java.lang.StringBuffer"); + assertThat(ex.getMessage()).contains("java.lang.Double"); } verify(mockPublisher, never()).publish(event); From b18fd9ce617f5a6803c1c9840ac9c586435c76ac Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Wed, 19 Dec 2018 17:49:44 -0500 Subject: [PATCH 3/5] Fix formatting. --- .../google/firebase/components/LazyTest.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java b/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java index a99c9523dce..6b5ef83d6da 100644 --- a/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java +++ b/firebase-common/src/test/java/com/google/firebase/components/LazyTest.java @@ -14,12 +14,13 @@ package com.google.firebase.components; -import com.google.firebase.inject.Provider; - -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import com.google.firebase.inject.Provider; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -30,12 +31,9 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicInteger; - -import static com.google.common.truth.Truth.assertThat; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public final class LazyTest { From f9e09166c641f349f699b48b2c76104626cb6b65 Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Fri, 4 Jan 2019 14:03:18 -0500 Subject: [PATCH 4/5] Refactored component initialization. --- .../firebase/components/ComponentRuntime.java | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java b/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java index 7b16d832805..ea024c015e0 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java +++ b/firebase-common/src/main/java/com/google/firebase/components/ComponentRuntime.java @@ -60,13 +60,32 @@ public ComponentRuntime( CycleDetector.detect(componentsToAdd); for (Component component : componentsToAdd) { - register(component); - } - validateDependencies(); + Lazy lazy = + new Lazy<>( + () -> + component.getFactory().create(new RestrictedComponentContainer(component, this))); + components.put(component, lazy); + } + processInstanceComponents(); processSetComponents(); } + private void processInstanceComponents() { + for (Map.Entry, Lazy> entry : components.entrySet()) { + Component component = entry.getKey(); + if (!component.isValue()) { + return; + } + + Lazy lazy = entry.getValue(); + for (Class anInterface : component.getProvidedInterfaces()) { + lazyInstanceMap.put(anInterface, lazy); + } + } + validateDependencies(); + } + /** Populates lazySetMap to make set components available for consumption via set dependencies. */ private void processSetComponents() { Map, Set>> setIndex = new HashMap<>(); @@ -140,20 +159,6 @@ public void initializeEagerComponents(boolean isDefaultApp) { eventBus.enablePublishingAndFlushPending(); } - private void register(Component component) { - Lazy lazy = - new Lazy<>( - () -> component.getFactory().create(new RestrictedComponentContainer(component, this))); - - components.put(component, lazy); - if (!component.isValue()) { - return; - } - for (Class anInterface : component.getProvidedInterfaces()) { - lazyInstanceMap.put(anInterface, lazy); - } - } - private void validateDependencies() { for (Component component : components.keySet()) { for (Dependency dependency : component.getDependencies()) { From a5072ecbf292ea50ead72ee94002990916b521a3 Mon Sep 17 00:00:00 2001 From: Vladimir Kryachko Date: Wed, 9 Jan 2019 16:20:44 -0800 Subject: [PATCH 5/5] Add javadoc. --- .../RestrictedComponentContainer.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java b/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java index 0490b9d6251..4fabbd4a960 100644 --- a/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java +++ b/firebase-common/src/main/java/com/google/firebase/components/RestrictedComponentContainer.java @@ -64,6 +64,11 @@ final class RestrictedComponentContainer extends AbstractComponentContainer { delegateContainer = container; } + /** + * Returns an instance of the requested class if it is allowed. + * + * @throws IllegalArgumentException otherwise. + */ @Override public T get(Class anInterface) { if (!allowedDirectInterfaces.contains(anInterface)) { @@ -85,6 +90,11 @@ public T get(Class anInterface) { return publisher; } + /** + * Returns an instance of the provider for the requested class if it is allowed. + * + * @throws IllegalArgumentException otherwise. + */ @Override public Provider getProvider(Class anInterface) { if (!allowedProviderInterfaces.contains(anInterface)) { @@ -95,6 +105,11 @@ public Provider getProvider(Class anInterface) { return delegateContainer.getProvider(anInterface); } + /** + * Returns an instance of the provider for the set of requested classes if it is allowed. + * + * @throws IllegalArgumentException otherwise. + */ @Override public Provider> setOfProvider(Class anInterface) { if (!allowedSetProviderInterfaces.contains(anInterface)) { @@ -105,6 +120,11 @@ public Provider> setOfProvider(Class anInterface) { return delegateContainer.setOfProvider(anInterface); } + /** + * Returns a set of requested classes if it is allowed. + * + * @throws IllegalArgumentException otherwise. + */ @Override public Set setOf(Class anInterface) { if (!allowedSetDirectInterfaces.contains(anInterface)) {