Add more transport version validation cases (#134597)

This commit adds a few more validations:
* we cannot jump the primary id more than the normal increment
* we cannot remove an existing id

Also fixed definition path output in some validation error messages.

Author: rjernst

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

@@ -126,8 +126,9 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest {
                 currentUpperBoundName = '9.2'
             }
         """
-        referableTransportVersion("existing_91", "8012000")
-        referableTransportVersion("existing_92", "8123000,8012001")
+        referableAndReferencedTransportVersion("existing_91", "8012000")
+        referableAndReferencedTransportVersion("older_92", "8122000")
+        referableAndReferencedTransportVersion("existing_92", "8123000,8012001")
         unreferableTransportVersion("initial_9.0.0", "8000000")
         unreferableTransportVersion("initial_8.19.7", "7123001")
         transportVersionUpperBound("9.2", "existing_92", "8123000")
@@ -140,10 +141,6 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest {
                 return null;
             }
         """)
-        javaSource("myserver", "org.elasticsearch", "Dummy", "", """
-            static final TransportVersion existing91 = TransportVersion.fromName("existing_91");
-            static final TransportVersion existing92 = TransportVersion.fromName("existing_92");
-        """)
 
         file("myplugin/build.gradle") << """
             apply plugin: 'java-library'

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

@@ -357,7 +357,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "new_tv,8123100")
     }
 
-    def "an invalid increment should fail"() {
+    def "a non-positive increment should fail"() {
         given:
         referencedTransportVersion("new_tv")
 
@@ -368,6 +368,17 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertOutputContains(result.output, "Invalid increment 0, must be a positive integer")
     }
 
+    def "an increment larger than 1000 should fail"() {
+        given:
+        referencedTransportVersion("new_tv")
+
+        when:
+        def result = runGenerateTask("--increment=1001").buildAndFail()
+
+        then:
+        assertOutputContains(result.output, "Invalid increment 1001, must be no larger than 1000")
+    }
+
     def "a new definition exists and is in the latest file, but the version id is wrong and needs to be updated"(){
         given:
         referableAndReferencedTransportVersion("new_tv", "1000000")

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

@@ -118,7 +118,17 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes
         def result = validateResourcesFails()
         then:
         assertValidateResourcesFailure(result, "Transport version definition file " +
-            "[myserver/src/main/resources/transport/definitions/referable/existing_92.csv] modifies existing patch id from 8012001 to 8012002")
+            "[myserver/src/main/resources/transport/definitions/referable/existing_92.csv] has modified patch id from 8012001 to 8012002")
+    }
+
+    def "cannot change committed ids"() {
+        given:
+        referableTransportVersion("existing_92", "8123000")
+        when:
+        def result = validateResourcesFails()
+        then:
+        assertValidateResourcesFailure(result, "Transport version definition file " +
+            "[myserver/src/main/resources/transport/definitions/referable/existing_92.csv] has removed id 8012001")
     }
 
     def "upper bounds files must reference defined name"() {
@@ -201,8 +211,8 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes
 
     def "upper bound can refer to an unreferable definition"() {
         given:
-        unreferableTransportVersion("initial_10.0.0", "10000000")
-        transportVersionUpperBound("10.0", "initial_10.0.0", "10000000")
+        unreferableTransportVersion("initial_9.3.0", "8124000")
+        transportVersionUpperBound("9.3", "initial_9.3.0", "8124000")
         when:
         def result = gradleRunner(":myserver:validateTransportVersionResources").build()
         then:
@@ -232,24 +242,39 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes
 
     def "highest id in an referable definition should exist in an upper bounds file"() {
         given:
-        referableAndReferencedTransportVersion("some_tv", "10000000")
+        referableAndReferencedTransportVersion("some_tv", "8124000")
         when:
         def result = validateResourcesFails()
         then:
         assertValidateResourcesFailure(result, "Transport version definition file " +
             "[myserver/src/main/resources/transport/definitions/referable/some_tv.csv] " +
-            "has the highest transport version id [10000000] but is not present in any upper bounds files")
+            "has the highest transport version id [8124000] but is not present in any upper bounds files")
     }
 
     def "highest id in an unreferable definition should exist in an upper bounds file"() {
         given:
-        unreferableTransportVersion("initial_10.0.0", "10000000")
+        unreferableTransportVersion("initial_9.3.0", "8124000")
         when:
         def result = validateResourcesFails()
         then:
-        // TODO: this should be _unreferable_ in the error message, but will require some rework
         assertValidateResourcesFailure(result, "Transport version definition file " +
-            "[myserver/src/main/resources/transport/definitions/referable/initial_10.0.0.csv] " +
-            "has the highest transport version id [10000000] but is not present in any upper bounds files")
+            "[myserver/src/main/resources/transport/definitions/unreferable/initial_9.3.0.csv] " +
+            "has the highest transport version id [8124000] but is not present in any upper bounds files")
+    }
+
+    def "primary ids cannot jump ahead too fast"() {
+        given:
+        referableAndReferencedTransportVersion("some_tv", "8125000")
+        transportVersionUpperBound("9.2", "some_tv", "8125000")
+
+        when:
+        def result = validateResourcesFails()
+
+        then:
+        assertValidateResourcesFailure(result, "Transport version definition file " +
+            "[myserver/src/main/resources/transport/definitions/referable/some_tv.csv] " +
+            "has primary id 8125000 which is more than maximum increment 1000 from id 8123000 in definition " +
+            "[myserver/src/main/resources/transport/definitions/referable/existing_92.csv]"
+        )
     }
 }

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

@@ -52,8 +52,8 @@ public void run() throws IOException {
         // minors increment by 1000 to create a unique base, patches increment by 1 as other patches do
         int increment = releaseVersion.getRevision() == 0 ? 1000 : 1;
         var id = TransportVersionId.fromInt(upstreamUpperBound.definitionId().complete() + increment);
-        var definition = new TransportVersionDefinition(initialDefinitionName, List.of(id));
-        resources.writeUnreferableDefinition(definition);
+        var definition = new TransportVersionDefinition(initialDefinitionName, List.of(id), false);
+        resources.writeDefinition(definition);
         var newUpperBound = new TransportVersionUpperBound(upperBoundName, initialDefinitionName, id);
         resources.writeUpperBound(newUpperBound, false);
 

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

@@ -107,7 +107,7 @@ public void run() throws IOException {
         } else {
             List<TransportVersionId> ids = updateUpperBounds(resources, upstreamUpperBounds, targetUpperBoundNames, targetDefinitionName);
             // (Re)write the definition file.
-            resources.writeReferableDefinition(new TransportVersionDefinition(targetDefinitionName, ids));
+            resources.writeDefinition(new TransportVersionDefinition(targetDefinitionName, ids, true));
         }
 
         removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames);
@@ -124,6 +124,9 @@ private List<TransportVersionId> updateUpperBounds(
         if (increment <= 0) {
             throw new IllegalArgumentException("Invalid increment " + increment + ", must be a positive integer");
         }
+        if (increment > 1000) {
+            throw new IllegalArgumentException("Invalid increment " + increment + ", must be no larger than 1000");
+        }
         List<TransportVersionId> ids = new ArrayList<>();
         boolean stageInGit = getResolveConflict().getOrElse(false);
 

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

@@ -13,8 +13,8 @@
 import java.util.ArrayList;
 import java.util.List;
 
-record TransportVersionDefinition(String name, List<TransportVersionId> ids) {
-    public static TransportVersionDefinition fromString(Path file, String contents) {
+record TransportVersionDefinition(String name, List<TransportVersionId> ids, boolean isReferable) {
+    public static TransportVersionDefinition fromString(Path file, String contents, boolean isReferable) {
         String filename = file.getFileName().toString();
         assert filename.endsWith(".csv");
         String name = filename.substring(0, filename.length() - 4);
@@ -41,6 +41,6 @@ public static TransportVersionDefinition fromString(Path file, String contents)
             }
         }
 
-        return new TransportVersionDefinition(name, ids);
+        return new TransportVersionDefinition(name, ids, isReferable);
     }
 }

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

@@ -97,26 +97,27 @@ Path getDefinitionsDir() {
         return transportResourcesDir.resolve(DEFINITIONS_DIR);
     }
 
-    // return the path, relative to the resources dir, of a referable definition
-    private Path getReferableDefinitionRelativePath(String name) {
-        return REFERABLE_DIR.resolve(name + ".csv");
+    // return the path, relative to the resources dir, of a definition
+    private Path getDefinitionRelativePath(String name, boolean isReferable) {
+        Path dir = isReferable ? REFERABLE_DIR : UNREFERABLE_DIR;
+        return dir.resolve(name + ".csv");
     }
 
     /** Return all referable definitions, mapped by their name. */
     Map<String, TransportVersionDefinition> getReferableDefinitions() throws IOException {
-        return readDefinitions(transportResourcesDir.resolve(REFERABLE_DIR));
+        return readDefinitions(transportResourcesDir.resolve(REFERABLE_DIR), true);
     }
 
     /** Return a single referable definition by name */
     TransportVersionDefinition getReferableDefinition(String name) throws IOException {
-        Path resourcePath = transportResourcesDir.resolve(getReferableDefinitionRelativePath(name));
-        return TransportVersionDefinition.fromString(resourcePath, Files.readString(resourcePath, StandardCharsets.UTF_8));
+        Path resourcePath = transportResourcesDir.resolve(getDefinitionRelativePath(name, true));
+        return TransportVersionDefinition.fromString(resourcePath, Files.readString(resourcePath, StandardCharsets.UTF_8), true);
     }
 
     /** Get a referable definition from upstream if it exists there, or null otherwise */
     TransportVersionDefinition getReferableDefinitionFromUpstream(String name) {
-        Path resourcePath = getReferableDefinitionRelativePath(name);
-        return getUpstreamFile(resourcePath, TransportVersionDefinition::fromString);
+        Path resourcePath = getDefinitionRelativePath(name, true);
+        return getUpstreamFile(resourcePath, (path, contents) -> TransportVersionDefinition.fromString(path, contents, true));
     }
 
     /** Get the definition names which have local changes relative to upstream */
@@ -137,17 +138,24 @@ List<String> getChangedReferableDefinitionNames() {
 
     /** Test whether the given referable definition exists */
     boolean referableDefinitionExists(String name) {
-        return Files.exists(transportResourcesDir.resolve(getReferableDefinitionRelativePath(name)));
+        return Files.exists(transportResourcesDir.resolve(getDefinitionRelativePath(name, true)));
     }
 
     /** Return the path within the repository of the given named definition */
-    Path getReferableDefinitionRepositoryPath(TransportVersionDefinition definition) {
-        return rootDir.relativize(transportResourcesDir.resolve(getReferableDefinitionRelativePath(definition.name())));
+    Path getDefinitionPath(TransportVersionDefinition definition) {
+        Path relativePath;
+        if (definition.isReferable()) {
+            relativePath = getDefinitionRelativePath(definition.name(), true);
+        } else {
+            relativePath = getDefinitionRelativePath(definition.name(), false);
+        }
+        return rootDir.relativize(transportResourcesDir.resolve(relativePath));
     }
 
-    void writeReferableDefinition(TransportVersionDefinition definition) throws IOException {
-        Path path = transportResourcesDir.resolve(getReferableDefinitionRelativePath(definition.name()));
-        logger.debug("Writing referable definition [" + definition + "] to [" + path + "]");
+    void writeDefinition(TransportVersionDefinition definition) throws IOException {
+        Path path = transportResourcesDir.resolve(getDefinitionRelativePath(definition.name(), definition.isReferable()));
+        String type = definition.isReferable() ? "referable" : "unreferable";
+        logger.info("Writing " + type + " definition [" + definition + "] to [" + path + "]");
         Files.writeString(
             path,
             definition.ids().stream().map(Object::toString).collect(Collectors.joining(",")) + "\n",
@@ -156,39 +164,19 @@ void writeReferableDefinition(TransportVersionDefinition definition) throws IOEx
     }
 
     void deleteReferableDefinition(String name) throws IOException {
-        Path path = transportResourcesDir.resolve(getReferableDefinitionRelativePath(name));
+        Path path = transportResourcesDir.resolve(getDefinitionRelativePath(name, true));
         Files.deleteIfExists(path);
     }
 
-    // return the path, relative to the resources dir, of an unreferable definition
-    private Path getUnreferableDefinitionRelativePath(String name) {
-        return UNREFERABLE_DIR.resolve(name + ".csv");
-    }
-
     /** Return all unreferable definitions, mapped by their name. */
     Map<String, TransportVersionDefinition> getUnreferableDefinitions() throws IOException {
-        return readDefinitions(transportResourcesDir.resolve(UNREFERABLE_DIR));
+        return readDefinitions(transportResourcesDir.resolve(UNREFERABLE_DIR), false);
     }
 
     /** Get a referable definition from upstream if it exists there, or null otherwise */
     TransportVersionDefinition getUnreferableDefinitionFromUpstream(String name) {
-        Path resourcePath = getUnreferableDefinitionRelativePath(name);
-        return getUpstreamFile(resourcePath, TransportVersionDefinition::fromString);
-    }
-
-    /** Return the path within the repository of the given referable definition */
-    Path getUnreferableDefinitionRepositoryPath(TransportVersionDefinition definition) {
-        return rootDir.relativize(transportResourcesDir.resolve(getUnreferableDefinitionRelativePath(definition.name())));
-    }
-
-    void writeUnreferableDefinition(TransportVersionDefinition definition) throws IOException {
-        Path path = transportResourcesDir.resolve(getUnreferableDefinitionRelativePath(definition.name()));
-        logger.debug("Writing unreferable definition [" + definition + "] to [" + path + "]");
-        Files.writeString(
-            path,
-            definition.ids().stream().map(Object::toString).collect(Collectors.joining(",")) + "\n",
-            StandardCharsets.UTF_8
-        );
+        Path resourcePath = getDefinitionRelativePath(name, false);
+        return getUpstreamFile(resourcePath, (path, contents) -> TransportVersionDefinition.fromString(path, contents, false));
     }
 
     /** Read all upper bound files and return them mapped by their release name */
@@ -332,15 +320,15 @@ private <T> T getUpstreamFile(Path resourcePath, BiFunction<Path, String, T> par
         return parser.apply(resourcePath, content);
     }
 
-    private static Map<String, TransportVersionDefinition> readDefinitions(Path dir) throws IOException {
+    private static Map<String, TransportVersionDefinition> readDefinitions(Path dir, boolean isReferable) throws IOException {
         if (Files.isDirectory(dir) == false) {
             return Map.of();
         }
         Map<String, TransportVersionDefinition> definitions = new HashMap<>();
         try (var definitionsStream = Files.list(dir)) {
             for (var definitionFile : definitionsStream.toList()) {
                 String contents = Files.readString(definitionFile, StandardCharsets.UTF_8).strip();
-                var definition = TransportVersionDefinition.fromString(definitionFile, contents);
+                var definition = TransportVersionDefinition.fromString(definitionFile, contents, isReferable);
                 definitions.put(definition.name(), definition);
             }
         }