metadata-resolve: disallow relationship comparisons in nested object filter expressions for model filters (#960)

<!-- The PR description should answer 2 important questions: -->

### What

We cannot support having relationship comparisons in nested object field
filters of a boolean expression. NDC does not support this natively
([slack
thread](https://hasurahq.slack.com/archives/C05HND0F6LB/p1722845028319099)).
This PR adds a metadata build check for this case and reject such
boolean expressions.

Tests in the PR is partially based on
hasura/v3-engine#935.

**Note:** This might break older builds having relationship comparisons
in the nested object field filter expressions. This is a rare scenario.
We will check through our schema-diff job for any failing builds. If
there are significant, we might hold this PR. Full context in this slack
thread -
https://hasurahq.slack.com/archives/C06P2U8U55G/p1723121764332539.

### How
- Add a metadata build check for boolean expressions that restricts them
to have nested object filters with relationship comparisons.
- Raise GraphQL API runtime internal error while building the filter IR
when a relationship comparison found within a nested field filter.

---------

Co-authored-by: Daniel Chambers <[email protected]>
V3_GIT_ORIGIN_REV_ID: 86dce0b784c77e57bb1888125dba5b599e40f741

Author: 2 people

v3/changelog.md

@@ -6,6 +6,9 @@
 
 ### Fixed
 
+- Disallow model filter boolean expressions having relationship comparisons in
+  their nested object filters.
+
 ### Changed
 
 ## [v2024.08.07]

v3/crates/engine/tests/db_definition.sql

@@ -114,16 +114,22 @@ CREATE TYPE public.staff AS (
   favourite_artist_id int
 );
 
+CREATE TYPE public.institution_songs AS (
+  primary_anthem_track_id int,
+  secondary_anthem_track_id int
+);
+
 CREATE TABLE public.institution (
   id integer NOT NULL,
   name text,
   location location,
   staff staff [],
   departments text [],
+  songs institution_songs,
   CONSTRAINT institution_pkey PRIMARY KEY (id)
 );
 
-INSERT INTO public.institution (id, name, location, staff, departments)
+INSERT INTO public.institution (id, name, location, staff, departments, songs)
 VALUES (
     1,
     'Queen Mary University of London',
@@ -135,7 +141,11 @@ VALUES (
     ARRAY [ROW('Peter','Landin',ARRAY['Computer Science','Education'],
     1
   )::staff ],
-  ARRAY ['Humanities and Social Sciences','Science and Engineering','Medicine and Dentistry']
+  ARRAY ['Humanities and Social Sciences','Science and Engineering','Medicine and Dentistry'],
+  ROW(
+    2270,
+    2271
+  )::institution_songs
 ),
 (
   2,
@@ -154,12 +164,17 @@ ROW(
   ARRAY ['Computer Science','Functional Programming','Automated Reasoning'],
   3
 )::staff ],
-ARRAY ['Architecture and Civil Engineering','Computer Science and Engineering','Electrical Engineering','Physics','Industrial and Materials Science']
+ARRAY ['Architecture and Civil Engineering','Computer Science and Engineering','Electrical Engineering','Physics','Industrial and Materials Science'],
+ROW(
+    3421,
+    NULL
+  )::institution_songs
 ),
 (
   3,
   'University of Nowhere',
   null,
   null,
-  ARRAY ['nothing',null]
+  ARRAY ['nothing',null],
+  NULL
 );

v3/crates/engine/tests/execute/common_metadata/postgres_connector_schema.json

@@ -499,6 +499,15 @@
                           "element_type": { "type": "named", "name": "staff" }
                         }
                       },
+                      "songs": {
+                        "type": {
+                          "type": "nullable",
+                          "underlying_type": {
+                            "type": "named",
+                            "name": "institution_songs"
+                          }
+                        }
+                      },
                       "departments": {
                         "description": "The institution's departments",
                         "arguments": {},
@@ -517,6 +526,22 @@
                       }
                     }
                   },
