From 8ed2381b666c3ad91855d73dc641d5e8b52d66b8 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 19 Aug 2025 16:12:34 -0700 Subject: [PATCH 1/2] Add separate validation for unreferenced transport version definitions This commit splits out the validation that is pertinent to unreferenced definitions. It also adds validation that names of named and unreferenced definitions cannot collide. --- .../TransportVersionValidationFuncTest.groovy | 31 ++++++++ .../TransportVersionResourcesService.java | 59 +++++++++----- ...ValidateTransportVersionResourcesTask.java | 79 +++++++++++++------ 3 files changed, 128 insertions(+), 41 deletions(-) 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 52fab5a4f1625..01e16ab69c52b 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 @@ -198,4 +198,35 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes then: result.task(":myserver:validateTransportVersionDefinitions").outcome == TaskOutcome.SUCCESS } + + def "latest can refer to an unreferenced definition"() { + given: + unreferencedTransportVersion("initial_10.0.0", "10000000") + latestTransportVersion("10.0", "initial_10.0.0", "10000000") + when: + def result = gradleRunner(":myserver:validateTransportVersionDefinitions").build() + then: + result.task(":myserver:validateTransportVersionDefinitions").outcome == TaskOutcome.SUCCESS + } + + def "named and unreferenced definitions cannot have the same name"() { + given: + unreferencedTransportVersion("existing_92", "10000000") + when: + def result = validateDefinitionsFails() + then: + assertDefinitionsFailure(result, "Transport version definition file " + + "[myserver/src/main/resources/transport/definitions/named/existing_92.csv] " + + "has same name as unreferenced definition " + + "[myserver/src/main/resources/transport/definitions/unreferenced/existing_92.csv]") + } + + def "unreferenced definitions can have primary ids that are patches"() { + given: + unreferencedTransportVersion("initial_10.0.1", "10000001") + when: + def result = gradleRunner(":myserver:validateTransportVersionDefinitions").build() + then: + result.task(":myserver:validateTransportVersionDefinitions").outcome == TaskOutcome.SUCCESS + } } 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 7101e8d9b8f18..6a5850239a3e3 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 @@ -98,25 +98,10 @@ private Path getNamedDefinitionRelativePath(String name) { /** Return all named definitions, mapped by their name. */ Map getNamedDefinitions() throws IOException { - Map definitions = new HashMap<>(); - // temporarily include unreferenced in named until validation understands the distinction - for (var dir : List.of(NAMED_DIR, UNREFERENCED_DIR)) { - Path path = transportResourcesDir.resolve(dir); - if (Files.isDirectory(path) == false) { - continue; - } - try (var definitionsStream = Files.list(path)) { - for (var definitionFile : definitionsStream.toList()) { - String contents = Files.readString(definitionFile, StandardCharsets.UTF_8).strip(); - var definition = TransportVersionDefinition.fromString(definitionFile.getFileName().toString(), contents); - definitions.put(definition.name(), definition); - } - } - } - return definitions; + return readDefinitions(transportResourcesDir.resolve(NAMED_DIR)); } - /** Test whether the given named definition exists */ + /** Get a named definition from main if it exists there, or null otherwise */ TransportVersionDefinition getNamedDefinitionFromMain(String name) { String resourcePath = getNamedDefinitionRelativePath(name).toString(); return getMainFile(resourcePath, TransportVersionDefinition::fromString); @@ -128,10 +113,31 @@ boolean namedDefinitionExists(String name) { } /** Return the path within the repository of the given named definition */ - Path getRepositoryPath(TransportVersionDefinition definition) { + Path getNamedDefinitionRepositoryPath(TransportVersionDefinition definition) { return rootDir.relativize(transportResourcesDir.resolve(getNamedDefinitionRelativePath(definition.name()))); } + // return the path, relative to the resources dir, of an unreferenced definition + private Path getUnreferencedDefinitionRelativePath(String name) { + return UNREFERENCED_DIR.resolve(name + ".csv"); + } + + /** Return all unreferenced definitions, mapped by their name. */ + Map getUnreferencedDefinitions() throws IOException { + return readDefinitions(transportResourcesDir.resolve(UNREFERENCED_DIR)); + } + + /** Get a named definition from main if it exists there, or null otherwise */ + TransportVersionDefinition getUnreferencedDefinitionFromMain(String name) { + String resourcePath = getUnreferencedDefinitionRelativePath(name).toString(); + return getMainFile(resourcePath, TransportVersionDefinition::fromString); + } + + /** Return the path within the repository of the given named definition */ + Path getUnreferencedDefinitionRepositoryPath(TransportVersionDefinition definition) { + return rootDir.relativize(transportResourcesDir.resolve(getUnreferencedDefinitionRelativePath(definition.name()))); + } + /** Read all latest files and return them mapped by their release branch */ Map getLatestByReleaseBranch() throws IOException { Map latests = new HashMap<>(); @@ -152,7 +158,7 @@ TransportVersionLatest getLatestFromMain(String releaseBranch) { } /** Return the path within the repository of the given latest */ - Path getRepositoryPath(TransportVersionLatest latest) { + Path getLatestRepositoryPath(TransportVersionLatest latest) { return rootDir.relativize(transportResourcesDir.resolve(getLatestRelativePath(latest.branch()))); } @@ -197,6 +203,21 @@ private T getMainFile(String resourcePath, BiFunction par return parser.apply(resourcePath, content); } + private static Map readDefinitions(Path dir) throws IOException { + if (Files.isDirectory(dir) == false) { + return Map.of(); + } + Map 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.getFileName().toString(), contents); + definitions.put(definition.name(), definition); + } + } + return definitions; + } + // run a git command, relative to the transport version resources directory private String gitCommand(String... args) { ByteArrayOutputStream stdout = new ByteArrayOutputStream(); 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 0b4f15eb5ccaa..b808cb4611c51 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,13 +60,20 @@ private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition @TaskAction public void validateTransportVersions() throws IOException { + TransportVersionResourcesService resources = getResources().get(); Set referencedNames = TransportVersionReference.collectNames(getReferencesFiles()); - Map definitions = getResources().get().getNamedDefinitions(); - Map> idsByBase = collectIdsByBase(definitions.values()); - Map latestByReleaseBranch = getResources().get().getLatestByReleaseBranch(); + Map namedDefinitions = resources.getNamedDefinitions(); + Map unreferencedDefinitions = resources.getUnreferencedDefinitions(); + Map allDefinitions = collectAllDefinitions(namedDefinitions, unreferencedDefinitions); + Map> idsByBase = collectIdsByBase(allDefinitions.values()); + Map latestByReleaseBranch = resources.getLatestByReleaseBranch(); + + for (var definition : namedDefinitions.values()) { + validateNamedDefinition(definition, referencedNames); + } - for (var definition : definitions.values()) { - validateDefinition(definition, referencedNames); + for (var definition : unreferencedDefinitions.values()) { + validateUnreferencedDefinition(definition); } for (var entry : idsByBase.entrySet()) { @@ -74,8 +81,20 @@ public void validateTransportVersions() throws IOException { } for (var latest : latestByReleaseBranch.values()) { - validateLatest(latest, definitions, idsByBase); + validateLatest(latest, allDefinitions, idsByBase); + } + } + + private Map collectAllDefinitions(Map namedDefinitions, Map unreferencedDefinitions) { + Map allDefinitions = new HashMap<>(namedDefinitions); + for (var entry : unreferencedDefinitions.entrySet()) { + TransportVersionDefinition existing = allDefinitions.put(entry.getKey(), entry.getValue()); + if (existing != null) { + Path unreferencedPath = getResources().get().getUnreferencedDefinitionRepositoryPath(entry.getValue()); + throwDefinitionFailure(existing, "has same name as unreferenced definition [" + unreferencedPath + "]"); + } } + return allDefinitions; } private Map> collectIdsByBase(Collection definitions) { @@ -97,23 +116,17 @@ private Map> collectIdsByBase(Collection referencedNames) { + private void validateNamedDefinition(TransportVersionDefinition definition, Set referencedNames) { // validate any modifications Map existingIdsByBase = new HashMap<>(); TransportVersionDefinition originalDefinition = getResources().get().getNamedDefinitionFromMain(definition.name()); if (originalDefinition != null) { - - int primaryId = definition.ids().get(0).complete(); - int originalPrimaryId = originalDefinition.ids().get(0).complete(); - if (primaryId != originalPrimaryId) { - throwDefinitionFailure(definition, "has modified primary id from " + originalPrimaryId + " to " + primaryId); - } - + validateIdenticalPrimaryId(definition, originalDefinition); originalDefinition.ids().forEach(id -> existingIdsByBase.put(id.base(), id)); } - if (referencedNames.contains(definition.name()) == false && definition.name().startsWith("initial_") == false) { + if (referencedNames.contains(definition.name()) == false) { throwDefinitionFailure(definition, "is not referenced"); } if (NAME_FORMAT.matcher(definition.name()).matches() == false) { @@ -129,9 +142,7 @@ private void validateDefinition(TransportVersionDefinition definition, Set 1) { + throwDefinitionFailure(definition, " contains more than one id"); + } + // note: no name validation, anything that is a valid filename is ok, this allows eg initial_8.9.1 + } + + private void validateIdenticalPrimaryId(TransportVersionDefinition definition, TransportVersionDefinition originalDefinition) { + assert definition.name().equals(originalDefinition.name()); + + int primaryId = definition.ids().get(0).complete(); + int originalPrimaryId = originalDefinition.ids().get(0).complete(); + if (primaryId != originalPrimaryId) { + throwDefinitionFailure(definition, "has modified primary id from " + originalPrimaryId + " to " + primaryId); + } + } + private void validateLatest( TransportVersionLatest latest, Map definitions, @@ -158,7 +193,7 @@ private void validateLatest( throwLatestFailure(latest, "contains transport version name [" + latest.name() + "] which is not defined"); } if (latestDefinition.ids().contains(latest.id()) == false) { - Path relativePath = getResources().get().getRepositoryPath(latestDefinition); + Path relativePath = getResources().get().getNamedDefinitionRepositoryPath(latestDefinition); throwLatestFailure(latest, "has id " + latest.id() + " which is not in definition [" + relativePath + "]"); } @@ -196,7 +231,7 @@ private void validateBase(int base, List ids) { IdAndDefinition current = ids.get(ndx); if (previous.id().equals(current.id())) { - Path existingDefinitionPath = getResources().get().getRepositoryPath(previous.definition); + Path existingDefinitionPath = getResources().get().getNamedDefinitionRepositoryPath(previous.definition); throwDefinitionFailure( current.definition(), "contains id " + current.id + " already defined in [" + existingDefinitionPath + "]" @@ -213,12 +248,12 @@ private void validateBase(int base, List ids) { } private void throwDefinitionFailure(TransportVersionDefinition definition, String message) { - Path relativePath = getResources().get().getRepositoryPath(definition); + Path relativePath = getResources().get().getNamedDefinitionRepositoryPath(definition); throw new IllegalStateException("Transport version definition file [" + relativePath + "] " + message); } private void throwLatestFailure(TransportVersionLatest latest, String message) { - Path relativePath = getResources().get().getRepositoryPath(latest); + Path relativePath = getResources().get().getLatestRepositoryPath(latest); throw new IllegalStateException("Latest transport version file [" + relativePath + "] " + message); } } From 30c32925a5edf7c8a05ba49d6a47e6508b266e79 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Tue, 19 Aug 2025 23:41:50 +0000 Subject: [PATCH 2/2] [CI] Auto commit changes from spotless --- .../transport/ValidateTransportVersionResourcesTask.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 b808cb4611c51..a51c8842c6be6 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 @@ -85,7 +85,10 @@ public void validateTransportVersions() throws IOException { } } - private Map collectAllDefinitions(Map namedDefinitions, Map unreferencedDefinitions) { + private Map collectAllDefinitions( + Map namedDefinitions, + Map unreferencedDefinitions + ) { Map allDefinitions = new HashMap<>(namedDefinitions); for (var entry : unreferencedDefinitions.entrySet()) { TransportVersionDefinition existing = allDefinitions.put(entry.getKey(), entry.getValue());