Skip to content

Commit a8d6582

Browse files
rjernstrjernst
authored
Add separate validation for unreferenced transport version definitions (#133189)
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.
1 parent c591e99 commit a8d6582

File tree

3 files changed

+131
-41
lines changed

3 files changed

+131
-41
lines changed

‎build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,4 +198,35 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes
198198
then:
199199
result.task(":myserver:validateTransportVersionResources").outcome == TaskOutcome.SUCCESS
200200
}
201+
202+
def "latest can refer to an unreferenced definition"() {
203+
given:
204+
unreferencedTransportVersion("initial_10.0.0", "10000000")
205+
latestTransportVersion("10.0", "initial_10.0.0", "10000000")
206+
when:
207+
def result = gradleRunner(":myserver:validateTransportVersionDefinitions").build()
208+
then:
209+
result.task(":myserver:validateTransportVersionDefinitions").outcome == TaskOutcome.SUCCESS
210+
}
211+
212+
def "named and unreferenced definitions cannot have the same name"() {
213+
given:
214+
unreferencedTransportVersion("existing_92", "10000000")
215+
when:
216+
def result = validateDefinitionsFails()
217+
then:
218+
assertDefinitionsFailure(result, "Transport version definition file " +
219+
"[myserver/src/main/resources/transport/definitions/named/existing_92.csv] " +
220+
"has same name as unreferenced definition " +
221+
"[myserver/src/main/resources/transport/definitions/unreferenced/existing_92.csv]")
222+
}
223+
224+
def "unreferenced definitions can have primary ids that are patches"() {
225+
given:
226+
unreferencedTransportVersion("initial_10.0.1", "10000001")
227+
when:
228+
def result = gradleRunner(":myserver:validateTransportVersionDefinitions").build()
229+
then:
230+
result.task(":myserver:validateTransportVersionDefinitions").outcome == TaskOutcome.SUCCESS
231+
}
201232
}

