Address duplicate binding issues

- Deduplicate LinkedKeyBinding by key before falling back to the target key - Rather than just dropping the Guice keys during deduplication, retains the original Guice key if possible. Otherwise bindings may fail to resolve, as is the case with map bindings - Avoid duplicating beans for untargetted bindings - Avoid duplicating multibindings
2022-05-25 14:08:14 +10:00
parent 365c6b9e9e
commit b363700e28
6 changed files with 458 additions and 102 deletions
--- a/src/test/java/org/springframework/guice/AdhocTestSuite.java
+++ b/src/test/java/org/springframework/guice/AdhocTestSuite.java
@@ -28,7 +28,7 @@ import org.springframework.guice.annotation.EnableGuiceModulesTests;
 * @author Dave Syer
 */
@Suite
-@SelectClasses({ BindingDeduplicationTests.class, EnableGuiceModulesTests.class })
+@SelectClasses({ MapBindingDeduplicationTests.class, BindingDeduplicationTests.class, EnableGuiceModulesTests.class })
@Disabled
 public class AdhocTestSuite {

--- a/src/test/java/org/springframework/guice/BindingAnnotationTests.java
+++ b/src/test/java/org/springframework/guice/BindingAnnotationTests.java
@@ -21,6 +21,7 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;

+import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Qualifier;

@@ -35,6 +36,7 @@ import com.google.inject.throwingproviders.CheckedProvides;
 import com.google.inject.throwingproviders.ThrowingProviderBinder;
 import org.junit.jupiter.api.Test;

+import org.springframework.beans.factory.NoUniqueBeanDefinitionException;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
@@ -42,6 +44,7 @@ import org.springframework.context.annotation.Configuration;
 import org.springframework.guice.annotation.EnableGuiceModules;

 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

 public class BindingAnnotationTests {

@@ -105,6 +108,15 @@ public class BindingAnnotationTests {
 		context.close();
 	}

+	@Test
+	public void verifyBindingAnnotationsDuplicateBeans() {
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				BindingAnnotationTestsConfig.class);
+		assertThatExceptionOfType(NoUniqueBeanDefinitionException.class)
+				.isThrownBy(() -> assertThat(context.getBean(SomeService.class)).isNotNull());
+		context.close();
+	}
+
 	public static class SomeDependencyWithQualifierOnProvider {

 	}
@@ -145,7 +157,7 @@ public class BindingAnnotationTests {
 	}

 	@BindingAnnotation
-	@Target({ ElementType.TYPE, ElementType.METHOD, ElementType.FIELD })
+	@Target({ ElementType.TYPE, ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER })
 	@Retention(RetentionPolicy.RUNTIME)
 	@interface SomeBindingAnnotation {

@@ -182,6 +194,32 @@ public class BindingAnnotationTests {

 	}

+	interface SomeService {
+
+	}
+
+	static class BaseSomeService implements SomeService {
+
+	}
+
+	static class ShadowingSomeService implements SomeService {
+
+		@Inject
+		ShadowingSomeService(@SomeBindingAnnotation SomeService baseService) {
+
+		}
+
+	}
+
+	public static class SomeProvider implements javax.inject.Provider<Object> {
+
+		@Override
+		public Object get() {
+			return null;
+		}
+
+	}
+
 	@EnableGuiceModules
 	@Configuration
 	static class BindingAnnotationTestsConfig {
@@ -245,6 +283,11 @@ public class BindingAnnotationTests {
 			return new SomeStringHolder();
 		}

+		@Bean
+		SomeProvider someProvider() {
+			return new SomeProvider();
+		}
+
 		@Bean
 		static AbstractModule module() {
 			return new AbstractModule() {
@@ -253,6 +296,9 @@ public class BindingAnnotationTests {
 					install(ThrowingProviderBinder.forModule(this));
 					bind(String.class).annotatedWith(SomeBindingAnnotation.class).toInstance("annotated");
 					bind(String.class).annotatedWith(SomeOtherBindingAnnotation.class).toInstance("other");
+
+					bind(SomeService.class).annotatedWith(SomeBindingAnnotation.class).to(BaseSomeService.class);
+					bind(SomeService.class).to(ShadowingSomeService.class);
 				}

 				@CheckedProvides(TestCheckedProvider.class)
--- a/src/test/java/org/springframework/guice/BindingDeduplicationTests.java
+++ b/src/test/java/org/springframework/guice/BindingDeduplicationTests.java
@@ -19,10 +19,15 @@ package org.springframework.guice;
 import com.google.inject.AbstractModule;
 import com.google.inject.CreationException;
 import com.google.inject.Module;
+import com.google.inject.Scopes;
+import com.google.inject.multibindings.Multibinder;
 import com.google.inject.multibindings.OptionalBinder;
-import org.junit.jupiter.api.AfterAll;
+import com.google.inject.name.Names;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;

+import org.springframework.beans.factory.NoUniqueBeanDefinitionException;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
@@ -33,20 +38,63 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

 public class BindingDeduplicationTests {

-	@AfterAll
-	public static void cleanUp() {
+	@BeforeEach
+	public void setup() {
+		System.setProperty("spring.guice.dedup", "true");
+	}
+
+	@AfterEach
+	public void cleanUp() {
 		System.clearProperty("spring.guice.dedup");
 	}

 	@Test
 	public void verifyNoDuplicateBindingErrorWhenDedupeEnabled() {
-		System.setProperty("spring.guice.dedup", "true");
+		try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				BindingDeduplicationTestsConfig.class)) {
+			Dependency dependency = context.getBean(Dependency.class);
+			assertThat(dependency).isNotNull();
+
+			OptionalDependency optionalDependency = context.getBean(OptionalDependency.class);
+			assertThat(optionalDependency).isNotNull();
+		}
+	}
+
+	@Test
+	public void annotatedBindingDoesNotDuplicate() {
+		try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				BindingDeduplicationTestsConfig.class)) {
+			FirstInterface firstInterface = context.getBean(FirstInterface.class);
+			assertThat(firstInterface).isNotNull();
+		}
+	}
+
+	@Test
+	public void untargettedBindingDoesNotDuplicate() {
+		try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				BindingDeduplicationTestsConfig.class)) {
+			UntargettedDependency untargettedDependency = context.getBean(UntargettedDependency.class);
+			assertThat(untargettedDependency).isNotNull();
+		}
+	}
+
+	@Test
+	public void setBindingDoesNotDuplicate() {
+		try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				BindingDeduplicationTestsConfig.class)) {
+			SetProvided setProvided = context.getBean(SetProvided.class);
+			assertThat(setProvided).isNotNull();
+		}
+	}
+
+	@Test
+	public void springBindingIsDuplicated() {
 		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
 				BindingDeduplicationTestsConfig.class);
