[dropshot-api-manager] fix panic on lockstep document version bumps

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
 

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<u8>) {
+        (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.

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<Item = &'a LocalApiDocFile> {
-    // 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

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(())
+}

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 "<documents dir>" ... 
+     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/<documents dir>/health.json
+             +++ b/<documents dir>/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)