correctly merge optional and non-optional dependencies (#1078)

If a dependency is discovered (non-optional) but created explicitly as optional, it will correctly be categorized as `optional`.

changelog/@unreleased/pr-1078.v2.yml

@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: If a dependency is discovered (non-optional) but declared explicitly as optional, it will be recognized as `optional`.
+  links:
+  - https://github.com/palantir/sls-packaging/pull/1078

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/ProductDependencyMerger.java

@@ -82,12 +82,16 @@ public static ProductDependency merge(ProductDependency dep1, ProductDependency
                 .filter(version -> satisfiesMaxVersion(maximumVersion, version))
                 .max(VersionComparator.INSTANCE);
 
+        // Optional iff both inputs are optional; otherwise, we want to conservatively propagate the strict requirement.
+        boolean optional = dep1.getOptional() && dep2.getOptional();
+
         ProductDependency result = new ProductDependency();
         result.setMinimumVersion(minimumVersion.toString());
         result.setMaximumVersion(maximumVersion.toString());
         recommendedVersion.map(Objects::toString).ifPresent(result::setRecommendedVersion);
         result.setProductGroup(dep1.getProductGroup());
         result.setProductName(dep1.getProductName());
+        result.setOptional(optional);
         result.isValid();
         return result;
     }

gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/tasks/CreateManifestTask.java

@@ -45,6 +45,7 @@
 import java.nio.file.Path;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -222,6 +223,8 @@ final void createManifest() throws IOException {
         }
 
         Map<ProductId, ProductDependency> allProductDependencies = new HashMap<>();
+        Set<ProductId> allOptionalDependencies =
+                new HashSet<>(getOptionalProductIds().get());
         getProductDependencies().get().forEach(declaredDep -> {
             ProductId productId = new ProductId(declaredDep.getProductGroup(), declaredDep.getProductName());
             Preconditions.checkArgument(
@@ -243,6 +246,10 @@ final void createManifest() throws IOException {
             }
             allProductDependencies.merge(
                     productId, declaredDep, (dep1, dep2) -> mergeDependencies(productId, dep1, dep2));
+            if (declaredDep.getOptional()) {
+                log.trace("Product dependency for '{}' declared as optional", productId);
+                allOptionalDependencies.add(productId);
+            }
         });
 
         // Merge all discovered and declared product dependencies
@@ -251,10 +258,6 @@ final void createManifest() throws IOException {
                 log.trace("Ignored product dependency for '{}'", productId);
                 return;
             }
-            if (getOptionalProductIds().get().contains(productId)) {
-                log.trace("Product dependency for '{}' set as optional", productId);
-                discoveredDependency.setOptional(true);
-            }
             allProductDependencies.merge(productId, discoveredDependency, (declaredDependency, _newDependency) -> {
                 ProductDependency mergedDependency =
                         mergeDependencies(productId, declaredDependency, discoveredDependency);
@@ -272,6 +275,15 @@ final void createManifest() throws IOException {
                 return mergedDependency;
             });
         });
+
+        // Ensure optional product dependencies are marked as such.
+        allOptionalDependencies.forEach(
+                productId -> allProductDependencies.computeIfPresent(productId, (_productId, existingDep) -> {
+                    log.trace("Product dependency for '{}' set as optional", productId);
+                    existingDep.setOptional(true);
+                    return existingDep;
+                }));
+
         List<ProductDependency> productDeps = allProductDependencies.values().stream()
                 .sorted(Comparator.comparing(ProductDependency::getProductGroup)
                         .thenComparing(ProductDependency::getProductName))

gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/ProductDependencyMergerTest.groovy

@@ -60,6 +60,20 @@ class ProductDependencyMergerTest extends Specification {
         e.message.contains("minimumVersion and maximumVersion must be different")
     }
 
+    def "merges optional and non-optional"() {
+        given:
+        def dep1 = newRecommendation("2.0.0", "2.x.x")
+        def dep2 = new ProductDependency("group", "name", "2.0.0", "2.x.x", null, true)
+
+        when:
+        def merged = ProductDependencyMerger.merge(dep1, dep2)
+        def merged2 = ProductDependencyMerger.merge(dep2, dep2)
+
+        then:
+        !merged.optional
+        merged2.optional
+    }
+
     private ProductDependency newRecommendation(String min, String max, String recommended = null) {
         return new ProductDependency("group", "name", min, max, recommended)
     }

gradle-sls-packaging/src/test/groovy/com/palantir/gradle/dist/tasks/CreateManifestTaskIntegrationSpec.groovy

@@ -247,13 +247,13 @@ class CreateManifestTaskIntegrationSpec extends GradleIntegrationSpec {
             
             createManifest {
                 productDependencies = [
-                    new com.palantir.gradle.dist.ProductDependency("group", "name", "1.1.0", "1.x.x", "1.2.0"), 
+                    new com.palantir.gradle.dist.ProductDependency("group", "name", "1.1.0", "1.x.x", "1.2.0", true), 
                 ]
             }
         """.stripIndent()
         file('product-dependencies.lock').text = """\
         # Run ./gradlew --write-locks to regenerate this file
-        group:name (1.1.0, 1.x.x)
+        group:name (1.1.0, 1.x.x) optional
         group:name2 (2.0.0, 2.x.x)
         """.stripIndent()
 
@@ -271,7 +271,7 @@ class CreateManifestTaskIntegrationSpec extends GradleIntegrationSpec {
                         "minimum-version"    : "1.1.0",
                         "recommended-version": "1.2.0",
                         "maximum-version"    : "1.x.x",
-                        "optional"           : false
+                        "optional"           : true
 
                 ],
                 [