Skip to content

Commit 9c69413

Browse files
dahyvuunSamBarker
dahyvuun
and
SamBarker
authored
refactor: Replace generic RuntimeException with specific exceptions in KRPC code geneation (kroxylicious#3661)
* Replace generic RuntimeException with specific exceptions in kroxylicious-krpc-plugin Signed-off-by: dahyvuun <dahyvuun@gmail.com> * Fix formatting in KrpcCodeGenerationException.java Signed-off-by: dahyvuun <dahyvuun@gmail.com> * Restore comments in FieldSpec.java Signed-off-by: dahyvuun <dahyvuun@gmail.com> * Apply formatter to KrpcGenerator.java Signed-off-by: dahyvuun <dahyvuun@gmail.com> * Update kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/MessageSpec.java Co-authored-by: Sam Barker <sam@quadrocket.co.uk> Signed-off-by: Dahyun Woo <164325611+dahyvuun@users.noreply.github.com> # Conflicts: # kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/MessageSpec.java * Add unit tests for KrpcCodeGenerationException Signed-off-by: dahyvuun <dahyvuun@gmail.com> * restore comment Signed-off-by: Sam Barker <sam@quadrocket.co.uk> * Cleanup remaining Idea warnings in FieldSpec. Signed-off-by: Sam Barker <sam@quadrocket.co.uk> * Add tests for StructSpec validation exceptions Tests cover missing versions, duplicate tag IDs, duplicate field names, non-contiguous tag IDs, and a happy-path parse. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk> * fix formmating Signed-off-by: Sam Barker <sam@quadrocket.co.uk> * Add validation tests for FieldSpec and MessageSpec Cover the IllegalArgumentException/IllegalStateException paths thrown by the constructor validations in FieldSpec (invalid name, missing versions, nullable/tagged invariants, zeroCopy, flexibleVersions, etc.) and the three validation paths in MessageSpec (non-open-ended flexibleVersions, listeners on non-request, latestVersionUnstable on non-request). Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk> --------- Signed-off-by: dahyvuun <dahyvuun@gmail.com> Signed-off-by: Dahyun Woo <164325611+dahyvuun@users.noreply.github.com> Signed-off-by: Sam Barker <sam@quadrocket.co.uk> Co-authored-by: Sam Barker <sam@quadrocket.co.uk>
1 parent ed73dc6 commit 9c69413

12 files changed

Lines changed: 375 additions & 42 deletions

File tree

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/KrpcCodeGenerationException.java‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,4 +17,21 @@ public class KrpcCodeGenerationException extends RuntimeException {
1717
public KrpcCodeGenerationException(String message) {
1818
super(message);
1919
}
20-
}
20+
21+
/**
22+
* Create KrpcCodeGenerationException
23+
* @param cause cause
24+
*/
25+
public KrpcCodeGenerationException(Throwable cause) {
26+
super(cause);
27+
}
28+
29+
/**
30+
* Create KrpcCodeGenerationException
31+
* @param message message
32+
* @param cause cause
33+
*/
34+
public KrpcCodeGenerationException(String message, Throwable cause) {
35+
super(message, cause);
36+
}
37+
}

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/main/KrpcGenerator.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.graalvm.polyglot.Context;
4141
import org.graalvm.polyglot.TypeLiteral;
4242

