From fc837e79239b7088a9c6ddc7183eef7f9ed776e8 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 17 Sep 2025 12:23:55 -0700 Subject: [PATCH 1/3] 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. --- .../AbstractTransportVersionFuncTest.groovy | 9 +-- .../TransportVersionGenerationFuncTest.groovy | 13 +++- .../TransportVersionValidationFuncTest.groovy | 43 ++++++++--- .../GenerateInitialTransportVersionTask.java | 4 +- ...enerateTransportVersionDefinitionTask.java | 5 +- .../transport/TransportVersionDefinition.java | 6 +- .../TransportVersionResourcesService.java | 68 +++++++---------- ...ValidateTransportVersionResourcesTask.java | 73 +++++++++++++------ 8 files changed, 136 insertions(+), 85 deletions(-) diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy index 0cba54ae713b5..358062e3909b4 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy +++ b/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' diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy index 0762607e15b9b..cda7fc665782c 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy +++ b/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") diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy index 36be4c4d94bff..0ef9bc35de631 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy +++ b/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]" + ) } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateInitialTransportVersionTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateInitialTransportVersionTask.java index 31909819c2ad5..b285e18d80dbf 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateInitialTransportVersionTask.java +++ b/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); diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java index 21af9e10557e3..cfeeea76d1d75 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java @@ -111,7 +111,7 @@ public void run() throws IOException { } else { List 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 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 ids = new ArrayList<>(); boolean stageInGit = getResolveConflict().getOrElse(false); diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionDefinition.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionDefinition.java index 5a345c239bd7e..5c73b720a0bc8 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionDefinition.java +++ b/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 ids) { - public static TransportVersionDefinition fromString(Path file, String contents) { +record TransportVersionDefinition(String name, List 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); } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index c115a2a7b1712..06103432e8ec7 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/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 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 */ @@ -136,17 +137,24 @@ List 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", @@ -155,39 +163,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 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 */ @@ -331,7 +319,7 @@ private T getUpstreamFile(Path resourcePath, BiFunction par return parser.apply(resourcePath, content); } - private static Map readDefinitions(Path dir) throws IOException { + private static Map readDefinitions(Path dir, boolean isReferable) throws IOException { if (Files.isDirectory(dir) == false) { return Map.of(); } @@ -339,7 +327,7 @@ private static Map readDefinitions(Path dir) 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); } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java index afbb8d1d38579..6a1796131598b 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java @@ -93,7 +93,7 @@ public void validateTransportVersions() throws IOException { validateUpperBound(upperBound, allDefinitions, idsByBase); } - validateLargestIdIsUsed(upperBounds, allDefinitions); + validatePrimaryIds(resources, upperBounds, allDefinitions); } private Map collectAllDefinitions( @@ -104,7 +104,7 @@ private Map collectAllDefinitions( 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 +131,6 @@ private Map> collectIdsByBase(Collection referencedNames) { - - // validate any modifications - Map 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 +155,28 @@ private void validateNamedDefinition(TransportVersionDefinition definition, Set< 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 +218,7 @@ private void validateUpperBound( ); } 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 +262,7 @@ private void validateBase(int base, List ids) { 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 +278,33 @@ private void validateBase(int base, List ids) { } } - private void validateLargestIdIsUsed( + private void validatePrimaryIds( + TransportVersionResourcesService resources, Map upperBounds, Map 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().getFirst())).toList(); + TransportVersionDefinition highestDefinition = sortedDefinitions.getFirst(); + TransportVersionId highestId = highestDefinition.ids().getFirst(); + + if (sortedDefinitions.size() > 1 && getShouldValidateDensity().get()) { + TransportVersionDefinition secondHighestDefinition = sortedDefinitions.get(1); + TransportVersionId secondHighestId = secondHighestDefinition.ids().getFirst(); + 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)) { @@ -292,7 +319,7 @@ private void validateLargestIdIsUsed( } 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); } From cbec989823cb8d155547686478e6b27017f407c8 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 19 Sep 2025 07:20:05 -0700 Subject: [PATCH 2/3] 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. --- .../AbstractTransportVersionFuncTest.groovy | 3 ++ .../TransportVersionValidationFuncTest.groovy | 17 +++++++++++ .../TransportVersionResourcesPlugin.java | 29 +++++++++++++++---- ...ValidateTransportVersionResourcesTask.java | 21 +++++++++++++- 4 files changed, 63 insertions(+), 7 deletions(-) diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy index 72c52a524dc33..18ebfb6a314d7 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy @@ -125,6 +125,9 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest { tasks.named('generateTransportVersion') { currentUpperBoundName = '9.2' } + tasks.named('validateTransportVersionResources') { + currentUpperBoundName = '9.2' + } """ referableAndReferencedTransportVersion("existing_91", "8012000") referableAndReferencedTransportVersion("older_92", "8122000") diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy index 0ef9bc35de631..861b7a11b8f1a 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy @@ -277,4 +277,21 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes "[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 + } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java index d387c34ff726e..44a00281dabb3 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java @@ -12,6 +12,9 @@ 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 { 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 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(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 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 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); diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java index 6a1796131598b..7161e16a0d7e9 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java @@ -60,6 +60,13 @@ public Path getResourcesDir() { @Input public abstract Property 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 getCurrentUpperBoundName(); + private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {} private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+"); @@ -76,6 +83,7 @@ public void validateTransportVersions() throws IOException { Map allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions); Map> idsByBase = collectIdsByBase(allDefinitions.values()); Map upperBounds = resources.getUpperBounds(); + boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds); for (var definition : referableDefinitions.values()) { validateNamedDefinition(definition, referencedNames); @@ -93,7 +101,9 @@ public void validateTransportVersions() throws IOException { validateUpperBound(upperBound, allDefinitions, idsByBase); } - validatePrimaryIds(resources, upperBounds, allDefinitions); + if (onReleaseBranch == false) { + validatePrimaryIds(resources, upperBounds, allDefinitions); + } } private Map collectAllDefinitions( @@ -318,6 +328,15 @@ private void validatePrimaryIds( ); } + private boolean checkIfDefinitelyOnReleaseBranch(Map 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().getDefinitionPath(definition); throw new VerificationException("Transport version definition file [" + relativePath + "] " + message); From 6d8fff72cb35685bb92ed51088f8669a60bac6ea Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 19 Sep 2025 08:17:02 -0700 Subject: [PATCH 3/3] fix compile --- .../transport/ValidateTransportVersionResourcesTask.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java index 7161e16a0d7e9..571043abca2c8 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java @@ -295,13 +295,13 @@ private void validatePrimaryIds( ) { // first id is always the highest within a definition, and validated earlier // 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().getFirst())).toList(); - TransportVersionDefinition highestDefinition = sortedDefinitions.getFirst(); - TransportVersionId highestId = highestDefinition.ids().getFirst(); + 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().getFirst(); + TransportVersionId secondHighestId = secondHighestDefinition.ids().get(0); if (highestId.complete() > secondHighestId.complete() + 1000) { throwDefinitionFailure( highestDefinition,