Add back resetting transport upper bounds (#135322)

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.

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,7 +416,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         then:
         assertGenerateAndValidateSuccess(result)
         assertUpperBound("9.2", "existing_92,8123000")
-    }*/
+    }
 
     def "upper bounds files must exist for backport branches"() {
         when:

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.ArrayList;
 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);
 
-        getLogger().lifecycle("Generating transport version name: " + 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);
+
+        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 {
+            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, 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 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
                     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 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
     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 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
         if (changedDefinitions.isEmpty()) {
             return "";
         } else {
-            String changedDefinitionName = changedDefinitions.getFirst();
+            String changedDefinitionName = changedDefinitions.iterator().next();
             if (referencedNames.contains(changedDefinitionName)) {
                 return changedDefinitionName;
             } else {
@@ -248,16 +260,38 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
         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.getLast();
+        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,6 +26,7 @@ import java.nio.file.Files;
 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;
@@ -33,6 +34,7 @@ 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 abstract class TransportVersionResourcesService implements BuildService<T
         Property<String> getUpstreamRefOverride();
     }
 
+    record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
+
     @Inject
     public abstract ExecOperations getExecOperations();
 
@@ -129,19 +133,8 @@ public abstract class TransportVersionResourcesService implements BuildService<T
     }
 
     /** 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 @@ public abstract class TransportVersionResourcesService implements BuildService<T
         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 @@ public abstract class TransportVersionResourcesService implements BuildService<T
         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 @@ public abstract class TransportVersionResourcesService implements BuildService<T
         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 @@ package org.elasticsearch.gradle.internal.transport;
 
 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 org.gradle.api.tasks.VerificationTask;
 
 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 abstract class ValidateTransportVersionResourcesTask extends DefaultTask
     @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 abstract class ValidateTransportVersionResourcesTask extends DefaultTask
         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 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
         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 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
             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 + "]"
                 );
             }