Migrate tests to access configuration through public interface (#457)

### What

This PR ensures that all test code that relies on NDC configurations
does so via the public interface rather than by directly depending on a
specific configuration version format module.

The tests that have the version format as their subject matter have been
moved into the configuration crate, cohabiting with the code they test.

The tests that don't have the version format as their subject matter now
rely only on the public interface of the configuration crate. The
biggest consequence of this by diff volume is that what used to be
`tables.json` holding a `Metadata` value now becomes full
`configuration.json` files, with their original content in the
`metadata` section and a `version: "3"` tag as well.

Cargo.toml

@@ -2,9 +2,7 @@
 resolver = "2"
 
 package.version = "0.6.0"
-
 package.edition = "2021"
-
 package.license = "Apache-2.0"
 
 members = [
@@ -31,5 +29,6 @@ similar_names = "allow"
 too_many_lines = "allow"
 
 [workspace.dependencies]
+ndc-models = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.2" }
 ndc-sdk = { git = "https://github.com/hasura/ndc-sdk-rs.git", rev = "6158b1adbcc4ac7d8acb721c1626f6f715424a27" }
 ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.2" }

crates/cli/tests/update_tests.rs

@@ -13,28 +13,14 @@ const CONNECTION_URI: &str = "postgresql://postgres:password@localhost:64002";
 async fn test_update_configuration() -> anyhow::Result<()> {
     let dir = tempfile::tempdir()?;
 
-    let connection_uri = configuration::ConnectionUri(configuration::Secret::FromEnvironment {
-        variable: "CONNECTION_URI".into(),
-    });
-
-    {
-        let connection_settings =
-            configuration::version3::connection_settings::DatabaseConnectionSettings {
-                connection_uri: connection_uri.clone(),
-                ..configuration::version3::connection_settings::DatabaseConnectionSettings::empty()
-            };
-        let input = ParsedConfiguration::Version3(configuration::version3::RawConfiguration {
-            connection_settings,
-            ..configuration::version3::RawConfiguration::empty()
-        });
-        configuration::write_parsed_configuration(input, &dir).await?;
-    }
+    configuration::write_parsed_configuration(configuration::ParsedConfiguration::initial(), &dir)
+        .await?;
 
     let environment =
         FixedEnvironment::from([("CONNECTION_URI".into(), CONNECTION_URI.to_string())]);
     let context = Context {
         context_path: dir.path().to_owned(),
-        environment,
+        environment: environment.clone(),
         release_version: None,
     };
     run(Command::Update, context).await?;
@@ -44,18 +30,10 @@ async fn test_update_configuration() -> anyhow::Result<()> {
     let contents = fs::read_to_string(configuration_file_path).await?;
     common::assert_ends_with_newline(&contents);
     let output: ParsedConfiguration = configuration::parse_configuration(&dir).await?;
-    match output {
-        ParsedConfiguration::Version3(configuration::version3::RawConfiguration {
-            connection_settings,
-            metadata,
-            ..
-        }) => {
-            assert_eq!(connection_settings.connection_uri, connection_uri);
-            let some_table_metadata = metadata.tables.0.get("Artist");
-            assert!(some_table_metadata.is_some());
-        }
-        ParsedConfiguration::Version4(_) => panic!("Expected version 3"),
-    }
+    let runtime_config = configuration::make_runtime_configuration(output, environment)?;
+
+    let some_table_metadata = runtime_config.metadata.tables.0.get("Artist");
+    assert!(some_table_metadata.is_some());
 
     Ok(())
 }

crates/configuration/Cargo.toml

@@ -16,5 +16,10 @@ serde = "1.0.200"
 serde_json = { version = "1.0.116", features = ["raw_value"] }
 sqlx = { version = "0.7.4", features = ["json", "postgres", "runtime-tokio-rustls"] }
 thiserror = "1.0.59"
-tokio = "1.37.0"
+tokio = { version = "1.37.0", features = ["full"]}
 tracing = "0.1.40"
+
+[dev-dependencies]
+insta = { version = "1.38.0", features = ["json"]}
+jsonschema = "0.17.1"
+similar-asserts = "1.5.0"

crates/configuration/src/configuration.rs

@@ -12,17 +12,28 @@ use crate::error::{
 use crate::values::{IsolationLevel, PoolSettings};
 use crate::version3;
 use crate::version4;
+use schemars::{gen::SchemaSettings, schema::RootSchema};
 
-/// The parsed connector configuration. This data type is an enum with cases for each supported
-/// version.
+pub fn generate_latest_schema() -> RootSchema {
+    SchemaSettings::openapi3()
+        .into_generator()
+        .into_root_schema_for::<version4::ParsedConfiguration>()
+}
+
+pub const DEFAULT_CONNECTION_URI_VARIABLE: &str = "CONNECTION_URI";
+
+/// The 'ParsedConfiguration' type models the various concrete configuration formats that are
+/// currently supported.
+///
+/// Introducing a breaking configuration format change involves adding a new case to this type.
 ///
-/// It supports various uses:
+/// 'ParsedConfiguration' is used to support serialization and deserialization of an NDC
+/// configuration. It retains all the salient information that constitues an instance of an NDC
+/// deployment, such that 'c = parse_configuration(dir) => { write_parsed_configuration(c, dir2) ;
+/// assert(c == parse_configuration(dir2))}'.
 ///
-/// * It can be turned into a `Configuration`, to be used at runtime.
-/// * It retains all information necessary to produce an equivalent serialized representation.
-/// * It supports updates between versions which may require more detailed information than is
-///   available in a `Configuration` (such as whether a native query was defined inline or in a
-///   file)
+/// Upgrades between different configuration format versions are version-specific functions on
+/// 'ParsedConfiguration' as well.
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum ParsedConfiguration {
     Version3(version3::RawConfiguration),
@@ -35,11 +46,17 @@ impl ParsedConfiguration {
     }
 }
 
-/// A configuration type, tailored to the needs of the query/mutation/explain methods (i.e., those
-/// not to do with configuration management).
+/// The 'Configuration' type collects all the information necessary to serve queries at runtime.
+///
+/// 'ParsedConfiguration' deals with a multitude of different concrete version formats, and each
+/// version is responsible for interpreting its serialized format into the current 'Configuration'.
+/// Values of this type are produced from a 'ParsedConfiguration' using
+/// 'make_runtime_configuration'.
+///
+/// Separating 'ParsedConfiguration' and 'Configuration' simplifies the main query translation
+/// logic by placing the responsibility of dealing with configuration format evolution in
+/// 'ParsedConfiguration.
 ///
-/// This separation also decouples the implementation from things like API versioning concerns
-/// somewhat.
 #[derive(Debug)]
 pub struct Configuration {
     pub metadata: metadata::Metadata,
@@ -78,6 +95,11 @@ pub async fn parse_configuration(
     }
 }
 
+/// Turn a 'ParsedConfiguration' into a into a 'Configuration', such that it may be used in main
+/// NDC business logic.
+///
+/// Each concrete supported version implementation is responsible for interpretation its format
+/// into the runtime configuration.
 pub fn make_runtime_configuration(
     parsed_config: ParsedConfiguration,
     environment: impl Environment,
@@ -99,6 +121,10 @@ pub async fn write_parsed_configuration(
     }
 }
 
+/// Produce an equivalent version of a parsed configuration in the latest supported version.
+///
+/// This is part of the configuration crate API to enable users to upgrade their configurations
+/// mechanically, using the ndc-postgres cli, when new versions are released..
 pub fn upgrade_to_latest_version(parsed_config: ParsedConfiguration) -> ParsedConfiguration {
     match parsed_config {
         ParsedConfiguration::Version3(v) => {

crates/configuration/src/error.rs

@@ -38,6 +38,7 @@ pub struct MultiError(pub Vec<Box<dyn std::error::Error + Send + Sync>>);
 impl Display for MultiError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         for err in &self.0 {
+            "\n".fmt(f)?;
             " * ".fmt(f)?;
             err.fmt(f)?;
             "\n".fmt(f)?;

crates/configuration/src/lib.rs

@@ -3,12 +3,58 @@ mod values;
 
 pub mod environment;
 pub mod error;
-// TODO: make these two private!
-pub mod version3;
-pub mod version4;
+mod version3;
+mod version4;
 
 pub use configuration::{
-    introspect, make_runtime_configuration, parse_configuration, upgrade_to_latest_version,
-    write_parsed_configuration, Configuration, ParsedConfiguration,
+    generate_latest_schema, introspect, make_runtime_configuration, parse_configuration,
+    upgrade_to_latest_version, write_parsed_configuration, Configuration, ParsedConfiguration,
+    DEFAULT_CONNECTION_URI_VARIABLE,
 };
 pub use values::{ConnectionUri, IsolationLevel, PoolSettings, Secret};
+
+#[cfg(test)]
+pub mod tests;
+
+#[cfg(test)]
+pub mod common {
+    use std::path::{Path, PathBuf};
+
+    pub mod postgres {
+
+        pub const CHINOOK_NDC_METADATA_PATH: &str = "static/postgres/v3-chinook-ndc-metadata";
+
+        pub const BROKEN_QUERIES_NDC_METADATA_PATH: &str =
+            "static/postgres/broken-queries-ndc-metadata";
+
+        pub const CONNECTION_URI: &str = "postgresql://postgres:password@localhost:64002";
+
+        pub const EMPTY_CONNECTION_URI: &str =
+            "postgresql://postgres:password@localhost:64002/empty";
+    }
+
+    pub mod citus {
+        pub const CHINOOK_NDC_METADATA_PATH: &str = "static/citus/v3-chinook-ndc-metadata";
+
+        pub const CONNECTION_URI: &str =
+            "postgresql://postgres:password@localhost:64004?sslmode=disable";
+    }
+
+    pub mod cockroach {
+        pub const CHINOOK_NDC_METADATA_PATH: &str = "static/cockroach/v3-chinook-ndc-metadata";
+
+        pub const CONNECTION_URI: &str = "postgresql://postgres:password@localhost:64003/defaultdb";
+    }
+
+    /// Find the project root via the crate root provided by `cargo test`,
+    /// and get our single static configuration file.
+    /// This depends on the convention that all our crates live in `/crates/<name>`
+    /// and will break in the unlikely case that we change this
+    pub fn get_path_from_project_root(ndc_metadata_path: impl AsRef<Path>) -> PathBuf {
+        let mut d = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+        d.push("../../");
+        d.push(ndc_metadata_path);
+
+        d
+    }
+}