Add back resetting transport upper bounds (#135322) (#135664)

When upper bounds files have been incorrectly modified, or changes to
them are no longer needed, transport version generation should rebuild
the upper bound. This commit adds back the ability to reset the upper
bound files in these cases and adds scanning for the latest within a
branch to find the appropriate id for a given upper bound file.

Author: rjernst

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy

@@ -90,9 +90,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "new_tv,8124000")
     }
 
-    /*
-    temporarily muted, see https://github.com/elastic/elasticsearch/pull/135226
-
     def "invalid changes to a upper bounds should be reverted"() {
         given:
         transportVersionUpperBound("9.2", "modification", "9000000")
@@ -147,7 +144,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertReferableDefinitionDoesNotExist("test_tv")
         assertUpperBound("9.2", "existing_92,8123000")
         assertUpperBound("9.1", "existing_92,8012001")
-    }*/
+    }
 
     def "a reference can be renamed"() {
         given:
@@ -245,11 +242,8 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
     def "unreferenced definitions are removed"() {
         given:
         referableTransportVersion("test_tv", "8124000,8012002")
-        /*
-        TODO: reset of upper bounds
         transportVersionUpperBound("9.2", "test_tv", "8124000")
         transportVersionUpperBound("9.1", "test_tv", "8012002")
-         */
 
         when:
         def result = runGenerateAndValidateTask().build()
@@ -412,8 +406,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "new_tv,8124000")
     }
 
-    /*
-    TODO: reset of upper bounds
     def "deleted upper bounds files are restored"() {
         given:
         file("myserver/src/main/resources/transport/upper_bounds/9.2.csv").delete()
@@ -424,11 +416,11 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         then:
         assertGenerateAndValidateSuccess(result)
         assertUpperBound("9.2", "existing_92,8123000")
-    }*/
+    }
 
     def "upper bounds files must exist for backport branches"() {
         when:
-        def result = runGenerateTask("--backport-branches=9.1,8.13,7.17,6.0").buildAndFail()
+        def result = runGenerateTask("--name", "new_tv", "--backport-branches=9.1,8.13,7.17,6.0").buildAndFail()
 
         then:
         assertGenerateFailure(result, "Missing upper bounds files for branches [6.0, 7.17, 8.13], known branches are [8.19, 9.0, 9.1, 9.2]")

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java

@@ -9,6 +9,7 @@
 
 package org.elasticsearch.gradle.internal.transport;
 
+import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition;
 import org.gradle.api.DefaultTask;
 import org.gradle.api.file.ConfigurableFileCollection;
 import org.gradle.api.file.RegularFileProperty;
@@ -31,6 +32,7 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -95,29 +97,39 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
     public void run() throws IOException {
         TransportVersionResourcesService resources = getResourceService().get();
         Set<String> referencedNames = TransportVersionReference.collectNames(getReferencesFiles());
-        List<String> changedDefinitionNames = resources.getChangedReferableDefinitionNames();
+        Set<String> changedDefinitionNames = resources.getChangedReferableDefinitionNames();
         String targetDefinitionName = getTargetDefinitionName(resources, referencedNames, changedDefinitionNames);
 
-        List<TransportVersionUpperBound> upstreamUpperBounds = resources.getUpperBoundsFromUpstream();
-        Set<String> targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName);
+        // First check for any unused definitions. This later generation to not get confused by a definition that can't be used.
+        removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames);
 
