Fix invalid joins when the order by relationship predicate is null (#655

) ### What If a relationship used in an order by specified a null predicate, the SQL generator would fail to add the join conditions when joining the related table into the query. This would result in an incorrect cross product between the table and the related table. ### How If the relationship predicate is null, we now correctly add the table's join conditions. Existing tests that assumed all predicates would contain an always true expression have been changed to contain a null predicate and an extra test has been added to cover receiving an always true predicate (to make sure that still works). Fixes ENG-1386
hasura · Dec 3, 2024 · 7a5a372 · 7a5a372
1 parent dc51e4a
commit 7a5a372
Show file tree

Hide file tree

Showing 12 changed files with 216 additions and 52 deletions.
diff --git a/changelog.md b/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

diff --git a/crates/query-engine/translation/src/translation/query/sorting.rs b/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)
 }
diff --git a/...y-engine/translation/tests/goldenfiles/sorting_by_nested_relationship_column/request.json b/...y-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
               }
             ]
           }

diff --git a/...ation/tests/goldenfiles/sorting_by_nested_relationship_column_with_predicate/request.json b/...ation/tests/goldenfiles/sorting_by_nested_relationship_column_with_predicate/request.json
@@ -41,10 +41,7 @@
               {
                 "relationship": "AlbumArtist",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

diff --git a/...ry-engine/translation/tests/goldenfiles/sorting_by_nested_relationship_count/request.json b/...ry-engine/translation/tests/goldenfiles/sorting_by_nested_relationship_count/request.json
@@ -19,10 +19,7 @@
               {
                 "relationship": "ArtistAlbum",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           },

diff --git a/...ngine/translation/tests/goldenfiles/sorting_by_recursive_relationship_column/request.json b/...ngine/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
               }
             ]
           }

diff --git a/...es/query-engine/translation/tests/goldenfiles/sorting_by_relationship_column/request.json b/...es/query-engine/translation/tests/goldenfiles/sorting_by_relationship_column/request.json
@@ -21,10 +21,7 @@
               {
                 "relationship": "AlbumArtist",
                 "arguments": {},
-                "predicate": {
-                  "type": "and",
-                  "expressions": []
-                }
+                "predicate": null
               }
             ]
           }

diff --git a/...n/tests/goldenfiles/sorting_by_relationship_column_with_true_predicate/configuration.json b/...n/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
+}
diff --git a/...slation/tests/goldenfiles/sorting_by_relationship_column_with_true_predicate/request.json b/...slation/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": {}
+    }
+  }
+}
diff --git a/crates/query-engine/translation/tests/goldenfiles/sorting_by_relationship_count/request.json b/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
               }
             ]
           }

diff --git a/...ranslation/tests/snapshots/tests__sorting_by_relationship_column_with_true_predicate.snap b/...ranslation/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";
+
+{}
diff --git a/crates/query-engine/translation/tests/tests.rs b/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")