server: fix the nullability of object relationships (fix hasura/graphql-engine#7201)

When adding object relationships, we set the nullability of the generated GraphQL field based on whether the database backend enforces that the referenced data always exists. For manual relationships (corresponding to `manual_configuration`), the database backend is unaware of any relationship between data, and hence such fields are always set to be nullable.

For relationships generated from foreign key constraints (corresponding to `foreign_key_constraint_on`), we distinguish between two cases:

1. The "forward" object relationship from a referencing table (i.e. which has the foreign key constraint) to a referenced table. This should be set to be non-nullable when all referencing columns are non-nullable. But in fact, it used to set it to be non-nullable if *any* referencing column is non-nullable, which is only correct in Postgres when `MATCH FULL` is set (a flag we don't consider). This fixes that by changing a boolean conjunction to a disjunction.
2. The "reverse" object relationship from a referenced table to a referencing table which has the foreign key constraint. This should always be set to be nullable. But in fact, it used to always be set to non-nullable, as was reported in hasura/graphql-engine#7201. This fixes that.

Moreover, we have moved the computation of the nullability from `Hasura.RQL.DDL.Relationship` to `Hasura.GraphQL.Schema.Select`: this nullability used to be passed through the `riIsNullable` field of `RelInfo`, but for array relationships this information is not actually used, and moreover the remaining fields of `RelInfo` are already enough to deduce the nullability.

This also adds regression tests for both (1) and (2) above.

https://github.com/hasura/graphql-engine-mono/pull/2159

GitOrigin-RevId: 617f12765614f49746d18d3368f41dfae2f3e6ca

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 (Add entries below in the order of server, console, cli, docs, others)
 
+- server: fix nullability of object relationships (close #7201)
 - server: optimize SQL query generation with LIMITs (close #5745)
 - server: update non-existent event trigger, action and query collection error msgs (close #7396)
 - server: fix broken `untrack_function` for non-default source

server/src-lib/Hasura/Backends/Postgres/Translate/BoolExp.hs

@@ -134,7 +134,7 @@ convColRhs tableQual = \case
         bExps = map (mkFieldCompExp tableQual $ LColumn colFld) opExps
     return $ foldr (S.BEBin S.AndOp) (S.BELit True) bExps
 
-  AVRelationship (RelInfo _ _ colMapping relTN _ _ _) nesAnn -> do
+  AVRelationship (RelInfo _ _ colMapping relTN _ _) nesAnn -> do
     -- Convert the where clause on the relationship
     curVarNum <- get
     put $ curVarNum + 1

server/src-lib/Hasura/Backends/Postgres/Translate/Select.hs

@@ -758,7 +758,7 @@ processOrderByItems sourcePrefix' fieldAlias' similarArrayFields distOnCols = \c
           S.mkQIdenExp (mkBaseTableAlias sourcePrefix) $ toIdentifier $ pgiColumn pgColInfo
 
         AOCObjectRelation relInfo relFilter rest -> withWriteObjectRelation $ do
-          let RelInfo relName _ colMapping relTable _ _ _ = relInfo
+          let RelInfo relName _ colMapping relTable _ _ = relInfo
               relSourcePrefix = mkObjectRelationTableAlias sourcePrefix relName
               fieldName = mkOrderByFieldName relName
           (relOrderByAlias, relOrdByExp) <-
@@ -773,7 +773,7 @@ processOrderByItems sourcePrefix' fieldAlias' similarArrayFields distOnCols = \c
                )
 
         AOCArrayAggregation relInfo relFilter aggOrderBy -> withWriteArrayRelation $ do
-          let RelInfo relName _ colMapping relTable _ _ _ = relInfo
+          let RelInfo relName _ colMapping relTable _ _ = relInfo
               fieldName = mkOrderByFieldName relName
               relSourcePrefix = mkArrayRelationSourcePrefix sourcePrefix fieldAlias
                                 similarArrayFields fieldName

server/src-lib/Hasura/GraphQL/Schema/Select.hs

@@ -993,7 +993,7 @@ fieldSelection sourceName table maybePkeyColumns fieldInfo selectPermissions = d
                <&> IR.mkAnnColumnField columnInfo caseBoolExpUnpreparedValue
 
     FIRelationship relationshipInfo ->
-      concat . maybeToList <$> relationshipField sourceName relationshipInfo
+      concat . maybeToList <$> relationshipField sourceName table relationshipInfo
 
     FIComputedField computedFieldInfo ->
       maybeToList <$> computedField sourceName computedFieldInfo table selectPermissions
@@ -1006,30 +1006,73 @@ relationshipField
   :: forall b r m n
    . MonadBuildSchema b r m n
   => SourceName
+  -> TableName b
   -> RelInfo b
   -> m (Maybe [FieldParser n (AnnotatedField b)])
-relationshipField sourceName relationshipInfo = runMaybeT do
-  let otherTableName = riRTable     relationshipInfo
-      colMapping     = riMapping    relationshipInfo
-      relName        = riName       relationshipInfo
-      nullable       = riIsNullable relationshipInfo
-  otherTableInfo <- lift $ askTableInfo sourceName otherTableName
+relationshipField sourceName table RelInfo{..} = runMaybeT do
+  otherTableInfo <- lift $ askTableInfo sourceName riRTable
   remotePerms  <- MaybeT $ tableSelectPermissions otherTableInfo
-  relFieldName <- lift $ textToName $ relNameToTxt relName
-  case riType relationshipInfo of
+  relFieldName <- lift $ textToName $ relNameToTxt riName
+  case riType of
     ObjRel -> do
       let desc = Just $ G.Description "An object relationship"
       selectionSetParser <- lift $ tableSelectionSet sourceName otherTableInfo remotePerms
+      -- We need to set the correct nullability of our GraphQL field.  Manual
+      -- relationships are always nullable, and so are "reverse" object
+      -- relationships, i.e. Hasura relationships that are generated from a
+      -- referenced table to a referencing table.  For automatic forward object
+      -- relationships, i.e. the generated relationship from table1 to table2,
+      -- where table1 has a foreign key constraint, we have to do some work.
+      --
+      -- Specifically, we would like to mark the relationship generated from a
+      -- foreign key constraint from table1 to table2 to be non-nullable if
+      -- Postgres enforces that for each row in table1, a corresponding row in
+      -- table2 exists.  From the Postgres manual:
+      --
+      -- "Normally, a referencing row need not satisfy the foreign key
+      -- constraint if any of its referencing columns are null.  If MATCH FULL
+      -- is added to the foreign key declaration, a referencing row escapes
+      -- satisfying the constraint only if all its referencing columns are null
+      -- (so a mix of null and non-null values is guaranteed to fail a MATCH
+      -- FULL constraint)."
+      --
+      -- https://www.postgresql.org/docs/9.5/ddl-constraints.html#DDL-CONSTRAINTS-FK
+      --
+      -- Since we don't store MATCH FULL in the RQL representation of the
+      -- database, the closest we can get is to only set the field to be
+      -- non-nullable if _all_ of the columns that reference the foreign table
+      -- are non-nullable.  Strictly speaking, we could do slightly better by
+      -- setting the field to be non-nullable also if the foreign key has MATCH
+      -- FULL set, and _any_ of the columns is non-nullable.  But we skip doing
+      -- this since, as of writing this, the old code used to make the wrong
+      -- decision about nullability of joint foreign keys entirely, so this is
+      -- probably not a very widely used mode of use.  The impact of this
+      -- suboptimality is merely that in introspection some fields might get
+      -- marked nullable which are in fact known to always be non-null.
+      nullable <- case (riIsManual, riInsertOrder) of
+        -- Automatically generated forward relationship
+        (False, BeforeParent) -> do
+          tableInfo <- askTableInfo @b sourceName table
+          let columns = Map.keys riMapping
+              fieldInfoMap = _tciFieldInfoMap $ _tiCoreInfo tableInfo
+              findColumn col = Map.lookup (fromCol @b col) fieldInfoMap ^? _Just._FIColumn
+          -- Fetch information about the referencing columns of the foreign key
+          -- constraint
+          colInfo <- traverse findColumn columns
+                     `onNothing` throw500 "could not find column info in schema cache"
+          pure $ boolToNullable $ any pgiIsNullable colInfo
+        -- Manual or reverse relationships are always nullable
+        _ -> pure Nullable
       pure $ pure $ case nullable of { Nullable -> id; NotNullable -> P.nonNullableField} $
         P.subselection_ relFieldName desc selectionSetParser
-             <&> \fields -> IR.AFObjectRelation $ IR.AnnRelationSelectG relName colMapping $
-                    IR.AnnObjectSelectG fields otherTableName $
+             <&> \fields -> IR.AFObjectRelation $ IR.AnnRelationSelectG riName riMapping $
+                    IR.AnnObjectSelectG fields riRTable $
                     IR._tpFilter $ tablePermissionsInfo remotePerms
     ArrRel -> do
       let arrayRelDesc = Just $ G.Description "An array relationship"
       otherTableParser <- lift $ selectTable sourceName otherTableInfo relFieldName arrayRelDesc remotePerms
       let arrayRelField = otherTableParser <&> \selectExp -> IR.AFArrayRelation $
-            IR.ASSimple $ IR.AnnRelationSelectG relName colMapping selectExp
+            IR.ASSimple $ IR.AnnRelationSelectG riName riMapping selectExp
           relAggFieldName = relFieldName <> $$(G.litName "_aggregate")
           relAggDesc      = Just $ G.Description "An aggregate relationship"
       remoteAggField <- lift $ selectTableAggregate sourceName otherTableInfo relAggFieldName relAggDesc remotePerms
@@ -1044,8 +1087,8 @@ relationshipField sourceName relationshipInfo = runMaybeT do
             relConnectionDesc = Just $ G.Description "An array relationship connection"
         MaybeT $ lift $ selectTableConnection sourceName otherTableInfo relConnectionName relConnectionDesc pkeyColumns remotePerms
       pure $ catMaybes [ Just arrayRelField
-                       , fmap (IR.AFArrayRelation . IR.ASAggregate . IR.AnnRelationSelectG relName colMapping) <$> remoteAggField
-                       , fmap (IR.AFArrayRelation . IR.ASConnection . IR.AnnRelationSelectG relName colMapping) <$> remoteConnectionField
+                       , fmap (IR.AFArrayRelation . IR.ASAggregate . IR.AnnRelationSelectG riName riMapping) <$> remoteAggField
+                       , fmap (IR.AFArrayRelation . IR.ASConnection . IR.AnnRelationSelectG riName riMapping) <$> remoteConnectionField
                        ]
 
 -- | Computed field parser

server/src-lib/Hasura/RQL/DDL/Relationship.hs

@@ -95,9 +95,8 @@ objRelP2Setup
   -> TableName b
   -> HashMap (TableName b) (HashSet (ForeignKey b))
   -> RelDef (ObjRelUsing b)
-  -> FieldInfoMap (ColumnInfo b)
   -> m (RelInfo b, [SchemaDependency])
-objRelP2Setup source qt foreignKeys (RelDef rn ru _) fieldInfoMap = case ru of
+objRelP2Setup source qt foreignKeys (RelDef rn ru _) = case ru of
   RUManual rm -> do
     let refqt = rmTable rm
         (lCols, rCols) = unzip $ HM.toList $ rmColumns rm
@@ -110,7 +109,7 @@ objRelP2Setup source qt foreignKeys (RelDef rn ru _) fieldInfoMap = case ru of
                                               reason
         dependencies = map (mkDependency qt DRLeftColumn) lCols
                     <> map (mkDependency refqt DRRightColumn) rCols
-    pure (RelInfo rn ObjRel (rmColumns rm) refqt True Nullable io, dependencies)
+    pure (RelInfo rn ObjRel (rmColumns rm) refqt True io, dependencies)
   RUFKeyOn (SameTable columns) -> do
     foreignTableForeignKeys <- findTable @b qt foreignKeys
     ForeignKey constraint foreignTable colMap <- getRequiredFkey columns (HS.toList foreignTableForeignKeys)
@@ -129,10 +128,7 @@ objRelP2Setup source qt foreignKeys (RelDef rn ru _) fieldInfoMap = case ru of
                 $ SOITable @b foreignTable)
               DRRemoteTable
           ] <> fmap (drUsingColumnDep @b source qt) (toList columns)
-    colInfo <- traverse ((`HM.lookup` fieldInfoMap) . fromCol @b) columns
-                 `onNothing` throw500 "could not find column info in schema cache"
-    let nullable = boolToNullable $ all pgiIsNullable colInfo
-    pure (RelInfo rn ObjRel colMap foreignTable False nullable BeforeParent, dependencies)
+    pure (RelInfo rn ObjRel colMap foreignTable False BeforeParent, dependencies)
   RUFKeyOn (RemoteTable remoteTable remoteCols) ->
     mkFkeyRel ObjRel AfterParent source rn qt remoteTable remoteCols foreignKeys
 
@@ -161,7 +157,7 @@ arrRelP2Setup foreignKeys source qt (RelDef rn ru _) = case ru of
                                     $ TOCol @b c)
                                   DRRightColumn)
                   rCols