-        getLogger().lifecycle("Generating transport version name: " + targetDefinitionName);
+        Map<Integer, List<IdAndDefinition>> idsByBase = resources.getIdsByBase();
         if (targetDefinitionName.isEmpty()) {
-            // TODO: resetting upper bounds needs to be done locally, otherwise it pulls in some (incomplete) changes from upstream main
-            // resetAllUpperBounds(resources);
+            getLogger().lifecycle("No transport version name detected, resetting upper bounds");
+            resetAllUpperBounds(resources, idsByBase);
         } else {
-            List<TransportVersionId> ids = updateUpperBounds(resources, upstreamUpperBounds, targetUpperBoundNames, targetDefinitionName);
+            getLogger().lifecycle("Generating transport version name: " + targetDefinitionName);
+            List<TransportVersionUpperBound> upstreamUpperBounds = resources.getUpperBoundsFromUpstream();
+            Set<String> targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName);
+
+            List<TransportVersionId> ids = updateUpperBounds(
+                resources,
+                upstreamUpperBounds,
+                targetUpperBoundNames,
+                idsByBase,
+                targetDefinitionName
+            );
             // (Re)write the definition file.
             resources.writeDefinition(new TransportVersionDefinition(targetDefinitionName, ids, true));
         }
 
-        removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames);
     }
 
     private List<TransportVersionId> updateUpperBounds(
         TransportVersionResourcesService resources,
         List<TransportVersionUpperBound> existingUpperBounds,
         Set<String> targetUpperBoundNames,
+        Map<Integer, List<IdAndDefinition>> idsByBase,
         String definitionName
     ) throws IOException {
         String currentUpperBoundName = getCurrentUpperBoundName().get();
@@ -146,9 +158,9 @@ private List<TransportVersionId> updateUpperBounds(
                     resources.writeUpperBound(newUpperBound, stageInGit);
                 }
                 ids.add(targetId);
-            } else {
+            } else if (resources.getChangedUpperBoundNames().contains(upperBoundName)) {
                 // Default case: we're not targeting this branch so reset it
-                resources.writeUpperBound(existingUpperBound, false);
+                resetUpperBound(resources, existingUpperBound, idsByBase, definitionName);
             }
         }
 
