From cf98d240907339ea9bd09c14fda507f1d8ed228b Mon Sep 17 00:00:00 2001 From: pyrite Date: Fri, 27 Jun 2025 16:59:38 +0200 Subject: [PATCH] feat(codegen): various improvements to robustness --- torn-api-codegen/Cargo.toml | 2 +- torn-api-codegen/src/model/enum.rs | 6 + torn-api-codegen/src/model/mod.rs | 126 +++++++++++++++++-- torn-api-codegen/src/model/object.rs | 175 ++++++++++++++++++--------- torn-api-codegen/src/model/path.rs | 108 +++++++++++++---- torn-api-codegen/src/model/union.rs | 19 ++- torn-api/build.rs | 4 + 7 files changed, 344 insertions(+), 96 deletions(-) diff --git a/torn-api-codegen/Cargo.toml b/torn-api-codegen/Cargo.toml index da49172..ad3162d 100644 --- a/torn-api-codegen/Cargo.toml +++ b/torn-api-codegen/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "torn-api-codegen" authors = ["Pyrit [2111649]"] -version = "0.6.2" +version = "0.7.0" edition = "2021" description = "Contains the v2 torn API model descriptions and codegen for the bindings" license-file = { workspace = true } diff --git a/torn-api-codegen/src/model/enum.rs b/torn-api-codegen/src/model/enum.rs index 4920c03..8a2d556 100644 --- a/torn-api-codegen/src/model/enum.rs +++ b/torn-api-codegen/src/model/enum.rs @@ -77,6 +77,11 @@ impl EnumVariantTupleValue { format: Some("int32"), .. } => Some(Self::Primitive(PrimitiveType::I32)), + OpenApiType { + r#type: Some("number"), + format: Some("float") | None, + .. + } => Some(Self::Primitive(PrimitiveType::Float)), _ => None, } } @@ -146,6 +151,7 @@ impl EnumVariantTupleValue { pub fn is_comparable(&self, resolved: &ResolvedSchema) -> bool { match self { + Self::Primitive(PrimitiveType::Float) => false, Self::Primitive(_) => true, Self::Enum { inner, .. } => inner.is_comparable(resolved), Self::Ref { ty_name } | Self::ArrayOfRefs { ty_name } => resolved diff --git a/torn-api-codegen/src/model/mod.rs b/torn-api-codegen/src/model/mod.rs index 1a8ea80..47748a2 100644 --- a/torn-api-codegen/src/model/mod.rs +++ b/torn-api-codegen/src/model/mod.rs @@ -1,8 +1,10 @@ +use std::{cell::RefCell, rc::Rc}; + use indexmap::IndexMap; use newtype::Newtype; use object::Object; use parameter::Parameter; -use path::Path; +use path::{Path, PrettySegments}; use proc_macro2::TokenStream; use r#enum::Enum; use scope::Scope; @@ -35,11 +37,73 @@ impl Model { } } -#[derive(Debug, Default)] +#[derive(Default)] pub struct ResolvedSchema { pub models: IndexMap, pub paths: IndexMap, pub parameters: Vec, + + pub warnings: WarningReporter, +} + +#[derive(Clone)] +pub struct Warning { + pub message: String, + pub path: Vec, +} + +impl std::fmt::Display for Warning { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "in {}: {}", self.path.join("."), self.message) + } +} + +#[derive(Default)] +struct WarningReporterState { + warnings: Vec, + path: Vec, +} + +#[derive(Clone, Default)] +pub struct WarningReporter { + state: Rc>, +} + +impl WarningReporter { + pub fn new() -> Self { + Self::default() + } + + fn push(&self, message: impl ToString) { + let mut state = self.state.borrow_mut(); + let path = state.path.iter().map(ToString::to_string).collect(); + state.warnings.push(Warning { + message: message.to_string(), + path, + }); + } + + fn child(&self, name: impl ToString) -> WarningReporter { + self.state.borrow_mut().path.push(name.to_string()); + + Self { + state: self.state.clone(), + } + } + + pub fn is_empty(&self) -> bool { + self.state.borrow().warnings.is_empty() + } + + pub fn get_warnings(&self) -> Vec { + self.state.borrow().warnings.clone() + } +} + +impl Drop for WarningReporter { + fn drop(&mut self) { + self.state.borrow_mut().path.pop(); + } } impl ResolvedSchema { @@ -49,14 +113,20 @@ impl ResolvedSchema { for (name, r#type) in &schema.components.schemas { result.models.insert( name.to_string(), - resolve(r#type, name, &schema.components.schemas), + resolve(r#type, name, &schema.components.schemas, &result.warnings), ); } for (path, body) in &schema.paths { result.paths.insert( path.to_string(), - Path::from_schema(path, body, &schema.components.parameters).unwrap(), + Path::from_schema( + path, + body, + &schema.components.parameters, + result.warnings.child(path), + ) + .unwrap(), ); } @@ -83,7 +153,9 @@ impl ResolvedSchema { let mut output = TokenStream::default(); for path in self.paths.values() { - output.extend(path.codegen_request(self)); + output.extend( + path.codegen_request(self, self.warnings.child(PrettySegments(&path.segments))), + ); } output @@ -112,7 +184,12 @@ impl ResolvedSchema { } } -pub fn resolve(r#type: &OpenApiType, name: &str, schemas: &IndexMap<&str, OpenApiType>) -> Model { +pub fn resolve( + r#type: &OpenApiType, + name: &str, + schemas: &IndexMap<&str, OpenApiType>, + warnings: &WarningReporter, +) -> Model { match r#type { OpenApiType { r#enum: Some(_), .. @@ -120,8 +197,12 @@ pub fn resolve(r#type: &OpenApiType, name: &str, schemas: &IndexMap<&str, OpenAp OpenApiType { r#type: Some("object"), .. - } => Object::from_schema_object(name, r#type, schemas) - .map_or(Model::Unresolved, Model::Object), + } => Model::Object(Object::from_schema_object( + name, + r#type, + schemas, + warnings.child(name), + )), OpenApiType { r#type: Some(_), .. } => Newtype::from_schema(name, r#type).map_or(Model::Unresolved, Model::Newtype), @@ -132,7 +213,12 @@ pub fn resolve(r#type: &OpenApiType, name: &str, schemas: &IndexMap<&str, OpenAp OpenApiType { all_of: Some(types), .. - } => Object::from_all_of(name, types, schemas).map_or(Model::Unresolved, Model::Object), + } => Model::Object(Object::from_all_of( + name, + types, + schemas, + warnings.child(name), + )), _ => Model::Unresolved, } } @@ -159,7 +245,14 @@ mod test { let user_id_schema = schema.components.schemas.get("UserId").unwrap(); - let user_id = resolve(user_id_schema, "UserId", &schema.components.schemas); + let reporter = WarningReporter::new(); + let user_id = resolve( + user_id_schema, + "UserId", + &schema.components.schemas, + &reporter, + ); + assert!(reporter.is_empty()); assert_eq!( user_id, @@ -174,7 +267,13 @@ mod test { let attack_code_schema = schema.components.schemas.get("AttackCode").unwrap(); - let attack_code = resolve(attack_code_schema, "AttackCode", &schema.components.schemas); + let attack_code = resolve( + attack_code_schema, + "AttackCode", + &schema.components.schemas, + &reporter, + ); + assert!(reporter.is_empty()); assert_eq!( attack_code, @@ -196,7 +295,10 @@ mod test { let total = schema.components.schemas.len(); for (name, desc) in &schema.components.schemas { - if resolve(desc, name, &schema.components.schemas) == Model::Unresolved { + let reporter = WarningReporter::new(); + if resolve(desc, name, &schema.components.schemas, &reporter) == Model::Unresolved + || !reporter.is_empty() + { unresolved.push(name); } } diff --git a/torn-api-codegen/src/model/object.rs b/torn-api-codegen/src/model/object.rs index 48b3688..b7f454f 100644 --- a/torn-api-codegen/src/model/object.rs +++ b/torn-api-codegen/src/model/object.rs @@ -1,12 +1,12 @@ use heck::{ToSnakeCase, ToUpperCamelCase}; -use indexmap::IndexMap; +use indexmap::{map::Entry, IndexMap}; use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; use syn::Ident; use crate::openapi::r#type::OpenApiType; -use super::{r#enum::Enum, ResolvedSchema}; +use super::{r#enum::Enum, ResolvedSchema, WarningReporter}; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum PrimitiveType { @@ -85,6 +85,7 @@ impl PropertyType { #[derive(Debug, Clone, PartialEq, Eq)] pub struct Property { + pub field_name: String, pub name: String, pub description: Option, pub required: bool, @@ -99,24 +100,31 @@ impl Property { required: bool, schema: &OpenApiType, schemas: &IndexMap<&str, OpenApiType>, + warnings: WarningReporter, ) -> Option { let name = name.to_owned(); + let field_name = name.to_snake_case(); let description = schema.description.as_deref().map(ToOwned::to_owned); match schema { OpenApiType { r#enum: Some(_), .. - } => Some(Self { - r#type: PropertyType::Enum(Enum::from_schema( - &name.clone().to_upper_camel_case(), - schema, - )?), - name, - description, - required, - deprecated: schema.deprecated, - nullable: false, - }), + } => { + let Some(r#enum) = Enum::from_schema(&name.clone().to_upper_camel_case(), schema) + else { + warnings.push("Failed to create enum"); + return None; + }; + Some(Self { + r#type: PropertyType::Enum(r#enum), + name, + field_name, + description, + required, + deprecated: schema.deprecated, + nullable: false, + }) + } OpenApiType { one_of: Some(types), .. @@ -125,7 +133,7 @@ impl Property { r#type: Some("null"), .. }] => { - let mut inner = Self::from_schema(&name, required, left, schemas)?; + let mut inner = Self::from_schema(&name, required, left, schemas, warnings)?; inner.nullable = true; Some(inner) } @@ -137,14 +145,19 @@ impl Property { one_of: Some(left.to_owned()), ..schema.clone() }; - let mut inner = Self::from_schema(&name, required, &rest, schemas)?; + let mut inner = Self::from_schema(&name, required, &rest, schemas, warnings)?; inner.nullable = true; Some(inner) } cases => { - let r#enum = Enum::from_one_of(&name.to_upper_camel_case(), cases)?; + let Some(r#enum) = Enum::from_one_of(&name.to_upper_camel_case(), cases) else { + warnings.push("Failed to create oneOf enum"); + return None; + }; + Some(Self { name, + field_name, description, required, nullable: false, @@ -157,9 +170,12 @@ impl Property { all_of: Some(types), .. } => { - let composite = Object::from_all_of(&name.to_upper_camel_case(), types, schemas)?; + let obj_name = name.to_upper_camel_case(); + let composite = + Object::from_all_of(&obj_name, types, schemas, warnings.child(&obj_name)); Some(Self { name, + field_name, description, required, nullable: false, @@ -170,23 +186,29 @@ impl Property { OpenApiType { r#type: Some("object"), .. - } => Some(Self { - r#type: PropertyType::Nested(Box::new(Object::from_schema_object( - &name.clone().to_upper_camel_case(), - schema, - schemas, - )?)), - name, - description, - required, - deprecated: schema.deprecated, - nullable: false, - }), + } => { + let obj_name = name.to_upper_camel_case(); + Some(Self { + r#type: PropertyType::Nested(Box::new(Object::from_schema_object( + &obj_name, + schema, + schemas, + warnings.child(&obj_name), + ))), + name, + field_name, + description, + required, + deprecated: schema.deprecated, + nullable: false, + }) + } OpenApiType { ref_path: Some(path), .. } => Some(Self { name, + field_name, description, r#type: PropertyType::Ref((*path).to_owned()), required, @@ -198,10 +220,11 @@ impl Property { items: Some(items), .. } => { - let inner = Self::from_schema(&name, required, items, schemas)?; + let inner = Self::from_schema(&name, required, items, schemas, warnings)?; Some(Self { name, + field_name, description, required, nullable: false, @@ -226,6 +249,7 @@ impl Property { Some(Self { name, + field_name, description, required, nullable: false, @@ -233,7 +257,10 @@ impl Property { r#type: PropertyType::Primitive(prim), }) } - _ => None, + _ => { + warnings.push("Could not resolve property type"); + None + } } } @@ -247,11 +274,11 @@ impl Property { let name = &self.name; let (name, serde_attr) = match name.as_str() { "type" => (format_ident!("r#type"), None), - name if name != name.to_snake_case() => ( - format_ident!("{}", name.to_snake_case()), + name if name != self.field_name => ( + format_ident!("{}", self.field_name), Some(quote! { #[serde(rename = #name)]}), ), - _ => (format_ident!("{name}"), None), + _ => (format_ident!("{}", self.field_name), None), }; let ty_inner = self.r#type.codegen(namespace, resolved)?; @@ -283,7 +310,7 @@ impl Property { pub struct Object { pub name: String, pub description: Option, - pub properties: Vec, + pub properties: IndexMap, } impl Object { @@ -291,7 +318,8 @@ impl Object { name: &str, schema: &OpenApiType, schemas: &IndexMap<&str, OpenApiType>, - ) -> Option { + warnings: WarningReporter, + ) -> Self { let mut result = Object { name: name.to_owned(), description: schema.description.as_deref().map(ToOwned::to_owned), @@ -299,39 +327,54 @@ impl Object { }; let Some(props) = &schema.properties else { - return None; + warnings.push("Missing properties"); + return result; }; let required = schema.required.clone().unwrap_or_default(); for (prop_name, prop) in props { - // HACK: This will cause a duplicate key otherwise - if ["itemDetails", "sci-fi", "non-attackers", "co-leader_id"].contains(prop_name) { - continue; - } - - // TODO: implement custom enum for this (depends on overrides being added) - // Maybe this is an issue with the schema instead? - if *prop_name == "value" && name == "TornHof" { - continue; - } - - result.properties.push(Property::from_schema( + let Some(prop) = Property::from_schema( prop_name, required.contains(prop_name), prop, schemas, - )?); + warnings.child(prop_name), + ) else { + continue; + }; + + let field_name = prop.field_name.clone(); + + let entry = result.properties.entry(field_name.clone()); + if let Entry::Occupied(mut entry) = entry { + let other_name = entry.get().name.clone(); + warnings.push(format!( + "Property name collision: {other_name} and {field_name}" + )); + // deprioritise kebab and camelcase + if other_name.contains('-') + || other_name + .chars() + .filter(|c| c.is_alphabetic()) + .all(|c| c.is_ascii_lowercase()) + { + entry.insert(prop); + } + } else { + entry.insert_entry(prop); + } } - Some(result) + result } pub fn from_all_of( name: &str, types: &[OpenApiType], schemas: &IndexMap<&str, OpenApiType>, - ) -> Option { + warnings: WarningReporter, + ) -> Self { let mut result = Self { name: name.to_owned(), ..Default::default() @@ -339,22 +382,29 @@ impl Object { for r#type in types { let r#type = if let OpenApiType { - ref_path: Some(path), + ref_path: Some(ref_path), .. } = r#type { - let name = path.strip_prefix("#/components/schemas/")?; - schemas.get(name)? + let Some(name) = ref_path.strip_prefix("#/components/schemas/") else { + warnings.push(format!("Malformed ref {ref_path}")); + continue; + }; + let Some(schema) = schemas.get(name) else { + warnings.push(format!("Missing schema for ref {name}")); + continue; + }; + schema } else { r#type }; - let obj = Self::from_schema_object(name, r#type, schemas)?; + let obj = Self::from_schema_object(name, r#type, schemas, warnings.child("variant")); result.description = result.description.or(obj.description); result.properties.extend(obj.properties); } - Some(result) + result } pub fn codegen(&self, resolved: &ResolvedSchema) -> Option { @@ -371,7 +421,7 @@ impl Object { }; let mut props = Vec::with_capacity(self.properties.len()); - for prop in &self.properties { + for (_, prop) in &self.properties { props.push(prop.codegen(&mut namespace, resolved)?); } @@ -441,7 +491,14 @@ mod test { for (name, desc) in &schema.components.schemas { if desc.r#type == Some("object") { objects += 1; - if Object::from_schema_object(name, desc, &schema.components.schemas).is_none() { + let reporter = WarningReporter::new(); + Object::from_schema_object( + name, + desc, + &schema.components.schemas, + reporter.clone(), + ); + if !reporter.is_empty() { unresolved.push(name); } } diff --git a/torn-api-codegen/src/model/path.rs b/torn-api-codegen/src/model/path.rs index 319360b..8360baa 100644 --- a/torn-api-codegen/src/model/path.rs +++ b/torn-api-codegen/src/model/path.rs @@ -14,7 +14,7 @@ use crate::openapi::{ use super::{ parameter::{Parameter, ParameterLocation, ParameterType}, union::Union, - ResolvedSchema, + ResolvedSchema, WarningReporter, }; #[derive(Debug, Clone)] @@ -23,6 +23,21 @@ pub enum PathSegment { Parameter { name: String }, } +pub struct PrettySegments<'a>(pub &'a [PathSegment]); + +impl std::fmt::Display for PrettySegments<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for segment in self.0 { + match segment { + PathSegment::Constant(c) => write!(f, "/{c}")?, + PathSegment::Parameter { name } => write!(f, "/{{{name}}}")?, + } + } + + Ok(()) + } +} + #[derive(Debug, Clone)] pub enum PathParameter { Inline(Parameter), @@ -51,6 +66,7 @@ impl Path { path: &str, schema: &OpenApiPath, parameters: &IndexMap<&str, OpenApiParameter>, + warnings: WarningReporter, ) -> Option { let mut segments = Vec::new(); for segment in path.strip_prefix('/')?.split('/') { @@ -111,9 +127,13 @@ impl Path { .strip_prefix("#/components/schemas/")? .to_owned(), }, - OpenApiResponseBody::Union { any_of: _ } => PathResponse::ArbitraryUnion( - Union::from_schema("Response", &schema.get.response_content)?, - ), + OpenApiResponseBody::Union { any_of: _ } => { + PathResponse::ArbitraryUnion(Union::from_schema( + "Response", + &schema.get.response_content, + warnings.child("response"), + )?) + } }; Some(Self { @@ -126,7 +146,11 @@ impl Path { }) } - pub fn codegen_request(&self, resolved: &ResolvedSchema) -> Option { + pub fn codegen_request( + &self, + resolved: &ResolvedSchema, + warnings: WarningReporter, + ) -> Option { let name = if self.segments.len() == 1 { let Some(PathSegment::Constant(first)) = self.segments.first() else { return None; @@ -207,17 +231,30 @@ impl Path { let query_val = ¶m.value; if param.location == ParameterLocation::Path { - discriminant.push(ty.clone()); - discriminant_val.push(quote! { self.#name }); - let path_name = format_ident!("{}", param.value); - start_fields.push(quote! { - #[cfg_attr(feature = "builder", builder(start_fn))] - #builder_param - pub #name: #ty - }); - fmt_val.push(quote! { - #path_name=self.#name - }); + if self.segments.iter().any(|s| { + if let PathSegment::Parameter { name } = s { + name == ¶m.value + } else { + false + } + }) { + discriminant.push(ty.clone()); + discriminant_val.push(quote! { self.#name }); + let path_name = format_ident!("{}", param.value); + start_fields.push(quote! { + #[cfg_attr(feature = "builder", builder(start_fn))] + #builder_param + pub #name: #ty + }); + fmt_val.push(quote! { + #path_name=self.#name + }); + } else { + warnings.push(format!( + "Provided path parameter is not present in the url: {}", + param.value + )); + } } else { let ty = if param.required { convert_field.push(quote! { @@ -329,7 +366,15 @@ impl Path { PathParameter::Component(param) => (param, false), }; - if param.location == ParameterLocation::Path { + if param.location == ParameterLocation::Path + && self.segments.iter().any(|s| { + if let PathSegment::Parameter { name } = s { + name == ¶m.value + } else { + false + } + }) + { let ty = match ¶m.r#type { ParameterType::I32 { .. } | ParameterType::Enum { .. } => { let ty_name = format_ident!("{}", param.name); @@ -444,8 +489,15 @@ impl Path { PathParameter::Inline(param) => (param, true), PathParameter::Component(param) => (param, false), }; - - if param.location == ParameterLocation::Path { + if param.location == ParameterLocation::Path + && self.segments.iter().any(|s| { + if let PathSegment::Parameter { name } = s { + name == ¶m.value + } else { + false + } + }) + { let ty = match ¶m.r#type { ParameterType::I32 { .. } | ParameterType::Enum { .. } => { let ty_name = format_ident!("{}", param.name); @@ -604,7 +656,14 @@ mod test { for (name, desc) in &schema.paths { paths += 1; - if Path::from_schema(name, desc, &schema.components.parameters).is_none() { + if Path::from_schema( + name, + desc, + &schema.components.parameters, + WarningReporter::new(), + ) + .is_none() + { unresolved.push(name); } } @@ -627,18 +686,23 @@ mod test { fn codegen_paths() { let schema = get_schema(); let resolved = ResolvedSchema::from_open_api(&schema); + let reporter = WarningReporter::new(); let mut paths = 0; let mut unresolved = vec![]; for (name, desc) in &schema.paths { paths += 1; - let Some(path) = Path::from_schema(name, desc, &schema.components.parameters) else { + let Some(path) = + Path::from_schema(name, desc, &schema.components.parameters, reporter.clone()) + else { unresolved.push(name); continue; }; - if path.codegen_scope_call().is_none() || path.codegen_request(&resolved).is_none() { + if path.codegen_scope_call().is_none() + || path.codegen_request(&resolved, reporter.clone()).is_none() + { unresolved.push(name); } } diff --git a/torn-api-codegen/src/model/union.rs b/torn-api-codegen/src/model/union.rs index 4cabae3..64e0def 100644 --- a/torn-api-codegen/src/model/union.rs +++ b/torn-api-codegen/src/model/union.rs @@ -4,6 +4,8 @@ use quote::{format_ident, quote}; use crate::openapi::path::OpenApiResponseBody; +use super::WarningReporter; + #[derive(Debug, Clone)] pub struct Union { pub name: String, @@ -11,10 +13,23 @@ pub struct Union { } impl Union { - pub fn from_schema(name: &str, schema: &OpenApiResponseBody) -> Option { + pub fn from_schema( + name: &str, + schema: &OpenApiResponseBody, + warnings: WarningReporter, + ) -> Option { let members = match schema { OpenApiResponseBody::Union { any_of } => { - any_of.iter().map(|l| l.ref_path.to_owned()).collect() + let mut members = Vec::with_capacity(any_of.len()); + for l in any_of { + let path = l.ref_path.to_owned(); + if members.contains(&path) { + warnings.push(format!("Duplicate member: {path}")); + } else { + members.push(path); + } + } + members } _ => return None, }; diff --git a/torn-api/build.rs b/torn-api/build.rs index 0d74a6b..5669358 100644 --- a/torn-api/build.rs +++ b/torn-api/build.rs @@ -28,4 +28,8 @@ fn main() { let scopes_file = syn::parse2(resolved.codegen_scopes()).unwrap(); let scopes_pretty = prettyplease::unparse(&scopes_file); fs::write(&scopes_dest, scopes_pretty).unwrap(); + + for warning in resolved.warnings.get_warnings() { + println!("cargo:warning={}", warning); + } }