Skip to content

Commit

Permalink
Fix an issue where subcomponent builder bindings could be accidentall…
Browse files Browse the repository at this point in the history
…y cached from the parent component.

RELNOTES=Fix an issue with cached subcomponent builder bindings
PiperOrigin-RevId: 360513850
  • Loading branch information
Chang-Eric authored and Dagger Team committed Mar 2, 2021
1 parent 2192014 commit c2d097f
Show file tree
Hide file tree
Showing 4 changed files with 306 additions and 1 deletion.
25 changes: 25 additions & 0 deletions java/dagger/internal/codegen/base/ComponentAnnotation.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,33 @@ public abstract class ComponentAnnotation {
private static final ImmutableSet<Class<? extends Annotation>> SUBCOMPONENT_ANNOTATIONS =
ImmutableSet.of(Subcomponent.class, ProductionSubcomponent.class);

// TODO(erichang): Move ComponentCreatorAnnotation into /base and use that here?
/** The component/subcomponent creator annotation types. */
private static final ImmutableSet<Class<? extends Annotation>> CREATOR_ANNOTATIONS =
ImmutableSet.of(
Component.Builder.class,
Component.Factory.class,
ProductionComponent.Builder.class,
ProductionComponent.Factory.class,
Subcomponent.Builder.class,
Subcomponent.Factory.class,
ProductionSubcomponent.Builder.class,
ProductionSubcomponent.Factory.class);

/** All component annotation types. */
private static final ImmutableSet<Class<? extends Annotation>> ALL_COMPONENT_ANNOTATIONS =
ImmutableSet.<Class<? extends Annotation>>builder()
.addAll(ROOT_COMPONENT_ANNOTATIONS)
.addAll(SUBCOMPONENT_ANNOTATIONS)
.build();

/** All component and creator annotation types. */
private static final ImmutableSet<Class<? extends Annotation>>
ALL_COMPONENT_AND_CREATOR_ANNOTATIONS = ImmutableSet.<Class<? extends Annotation>>builder()
.addAll(ALL_COMPONENT_ANNOTATIONS)
.addAll(CREATOR_ANNOTATIONS)
.build();

/** The annotation itself. */
public abstract AnnotationMirror annotation();

Expand Down Expand Up @@ -206,6 +226,11 @@ public static ImmutableSet<Class<? extends Annotation>> allComponentAnnotations(
return ALL_COMPONENT_ANNOTATIONS;
}

/** All component and creator annotation types. */
public static ImmutableSet<Class<? extends Annotation>> allComponentAndCreatorAnnotations() {
return ALL_COMPONENT_AND_CREATOR_ANNOTATIONS;
}

/**
* An actual component annotation.
*
Expand Down
14 changes: 14 additions & 0 deletions java/dagger/internal/codegen/base/Keys.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

package dagger.internal.codegen.base;

import static com.google.auto.common.MoreTypes.asTypeElement;
import static dagger.internal.codegen.base.ComponentAnnotation.allComponentAndCreatorAnnotations;
import static dagger.internal.codegen.langmodel.DaggerElements.isAnyAnnotationPresent;

import com.google.auto.common.MoreElements;
import com.google.auto.common.MoreTypes;
import dagger.internal.codegen.langmodel.DaggerTypes;
Expand Down Expand Up @@ -88,4 +92,14 @@ public Boolean visitDeclared(DeclaredType type, Void ignored) {
},
null);
}

/**
* Returns {@code true} if the given key is for a component/subcomponent or a creator of a
* component/subcomponent.
*/
public static boolean isComponentOrCreator(Key key) {
return !key.qualifier().isPresent()
&& key.type().getKind() == TypeKind.DECLARED
&& isAnyAnnotationPresent(asTypeElement(key.type()), allComponentAndCreatorAnnotations());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import dagger.Reusable;
import dagger.internal.codegen.base.ClearableCache;
import dagger.internal.codegen.base.ContributionType;
import dagger.internal.codegen.base.Keys;
import dagger.internal.codegen.base.MapType;
import dagger.internal.codegen.base.OptionalType;
import dagger.internal.codegen.compileroption.CompilerOptions;
Expand Down Expand Up @@ -796,7 +797,7 @@ void resolve(Key key) {
* 2. If there are any explicit bindings in this component, they may conflict with those in
* the ancestor component, so resolve them here so that conflicts can be caught.
*/
if (getPreviouslyResolvedBindings(key).isPresent()) {
if (getPreviouslyResolvedBindings(key).isPresent() && !Keys.isComponentOrCreator(key)) {
/* Resolve in the parent in case there are multibinding contributions or conflicts in some
* component between this one and the previously-resolved one. */
parentResolver.get().resolve(key);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
/*
* Copyright (C) 2015 The Dagger Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package dagger.functional.subcomponent;

import static com.google.common.truth.Truth.assertThat;

import dagger.Component;
import dagger.Module;
import dagger.Provides;
import dagger.Subcomponent;
import dagger.multibindings.IntoSet;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.Set;
import javax.inject.Inject;
import javax.inject.Qualifier;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
* Regression tests for an issue where subcomponent builder bindings were incorrectly reused from
* a parent even if the subcomponent were redeclared on the child component. This manifested via
* multibindings, especially since subcomponent builder bindings are special in that we cannot
* traverse them to see if they depend on local multibinding contributions.
*/
@RunWith(JUnit4.class)
public final class SubcomponentBuilderMultibindingsTest {

@Retention(RetentionPolicy.RUNTIME)
@Qualifier
public @interface ParentFoo{}

@Retention(RetentionPolicy.RUNTIME)
@Qualifier
public @interface ChildFoo{}

public static final class Foo {
final Set<String> multi;
@Inject Foo(Set<String> multi) {
this.multi = multi;
}
}

// This tests the case where a subcomponent is installed in both the parent and child component.
// In this case, we expect two subcomponents to be generated with the child one including the
// child multibinding contribution.
public static final class ChildInstallsFloating {
@Component(modules = ParentModule.class)
public interface Parent {
@ParentFoo Foo getParentFoo();

Child getChild();
}

@Subcomponent(modules = ChildModule.class)
public interface Child {
@ChildFoo Foo getChildFoo();
}

@Subcomponent
public interface FloatingSub {
Foo getFoo();

@Subcomponent.Builder
public interface Builder {
FloatingSub build();
}
}

@Module(subcomponents = FloatingSub.class)
public interface ParentModule {
@Provides
@IntoSet
static String provideStringMulti() {
return "parent";
}

@Provides
@ParentFoo
static Foo provideParentFoo(FloatingSub.Builder builder) {
return builder.build().getFoo();
}
}

// The subcomponent installation of FloatingSub here is the key difference
@Module(subcomponents = FloatingSub.class)
public interface ChildModule {
@Provides
@IntoSet
static String provideStringMulti() {
return "child";
}

@Provides
@ChildFoo
static Foo provideChildFoo(FloatingSub.Builder builder) {
return builder.build().getFoo();
}
}

private ChildInstallsFloating() {}
}

// This is the same as the above, except this time the child does not install the subcomponent
// builder. Here, we expect the child to reuse the parent subcomponent binding (we want to avoid
// any mistakes that might implicitly create a new subcomponent relationship) and so therefore
// we expect only one subcomponent to be generated in the parent resulting in the child not seeing
// the child multibinding contribution.
public static final class ChildDoesNotInstallFloating {
@Component(modules = ParentModule.class)
public interface Parent {
@ParentFoo Foo getParentFoo();

Child getChild();
}

@Subcomponent(modules = ChildModule.class)
public interface Child {
@ChildFoo Foo getChildFoo();
}

@Subcomponent
public interface FloatingSub {
Foo getFoo();

@Subcomponent.Builder
public interface Builder {
FloatingSub build();
}
}

@Module(subcomponents = FloatingSub.class)
public interface ParentModule {
@Provides
@IntoSet
static String provideStringMulti() {
return "parent";
}

@Provides
@ParentFoo
static Foo provideParentFoo(FloatingSub.Builder builder) {
return builder.build().getFoo();
}
}

// The lack of a subcomponent installation of FloatingSub here is the key difference
@Module
public interface ChildModule {
@Provides
@IntoSet
static String provideStringMulti() {
return "child";
}

@Provides
@ChildFoo
static Foo provideChildFoo(FloatingSub.Builder builder) {
return builder.build().getFoo();
}
}

private ChildDoesNotInstallFloating() {}
}

// This is similar to the first, except this time the components installs the subcomponent via
// factory methods. Here, we expect the child to get a new subcomponent and so should see its
// multibinding contribution.
public static final class ChildInstallsFloatingFactoryMethod {
@Component(modules = ParentModule.class)
public interface Parent {
@ParentFoo Foo getParentFoo();

Child getChild();

FloatingSub getFloatingSub();
}

@Subcomponent(modules = ChildModule.class)
public interface Child {
@ChildFoo Foo getChildFoo();

FloatingSub getFloatingSub();
}

@Subcomponent
public interface FloatingSub {
Foo getFoo();
}

@Module
public interface ParentModule {
@Provides
@IntoSet
static String provideStringMulti() {
return "parent";
}

@Provides
@ParentFoo
static Foo provideParentFoo(Parent componentSelf) {
return componentSelf.getFloatingSub().getFoo();
}
}

@Module
public interface ChildModule {
@Provides
@IntoSet
static String provideStringMulti() {
return "child";
}

@Provides
@ChildFoo
static Foo provideChildFoo(Child componentSelf) {
return componentSelf.getFloatingSub().getFoo();
}
}

private ChildInstallsFloatingFactoryMethod() {}
}

@Test
public void testChildInstallsFloating() {
ChildInstallsFloating.Parent parentComponent =
DaggerSubcomponentBuilderMultibindingsTest_ChildInstallsFloating_Parent.create();
assertThat(parentComponent.getParentFoo().multi).containsExactly("parent");
assertThat(parentComponent.getChild().getChildFoo().multi).containsExactly("parent", "child");
}

@Test
public void testChildDoesNotInstallFloating() {
ChildDoesNotInstallFloating.Parent parentComponent =
DaggerSubcomponentBuilderMultibindingsTest_ChildDoesNotInstallFloating_Parent.create();
assertThat(parentComponent.getParentFoo().multi).containsExactly("parent");
// Don't expect the child contribution because the child didn't redeclare the subcomponent
// dependency, meaning it intends to just use the subcomponent relationship from the parent
// component.
assertThat(parentComponent.getChild().getChildFoo().multi).containsExactly("parent");
}

@Test
public void testChildInstallsFloatingFactoryMethod() {
ChildInstallsFloatingFactoryMethod.Parent parentComponent =
DaggerSubcomponentBuilderMultibindingsTest_ChildInstallsFloatingFactoryMethod_Parent.create();
assertThat(parentComponent.getParentFoo().multi).containsExactly("parent");
assertThat(parentComponent.getChild().getChildFoo().multi).containsExactly("parent", "child");
}
}

0 comments on commit c2d097f

Please sign in to comment.