diff --git a/CHANGELOG.md b/CHANGELOG.md index 1185514..e996493 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ### Fixed - Previously, a `.json.gitstub` file whose filename hash didn't match its resolved contents (e.g. leftover from a mismerge) crashed `generate` and `check`. These files are now detected and cleaned up like other stale files. +- Previously, a valid lockstep document whose `info.version` didn't match the version declared in Rust (e.g. from bumping the version in Rust) crashed the API manager. The document is now reported as stale, and `generate` regenerates it. ### Changed diff --git a/crates/dropshot-api-manager/src/doc_files_generic.rs b/crates/dropshot-api-manager/src/doc_files_generic.rs index 8dba59d..86e0092 100644 --- a/crates/dropshot-api-manager/src/doc_files_generic.rs +++ b/crates/dropshot-api-manager/src/doc_files_generic.rs @@ -203,6 +203,14 @@ pub enum ApiDocFileParseError { filename" )] VersionMismatch { file_version: semver::Version }, + #[error( + "version in the file ({file_version}) differs from the declared \ + lockstep version ({declared_version})" + )] + LockstepVersionMismatch { + file_version: semver::Version, + declared_version: semver::Version, + }, #[error( "computed hash {expected:?}, but file name has different hash \ {actual:?}" @@ -379,6 +387,11 @@ impl ApiDocFile { pub fn contents(&self) -> &[u8] { &self.contents_buf } + + /// Consumes self, returning the name and raw contents. + pub(crate) fn into_name_and_contents(self) -> (ApiDocFileName, Vec) { + (self.name, self.contents_buf.0) + } } /// Builder for constructing a set of found OpenAPI documents @@ -659,26 +672,63 @@ impl<'a, T: ApiLoad + AsRawFiles> ApiDocFilesBuilder<'a, T> { /// Load an already-parsed API document. pub fn load_parsed(&mut self, file: ApiDocFile) { - let ident = file.doc_file_name().ident(); - let api_version = file.version(); - let entry = self - .doc_files - .entry(ident.clone()) - .or_insert_with(ApiFiles::new) - .doc_files - .entry(api_version.clone()); - - match entry { - Entry::Vacant(vacant_entry) => { - vacant_entry.insert(T::make_item(file)); + let version_mismatch = match file.doc_file_name() { + ApiDocFileName::Lockstep(_) => { + // Lockstep file names don't carry a version, so check against + // the version specified in Rust. + let api = self + .apis + .api(file.doc_file_name().ident()) + .expect("parsed lockstep file name implies a known API"); + let declared_version = api + .iter_versions_semver() + .next() + .expect("lockstep API has exactly one version"); + if file.version() != declared_version { + Some(ApiDocFileParseError::LockstepVersionMismatch { + file_version: file.version().clone(), + declared_version: declared_version.clone(), + }) + } else { + None + } + } + // For versioned files, the version inside the document + // (info.version) is known to match the version in the file name, + // since ApiDocFile::for_contents rejects documents where the two + // aren't the same. So we don't need to do any additional checks + // here. + ApiDocFileName::Versioned(_) => None, + }; + + match version_mismatch { + Some(reason) => { + let (name, contents) = file.into_name_and_contents(); + self.insert_unparseable(name, contents, reason.into()); } - Entry::Occupied(mut occupied_entry) => { - match occupied_entry.get_mut().try_extend(file) { - Ok(()) => (), - Err(error) => self.load_error(error), + None => { + let ident = file.doc_file_name().ident(); + let api_version = file.version(); + let entry = self + .doc_files + .entry(ident.clone()) + .or_insert_with(ApiFiles::new) + .doc_files + .entry(api_version.clone()); + + match entry { + Entry::Vacant(vacant_entry) => { + vacant_entry.insert(T::make_item(file)); + } + Entry::Occupied(mut occupied_entry) => { + match occupied_entry.get_mut().try_extend(file) { + Ok(()) => (), + Err(error) => self.load_error(error), + }; + } }; } - }; + } } /// Load an API document that may or may not have parsed successfully. diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index 04c79e3..b450f93 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -1385,8 +1385,9 @@ fn resolve_orphaned_local_docs<'a>( supported_versions_by_api: &BTreeMap<&ApiIdent, BTreeSet<&semver::Version>>, local: &'a LocalFiles, ) -> impl Iterator { - // Orphaned documents are always versioned: lockstep APIs have exactly one - // file, so orphans can't exist for them. + // Orphaned documents are always versioned, because lockstep APIs are always + // keyed under the API's declared version. This is enforced by load_parsed + // in doc_files_generic.rs. local.iter().flat_map(|(ident, api_files)| { let set = supported_versions_by_api.get(ident); api_files diff --git a/crates/integration-tests/tests/integration/lockstep.rs b/crates/integration-tests/tests/integration/lockstep.rs index cd62f96..6a685cb 100644 --- a/crates/integration-tests/tests/integration/lockstep.rs +++ b/crates/integration-tests/tests/integration/lockstep.rs @@ -184,3 +184,53 @@ fn test_unparseable_conflict_markers() -> Result<()> { Ok(()) } + +// Test version mismatches in lockstep files. +#[test] +fn test_lockstep_version_mismatch() -> Result<()> { + let env = TestEnvironment::new_git()?; + let apis = lockstep_health_apis()?; + + env.generate_documents(&apis)?; + env.commit_documents()?; + + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::Success); + + let document_content = env.read_lockstep_document("health")?; + let modified_content = document_content.replacen( + "\"version\": \"1.0.0\"", + "\"version\": \"9.9.9\"", + 1, + ); + assert_ne!( + document_content, modified_content, + "replaced the version string in the document" + ); + env.create_file("documents/health.json", &modified_content)?; + + let (result, summaries, rendered) = + check_apis_with_render(env.environment(), &apis)?; + assert_eq!(result, CheckResult::NeedsUpdate); + crate::snapshot::assert_render_snapshot( + &env, + "lockstep_version_mismatch.txt", + &rendered, + ); + assert_eq!( + summaries, + [ProblemSummary::new("health", "1.0.0", ProblemKind::LockstepStale)], + ); + + env.generate_documents(&apis)?; + + let result = check_apis_up_to_date(env.environment(), &apis)?; + assert_eq!(result, CheckResult::Success); + + let document_content = env.read_lockstep_document("health")?; + let parsed: OpenAPI = serde_json::from_str(&document_content) + .expect("regenerated document is valid JSON"); + assert_eq!(parsed.info.version, "1.0.0"); + + Ok(()) +} diff --git a/crates/integration-tests/tests/output/integration/lockstep_version_mismatch.txt b/crates/integration-tests/tests/output/integration/lockstep_version_mismatch.txt new file mode 100644 index 0000000..2e2334e --- /dev/null +++ b/crates/integration-tests/tests/output/integration/lockstep_version_mismatch.txt @@ -0,0 +1,27 @@ + ------- + Generating OpenAPI documents from API definitions ... + Loading local OpenAPI documents from "" ... + Loading blessed OpenAPI documents from VCS revision "main" path "documents" + ------- + Checking 1 OpenAPI document... + Stale health (lockstep v1.0.0): Health API + problem: For this lockstep API, the local file could not be loaded: + health.json (version in the file (9.9.9) differs from the declared + lockstep version (1.0.0)). This tool can regenerate the file + for you. + fix: will rewrite lockstep file health.json from generated + --- a//health.json + +++ b//health.json + @@ -3,7 +3,7 @@ + "info": { + "title": "Health API", + "description": "A health API for testing schema evolution", + - "version": "9.9.9" + + "version": "1.0.0" + }, + "paths": { + "/health": { + + ------- + Stale 1 document checked: 0 fresh, 1 stale, 0 failed, 0 other problems + (run test-openapi-manager generate to update)