WIP tirumaraiselvan#27

preserve remoteResp errors in insertBatchResults
misc renaming for clarity
some minor refactorings for brevity/clarity
TODO extractRemoteRelArguments

server/src-lib/Hasura/GraphQL/Execute/RemoteJoins.hs

@@ -238,7 +238,7 @@ joinResults :: GQRespValue
             -> [(Batch, EncJSON)]
             -> GQRespValue
 joinResults hasuraValue0 =
-  foldl (uncurry . insertBatchResults) hasuraValue0 . map (fmap jsonToGQRespVal)
+  foldl' (uncurry . insertBatchResults) hasuraValue0 . map (fmap jsonToGQRespVal)
   where
     jsonToGQRespVal = either mkErr id . parseGQRespValue
     mkErr = appendJoinError emptyResp . ("joinResults: eitherDecode: " <>) . fromString
@@ -249,20 +249,25 @@ emptyResp = GQRespValue OJ.empty VB.empty
 -- | Insert at path, index the value in the larger structure.
 insertBatchResults ::
      GQRespValue
+  -- ^ Original hasura response with accumulated remote responses
   -> Batch
   -> GQRespValue
   -> GQRespValue
-insertBatchResults hasuraResp Batch{..} remoteResp =
-  case inHashmap batchRelFieldPath hasuraData0 remoteData0 of
-    Left err  -> appendJoinError hasuraResp err
-    Right val -> GQRespValue (fst val) (_gqRespErrors hasuraResp)
+insertBatchResults accumResp Batch{..} remoteResp =
+  -- It's not clear what to do about errors here, or when to short-circuit so
+  -- try to do as much computation as possible for now:
+  case inHashmap batchRelFieldPath accumData0 remoteData0 of
+    Left err  -> appendJoinError accumRespWithRemoteErrs err
+    Right (val, _) -> set gqRespData val accumRespWithRemoteErrs
   where
-    hasuraData0 = _gqRespData hasuraResp
+    accumRespWithRemoteErrs = gqRespErrors <>~ _gqRespErrors remoteResp $ accumResp 
+    accumData0 = _gqRespData accumResp
     -- The 'sortOn' below is not strictly necessary by the spec, but
     -- implementations may not guarantee order of results matching
     -- order of query.
     -- Since remote results are aliased by keys of the form 'remote_result_1', 'remote_result_2',
     -- we can sort by the keys for a ordered list
+    -- TODO look again at this...
     remoteData0 =
       map snd $ sortOn fst $ OJ.toList $ _gqRespData remoteResp
 
@@ -274,29 +279,29 @@ insertBatchResults hasuraResp Batch{..} remoteResp =
       -> [OJ.Value]
       -- ^ The remote result data with no keys, only the values sorted by key
       -> Either GQJoinError (OJ.Object, [OJ.Value])
-    inHashmap (RelFieldPath p) hasuraData remoteDataVals = case p of
+    inHashmap (RelFieldPath p) accumData remoteDataVals = case p of
       Seq.Empty ->
         case remoteDataVals of
-          [] -> Left $ err <> showHashO hasuraData
+          [] -> Left $ err <> showHashO accumData
             where err = case cardinality of
                           One -> "Expected one remote object but got none, while traversing "
                           Many -> "Expected many objects but got none, while traversing "
 
           (remoteDataVal:rest)
             | cardinality == One && not (null rest) ->
                 Left $
-                  "Expected one remote object but got many, while traversing " <> showHashO hasuraData
+                  "Expected one remote object but got many, while traversing " <> showHashO accumData
             | otherwise ->
-                (, rest) <$> spliceRemote remoteDataVal hasuraData
+                (, rest) <$> spliceRemote remoteDataVal accumData
 
       ((idx, G.Alias (G.Name key)) Seq.:<| rest) ->
-        case OJ.lookup key hasuraData of
+        case OJ.lookup key accumData of
           Nothing ->
             Left $