-    pure (RelInfo rn ArrRel (rmColumns rm) refqt True Nullable AfterParent, deps)
+    pure (RelInfo rn ArrRel (rmColumns rm) refqt True  AfterParent, deps)
   RUFKeyOn (ArrRelUsingFKeyOn refqt refCols) ->
     mkFkeyRel ArrRel AfterParent source rn qt refqt refCols foreignKeys
 
@@ -195,11 +191,12 @@ mkFkeyRel relType io source rn sourceTable remoteTable remoteColumns foreignKeys
                 $ SOITable @b remoteTable)
               DRRemoteTable
           ] <> fmap (drUsingColumnDep @b source remoteTable) (toList remoteColumns)
-    pure (RelInfo rn relType (reverseHM colMap) remoteTable False NotNullable io, dependencies)
+    pure (RelInfo rn relType (reverseHM colMap) remoteTable False io, dependencies)
   where
     reverseHM :: Eq y => Hashable y => HashMap x y -> HashMap y x
     reverseHM = HM.fromList . fmap swap . HM.toList
 
+-- | Try to find a foreign key constraint, identifying a constraint by its set of columns
 getRequiredFkey
   :: (QErrM m, Backend b)
   => NonEmpty (Column b)
@@ -208,8 +205,8 @@ getRequiredFkey
 getRequiredFkey cols fkeys =
   case filteredFkeys of
     [k] -> return k
