Raise a warning instead of an error if aggregates are configured for …

…graphql use but GraphqlConfig is missing aggregates configuration (#1079)

### What
If a user configures `AggregateExpressions` in their metadata with the
`graphql` section configured, but forgets to configure `aggregates` in
`GraphqlConfig`, an error was raised and the build failed. This scenario
occurs when a user has an existing pre-aggregrates metadata and are
adding aggregates to it later.

While simply adding the required configuration to `GraphqlConfig` would
fix the error, there are cases where the `GraphqlConfig` is not in the
user's current repo, such as where they are working in a separate
subgraph repo and the `GraphqlConfig` is managed elsewhere and requires
a coordinated change.

This PR turns that error into a warning and allows a successful build.
The build will not have aggregates show up in the GraphQL, but does
succeed, which allows the user to progress until the `GraphqlConfig` is
updated separately.

### How

A new `AggregateExpressionIssue` type is added and the error is moved
from `AggregateExpressionError` to that type instead. The code then logs
the new issue and contributes it to the main issues collection.

The test that checked for this error (a failure test) has been moved to
a successful test and the warning can be seen at the bottom of the new
snapshot file.

V3_GIT_ORIGIN_REV_ID: 751590c484feec4ae03f079ae6a1bc0bf867ff64

v3/changelog.md

@@ -7,6 +7,14 @@
 ### Fixed
 
 - Disallow recursive types in SQL table column types
+- Previously, if you had `AggregateExpressions` that were configured to be used
+  in GraphQL, but you did not set the appropriate configuration in
+  `GraphqlConfig.definition.query.aggregates`, the build would fail with an
+  error. This has been relaxed so that the build now succeeds, but a warning is
+  raised instead. However, the aggregates will not appear in your GraphQL API
+  until the `GraphqlConfig` is updated. This allows you to add
+  `AggregateExpressions` and update your `GraphqlConfig` separately, which is
+  useful if they are in separate repositories.
 
 ### Changed
 

v3/crates/metadata-resolve/src/stages/aggregates/mod.rs

@@ -34,6 +34,7 @@ pub fn resolve(
 ) -> Result<AggregateExpressionsOutput, AggregateExpressionError> {
     let mut resolved_aggregate_expressions =
         BTreeMap::<Qualified<AggregateExpressionName>, AggregateExpression>::new();
+    let mut issues = vec![];
 
     for open_dds::accessor::QualifiedObject {
         subgraph,
@@ -63,6 +64,7 @@ pub fn resolve(
             graphql_config,
             &aggregate_expression_name,
             aggregate_expression,
+            &mut issues,
         )?;
 
         resolved_aggregate_expressions
@@ -72,6 +74,7 @@ pub fn resolve(
     Ok(AggregateExpressionsOutput {
         aggregate_expressions: resolved_aggregate_expressions,
         graphql_types: existing_graphql_types,
+        issues,
     })
 }
 
@@ -88,6 +91,7 @@ fn resolve_aggregate_expression(
     graphql_config: &graphql_config::GraphqlConfig,
     aggregate_expression_name: &Qualified<AggregateExpressionName>,
     aggregate_expression: &open_dds::aggregates::AggregateExpressionV1,
+    issues: &mut Vec<AggregateExpressionIssue>,
 ) -> Result<AggregateExpression, AggregateExpressionError> {
     let operand = match &aggregate_expression.operand {
         open_dds::aggregates::AggregateOperand::Object(object_operand) => resolve_object_operand(
@@ -112,6 +116,7 @@ fn resolve_aggregate_expression(
         aggregate_expression_name,
         &operand,
         &aggregate_expression.graphql,
+        issues,
     )?;
 
     Ok(AggregateExpression {
@@ -615,6 +620,7 @@ fn resolve_aggregate_expression_graphql_config(
     aggregate_expression_graphql_definition: &Option<
         open_dds::aggregates::AggregateExpressionGraphQlDefinition,
     >,
+    issues: &mut Vec<AggregateExpressionIssue>,
 ) -> Result<Option<AggregateExpressionGraphqlConfig>, AggregateExpressionError> {
     let select_type_name = aggregate_expression_graphql_definition
         .as_ref()
@@ -634,21 +640,19 @@ fn resolve_aggregate_expression_graphql_config(
         },
     )?;
 
-    let graphql_config = select_type_name
-        .map(|select_type_name| -> Result<_, AggregateExpressionError> {
-            // Check that the aggregate config is configured in graphql config
-            let aggregate_config =
-                graphql_config
-                    .query
-                    .aggregate_config
-                    .as_ref()
-                    .ok_or_else(
-                        || AggregateExpressionError::ConfigMissingFromGraphQlConfig {
-                            name: aggregate_expression_name.clone(),
-                            config_name: "query.aggregate".to_string(),
-                        },
-                    )?;
-
+    let graphql_config = match (select_type_name, &graphql_config.query.aggregate_config) {
+        (None, _) => None,
+        (Some(_select_type_name), None) => {
+            // If the a select type name has been configured, but global aggregate config
+            // is missing from GraphqlConfig, raise a warning since this is likely a user
+            // misconfiguration issue.
+            issues.push(AggregateExpressionIssue::ConfigMissingFromGraphQlConfig {
+                name: aggregate_expression_name.clone(),
+                config_name: "query.aggregate".to_string(),
+            });
+            None
+        }
+        (Some(select_type_name), Some(aggregate_config)) => {
             // Check that no aggregatable field conflicts with the _count field
             if let Some(field) = aggregate_operand.aggregatable_fields.iter().find(|field| {
                 field.field_name.as_str() == aggregate_config.count_field_name.as_str()
@@ -704,13 +708,13 @@ fn resolve_aggregate_expression_graphql_config(
                 });
             }
 
-            Ok(AggregateExpressionGraphqlConfig {
+            Some(AggregateExpressionGraphqlConfig {
                 select_output_type_name: select_type_name,
                 count_field_name: aggregate_config.count_field_name.clone(),
                 count_distinct_field_name: aggregate_config.count_distinct_field_name.clone(),
             })
-        })
-        .transpose()?;
+        }
+    };
 
     Ok(graphql_config)
 }

v3/crates/metadata-resolve/src/stages/aggregates/types.rs

@@ -17,6 +17,7 @@ use crate::{Qualified, QualifiedTypeName, QualifiedTypeReference};
 pub struct AggregateExpressionsOutput {
     pub aggregate_expressions: BTreeMap<Qualified<AggregateExpressionName>, AggregateExpression>,
     pub graphql_types: BTreeSet<ast::TypeName>,
+    pub issues: Vec<AggregateExpressionIssue>,
 }
 
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
@@ -72,19 +73,22 @@ pub struct AggregateExpressionGraphqlConfig {
     pub select_output_type_name: ast::TypeName,
 }
 
+#[derive(Debug, thiserror::Error, PartialEq, Eq, Clone)]
+pub enum AggregateExpressionIssue {
+    #[error("the aggregate expression {name} defines a graphql section but it will not appear in the GraphQL API unless {config_name} is also configured in the GraphqlConfig")]
+    ConfigMissingFromGraphQlConfig {
+        name: Qualified<AggregateExpressionName>,
+        config_name: String,
+    },
+}
+
 #[derive(Debug, thiserror::Error)]
 pub enum AggregateExpressionError {
     #[error("the following aggregate expression is defined more than once: {name}")]
     DuplicateAggregateExpressionDefinition {
         name: Qualified<AggregateExpressionName>,
     },
 
-    #[error("the aggregate expression {name} defines a graphql section and so {config_name} must be set in the GraphqlConfig")]
-    ConfigMissingFromGraphQlConfig {
-        name: Qualified<AggregateExpressionName>,
-        config_name: String,
-    },
-
     #[error("the name used by {config_name} from the GraphqlConfig conflicts with the aggregatable field name {aggregatable_field_name} in the aggregate expression {name}")]
     AggregatableFieldNameConflict {
         name: Qualified<AggregateExpressionName>,

v3/crates/metadata-resolve/src/stages/mod.rs

@@ -119,6 +119,7 @@ pub fn resolve(
     let aggregates::AggregateExpressionsOutput {
         aggregate_expressions,
         graphql_types,
+        issues,
     } = aggregates::resolve(
         &metadata_accessor,
         &data_connectors,
@@ -129,6 +130,8 @@ pub fn resolve(
         &graphql_config,
     )?;
 
+    all_warnings.extend(issues.into_iter().map(Warning::from));
+
     // Validate `ObjectBooleanExpressionType` metadata. This will soon be deprecated and subsumed
     // by `BooleanExpressionType`.
     let object_boolean_expressions::ObjectBooleanExpressionsOutput {

v3/crates/metadata-resolve/src/types/warning.rs

@@ -1,7 +1,7 @@
 use open_dds::flags;
 
 use crate::stages::{
-    boolean_expressions, commands, data_connectors, models, object_boolean_expressions,
+    aggregates, boolean_expressions, commands, data_connectors, models, object_boolean_expressions,
 };
 
 use super::error::ShouldBeAnError;
@@ -22,6 +22,8 @@ pub enum Warning {
     ModelIssue(#[from] models::ModelsIssue),
     #[error("{0}")]
     CommandIssue(#[from] commands::CommandsIssue),
+    #[error("{0}")]
+    AggregateExpressionIssue(#[from] aggregates::AggregateExpressionIssue),
 }
 
 impl ShouldBeAnError for Warning {