‎build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java‎

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -98,25 +98,10 @@ private Path getNamedDefinitionRelativePath(String name) {
9898

9999
/** Return all named definitions, mapped by their name. */
100100
Map<String, TransportVersionDefinition> getNamedDefinitions() throws IOException {
101-
Map<String, TransportVersionDefinition> definitions = new HashMap<>();
102-
// temporarily include unreferenced in named until validation understands the distinction
103-
for (var dir : List.of(NAMED_DIR, UNREFERENCED_DIR)) {
104-
Path path = transportResourcesDir.resolve(dir);
105-
if (Files.isDirectory(path) == false) {
106-
continue;
107-
}
108-
try (var definitionsStream = Files.list(path)) {
109-
for (var definitionFile : definitionsStream.toList()) {
110-
String contents = Files.readString(definitionFile, StandardCharsets.UTF_8).strip();
111-
var definition = TransportVersionDefinition.fromString(definitionFile.getFileName().toString(), contents);
112-
definitions.put(definition.name(), definition);
113-
}
114-
}
115-
}
116-
return definitions;
101+
return readDefinitions(transportResourcesDir.resolve(NAMED_DIR));
117102
}
118103

119-
/** Test whether the given named definition exists */
104+
/** Get a named definition from main if it exists there, or null otherwise */
120105
TransportVersionDefinition getNamedDefinitionFromMain(String name) {
121106
String resourcePath = getNamedDefinitionRelativePath(name).toString();
122107
return getMainFile(resourcePath, TransportVersionDefinition::fromString);
@@ -128,10 +113,31 @@ boolean namedDefinitionExists(String name) {
128113
}
129114

130115
/** Return the path within the repository of the given named definition */
131-
Path getRepositoryPath(TransportVersionDefinition definition) {
116+
Path getNamedDefinitionRepositoryPath(TransportVersionDefinition definition) {
132117
return rootDir.relativize(transportResourcesDir.resolve(getNamedDefinitionRelativePath(definition.name())));
133118
}
134119

120+
// return the path, relative to the resources dir, of an unreferenced definition
121+
private Path getUnreferencedDefinitionRelativePath(String name) {
122+
return UNREFERENCED_DIR.resolve(name + ".csv");
123+
}
124+
125+
/** Return all unreferenced definitions, mapped by their name. */
126+
Map<String, TransportVersionDefinition> getUnreferencedDefinitions() throws IOException {
127+
return readDefinitions(transportResourcesDir.resolve(UNREFERENCED_DIR));
128+
}
129+
130+
/** Get a named definition from main if it exists there, or null otherwise */
131+
TransportVersionDefinition getUnreferencedDefinitionFromMain(String name) {
132+
String resourcePath = getUnreferencedDefinitionRelativePath(name).toString();
133+
return getMainFile(resourcePath, TransportVersionDefinition::fromString);
134+
}
135+
136+
/** Return the path within the repository of the given named definition */
137+
Path getUnreferencedDefinitionRepositoryPath(TransportVersionDefinition definition) {
138+
return rootDir.relativize(transportResourcesDir.resolve(getUnreferencedDefinitionRelativePath(definition.name())));
139+
}
140+
135141
/** Read all latest files and return them mapped by their release branch */
136142
Map<String, TransportVersionLatest> getLatestByReleaseBranch() throws IOException {
137143
Map<String, TransportVersionLatest> latests = new HashMap<>();
@@ -152,7 +158,7 @@ TransportVersionLatest getLatestFromMain(String releaseBranch) {
152158
}
153159

154160
/** Return the path within the repository of the given latest */
155-
Path getRepositoryPath(TransportVersionLatest latest) {
161+
Path getLatestRepositoryPath(TransportVersionLatest latest) {
156162
return rootDir.relativize(transportResourcesDir.resolve(getLatestRelativePath(latest.branch())));
157163
}
158164

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

206+
private static Map<String, TransportVersionDefinition> readDefinitions(Path dir) throws IOException {
207+
if (Files.isDirectory(dir) == false) {
208+
return Map.of();
209+
}
210+
Map<String, TransportVersionDefinition> definitions = new HashMap<>();
211+
try (var definitionsStream = Files.list(dir)) {
212+
for (var definitionFile : definitionsStream.toList()) {
213+
String contents = Files.readString(definitionFile, StandardCharsets.UTF_8).strip();
214+
var definition = TransportVersionDefinition.fromString(definitionFile.getFileName().toString(), contents);
215+
definitions.put(definition.name(), definition);
216+
}
217+
}
218+
return definitions;
219+
}
220+
200221
// run a git command, relative to the transport version resources directory
201222
private String gitCommand(String... args) {
202223
ByteArrayOutputStream stdout = new ByteArrayOutputStream();

‎build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java‎

Lines changed: 60 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -60,22 +60,44 @@ private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition
6060

6161
@TaskAction
6262
public void validateTransportVersions() throws IOException {
63+
TransportVersionResourcesService resources = getResources().get();
6364
Set<String> referencedNames = TransportVersionReference.collectNames(getReferencesFiles());
64-
Map<String, TransportVersionDefinition> definitions = getResources().get().getNamedDefinitions();
65-
Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(definitions.values());
66-
Map<String, TransportVersionLatest> latestByReleaseBranch = getResources().get().getLatestByReleaseBranch();
65+
Map<String, TransportVersionDefinition> namedDefinitions = resources.getNamedDefinitions();
66+
Map<String, TransportVersionDefinition> unreferencedDefinitions = resources.getUnreferencedDefinitions();
67+
Map<String, TransportVersionDefinition> allDefinitions = collectAllDefinitions(namedDefinitions, unreferencedDefinitions);
68+
Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(allDefinitions.values());
69+
Map<String, TransportVersionLatest> latestByReleaseBranch = resources.getLatestByReleaseBranch();
70+
71+
for (var definition : namedDefinitions.values()) {
72+
validateNamedDefinition(definition, referencedNames);
73+
}
6774

68-
for (var definition : definitions.values()) {
69-
validateDefinition(definition, referencedNames);
75+
for (var definition : unreferencedDefinitions.values()) {
76+
validateUnreferencedDefinition(definition);
7077
}
7178

7279
for (var entry : idsByBase.entrySet()) {
7380
validateBase(entry.getKey(), entry.getValue());
7481
}
7582

7683
for (var latest : latestByReleaseBranch.values()) {
77-
validateLatest(latest, definitions, idsByBase);
84+
validateLatest(latest, allDefinitions, idsByBase);
85+
}
86+
}
87+
88+
private Map<String, TransportVersionDefinition> collectAllDefinitions(
89+
Map<String, TransportVersionDefinition> namedDefinitions,
90+
Map<String, TransportVersionDefinition> unreferencedDefinitions
91+
) {
92+
Map<String, TransportVersionDefinition> allDefinitions = new HashMap<>(namedDefinitions);
93+
for (var entry : unreferencedDefinitions.entrySet()) {
94+
TransportVersionDefinition existing = allDefinitions.put(entry.getKey(), entry.getValue());
95+
if (existing != null) {
96+
Path unreferencedPath = getResources().get().getUnreferencedDefinitionRepositoryPath(entry.getValue());
97+
throwDefinitionFailure(existing, "has same name as unreferenced definition [" + unreferencedPath + "]");
98+
}
7899
}
100+
return allDefinitions;
79101
}
80102

81103
private Map<Integer, List<IdAndDefinition>> collectIdsByBase(Collection<TransportVersionDefinition> definitions) {
@@ -97,23 +119,17 @@ private Map<Integer, List<IdAndDefinition>> collectIdsByBase(Collection<Transpor
97119
return idsByBase;
98120
}
99121

100-
private void validateDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
122+
private void validateNamedDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
101123

102124
// validate any modifications
103125
Map<Integer, TransportVersionId> existingIdsByBase = new HashMap<>();
104126
TransportVersionDefinition originalDefinition = getResources().get().getNamedDefinitionFromMain(definition.name());
105127
if (originalDefinition != null) {
106-
107-
int primaryId = definition.ids().get(0).complete();
108-
int originalPrimaryId = originalDefinition.ids().get(0).complete();
109-
if (primaryId != originalPrimaryId) {
110-
throwDefinitionFailure(definition, "has modified primary id from " + originalPrimaryId + " to " + primaryId);
111-
}
112-
128+
validateIdenticalPrimaryId(definition, originalDefinition);
113129
originalDefinition.ids().forEach(id -> existingIdsByBase.put(id.base(), id));
114130
}
115131

116-
if (referencedNames.contains(definition.name()) == false && definition.name().startsWith("initial_") == false) {
132+
if (referencedNames.contains(definition.name()) == false) {
117133
throwDefinitionFailure(definition, "is not referenced");
118134
}
119135
if (NAME_FORMAT.matcher(definition.name()).matches() == false) {
@@ -129,9 +145,7 @@ private void validateDefinition(TransportVersionDefinition definition, Set<Strin
129145
TransportVersionId id = definition.ids().get(ndx);
130146

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

165+
private void validateUnreferencedDefinition(TransportVersionDefinition definition) {
166+
TransportVersionDefinition originalDefinition = getResources().get().getUnreferencedDefinitionFromMain(definition.name());
167+
if (originalDefinition != null) {
168+
validateIdenticalPrimaryId(definition, originalDefinition);
169+
}
170+
if (definition.ids().isEmpty()) {
171+
throwDefinitionFailure(definition, "does not contain any ids");
172+
}
173+
if (definition.ids().size() > 1) {
174+
throwDefinitionFailure(definition, " contains more than one id");
175+
}
176+
// note: no name validation, anything that is a valid filename is ok, this allows eg initial_8.9.1
177+
}
178+
179+
private void validateIdenticalPrimaryId(TransportVersionDefinition definition, TransportVersionDefinition originalDefinition) {
180+
assert definition.name().equals(originalDefinition.name());
181+
182+
int primaryId = definition.ids().get(0).complete();
183+
int originalPrimaryId = originalDefinition.ids().get(0).complete();
184+
if (primaryId != originalPrimaryId) {
185+
throwDefinitionFailure(definition, "has modified primary id from " + originalPrimaryId + " to " + primaryId);
186+
}
187+
}
188+
151189
private void validateLatest(
152190
TransportVersionLatest latest,
153191
Map<String, TransportVersionDefinition> definitions,
@@ -158,7 +196,7 @@ private void validateLatest(
158196
throwLatestFailure(latest, "contains transport version name [" + latest.name() + "] which is not defined");
159197
}
160198
if (latestDefinition.ids().contains(latest.id()) == false) {
161-
Path relativePath = getResources().get().getRepositoryPath(latestDefinition);
199+
Path relativePath = getResources().get().getNamedDefinitionRepositoryPath(latestDefinition);
162200
throwLatestFailure(latest, "has id " + latest.id() + " which is not in definition [" + relativePath + "]");
163201
}
164202

@@ -196,7 +234,7 @@ private void validateBase(int base, List<IdAndDefinition> ids) {
196234
IdAndDefinition current = ids.get(ndx);
197235

198236
if (previous.id().equals(current.id())) {
199-
Path existingDefinitionPath = getResources().get().getRepositoryPath(previous.definition);
237+
Path existingDefinitionPath = getResources().get().getNamedDefinitionRepositoryPath(previous.definition);
200238
throwDefinitionFailure(
201239
current.definition(),
202240
"contains id " + current.id + " already defined in [" + existingDefinitionPath + "]"
@@ -213,12 +251,12 @@ private void validateBase(int base, List<IdAndDefinition> ids) {
213251
}
214252

215253
private void throwDefinitionFailure(TransportVersionDefinition definition, String message) {
216-
Path relativePath = getResources().get().getRepositoryPath(definition);
254+
Path relativePath = getResources().get().getNamedDefinitionRepositoryPath(definition);
217255
throw new IllegalStateException("Transport version definition file [" + relativePath + "] " + message);
218256
}
219257

220258
private void throwLatestFailure(TransportVersionLatest latest, String message) {
221-
Path relativePath = getResources().get().getRepositoryPath(latest);
259+
Path relativePath = getResources().get().getLatestRepositoryPath(latest);
222260
throw new IllegalStateException("Latest transport version file [" + relativePath + "] " + message);
223261
}
224262
}

0 commit comments

Comments
 (0)