-    []  -> throw400 ConstraintError "no foreign constraint exists on the given column"
-    _   -> throw400 ConstraintError "more than one foreign key constraint exists on the given column"
+    []  -> throw400 ConstraintError "no foreign constraint exists on the given column(s)"
+    _   -> throw400 ConstraintError "more than one foreign key constraint exists on the given column(s)"
   where
     filteredFkeys = filter ((== HS.fromList (toList cols)) . HM.keysSet . _fkColumnMapping) fkeys
 

server/src-lib/Hasura/RQL/DDL/Schema/Cache/Fields.hs

@@ -52,10 +52,10 @@ addNonColumnFields = proc ( allSources
                           ) -> do
   objectRelationshipInfos
     <- buildInfoMapPreservingMetadata
-         (_rdName . (^. _4))
-         (\(s, _, t, c) -> mkRelationshipMetadataObject @b ObjRel (s,t,c))
+         (_rdName . (^. _3))
+         (\(s, t, c) -> mkRelationshipMetadataObject @b ObjRel (s,t,c))
          buildObjectRelationship
-    -< (_tciForeignKeys <$> rawTableInfo, map (source, columns, _nctiTable,) _nctiObjectRelationships)
+    -< (_tciForeignKeys <$> rawTableInfo, map (source, _nctiTable,) _nctiObjectRelationships)
 
   arrayRelationshipInfos
     <- buildInfoMapPreservingMetadata
