Skip to content

config v5 with native operations rename #522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ multimap = "0.9"
nonempty = "0.10"
percent-encoding = "2"
prometheus = "0.13"
ref-cast = "1"
reqwest = "0.11"
schemars = "0.8"
serde = "1"
Expand Down
2 changes: 1 addition & 1 deletion benchmarks/component/docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ services:
OTEL_TRACES_SAMPLER: "always_off"
volumes:
- type: bind
source: ../../static/postgres/v3-chinook-ndc-metadata
source: ../../static/postgres/v5-configuration
target: /etc/connector
read_only: true
healthcheck:
Expand Down
2 changes: 1 addition & 1 deletion benchmarks/component/start.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ CONNECTION_URI="postgresql://postgres:password@${POSTGRESQL_SOCKET}" \
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT='http://localhost:4317' \
OTEL_SERVICE_NAME='ndc-postgres' \
cargo run -p ndc-postgres --quiet --release -- \
serve --configuration='../../static/postgres/v3-chinook-ndc-metadata' \
serve --configuration='../../static/postgres/v5-configuration' \
>& agent.log &
AGENT_PID=$!
echo "$AGENT_PID" > ./agent.pid
Expand Down
3 changes: 3 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

### Added

- Introduce configuration version "v5".
[#522](https://github.com/hasura/ndc-postgres/pull/522)

### Changed

### Fixed
Expand Down
113 changes: 112 additions & 1 deletion crates/cli/src/native_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@ async fn list(context: Context<impl Environment>) -> anyhow::Result<()> {
println!("- {}", native_operation.0);
}
}
configuration::ParsedConfiguration::Version5(ref mut configuration) => {
let operations = &configuration.metadata.native_operations;
println!("Native Queries:");
for native_operation in &operations.queries.0 {
println!("- {}", native_operation.0);
}
println!("Native Mutations:");
for native_operation in &operations.mutations.0 {
println!("- {}", native_operation.0);
}
}
};
Ok(())
}
Expand Down Expand Up @@ -157,7 +168,70 @@ async fn create(
{
entry.insert(new_native_operation);
} else {
anyhow::bail!("A Native Operation with the name '{}' already exists. To override, use the --override flag.", name);
anyhow::bail!("A Native Operation with the name '{name}' already exists. To override, use the --override flag.");
}
}
}
}
configuration::ParsedConfiguration::Version5(ref mut configuration) => {
let connection_string = configuration.get_connection_uri()?;

let kind = match kind {
configuration::version4::native_operations::Kind::Query => {
configuration::version5::native_operations::Kind::Query
}
configuration::version4::native_operations::Kind::Mutation => {
configuration::version5::native_operations::Kind::Mutation
}
};

let new_native_operation = configuration::version5::native_operations::create(
configuration,
&connection_string,
&operation_path,
&file_contents,
)
.await?;

// Add the new native operation to the configuration.
match override_entry {
Override::Yes => match kind {
configuration::version5::native_operations::Kind::Query => {
configuration
.metadata
.native_operations
.queries
.0
.insert(name, new_native_operation);
}
configuration::version5::native_operations::Kind::Mutation => {
configuration
.metadata
.native_operations
.mutations
.0
.insert(name, new_native_operation);
}
},
Override::No => {
// Only insert if vacant.
if let std::collections::btree_map::Entry::Vacant(entry) = match kind {
configuration::version5::native_operations::Kind::Query => configuration
.metadata
.native_operations
.queries
.0
.entry(name.clone()),
configuration::version5::native_operations::Kind::Mutation => configuration
.metadata
.native_operations
.mutations
.0
.entry(name.clone()),
} {
entry.insert(new_native_operation);
} else {
anyhow::bail!("A Native Operation with the name '{name}' already exists. To override, use the --override flag.");
}
}
}
Expand Down Expand Up @@ -224,6 +298,43 @@ async fn delete(
}
}
}
configuration::ParsedConfiguration::Version5(ref mut configuration) => {
// Delete if exists and is of the same type, error if not.
match kind {
Kind::Mutation => {
match configuration
.metadata
.native_operations
.mutations
.0
.entry(name.clone())
{
std::collections::btree_map::Entry::Occupied(entry) => {
entry.remove_entry();
}
std::collections::btree_map::Entry::Vacant(_) => {
anyhow::bail!(error_message_not_exist);
}
}
}
Kind::Query => {
match configuration
.metadata
.native_operations
.queries
.0
.entry(name.clone())
{
std::collections::btree_map::Entry::Occupied(entry) => {
entry.remove_entry();
}
std::collections::btree_map::Entry::Vacant(_) => {
anyhow::bail!(error_message_not_exist);
}
}
}
}
}
}

// We write the configuration excluding the deleted Native Operation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
source: crates/cli/tests/initialize_tests.rs
expression: version
---
"4"
"5"
40 changes: 27 additions & 13 deletions crates/configuration/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::error::{
use crate::values::{IsolationLevel, PoolSettings};
use crate::version3;
use crate::version4;
use crate::version5;
use crate::VersionTag;
use schemars::{gen::SchemaSettings, schema::RootSchema};

Expand Down Expand Up @@ -39,11 +40,12 @@ pub const DEFAULT_CONNECTION_URI_VARIABLE: &str = "CONNECTION_URI";
pub enum ParsedConfiguration {
Version3(version3::RawConfiguration),
Version4(version4::ParsedConfiguration),
Version5(version5::ParsedConfiguration),
}

impl ParsedConfiguration {
pub fn initial() -> Self {
ParsedConfiguration::Version4(version4::ParsedConfiguration::empty())
ParsedConfiguration::Version5(version5::ParsedConfiguration::empty())
}
}

Expand Down Expand Up @@ -78,24 +80,31 @@ pub async fn introspect(
ParsedConfiguration::Version4(config) => Ok(ParsedConfiguration::Version4(
version4::introspect(config, environment).await?,
)),
ParsedConfiguration::Version5(config) => Ok(ParsedConfiguration::Version5(
version5::introspect(config, environment).await?,
)),
}
}