43+
import io.kroxylicious.krpccodegen.KrpcCodeGenerationException;
4344
import io.kroxylicious.krpccodegen.model.EntityTypeSetFactory;
4445
import io.kroxylicious.krpccodegen.model.KrpcSchemaObjectWrapper;
4546
import io.kroxylicious.krpccodegen.model.RetrieveApiKey;
@@ -388,7 +389,7 @@ public void accept(Writer writer, File finalFile) throws TemplateException, IOEx
388389
throw new UncheckedIOException(e);
389390
}
390391
catch (TemplateException e) {
391-
throw new RuntimeException(e);
392+
throw new KrpcCodeGenerationException(e);
392393
}
393394
}).sum();
394395
}
@@ -477,7 +478,7 @@ public void accept(Writer writer, File finalFile) throws TemplateException, IOEx
477478
throw new UncheckedIOException(e);
478479
}
479480
catch (TemplateException e) {
480-
throw new RuntimeException(e);
481+
throw new KrpcCodeGenerationException(e);
481482
}
482483
}).sum();
483484
}
@@ -544,7 +545,7 @@ private Set<MessageSpec> messageSpecs() {
544545
return messageSpec;
545546
}
546547
catch (Exception e) {
547-
throw new RuntimeException("Exception while processing " + inputPath.toString(), e);
548+
throw new KrpcCodeGenerationException("Exception while processing " + inputPath.toString(), e);
548549
}
549550
})
550551
.filter(Predicate.not(v -> v.validVersions().equals(Versions.NONE)))
@@ -613,7 +614,7 @@ private Set<ApiSpec> toApiSpecs(Set<MessageSpec> messageSpecs) {
613614
Function.identity()));
614615

615616
if (allRequests.size() != allResponses.size()) {
616-
throw new RuntimeException("Can't pair up requests to responses");
617+
throw new IllegalStateException("Can't pair up requests to responses");
617618
}
618619