@@ -159,13 +159,12 @@ buildObjectRelationship
      )
   => ( HashMap (TableName b) (HashSet (ForeignKey b))
      , ( SourceName
-       , FieldInfoMap (ColumnInfo b)
        , TableName b
        , ObjRelDef b
        )
      ) `arr` Maybe (RelInfo b)
-buildObjectRelationship = proc (fkeysMap, (source, columns, table, relDef)) -> do
-  let buildRelInfo def = objRelP2Setup source table fkeysMap def columns
+buildObjectRelationship = proc (fkeysMap, (source, table, relDef)) -> do
+  let buildRelInfo def = objRelP2Setup source table fkeysMap def
   buildRelationship -< (source, table, buildRelInfo, ObjRel, relDef)
 
 buildArrayRelationship

server/src-lib/Hasura/RQL/DML/Select.hs

@@ -53,7 +53,7 @@ convSelCol fieldInfoMap _ (SCExtRel rn malias selQ) = do
   let pgWhenRelErr = "only relationships can be expanded"
   relInfo <- withPathK "name" $
     askRelType fieldInfoMap rn pgWhenRelErr
-  let (RelInfo _ _ _ relTab _ _ _) = relInfo
+  let (RelInfo _ _ _ relTab _ _) = relInfo
   (rfim, rspi) <- fetchRelDet rn relTab
   resolvedSelQ <- resolveStar rfim rspi selQ
   pure [ECRel rn malias resolvedSelQ]