-              "Couldn't find expected key " <> fromString (show key) <> " in " <> showHashO hasuraData <>
-              ", while traversing " <> showHashO hasuraData0
+              "Couldn't find expected key " <> fromString (show key) <> " in " <> showHashO accumData <>
+              ", while traversing " <> showHashO accumData0
           Just currentValue ->
-            first (\newValue -> OJ.insert (idx, key) newValue hasuraData)
+            first (\newValue -> OJ.insert (idx, key) newValue accumData)
               <$> inValue rest currentValue remoteDataVals
 
     -- TODO Brandon note:
@@ -311,14 +316,14 @@ insertBatchResults hasuraResp Batch{..} remoteResp =
       -> Either GQJoinError (OJ.Value, [OJ.Value])
     inValue path currentValue remoteDataVals =
       case currentValue of
-        OJ.Object hasuraData ->
+        OJ.Object accumData ->
           first OJ.Object <$>
-            inHashmap (RelFieldPath path) hasuraData remoteDataVals
+            inHashmap (RelFieldPath path) accumData remoteDataVals
 
         OJ.Array hasuraRowValues -> first (OJ.Array . Vec.fromList . reverse) <$>
           let foldHasuraRowObjs f = foldM f_onObj ([], remoteDataVals) hasuraRowValues
                 where
-                  f_onObj tup (OJ.Object hasuraData) = f tup hasuraData
+                  f_onObj tup (OJ.Object accumData) = f tup accumData
                   f_onObj _    hasuraRowValue = Left $
                     "expected array of objects in " <> showHash hasuraRowValue
            in case path of
@@ -328,24 +333,24 @@ insertBatchResults hasuraResp Batch{..} remoteResp =
                 Seq.Empty ->
                   case cardinality of
                     Many -> foldHasuraRowObjs $
-                      \(hasuraRowsSoFar, remainingRemotes) hasuraData ->
+                      \(hasuraRowsSoFar, remainingRemotes) accumData ->
                          case remainingRemotes of
                            [] ->
                              Left $
-                               "no remote objects left for joining at " <> showHashO hasuraData
+                               "no remote objects left for joining at " <> showHashO accumData
                            (remoteDataVal:rest) -> do
-                             spliced <- spliceRemote remoteDataVal hasuraData
+                             spliced <- spliceRemote remoteDataVal accumData
                              pure
                                (OJ.Object spliced : hasuraRowsSoFar , rest)
                     One ->
                       Left "Cardinality mismatch: found array in hasura value, but expected object"
 
                 nonEmptyPath -> foldHasuraRowObjs $
-                  \(hasuraRowsSoFar, remainingRemotes) hasuraData ->
+                  \(hasuraRowsSoFar, remainingRemotes) accumData ->
                      first (\hasuraRowHash'-> OJ.Object hasuraRowHash' : hasuraRowsSoFar) <$>
                        inHashmap
                          (RelFieldPath nonEmptyPath)
-                         hasuraData
+                         accumData
                          remainingRemotes
 
         OJ.Null -> Right (OJ.Null, remoteDataVals)
@@ -355,18 +360,18 @@ insertBatchResults hasuraResp Batch{..} remoteResp =
 
     -- splice the remote result into the hasura result with the proper key (and
     -- at the right index), removing phantom fields
-    spliceRemote remoteDataVal hasuraData = do
+    spliceRemote remoteDataVal accumData = do
       peeledValue <- peelOffNestedFields batchNestedFields remoteDataVal
       pure $
         -- TODO Brandon note:
         -- document rrPhantomFields/batchPhantoms so that we can understand if or why it's okay that the OJ.delete might silently be a noop here:
         foldl' (flip OJ.delete)
                 -- TODO Brandon note:
-                -- document why we know OJ.insert might not silently clobber an existing field in hasuraData; e.g. with an assertion here, or directing reader to someplace validation is performed
+                -- document why we know OJ.insert might not silently clobber an existing field in accumData; e.g. with an assertion here, or directing reader to someplace validation is performed
                (OJ.insert
                   (batchRelationshipKeyIndex, batchRelationshipKeyToMake)
                   peeledValue
-                  hasuraData)
+                  accumData)
                batchPhantoms
 
     -- logging helpers:
