WebServicesAutoConfiguration @ConditionalOnProperty does not match against lists

[snicoll]

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

If you have contributed before, consider leaving this one for someone new, and looking through our general ideal-for-contribution issues. Thanks!

Problem

The WebServicesAutoConfiguration class has a conditional on property but @ConditionalOnProperty does not work with lists. There is a test that exercises the behavior but it only works because the full list is specified in a single property. If it was provided as individual item, such as spring.webservices.wsdl-locations[0] (as one would do with YAML) the condition wouldn't match.

See #9635

Solution

We would need a custom condition similar to this so that can be used instead of @ConditionalOnProperty here. A new test case with one element of the list provided would need to be added to this class.

An abstract class can be created to extract the common code between OnBootstrapHostsCondition and the new condition. Additionally, similar to OnBootstrapHostsConditionTests, we would also need tests for the new condition.

Steps to Fix

• Claim this issue with a comment here, below, and ask any clarifying questions you need
• Set up a repository locally following the Contributing Guidelines
• Try to fix the issue following the steps above
• Commit your changes and start a pull request.

[eneiascs]

Hi, I would like to work on this issue.

[philwebb]

@eneiascs Thanks! Let us know if you need any assistance.

[govi20]

Seems like @eneiascs is not working on it. Would like to take over this issue.

[govi20]

@philwebb Few queries.

• My understanding is @ConditionalOnProperty doesn't work with the list of properties and hence need to be replaced with @conditional with conditional class. is my understanding right?

• If yes, Please help to understand getMatchOutcome() working.

[eneiascs]

Hi @govi20,

Yes, I'm working on it. This is what I've done so far:

First, I created a test case on WebServicesAutoConfigurationTests, which initially fails:

@Test
public void withWsdlBeansAsList() {
	this.contextRunner
			.withPropertyValues(
					"spring.webservices.wsdl-locations[0]=classpath:/wsdl")
			.run((context) -> {
				assertThat(context.getBeansOfType(SimpleWsdl11Definition.class))
						.hasSize(1).containsKey("service");
				assertThat(context.getBeansOfType(SimpleXsdSchema.class)).hasSize(1)
						.containsKey("types");
			});
}

Then, I created OnWsdlLocationsCondition class and the corresponding tests, using the same approach as OnBootstrapHostsCondition.
Next, I changed WebServicesAutoConfiguration to use @Conditional(OnWsdlLocationsCondition.class) instead of @ConditionalOnProperty. At this point, all the tests pass. However, we have duplicated code between OnBootstrapHostsCondition and OnWsdlLocationsCondition. For this reason, I created AbstractListCondition class:

public abstract class AbstractListCondition extends SpringBootCondition {

	private static final Bindable<List<String>> STRING_LIST = Bindable
			.listOf(String.class);

	@Override
	public ConditionOutcome getMatchOutcome(ConditionContext context,
			AnnotatedTypeMetadata metadata) {
		BindResult<?> property = Binder.get(context.getEnvironment())
				.bind(getPropertyName(), STRING_LIST);
		if (property.isBound()) {
			return ConditionOutcome.match(ConditionMessage.forCondition(getClassName())
					.found("property").items(getPropertyName()));
		}
		return ConditionOutcome.noMatch(ConditionMessage.forCondition(getClassName())
				.didNotFind("property").items(getPropertyName()));
	}

	/**
	 * @return the property name
	 */
	protected abstract String getPropertyName();

	/**
	 * @return the class name
	 */
	protected abstract String getClassName();
}

OnBootstrapHostsCondition:

class OnBootstrapHostsCondition extends AbstractListCondition {

	@Override
	protected String getPropertyName() {
		return "spring.couchbase.bootstrap-hosts";
	}

	@Override
	protected String getClassName() {
		return OnBootstrapHostsCondition.class.getName();
	}

}

OnWsdlLocationsCondition:

class OnWsdlLocationsCondition extends AbstractListCondition {

	@Override
	protected String getPropertyName() {
		return "spring.webservices.wsdl-locations";
	}

	@Override
	protected String getClassName() {
		return OnWsdlLocationsCondition.class.getName();
	}

}

OnWsdlLocationsConditionTests:

public class OnWsdlLocationsConditionTests {

	private AnnotationConfigApplicationContext context;

	@After
	public void tearDown() {
		if (this.context != null) {
			this.context.close();
		}
	}

	@Test
	public void wsdlLocationsNotDefined() {
		load(TestConfig.class);
		assertThat(this.context.containsBean("foo")).isFalse();
	}

	@Test
	public void wsdlLocationsDefinedAsCommaSeparated() {
		load(TestConfig.class, "spring.webservices.wsdl-locations=value1");
		assertThat(this.context.containsBean("foo")).isTrue();
	}

	@Test
	public void wsdlLocationsDefinedAsList() {
		load(TestConfig.class, "spring.webservices.wsdl-locations[0]=value1");
		assertThat(this.context.containsBean("foo")).isTrue();
	}

	private void load(Class<?> config, String... environment) {
		this.context = new AnnotationConfigApplicationContext();
		TestPropertyValues.of(environment).applyTo(this.context);
		this.context.register(config);
		this.context.refresh();
	}

	@Configuration
	@Conditional(OnWsdlLocationsCondition.class)
	protected static class TestConfig {

		@Bean
		public String foo() {
			return "foo";
		}

	}

}

@philwebb Is that ok?

[snicoll]

@eneiascs This looks good overall, you've done quite a lot there already! Could you please submit that code as a Pull Request so that we can comment on that if necessary?

[eneiascs]

@snicoll Sure, thanks.