@@ -257,7 +257,7 @@ convExtRel fieldInfoMap relName mAlias selQ sessVarBldr prepValBldr = do
   -- Point to the name key
   relInfo <- withPathK "name" $
     askRelType fieldInfoMap relName pgWhenRelErr
-  let (RelInfo _ relTy colMapping relTab _ _ _) = relInfo
+  let (RelInfo _ relTy colMapping relTab _ _) = relInfo
   (relCIM, relSPI) <- fetchRelDet relName relTab
   annSel <- convSelectQ relTab relCIM relSPI selQ sessVarBldr prepValBldr
   case relTy of

server/src-lib/Hasura/RQL/Types/Relationship.hs

@@ -204,7 +204,6 @@ data RelInfo (b :: BackendType)
   , riMapping     :: !(HashMap (Column b) (Column b))
   , riRTable      :: !(TableName b)
   , riIsManual    :: !Bool
-  , riIsNullable  :: !Nullable
   , riInsertOrder :: !InsertOrder
   } deriving (Generic)
 deriving instance Backend b => Show (RelInfo b)

server/tests-py/queries/graphql_introspection/nullable_object_relationship/nullability.yaml

@@ -0,0 +1,81 @@
+description: Test that object relationships have the correct nullability
+url: /v1/graphql
+status: 200
+query:
+  query: |
+    query {
+      table1 : __type(name: "table1") {
+        fields {
+          name
+          type {
+            kind
+          }
+        }
+      }
+      table2 : __type(name: "table2") {
+        fields {
+          name
+          type {
+            kind
+          }
+        }
+      }
+      joint_foreign_key : __type(name: "joint_foreign_key") {
+        fields {
+          name
+          type {
+            kind
+          }
+        }
+      }
+    }
+response:
+  data:
+    table1:
+      fields:
+      - name: id
+        type:
+          kind: NON_NULL
+      - name: name
+        type:
+          kind: SCALAR
+      - name: via_table2
+        type:
+          kind: OBJECT    # this is what we're testing
+      - name: via_table2_not_null
+        type:
+          kind: OBJECT    # this is what we're testing
+    table2:
+      fields:
+      - name: id
+        type:
+          kind: NON_NULL
+      - name: name
+        type:
+          kind: SCALAR
+      - name: table1_id
+        type:
+          kind: SCALAR
+      - name: table1_id_not_null
+        type:
+          kind: NON_NULL
+      - name: via_table1
+        type:
+          kind: OBJECT    # this is what we're testing
+      - name: via_table1_not_null
+        type:
+          kind: NON_NULL  # this is what we're testing
+    joint_foreign_key:
+      fields:
+      - name: id
+        type:
+          kind: NON_NULL
+      - name: table2
+        type:
+          kind: OBJECT    # This is what we're testing
+      - name: table2_name
+        type:
+          kind: NON_NULL
+      - name: table2_table1_id
+        type:
+          kind: SCALAR

server/tests-py/queries/graphql_introspection/nullable_object_relationship/setup.yaml