@@ -159,7 +171,7 @@ private List<TransportVersionId> updateUpperBounds(
     private String getTargetDefinitionName(
         TransportVersionResourcesService resources,
         Set<String> referencedNames,
-        List<String> changedDefinitions
+        Set<String> changedDefinitions
     ) {
         if (getDefinitionName().isPresent()) {
             // an explicit name was passed in, so use it
@@ -180,7 +192,7 @@ private String getTargetDefinitionName(
         if (changedDefinitions.isEmpty()) {
             return "";
         } else {
-            String changedDefinitionName = changedDefinitions.get(0);
+            String changedDefinitionName = changedDefinitions.iterator().next();
             if (referencedNames.contains(changedDefinitionName)) {
                 return changedDefinitionName;
             } else {
@@ -248,16 +260,38 @@ private Set<String> getUpperBoundNamesFromDefinition(
         return upperBoundNames;
     }
 
-    private void resetAllUpperBounds(TransportVersionResourcesService resources) throws IOException {
-        for (TransportVersionUpperBound upperBound : resources.getUpperBoundsFromUpstream()) {
-            resources.writeUpperBound(upperBound, false);
+    private void resetAllUpperBounds(TransportVersionResourcesService resources, Map<Integer, List<IdAndDefinition>> idsByBase)
+        throws IOException {
+        for (String upperBoundName : resources.getChangedUpperBoundNames()) {
+            TransportVersionUpperBound upstreamUpperBound = resources.getUpperBoundFromUpstream(upperBoundName);
+            resetUpperBound(resources, upstreamUpperBound, idsByBase, null);
+        }
+    }
+
+    private void resetUpperBound(
+        TransportVersionResourcesService resources,
+        TransportVersionUpperBound upperBound,
+        Map<Integer, List<IdAndDefinition>> idsByBase,
+        String ignoreDefinitionName
+    ) throws IOException {
+        List<IdAndDefinition> idsForUpperBound = idsByBase.get(upperBound.definitionId().base());
+        if (idsForUpperBound == null) {
+            throw new RuntimeException("Could not find base id: " + upperBound.definitionId().base());
+        }
+        IdAndDefinition resetValue = idsForUpperBound.get(idsForUpperBound.size() - 1);
+        if (resetValue.definition().name().equals(ignoreDefinitionName)) {
+            // there must be another definition in this base since the ignored definition is new
+            assert idsForUpperBound.size() >= 2;
+            resetValue = idsForUpperBound.get(idsForUpperBound.size() - 2);
         }
+        var resetUpperBound = new TransportVersionUpperBound(upperBound.name(), resetValue.definition().name(), resetValue.id());
+        resources.writeUpperBound(resetUpperBound, false);
     }
 
     private void removeUnusedNamedDefinitions(
         TransportVersionResourcesService resources,
         Set<String> referencedNames,
-        List<String> changedDefinitions
+        Set<String> changedDefinitions
     ) throws IOException {
         for (String definitionName : changedDefinitions) {
             if (referencedNames.contains(definitionName) == false) {

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java

@@ -26,13 +26,15 @@
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BiFunction;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
 import javax.inject.Inject;
@@ -66,6 +68,8 @@ public interface Parameters extends BuildServiceParameters {
         Property<String> getUpstreamRefOverride();
     }
 
+    record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
+
     @Inject
     public abstract ExecOperations getExecOperations();
 
@@ -129,19 +133,8 @@ TransportVersionDefinition getReferableDefinitionFromUpstream(String name) {
     }
 
     /** Get the definition names which have local changes relative to upstream */
-    List<String> getChangedReferableDefinitionNames() {
-        List<String> changedDefinitions = new ArrayList<>();
-        // make sure the prefix is git style paths, always forward slashes
-        String referablePrefix = REFERABLE_DIR.toString().replace('\\', '/');
-        for (String changedPath : getChangedResources()) {
-            if (changedPath.contains(referablePrefix) == false) {
-                continue;
-            }
-            int lastSlashNdx = changedPath.lastIndexOf('/');
-            String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */);
-            changedDefinitions.add(name);
-        }
-        return changedDefinitions;
+    Set<String> getChangedReferableDefinitionNames() {
+        return getChangedNames(REFERABLE_DIR);
     }
 
     /** Test whether the given referable definition exists */
@@ -187,6 +180,27 @@ TransportVersionDefinition getUnreferableDefinitionFromUpstream(String name) {
         return getUpstreamFile(resourcePath, (path, contents) -> TransportVersionDefinition.fromString(path, contents, false));
     }
 
+    /** Get all the ids and definitions for a given base id, sorted by the complete id */
+    Map<Integer, List<IdAndDefinition>> getIdsByBase() throws IOException {
+        Map<Integer, List<IdAndDefinition>> idsByBase = new HashMap<>();
+
+        // first collect all ids, organized by base
+        Consumer<TransportVersionDefinition> addToBase = definition -> {
+            for (TransportVersionId id : definition.ids()) {
+                idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition));
+            }
+        };
+        getReferableDefinitions().values().forEach(addToBase);
+        getUnreferableDefinitions().values().forEach(addToBase);
+
+        // now sort the ids within each base so we can check density later
+        for (var ids : idsByBase.values()) {
+            // first sort the ids list so we can check compactness and quickly lookup the highest id later
+            ids.sort(Comparator.comparingInt(a -> a.id().complete()));
+        }
+        return idsByBase;
+    }
+
     /** Read all upper bound files and return them mapped by their release name */
     Map<String, TransportVersionUpperBound> getUpperBounds() throws IOException {
         Map<String, TransportVersionUpperBound> upperBounds = new HashMap<>();
@@ -220,6 +234,10 @@ List<TransportVersionUpperBound> getUpperBoundsFromUpstream() throws IOException
         return upperBounds;
     }
 
+    Set<String> getChangedUpperBoundNames() {
+        return getChangedNames(UPPER_BOUNDS_DIR);
+    }
+
     /** Write the given upper bound to a file in the transport resources */
     void writeUpperBound(TransportVersionUpperBound upperBound, boolean stageInGit) throws IOException {
         Path path = transportResourcesDir.resolve(getUpperBoundRelativePath(upperBound.name()));
@@ -317,6 +335,21 @@ private Set<String> getChangedResources() {
         return changedResources.get();
     }
 
+    private Set<String> getChangedNames(Path resourcesDir) {
+        Set<String> changedNames = new HashSet<>();
+        // make sure the prefix is git style paths, always forward slashes
+        String resourcesPrefix = resourcesDir.toString().replace('\\', '/');
+        for (String changedPath : getChangedResources()) {
+            if (changedPath.contains(resourcesPrefix) == false) {
+                continue;
+            }
+            int lastSlashNdx = changedPath.lastIndexOf('/');
+            String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */);
+            changedNames.add(name);
+        }
+        return changedNames;
+    }
+
     // Read a transport version resource from the upstream, or return null if it doesn't exist there
     private <T> T getUpstreamFile(Path resourcePath, BiFunction<Path, String, T> parser) {
         String pathString = resourcePath.toString().replace('\\', '/'); // normalize to forward slash that git uses

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java

@@ -11,6 +11,7 @@
 
 import com.google.common.collect.Comparators;
 
+import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition;
 import org.gradle.api.DefaultTask;
 import org.gradle.api.file.ConfigurableFileCollection;
 import org.gradle.api.provider.Property;
@@ -28,8 +29,6 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
@@ -67,8 +66,6 @@ public Path getResourcesDir() {
     @Input
     public abstract Property<String> getCurrentUpperBoundName();
 
-    private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
-
     private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+");
 
     @ServiceReference("transportVersionResources")
@@ -81,7 +78,7 @@ public void validateTransportVersions() throws IOException {
         Map<String, TransportVersionDefinition> referableDefinitions = resources.getReferableDefinitions();
         Map<String, TransportVersionDefinition> unreferableDefinitions = resources.getUnreferableDefinitions();
         Map<String, TransportVersionDefinition> allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions);
-        Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(allDefinitions.values());
+        Map<Integer, List<IdAndDefinition>> idsByBase = resources.getIdsByBase();
         Map<String, TransportVersionUpperBound> upperBounds = resources.getUpperBounds();
         TransportVersionUpperBound currentUpperBound = upperBounds.get(getCurrentUpperBoundName().get());
         boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds);
@@ -129,25 +126,6 @@ private Map<String, TransportVersionDefinition> collectAllDefinitions(
         return allDefinitions;
     }
 
-    private Map<Integer, List<IdAndDefinition>> collectIdsByBase(Collection<TransportVersionDefinition> definitions) {
-        Map<Integer, List<IdAndDefinition>> idsByBase = new HashMap<>();
-
-        // first collect all ids, organized by base
-        for (TransportVersionDefinition definition : definitions) {
-            for (TransportVersionId id : definition.ids()) {
-                idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition));
-            }
-        }
-
-        // now sort the ids within each base so we can check density later
-        for (var ids : idsByBase.values()) {
-            // first sort the ids list so we can check compactness and quickly lookup the highest id later
-            ids.sort(Comparator.comparingInt(a -> a.id().complete()));
-        }
-
-        return idsByBase;
-    }
-
     private void validateNamedDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
         if (referencedNames.contains(definition.name()) == false) {
             throwDefinitionFailure(definition, "is not referenced");
@@ -280,10 +258,10 @@ private void validateBase(int base, List<IdAndDefinition> ids) {
             IdAndDefinition current = ids.get(ndx);
 
             if (previous.id().equals(current.id())) {
-                Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition);
+                Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition());
                 throwDefinitionFailure(
                     current.definition(),
-                    "contains id " + current.id + " already defined in [" + existingDefinitionPath + "]"
+                    "contains id " + current.id() + " already defined in [" + existingDefinitionPath + "]"
                 );
             }