Skip to content

Add separate validation for unreferenced transport version definitions #133189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rjernst merged 2 commits into from
Aug 20, 2025
Merged

Add separate validation for unreferenced transport version definitions #133189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ed2381
Add separate validation for unreferenced transport version definitions
rjernst Aug 19, 2025
30c3292
[CI] Auto commit changes from spotless
Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions ...ovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"() {
Copy link
Contributor

@JVerwolf JVerwolf Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, why? These are created when a new branch is cut, which will result in a "base id" without a patch section, right? Is there another scenario where a patch is needed?

Copy link
Contributor

@mark-vieira mark-vieira Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We bump the transport version on every release. So when we release 9.2.0 and bump the version to 9.2.1, we also bump the transport version, which in that case would be a patch version. This is to ensure that every stack version has a unique transport version.

given:
unreferencedTransportVersion("initial_10.0.1", "10000001")
when:
def result = gradleRunner(":myserver:validateTransportVersionDefinitions").build()
then:
result.task(":myserver:validateTransportVersionDefinitions").outcome == TaskOutcome.SUCCESS
}
}
59 changes: 40 additions & 19 deletions ...in/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,10 @@ private Path getNamedDefinitionRelativePath(String name) {

/** Return all named definitions, mapped by their name. */
Map<String, TransportVersionDefinition> getNamedDefinitions() throws IOException {
Map<String, TransportVersionDefinition> 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);
Expand All @@ -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<String, TransportVersionDefinition> 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<String, TransportVersionLatest> getLatestByReleaseBranch() throws IOException {
Map<String, TransportVersionLatest> latests = new HashMap<>();
Expand All @@ -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())));
}

Expand Down Expand Up @@ -197,6 +203,21 @@ private <T> T getMainFile(String resourcePath, BiFunction<String, String, T> par
return parser.apply(resourcePath, content);
}

private static Map<String, TransportVersionDefinition> readDefinitions(Path dir) throws IOException {
if (Files.isDirectory(dir) == false) {
return Map.of();
}
Map<String, TransportVersionDefinition> 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();
Expand Down
82 changes: 60 additions & 22 deletions ...va/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,44 @@ private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition

@TaskAction
public void validateTransportVersions() throws IOException {
TransportVersionResourcesService resources = getResources().get();
Set<String> referencedNames = TransportVersionReference.collectNames(getReferencesFiles());
Map<String, TransportVersionDefinition> definitions = getResources().get().getNamedDefinitions();
Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(definitions.values());
Map<String, TransportVersionLatest> latestByReleaseBranch = getResources().get().getLatestByReleaseBranch();
Map<String, TransportVersionDefinition> namedDefinitions = resources.getNamedDefinitions();
Map<String, TransportVersionDefinition> unreferencedDefinitions = resources.getUnreferencedDefinitions();
Map<String, TransportVersionDefinition> allDefinitions = collectAllDefinitions(namedDefinitions, unreferencedDefinitions);
Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(allDefinitions.values());
Map<String, TransportVersionLatest> 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()) {
validateBase(entry.getKey(), entry.getValue());
}

for (var latest : latestByReleaseBranch.values()) {
validateLatest(latest, definitions, idsByBase);
validateLatest(latest, allDefinitions, idsByBase);
}
}

private Map<String, TransportVersionDefinition> collectAllDefinitions(
Map<String, TransportVersionDefinition> namedDefinitions,
Map<String, TransportVersionDefinition> unreferencedDefinitions
) {
Map<String, TransportVersionDefinition> 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<Integer, List<IdAndDefinition>> collectIdsByBase(Collection<TransportVersionDefinition> definitions) {
Expand All @@ -97,23 +119,17 @@ private Map<Integer, List<IdAndDefinition>> collectIdsByBase(Collection<Transpor
return idsByBase;
}

private void validateDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
private void validateNamedDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {

// validate any modifications
Map<Integer, TransportVersionId> 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) {
Expand All @@ -129,9 +145,7 @@ private void validateDefinition(TransportVersionDefinition definition, Set<Strin
TransportVersionId id = definition.ids().get(ndx);

if (ndx == 0) {
// TODO: initial versions will only be applicable to a release branch, so they won't have an associated
// main version. They will also be loaded differently in the future, but until they are separate, we ignore them here.
if (id.patch() != 0 && definition.name().startsWith("initial_") == false) {
if (id.patch() != 0) {
throwDefinitionFailure(definition, "has patch version " + id.complete() + " as primary id");
}
} else {
Expand All @@ -148,6 +162,30 @@ private void validateDefinition(TransportVersionDefinition definition, Set<Strin
}
}

private void validateUnreferencedDefinition(TransportVersionDefinition definition) {
TransportVersionDefinition originalDefinition = getResources().get().getUnreferencedDefinitionFromMain(definition.name());
if (originalDefinition != null) {
validateIdenticalPrimaryId(definition, originalDefinition);
}
if (definition.ids().isEmpty()) {
throwDefinitionFailure(definition, "does not contain any ids");
}
if (definition.ids().size() > 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<String, TransportVersionDefinition> definitions,
Expand All @@ -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 + "]");
}

Expand Down Expand Up @@ -196,7 +234,7 @@ private void validateBase(int base, List<IdAndDefinition> 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 + "]"
Expand All @@ -213,12 +251,12 @@ private void validateBase(int base, List<IdAndDefinition> 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);
}
}