-		SomeDependency someDependency = context.getBean(SomeDependency.class);
-		assertThat(someDependency).isNotNull();
-		SomeOptionalDependency someOptionalDependency = context.getBean(SomeOptionalDependency.class);
-		assertThat(someOptionalDependency).isNotNull();
+
+		assertThatExceptionOfType(NoUniqueBeanDefinitionException.class)
+				.isThrownBy(() -> context.getBean(String.class));
+
 		context.close();
 	}

@@ -60,11 +108,47 @@ public class BindingDeduplicationTests {
 		});
 	}

-	public static class SomeDependency {
+	public interface Dependency {

 	}

-	public static class SomeOptionalDependency {
+	private static class PrivateDependency implements Dependency {
+
+	}
+
+	public static class SomeSingleton {
+
+	}
+
+	public interface OptionalDependency {
+
+	}
+
+	public static class SomeOptionalDependency implements OptionalDependency {
+
+	}
+
+	interface FirstInterface {
+
+	}
+
+	interface SecondInterface {
+
+	}
+
+	static class MultiInterfaceSingleton implements FirstInterface, SecondInterface {
+
+	}
+
+	static class UntargettedDependency {
+
+	}
+
+	interface SetProvided {
+
+	}
+
+	public static class SomeSetProvided implements SetProvided {

 	}

@@ -73,23 +157,53 @@ public class BindingDeduplicationTests {
 	static class BindingDeduplicationTestsConfig {

 		@Bean
-		SomeDependency someBean() {
-			return new SomeDependency();
+		SomeSingleton someSingleton() {
+			return new SomeSingleton();
 		}

 		@Bean
-		SomeOptionalDependency someOptionalBean() {
+		PrivateDependency privateDependency() {
+			return new PrivateDependency();
+		}
+
+		@Bean
+		OptionalDependency someOptionalDependency() {
 			return new SomeOptionalDependency();
 		}

+		@Bean
+		String barString() {
+			return "bar";
+		}
+
+		@Bean
+		SomeSetProvided someSetProvided() {
+			return new SomeSetProvided();
+		}
+
 		@Bean
 		static Module module() {
 			return new AbstractModule() {
 				@Override
 				protected void configure() {
-					bind(SomeDependency.class).asEagerSingleton();
-					OptionalBinder.newOptionalBinder(binder(), SomeOptionalDependency.class).setDefault()
+					bind(Dependency.class).to(PrivateDependency.class);
+					bind(SomeSingleton.class).asEagerSingleton();
+
+					OptionalBinder.newOptionalBinder(binder(), OptionalDependency.class).setDefault()
 							.to(SomeOptionalDependency.class);
+
+					Multibinder<SetProvided> setBinder = Multibinder.newSetBinder(binder(), SetProvided.class);
+					setBinder.addBinding().toInstance(new SomeSetProvided());
+
+					bind(UntargettedDependency.class);
+
+					// Untargetted binding to provide a singleton for the interface
+					// bindings
+					bind(MultiInterfaceSingleton.class).in(Scopes.SINGLETON);
+					bind(FirstInterface.class).to(MultiInterfaceSingleton.class).in(Scopes.SINGLETON);
+					bind(SecondInterface.class).to(MultiInterfaceSingleton.class).in(Scopes.SINGLETON);
+
+					bind(String.class).annotatedWith(Names.named("fooString")).toInstance("foo");
 				}
 			};
 		}
--- a/src/test/java/org/springframework/guice/MapBindingDeduplicationTests.java
+++ b/src/test/java/org/springframework/guice/MapBindingDeduplicationTests.java
@@ -0,0 +1,153 @@
+/*
+ * Copyright 2018-2022 the original author or 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
+ *
+ *      https://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 org.springframework.guice;
+
+import java.util.Map;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.Module;
+import com.google.inject.Provider;
+import com.google.inject.multibindings.MapBinder;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Test;
+
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.core.ResolvableType;
+import org.springframework.guice.annotation.EnableGuiceModules;
+import org.springframework.guice.module.SpringModule;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class MapBindingDeduplicationTests {
+
+	@AfterAll
+	public static void cleanUp() {
+		System.clearProperty("spring.guice.dedup");
+	}
+
+	@Test
+	public void mapBindingGuiceOnly() {
+		System.setProperty("spring.guice.dedup", "false");
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				MapBindingGuiceOnlyTestsConfig.class);
+
+		String[] beanNamesForType = context
+				.getBeanNamesForType(ResolvableType.forClassWithGenerics(Map.class, String.class, Provider.class));
+		@SuppressWarnings("unchecked")
+		Map<String, Provider<Dependency>> dependencyProvider = (Map<String, Provider<Dependency>>) context
+				.getBean(beanNamesForType[0]);
+
+		assertThat(dependencyProvider.size()).isEqualTo(2);
+		assertThat(dependencyProvider.get("someQualifier").get()).isInstanceOf(SomeDependency.class);
+
+		context.close();
+	}
+
+	@Test
+	public void mapBindingConflictingConcreteClass() {
+		System.setProperty("spring.guice.dedup", "true");
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				MapBindingConcreteClassTestsConfig.class);
+
+		String[] beanNamesForType = context
+				.getBeanNamesForType(ResolvableType.forClassWithGenerics(Map.class, String.class, Provider.class));
+		@SuppressWarnings("unchecked")
+		Map<String, Provider<Dependency>> dependencyProvider = (Map<String, Provider<Dependency>>) context
+				.getBean(beanNamesForType[0]);
+
+		assertThat(dependencyProvider.size()).isEqualTo(2);
+		assertThat(dependencyProvider.get("someQualifier").get()).isInstanceOf(SomeDependency.class);
+
+		SomeDependency someDependency = context.getBean(SomeDependency.class);
+		assertThat(someDependency.getSource()).isEqualTo(SpringModule.SPRING_GUICE_SOURCE);
+
+		context.close();
+	}
+
+	interface Dependency {
+
+	}
+
+	public static class SomeDependency implements Dependency {
+
+		private String source = "guice";
+
+		public void setSource(String source) {
+			this.source = source;
+		}
+
+		public String getSource() {
+			return this.source;
+		}
+
+	}
+
+	public static class SomeOptionalDependency implements Dependency {
+
+	}
+
+	@EnableGuiceModules
+	@Configuration
+	static class MapBindingGuiceOnlyTestsConfig {
+
+		@Bean
+		static Module module() {
+			return new AbstractModule() {
+				@Override
+				protected void configure() {
+					MapBinder<String, Dependency> bindings = MapBinder.newMapBinder(binder(), String.class,
+							Dependency.class);
+					bindings.addBinding("someQualifier").to(SomeDependency.class);
+					bindings.addBinding("someOtherQualifier").to(SomeOptionalDependency.class);
+				}
+			};
+		}
+
+	}
+
+	@EnableGuiceModules
+	@Configuration
+	static class MapBindingConcreteClassTestsConfig {
+
+		@Bean
+		SomeDependency dependency() {
+			SomeDependency someDependency = new SomeDependency();
+			someDependency.setSource(SpringModule.SPRING_GUICE_SOURCE);
+			return someDependency;
+		}
+
+		@Bean
+		static Module module() {
+			return new AbstractModule() {
+				@Override
+				protected void configure() {
+					MapBinder<String, Dependency> bindings = MapBinder.newMapBinder(binder(), String.class,
+							Dependency.class);
+					bindings.addBinding("someQualifier").to(SomeDependency.class);
+					// Intentionally duplicate the binding to ensure that every key is
+					// available after deduplication
+					bindings.addBinding("someQualifier").to(SomeDependency.class);
+					bindings.addBinding("someOtherQualifier").to(SomeOptionalDependency.class);
+				}
+			};
+		}
+
+	}
+
+}