compare latest blessed version for bytewise equality (#36)

The Dropshot API manager detects semantic equality using
[drift](https://docs.rs/drift). This was primarily intended so that
wire-compatible changes could be made to endpoint handlers for old
blessed versions. But, as a side-effect, this property also applies to
the latest blessed version. This means that trivial changes to blessed
versions can accumulate, up to the point at which a non-trivial change
happens, and then the diff becomes quite confusing.

Address this by special-casing the latest blessed version, and requiring
both bytewise and semantic equality for it. Also allow
client-side-versioned APIs (which are currently frozen; see
oxidecomputer/omicron#9290) to opt out of this
scheme.

Closes #35.

Author: sunshowers

crates/dropshot-api-manager/src/apis.rs

@@ -46,9 +46,13 @@ pub struct ManagedApiConfig {
     pub extra_validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
 }
 
-/// Used internally to describe an API managed by this tool.
+/// Describes an API managed by the Dropshot API manager.
+///
+/// This type is typically created from a [`ManagedApiConfig`] and can be
+/// further configured using builder methods before being passed to
+/// [`ManagedApis::new`].
 #[derive(Debug)]
-pub(crate) struct ManagedApi {
+pub struct ManagedApi {
     /// The API-specific part of the filename that's used for API descriptions
     ///
     /// This string is sometimes used as an identifier for developers.
@@ -73,6 +77,12 @@ pub(crate) struct ManagedApi {
 
     /// Extra validation to perform on the OpenAPI document, if any.
     extra_validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
+
+    /// If true, allow trivial changes (doc updates, type renames) for the
+    /// latest blessed version without requiring version bumps.
+    ///
+    /// Default: false (bytewise check is performed for latest version).
+    allow_trivial_changes_for_latest: bool,
 }
 
 impl From<ManagedApiConfig> for ManagedApi {
@@ -84,46 +94,70 @@ impl From<ManagedApiConfig> for ManagedApi {
             metadata: value.metadata,
             api_description: value.api_description,
             extra_validation: value.extra_validation,
+            allow_trivial_changes_for_latest: false,
         }
     }
 }
 
 impl ManagedApi {
+    /// Returns the API identifier.
     pub fn ident(&self) -> &ApiIdent {
         &self.ident
     }
 
+    /// Returns the API versions.
     pub fn versions(&self) -> &Versions {
         &self.versions
     }
 
+    /// Returns the API title.
     pub fn title(&self) -> &'static str {
         self.title
     }
 
+    /// Returns the API metadata.
     pub fn metadata(&self) -> &ManagedApiMetadata {
         &self.metadata
     }
 
+    /// Returns true if the API is lockstep.
     pub fn is_lockstep(&self) -> bool {
         self.versions.is_lockstep()
     }
 
+    /// Returns true if the API is versioned.
     pub fn is_versioned(&self) -> bool {
         self.versions.is_versioned()
     }
 
-    pub fn iter_versioned_versions(
+    /// Allows trivial changes (doc updates, type renames) for the latest
+    /// blessed version without requiring a version bump.
+    ///
+    /// By default, the latest blessed version requires bytewise equality
+    /// between blessed and generated documents. This prevents trivial changes
+    /// from accumulating invisibly. Calling this method allows semantic-only
+    /// checking for all versions, including the latest.
+    pub fn allow_trivial_changes_for_latest(mut self) -> Self {
+        self.allow_trivial_changes_for_latest = true;
+        self
+    }
+
+    /// Returns true if trivial changes are allowed for the latest version.
+    pub fn allows_trivial_changes_for_latest(&self) -> bool {
+        self.allow_trivial_changes_for_latest
+    }
+
+    pub(crate) fn iter_versioned_versions(
         &self,
     ) -> Option<impl Iterator<Item = &SupportedVersion> + '_> {
         self.versions.iter_versioned_versions()
     }
 
-    pub fn iter_versions_semver(&self) -> IterVersionsSemvers<'_> {
+    pub(crate) fn iter_versions_semver(&self) -> IterVersionsSemvers<'_> {
         self.versions.iter_versions_semvers()
     }
 
-    pub fn generate_openapi_doc(
+    pub(crate) fn generate_openapi_doc(
         &self,
         version: &semver::Version,
     ) -> anyhow::Result<OpenAPI> {
@@ -136,7 +170,7 @@ impl ManagedApi {
             .context("generated document is not valid OpenAPI")
     }
 
-    pub fn generate_spec_bytes(
+    pub(crate) fn generate_spec_bytes(
         &self,
         version: &semver::Version,
     ) -> anyhow::Result<Vec<u8>> {
@@ -165,7 +199,7 @@ impl ManagedApi {
         Ok(contents)
     }
 
-    pub fn extra_validation(
+    pub(crate) fn extra_validation(
         &self,
         openapi: &OpenAPI,
         validation_context: ValidationContext<'_>,
@@ -192,10 +226,16 @@ impl ManagedApis {
     /// configurations.
     ///
     /// This is the main entry point for creating a new `ManagedApis` instance.
-    pub fn new(api_list: Vec<ManagedApiConfig>) -> anyhow::Result<ManagedApis> {
+    /// Accepts any iterable of items that can be converted into [`ManagedApi`],
+    /// including `Vec<ManagedApiConfig>` and `Vec<ManagedApi>`.
+    pub fn new<I>(api_list: I) -> anyhow::Result<ManagedApis>
+    where
+        I: IntoIterator,
+        I::Item: Into<ManagedApi>,
+    {
         let mut apis = BTreeMap::new();
         for api in api_list {
-            let api = ManagedApi::from(api);
+            let api = api.into();
             if let Some(old) = apis.insert(api.ident.clone(), api) {
                 bail!("API is defined twice: {:?}", &old.ident);
             }

crates/dropshot-api-manager/src/output.rs

@@ -529,6 +529,35 @@ pub fn display_resolution_problems<'a, T>(
             }
         }
 
+        // For BlessedLatestVersionBytewiseMismatch, show a diff between blessed
+        // and generated versions even though there's no fix.
+        if let Problem::BlessedLatestVersionBytewiseMismatch {
+            blessed,
+            generated,
+        } = p
+        {
+            let diff =
+                TextDiff::from_lines(blessed.contents(), generated.contents());
+            let path1 =
+                env.openapi_abs_dir().join(blessed.spec_file_name().path());
+            let path2 =
+                env.openapi_abs_dir().join(generated.spec_file_name().path());
+            let indent = " ".repeat(HEADER_WIDTH + 1);
+            let _ = write_diff(
+                &diff,
+                &path1,
+                &path2,
+                styles,
+                // context_radius: show enough context to understand the changes.
+                3,
+                /* missing_newline_hint */ true,
+                &mut indent_write::io::IndentWriter::new(
+                    &indent,
+                    std::io::stderr(),
+                ),
+            );
+        }
+
         let Some(fix) = p.fix() else {
             continue;
         };

crates/dropshot-api-manager/src/resolved.rs

@@ -176,6 +176,20 @@ pub enum Problem<'a> {
     )]
     BlessedVersionBroken { compatibility_issues: Vec<ApiCompatIssue> },
 
+    #[error(
+        "For the latest blessed version, the OpenAPI document generated from \
+         the current code is wire-compatible but not bytewise \
+         identical to the blessed document. This implies one or more \
+         trivial changes such as type renames or documentation updates. \
+         To proceed, bump the API version in the `api_versions!` macro; \
+         unless you're introducing other changes, there's no need to make \
+         changes to any endpoints."
+    )]
+    BlessedLatestVersionBytewiseMismatch {
+        blessed: &'a BlessedApiSpecFile,
+        generated: &'a GeneratedApiSpecFile,
+    },
+
     #[error(
         "No local OpenAPI document was found for this lockstep API.  This is \
          only expected if you're adding a new lockstep API.  This tool can \
@@ -281,6 +295,7 @@ impl<'a> Problem<'a> {
             }
             Problem::BlessedVersionCompareError { .. } => None,
             Problem::BlessedVersionBroken { .. } => None,
+            Problem::BlessedLatestVersionBytewiseMismatch { .. } => None,
             Problem::LockstepMissingLocal { generated }
             | Problem::LockstepStale { generated, .. } => {
                 Some(Fix::UpdateLockstepFile { generated })
@@ -945,6 +960,7 @@ fn resolve_api_version_blessed<'a>(
     local: &'a [LocalApiSpecFile],
 ) -> Resolution<'a> {
     let mut problems = Vec::new();
+    let is_latest = version.is_latest;
 
     // Validate the generated API document.
     //
@@ -973,6 +989,22 @@ fn resolve_api_version_blessed<'a>(
         }
     };
 
+    // For the latest version, also require bytewise equality. This ensures that
+    // trivial changes don't accumulate invisibly. If the generated spec is
+    // semantically equivalent but bytewise different, require a version bump.
+    //
+    // This check can be disabled via `allow_trivial_changes_for_latest()`.
+    if is_latest
+        && !api.allows_trivial_changes_for_latest()
+        && problems.is_empty()
+        && generated.contents() != blessed.contents()
+    {
+        problems.push(Problem::BlessedLatestVersionBytewiseMismatch {
+            blessed,
+            generated,
+        });
+    }
+
     // Now, there should be at least one local spec that exactly matches the
     // blessed one.
     let (matching, non_matching): (Vec<_>, Vec<_>) =