@@ -568,13 +573,15 @@ extractRemoteRelArguments ::
   -> EncJSON
   -> [RemoteRelField]
   -> Either GQRespValue ( GQRespValue , [ BatchInputs ])
+-- ^ TODO I think we can just return (GQRespValue, [BatchInputs]) but need to look closer at the callsite.
 extractRemoteRelArguments remoteSchemaMap hasuraJson rels =
   -- TODO refactor with MonadError
   case parseGQRespValue hasuraJson of
     Left (fromString-> err) ->
       Left $
       appendJoinError emptyResp $ "decode error: " <> err
-    Right gqResp@GQRespValue{_gqRespData} -> do
+    -- TODO make sure we're not throwing away any _gqRespErrors below:
+    Right gqResp@GQRespValue{..} -> do
       let hashE =
             execStateT
               (extractFromResult One keyedRemotes (OJ.Object _gqRespData))
@@ -596,15 +603,16 @@ extractRemoteRelArguments remoteSchemaMap hasuraJson rels =
                                  (rrRemoteField remoteRel)))
                            remoteSchemaMap of
                       Just remoteSchemaCtx ->
-                        pure (remoteRel, (rscInfo remoteSchemaCtx), rows)
+                        -- TODO the first two parts of this 3-tuple are just discarded immediately below; are we missing functionality here?
+                        pure (remoteRel, rscInfo remoteSchemaCtx, rows)
                       Nothing ->
                         Left $
                           appendJoinError gqResp "could not find remote schema info"
 
           pure (gqResp, map (\(_, _, batchInputs ) -> batchInputs) $ Map.elems remotes)
   where
     keyedRemotes = zip (fmap RemoteRelKey [0 ..]) rels
-    keyedMap = Map.fromList (toList keyedRemotes)
+    keyedMap = Map.fromList keyedRemotes
 
 data BatchInputs =
   BatchInputs
@@ -689,7 +697,7 @@ extractFromResult cardinality keyedRemotes value =
              outerHashmap
              keys)
         mempty
-        (toList (fmap peelRemoteKeys keyedRemotes))
+        (fmap peelRemoteKeys keyedRemotes)
 
 -- | Peel one layer of expected keys from the remote to be looked up
 -- at the current level of the result object.

server/src-lib/Hasura/GraphQL/Transport/HTTP.hs

@@ -58,10 +58,7 @@ runGQ reqId userInfo reqHdrs req = do
             Right (initValue, remoteBatchInputs) -> do
               let joinParamsPartial = E.JoinParams G.OperationTypeQuery -- TODO: getOpType
                   resolvedPlansWithBatches =
-                    map
-                      (\(batchInput, unresolvedPlan) ->
-                         E.mkQuery (joinParamsPartial batchInput) unresolvedPlan)
-                      (zip remoteBatchInputs unresolvedPlans)
+                    zipWith (E.mkQuery . joinParamsPartial) remoteBatchInputs unresolvedPlans 
                       -- TODO ^ can we be sure we're not throwing away a tail of either of these lists?
               results <-
                 traverse
@@ -129,9 +126,8 @@ runHasuraGQ reqId query userInfo resolvedOp = do
 
 -- | Merge the list of response objects by the @data@ key.
 mergeResponseData :: [EncJSON] -> Either String EncJSON
-mergeResponseData responses = do
-  resps <- traverse E.parseGQRespValue responses
-  fmap E.encodeGQRespValue (mergeGQResp resps)
+mergeResponseData =
+  fmap E.encodeGQRespValue . mergeGQResp <=< traverse E.parseGQRespValue
 
   where mergeGQResp = flip foldM E.emptyResp $ \respAcc E.GQRespValue{..} ->
           respAcc & E.gqRespErrors <>~ _gqRespErrors