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 527130c7b9a5f..7d5baa1b5fc64 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 9080de1fd79a5..829ce577891df 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); }