fix: explicitly list name collisions in JSON schema generation (#12406)
## Why JSON schema codegen was silently resolving naming collisions by appending numeric suffixes (for example `...2`, `...3`). That makes the generated schema names unstable: removing an earlier colliding type can cause a later type to be renumbered, which is a breaking change for consumers that referenced the old generated name. This PR makes those collisions explicit and reviewable. Though note that once we remove `v1` from the codegen, we will no longer support naming collisions. Or rather, naming collisions will have to be handled explicitly rather than the numeric suffix approach. ## What Changed - In `codex-rs/app-server-protocol/src/export.rs`, replaced implicit numeric suffix collision handling for generated variant titles with explicit special-case maps. - Added a panic when a collision occurs without an entry in the map, so new collisions fail loudly instead of silently renaming generated schema types. - Added the currently required special cases so existing generated names remain stable. - Extended the same approach to numbered `definitions` / `$defs` collisions (for example `MessagePhase2`-style names) so those are also explicitly tracked. ## Verification - Ran targeted generator-path test: - `cargo test -p codex-app-server-protocol generate_json_filters_experimental_fields_and_methods -- --nocapture` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/12406). * #12408 * __->__ #12406
This commit is contained in:
Michael Bolin
GitHub
parent
36a2a9fdbb
commit
53bcfaf42d
1 changed files with 235 additions and 4 deletions
|
|
@ -32,10 +32,93 @@ use std::io::Write;
|
|||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
use std::sync::LazyLock;
|
||||
use ts_rs::TS;
|
||||
|
||||
const HEADER: &str = "// GENERATED CODE! DO NOT MODIFY BY HAND!\n\n";
|
||||
const IGNORED_DEFINITIONS: &[&str] = &["Option<()>"];
|
||||
static VARIANT_TITLE_COLLISION_OVERRIDES: LazyLock<HashMap<&'static str, &'static str>> =
|
||||
LazyLock::new(|| {
|
||||
HashMap::from([
|
||||
(
|
||||
"base=McpStartupStatus|generated=StateMcpStartupStatus|literal:state=ready|only_property=state|required_only=state",
|
||||
"StateMcpStartupStatus2",
|
||||
),
|
||||
(
|
||||
"base=McpStartupStatus|generated=StateMcpStartupStatus|literal:state=cancelled|only_property=state|required_only=state",
|
||||
"StateMcpStartupStatus3",
|
||||
),
|
||||
])
|
||||
});
|
||||
static NUMBERED_DEFINITION_COLLISION_OVERRIDES: LazyLock<HashMap<&'static str, &'static str>> =
|
||||
LazyLock::new(|| {
|
||||
HashMap::from([
|
||||
(
|
||||
"schema=ClientRequest|container=definitions|generated=AskForApproval2|base=AskForApproval",
|
||||
"AskForApproval2",
|
||||
),
|
||||
(
|
||||
"schema=ClientRequest|container=definitions|generated=NetworkAccess2|base=NetworkAccess",
|
||||
"NetworkAccess2",
|
||||
),
|
||||
(
|
||||
"schema=ClientRequest|container=definitions|generated=ReadOnlyAccess2|base=ReadOnlyAccess",
|
||||
"ReadOnlyAccess2",
|
||||
),
|
||||
(
|
||||
"schema=ClientRequest|container=definitions|generated=SandboxMode2|base=SandboxMode",
|
||||
"SandboxMode2",
|
||||
),
|
||||
(
|
||||
"schema=ClientRequest|container=definitions|generated=SandboxPolicy2|base=SandboxPolicy",
|
||||
"SandboxPolicy2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=ByteRange2|base=ByteRange",
|
||||
"ByteRange2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=CodexErrorInfo2|base=CodexErrorInfo",
|
||||
"CodexErrorInfo2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=CreditsSnapshot2|base=CreditsSnapshot",
|
||||
"CreditsSnapshot2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=MessagePhase2|base=MessagePhase",
|
||||
"MessagePhase2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=ModelRerouteReason2|base=ModelRerouteReason",
|
||||
"ModelRerouteReason2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=PatchApplyStatus2|base=PatchApplyStatus",
|
||||
"PatchApplyStatus2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=RateLimitSnapshot2|base=RateLimitSnapshot",
|
||||
"RateLimitSnapshot2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=RateLimitWindow2|base=RateLimitWindow",
|
||||
"RateLimitWindow2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=TextElement2|base=TextElement",
|
||||
"TextElement2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=UserInput2|base=UserInput",
|
||||
"UserInput2",
|
||||
),
|
||||
(
|
||||
"schema=ServerNotification|container=definitions|generated=WebSearchAction2|base=WebSearchAction",
|
||||
"WebSearchAction2",
|
||||
),
|
||||
])
|
||||
});
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct GeneratedSchema {
|
||||
|
|
@ -950,6 +1033,7 @@ where
|
|||
let file_stem = name.trim();
|
||||
let schema = schema_for!(T);
|
||||
let mut schema_value = serde_json::to_value(schema)?;
|
||||
enforce_numbered_definition_collision_overrides(file_stem, &mut schema_value);
|
||||
annotate_schema(&mut schema_value, Some(file_stem));
|
||||
// If the name looks like a namespaced path (e.g., "v2::Type"), mirror
|
||||
// the TypeScript layout and write to out_dir/v2/Type.json. Otherwise
|
||||
|
|
@ -980,6 +1064,101 @@ where
|
|||
})
|
||||
}
|
||||
|
||||
fn enforce_numbered_definition_collision_overrides(schema_name: &str, schema: &mut Value) {
|
||||
for defs_key in ["definitions", "$defs"] {
|
||||
let renames = {
|
||||
let Some(defs) = schema.get(defs_key).and_then(Value::as_object) else {
|
||||
continue;
|
||||
};
|
||||
collect_numbered_definition_collision_renames(schema_name, defs_key, defs)
|
||||
};
|
||||
|
||||
if renames.is_empty() {
|
||||
continue;
|
||||
}
|
||||
|
||||
#[expect(clippy::expect_used)]
|
||||
let defs = schema
|
||||
.get_mut(defs_key)
|
||||
.and_then(Value::as_object_mut)
|
||||
.expect("definition map should still be present");
|
||||
for (old_name, new_name, collision_key) in &renames {
|
||||
if old_name == new_name {
|
||||
continue;
|
||||
}
|
||||
assert!(
|
||||
!defs.contains_key(new_name),
|
||||
"Numbered definition collision override produced duplicate definition {new_name} for {collision_key}",
|
||||
);
|
||||
let schema_value = defs.remove(old_name).unwrap_or_else(|| {
|
||||
panic!("Missing generated numbered definition {old_name} for {collision_key}")
|
||||
});
|
||||
defs.insert(new_name.clone(), schema_value);
|
||||
}
|
||||
|
||||
for (old_name, new_name, _) in renames {
|
||||
if old_name != new_name {
|
||||
rewrite_named_local_ref(schema, defs_key, &old_name, &new_name);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn collect_numbered_definition_collision_renames(
|
||||
schema_name: &str,
|
||||
defs_key: &str,
|
||||
defs: &Map<String, Value>,
|
||||
) -> Vec<(String, String, String)> {
|
||||
let mut renames = Vec::new();
|
||||
|
||||
for generated_name in defs.keys() {
|
||||
let base_name = generated_name.trim_end_matches(|c: char| c.is_ascii_digit());
|
||||
if base_name == generated_name || !defs.contains_key(base_name) {
|
||||
continue;
|
||||
}
|
||||
|
||||
let collision_key = format!(
|
||||
"schema={schema_name}|container={defs_key}|generated={generated_name}|base={base_name}"
|
||||
);
|
||||
let replacement = NUMBERED_DEFINITION_COLLISION_OVERRIDES
|
||||
.get(collision_key.as_str())
|
||||
.map(|value| (*value).to_string())
|
||||
.unwrap_or_else(|| {
|
||||
panic!("Missing numbered definition collision override for {collision_key}")
|
||||
});
|
||||
renames.push((generated_name.clone(), replacement, collision_key));
|
||||
}
|
||||
|
||||
renames
|
||||
}
|
||||
|
||||
fn rewrite_named_local_ref(value: &mut Value, defs_key: &str, old_name: &str, new_name: &str) {
|
||||
let direct = format!("#/{defs_key}/{old_name}");
|
||||
let prefixed = format!("{direct}/");
|
||||
let replacement = format!("#/{defs_key}/{new_name}");
|
||||
let replacement_prefixed = format!("{replacement}/");
|
||||
match value {
|
||||
Value::Object(obj) => {
|
||||
if let Some(Value::String(reference)) = obj.get_mut("$ref") {
|
||||
if reference == &direct {
|
||||
*reference = replacement;
|
||||
} else if let Some(rest) = reference.strip_prefix(&prefixed) {
|
||||
*reference = format!("{replacement_prefixed}{rest}");
|
||||
}
|
||||
}
|
||||
for child in obj.values_mut() {
|
||||
rewrite_named_local_ref(child, defs_key, old_name, new_name);
|
||||
}
|
||||
}
|
||||
Value::Array(items) => {
|
||||
for child in items {
|
||||
rewrite_named_local_ref(child, defs_key, old_name, new_name);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn write_json_schema<T>(out_dir: &Path, name: &str) -> Result<GeneratedSchema>
|
||||
where
|
||||
T: JsonSchema,
|
||||
|
|
@ -1198,10 +1377,20 @@ fn annotate_variant_list(variants: &mut [Value], base: Option<&str>) {
|
|||
&& let Some(name) = variant_definition_name(base_name, variant)
|
||||
{
|
||||
let mut candidate = name.clone();
|
||||
let mut index = 2;
|
||||
while seen.contains(&candidate) {
|
||||
candidate = format!("{name}{index}");
|
||||
index += 1;
|
||||
if seen.contains(&candidate) {
|
||||
let collision_key = variant_title_collision_key(base_name, &name, variant);
|
||||
candidate = VARIANT_TITLE_COLLISION_OVERRIDES
|
||||
.get(collision_key.as_str())
|
||||
.map(|value| (*value).to_string())
|
||||
.unwrap_or_else(|| {
|
||||
panic!(
|
||||
"Missing variant title collision override for {collision_key} (generated name: {name})"
|
||||
)
|
||||
});
|
||||
assert!(
|
||||
!seen.contains(&candidate),
|
||||
"Variant title collision override produced duplicate title {candidate} for {collision_key}",
|
||||
);
|
||||
}
|
||||
if let Some(obj) = variant.as_object_mut() {
|
||||
obj.insert("title".into(), Value::String(candidate.clone()));
|
||||
|
|
@ -1221,6 +1410,48 @@ fn annotate_variant_list(variants: &mut [Value], base: Option<&str>) {
|
|||
}
|
||||
}
|
||||
|
||||
fn variant_title_collision_key(base: &str, generated_name: &str, variant: &Value) -> String {
|
||||
let mut parts = vec![
|
||||
format!("base={base}"),
|
||||
format!("generated={generated_name}"),
|
||||
];
|
||||
|
||||
if let Some(props) = variant.get("properties").and_then(Value::as_object) {
|
||||
for key in DISCRIMINATOR_KEYS {
|
||||
if let Some(value) = literal_from_property(props, key) {
|
||||
parts.push(format!("{key}={value}"));
|
||||
}
|
||||
}
|
||||
for (key, value) in props {
|
||||
if DISCRIMINATOR_KEYS.contains(&key.as_str()) {
|
||||
continue;
|
||||
}
|
||||
if let Some(literal) = string_literal(value) {
|
||||
parts.push(format!("literal:{key}={literal}"));
|
||||
}
|
||||
}
|
||||
|
||||
if props.len() == 1
|
||||
&& let Some(key) = props.keys().next()
|
||||
{
|
||||
parts.push(format!("only_property={key}"));
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(required) = variant.get("required").and_then(Value::as_array)
|
||||
&& required.len() == 1
|
||||
&& let Some(key) = required[0].as_str()
|
||||
{
|
||||
parts.push(format!("required_only={key}"));
|
||||
}
|
||||
|
||||
if parts.len() == 2 {
|
||||
parts.push(format!("variant={variant}"));
|
||||
}
|
||||
|
||||
parts.join("|")
|
||||
}
|
||||
|
||||
const DISCRIMINATOR_KEYS: &[&str] = &["type", "method", "mode", "status", "role", "reason"];
|
||||
|
||||
fn set_discriminator_titles(props: &mut Map<String, Value>, owner: &str) {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue