Ideas for standardizing the errors returned to clients.

[xoldd]

Right now I'm outlining the structure for graphql error object that will be returned for client's input.

For the following graphql schema:-

input DummyLocationInput {
  latitude: String!
  longitude: String!
}

input DummyMutationInput {
  username: String!
  location: DummyLocationInput!
}

type Mutation {
  dummyMutation(id: ID!, input: DummyMutationInput!): DummyResult
}

The errors can be mapped like this:-

const GraphQLError = {
  message: "Invalid resolver argument values provided.",
  path: ["dummyMutation"],
  extensions: {
    code: "BAD_USER_INPUT",

    fieldErrors: {
      id: {<----------------------------------- Scalar argument id section
        message:"",

        // ...other constraints here
      },

      input: {<----------------------------------- Argument input section
        message: "",
        // ...other constraints here,

        fieldErrors: {
          username: {<----------------------------------- Nested scalar argument username section
            message: "",
            // ...other constraints here
          },

          location: {<----------------------------------- Nested input argument location section
            message: "",

            // ...other constraints here

            fieldErrors: {
              latitude: {<----------------------------------- Nested scalar argument latitude section
                message: "",

                // ...other constraints here
              },

              longitude: {<----------------------------------- Nested scalar argument longitude section
                message: "",

                // ...other constraints here
              },
            },
          },
        },
      },
    },
  },
};

This is only a trivial example, the process is same for queries, mutations and nested resolvers, no matter how many levels of nested arguments you have, or what their type is.

You see that path list on the root object? That tells us the resolver/field in which the error happened. If it was a nested resolver/field the path list would look like this ["dummyMutation", "nestedResolverName"].

Most of the time you'd not need a nested resolver for a scalar fields, but you can even implement that if it's preferred. A use case would be to protect some data, and throw error if the user who's accessing that field isn't authorized to do so. For example the email field of a user, shouldn't be accessible to anyone but the user who it belongs to and higher ups like admins.

This design would help us return all errors that are present in user's input in one go. But implementing a validator/transformer function for this structure would be equally as hard.

Another way to return errors could be one by one for each argument. Though it would be less flexible and would need many cycles to convey all errors that client's input has.

I had given thought to defining errors as a part of schema itself, and though it looks like an attractive approach, there's way too much boilerplate required for it. And since it's not a part of graphql specification, no tool in graphql ecosystem supports it in any way, so you'd need a boilerplaty approach on both client and server side to handle these errors.

[kb-0311]

@xoldyckk

I had given thought to defining errors as a part of schema itself, and though it looks like an attractive approach, there's way too much boilerplate required for it. And since it's not a part of graphql specification, no tool in graphql ecosystem supports it in any way, so you'd need a boilerplaty approach on both client and server side to handle these errors.

Are you talking about the unions+interfaces approach we discussed few days back?

I agree it is a lot of boilerplates, that's true, but I still feel like the added benefit of Type-safety and customizable error behavior are a considerable pro when considering that.

Not only that but paired up with graphql-codegen the approach becomes extremely worthy of consideration. https://the-guild.dev/blog/graphql-error-handling-with-fp#typesafe-error-handling-on-the-client-side . (The entire blog is pretty informative)

I would say not to dismiss that approach only based on the boilerplate code thingy. If it is developed in a more modular approach yes, even though the API code will increase complexity a little, the good thing will be our API will be safer and the error handling will be a lot more consistent in the future anytime.

The client-side boilerplate will be minimal.

[xoldd]

@kb-0311 Read this again:-https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors

I know you're suggesting to implement Stage 5: Result Types, but you're overlooking the cons of it. Read through its cons again.

Stage 5: Result Types
Stage 6: Error Union List
Stage 6a: Error Union List + Interface

These three are the only good approaches to handling graphql errors by defining them in the schema itself. Basically the last approach Stage 6a is the most flexible, but it is equally as verbose. Your approach is comparatively easier to implement but it has cons too. But we can ignore those cons because talawa-api isn't being served to a wide variety of applications, there are only two clients, and it can be easily kept in sync.

You cannot choose more than one approach at a time. Once you've chosen one approach the whole application needs to stick to that. If you use errors in schema approach, directly throwing graphql errors in resolvers will prohibited. Similarly, if you're throwing graphql errors using GraphQLError class, defining errors in schema will be prohibited.

Regarding your approach, what I'm saying is that what you did for postsConnection, defining unions of errors possible for it in schema, same will have to be done for fields which have certain roles associated to them to be accessible.

