Skip to content

Commit ef1de83

Browse files
Copilotedburns
andauthored
Fix review comments: getRpc() IllegalStateException, UnknownSessionEvent wire type, anyOf heuristic, remove unused var
Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/9b8b782c-22ad-450f-885d-2b11d5808a0c Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
1 parent 88f88c9 commit ef1de83

File tree

6 files changed

+66
-11
lines changed

6 files changed

+66
-11
lines changed

scripts/codegen/java.ts

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,12 @@ function schemaTypeToJava(
115115
}
116116
// When exactly two non-null types and one of them is string, prefer String
117117
// over Object to avoid unnecessary type erasure on common wire-level unions
118-
// (e.g., string | null, string | boolean). For wider unions keep Object.
118+
// (e.g., string | null, string | boolean). For string | object keep Object
119+
// so downstream code is not forced to cast. For wider unions keep Object.
119120
if (nonNull.length === 2) {
120121
const hasString = nonNull.some((s) => typeof s === "object" && (s as JSONSchema7).type === "string");
121-
if (hasString) {
122+
const hasObject = nonNull.some((s) => typeof s === "object" && (s as JSONSchema7).type === "object");
123+
if (hasString && !hasObject) {
122124
return { javaType: "String", imports };
123125
}
124126
}
@@ -308,7 +310,7 @@ async function generateSessionEventBaseClass(
308310
lines.push(` */`);
309311
lines.push(`@JsonIgnoreProperties(ignoreUnknown = true)`);
310312
lines.push(`@JsonInclude(JsonInclude.Include.NON_NULL)`);
311-
lines.push(`@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = UnknownSessionEvent.class)`);
313+
lines.push(`@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", visible = true, defaultImpl = UnknownSessionEvent.class)`);
312314
lines.push(`@JsonSubTypes({`);
313315
for (let i = 0; i < variants.length; i++) {
314316
const v = variants[i];
@@ -378,20 +380,46 @@ async function generateUnknownEventClass(packageName: string, packageDir: string
378380
lines.push("");
379381
lines.push(`package ${packageName};`);
380382
lines.push("");
383+
lines.push(`import com.fasterxml.jackson.annotation.JsonIgnore;`);
381384
lines.push(`import com.fasterxml.jackson.annotation.JsonIgnoreProperties;`);
385+
lines.push(`import com.fasterxml.jackson.annotation.JsonProperty;`);
382386
lines.push(`import javax.annotation.processing.Generated;`);
383387
lines.push("");
384388
lines.push(`/**`);
385389
lines.push(` * Fallback for event types not yet known to this SDK version.`);
386390
lines.push(` *`);
391+
lines.push(` * <p>The {@link #getOriginalType()} method returns the raw event-type discriminator`);
392+
lines.push(` * value received on the wire, which can be used for forward-compatibility`);
393+
lines.push(` * telemetry and handling.`);
394+
lines.push(` *`);
387395
lines.push(` * @since 1.0.0`);
388396
lines.push(` */`);
389397
lines.push(`@JsonIgnoreProperties(ignoreUnknown = true)`);
390398
lines.push(GENERATED_ANNOTATION);
391399
lines.push(`public final class UnknownSessionEvent extends SessionEvent {`);
392400
lines.push("");
401+
lines.push(` @JsonProperty("type")`);
402+
lines.push(` private String originalType;`);
403+
lines.push("");
404+
lines.push(` /**`);
405+
lines.push(` * Returns the raw event-type discriminator string received on the wire,`);
406+
lines.push(` * or {@code "unknown"} if the value was not present in the JSON payload.`);
407+
lines.push(` *`);
408+
lines.push(` * @return the original wire type string, or {@code "unknown"}`);
409+
lines.push(` */`);
393410
lines.push(` @Override`);
394-
lines.push(` public String getType() { return "unknown"; }`);
411+
lines.push(` @JsonProperty("type")`);
412+
lines.push(` public String getType() { return originalType != null ? originalType : "unknown"; }`);
413+
lines.push("");
414+
lines.push(` /**`);
415+
lines.push(` * Returns the raw event-type discriminator string received on the wire.`);
416+
lines.push(` *`);
417+
lines.push(` * @return the original wire type string, or {@code null} if not present`);
418+
lines.push(` */`);
419+
lines.push(` @JsonIgnore`);
420+
lines.push(` public String getOriginalType() { return originalType; }`);
421+
lines.push("");
422+
lines.push(` public void setOriginalType(String originalType) { this.originalType = originalType; }`);
395423
lines.push(`}`);
396424
lines.push("");
397425

@@ -430,7 +458,6 @@ function renderNestedType(nested: JavaClassDef, indentLevel: number, nestedTypes
430458
lines.push(`${ind}}`);
431459
} else if (nested.kind === "class" && nested.schema?.properties) {
432460
const localNestedTypes = new Map<string, JavaClassDef>();
433-
const requiredSet = new Set(nested.schema.required || []);
434461
const fields: { jsonName: string; javaName: string; javaType: string; description?: string }[] = [];
435462

436463
for (const [propName, propSchema] of Object.entries(nested.schema.properties)) {

src/generated/java/com/github/copilot/sdk/generated/SessionEvent.java

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/generated/java/com/github/copilot/sdk/generated/UnknownSessionEvent.java

Lines changed: 27 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/main/java/com/github/copilot/sdk/CopilotSession.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,8 @@ public SessionUiApi getUi() {
299299
*/
300300
public SessionRpc getRpc() {
301301
if (rpc == null) {
302-
return null;
302+
throw new IllegalStateException(
303+
"Session is not connected or initialized: getRpc() requires an active session.");
303304
}
304305
SessionRpc current = sessionRpc;
305306
if (current == null) {

src/test/java/com/github/copilot/sdk/ForwardCompatibilityTest.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ void parse_unknownEventType_returnsUnknownSessionEvent() throws Exception {
5353
SessionEvent result = MAPPER.readValue(json, SessionEvent.class);
5454

5555
assertInstanceOf(UnknownSessionEvent.class, result);
56-
assertEquals("unknown", result.getType());
56+
assertEquals("future.feature_from_server", result.getType());
5757
}
5858

5959
@Test
@@ -69,7 +69,8 @@ void parse_unknownEventType_preservesOriginalType() throws Exception {
6969
SessionEvent result = MAPPER.readValue(json, SessionEvent.class);
7070

7171
assertInstanceOf(UnknownSessionEvent.class, result);
72-
assertEquals("unknown", result.getType());
72+
assertEquals("future.feature_from_server", result.getType());
73+
assertEquals("future.feature_from_server", ((UnknownSessionEvent) result).getOriginalType());
7374
}
7475

7576
@Test

src/test/java/com/github/copilot/sdk/SessionEventDeserializationTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -844,7 +844,7 @@ void testParseUnknownEventType() throws Exception {
844844
assertNotNull(event, "Unknown event types should return an UnknownSessionEvent");
845845
assertInstanceOf(com.github.copilot.sdk.generated.UnknownSessionEvent.class, event,
846846
"Unknown event types should return UnknownSessionEvent for forward compatibility");
847-
assertEquals("unknown", event.getType());
847+
assertEquals("unknown.event.type", event.getType());
848848
}
849849

850850
@Test

0 commit comments

Comments
 (0)