@@ -10,7 +10,15 @@ args:
       CREATE TABLE table2 (
         id serial primary key,
         name text,
-        table1_id INTEGER REFERENCES table1(id)
+        table1_id INTEGER UNIQUE REFERENCES table1(id),
+        table1_id_not_null INTEGER NOT NULL UNIQUE REFERENCES table1(id),
+        unique (name, table1_id)
+      );
+      CREATE TABLE joint_foreign_key (
+        id serial primary key,
+        table2_name text not null,
+        table2_table1_id integer null,
+        foreign key (table2_name, table2_table1_id) references table2 (name, table1_id)
       );
 
 - type: track_table
@@ -21,6 +29,10 @@ args:
   version: 2
   args:
     table: table2
+- type: track_table
+  version: 2
+  args:
+    table: joint_foreign_key
 
 - type: create_object_relationship
   args:
@@ -28,3 +40,39 @@ args:
     table: table2
     using:
       foreign_key_constraint_on: table1_id
+
+- type: create_object_relationship
+  args:
+    name: via_table1_not_null
+    table: table2
+    using:
+      foreign_key_constraint_on: table1_id_not_null
+
+- type: create_object_relationship
+  args:
+    name: via_table2
+    table: table1
+    using:
+      foreign_key_constraint_on:
+        table: table2
+        columns:
+        - table1_id
+
+- type: create_object_relationship
+  args:
+    name: via_table2_not_null
+    table: table1
+    using:
+      foreign_key_constraint_on:
+        table: table2
+        columns:
+        - table1_id_not_null
+
+- type: create_object_relationship
+  args:
+    name: table2
+    table: joint_foreign_key
+    using:
+      foreign_key_constraint_on:
+      - table2_name
+      - table2_table1_id

server/tests-py/queries/graphql_introspection/nullable_object_relationship/teardown.yaml

@@ -3,3 +3,4 @@ args:
   sql: |
     DROP TABLE table1 cascade;
     DROP TABLE table2 cascade;
+    DROP TABLE joint_foreign_key cascade;

server/tests-py/queries/v1/relationships/array_relationship_col_not_foreign_key_error.yaml

@@ -18,13 +18,13 @@ response:
           name: author
       reason:
         'Inconsistent object: in table "author": in relationship "articles": no
-        foreign constraint exists on the given column'
+        foreign constraint exists on the given column(s)'
       name: array_relation articles in table author in source default
       type: array_relation
   path: $.args
   error:
     'Inconsistent object: in table "author": in relationship "articles": no foreign
-    constraint exists on the given column'
+    constraint exists on the given column(s)'
   code: invalid-configuration
 query:
   type: create_array_relationship

server/tests-py/queries/v1/relationships/object_relationship_col_not_foreign_key_error.yaml

@@ -14,13 +14,13 @@ response:
           name: article
       reason:
         'Inconsistent object: in table "article": in relationship "author": no
-        foreign constraint exists on the given column'
+        foreign constraint exists on the given column(s)'
       name: object_relation author in table article in source default
       type: object_relation
   path: $.args
   error:
     'Inconsistent object: in table "article": in relationship "author": no foreign
-    constraint exists on the given column'
+    constraint exists on the given column(s)'
   code: invalid-configuration
 query:
   type: create_object_relationship

server/tests-py/test_graphql_introspection.py

@@ -40,21 +40,8 @@ class TestNullableObjectRelationshipInSchema:
     def dir(cls):
         return "queries/graphql_introspection/nullable_object_relationship"
 
-    def test_introspection(self, hge_ctx):
-        with open(self.dir() + "/../introspection.yaml") as c:
-            conf = yaml.safe_load(c)
-        resp, _ = check_query(hge_ctx, conf)
-        for t in resp['data']['__schema']['types']:
-            if t['name'] == 'table2':
-                for fld in t['fields']:
-                    if fld['name'] == 'via_table1':
-                        # graphql schema introspection doesn't explictly mark
-                        # the fields to be nullable (it does in the non-null
-                        # case). So checking this should be sufficient to detect
-                        # if its nullable. If this is not nullable, the
-                        # top-level kind would be `NON_NULL` and its `ofType`
-                        # would have the actual type
-                        assert fld['type']['kind'] == 'OBJECT'
+    def test_introspection_both_directions_both_nullabilities(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + "/nullability.yaml")
 
 def getTypeNameFromType(typeObject):
     if typeObject['name'] != None: