[8.19] Add more transport version validation cases (#134597) (#134946)

* 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.

* Only validate primary ids on release branches (#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.

* fix compile

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

@@ -125,9 +125,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")
@@ -140,10 +144,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 abstract class GenerateInitialTransportVersionTask extends DefaultTask {
         // 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 abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
         } 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 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
         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.nio.file.Path;
 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 @@ record TransportVersionDefinition(String name, List<TransportVersionId> ids) {
             }
         }
 
-        return new TransportVersionDefinition(name, ids);
+        return new TransportVersionDefinition(name, ids, isReferable);
     }
 }

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

@@ -12,6 +12,9 @@ package org.elasticsearch.gradle.internal.transport;
 import org.elasticsearch.gradle.Version;
 import org.elasticsearch.gradle.internal.ProjectSubscribeServicePlugin;
 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;
@@ -30,6 +33,7 @@ public class TransportVersionResourcesPlugin implements Plugin<Project> {
     public void apply(Project project) {
         project.getPluginManager().apply(LifecycleBasePlugin.class);
         project.getPluginManager().apply(VersionPropertiesPlugin.class);
+        project.getPluginManager().apply(PrecommitTaskPlugin.class);
         var psService = project.getPlugins().apply(ProjectSubscribeServicePlugin.class).getService();
 
         Properties versions = (Properties) project.getExtensions().getByName(VersionPropertiesPlugin.VERSIONS_EXT);
@@ -66,8 +70,9 @@ public class TransportVersionResourcesPlugin implements Plugin<Project> {
                 t.getReferencesFiles().setFrom(tvReferencesConfig);
                 t.getShouldValidateDensity().convention(true);
                 t.getShouldValidatePrimaryIdNotPatch().convention(true);
+                t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
             });
-        project.getTasks().named(LifecycleBasePlugin.CHECK_TASK_NAME).configure(t -> t.dependsOn(validateTask));
+        project.getTasks().named(PrecommitPlugin.PRECOMMIT_TASK_NAME).configure(t -> t.dependsOn(validateTask));
 
         var generateManifestTask = project.getTasks()
             .register("generateTransportVersionManifest", GenerateTransportVersionManifestTask.class, t -> {
@@ -76,19 +81,31 @@ public class TransportVersionResourcesPlugin implements Plugin<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);

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

@@ -97,26 +97,27 @@ public abstract class TransportVersionResourcesService implements BuildService<T
         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 @@ public abstract class TransportVersionResourcesService implements BuildService<T
 
     /** 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 @@ public abstract class TransportVersionResourcesService implements BuildService<T
     }
 
     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,7 +320,7 @@ public abstract class TransportVersionResourcesService implements BuildService<T
         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();
         }
@@ -340,7 +328,7 @@ public abstract class TransportVersionResourcesService implements BuildService<T
         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);
             }
         }

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

@@ -60,6 +60,13 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
     @Input
     public abstract Property<Boolean> getShouldValidatePrimaryIdNotPatch();
 
+    /**
+     * The name of the upper bounds file which will be used at runtime on the current branch. Normally
+     * this equates to VersionProperties.getElasticsearchVersion().
+     */
+    @Input
+    public abstract Property<String> getCurrentUpperBoundName();
+
     private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
 
     private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+");
@@ -76,6 +83,7 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
         Map<String, TransportVersionDefinition> allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions);
         Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(allDefinitions.values());
         Map<String, TransportVersionUpperBound> upperBounds = resources.getUpperBounds();
+        boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds);
 
         for (var definition : referableDefinitions.values()) {
             validateNamedDefinition(definition, referencedNames);
@@ -93,7 +101,9 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
             validateUpperBound(upperBound, allDefinitions, idsByBase);
         }
 
-        validateLargestIdIsUsed(upperBounds, allDefinitions);
+        if (onReleaseBranch == false) {
+            validatePrimaryIds(resources, upperBounds, allDefinitions);
+        }
     }
 
     private Map<String, TransportVersionDefinition> collectAllDefinitions(
@@ -104,7 +114,7 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
         for (var entry : unreferableDefinitions.entrySet()) {
             TransportVersionDefinition existing = allDefinitions.put(entry.getKey(), entry.getValue());
             if (existing != null) {
-                Path unreferablePath = getResources().get().getUnreferableDefinitionRepositoryPath(entry.getValue());
+                Path unreferablePath = getResources().get().getDefinitionPath(entry.getValue());
                 throwDefinitionFailure(existing, "has same name as unreferable definition [" + unreferablePath + "]");
             }
         }
@@ -131,15 +141,6 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
     }
 
     private void validateNamedDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
-
-        // validate any modifications
-        Map<Integer, TransportVersionId> existingIdsByBase = new HashMap<>();
-        TransportVersionDefinition originalDefinition = getResources().get().getReferableDefinitionFromUpstream(definition.name());
-        if (originalDefinition != null) {
-            validateIdenticalPrimaryId(definition, originalDefinition);
-            originalDefinition.ids().forEach(id -> existingIdsByBase.put(id.base(), id));
-        }
-
         if (referencedNames.contains(definition.name()) == false) {
             throwDefinitionFailure(definition, "is not referenced");
         }
@@ -164,11 +165,28 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
                     throwDefinitionFailure(definition, "contains bwc id [" + id + "] with a patch part of 0");
                 }
             }
-
-            // check modifications of ids on same name, ie sharing same base
-            TransportVersionId maybeModifiedId = existingIdsByBase.get(id.base());
-            if (maybeModifiedId != null && maybeModifiedId.complete() != id.complete()) {
-                throwDefinitionFailure(definition, "modifies existing patch id from " + maybeModifiedId + " to " + id);
+        }
+        // validate any modifications
+        TransportVersionDefinition originalDefinition = getResources().get().getReferableDefinitionFromUpstream(definition.name());
+        if (originalDefinition != null) {
+            validateIdenticalPrimaryId(definition, originalDefinition);
+            for (int i = 1; i < originalDefinition.ids().size(); ++i) {
+                TransportVersionId originalId = originalDefinition.ids().get(i);
+
+                // we have a very small number of ids in a definition, so just search linearly
+                boolean found = false;
+                for (int j = 1; j < definition.ids().size(); ++j) {
+                    TransportVersionId id = definition.ids().get(j);
+                    if (id.base() == originalId.base()) {
+                        found = true;
+                        if (id.complete() != originalId.complete()) {
+                            throwDefinitionFailure(definition, "has modified patch id from " + originalId + " to " + id);
+                        }
+                    }
+                }
+                if (found == false) {
+                    throwDefinitionFailure(definition, "has removed id " + originalId);
+                }
             }
         }
     }
@@ -210,7 +228,7 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
             );
         }
         if (upperBoundDefinition.ids().contains(upperBound.definitionId()) == false) {
-            Path relativePath = getResources().get().getReferableDefinitionRepositoryPath(upperBoundDefinition);
+            Path relativePath = getResources().get().getDefinitionPath(upperBoundDefinition);
             throwUpperBoundFailure(
                 upperBound,
                 "has id " + upperBound.definitionId() + " which is not in definition [" + relativePath + "]"
@@ -254,7 +272,7 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
             IdAndDefinition current = ids.get(ndx);
 
             if (previous.id().equals(current.id())) {
-                Path existingDefinitionPath = getResources().get().getReferableDefinitionRepositoryPath(previous.definition);
+                Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition);
                 throwDefinitionFailure(
                     current.definition(),
                     "contains id " + current.id + " already defined in [" + existingDefinitionPath + "]"
@@ -270,14 +288,33 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
         }
     }
 
-    private void validateLargestIdIsUsed(
+    private void validatePrimaryIds(
+        TransportVersionResourcesService resources,
         Map<String, TransportVersionUpperBound> upperBounds,
         Map<String, TransportVersionDefinition> allDefinitions
     ) {
         // first id is always the highest within a definition, and validated earlier
-        // note we use min instead of max because the id comparator is in descending order
-        var highestDefinition = allDefinitions.values().stream().min(Comparator.comparing(d -> d.ids().get(0))).get();
-        var highestId = highestDefinition.ids().get(0);
+        // note the first element is actually the highest because the id comparator is in descending order
+        var sortedDefinitions = allDefinitions.values().stream().sorted(Comparator.comparing(d -> d.ids().get(0))).toList();
+        TransportVersionDefinition highestDefinition = sortedDefinitions.get(0);
+        TransportVersionId highestId = highestDefinition.ids().get(0);
+
+        if (sortedDefinitions.size() > 1 && getShouldValidateDensity().get()) {
+            TransportVersionDefinition secondHighestDefinition = sortedDefinitions.get(1);
+            TransportVersionId secondHighestId = secondHighestDefinition.ids().get(0);
+            if (highestId.complete() > secondHighestId.complete() + 1000) {
+                throwDefinitionFailure(
+                    highestDefinition,
+                    "has primary id "
+                        + highestId
+                        + " which is more than maximum increment 1000 from id "
+                        + secondHighestId
+                        + " in definition ["
+                        + resources.getDefinitionPath(secondHighestDefinition)
+                        + "]"
+                );
+            }
+        }
 
         for (var upperBound : upperBounds.values()) {
             if (upperBound.definitionId().equals(highestId)) {
@@ -291,8 +328,17 @@ public abstract class ValidateTransportVersionResourcesTask extends DefaultTask
         );
     }
 
+    private boolean checkIfDefinitelyOnReleaseBranch(Map<String, TransportVersionUpperBound> upperBounds) {
+        // only want to look at definitions <= the current upper bound.
+        // TODO: we should filter all of the upper bounds/definitions that are validated by this, not just in this method
+        String currentUpperBoundName = getCurrentUpperBoundName().get();
+        TransportVersionUpperBound currentUpperBound = upperBounds.get(currentUpperBoundName);
+
+        return upperBounds.values().stream().anyMatch(u -> u.definitionId().complete() > currentUpperBound.definitionId().complete());
+    }
+
     private void throwDefinitionFailure(TransportVersionDefinition definition, String message) {
-        Path relativePath = getResources().get().getReferableDefinitionRepositoryPath(definition);
+        Path relativePath = getResources().get().getDefinitionPath(definition);
         throw new VerificationException("Transport version definition file [" + relativePath + "] " + message);
     }