[9.1] Add more transport version validation cases (elastic#134597) (elastic#134947)

* Add more transport version validation cases (elastic#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.

* Only validate primary ids on release branches (elastic#135044)

Primary ids are only incremented on the main branch. For release branches
only patch ids will be created. The primary id validation won't always
work on release branches because only some of the primary ids will be
backported.

This commit skips primary id validation if we are sure we are on a
release branch. We can tell this by looking at the current upper bound
compared to other upper bounds. If there is a newer one, we know we are
on a release branch. If there isn't a newer one, we _might_ be on a
release branch, or on main, but in this case doing primary id validation
is ok because there's effectively no difference, we are looking at the
latest upper bound.

Author: rjernst

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

@@ -128,9 +128,13 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest {
             tasks.named('generateTransportVersion') {
                 currentUpperBoundName = '9.2'
             }
+            tasks.named('validateTransportVersionResources') {
+                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")
@@ -143,10 +147,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

@@ -428,7 +428,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")
 
@@ -439,6 +439,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,56 @@ 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]"
+        )
+    }
+
+    def "primary id checks skipped on release branch"() {
+        given:
+        file("myserver/build.gradle") << """
+            tasks.named('validateTransportVersionResources') {
+                currentUpperBoundName = '9.1'
+            }
+        """
+        referableAndReferencedTransportVersion("some_tv", "8125000")
+        transportVersionUpperBound("9.2", "some_tv", "8125000")
+
+        when:
+        def result = gradleRunner("validateTransportVersionResources").build()
+
+        then:
+        result.task(":myserver:validateTransportVersionResources").outcome == TaskOutcome.SUCCESS
     }
 }

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

@@ -111,7 +111,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);
@@ -128,6 +128,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/TransportVersionResourcesPlugin.java

@@ -14,6 +14,7 @@
 import org.elasticsearch.gradle.internal.conventions.VersionPropertiesPlugin;
 import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitPlugin;
 import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitTaskPlugin;
+import org.gradle.api.Action;
 import org.gradle.api.Plugin;
 import org.gradle.api.Project;
 import org.gradle.api.file.Directory;
@@ -73,6 +74,7 @@ public void apply(Project project) {
                 t.getReferencesFiles().setFrom(tvReferencesConfig);
                 t.getShouldValidateDensity().convention(true);
                 t.getShouldValidatePrimaryIdNotPatch().convention(true);
+                t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
             });
         project.getTasks().named(PrecommitPlugin.PRECOMMIT_TASK_NAME).configure(t -> t.dependsOn(validateTask));
 
@@ -83,19 +85,31 @@ public void apply(Project project) {
                 t.getManifestFile().set(project.getLayout().getBuildDirectory().file("generated-resources/manifest.txt"));
             });
         project.getTasks().named(JavaPlugin.PROCESS_RESOURCES_TASK_NAME, Copy.class).configure(t -> {
-            t.into("transport/definitions", c -> c.from(generateManifestTask));
+            t.into(resourceRoot + "/definitions", c -> c.from(generateManifestTask));
         });
 
+        Action<GenerateTransportVersionDefinitionTask> generationConfiguration = t -> {
+            t.setGroup(taskGroup);
+            t.getReferencesFiles().setFrom(tvReferencesConfig);
+            t.getIncrement().convention(1000);
+            t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
+        };
+
         var generateDefinitionsTask = project.getTasks()
             .register("generateTransportVersion", GenerateTransportVersionDefinitionTask.class, t -> {
-                t.setGroup(taskGroup);
                 t.setDescription("(Re)generates a transport version definition file");
-                t.getReferencesFiles().setFrom(tvReferencesConfig);
-                t.getIncrement().convention(1000);
-                t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
             });
+        generateDefinitionsTask.configure(generationConfiguration);
         validateTask.configure(t -> t.mustRunAfter(generateDefinitionsTask));
 
+        var resolveConflictTask = project.getTasks()
+            .register("resolveTransportVersionConflict", GenerateTransportVersionDefinitionTask.class, t -> {
+                t.setDescription("Resolve merge conflicts in transport version internal state files");
+                t.getResolveConflict().set(true);
+            });
+        resolveConflictTask.configure(generationConfiguration);
+        validateTask.configure(t -> t.mustRunAfter(resolveConflictTask));
+
         var generateInitialTask = project.getTasks()
             .register("generateInitialTransportVersion", GenerateInitialTransportVersionTask.class, t -> {
                 t.setGroup(taskGroup);