+                  "institution_songs": {
+                    "fields": {
+                      "primary_anthem_track_id": {
+                        "type": {
+                          "type": "named",
+                          "name": "int4"
+                        }
+                      },
+                      "secondary_anthem_track_id": {
+                        "type": {
+                          "type": "named",
+                          "name": "int4"
+                        }
+                      }
+                    }
+                  },
 
                   "test_nullable_and_array": {
                     "description": "Testing nullable and array types",

v3/crates/engine/tests/ndc-postgres-configuration/configuration.json

@@ -841,6 +841,14 @@
             "nullable": "nullable",
             "description": null
           },
+          "songs": {
+            "name": "songs",
+            "type": {
+              "compositeType": "institution_songs"
+            },
+            "nullable": "nullable",
+            "description": null
+          },
           "staff": {
             "name": "staff",
             "type": {
@@ -2578,6 +2586,27 @@
         },
         "description": null
       },
+      "institution_songs": {
+        "typeName": "institution_songs",
+        "schemaName": "public",
+        "fields": {
+          "primary_anthem_track_id": {
+            "fieldName": "primary_anthem_track_id",
+            "type": {
+              "scalarType": "int4"
+            },
+            "description": null
+          },
+          "secondary_anthem_track_id": {
+            "fieldName": "secondary_anthem_track_id",
+            "type": {
+              "scalarType": "int4"
+            },
+            "description": null
+          }
+        },
+        "description": null
+      },
       "location": {
         "typeName": "location",
         "schemaName": "public",

v3/crates/engine/tests/ndc-postgres-configuration/schema.json

@@ -91,6 +91,11 @@
             "exposedName": "_lt",
             "operatorKind": "custom"
           },
+          {
+            "operatorName": "<>",
+            "exposedName": "_neq",
+            "operatorKind": "custom"
+          },
           {
             "operatorName": "!=",
             "exposedName": "_neq",
@@ -589,7 +594,7 @@
           "required": ["scalarType"],
           "properties": {
             "scalarType": {
-              "$ref": "#/definitions/ScalarTypeName"
+              "type": "string"
             }
           },
           "additionalProperties": false
@@ -599,7 +604,7 @@
           "required": ["compositeType"],
           "properties": {
             "compositeType": {
-              "$ref": "#/definitions/CompositeTypeName"
+              "type": "string"
             }
           },
           "additionalProperties": false
@@ -616,14 +621,6 @@
         }
       ]
     },
-    "ScalarTypeName": {
-      "description": "A name of a Scalar Type, as it appears in the NDC scheme.",
-      "type": "string"
-    },
-    "CompositeTypeName": {
-      "description": "The name of a Composite Type, as it appears in the NDC schema",
-      "type": "string"
-    },
     "Nullable": {
       "description": "Can this column contain null values",
       "type": "string",
@@ -652,7 +649,7 @@
       }
     },
     "UniquenessConstraint": {
-      "description": "The set of columns that make up a uniqueness constraint.",
+      "description": "The set of columns that make up a uniqueness constraint. We map each table column to their ndc field names.",
       "type": "array",
       "items": {
         "type": "string"
@@ -740,7 +737,7 @@
       "required": ["returnType"],
       "properties": {
         "returnType": {
-          "$ref": "#/definitions/ScalarTypeName"
+          "type": "string"
         }
       }
     },
@@ -756,7 +753,7 @@
           "$ref": "#/definitions/OperatorKind"
         },
         "argumentType": {
-          "$ref": "#/definitions/ScalarTypeName"
+          "type": "string"
         },
         "isInfix": {
           "default": true,
@@ -1110,6 +1107,11 @@
               "exposedName": "_lt",
               "operatorKind": "custom"
             },
+            {
+              "operatorName": "<>",
+              "exposedName": "_neq",
+              "operatorKind": "custom"
+            },
             {
               "operatorName": "!=",
               "exposedName": "_neq",

v3/crates/ir/src/error.rs

@@ -180,6 +180,9 @@ pub enum InternalDeveloperError {
         aggregate_operand_type: QualifiedTypeName,
         aggregation_function: AggregationFunctionName,
     },
+
+    #[error("The relationship '{relationship_name}' is from a nested object and cannot be used in a predicate")]
+    NestedObjectRelationshipInPredicate { relationship_name: RelationshipName },
 }
 
 #[derive(Debug, thiserror::Error)]