postsConnection is an object, but this union errors in schema thing will need to be implemented for scalar fields too which have certain roles associated to them and need to be protected. If a random user tries to access a field like bankAccountNumber from a user's object, we return a suitable error using union types. And yes this means that relations between objects and fields will not be 1:1. bankAccountNumber won't be a String or Int type, it'll be a BankAccountNumberResult type, which will be composed of graphql union types, the same way you showed for postsConnection.

Fields which have certain fields associated to them will not be very large in number. Usually it'll be the objects themselves that have roles associated to it, instead of their fields.

[kb-0311]

@xoldyckk

Read this again:-https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors

I know you're suggesting to implement Stage 5: Result Types, but you're overlooking the cons of it. Read through its cons again.

type CreateUserPayload {
  user: User
  userErrors: [CreateUserError!]!
}

union CreateUserError = UserNameTaken | PasswordTooShort | MustBeOver18

type UserNameTaken implements UserError {
  message: String!
  path: String!
  suggestion: String!
}

interface UserError {
  message: String!
  path: String!
}

Went through 6a section and cons of Stange 5 again.My approach is not complete Stage 5, it also combines a few ideas from section 6a to deal with 5's cons though it may not apparently seem like so. The cons of Result 5 were ->

1. syncing with clients in case of error changes in the union. (Which we are on the same page about that since there are only 2 client apps)
2. Failure of multiple errors.

What I thought the first time I read it was most of the errors where multiple would errors would be needed to be sent at the same time were attached to particular fields like username , password etc so that is why I added the fieldErrors demonstrated in a comment at the start which takes care of that, here error would not be displayed in terms of notifications so it is okay to send them as an array.

For other errors unspecific to fields, I thought it was maybe best not to send everything at once as an array since it could be a little annoying to get rid of all the notifications for errors as a mobile/web app user if we are listening for specific errors in the errors array in the payload. There could be ways around it but would require extra work on the client side.Like you know I am traversing the userErrors array in the eg and I am displaying a notification for each error in it yk .So I thought it would be best to have one error at a time when it came to API logic type errors to define the 5-second notification behavior. more accurately and send partial data with it.

I am open to the 6a approach too, no doubt it is the best approach, for the record but I feel like then to take care of errors in the error array would require more work than my approach with not a lot of benefits since the multiple errors are already there in field errors and the base interface contract to fall back to can be applied in my solution with the same ease to allow easier evolution.

Considering ease of implementation and the corner cases solved with fieldErrors array I think my approach is alright in context to talawa-api after the Error interface contract is also implemented.

Regarding your approach, what I'm saying is that what you did for postsConnection, defining unions of errors possible for it in schema, same will have to be done for fields which have certain roles associated to them to be accessible.

Ohhh ok yeah I had overlooked the scalars part but yeah sure it could be implemented exactly like the postConnection object one was implemented just like you said.

So I guess you agree with my tweaked 5a to take care of its cons instead of the complete 6a approach just because of the nature of talawa api?

[xoldd]

@kb-0311 Yes. As long as the errors for partial data are handled as union types as well, this is a solid approach. So, instead of sending back null for partial data we send back an error. Client components will be built keeping this idea in mind. So, fields which implement union type results will need a component which abstracts away the success and error cases within itself.

[kb-0311]

@xoldyckk Yes I will keep that in mind.

postsConnection is an object, but this union errors in schema thing will need to be implemented for scalar fields too which have certain roles associated to them and need to be protected. If a random user tries to access a field like bankAccountNumber from a user's object, we return a suitable error using union types. And yes this means that relations between objects and fields will not be 1:1. bankAccountNumber won't be a String or Int type, it'll be a BankAccountNumberResult type, which will be composed of graphql union types, the same way you showed for postsConnection.

Fields which have certain fields associated to them will not be very large in number. Usually it'll be the objects themselves that have roles associated to it, instead of their fields.

If this is what you are suggesting that for fields which has some sort of RBAC associated to it then I completely agree with you.

@palisadoes The thread has been concluded. Pls do take a look and suggest how to proceed further.

[palisadoes]

@kb-0311 Update the document link so that we can use it as a reference.

[palisadoes]

We need a definitive document on this that we can add to Talawa-Docs. It makes no sense making a GitHub discussion the source of truth. Create a Google Doc and when done, let's get the content added to Talawa-Docs