Fix invalid joins when the order by relationship predicate is null

changelog.md

@@ -8,6 +8,9 @@
 
 ### Fixed
 
+- Predicates in relationships using in ordering (usually supplied by the engine's permission system) would fail to join the related tables correctly if the predicate was null.
+  [#655](https://github.com/hasura/ndc-postgres/pull/655)
+
 ## [v1.2.0] - 2024-10-25
 
 ### Added

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

@@ -776,31 +776,32 @@ fn select_for_path_element(
     select.select_list = select_list;
     select.from = Some(from_clause);
 
-    match predicate {
-        None => Ok(select),
+    let predicate_tables = RootAndCurrentTables {
+        root_table: root_and_current_tables.root_table.clone(),
+        current_table: join_table,
+    };
+
+    // generate a condition for this join.
+    let join_condition = relationships::translate_column_mapping(
+        env,
+        &root_and_current_tables.current_table,
+        &predicate_tables.current_table.reference,
+        sql::helpers::empty_where(),
+        relationship,
+    )?;
+
+    select.where_ = match predicate {
+        None => sql::ast::Where(join_condition),
         Some(predicate) => {
             // generate a condition for the predicate.
-            let predicate_tables = RootAndCurrentTables {
-                root_table: root_and_current_tables.root_table.clone(),
-                current_table: join_table,
-            };
             let predicate_expr = filtering::translate(env, state, &predicate_tables, predicate)?;
 
-            // generate a condition for this join.
-            let join_condition = relationships::translate_column_mapping(
-                env,
-                &root_and_current_tables.current_table,
-                &predicate_tables.current_table.reference,
-                sql::helpers::empty_where(),
-                relationship,
-            )?;
-
-            select.where_ = sql::ast::Where(sql::ast::Expression::And {
+            sql::ast::Where(sql::ast::Expression::And {
                 left: Box::new(join_condition),
                 right: Box::new(predicate_expr),
-            });
-
-            Ok(select)
+            })
         }
-    }
+    };
+
+    Ok(select)
 }

crates/query-engine/translation/tests/goldenfiles/sorting_by_nested_relationship_column/request.json

@@ -19,18 +19,12 @@
               {
                 "relationship": "TrackAlbum",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               },
               {
                 "relationship": "AlbumArtist",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

crates/query-engine/translation/tests/goldenfiles/sorting_by_nested_relationship_column_with_predicate/request.json

@@ -41,10 +41,7 @@
               {
                 "relationship": "AlbumArtist",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

crates/query-engine/translation/tests/goldenfiles/sorting_by_nested_relationship_count/request.json

@@ -19,10 +19,7 @@
               {
                 "relationship": "ArtistAlbum",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           },

crates/query-engine/translation/tests/goldenfiles/sorting_by_recursive_relationship_column/request.json

@@ -19,18 +19,12 @@
               {
                 "relationship": "CompanyCEO",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               },
               {
                 "relationship": "PersonParent",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

crates/query-engine/translation/tests/goldenfiles/sorting_by_relationship_column/request.json

@@ -21,10 +21,7 @@
               {
                 "relationship": "AlbumArtist",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

crates/query-engine/translation/tests/goldenfiles/sorting_by_relationship_column_with_true_predicate/configuration.json

@@ -0,0 +1,86 @@
+{
+  "version": "4",
+  "metadata": {
+    "tables": {
+      "Album": {
+        "schemaName": "public",
+        "tableName": "Album",
+        "columns": {
+          "AlbumId": {
+            "name": "AlbumId",
+            "type": {
+              "scalarType": "int4"
+            },
+            "nullable": "nullable",
+            "description": null
+          },
+          "ArtistId": {
+            "name": "ArtistId",
+            "type": {
+              "scalarType": "int4"
+            },
+            "nullable": "nullable",
+            "description": null
+          },
+          "Title": {
+            "name": "Title",
+            "type": {
+              "scalarType": "varchar"
+            },
+            "nullable": "nullable",
+            "description": null
+          }
+        },
+        "uniquenessConstraints": {},
+        "foreignRelations": {},
+        "description": null
+      },
+      "Artist": {
+        "schemaName": "public",
+        "tableName": "Artist",
+        "columns": {
+          "ArtistId": {
+            "name": "ArtistId",
+            "type": {
+              "scalarType": "int4"
+            },
+            "nullable": "nullable",
+            "description": null
+          },
+          "Name": {
+            "name": "Name",
+            "type": {
+              "scalarType": "varchar"
+            },
+            "nullable": "nullable",
+            "description": null
+          }
+        },
+        "uniquenessConstraints": {},
+        "foreignRelations": {},
+        "description": null
+      }
+    },
+    "scalarTypes": {
+      "int4": {
+        "typeName": "int4",
+        "schemaName": "pg_catalog",
+        "description": null,
+        "aggregateFunctions": {},
+        "comparisonOperators": {},
+        "typeRepresentation": null
+      },
+      "varchar": {
+        "typeName": "varchar",
+        "schemaName": "pg_catalog",
+        "description": null,
+        "aggregateFunctions": {},
+        "comparisonOperators": {},
+        "typeRepresentation": null
+      }
+    },
+    "compositeTypes": {},
+    "nativeQueries": {}
+  },
+  "mutationsVersion": null
+}

crates/query-engine/translation/tests/goldenfiles/sorting_by_relationship_column_with_true_predicate/request.json

@@ -0,0 +1,46 @@
+{
+  "collection": "Album",
+  "query": {
+    "fields": {
+      "Name": {
+        "type": "column",
+        "column": "Title",
+        "arguments": {}
+      }
+    },
+    "limit": 5,
+    "offset": 3,
+    "order_by": {
+      "elements": [
+        {
+          "order_direction": "asc",
+          "target": {
+            "type": "column",
+            "name": "Name",
+            "path": [
+              {
+                "relationship": "AlbumArtist",
+                "arguments": {},
+                "predicate": {
+                  "type": "and",
+                  "expressions": []
+                }
+              }
+            ]
+          }
+        }
+      ]
+    }
+  },
+  "arguments": {},
+  "collection_relationships": {
+    "AlbumArtist": {
+      "column_mapping": {
+        "ArtistId": "ArtistId"
+      },
+      "relationship_type": "object",
+      "target_collection": "Artist",
+      "arguments": {}
+    }
+  }
+}

crates/query-engine/translation/tests/goldenfiles/sorting_by_relationship_count/request.json

@@ -19,10 +19,7 @@
               {
                 "relationship": "ArtistAlbums",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

crates/query-engine/translation/tests/snapshots/tests__sorting_by_relationship_column_with_true_predicate.snap

@@ -0,0 +1,44 @@
+---
+source: crates/query-engine/translation/tests/tests.rs
+expression: result
+---
+SELECT
+  coalesce(json_agg(row_to_json("%3_universe")), '[]') AS "universe"
+FROM
+  (
+    SELECT
+      *
+    FROM
+      (
+        SELECT
+          coalesce(json_agg(row_to_json("%4_rows")), '[]') AS "rows"
+        FROM
+          (
+            SELECT
+              "%0_Album"."Title" AS "Name"
+            FROM
+              "public"."Album" AS "%0_Album"
+              LEFT OUTER JOIN LATERAL (
+                SELECT
+                  "%1_ORDER_PART_Artist"."Name" AS "Name"
+                FROM
+                  (
+                    SELECT
+                      "%1_ORDER_PART_Artist"."Name" AS "Name"
+                    FROM
+                      "public"."Artist" AS "%1_ORDER_PART_Artist"
+                    WHERE
+                      (
+                        "%0_Album"."ArtistId" = "%1_ORDER_PART_Artist"."ArtistId"
+                      )
+                  ) AS "%1_ORDER_PART_Artist"
+              ) AS "%2_ORDER_FOR_Album" ON ('true')
+            ORDER BY
+              "%2_ORDER_FOR_Album"."Name" ASC
+            LIMIT
+              5 OFFSET 3
+          ) AS "%4_rows"
+      ) AS "%4_rows"
+  ) AS "%3_universe";
+
+{}

crates/query-engine/translation/tests/tests.rs

@@ -259,6 +259,14 @@ async fn sorting_by_relationship_column() {
     insta::assert_snapshot!(result);
 }
 
+#[tokio::test]
+async fn sorting_by_relationship_column_with_true_predicate() {
+    let result = common::test_translation("sorting_by_relationship_column_with_true_predicate")
+        .await
+        .unwrap();
+    insta::assert_snapshot!(result);
+}
+
 #[tokio::test]
 async fn sorting_by_nested_relationship_column() {
     let result = common::test_translation("sorting_by_nested_relationship_column")