v3/crates/ir/src/filter.rs

@@ -358,6 +358,7 @@ fn build_comparison_expression<'s>(
                 );
                 expressions.push(expression);
             }
+            // Nested field comparison
             schema::Annotation::Input(InputAnnotation::BooleanExpression(
                 BooleanExpressionAnnotation::BooleanExpressionArgument {
                     field:
@@ -391,6 +392,20 @@ fn build_comparison_expression<'s>(
 
                 expressions.push(inner_expression);
             }
+            // Nested relationship comparison
+            schema::Annotation::Input(InputAnnotation::BooleanExpression(
+                BooleanExpressionAnnotation::BooleanExpressionArgument {
+                    field:
+                        schema::ModelFilterArgument::RelationshipField(FilterRelationshipAnnotation {
+                            relationship_name,
+                            ..
+                        }),
+                },
+            )) => Err(
+                error::InternalDeveloperError::NestedObjectRelationshipInPredicate {
+                    relationship_name: relationship_name.clone(),
+                },
+            )?,
 
             annotation => Err(error::InternalEngineError::UnexpectedAnnotation {
                 annotation: annotation.clone(),

v3/crates/metadata-resolve/src/helpers/boolean_expression.rs

@@ -27,7 +27,7 @@ pub(crate) fn validate_data_connector_with_object_boolean_expression_type(
     models: &IndexMap<Qualified<ModelName>, models::Model>,
 ) -> Result<(), Error> {
     if let Some(graphql_config) = &object_boolean_expression_type.graphql {
-        for object_comparison_expression_info in graphql_config.object_fields.values() {
+        for (field_name, object_comparison_expression_info) in &graphql_config.object_fields {
             // look up the leaf boolean expression type
             let leaf_boolean_expression = boolean_expression_types
                 .objects
@@ -53,6 +53,21 @@ pub(crate) fn validate_data_connector_with_object_boolean_expression_type(
                         }));
             }
 
+            // This nested object field should not contain any relationship comparisons
+            if leaf_boolean_expression
+                .graphql
+                .as_ref()
+                .is_some_and(|leaf_graphql_config| {
+                    !leaf_graphql_config.relationship_fields.is_empty()
+                })
+            {
+                return Err(Error::from(boolean_expressions::BooleanExpressionError::NestedObjectFieldContainsRelationshipComparison {
+                    parent_boolean_expression_type_name: object_boolean_expression_type.name.clone(),
+                    field_name: field_name.clone(),
+                    nested_boolean_expression_type_name: object_comparison_expression_info.object_type_name.clone(),
+                }));
+            }
+
             // continue checking the nested object...
             validate_data_connector_with_object_boolean_expression_type(
                 data_connector,

v3/crates/metadata-resolve/src/stages/boolean_expressions/error.rs

@@ -76,6 +76,15 @@ pub enum BooleanExpressionError {
         nested_type_name: Qualified<CustomTypeName>,
         data_connector_name: Qualified<DataConnectorName>,
     },
+    #[error(
+        "The nested object field '{field_name}' within '{parent_boolean_expression_type_name}' cannot be used for comparison \
+         because its boolean expression type '{nested_boolean_expression_type_name}' involves a relationship comparison field."
+    )]
+    NestedObjectFieldContainsRelationshipComparison {
+        field_name: FieldName,
+        parent_boolean_expression_type_name: Qualified<CustomTypeName>,
+        nested_boolean_expression_type_name: Qualified<CustomTypeName>,
+    },
     #[error("The field {field_name:} has type {field_type:} but the field's boolean expression type {field_boolean_expression_type_name:} has type {underlying_type:}")]
     FieldTypeMismatch {
         field_name: FieldName,