fix(stitch) fix merged error paths from batched arrays

[gmac]

At present there's a bug in error paths mapped from batched arrays... an error taken from a batched array always ends with its position in the underlying batch set, even though that position is not applicable within the stitched document path:

{
  "errors": [
    {
      "message": "nope",
      "locations": [],
      "path": [
        "parent",
        "thing",
         0
      ]
    }
  ],
  "data": {
    "parent": {
      "thing": null
    }
  }
}

This checks the return type of the delegation query and omits the final path position from array values when they check out as integers.

cc @yaacovCR

[changeset-bot]

🦋 Changeset detected

Latest commit: 80683d9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@graphql-tools/batch-delegate	Patch
@graphql-tools/delegate	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[gmac]

@yaacovCR – I'm also very curious about the lines I have commented out here: https://github.com/ardatan/graphql-tools/pull/2288/files#diff-b5b7aa7f61f68d5eb95eec0f2a889d9afe4cf5d043547167ee3cee7bb127ea56R138-R143

That enables a single-record query, which appears to have higher fidelity merge results than the array-batched counterpart. For example, using args with the thing field gives this:

{
  "errors": [
    {
      "message": "nope",
      "locations": [],
      "path": [
        "parent",
        "thing",
        "name"
      ]
    },
    {
      "message": "nope",
      "locations": [],
      "path": [
        "parent",
        "thing",
        "desc"
      ]
    }
  ],
  "data": {
    "parent": {
      "thing": {
        "name": null,
        "desc": null,
        "id": "23"
      }
    }
  }
}

While using argsFromKeys with the things field gives this:

{
  "errors": [
    {
      "message": "nope",
      "locations": [],
      "path": [
        "parent",
        "thing"
      ]
    }
  ],
  "data": {
    "parent": {
      "thing": null
    }
  }
}

Technically I'd expect single-record versus array-batched to produce analogous results. The single-record merge actually returned partial data (the local ID), and then gave two field errors, while the array-batched merge failed resolving the entire object.

The single-record result is slightly more granular and therefor preferable from the results standpoint, but is less efficient to process than array batching. Is there a simple way that the more efficient array-batched version could match the granularity of the single-record result? If not, it’s not a big deal. I’ll take the efficiency of array batching over the granularity of a failed record, to which half a record is generally about as useful as no record at all.

[yaacovCR]

Ack, you are completely right about this!

The "right" solution is to either traverse the result tree on every batchDelegateToSchema call and adjust every embedded error OR to somehow alert the CheckResult... transform that when it embeds the errors into the object by full path, it should transform the errors first.... But this should only happen when using batchDelegateToSchema and we know that the array result is actually not being returned as an array.... When delegating from list to list, error positions within the list need to be saved....

We have the ability to alter the default transforms passed to delegateToSchema using the delegation bindings argument...

But rather than changing all of that, keeping two setsb of bindings in sync, we should try to think of a simpler option

[gmac]

I don’t have strong feels... adjusting errors on the way out of batchDelegateToSchema seems easy enough. I was going to ask, is there a way to recognize ‘argsFromKeys’ within delegation bindings? If so, just recognizing that aspect of active subschema config would be sufficient. I suppose that doesn’t fix the issue for batchDelegateToSchema outside the context of stitching though. :/

…

On Sat, Nov 28, 2020 at 4:28 PM Yaacov Rydzinski ***@***.***> wrote: Ack, you are completely right about this! The "right" solution is to either traverse the result tree on every batchDelegateToSchema call and adjust every embedded error OR to somehow alert the CheckResult... transform that when it embeds the errors into the object by full path, it should transform the errors first.... But this should only happen when using batchDelegateToSchema and we know that the array result is actually not being returned as an array.... When delegating from list to list, error positions within the list need to be saved.... We have the ability to alter the default transforms passed to delegateToSchema using the delegation bindings argument... But rather than changing all of that, keeping two setsb of bindings in sync, we should try to think of a simpler option — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2288 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFRROB7TNJAEFIJVRAXGVLSSFTOTANCNFSM4UFRFQ2Q> .