config v5 with native operations rename #522

soupi · 2024-07-05T14:57:56Z

What

We introduce ndc-postgres configuration version "v5".

This version changes two things:

Renames the field nativeQueries to nativeOperations, and instead of containing native queries and native mutations together with a flag on each marking if it is mutation or not (isProcedure), we nest the in different objects, so we have:
```
nativeOperations:
  queries:
  mutations:
```
We nest scalarTypes and compositeTypes under one field, types:
```
types:
  scalar:
  composite:
```

How

Might be useful to read as commits.

We first copy paste v4 to v5, then create types for Types and NativeOperations which nest the other information, then we fix all the relevant things: ndc-spec schema, cli handling, upgrading from older configs, conversion to runtime config, etc.

Then we fix the tests, including introducing new configs and changing the paths the the configs.

Then we get rid of the older configurations in the static/<db> directories, as they are not longer used for anything.

…tive-operations-rename

soupi · 2024-07-09T12:13:03Z

crates/configuration/src/version5/comparison.rs

copy paste from v4.

soupi · 2024-07-09T12:13:15Z

crates/configuration/src/version5/connection_settings.rs

copy paste from v4.

soupi · 2024-07-09T12:13:36Z

crates/configuration/src/version5/introspection.sql

copy paste from v4.

soupi · 2024-07-09T12:17:01Z

crates/configuration/src/version5/metadata/database.rs

copy paste from v4.

soupi · 2024-07-09T12:17:26Z

crates/configuration/src/version5/metadata/mod.rs

+    pub types: Types,
+    #[serde(default)]
+    pub native_operations: NativeOperations,


these two are different.

soupi · 2024-07-09T12:39:51Z

crates/configuration/src/version5/to_runtime_configuration.rs

Fairly straightforward because they are almost the same shape.

soupi · 2024-07-09T12:41:00Z

crates/configuration/src/version5/upgrade_from_v4.rs

+fn upgrade_native_queries(
+    native_queries: version4::metadata::NativeQueries,
+) -> metadata::NativeOperations {
+    let version4::metadata::NativeQueries(native_queries_map) = native_queries;
+
+    let mut queries = BTreeMap::new();
+    let mut mutations = BTreeMap::new();
+
+    for (name, native_query_info) in native_queries_map {
+        let is_procedure = native_query_info.is_procedure;
+        let operation = upgrade_native_query_info(native_query_info);
+        if is_procedure {
+            mutations.insert(name, operation);
+        } else {
+            queries.insert(name, operation);
+        }
+    }
+
+    metadata::NativeOperations {
+        queries: metadata::NativeQueries(queries),
+        mutations: metadata::NativeQueries(mutations),
+    }
+}


Pretty much the only part this is interesting in the file.

soupi · 2024-07-09T12:47:49Z

crates/query-engine/translation/src/translation/helpers.rs

-    pub fn lookup_native_query(
+    pub fn lookup_native_mutation(
        &self,
        procedure_name: &str,
    ) -> Result<&metadata::NativeQueryInfo, Error> {


only used by mutations anyway, hinted by the argument named procedure_name.

soupi · 2024-07-09T12:49:27Z

crates/query-engine/translation/tests/goldenfiles/mutations/v2_insert/configuration.json

We didn't switch all of the translation tests to work against version 4. Perhaps we should, but we also have coverage from the db tests.

soupi · 2024-07-09T12:52:11Z

static/schema.json

+        "types": {
+          "scalar": {},
+          "composite": {}
+        },
+        "nativeOperations": {
+          "queries": {},
+          "mutations": {}
+        }


The change.

SamirTalwar · 2024-07-09T14:34:19Z

crates/configuration/src/configuration.rs

+                    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)),
+                    ]),


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

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.

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

SamirTalwar · 2024-07-09T15:14:11Z

crates/cli/src/native_operations.rs

+                    } {
+                        entry.insert(new_native_operation);
+                    } else {
+                        anyhow::bail!("A Native Operation with the name '{}' already exists. To override, use the --override flag.", name);


Suggested change

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.");

SamirTalwar · 2024-07-09T15:29:40Z

crates/configuration/src/version5/metadata/database.rs

+// This code was copied from a different place that predated the introduction of clippy to the
+// project. Therefore we disregard certain clippy lints:
+#![allow(
+    clippy::enum_variant_names,
+    clippy::upper_case_acronyms,
+    clippy::wrong_self_convention
+)]


Feels appropriate to fix these when making breaking changes.

SamirTalwar · 2024-07-09T15:29:52Z

crates/configuration/src/version5/metadata/native_operations.rs

+// This code was copied from a different place that predated the introduction of clippy to the
+// project. Therefore we disregard certain clippy lints:
+#![allow(clippy::wrong_self_convention)]


Likewise, here.

SamirTalwar · 2024-07-09T15:33:38Z

crates/query-engine/translation/src/translation/helpers.rs

+                self.metadata
+                    .composite_types
+                    .0
+                    .get(&metadata::CompositeTypeName(type_name.to_string()))


Use ::ref_cast here, perhaps?

Gil Mizrahi added 8 commits July 5, 2024 15:52

make naked v5

098603f

nested scalar and composite types, and native operations

40849a4

split native queries into native operations

1d6a281

update configurations

878047d

update tests and configs

0fc633b

add metadata snapshot

da36f40

remove config tests for v5

fa5e4ba

Merge remote-tracking branch 'origin/main' into gil/config-v5-with-na…

fa8cc84

…tive-operations-rename

soupi commented Jul 9, 2024

View reviewed changes

goodbye older configs, you will live on as ndc-metadata-snapshots

bcf7143

soupi marked this pull request as ready for review July 9, 2024 13:00

Gil Mizrahi added 2 commits July 9, 2024 16:03

changelog

4e5ffe8

fix benchmarks

29dd548

soupi requested a review from plcplc July 9, 2024 13:34

SamirTalwar reviewed Jul 9, 2024

View reviewed changes

lint, macros and refcasts

553f13a

SamirTalwar approved these changes Jul 9, 2024

View reviewed changes

soupi added this pull request to the merge queue Jul 10, 2024

Merged via the queue into main with commit 3941422 Jul 10, 2024
31 checks passed

soupi deleted the gil/config-v5-with-native-operations-rename branch July 10, 2024 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config v5 with native operations rename #522

config v5 with native operations rename #522

soupi commented Jul 5, 2024 •

edited

Loading

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

soupi Jul 9, 2024

SamirTalwar Jul 9, 2024

soupi Jul 9, 2024

SamirTalwar Jul 9, 2024

SamirTalwar Jul 9, 2024

soupi Jul 9, 2024

SamirTalwar Jul 9, 2024

soupi Jul 9, 2024

SamirTalwar Jul 9, 2024

soupi Jul 9, 2024

SamirTalwar Jul 9, 2024

soupi Jul 9, 2024

	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.");

config v5 with native operations rename #522

config v5 with native operations rename #522

Conversation

soupi commented Jul 5, 2024 • edited Loading

What

How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soupi commented Jul 5, 2024 •

edited

Loading