lynxtaa / awesome-graphql-client

GraphQL Client with file upload support for NodeJS and browser
https://npm.im/awesome-graphql-client
MIT License
59 stars 7 forks source link

Support for partial response #96

Closed redbit86k closed 1 month ago

redbit86k commented 1 month ago

Is your feature request related to a problem? Please describe. Currently it is not possible to have partial responses by using this graphql client as described in here

Describe the solution you'd like We are using this graph client for a big project and need to be able to receive 200 OK responses when graphql field errors occur. This is also described in the graphql spec and therefor should be a feasible feature addition to this client.

So the most easy thing is to have an additional option/parameter to specify how the client should handle responses with errors. Therefore we added allowPartialResponse?: boolean to the FetchOptions, because we think it allows to implement this feature in the most minimal invasive and backwards compatible way.

See #95 for all the implementation details.

This means the response would look like this:

HTTP 200 OK

{
  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [{ "line": 6, "column": 7 }],
      "path": ["hero", "heroFriends", 1, "name"]
    }
  ],
  "data": {
    "hero": {
      "name": "R2-D2",
      "heroFriends": [
        {
          "id": "1000",
          "name": "Luke Skywalker"
        },
        {
          "id": "1002",
          "name": null
        },
        {
          "id": "1003",
          "name": "Leia Organa"
        }
      ]
    }
  }
}

Describe alternatives you've considered The feature could be implemented in the following ways, as we thought of it:

Additional Context My team and me considered the following options on how to deal with the problem

  1. Change to a different graphql client which fulfills all of our requirements (bigger change we want to prevent currently)
  2. Fork this client and add the code to our repo (if it can be avoided we would prefer to just continue using the client as package)
  3. Implement the feature our self and make an MR

So we hope we can get this PR accepted, instead of having to go for one of the other options.

lynxtaa commented 1 month ago

Hi! Thanks for such a detailed description, it was really helpful.

I'm preparing a solution where I would always return a partial data from a requestSafe method. It would return like this

Promise<
    | { ok: true; data: TData; response: TRequestResult }
    | {
        ok: false
        data?: DeepNullable<TData>
        error: GraphQLRequestError<TRequestResult> | Error
      }
>

And GraphQLRequestError would have all the fieldErrors as a field

export class GraphQLRequestError {
    fieldErrors?: GraphQLFieldError[]
    // ...
}

So it would not require setting a new option.


Have you considered using GraphQL union types for expected errors (like validation errors) instead of throwing an error on server? The server could return something like

type ValidationError {
  code: String!
  message: String!
}

union Result = MyModel | ValidationError

type Mutation {
  foo: Result!
}
redbit86k commented 1 month ago

Thanks for responding so quickly! Im glad you found the description helpful :)

Would be quite similar to the solution from the MR, except the new option, so the nullable "data?" property should work for us 👍 I guess it would still return HTTP 200 ok in that case or are you planning on sending a different HTTP Statuscode?

What im unsure about is this:

export class GraphQLRequestError {
    fieldErrors?: GraphQLFieldError[]
    // ...
}

Since the property name "fieldErrors" would also be serialized in the response, it would kinda break with the GraphQL spec i had linked or maybe did i understand something wrong here?


Our backend is .NET and we are using the Hotchocolate GraphQL package (https://chillicream.com/docs/hotchocolate/v12). Mutations are not the main reason for why we need this. Rather we have some nested object trees and ACLs (like ABAC) for some properties/objects. So if the current user is not authorised to access those sub data, then the resolver throws an field exception for that currently. We dont just want to return null because then we dont easily know if there is no data or if the user can't access it.

Hope this makes our usecase more clear.

lynxtaa commented 1 month ago

https://github.com/lynxtaa/awesome-graphql-client/pull/98

I'm exposing a new field partialData for a requestSafe. Sorry for the confusion about GraphQLRequestError, I was talking about the class https://github.com/lynxtaa/awesome-graphql-client/blob/master/packages/awesome-graphql-client/src/GraphQLRequestError.ts for working with client errors.

So the usage would be like this:

const result = await client.requestSafe(query, variables)
if (!result.ok) {
  const { partialData, error } = result
  const fieldErrors = error instanceof GraphQLRequestError ? error.fieldErrors : undefined
  // ...
}
redbit86k commented 1 month ago

Should work for us, thanks for the quick implementation! :+1: Sry about the GraphQLRequestError confusion, i mixed up the client and server side there but everything is clear now.

lynxtaa commented 1 month ago

Solved in https://github.com/lynxtaa/awesome-graphql-client/releases/tag/v2.1.0