Cleanup

3 files changed, 53 insertions, 52 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java b/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java
index df5b10a5cfa..b3d2aa5e2a1 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/Endpoint.java
@@ -4,7 +4,6 @@ package com.yahoo.config.application.api;
 import com.yahoo.config.provision.RegionName;
 
 import java.util.Objects;
-import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
@@ -20,6 +19,8 @@ import java.util.stream.Collectors;
  */
 public class Endpoint {
 
+    public static final String DEFAULT_ID = "default";
+
     /*
      * Endpoint IDs must be:
      * - lowercase
@@ -30,13 +31,12 @@ public class Endpoint {
      */
     private static final Pattern endpointPattern = Pattern.compile("^[a-z](?:-?[a-z0-9]+)*$");
     private static final int endpointMaxLength = 12;
-    private static final String defaultEndpointId = "default";
 
-    private final Optional<String> endpointId;
+    private final String endpointId;
     private final String containerId;
     private final Set<RegionName> regions;
 
-    public Endpoint(Optional<String> endpointId, String containerId, Set<String> regions) {
+    public Endpoint(String endpointId, String containerId, Set<String> regions) {
         this.endpointId = Objects.requireNonNull(endpointId, "endpointId must be non-null");
         this.containerId = Objects.requireNonNull(containerId, "containerId must be non-null");
         this.regions = Set.copyOf(
@@ -50,7 +50,7 @@ public class Endpoint {
     }
 
     public String endpointId() {
-        return endpointId.orElse(defaultEndpointId);
+        return endpointId;
     }
 
     public String containerId() {
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
index 7a4e5ec8857..56539c2eb84 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
@@ -37,6 +37,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -61,6 +62,7 @@ public class DeploymentSpecXmlReader {
     private static final String endpointTag = "endpoint";
     private static final String notificationsTag = "notifications";
 
+
     private static final String idAttribute = "id";
     private static final String athenzServiceAttribute = "athenz-service";
     private static final String athenzDomainAttribute = "athenz-domain";
@@ -263,22 +265,18 @@ public class DeploymentSpecXmlReader {
     private List<Endpoint> readEndpoints(Element parent) {
         var endpointsElement = XML.getChild(parent, endpointsTag);
         if (endpointsElement == null)
-            return Collections.emptyList();
+            return List.of();
 
         var endpoints = new LinkedHashMap<String, Endpoint>();
 
         for (var endpointElement : XML.getChildren(endpointsElement, endpointTag)) {
-            Optional<String> rotationId = stringAttribute("id", endpointElement);
-            Optional<String> containerId = stringAttribute("container-id", endpointElement);
-            var regions = new HashSet<String>();
-
-            if (containerId.isEmpty()) {
-                throw new IllegalArgumentException("Missing 'container-id' from 'endpoint' tag.");
-            }
+            String endpointId = stringAttribute("id", endpointElement).orElse(Endpoint.DEFAULT_ID);
+            String containerId = requireStringAttribute("container-id", endpointElement);
+            Set<String> regions = new HashSet<>();
 
             for (var regionElement : XML.getChildren(endpointElement, "region")) {
                 var region = regionElement.getTextContent();
-                if (region == null || region.isEmpty() || region.isBlank()) {
+                if (region == null || region.isBlank()) {
                     throw new IllegalArgumentException("Empty 'region' element in 'endpoint' tag.");
                 }
                 if (regions.contains(region)) {
@@ -287,7 +285,7 @@ public class DeploymentSpecXmlReader {
                 regions.add(region);
             }
 
-            var endpoint = new Endpoint(rotationId, containerId.get(), regions);
+            Endpoint endpoint = new Endpoint(endpointId, containerId, regions);
             if (endpoints.containsKey(endpoint.endpointId())) {
                 throw new IllegalArgumentException("Duplicate attribute 'id' on 'endpoint': " + endpoint.endpointId());
             }
@@ -350,12 +348,17 @@ public class DeploymentSpecXmlReader {
         }
     }
 
-    /**
-     * Returns the given attribute as a string, or Optional.empty if it is not present or empty
-     */
+    /** Returns the given non-blank attribute of tag as a string, if any */
     private static Optional<String> stringAttribute(String attributeName, Element tag) {
         String value = tag.getAttribute(attributeName);
-        return Optional.ofNullable(value).filter(s -> !s.equals(""));
+        return Optional.ofNullable(value).filter(s -> !s.isBlank());
+    }
+
+    /** Returns the given non-blank attribute of tag or throw */
+    private static String requireStringAttribute(String attributeName, Element tag) {
+        return stringAttribute(attributeName, tag)
+                .orElseThrow(() -> new IllegalArgumentException("Missing required attribute '" + attributeName +
+                                                                "' in '" + tag.getTagName() + "'"));
     }
 
     private DeclaredZone readDeclaredZone(Environment environment, Optional<AthenzService> athenzService,
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
index 053411bc5fb..7cc209f9f7f 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
@@ -3,7 +3,6 @@ package com.yahoo.config.application.api;
 
 import com.google.common.collect.ImmutableSet;
 import com.yahoo.config.provision.Environment;
-import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
 import org.junit.Test;
 
@@ -31,7 +30,7 @@ import static org.junit.Assert.fail;
 public class DeploymentSpecTest {
 
     @Test
-    public void testSpec() {
+    public void simpleSpec() {
         String specXml = "<deployment version='1.0'>" +
                          "   <instance id='default'>" +
                          "      <test/>" +
@@ -52,7 +51,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testSpecPinningMajorVersion() {
+    public void specPinningMajorVersion() {
         String specXml = "<deployment version='1.0' major-version='6'>" +
                          "   <instance id='default'>" +
                          "      <test/>" +
@@ -252,7 +251,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testMultipleInstancesShortForm() {
+    public void multipleInstancesShortForm() {
         StringReader r = new StringReader(
                 "<deployment version='1.0'>" +
                 "   <instance id='instance1, instance2'>" + // The block checked by assertCorrectFirstInstance
@@ -436,7 +435,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testOnlyAthenzServiceDefinedInInstance() {
+    public void onlyAthenzServiceDefinedInInstance() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain'>" +
                 "  <instance id='default' athenz-service='service' />" +
@@ -475,7 +474,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testTestAndStagingOutsideAndInsideInstance() {
+    public void testAndStagingOutsideAndInsideInstance() {
         StringReader r = new StringReader(
                 "<deployment>" +
                 "   <test/>" +
@@ -515,7 +514,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testNestedParallelAndSteps() {
+    public void nestedParallelAndSteps() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain'>" +
                 "   <staging />" +
@@ -588,7 +587,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testParallelInstances() {
+    public void parallelInstances() {
         StringReader r = new StringReader(
                 "<deployment>" +
                 "   <parallel>" +
@@ -617,7 +616,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testInstancesWithDelay() {
+    public void instancesWithDelay() {
         StringReader r = new StringReader(
                 "<deployment>" +
                 "    <instance id='instance0'>" +
@@ -798,7 +797,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void testChangeBlockerInheritance() {
+    public void changeBlockerInheritance() {
         StringReader r = new StringReader(
                 "<deployment version='1.0'>" +
                 "   <block-change revision='false' days='mon,tue' hours='15-16'/>" +
@@ -824,7 +823,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void athenz_config_is_read_from_deployment() {
+    public void athenzConfigIsReadFromDeployment() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain' athenz-service='service'>" +
                 "   <instance id='instance1'>" +
@@ -842,7 +841,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void athenz_config_is_propagated_through_parallel_zones() {
+    public void athenzConfigPropagatesThroughParallelZones() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain' athenz-service='service'>" +
                 "   <instance id='instance1'>" +
@@ -869,7 +868,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void athenz_config_is_propagated_through_parallel_zones_and_instances() {
+    public void athenzConfigPropagatesThroughParallelZonesAndInstances() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain' athenz-service='service'>" +
                 "   <parallel>" +
@@ -903,7 +902,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void athenz_config_is_read_from_instance() {
+    public void athenzConfigIsReadFromInstance() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain'>" +
                 "   <instance id='default' athenz-service='service'>" +
@@ -920,7 +919,7 @@ public class DeploymentSpecTest {
     }
 
     @Test
-    public void athenz_service_is_overridden_from_environment() {
+    public void athenzServiceIsOverriddenFromEnvironment() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain' athenz-service='unused-service'>" +
                 "   <instance id='default' athenz-service='service'>" +
@@ -945,7 +944,7 @@ public class DeploymentSpecTest {
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void it_fails_when_athenz_service_is_not_defined() {
+    public void missingAthenzServiceFails() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain'>" +
                 "   <instance id='default'>" +
@@ -959,7 +958,7 @@ public class DeploymentSpecTest {
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void it_fails_when_athenz_service_is_configured_but_not_athenz_domain() {
+    public void athenzServiceWithoutDomainFails() {
         StringReader r = new StringReader(
                 "<deployment>" +
                 "   <instance id='default'>" +
@@ -1100,7 +1099,7 @@ public class DeploymentSpecTest {
                                           "      <endpoints/>" +
                                           "   </instance>" +
                                           "</deployment>");
-        assertEquals(Collections.emptyList(), spec.requireInstance("default").endpoints());
+        assertEquals(List.of(), spec.requireInstance("default").endpoints());
     }
 
     @Test
@@ -1136,14 +1135,14 @@ public class DeploymentSpecTest {
 
     @Test
     public void invalidEndpoints() {
-        assertInvalid("<endpoint id='FOO' container-id='qrs'/>"); // Uppercase
-        assertInvalid("<endpoint id='123' container-id='qrs'/>"); // Starting with non-character
-        assertInvalid("<endpoint id='foo!' container-id='qrs'/>"); // Non-alphanumeric
-        assertInvalid("<endpoint id='foo.bar' container-id='qrs'/>");
-        assertInvalid("<endpoint id='foo--bar' container-id='qrs'/>"); // Multiple consecutive dashes
-        assertInvalid("<endpoint id='foo-' container-id='qrs'/>"); // Trailing dash
-        assertInvalid("<endpoint id='foooooooooooo' container-id='qrs'/>"); // Too long
-        assertInvalid("<endpoint id='foo' container-id='qrs'/><endpoint id='foo' container-id='qrs'/>"); // Duplicate
+        assertInvalidEndpoints("<endpoint id='FOO' container-id='qrs'/>"); // Uppercase
+        assertInvalidEndpoints("<endpoint id='123' container-id='qrs'/>"); // Starting with non-character
+        assertInvalidEndpoints("<endpoint id='foo!' container-id='qrs'/>"); // Non-alphanumeric
+        assertInvalidEndpoints("<endpoint id='foo.bar' container-id='qrs'/>");
+        assertInvalidEndpoints("<endpoint id='foo--bar' container-id='qrs'/>"); // Multiple consecutive dashes
+        assertInvalidEndpoints("<endpoint id='foo-' container-id='qrs'/>"); // Trailing dash
+        assertInvalidEndpoints("<endpoint id='foooooooooooo' container-id='qrs'/>"); // Too long
+        assertInvalidEndpoints("<endpoint id='foo' container-id='qrs'/><endpoint id='foo' container-id='qrs'/>"); // Duplicate
     }
 
     @Test
@@ -1159,8 +1158,7 @@ public class DeploymentSpecTest {
 
     @Test
     public void endpointDefaultRegions() {
-        var spec = DeploymentSpec.fromXml("" +
-                                          "<deployment>" +
+        var spec = DeploymentSpec.fromXml("<deployment>" +
                                           "   <instance id='default'>" +
                                           "      <prod>" +
                                           "         <region active=\"true\">us-east</region>" +
@@ -1181,10 +1179,10 @@ public class DeploymentSpecTest {
         assertEquals(Set.of("us-east", "us-west"), endpointRegions("default", spec));
     }
 
-    private static void assertInvalid(String endpointTag) {
+    private static void assertInvalidEndpoints(String endpointsBody) {
         try {
-            endpointIds(endpointTag);
-            fail("Expected exception for input '" + endpointTag + "'");
+            endpointIds(endpointsBody);
+            fail("Expected exception for input '" + endpointsBody + "'");
         } catch (IllegalArgumentException ignored) {}
     }
 
@@ -1196,14 +1194,14 @@ public class DeploymentSpecTest {
                 .collect(Collectors.toSet());
     }
 
-    private static List<String> endpointIds(String endpointTag) {
+    private static List<String> endpointIds(String endpointsBody) {
         var xml = "<deployment>" +
                   "   <instance id='default'>" +
                   "      <prod>" +
                   "         <region active=\"true\">us-east</region>" +
                   "      </prod>" +
                   "      <endpoints>" +
-                  endpointTag +
+                  endpointsBody +
                   "      </endpoints>" +
                   "   </instance>" +
                   "</deployment>";