619620
return allRequests.keySet().stream()

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/EntityType.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public void verifyTypeMatches(String fieldName, FieldType type) {
4141
}
4242
else {
4343
if (!type.toString().equals(baseType.toString())) {
44-
throw new RuntimeException("Field " + fieldName + " has field type " +
44+
throw new IllegalArgumentException("Field " + fieldName + " has field type " +
4545
name() + ", but field type " + type.toString() + ", which does " +
4646
"not match.");
4747
}

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/FieldSpec.java‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,10 @@ public final class FieldSpec {
4242

4343
private final Versions taggedVersions;
4444

45+
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
4546
private final Optional<Versions> flexibleVersions;
4647

48+
@SuppressWarnings("OptionalUsedAsFieldOrParameterType")
4749
private final Optional<Integer> tag;
4850

4951
private final boolean zeroCopy;
@@ -89,7 +91,7 @@ public FieldSpec(@JsonProperty("name") String name,
8991
private static String requireValidName(String name) {
9092
Objects.requireNonNull(name);
9193
if (!VALID_FIELD_NAMES.matcher(name).matches()) {
92-
throw new RuntimeException("Invalid field name " + name);
94+
throw new IllegalArgumentException("Invalid field name " + name);
9395
}
9496
return name;
9597
}
@@ -98,7 +100,7 @@ private static String requireValidName(String name) {
98100
private Versions requireVersions(String versions) {
99101
Versions v = Versions.parse(versions, this.taggedVersions.empty() ? null : this.taggedVersions);
100102
if (v == null) {
101-
throw new RuntimeException("You must specify the version of the " + name + " structure.");
103+
throw new IllegalArgumentException("You must specify the version of the " + name + " structure.");
102104
}
103105
return v;
104106
}
@@ -115,7 +117,7 @@ private Optional<Versions> parseFlexibleVersions(String flexibleVersions) {
115117
// For now, only allow flexibleVersions overrides for the string and bytes
116118
// types. Overrides are only needed to keep compatibility with some old formats,
117119
// so there isn't any need to support them for all types.
118-
throw new RuntimeException("Invalid flexibleVersions override for " + name +
120+
throw new IllegalArgumentException("Invalid flexibleVersions override for " + name +
119121
". Only fields of type string or bytes can specify a flexibleVersions " +
120122
"override.");
121123
}
@@ -124,59 +126,59 @@ private Optional<Versions> parseFlexibleVersions(String flexibleVersions) {
124126

125127
private void validateNullableVersions() {
126128
if (!this.nullableVersions.empty() && !this.type.canBeNullable()) {
127-
throw new RuntimeException("Type " + this.type + " cannot be nullable.");
129+
throw new IllegalStateException("Type " + this.type + " cannot be nullable.");
128130
}
129131
}
130132

131133
private void validateSubFields() {
132134
if (!this.fields().isEmpty() && !this.type.isArray() && !this.type.isStruct()) {
133-
throw new RuntimeException("Non-array or Struct field " + name + " cannot have fields");
135+
throw new IllegalArgumentException("Non-array or Struct field " + name + " cannot have fields");
134136
}
135137
}
136138

137139
private void validateTagNotMapKey() {
138140
if (this.tag.isPresent() && mapKey) {
139-
throw new RuntimeException("Tagged fields cannot be used as keys.");
141+
throw new IllegalArgumentException("Tagged fields cannot be used as keys.");
140142
}
141143
}
142144

143145
private void validateZeroCopy() {
144146
if (this.zeroCopy && !this.type.isBytes()) {
145-
throw new RuntimeException("Invalid zeroCopy value for " + name +
147+
throw new IllegalArgumentException("Invalid zeroCopy value for " + name +
146148
". Only fields of type bytes can use zeroCopy flag.");
147149
}
148150
}
149151

150152
private void checkTagInvariants() {
151153
if (this.tag.isPresent()) {
152154
if (this.tag.get() < 0) {
153-
throw new RuntimeException("Field " + name + " specifies a tag of " + this.tag.get() +
155+
throw new IllegalArgumentException("Field " + name + " specifies a tag of " + this.tag.get() +
154156
". Tags cannot be negative.");
155157
}
156158
if (this.taggedVersions.empty()) {
157-
throw new RuntimeException("Field " + name + " specifies a tag of " + this.tag.get() +
159+
throw new IllegalArgumentException("Field " + name + " specifies a tag of " + this.tag.get() +
158160
", but has no tagged versions. If a tag is specified, taggedVersions must " +
159161
"be specified as well.");
160162
}
161163
Versions nullableTaggedVersions = this.nullableVersions.intersect(this.taggedVersions);
162164
if (!(nullableTaggedVersions.empty() || nullableTaggedVersions.equals(this.taggedVersions))) {
163-
throw new RuntimeException("Field " + name + " specifies nullableVersions " +
165+
throw new IllegalArgumentException("Field " + name + " specifies nullableVersions " +
164166
this.nullableVersions + " and taggedVersions " + this.taggedVersions + ". " +
165167
"Either all tagged versions must be nullable, or none must be.");
166168
}
167169
if (this.taggedVersions.highest() < Short.MAX_VALUE) {
168-
throw new RuntimeException("Field " + name + " specifies taggedVersions " +
170+
throw new IllegalArgumentException("Field " + name + " specifies taggedVersions " +
169171
this.taggedVersions + ", which is not open-ended. taggedVersions must " +
170172
"be either none, or an open-ended range (that ends with a plus sign).");
171173
}
172174
if (!this.taggedVersions.intersect(this.versions).equals(this.taggedVersions)) {
173-
throw new RuntimeException("Field " + name + " specifies taggedVersions " +
175+
throw new IllegalArgumentException("Field " + name + " specifies taggedVersions " +
174176
this.taggedVersions + ", and versions " + this.versions + ". " +
175177
"taggedVersions must be a subset of versions.");
176178
}
177179
}
178180
else if (!this.taggedVersions.empty()) {
179-
throw new RuntimeException("Field " + name + " does not specify a tag, " +
181+
throw new IllegalArgumentException("Field " + name + " does not specify a tag, " +
180182
"but specifies tagged versions of " + this.taggedVersions + ". " +
181183
"Please specify a tag, or remove the taggedVersions.");
182184
}
@@ -255,7 +257,7 @@ public Versions taggedVersions() {
255257

256258
@JsonProperty("flexibleVersions")
257259
public String flexibleVersionsString() {
258-
return flexibleVersions.isPresent() ? flexibleVersions.get().toString() : null;
260+
return flexibleVersions.map(Versions::toString).orElse(null);
259261
}
260262

261263
public Optional<Versions> flexibleVersions() {

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/FieldType.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -312,12 +312,12 @@ static FieldType parse(String string) {
312312
if (string.startsWith(ARRAY_PREFIX)) {
313313
String elementTypeString = string.substring(ARRAY_PREFIX.length());
314314
if (elementTypeString.isEmpty()) {
315-
throw new RuntimeException("Can't parse array type " + string +
315+
throw new IllegalArgumentException("Can't parse array type " + string +
316316
". No element type found.");
317317
}
318318
FieldType elementType = parse(elementTypeString);
319319
if (elementType.isArray()) {
320-
throw new RuntimeException("Can't have an array of arrays. " +
320+
throw new IllegalArgumentException("Can't have an array of arrays. " +
321321
"Use an array of structs containing an array instead.");
322322
}
323323
return new ArrayType(elementType);
@@ -326,7 +326,7 @@ else if (StructRegistry.firstIsCapitalized(string)) {
326326
return new StructType(string);
327327
}
328328
else {
329-
throw new RuntimeException("Can't parse type " + string);
329+
throw new IllegalArgumentException("Can't parse type " + string);
330330
}
331331
}
332332
}

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/MessageSpec.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ public MessageSpec(@JsonProperty("name") String name,
5151
this.latestVersionUnstable = Optional.ofNullable(latestVersionUnstable);
5252
this.type = Objects.requireNonNull(type);
5353
this.commonStructs = commonStructs == null ? Collections.emptyList() : List.copyOf(commonStructs);
54-
5554
// If the struct has no valid versions (the typical use case is to completely remove support for
5655
// an existing protocol api while ensuring the api key id is not reused), we configure the spec
5756
// to effectively be empty
@@ -60,26 +59,24 @@ public MessageSpec(@JsonProperty("name") String name,
6059
this.listeners = Collections.emptyList();
6160
}
6261
else {
63-
if (flexibleVersions == null) {
64-
throw new RuntimeException("You must specify a value for flexibleVersions. " +
65-
"Please use 0+ for all new messages.");
66-
}
62+
Objects.requireNonNull(flexibleVersions, "You must specify a value for flexibleVersions. " +
63+
"Please use 0+ for all new messages.");
6764
this.flexibleVersions = Versions.parse(flexibleVersions, Versions.NONE);
6865
if ((!this.flexibleVersions().empty()) &&
6966
(this.flexibleVersions.highest() < Short.MAX_VALUE)) {
70-
throw new RuntimeException("Field " + name + " specifies flexibleVersions " +
67+
throw new IllegalArgumentException("Field " + name + " specifies flexibleVersions " +
7168
this.flexibleVersions + ", which is not open-ended. flexibleVersions must " +
7269
"be either none, or an open-ended range (that ends with a plus sign).");
7370
}
7471

7572
if (listeners != null && !listeners.isEmpty() && type != MessageSpecType.REQUEST) {
76-
throw new RuntimeException("The `requestScope` property is only valid for " +
73+
throw new IllegalArgumentException("The `requestScope` property is only valid for " +
7774
"messages with type `request`");
7875
}
7976
this.listeners = listeners;
8077

8178
if (Boolean.TRUE.equals(latestVersionUnstable) && type != MessageSpecType.REQUEST) {
82-
throw new RuntimeException("The `latestVersionUnstable` property is only valid for " +
79+
throw new IllegalArgumentException("The `latestVersionUnstable` property is only valid for " +
8380
"messages with type `request`");
8481
}
8582
}

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/StructSpec.java‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,8 @@ public StructSpec(@JsonProperty("name") String name,
3939
}
4040

4141
private static Versions parseVersions(String versions, String name) {
42-
Versions parsed = Versions.parse(versions, null);
43-
if (parsed == null) {
44-
throw new RuntimeException("You must specify the version of the " +
45-
name + " structure.");
46-
}
47-
return parsed;
42+
return Objects.requireNonNull(Versions.parse(versions, null),
43+
"You must specify the version of the " + name + " structure.");
4844
}
4945

5046
private static List<FieldSpec> validateFields(List<FieldSpec> fields, String name) {
@@ -70,7 +66,7 @@ private static List<FieldSpec> validateFields(List<FieldSpec> fields, String nam
7066
private static void validateTagUniqueness(FieldSpec field, Set<Integer> tags, String name) {
7167
field.tag().ifPresent(tag -> {
7268
if (!tags.add(tag)) {
73-
throw new RuntimeException("In " + name + ", field " + field.name() +
69+
throw new IllegalArgumentException("In " + name + ", field " + field.name() +
7470
" has a duplicate tag ID " + tag + ". All tags IDs " +
7571
"must be unique.");
7672
}
@@ -79,7 +75,7 @@ private static void validateTagUniqueness(FieldSpec field, Set<Integer> tags, St
7975

8076
private static void validateNameUniqueness(FieldSpec field, Set<String> names, String name) {
8177
if (!names.add(field.name())) {
82-
throw new RuntimeException("In " + name + ", field " + field.name() +
78+
throw new IllegalStateException("In " + name + ", field " + field.name() +
8379
" has a duplicate name " + field.name() + ". All field names " +
8480
"must be unique.");
8581
}
@@ -88,7 +84,7 @@ private static void validateNameUniqueness(FieldSpec field, Set<String> names, S
8884
private static void validateTagContiguity(Set<Integer> tags, String name) {
8985
for (int i = 0; i < tags.size(); i++) {
9086
if (!tags.contains(i)) {
91-
throw new RuntimeException("In " + name + ", the tag IDs are not " +
87+
throw new IllegalStateException("In " + name + ", the tag IDs are not " +
9288
"contiguous. Make use of tag " + i + " before using any " +
9389
"higher tag IDs.");
9490
}
@@ -132,4 +128,4 @@ public String toString() {
132128
", hasKeys=" + hasKeys +
133129
'}';
134130
}
135-
}
131+
}

‎kroxylicious-krpc-plugin/src/main/java/io/kroxylicious/krpccodegen/schema/Versions.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ private Versions() {
6666

6767
public Versions(short lowest, short highest) {
6868
if ((lowest < 0) || (highest < 0)) {
69-
throw new RuntimeException("Invalid version range " +
69+
throw new IllegalArgumentException("Invalid version range " +
7070
lowest + " to " + highest);
7171
}
7272
this.lowest = lowest;

‎kroxylicious-krpc-plugin/src/test/java/io/kroxylicious/krpccodegen/KrpcCodeGenerationExceptionTest.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/*
2+
* Copyright Kroxylicious Authors.
3+
*
4+
* Licensed under the Apache Software License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
5+
*/
6+
7+
package io.kroxylicious.krpccodegen;
8+
9+
import org.junit.jupiter.api.Test;
10+
11+
import static org.assertj.core.api.Assertions.assertThat;
12+
13+
class KrpcCodeGenerationExceptionTest {
14+
15+
@Test
16+
void shouldCreateExceptionWithMessage() {
17+
var exception = new KrpcCodeGenerationException("test message");
18+
assertThat(exception.getMessage()).isEqualTo("test message");
19+
}
20+
21+
@Test
22+
void shouldCreateExceptionWithCause() {
23+
var cause = new RuntimeException("cause");
24+
var exception = new KrpcCodeGenerationException(cause);
25+
assertThat(exception.getCause()).isEqualTo(cause);
26+
}
27+
28+
@Test
29+
void shouldCreateExceptionWithMessageAndCause() {
30+
var cause = new RuntimeException("cause");
31+
var exception = new KrpcCodeGenerationException("test message", cause);
32+
assertThat(exception.getMessage()).isEqualTo("test message");
33+
assertThat(exception.getCause()).isEqualTo(cause);
34+
}
35+
}

0 commit comments

Comments
 (0)