From 0610378d2f5bc72f782496ce760d7713b2257107 Mon Sep 17 00:00:00 2001 From: Greg Turnquist Date: Fri, 13 Dec 2013 09:09:32 -0600 Subject: [PATCH] Resolves #127: Prevent duplicate report outcomes The collection of outcomes is a list. Sometimes a race condition causes to instances of the same outcome to get added to the list shown in the report. By replacing this with a set and propery equals/hashCode, duplicates are prevented from appearing in the report. I added test cases to prove that that POJO is properly managed inside a Set and also to show that duplicates don't appear in the final report. --- .../AutoConfigurationReport.java | 44 ++++--- .../AutoConfigurationReportTests.java | 114 ++++++++++++++++-- .../AbstractJpaAutoConfigurationTests.java | 1 + ...tEmbeddedServletContainerFactoryTests.java | 5 +- 4 files changed, 136 insertions(+), 28 deletions(-) diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/AutoConfigurationReport.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/AutoConfigurationReport.java index b1ab2e60325..6e0df96d85d 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/AutoConfigurationReport.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/AutoConfigurationReport.java @@ -16,13 +16,7 @@ package org.springframework.boot.autoconfigure; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Iterator; -import java.util.List; -import java.util.Map; -import java.util.SortedMap; -import java.util.TreeMap; +import java.util.*; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.NoSuchBeanDefinitionException; @@ -40,9 +34,7 @@ import org.springframework.context.annotation.Condition; public class AutoConfigurationReport { private static final String BEAN_NAME = "autoConfigurationReport"; - private final SortedMap outcomes = new TreeMap(); - private AutoConfigurationReport parent; /** @@ -75,7 +67,7 @@ public class AutoConfigurationReport { /** * The parent report (from a parent BeanFactory if there is one). - * + * * @return the parent report (or null if there isn't one) */ public AutoConfigurationReport getParent() { @@ -115,7 +107,7 @@ public class AutoConfigurationReport { */ public static class ConditionAndOutcomes implements Iterable { - private List outcomes = new ArrayList(); + private Set outcomes = new HashSet(); public void add(Condition condition, ConditionOutcome outcome) { this.outcomes.add(new ConditionAndOutcome(condition, outcome)); @@ -135,7 +127,7 @@ public class AutoConfigurationReport { @Override public Iterator iterator() { - return Collections.unmodifiableList(this.outcomes).iterator(); + return Collections.unmodifiableSet(this.outcomes).iterator(); } } @@ -146,7 +138,6 @@ public class AutoConfigurationReport { public static class ConditionAndOutcome { private final Condition condition; - private final ConditionOutcome outcome; public ConditionAndOutcome(Condition condition, ConditionOutcome outcome) { @@ -161,6 +152,31 @@ public class AutoConfigurationReport { public ConditionOutcome getOutcome() { return this.outcome; } - } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + + if (this.outcome == null || this.outcome.getMessage() == null) { + return false; + } + + ConditionAndOutcome that = (ConditionAndOutcome) o; + + if (that.getOutcome() == null || this.getOutcome().getMessage() == null) { + return false; + } + + return this.getOutcome().getMessage().equals(that.getOutcome().getMessage()); + } + + @Override + public int hashCode() { + return outcome != null && outcome.getMessage() != null ? outcome.getMessage().hashCode() : 0; + } + } } diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/AutoConfigurationReportTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/AutoConfigurationReportTests.java index e657358cfb1..fad46f13ea6 100644 --- a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/AutoConfigurationReportTests.java +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/AutoConfigurationReportTests.java @@ -16,8 +16,10 @@ package org.springframework.boot.autoconfigure; +import java.util.HashSet; import java.util.Iterator; import java.util.Map; +import java.util.Set; import org.junit.Before; import org.junit.Test; @@ -29,17 +31,19 @@ import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.boot.autoconfigure.AutoConfigurationReport.ConditionAndOutcome; import org.springframework.boot.autoconfigure.AutoConfigurationReport.ConditionAndOutcomes; import org.springframework.boot.autoconfigure.condition.ConditionOutcome; +import org.springframework.boot.autoconfigure.web.MultipartAutoConfiguration; import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Condition; import org.springframework.context.annotation.Import; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.nullValue; -import static org.hamcrest.Matchers.sameInstance; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; import static org.mockito.BDDMockito.given; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; /** * Tests for {@link AutoConfigurationReport}. @@ -88,8 +92,7 @@ public class AutoConfigurationReportTests { @Test public void parent() throws Exception { this.beanFactory.setParentBeanFactory(new DefaultListableBeanFactory()); - AutoConfigurationReport.get((ConfigurableListableBeanFactory) this.beanFactory - .getParentBeanFactory()); + AutoConfigurationReport.get((ConfigurableListableBeanFactory) this.beanFactory.getParentBeanFactory()); assertThat(this.report, sameInstance(AutoConfigurationReport.get(this.beanFactory))); assertThat(this.report, not(nullValue())); @@ -98,12 +101,15 @@ public class AutoConfigurationReportTests { @Test public void recordConditionEvaluations() throws Exception { + given(this.outcome1.getMessage()).willReturn("Message 1"); + given(this.outcome2.getMessage()).willReturn("Message 2"); + given(this.outcome3.getMessage()).willReturn("Message 3"); + this.report.recordConditionEvaluation("a", this.condition1, this.outcome1); this.report.recordConditionEvaluation("a", this.condition2, this.outcome2); this.report.recordConditionEvaluation("b", this.condition3, this.outcome3); - Map map = this.report - .getConditionAndOutcomesBySource(); + Map map = this.report.getConditionAndOutcomesBySource(); assertThat(map.size(), equalTo(2)); Iterator iterator = map.get("a").iterator(); ConditionAndOutcome conditionAndOutcome = iterator.next(); @@ -146,16 +152,102 @@ public class AutoConfigurationReportTests { @Test @SuppressWarnings("resource") public void springBootConditionPopulatesReport() throws Exception { - AutoConfigurationReport report = AutoConfigurationReport - .get(new AnnotationConfigApplicationContext(Config.class) - .getBeanFactory()); + AutoConfigurationReport report = AutoConfigurationReport.get(new AnnotationConfigApplicationContext( + Config.class).getBeanFactory()); assertThat(report.getConditionAndOutcomesBySource().size(), not(equalTo(0))); } + @Test + public void testDuplicateConditionAndOutcomes() { + Condition condition1 = mock(Condition.class); + ConditionOutcome conditionOutcome1 = mock(ConditionOutcome.class); + given(conditionOutcome1.getMessage()).willReturn("This is message 1"); + + Condition condition2 = mock(Condition.class); + ConditionOutcome conditionOutcome2 = mock(ConditionOutcome.class); + given(conditionOutcome2.getMessage()).willReturn("This is message 2"); + + Condition condition3 = mock(Condition.class); + ConditionOutcome conditionOutcome3 = mock(ConditionOutcome.class); + given(conditionOutcome3.getMessage()).willReturn("This is message 2"); // identical in value to #2 + + ConditionAndOutcome outcome1 = new ConditionAndOutcome(condition1, + conditionOutcome1); + assertThat(outcome1, equalTo(outcome1)); + + ConditionAndOutcome outcome2 = new ConditionAndOutcome(condition2, + conditionOutcome2); + assertThat(outcome1, not(equalTo(outcome2))); + + ConditionAndOutcome outcome3 = new ConditionAndOutcome(condition3, + conditionOutcome3); + assertThat(outcome2, equalTo(outcome3)); + + Set set = new HashSet(); + set.add(outcome1); + set.add(outcome2); + set.add(outcome3); + assertThat(set.size(), equalTo(2)); + + ConditionAndOutcomes outcomes = new ConditionAndOutcomes(); + outcomes.add(condition1, conditionOutcome1); + outcomes.add(condition2, conditionOutcome2); + outcomes.add(condition3, conditionOutcome3); + + int i = 0; + Iterator iterator = outcomes.iterator(); + while (iterator.hasNext()) { + i++; + iterator.next(); + } + assertThat(i, equalTo(2)); + } + + @Test + public void duplicateOutcomes() { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + DuplicateConfig.class); + AutoConfigurationReport report = AutoConfigurationReport.get(context.getBeanFactory()); + String autoconfigKey = "org.springframework.boot.autoconfigure.web.MultipartAutoConfiguration"; + + assertThat(report.getConditionAndOutcomesBySource().keySet(), + hasItem(autoconfigKey)); + + ConditionAndOutcomes conditionAndOutcomes = report.getConditionAndOutcomesBySource().get( + autoconfigKey); + Iterator iterator = conditionAndOutcomes.iterator(); + int i = 0; + boolean foundConditionalOnClass = false; + boolean foundConditionalOnBean = false; + while (iterator.hasNext()) { + ConditionAndOutcome conditionAndOutcome = iterator.next(); + if (conditionAndOutcome.getOutcome().getMessage().contains( + "@ConditionalOnClass classes found: javax.servlet.Servlet,org.springframework.web.multipart.support.StandardServletMultipartResolver")) { + foundConditionalOnClass = true; + } + if (conditionAndOutcome.getOutcome().getMessage().contains( + "@ConditionalOnBean (types: javax.servlet.MultipartConfigElement; SearchStrategy: all) found no beans")) { + foundConditionalOnBean = true; + } + i++; + } + + assertThat(i, equalTo(2)); + assertTrue(foundConditionalOnClass); + assertTrue(foundConditionalOnBean); + + } + @Configurable @Import(WebMvcAutoConfiguration.class) static class Config { } + @Configurable + @Import(MultipartAutoConfiguration.class) + static class DuplicateConfig { + + } + } diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/AbstractJpaAutoConfigurationTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/AbstractJpaAutoConfigurationTests.java index 225af0b7d16..0a02fa618b0 100644 --- a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/AbstractJpaAutoConfigurationTests.java +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/orm/jpa/AbstractJpaAutoConfigurationTests.java @@ -216,4 +216,5 @@ public abstract class AbstractJpaAutoConfigurationTests { static class CustomJpaTransactionManager extends JpaTransactionManager { } + } diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java index 508a611b0b8..45ba4a8203b 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java @@ -18,7 +18,6 @@ package org.springframework.boot.context.embedded; import java.io.FileWriter; import java.io.IOException; -import java.net.SocketException; import java.net.URI; import java.net.URISyntaxException; import java.nio.charset.Charset; @@ -99,7 +98,7 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); this.container.start(); - this.thrown.expect(SocketException.class); + this.thrown.expect(IOException.class); getResponse("http://localhost:8080/hello"); } @@ -110,7 +109,7 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { .getEmbeddedServletContainer(exampleServletRegistration()); this.container.start(); this.container.stop(); - this.thrown.expect(SocketException.class); + this.thrown.expect(IOException.class); getResponse("http://localhost:8080/hello"); }