Routes property overrides conflicting app manifest properties

Currently App Broker allows users to specify mutually exclusive
manifest properties - specifically `routes` along with any combination
of the deprecated `host`, `hosts`, `domain` and `domains`. This change
makes `routes` take precedence if specified.

spring-cloud-app-broker-deployer-cloudfoundry/src/main/java/org/springframework/cloud/appbroker/deployer/cloudfoundry/CloudFoundryAppDeployer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -21,6 +21,7 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.time.Duration;
 import java.time.Instant;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -207,7 +208,14 @@ public class CloudFoundryAppDeployer implements AppDeployer, ResourceLoaderAware
 				.doOnSuccess(response -> LOG.info("Success getting application {}", name))
 				.doOnError(e -> LOG.warn(String.format("Error getting application %s: %s", name, e.getMessage())))
 				.map(ApplicationDetail::getId)
-				.flatMap(applicationId -> associateHostName(applicationId, request.getProperties()))
+				.flatMap(applicationId -> {
+					if (request.getProperties().containsKey("routes")) {
+						return associateRoutes(applicationId, request.getProperties());
+					}
+					else {
+						return associateHostName(applicationId, request.getProperties());
+					}
+				})
 				.flatMap(applicationId -> updateEnvironment(request, applicationId))
 				.flatMap(applicationId -> Mono.zip(Mono.just(applicationId),
 					upgradeApplication(request, applicationId)))
@@ -242,7 +250,8 @@ public class CloudFoundryAppDeployer implements AppDeployer, ResourceLoaderAware
 			return Mono.just(applicationId);
 		}
 
-		return operationsUtils.getOperations(properties).map(cfOperations -> cfOperations.domains().list())
+		return operationsUtils.getOperations(properties)
+			.map(cfOperations -> cfOperations.domains().list())
 			.flatMap(Flux::collectList)
 			.map(allDomains -> Stream.concat(Stream.of(domain), domains.stream())
 				.map(d -> getDomainId(d, allDomains))
@@ -257,6 +266,30 @@ public class CloudFoundryAppDeployer implements AppDeployer, ResourceLoaderAware
 			.then(Mono.just(applicationId));
 	}
 
