Handle overrides correctly

* Make sure to process all elements before deciding which to retain

2 files changed, 61 insertions, 35 deletions

diff --git a/config-application-package/src/main/java/com/yahoo/config/application/OverrideProcessor.java b/config-application-package/src/main/java/com/yahoo/config/application/OverrideProcessor.java
index 6776119567b..bd913393b9d 100644
--- a/config-application-package/src/main/java/com/yahoo/config/application/OverrideProcessor.java
+++ b/config-application-package/src/main/java/com/yahoo/config/application/OverrideProcessor.java
@@ -138,40 +138,40 @@ class OverrideProcessor implements PreProcessor {
         // Put elements with same attributes in a map with a key that is the concatenation of attribute names
         // (except the override attribute names) and process values for each key
         Map<String, List<Element>> elementsByEqualAttributeSet = elementsByEqualAttributeSet(children);
+
+        // Keep track of elements with highest number of matches (might be more than one element with same tag, need a list)
+        List<Element> bestMatches = new ArrayList<>();
+        int bestMatch = 0;
         for (Map.Entry<String, List<Element>> entry : elementsByEqualAttributeSet.entrySet()) {
-            List<Element> elements = entry.getValue();
-
-            // Keep track of elements with highest number of matches (might be more than one element with same tag, need a list)
-            List<Element> bestMatchElements = new ArrayList<>();
-            int bestMatch = 0;
-            for (Element child : elements) {
-                int overrideCount = getNumberOfOverrides(child, context);
-                if (overrideCount >= bestMatch) {
-                    updateBestMatchElements(bestMatchElements, child, overrideCount, bestMatch);
-                    bestMatch = overrideCount;
-                }
+            for (Element child : entry.getValue()) {
+                bestMatch = findBestMatches(bestMatches, child, bestMatch, context);
             }
-
-            if (bestMatch > 1) { // there was a region/environment specific override
-                doElementSpecificProcessingOnOverride(bestMatchElements);
-                for (Element child : elements) {
-                    // Remove elements not specific
-                    if (!bestMatchElements.contains(child))
-                        parent.removeChild(child);
+        }
+        if (bestMatch > 0) { // there was a region/environment specific override
+            doElementSpecificProcessingOnOverride(bestMatches);
+            for (Element child : children) {
+                if ( ! bestMatches.contains(child)) {
+                    parent.removeChild(child);
                 }
             }
         }
     }
 
-    private void updateBestMatchElements(List<Element> bestMatchElements, Element child, int currentMatch, int bestMatch) {
-        if (bestMatch != currentMatch)
-            bestMatchElements.clear();
+    private int findBestMatches(List<Element> bestMatchElements, Element child, int bestMatch, Context context) {
+        int overrideCount = getNumberOfOverrides(child, context);
+        if (overrideCount >= bestMatch) {
+            if (bestMatch != overrideCount)
+                bestMatchElements.clear();
 
-        bestMatchElements.add(child);
+            bestMatchElements.add(child);
+            return overrideCount;
+        } else {
+            return bestMatch;
+        }
     }
 
     private int getNumberOfOverrides(Element child, Context context) {
-        int currentMatch = 1;
+        int currentMatch = 0;
         Optional<Environment> elementEnvironment = hasEnvironment(child) ? getEnvironment(child) : context.environment;
         RegionName elementRegion = hasRegion(child) ? getRegion(child) : context.region;
         if (elementEnvironment.isPresent() && elementEnvironment.get().equals(environment))
diff --git a/config-application-package/src/test/java/com/yahoo/config/application/HostedOverrideProcessorTest.java b/config-application-package/src/test/java/com/yahoo/config/application/HostedOverrideProcessorTest.java
index 338302e9e57..50ec8062e27 100644
--- a/config-application-package/src/test/java/com/yahoo/config/application/HostedOverrideProcessorTest.java
+++ b/config-application-package/src/test/java/com/yahoo/config/application/HostedOverrideProcessorTest.java
@@ -24,15 +24,17 @@ public class HostedOverrideProcessorTest {
     }
 
     private static final String input =
-                "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>" +
-                "<services xmlns:deploy=\"vespa\" xmlns:preprocess=\"?\" version=\"1.0\">" +
-                "  <container id=\"foo\" version=\"1.0\">" +
-                "    <nodes count='1'/>" +
-                "    <nodes deploy:environment=\"staging\" count='2'/>" +
-                "    <nodes deploy:environment=\"prod\" count='3'/>" +
-                "    <nodes deploy:environment=\"prod\" deploy:region=\"us-west\" count='4'/>" +
-                "  </container>" +
-                "</services>";
+            "<?xml version='1.0' encoding='UTF-8' standalone='no'?>" +
+                    "<services xmlns:deploy='vespa' xmlns:preprocess='?' version='1.0'>" +
+                    "  <container id='foo' version='1.0'>" +
+                    "    <nodes count='1'/>" +
+                    "    <nodes count='3' deploy:environment='perf'/>" +
+                    "    <nodes deploy:environment='staging' count='2' required='true'/>" +
+                    "    <nodes deploy:environment='prod' count='3' flavor='v-4-8-100'/>" +
+                    "    <nodes deploy:environment='prod' deploy:region='us-west' count='4'/>" +
+                    "    <nodes deploy:environment='prod' deploy:region='us-east-3' flavor='v-8-8-100' count='5'/>" +
+                    "  </container>" +
+                    "</services>";
 
 
     @Test
@@ -60,15 +62,39 @@ public class HostedOverrideProcessorTest {
     }
 
     @Test
+    public void testParsingEnvironmentAndRegion2() throws ParserConfigurationException, IOException, SAXException, TransformerException {
+        String expected =
+                "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>" +
+                        "<services xmlns:deploy=\"vespa\" xmlns:preprocess=\"?\" version=\"1.0\">" +
+                        "  <container id=\"foo\" version=\"1.0\">" +
+                        "    <nodes count='5' flavor='v-8-8-100' required='true'/>" +
+                        "  </container>" +
+                        "</services>";
+        assertOverride(Environment.from("prod"), RegionName.from("us-east-3"), expected);
+    }
+
+    @Test
+    public void testParsingEnvironmentAndRegion3() throws ParserConfigurationException, IOException, SAXException, TransformerException {
+        String expected =
+                "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>" +
+                        "<services xmlns:deploy=\"vespa\" xmlns:preprocess=\"?\" version=\"1.0\">" +
+                        "  <container id=\"foo\" version=\"1.0\">" +
+                        "    <nodes count='3' required='true'/>" +
+                        "  </container>" +
+                        "</services>";
+        assertOverride(Environment.from("perf"), RegionName.from("us-east-3"), expected);
+    }
+
+    @Test
     public void testParsingEnvironmentUnknownRegion() throws ParserConfigurationException, IOException, SAXException, TransformerException {
         String expected =
                 "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>" +
                 "<services xmlns:deploy=\"vespa\" xmlns:preprocess=\"?\" version=\"1.0\">" +
                 "  <container id=\"foo\" version=\"1.0\">" +
-                "    <nodes count='3' required='true'/>" +
+                "    <nodes count='3' flavor='v-4-8-100' required='true'/>" +
                 "  </container>" +
                 "</services>";
-        assertOverride(Environment.valueOf("prod"), RegionName.from("us-east"), expected);
+        assertOverride(Environment.valueOf("prod"), RegionName.from("unknown"), expected);
     }
 
     @Test
@@ -77,7 +103,7 @@ public class HostedOverrideProcessorTest {
                 "<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"no\"?>" +
                 "<services xmlns:deploy=\"vespa\" xmlns:preprocess=\"?\" version=\"1.0\">" +
                 "  <container id=\"foo\" version=\"1.0\">" +
-                "    <nodes count='3' required='true'/>" +
+                "    <nodes count='3' flavor='v-4-8-100' required='true'/>" +
                 "  </container>" +
                 "</services>";
         assertOverride(Environment.from("prod"), RegionName.defaultName(), expected);