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..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 @@ -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,23 @@ 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 +119,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 +145,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 +196,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 +234,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 +251,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); } }