+	private Mono<String> associateRoutes(String applicationId, Map<String, String> properties) {
+		List<String[]> routes = Arrays.stream(properties.get("routes").split(","))
+			.map(uri -> uri.split("\\.", 2))
+			.collect(Collectors.toList());
+
+		return Mono.just(routes)
+			.zipWith(operationsUtils.getOperations(properties)
+				.map(cfOperations -> cfOperations.domains().list())
+				.flatMap(domains -> domains.collectMap(Domain::getName)))
+			.zipWith(getSpaceId(properties))
+			.flatMapMany(routesDomainsAndSpace -> {
+				List<String[]> routesComponents = routesDomainsAndSpace.getT1().getT1();
+				Map<String, Domain> domainsByName = routesDomainsAndSpace.getT1().getT2();
+				String spaceId = routesDomainsAndSpace.getT2();
+
+				return Flux.fromStream(routesComponents.stream()
+					.map(route -> new String[] {route[0], domainsByName.get(route[1]).getId()}))
+					.flatMap(
+						hostAndDomainId -> associateHostForDomain(applicationId, hostAndDomainId[0], hostAndDomainId[1],
+							spaceId));
+			})
+			.then(Mono.just(applicationId));
+	}
+
 	private Mono<Void> associateHostForDomain(String applicationId, String host, String domainId, String spaceId) {
 		return client.routes()
 			.create(org.cloudfoundry.client.v2.routes.CreateRouteRequest.builder()
@@ -541,31 +574,29 @@ public class CloudFoundryAppDeployer implements AppDeployer, ResourceLoaderAware
 			.timeout(healthCheckTimeout(deploymentProperties))
 			.noRoute(toggleNoRoute(deploymentProperties));
 
-		Optional.ofNullable(host(deploymentProperties)).ifPresent(manifest::host);
-		Optional.ofNullable(domain(deploymentProperties)).ifPresent(manifest::domain);
 		Optional.ofNullable(routePath(deploymentProperties)).ifPresent(manifest::routePath);
 
-		if (route(deploymentProperties) != null) {
-			manifest.route(Route.builder().route(route(deploymentProperties)).build());
+		if (routes(deploymentProperties).isEmpty()) {
+			Optional.ofNullable(host(deploymentProperties)).ifPresent(manifest::host);
+			Optional.ofNullable(domain(deploymentProperties)).ifPresent(manifest::domain);
+			if (!domains(deploymentProperties).isEmpty()) {
+				domains(deploymentProperties).forEach(manifest::domain);
+			}
 		}
-
-		if (!routes(deploymentProperties).isEmpty()) {
+		else {
 			Set<Route> routes = routes(deploymentProperties).stream()
 				.map(r -> Route.builder().route(r).build())
 				.collect(Collectors.toSet());
 			manifest.routes(routes);
 		}
 
-		if (!domains(deploymentProperties).isEmpty()) {
-			domains(deploymentProperties).forEach(manifest::domain);
-		}
-
 		if (getDockerImage(appResource) == null) {
 			manifest.buildpack(buildpack(deploymentProperties));
 		}
 		else {
 			manifest.docker(Docker.builder().image(getDockerImage(appResource)).build());
 		}
+
 		return manifest.build();
 	}
 
@@ -820,10 +851,6 @@ public class CloudFoundryAppDeployer implements AppDeployer, ResourceLoaderAware
 		return routePath;
 	}
 
-	private String route(Map<String, String> properties) {
-		return properties.get(CloudFoundryDeploymentProperties.ROUTE_PROPERTY);
-	}
-
 	private Set<String> routes(Map<String, String> properties) {
 		Set<String> routes = new HashSet<>();
 		routes.addAll(this.defaultDeploymentProperties.getRoutes());

spring-cloud-app-broker-deployer-cloudfoundry/src/main/java/org/springframework/cloud/appbroker/deployer/cloudfoundry/CloudFoundryDeploymentProperties.java

@@ -55,11 +55,6 @@ public class CloudFoundryDeploymentProperties extends DeploymentProperties {
 	 */
 	protected static final String ROUTE_PATH_PROPERTY = "route-path";
 
-	/**
-	 * Key for storing the route deployment property
-	 */
-	protected static final String ROUTE_PROPERTY = "route";
-
 	/**
 	 * Key for storing the routes deployment property
 	 */

spring-cloud-app-broker-deployer-cloudfoundry/src/test/java/org/springframework/cloud/appbroker/deployer/cloudfoundry/CloudFoundryAppDeployerTest.java

@@ -41,6 +41,7 @@ import org.cloudfoundry.operations.applications.ApplicationHealthCheck;
 import org.cloudfoundry.operations.applications.ApplicationManifest;
 import org.cloudfoundry.operations.applications.Applications;
 import org.cloudfoundry.operations.applications.PushApplicationManifestRequest;
+import org.cloudfoundry.operations.applications.Route;
 import org.cloudfoundry.operations.organizations.OrganizationDetail;
 import org.cloudfoundry.operations.organizations.OrganizationInfoRequest;
 import org.cloudfoundry.operations.organizations.OrganizationQuota;
@@ -85,7 +86,7 @@ import static org.mockito.BDDMockito.given;
 import static org.mockito.BDDMockito.then;
 import static org.springframework.cloud.appbroker.deployer.DeploymentProperties.TARGET_PROPERTY_KEY;
 
-@SuppressWarnings("UnassignedFluxMonoInstance")
+@SuppressWarnings({"UnassignedFluxMonoInstance", "PMD.ExcessiveClassLength"})
 @ExtendWith(MockitoExtension.class)
 @MockitoSettings(strictness = Strictness.LENIENT)
 class CloudFoundryAppDeployerTest {
@@ -352,6 +353,52 @@ class CloudFoundryAppDeployerTest {
 		then(operationsApplications).should().pushManifest(argThat(matchesManifest(expectedManifest)));
 	}
 
+	@Test
+	void deployAppWithRoutesAndMutuallyExclusiveProperties() {
+		deploymentProperties.setCount(3);
+		deploymentProperties.setMemory("2G");
+		deploymentProperties.setDisk("3G");
+		deploymentProperties.setBuildpack("buildpack1");
+		deploymentProperties.setHealthCheck(ApplicationHealthCheck.HTTP);
+		deploymentProperties.setHealthCheckHttpEndpoint("/healthcheck1");
+		deploymentProperties.setHost("host1");
+		deploymentProperties.setDomain("domain1");
+		deploymentProperties.setDomains(singleton("domain2"));
+
+		DeployApplicationRequest request = DeployApplicationRequest.builder()
+			.name(APP_NAME)
+			.path(APP_PATH)
+			.serviceInstanceId(SERVICE_INSTANCE_ID)
+			.property(DeploymentProperties.COUNT_PROPERTY_KEY, "5")
+			.property(DeploymentProperties.MEMORY_PROPERTY_KEY, "4G")
+			.property(DeploymentProperties.DISK_PROPERTY_KEY, "5G")
+			.property(CloudFoundryDeploymentProperties.HEALTHCHECK_PROPERTY_KEY, "port")
+			.property(CloudFoundryDeploymentProperties.HEALTHCHECK_HTTP_ENDPOINT_PROPERTY_KEY, "/healthcheck2")
+			.property(CloudFoundryDeploymentProperties.BUILDPACK_PROPERTY_KEY, "buildpack2")
+			.property(CloudFoundryDeploymentProperties.DOMAIN_PROPERTY, "domain1")
+			.property(CloudFoundryDeploymentProperties.DOMAINS_PROPERTY, "domain2")
+			.property(CloudFoundryDeploymentProperties.ROUTES_PROPERTY, "route.domain3")
+			.build();
+
+		StepVerifier.create(appDeployer.deploy(request))
+					.assertNext(response -> assertThat(response.getName()).isEqualTo(APP_NAME))
+					.verifyComplete();
+
+		ApplicationManifest expectedManifest = baseManifestWithSpringAppJson()
+			.name(APP_NAME)
+			.path(new File(APP_PATH).toPath())
+			.instances(5)
+			.memory(4096)
+			.disk(5120)
+			.healthCheckType(ApplicationHealthCheck.PORT)
+			.healthCheckHttpEndpoint("/healthcheck2")
+			.buildpack("buildpack2")
+			.routes(Route.builder().route("route.domain3").build())
+			.build();
+
+		then(operationsApplications).should().pushManifest(argThat(matchesManifest(expectedManifest)));
+	}
+
 	@Test
 	void deployAppWithEnvironmentUsingSpringAppJson() {
 		deploymentProperties.setUseSpringApplicationJson(true);

spring-cloud-app-broker-deployer-cloudfoundry/src/test/java/org/springframework/cloud/appbroker/deployer/cloudfoundry/CloudFoundryAppDeployerUpdateApplicationTest.java

@@ -17,8 +17,11 @@
 package org.springframework.cloud.appbroker.deployer.cloudfoundry;
 
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.stream.IntStream;
 
+import org.assertj.core.api.SoftAssertions;
 import org.cloudfoundry.client.CloudFoundryClient;
 import org.cloudfoundry.client.v2.Metadata;
 import org.cloudfoundry.client.v2.applications.ApplicationsV2;
@@ -83,6 +86,7 @@ import static org.mockito.ArgumentMatchers.anyMap;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.BDDMockito.then;
+import static org.mockito.Mockito.times;
 import static org.springframework.cloud.appbroker.deployer.DeploymentProperties.TARGET_PROPERTY_KEY;
 
 @ExtendWith(MockitoExtension.class)
@@ -359,26 +363,7 @@ class CloudFoundryAppDeployerUpdateApplicationTest {
 
 	@Test
 	void updateAppWithHostAndDomain() {
-		given(applicationsV2.update(any()))
-			.willReturn(Mono.just(UpdateApplicationResponse.builder()
-				.build()));
-
-		given(domains.list()).willReturn(getDomains());
-
-		given(spaces.get(any())).willReturn(
-			Mono.just(
-				SpaceDetail.builder()
-					.id("space-id")
-					.name("space-name")
-					.organization("org-name")
-					.build()));
-		given(routes.create(any()))
-			.willReturn(Mono.just(CreateRouteResponse.builder()
-				.metadata(Metadata.builder()
-					.id("route-id")
-					.build()).build()));
-		given(applicationsV2.associateRoute(any()))
-			.willReturn(Mono.empty());
+		mockDomainsAndRoutesUpdate();
 
 		Map<String, String> properties = new HashMap<>();
 		properties.put("host", "my.host");
@@ -402,23 +387,78 @@ class CloudFoundryAppDeployerUpdateApplicationTest {
 			.build());
 		then(applicationsV2).should().associateRoute(AssociateApplicationRouteRequest.builder()
 			.applicationId(APP_ID)
-			.routeId("route-id")
+			.routeId("myDomainComId-route")
 			.build());
 	}
 
+	@Test
+	void updateAppWithHostDomainAndDomains() {
+		mockDomainsAndRoutesUpdate();
+		given(domains.list()).willReturn(Flux.fromStream(IntStream.range(1, 4)
+					.mapToObj(i -> Domain.builder().name("domain" + i).id("domain" + i + "-id").status(Status.OWNED).build())));
+
+		Map<String, String> properties = new HashMap<>();
+		properties.put("host", "foo");
+		properties.put("domain", "domain1");
+		properties.put("domains", "domain2,domain3");
+		UpdateApplicationRequest request = UpdateApplicationRequest.builder()
+			.name(APP_NAME)
+			.properties(properties)
+			.build();
+
+		StepVerifier.create(
+			appDeployer.update(request))
+					.assertNext(response -> assertThat(response.getName()).isEqualTo(APP_NAME))
+					.verifyComplete();
+
+		ArgumentCaptor<CreateRouteRequest> createRouteRequestArgumentCaptor = ArgumentCaptor.forClass(CreateRouteRequest.class);
+		then(routes).should(times(3)).create(createRouteRequestArgumentCaptor.capture());
+
+		List<CreateRouteRequest> routeRequests = createRouteRequestArgumentCaptor.getAllValues();
+		SoftAssertions softAssertions = new SoftAssertions();
+		softAssertions.assertThat(routeRequests.get(0).getHost()).isEqualTo("foo");
+		softAssertions.assertThat(routeRequests.get(0).getDomainId()).isEqualTo("domain1-id");
+		softAssertions.assertThat(routeRequests.get(1).getHost()).isEqualTo("foo");
+		softAssertions.assertThat(routeRequests.get(1).getDomainId()).isEqualTo("domain2-id");
+		softAssertions.assertThat(routeRequests.get(2).getHost()).isEqualTo("foo");
+		softAssertions.assertThat(routeRequests.get(2).getDomainId()).isEqualTo("domain3-id");
+		softAssertions.assertAll();
+	}
+
+	@Test
+	void updateAppWithRoutes() {
+		mockDomainsAndRoutesUpdate();
+
+		given(domains.list()).willReturn(Flux.fromStream(IntStream.range(1, 4)
+					.mapToObj(i -> Domain.builder().name("domain" + i).id("domain" + i + "-id").status(Status.OWNED).build())));
+
+		Map<String, String> properties = singletonMap("routes", "foo.domain1,bar.domain2");
+
+		UpdateApplicationRequest request = UpdateApplicationRequest.builder()
+			.name(APP_NAME)
+			.properties(properties)
+			.build();
+
+		StepVerifier.create(
+			appDeployer.update(request))
+					.assertNext(response -> assertThat(response.getName()).isEqualTo(APP_NAME))
+					.verifyComplete();
+
+		ArgumentCaptor<CreateRouteRequest> createRouteRequestArgumentCaptor = ArgumentCaptor.forClass(CreateRouteRequest.class);
+		then(routes).should(times(2)).create(createRouteRequestArgumentCaptor.capture());
+
+		List<CreateRouteRequest> routeRequests = createRouteRequestArgumentCaptor.getAllValues();
+		SoftAssertions softAssertions = new SoftAssertions();
+		softAssertions.assertThat(routeRequests.get(0).getHost()).isEqualTo("foo");
+		softAssertions.assertThat(routeRequests.get(0).getDomainId()).isEqualTo("domain1-id");
+		softAssertions.assertThat(routeRequests.get(1).getHost()).isEqualTo("bar");
+		softAssertions.assertThat(routeRequests.get(1).getDomainId()).isEqualTo("domain2-id");
+		softAssertions.assertAll();
+	}
+
 	@Test
 	void updateAppWithHostAndMergedDomains() {
-		given(applicationsV2.update(any()))
-			.willReturn(Mono.just(UpdateApplicationResponse.builder()
-				.build()));
-
-		given(spaces.get(any())).willReturn(Mono.just(SpaceDetail.builder()
-			.id("space-id")
-			.name("space-name")
-			.organization("org-name")
-			.build()));
-
-		mockDomainsAndRoutes();
+		mockDomainsAndRoutesUpdate();
 
 		Map<String, String> properties = new HashMap<>();
 		properties.put("host", "my.host");
@@ -442,17 +482,7 @@ class CloudFoundryAppDeployerUpdateApplicationTest {
 
 	@Test
 	void updateAppWithHostAndDomains() {
-		given(applicationsV2.update(any()))
-			.willReturn(Mono.just(UpdateApplicationResponse.builder()
-				.build()));
-
-		given(spaces.get(any())).willReturn(Mono.just(SpaceDetail.builder()
-			.id("space-id")
-			.name("space-name")
-			.organization("org-name")
-			.build()));
-
-		mockDomainsAndRoutes();
+		mockDomainsAndRoutesUpdate();
 
 		Map<String, String> properties = new HashMap<>();
 		properties.put("host", "my.host");
@@ -474,28 +504,7 @@ class CloudFoundryAppDeployerUpdateApplicationTest {
 
 	@Test
 	void updateAppWithHostAndNoDomain() {
-		given(applicationsV2.update(any()))
-			.willReturn(Mono.just(UpdateApplicationResponse.builder()
-				.build()));
-
-		given(domains.list())
-			.willReturn(getDomains());
-
-		given(spaces.get(any()))
-			.willReturn(Mono.just(SpaceDetail.builder()
-				.id("space-id")
-				.name("space-name")
-				.organization("org-name")
-				.build()));
-		given(routes.create(any()))
-			.willReturn(Mono.just(CreateRouteResponse.builder()
-				.metadata(Metadata
-					.builder()
-					.id("route-id")
-					.build())
-				.build()));
-		given(applicationsV2.associateRoute(any()))
-			.willReturn(Mono.empty());
+		mockDomainsAndRoutesUpdate();
 
 		Map<String, String> properties = new HashMap<>();
 		properties.put("host", "my.host");
@@ -517,7 +526,7 @@ class CloudFoundryAppDeployerUpdateApplicationTest {
 			.build());
 		then(applicationsV2).should().associateRoute(AssociateApplicationRouteRequest.builder()
 			.applicationId(APP_ID)
-			.routeId("route-id")
+			.routeId("myDomainDefaultId-route")
 			.build());
 	}
 
@@ -671,7 +680,17 @@ class CloudFoundryAppDeployerUpdateApplicationTest {
 		return domainId + "-route";
 	}
 
-	private void mockDomainsAndRoutes() {
+	private void mockDomainsAndRoutesUpdate() {
+		given(applicationsV2.update(any()))
+			.willReturn(Mono.just(UpdateApplicationResponse.builder()
+				.build()));
+		given(spaces.get(any())).willReturn(
+			Mono.just(
+				SpaceDetail.builder()
+					.id("space-id")
+					.name("space-name")
+					.organization("org-name")
+					.build()));
 		given(domains.list()).willReturn(getDomains());
 		given(routes.create(any())).willAnswer(invocation -> {
 			CreateRouteRequest createRouteRequest = invocation.getArgument(0);