support extra validation for versioned APIs (#25)

Include information about:

* whether this is the latest version, for use with determining whether
extra files should be written out.
* whether this is a blessed version, in which case users may want to
skip part or all of validation.

Author: sunshowers

crates/dropshot-api-manager-types/src/validation.rs

@@ -25,10 +25,29 @@ impl<'a> ValidationContext<'a> {
     }
 
     /// Returns a descriptor for the API's file name.
+    ///
+    /// The file name can be used to identify the version of the API being
+    /// validated.
     pub fn file_name(&self) -> &ApiSpecFileName {
         self.backend.file_name()
     }
 
+    /// Returns true if this is the latest version of a versioned API, or if the
+    /// API is lockstep.
+    ///
+    /// This is particularly useful for extra files which might not themselves
+    /// be versioned. In that case, you may wish to only generate the extra file
+    /// for the latest version.
+    pub fn is_latest(&self) -> bool {
+        self.backend.is_latest()
+    }
+
+    /// Returns whether this version is blessed, or None if this is not a
+    /// versioned API.
+    pub fn is_blessed(&self) -> Option<bool> {
+        self.backend.is_blessed()
+    }
+
     /// Retrieves the versioning strategy for this API.
     pub fn versions(&self) -> &Versions {
         self.backend.versions()
@@ -73,6 +92,8 @@ pub trait ValidationBackend {
     fn ident(&self) -> &ApiIdent;
     fn file_name(&self) -> &ApiSpecFileName;
     fn versions(&self) -> &Versions;
+    fn is_latest(&self) -> bool;
+    fn is_blessed(&self) -> Option<bool>;
     fn title(&self) -> &str;
     fn metadata(&self) -> &ManagedApiMetadata;
     fn report_error(&mut self, error: anyhow::Error);

crates/dropshot-api-manager-types/src/versions.rs

@@ -166,6 +166,18 @@ impl<'a> Iterator for IterVersionsSemvers<'a> {
     }
 }
 
+impl<'a> ExactSizeIterator for IterVersionsSemvers<'a> {
+    fn len(&self) -> usize {
+        self.inner.len()
+    }
+}
+
+impl<'a> DoubleEndedIterator for IterVersionsSemvers<'a> {
+    fn next_back(&mut self) -> Option<Self::Item> {
+        self.inner.next_back()
+    }
+}
+
 #[derive(Debug)]
 enum IterVersionsSemversInner<'a> {
     Lockstep(Option<&'a semver::Version>),
@@ -200,6 +212,17 @@ impl<'a> ExactSizeIterator for IterVersionsSemversInner<'a> {
     }
 }
 
+impl<'a> DoubleEndedIterator for IterVersionsSemversInner<'a> {
+    fn next_back(&mut self) -> Option<Self::Item> {
+        match self {
+            IterVersionsSemversInner::Lockstep(version) => version.take(),
+            IterVersionsSemversInner::Versioned(versions) => {
+                versions.next_back().map(|v| &v.semver)
+            }
+        }
+    }
+}
+
 /// Helper macro used to define API versions.
 ///
 /// ```

crates/dropshot-api-manager/README.md

@@ -346,3 +346,9 @@ An existing lockstep API can be made versioned.  You would do this when transiti
 That should be it!  Now, when iterating on the API, you'll need to follow the procedure described above for versioned APIs (which is slightly more complicated than the one for lockstep APIs).
 
 In principle, this process could be reversed to convert an API from versioned to lockstep, but this almost certainly has runtime implications that would need to be considered.
+
+## Contributing
+
+Bugfixes and other minor fixes are welcome! Before working on a major feature, please [open an issue](https://github.com/oxidecomputer/dropshot-api-manager/issues/new) to discuss it.
+
+Tests must be run using [cargo-nextest](https://nexte.st/).

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

@@ -3,7 +3,8 @@
 use anyhow::{Context, bail};
 use dropshot::{ApiDescription, ApiDescriptionBuildErrors, StubContext};
 use dropshot_api_manager_types::{
-    ApiIdent, ManagedApiMetadata, SupportedVersion, ValidationContext, Versions,
+    ApiIdent, IterVersionsSemvers, ManagedApiMetadata, SupportedVersion,
+    ValidationContext, Versions,
 };
 use openapiv3::OpenAPI;
 use std::collections::{BTreeMap, BTreeSet};
@@ -31,12 +32,17 @@ pub struct ManagedApiConfig {
     /// The API description function, typically a reference to
     /// `stub_api_description`
     ///
-    /// This is used to generate the OpenAPI spec that matches the current
+    /// This is used to generate the OpenAPI document that matches the current
     /// server implementation.
     pub api_description:
         fn() -> Result<ApiDescription<StubContext>, ApiDescriptionBuildErrors>,
 
-    /// Extra validation to perform on the OpenAPI spec, if any.
+    /// Extra validation to perform on the OpenAPI document, if any.
+    ///
+    /// For versioned APIs, extra validation is performed on *all* versions,
+    /// including blessed ones. You may want to skip performing validation on
+    /// blessed versions, though, because they're immutable. To do so, use
+    /// [`ValidationContext::is_blessed`].
     pub extra_validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
 }
 
@@ -60,12 +66,12 @@ pub(crate) struct ManagedApi {
     /// The API description function, typically a reference to
     /// `stub_api_description`
     ///
-    /// This is used to generate the OpenAPI spec that matches the current
+    /// This is used to generate the OpenAPI document that matches the current
     /// server implementation.
     api_description:
         fn() -> Result<ApiDescription<StubContext>, ApiDescriptionBuildErrors>,
 
-    /// Extra validation to perform on the OpenAPI spec, if any.
+    /// Extra validation to perform on the OpenAPI document, if any.
     extra_validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
 }
 
@@ -113,9 +119,7 @@ impl ManagedApi {
         self.versions.iter_versioned_versions()
     }
 
-    pub fn iter_versions_semver(
-        &self,
-    ) -> impl Iterator<Item = &semver::Version> + '_ {
+    pub fn iter_versions_semver(&self) -> IterVersionsSemvers<'_> {
         self.versions.iter_versions_semvers()
     }
 
@@ -192,20 +196,6 @@ impl ManagedApis {
         let mut apis = BTreeMap::new();
         for api in api_list {
             let api = ManagedApi::from(api);
-            if api.extra_validation.is_some() && api.is_versioned() {
-                // Extra validation is not yet supported for versioned APIs.
-                // The reason is that extra validation can instruct this tool to
-                // check the contents of additional files (e.g.,
-                // nexus_tags.txt).  We'd need to figure out if we want to
-                // maintain expected output for each supported version, only
-                // check the latest version, or what.  (Since this is currenty
-                // only used for nexus_tags, it would probably be okay to just
-                // check the latest version.)  Rather than deal with any of
-                // this, we punt for now.  We can revisit this if/when it comes
-                // up.
-                bail!("extra validation is not supported for versioned APIs");
-            }
-
             if let Some(old) = apis.insert(api.ident.clone(), api) {
                 bail!("API is defined twice: {:?}", &old.ident);
             }

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

@@ -423,10 +423,13 @@ impl Fix<'_> {
                     CheckStale::Modified { expected, .. } => expected,
                     CheckStale::New { expected } => expected,
                 };
+                // Extra file paths are relative to the repo root, not the
+                // documents directory.
+                let full_path = env.repo_root.join(path);
                 Ok(vec![format!(
                     "wrote {}: {:?}",
                     &path,
-                    overwrite_file(path, expected_contents)?
+                    overwrite_file(&full_path, expected_contents)?
                 )])
             }
             Fix::UpdateSymlink { api_ident, link } => {
@@ -652,18 +655,32 @@ fn resolve_api<'a>(
     } else {
         let by_version: BTreeMap<_, _> = api
             .iter_versions_semver()
-            .map(|version| {
+            // Reverse the order of versions: they are stored in sorted order,
+            // so the last version (first one from the back) is the latest.
+            .rev()
+            .enumerate()
+            .map(|(index, version)| {
+                let is_latest = index == 0;
                 let version = version.clone();
                 let blessed =
                     api_blessed.and_then(|b| b.versions().get(&version));
+                let is_blessed = Some(blessed.is_some());
                 let generated = api_generated.versions().get(&version).unwrap();
                 let local = api_local
                     .and_then(|b| b.versions().get(&version))
                     .map(|v| v.as_slice())
                     .unwrap_or(&[]);
+
                 let resolution = resolve_api_version(
-                    env, api, validation, &version, blessed, generated, local,
+                    env,
+                    api,
+                    validation,
+                    ApiVersion { version: &version, is_latest, is_blessed },
+                    blessed,
+                    generated,
+                    local,
                 );
+
                 (version, resolution)
             })
             .collect();
@@ -869,7 +886,18 @@ fn resolve_api_lockstep<'a>(
     let mut problems = Vec::new();
 
     // Validate the generated API document.
-    validate_generated(env, api, validation, version, generated, &mut problems);
+    validate_generated(
+        env,
+        api,
+        validation,
+        ApiVersion {
+            version,
+            is_latest: true, // is_latest is always true for lockstep APIs
+            is_blessed: None,
+        },
+        generated,
+        &mut problems,
+    );
 
     match local {
         Some(local_file) if local_file.contents() == generated.contents() => (),
@@ -882,11 +910,17 @@ fn resolve_api_lockstep<'a>(
     BTreeMap::from([(version.clone(), Resolution::new_lockstep(problems))])
 }
 
+struct ApiVersion<'a> {
+    version: &'a semver::Version,
+    is_latest: bool,
+    is_blessed: Option<bool>,
+}
+
 fn resolve_api_version<'a>(
     env: &'_ ResolvedEnv,
     api: &'_ ManagedApi,
     validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
-    version: &'_ semver::Version,
+    version: ApiVersion<'_>,
     blessed: Option<&'a BlessedApiSpecFile>,
     generated: &'a GeneratedApiSpecFile,
     local: &'a [LocalApiSpecFile],
@@ -905,14 +939,21 @@ fn resolve_api_version_blessed<'a>(
     env: &'_ ResolvedEnv,
     api: &'_ ManagedApi,
     validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
-    version: &'_ semver::Version,
+    version: ApiVersion<'_>,
     blessed: &'a BlessedApiSpecFile,
     generated: &'a GeneratedApiSpecFile,
     local: &'a [LocalApiSpecFile],
 ) -> Resolution<'a> {
     let mut problems = Vec::new();
 
     // Validate the generated API document.
+    //
+    // Blessed versions are immutable, so why do we call validation on them in a
+    // way that can fail? The reason is that validation may also want to
+    // generate extra files, particularly for the latest version. Whether or not
+    // the API version is blessed, the user might still want to generate extra
+    // files for that version. So we validate unconditionally, but let the user
+    // know via `is_blessed`, letting them skip validation where appropriate.
     validate_generated(env, api, validation, version, generated, &mut problems);
 
     // First off, the blessed spec must be a subset of the generated one.
@@ -980,13 +1021,13 @@ fn resolve_api_version_local<'a>(
     env: &'_ ResolvedEnv,
     api: &'_ ManagedApi,
     validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
-    version: &'_ semver::Version,
+    version: ApiVersion<'_>,
     generated: &'a GeneratedApiSpecFile,
     local: &'a [LocalApiSpecFile],
 ) -> Resolution<'a> {
     let mut problems = Vec::new();
 
-    // Validate the generated API document.
+    // alidate the generated API document.
     validate_generated(env, api, validation, version, generated, &mut problems);
 
     let (matching, non_matching): (Vec<_>, Vec<_>) = local
@@ -1021,15 +1062,22 @@ fn validate_generated(
     env: &ResolvedEnv,
     api: &ManagedApi,
     validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
-    version: &semver::Version,
+    version: ApiVersion<'_>,
     generated: &GeneratedApiSpecFile,
     problems: &mut Vec<Problem<'_>>,
 ) {
-    match validate(env, api, validation, generated) {
+    match validate(
+        env,
+        api,
+        version.is_latest,
+        version.is_blessed,
+        validation,
+        generated,
+    ) {
         Err(source) => {
             problems.push(Problem::GeneratedValidationError {
                 api_ident: api.ident().clone(),
-                version: version.clone(),
+                version: version.version.clone(),
                 source,
             });
         }

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

@@ -17,6 +17,8 @@ use std::io::Write;
 pub fn validate(
     env: &ResolvedEnv,
     api: &ManagedApi,
+    is_latest: bool,
+    is_blessed: Option<bool>,
     validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
     generated: &GeneratedApiSpecFile,
 ) -> anyhow::Result<Vec<(Utf8PathBuf, CheckStatus)>> {
@@ -25,6 +27,8 @@ pub fn validate(
         api,
         openapi,
         generated.spec_file_name(),
+        is_latest,
+        is_blessed,
         validation,
     )?;
     let extra_files = validation_result
@@ -43,12 +47,16 @@ fn validate_generated_openapi_document(
     api: &ManagedApi,
     openapi_doc: &OpenAPI,
     file_name: &ApiSpecFileName,
+    is_latest: bool,
+    is_blessed: Option<bool>,
     validation: Option<fn(&OpenAPI, ValidationContext<'_>)>,
 ) -> anyhow::Result<ValidationResult> {
     let mut validation_context = ValidationContextImpl {
         ident: api.ident().clone(),
         file_name: file_name.clone(),
         versions: api.versions().clone(),
+        is_latest,
+        is_blessed,
         title: api.title(),
         metadata: api.metadata().clone(),
         errors: Vec::new(),
@@ -182,6 +190,8 @@ struct ValidationContextImpl {
     ident: ApiIdent,
     file_name: ApiSpecFileName,
     versions: Versions,
+    is_latest: bool,
+    is_blessed: Option<bool>,
     title: &'static str,
     metadata: ManagedApiMetadata,
     errors: Vec<anyhow::Error>,
@@ -201,6 +211,14 @@ impl ValidationBackend for ValidationContextImpl {
         &self.versions
     }
 
+    fn is_latest(&self) -> bool {
+        self.is_latest
+    }
+
+    fn is_blessed(&self) -> Option<bool> {
+        self.is_blessed
+    }
+
     fn title(&self) -> &str {
         self.title
     }