From e4379c321964d15c5db1c3ac5afda8428fbc7dff Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 2 Jan 2026 02:39:42 +0000 Subject: [PATCH 1/3] review comments + restore fast path Created using spr 1.3.6-beta.1 --- crates/dropshot-api-manager/src/resolved.rs | 69 ++++++++++--------- .../src/spec_files_local.rs | 5 +- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index 034ee37..8a1c9bd 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -1479,11 +1479,11 @@ fn resolve_api_version_blessed<'a>( } } - // Determine if this version should be stored as a git ref (slow path). - // We need this info before handling corrupted files so we can regenerate - // them in the correct format. - let should_be_git_ref = if use_git_ref_storage && !is_latest { - match git_ref { + // Local function to compute the git ref for this version. This is + // expensive because it may need to resolve a git revision to a commit + // hash. + let compute_should_be_git_ref = + |problems: &mut Vec>| match git_ref { Some(r) => match r.to_git_ref(&env.repo_root) { Ok(current) => should_convert_to_git_ref( latest_first_commit, @@ -1499,39 +1499,31 @@ fn resolve_api_version_blessed<'a>( } }, None => None, - } - } else { - None - }; - - // Report corrupted local files that need regeneration from blessed. - for local_file in &corrupted { - problems.push(Problem::BlessedVersionCorruptedLocal { - local_file, - blessed, - git_ref: should_be_git_ref.clone(), - }); - } + }; if matching.is_empty() && corrupted.is_empty() { // No valid or corrupted local files match the blessed version. problems.push(Problem::BlessedVersionMissingLocal { spec_file_name: blessed.spec_file_name().clone(), }); - } else if matching.is_empty() { - // Only corrupted files match - they'll be regenerated. Still need to - // mark non-matching files as extra. - problems.extend(non_matching.into_iter().map(|s| { - Problem::BlessedVersionExtraLocalSpec { - spec_file_name: s.spec_file_name().clone(), - } - })); } else if !use_git_ref_storage || is_latest { // Fast path: git ref storage disabled or this is the latest version. - // Computing first commits is slow, and we know we always want JSON in - // this case, so we can avoid computing them here. + // We know we always want JSON in this case, so we can avoid computing + // git refs here. + + // Report corrupted local files that need regeneration from blessed. + for local_file in &corrupted { + problems.push(Problem::BlessedVersionCorruptedLocal { + local_file, + blessed, + git_ref: None, + }); + } - if matching.len() > 1 { + if matching.is_empty() { + // Only corrupted files match - they'll be regenerated. Still need + // to mark non-matching files as extra. + } else if matching.len() > 1 { // We might have both api.json and api.json.gitref for the same // version. Mark the redundant file (always the gitref file in this // case) for deletion. @@ -1554,10 +1546,23 @@ fn resolve_api_version_blessed<'a>( } })); } else { - // Slow path: git ref storage enabled and not latest. - // should_be_git_ref was computed earlier. + // Slow path: git ref storage enabled and not latest. Compute whether + // this version should be stored as a git ref. + let should_be_git_ref = compute_should_be_git_ref(&mut problems); + + // Report corrupted local files that need regeneration from blessed. + for local_file in &corrupted { + problems.push(Problem::BlessedVersionCorruptedLocal { + local_file, + blessed, + git_ref: should_be_git_ref.clone(), + }); + } - if matching.len() > 1 { + if matching.is_empty() { + // Only corrupted files match - they'll be regenerated. Still need + // to mark non-matching files as extra. + } else if matching.len() > 1 { // We might have both api.json and api.json.gitref for the same // version. Mark the redundant file for deletion. for local_file in matching { diff --git a/crates/dropshot-api-manager/src/spec_files_local.rs b/crates/dropshot-api-manager/src/spec_files_local.rs index 4867f19..4806aa3 100644 --- a/crates/dropshot-api-manager/src/spec_files_local.rs +++ b/crates/dropshot-api-manager/src/spec_files_local.rs @@ -72,10 +72,7 @@ impl LocalApiSpecFile { impl SpecFileInfo for LocalApiSpecFile { fn spec_file_name(&self) -> &ApiSpecFileName { - match self { - LocalApiSpecFile::Valid(spec) => spec.spec_file_name(), - LocalApiSpecFile::Unparseable { name, .. } => name, - } + self.spec_file_name() } fn parsed_version(&self) -> Option<&semver::Version> { From 17ddddb01fd67fac738612bf2c4e968658909790 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 2 Jan 2026 04:02:49 +0000 Subject: [PATCH 2/3] simplify unparseable file handling Created using spr 1.3.6-beta.1 --- .../src/spec_files_blessed.rs | 13 +++++--- .../src/spec_files_generated.rs | 13 +++++--- .../src/spec_files_generic.rs | 30 +++++++++---------- .../src/spec_files_local.rs | 7 ++--- 4 files changed, 34 insertions(+), 29 deletions(-) diff --git a/crates/dropshot-api-manager/src/spec_files_blessed.rs b/crates/dropshot-api-manager/src/spec_files_blessed.rs index c5535aa..9570aa1 100644 --- a/crates/dropshot-api-manager/src/spec_files_blessed.rs +++ b/crates/dropshot-api-manager/src/spec_files_blessed.rs @@ -61,9 +61,11 @@ impl ApiLoad for BlessedApiSpecFile { fn make_unparseable_item( _name: ApiSpecFileName, _contents: Vec, - ) -> Option { - // Blessed files should always be valid. - None + ) -> Self { + panic!( + "make_unparseable_item called on BlessedApiSpecFile, but \ + UNPARSEABLE_FILES_ALLOWED is false" + ); } fn try_extend_unparseable( @@ -71,7 +73,10 @@ impl ApiLoad for BlessedApiSpecFile { _name: ApiSpecFileName, _contents: Vec, ) { - // Blessed files should always be valid. + panic!( + "try_extend_unparseable called on BlessedApiSpecFile, but \ + UNPARSEABLE_FILES_ALLOWED is false" + ); } } diff --git a/crates/dropshot-api-manager/src/spec_files_generated.rs b/crates/dropshot-api-manager/src/spec_files_generated.rs index b33295a..51a34d5 100644 --- a/crates/dropshot-api-manager/src/spec_files_generated.rs +++ b/crates/dropshot-api-manager/src/spec_files_generated.rs @@ -54,9 +54,11 @@ impl ApiLoad for GeneratedApiSpecFile { fn make_unparseable_item( _name: ApiSpecFileName, _contents: Vec, - ) -> Option { - // Generated files should always be valid. - None + ) -> Self { + panic!( + "make_unparseable_item called on GeneratedApiSpecFile, but \ + UNPARSEABLE_FILES_ALLOWED is false" + ); } fn try_extend_unparseable( @@ -64,7 +66,10 @@ impl ApiLoad for GeneratedApiSpecFile { _name: ApiSpecFileName, _contents: Vec, ) { - // Generated files should always be valid. + panic!( + "try_extend_unparseable called on GeneratedApiSpecFile, but \ + UNPARSEABLE_FILES_ALLOWED is false" + ); } } diff --git a/crates/dropshot-api-manager/src/spec_files_generic.rs b/crates/dropshot-api-manager/src/spec_files_generic.rs index a715f0f..c9d3d74 100644 --- a/crates/dropshot-api-manager/src/spec_files_generic.rs +++ b/crates/dropshot-api-manager/src/spec_files_generic.rs @@ -731,14 +731,11 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { match entry { Entry::Vacant(vacant_entry) => { - if let Some(unparseable_item) = - T::make_unparseable_item( - file_name.clone(), - contents, - ) - { - vacant_entry.insert(unparseable_item); - } + let unparseable_item = T::make_unparseable_item( + file_name.clone(), + contents, + ); + vacant_entry.insert(unparseable_item); } Entry::Occupied(mut occupied_entry) => { occupied_entry @@ -962,19 +959,20 @@ pub trait ApiLoad { /// couldn't be parsed. This allows the contents to still be accessed even /// though parsing failed. /// - /// Returns `Some` if this type supports tracking unparseable files (e.g., - /// local files), or `None` if not (e.g., blessed files which should always - /// be valid). - fn make_unparseable_item( - name: ApiSpecFileName, - contents: Vec, - ) -> Option + /// # Panics + /// + /// Panics if `UNPARSEABLE_FILES_ALLOWED` is `false`. The caller must check + /// `UNPARSEABLE_FILES_ALLOWED` before calling this method. + fn make_unparseable_item(name: ApiSpecFileName, contents: Vec) -> Self where Self: Sized; /// Try to add an unparseable file to an existing entry. /// - /// For types that don't support unparseable files, this should do nothing. + /// # Panics + /// + /// Panics if `UNPARSEABLE_FILES_ALLOWED` is `false`. The caller must check + /// `UNPARSEABLE_FILES_ALLOWED` before calling this method. fn try_extend_unparseable( &mut self, name: ApiSpecFileName, diff --git a/crates/dropshot-api-manager/src/spec_files_local.rs b/crates/dropshot-api-manager/src/spec_files_local.rs index 4806aa3..34366af 100644 --- a/crates/dropshot-api-manager/src/spec_files_local.rs +++ b/crates/dropshot-api-manager/src/spec_files_local.rs @@ -101,11 +101,8 @@ impl ApiLoad for Vec { vec![LocalApiSpecFile::Valid(Box::new(raw))] } - fn make_unparseable_item( - name: ApiSpecFileName, - contents: Vec, - ) -> Option { - Some(vec![LocalApiSpecFile::Unparseable { name, contents }]) + fn make_unparseable_item(name: ApiSpecFileName, contents: Vec) -> Self { + vec![LocalApiSpecFile::Unparseable { name, contents }] } fn try_extend_unparseable( From 66e6b23f72e9432fc6ec23f3694c710e7367ad38 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 2 Jan 2026 05:36:33 +0000 Subject: [PATCH 3/3] remove some panics Created using spr 1.3.6-beta.1 --- crates/dropshot-api-manager/src/cmd/debug.rs | 2 +- .../src/spec_files_blessed.rs | 26 ++-- .../src/spec_files_generated.rs | 26 ++-- .../src/spec_files_generic.rs | 132 ++++++++---------- .../src/spec_files_local.rs | 67 +++++---- 5 files changed, 118 insertions(+), 135 deletions(-) diff --git a/crates/dropshot-api-manager/src/cmd/debug.rs b/crates/dropshot-api-manager/src/cmd/debug.rs index 4d5fdf4..ddba2ce 100644 --- a/crates/dropshot-api-manager/src/cmd/debug.rs +++ b/crates/dropshot-api-manager/src/cmd/debug.rs @@ -114,7 +114,7 @@ fn dump_structure( println!(" version {}:", version); for api_spec in files.as_raw_files() { let version_str = api_spec - .parsed_version() + .version() .map(|v| v.to_string()) .unwrap_or_else(|| "unparseable".to_string()); println!( diff --git a/crates/dropshot-api-manager/src/spec_files_blessed.rs b/crates/dropshot-api-manager/src/spec_files_blessed.rs index 9570aa1..aef6466 100644 --- a/crates/dropshot-api-manager/src/spec_files_blessed.rs +++ b/crates/dropshot-api-manager/src/spec_files_blessed.rs @@ -42,7 +42,7 @@ NewtypeFrom! { () pub struct BlessedApiSpecFile(ApiSpecFile); } impl ApiLoad for BlessedApiSpecFile { const MISCONFIGURATIONS_ALLOWED: bool = true; - const UNPARSEABLE_FILES_ALLOWED: bool = false; + type Unparseable = std::convert::Infallible; fn make_item(raw: ApiSpecFile) -> Self { BlessedApiSpecFile(raw) @@ -58,25 +58,19 @@ impl ApiLoad for BlessedApiSpecFile { ); } - fn make_unparseable_item( + fn make_unparseable( _name: ApiSpecFileName, _contents: Vec, - ) -> Self { - panic!( - "make_unparseable_item called on BlessedApiSpecFile, but \ - UNPARSEABLE_FILES_ALLOWED is false" - ); + ) -> Option { + None } - fn try_extend_unparseable( - &mut self, - _name: ApiSpecFileName, - _contents: Vec, - ) { - panic!( - "try_extend_unparseable called on BlessedApiSpecFile, but \ - UNPARSEABLE_FILES_ALLOWED is false" - ); + fn unparseable_into_self(unparseable: Self::Unparseable) -> Self { + match unparseable {} + } + + fn extend_unparseable(&mut self, unparseable: Self::Unparseable) { + match unparseable {} } } diff --git a/crates/dropshot-api-manager/src/spec_files_generated.rs b/crates/dropshot-api-manager/src/spec_files_generated.rs index 51a34d5..e976c00 100644 --- a/crates/dropshot-api-manager/src/spec_files_generated.rs +++ b/crates/dropshot-api-manager/src/spec_files_generated.rs @@ -35,7 +35,7 @@ NewtypeFrom! { () pub struct GeneratedApiSpecFile(ApiSpecFile); } impl ApiLoad for GeneratedApiSpecFile { const MISCONFIGURATIONS_ALLOWED: bool = false; - const UNPARSEABLE_FILES_ALLOWED: bool = false; + type Unparseable = std::convert::Infallible; fn make_item(raw: ApiSpecFile) -> Self { GeneratedApiSpecFile(raw) @@ -51,25 +51,19 @@ impl ApiLoad for GeneratedApiSpecFile { ); } - fn make_unparseable_item( + fn make_unparseable( _name: ApiSpecFileName, _contents: Vec, - ) -> Self { - panic!( - "make_unparseable_item called on GeneratedApiSpecFile, but \ - UNPARSEABLE_FILES_ALLOWED is false" - ); + ) -> Option { + None } - fn try_extend_unparseable( - &mut self, - _name: ApiSpecFileName, - _contents: Vec, - ) { - panic!( - "try_extend_unparseable called on GeneratedApiSpecFile, but \ - UNPARSEABLE_FILES_ALLOWED is false" - ); + fn unparseable_into_self(unparseable: Self::Unparseable) -> Self { + match unparseable {} + } + + fn extend_unparseable(&mut self, unparseable: Self::Unparseable) { + match unparseable {} } } diff --git a/crates/dropshot-api-manager/src/spec_files_generic.rs b/crates/dropshot-api-manager/src/spec_files_generic.rs index c9d3d74..7dc8d3f 100644 --- a/crates/dropshot-api-manager/src/spec_files_generic.rs +++ b/crates/dropshot-api-manager/src/spec_files_generic.rs @@ -710,53 +710,50 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiSpecFilesBuilder<'a, T> { }; } Err((error, contents)) => { - if T::UNPARSEABLE_FILES_ALLOWED { - // For local files, track the unparseable file so it can - // be cleaned up during generate. Record a warning so the - // user knows about it. - self.load_warning( - error.context("skipping unparseable file"), - ); + match T::make_unparseable(file_name.clone(), contents) { + Some(unparseable) => { + // For local files, track the unparseable file so it + // can be cleaned up during generate. Record a warning + // so the user knows about it. + self.load_warning( + error.context("skipping unparseable file"), + ); - // Try to track the unparseable file per version if the type - // supports it. - if let Some(version) = file_name.version() { - let ident = file_name.ident().clone(); - let entry = self - .spec_files - .entry(ident) - .or_insert_with(ApiFiles::new) - .spec_files - .entry(version.clone()); - - match entry { - Entry::Vacant(vacant_entry) => { - let unparseable_item = T::make_unparseable_item( - file_name.clone(), - contents, - ); - vacant_entry.insert(unparseable_item); - } - Entry::Occupied(mut occupied_entry) => { - occupied_entry - .get_mut() - .try_extend_unparseable( - file_name.clone(), - contents, + // Can the file be associated with a version? + if let Some(version) = file_name.version() { + let ident = file_name.ident().clone(); + let entry = self + .spec_files + .entry(ident) + .or_insert_with(ApiFiles::new) + .spec_files + .entry(version.clone()); + + match entry { + Entry::Vacant(vacant_entry) => { + vacant_entry.insert( + T::unparseable_into_self(unparseable), ); + } + Entry::Occupied(mut occupied_entry) => { + occupied_entry + .get_mut() + .extend_unparseable(unparseable); + } } + } else { + // No version info, fall back to old behavior. + self.record_unparseable_file( + file_name.ident().clone(), + UnparseableFile { + path: file_name.path().to_owned(), + }, + ); } - } else { - // No version info, fall back to old behavior. - self.record_unparseable_file( - file_name.ident().clone(), - UnparseableFile { - path: file_name.path().to_owned(), - }, - ); } - } else { - self.load_error(error); + None => { + self.load_error(error); + } } } } @@ -879,7 +876,7 @@ pub trait SpecFileInfo { /// For unparseable files, this returns `None`. Use /// `spec_file_name().version()` to get the version from the filename /// instead. - fn parsed_version(&self) -> Option<&semver::Version>; + fn version(&self) -> Option<&semver::Version>; } impl SpecFileInfo for ApiSpecFile { @@ -887,7 +884,7 @@ impl SpecFileInfo for ApiSpecFile { &self.name } - fn parsed_version(&self) -> Option<&semver::Version> { + fn version(&self) -> Option<&semver::Version> { Some(&self.version) } } @@ -933,15 +930,12 @@ pub trait ApiLoad { /// necessary in order to convert an API from lockstep to versioned. const MISCONFIGURATIONS_ALLOWED: bool; - /// Determines whether unparseable files are allowed in this context. + /// The type representing unparseable file data. /// - /// When `true`, files that cannot be parsed (e.g., due to merge conflict - /// markers) are recorded as warnings and tracked for cleanup during - /// generate. When `false`, such files are treated as errors. - /// - /// This is `true` for local files (allowing generate to clean them up) - /// and `false` for blessed files (which should always be valid). - const UNPARSEABLE_FILES_ALLOWED: bool; + /// For contexts in which unparseable files are allowed (local files), this + /// is a concrete type holding the filename and contents. Otherwise, this is + /// `std::convert::Infallible`, making it impossible to construct. + type Unparseable; /// Record having loaded a single OpenAPI document for an API. fn make_item(raw: ApiSpecFile) -> Self; @@ -953,31 +947,25 @@ pub trait ApiLoad { /// some allow more than one.) fn try_extend(&mut self, raw: ApiSpecFile) -> anyhow::Result<()>; - /// Record an unparseable file for an API. - /// - /// The `contents` parameter contains the raw bytes of the file that - /// couldn't be parsed. This allows the contents to still be accessed even - /// though parsing failed. + /// Try to create unparseable file data. /// - /// # Panics + /// Returns `Some` with the unparseable data if unparseable files are + /// allowed in this context, `None` otherwise. When `Self::Unparseable` is + /// `Infallible`, this always returns `None`. + fn make_unparseable( + name: ApiSpecFileName, + contents: Vec, + ) -> Option; + + /// Convert unparseable file data into a `Self` for insertion. /// - /// Panics if `UNPARSEABLE_FILES_ALLOWED` is `false`. The caller must check - /// `UNPARSEABLE_FILES_ALLOWED` before calling this method. - fn make_unparseable_item(name: ApiSpecFileName, contents: Vec) -> Self + /// This is used when inserting into a vacant entry. + fn unparseable_into_self(unparseable: Self::Unparseable) -> Self where Self: Sized; - /// Try to add an unparseable file to an existing entry. - /// - /// # Panics - /// - /// Panics if `UNPARSEABLE_FILES_ALLOWED` is `false`. The caller must check - /// `UNPARSEABLE_FILES_ALLOWED` before calling this method. - fn try_extend_unparseable( - &mut self, - name: ApiSpecFileName, - contents: Vec, - ); + /// Add unparseable file data to an existing entry. + fn extend_unparseable(&mut self, unparseable: Self::Unparseable); } /// Return the hash of an OpenAPI document file for the purposes of this tool diff --git a/crates/dropshot-api-manager/src/spec_files_local.rs b/crates/dropshot-api-manager/src/spec_files_local.rs index 34366af..9c4339c 100644 --- a/crates/dropshot-api-manager/src/spec_files_local.rs +++ b/crates/dropshot-api-manager/src/spec_files_local.rs @@ -17,6 +17,22 @@ use camino::Utf8Path; use dropshot_api_manager_types::{ApiIdent, ApiSpecFileName}; use std::collections::BTreeMap; +/// A local file that exists but couldn't be parsed. +/// +/// This happens when a file has merge conflict markers or is otherwise +/// corrupted. The version is determined from the filename, allowing the +/// generate command to regenerate the correct contents. +/// +/// We still store the raw contents so they can be accessed if needed (e.g., +/// for diffing or debugging). +#[derive(Debug)] +pub struct LocalApiUnparseable { + /// The parsed filename (contains version and hash info). + pub name: ApiSpecFileName, + /// The raw file contents that couldn't be parsed. + pub contents: Vec, +} + /// Represents an OpenAPI document found in this working tree. /// /// This includes documents for lockstep APIs and versioned APIs, for both @@ -30,27 +46,15 @@ pub enum LocalApiSpecFile { /// A valid, successfully parsed OpenAPI document. Valid(Box), /// A file that exists but couldn't be parsed. - /// - /// This happens when a file has merge conflict markers or is otherwise - /// corrupted. The version is known from the filename, allowing the - /// generate command to regenerate the correct content. - /// - /// We still store the raw contents so they can be accessed if needed - /// (e.g., for diffing or debugging). - Unparseable { - /// The parsed filename (contains version and hash info). - name: ApiSpecFileName, - /// The raw file contents that couldn't be parsed. - contents: Vec, - }, + Unparseable(LocalApiUnparseable), } impl LocalApiSpecFile { /// Returns the spec file name. pub fn spec_file_name(&self) -> &ApiSpecFileName { match self { - LocalApiSpecFile::Valid(spec) => spec.spec_file_name(), - LocalApiSpecFile::Unparseable { name, .. } => name, + Self::Valid(spec) => spec.spec_file_name(), + Self::Unparseable(u) => &u.name, } } @@ -59,14 +63,14 @@ impl LocalApiSpecFile { /// This works for both valid and unparseable files. pub fn contents(&self) -> &[u8] { match self { - LocalApiSpecFile::Valid(spec) => spec.contents(), - LocalApiSpecFile::Unparseable { contents, .. } => contents, + Self::Valid(spec) => spec.contents(), + Self::Unparseable(u) => &u.contents, } } /// Returns true if this file is unparseable. pub fn is_unparseable(&self) -> bool { - matches!(self, LocalApiSpecFile::Unparseable { .. }) + matches!(self, Self::Unparseable(_)) } } @@ -75,10 +79,10 @@ impl SpecFileInfo for LocalApiSpecFile { self.spec_file_name() } - fn parsed_version(&self) -> Option<&semver::Version> { + fn version(&self) -> Option<&semver::Version> { match self { - LocalApiSpecFile::Valid(spec) => Some(spec.version()), - LocalApiSpecFile::Unparseable { .. } => None, + Self::Valid(spec) => Some(spec.version()), + Self::Unparseable(_) => None, } } } @@ -90,7 +94,7 @@ impl SpecFileInfo for LocalApiSpecFile { impl ApiLoad for Vec { const MISCONFIGURATIONS_ALLOWED: bool = false; - const UNPARSEABLE_FILES_ALLOWED: bool = true; + type Unparseable = LocalApiUnparseable; fn try_extend(&mut self, item: ApiSpecFile) -> anyhow::Result<()> { self.push(LocalApiSpecFile::Valid(Box::new(item))); @@ -101,16 +105,19 @@ impl ApiLoad for Vec { vec![LocalApiSpecFile::Valid(Box::new(raw))] } - fn make_unparseable_item(name: ApiSpecFileName, contents: Vec) -> Self { - vec![LocalApiSpecFile::Unparseable { name, contents }] - } - - fn try_extend_unparseable( - &mut self, + fn make_unparseable( name: ApiSpecFileName, contents: Vec, - ) { - self.push(LocalApiSpecFile::Unparseable { name, contents }); + ) -> Option { + Some(LocalApiUnparseable { name, contents }) + } + + fn unparseable_into_self(unparseable: Self::Unparseable) -> Self { + vec![LocalApiSpecFile::Unparseable(unparseable)] + } + + fn extend_unparseable(&mut self, unparseable: Self::Unparseable) { + self.push(LocalApiSpecFile::Unparseable(unparseable)); } }