pub async fn parse_configuration(
configuration_dir: impl AsRef<Path> + Send,
) -> Result<ParsedConfiguration, ParseConfigurationError> {
// Try parsing each supported version in turn
match version4::parse_configuration(configuration_dir.as_ref()).await {
Err(v4_err) => match version3::parse_configuration(configuration_dir.as_ref()).await {
Err(v3_err) => Err(ParseConfigurationError::UnableToParseAnyVersions(
MultiError(vec![
("Trying V3".to_string(), Box::new(v3_err)),
("Trying V4".to_string(), Box::new(v4_err)),
]),
)),
Ok(config) => Ok(ParsedConfiguration::Version3(config)),
match version5::parse_configuration(configuration_dir.as_ref()).await {
Err(v5_err) => match version4::parse_configuration(configuration_dir.as_ref()).await {
Err(v4_err) => match version3::parse_configuration(configuration_dir.as_ref()).await {
Err(v3_err) => Err(ParseConfigurationError::UnableToParseAnyVersions(
MultiError(vec![
("Trying V3".to_string(), Box::new(v3_err)),
("Trying V4".to_string(), Box::new(v4_err)),
("Trying V5".to_string(), Box::new(v5_err)),
]),
Comment on lines +97 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have a "version" field we can go on here to differentiate before trying to parse the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a hidden assumption here that we might not have the same configuration structure in later versions.

For example, if one version calls the configuration file configuration.json and the other config.json, then we need to ask each version module, how to get the config version.

Might be worth thinking about, perhaps when @plcplc returns, but for now let's continue with this way of doing things and improve it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point; we don't know how it will continue.

)),
Ok(config) => Ok(ParsedConfiguration::Version3(config)),
},
Ok(config) => Ok(ParsedConfiguration::Version4(config)),
},
Ok(config) => Ok(ParsedConfiguration::Version4(config)),
Ok(config) => Ok(ParsedConfiguration::Version5(config)),
}
}

Expand All @@ -111,6 +120,7 @@ pub fn make_runtime_configuration(
match parsed_config {
ParsedConfiguration::Version3(c) => version3::make_runtime_configuration(c, environment),
ParsedConfiguration::Version4(c) => version4::make_runtime_configuration(c, environment),
ParsedConfiguration::Version5(c) => version5::make_runtime_configuration(c, environment),
}
}

Expand All @@ -122,6 +132,7 @@ pub async fn write_parsed_configuration(
match parsed_config {
ParsedConfiguration::Version3(c) => version3::write_parsed_configuration(c, out_dir).await,
ParsedConfiguration::Version4(c) => version4::write_parsed_configuration(c, out_dir).await,
ParsedConfiguration::Version5(c) => version5::write_parsed_configuration(c, out_dir).await,
}
}

Expand All @@ -132,8 +143,11 @@ pub async fn write_parsed_configuration(
pub fn upgrade_to_latest_version(parsed_config: ParsedConfiguration) -> ParsedConfiguration {
match parsed_config {
ParsedConfiguration::Version3(v) => {
ParsedConfiguration::Version4(version4::upgrade_from_v3(v))
ParsedConfiguration::Version5(version5::upgrade_from_v4(version4::upgrade_from_v3(v)))
}
ParsedConfiguration::Version4(v) => {
ParsedConfiguration::Version5(version5::upgrade_from_v4(v))
}
ParsedConfiguration::Version4(_) => parsed_config,
ParsedConfiguration::Version5(_) => parsed_config,
}
}
2 changes: 2 additions & 0 deletions crates/configuration/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub mod metrics;

pub mod version3;
pub mod version4;
pub mod version5;

pub use configuration::{
generate_latest_schema, introspect, make_runtime_configuration, parse_configuration,
Expand All @@ -21,6 +22,7 @@ pub use metrics::Metrics;
pub enum VersionTag {
Version3,
Version4,
Version5,
}

#[cfg(test)]
Expand Down
9 changes: 9 additions & 0 deletions crates/configuration/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::VersionTag;
pub struct Metrics {
configuration_version_3: IntGauge,
configuration_version_4: IntGauge,
configuration_version_5: IntGauge,
}

impl Metrics {
Expand All @@ -26,9 +27,16 @@ impl Metrics {
"Get whether configuration version 4 is used",
)?;

let configuration_version_5 = add_int_gauge_metric(
metrics_registry,
"ndc_postgres_configuration_version_5",
"Get whether configuration version 5 is used",
)?;

Ok(Self {
configuration_version_3,
configuration_version_4,
configuration_version_5,
})
}

Expand All @@ -37,6 +45,7 @@ impl Metrics {
match version {
VersionTag::Version3 => self.configuration_version_3.set(1),
VersionTag::Version4 => self.configuration_version_4.set(1),
VersionTag::Version5 => self.configuration_version_5.set(1),
}
}
}
Expand Down
Loading