Add error field & handler for mutations' payload

[RobertLowe]

Hey,

Here's the work for errors payloads. I know we talked about moving errors inside the records payload, but that still feels awkward to me, I mean what if the model has an errors property, should we use _errors, additional where should we be injecting the errors payload type. Inside createInputType?

Cheers,

• Rob

[RobertLowe]

Follow up, after we get the above stored we can address the other operations ...Many, remove..., etc

[nodkz]

I wanted to review your PR today but catch a bad error with embedded documents:
fix: required fields in embedded documents became optional for filter args and became optional for update(ById|One|Many) resolvers. All such embedded types are now cloned and have their own unique names.

I spent a half-day fixing that.

[RobertLowe]

Cool, no worries, thanks for the update. I had a look at the fix it looks good.

OT: food for thought in future we could have a cache/reuse system to generate the smallest schema as possible, I think there may be opportunities to reuse types when used in with the default configuration, but leave that to way down the line.

[nodkz]

awkward to me, I mean what if the model has an errors property, should we use _errors

Don't worry about overlapping field names. Document fields are inside record field, so you can easily put the errors field near record.

[nodkz]

BTW. I merged master with your PR via force push. So this PR has the latest master changes. And I also migrated to Github Actions from Travis CI (Travis has not been working well lately).

[RobertLowe]

Yeah, that's the way it currently is. You have:

recordId: MongoId
record: User
errors: [ErrorPayload]

Look good?

Cool, thanks, I'll pull the latest.

[nodkz]

I checked PR and have some thoughts.

For such query:

mutation {
  createOne(...) {
    recordId
    record {
      _id
    }
    errors: {
      messages
    }
  }
} 

We will have the following response:

{
  data: {
    createOne {
      recordId: 1
      record {
        _id: 1
      }
      errors: [  # <--- array-level-1 
        {
          messages: [   # <--- array-level-2
             "Some error"
          ]
        }
     ]
    }
  }
}

With 2 nested arrays, It will be quite uncomfortable to work with it on the client-side. Moreover, frontend developers will be in confusion about how to deal with them. I suggest getting rid of array-level-2.

So my proposal is:

1) Create a base ErrorInterface which should be used for any kind of errors not only for validation:

interface ErrorInterface {
  message: String
}

And use this interface for field errors: [ErrorInterface!] in mutation payload. We can return not only validation errors but also any kind of error from resolvers. So interfaces will help clients to write queries without fragments:

{
   errors: {
      message
   }
}

Or with fragments when they need specific fields from error:

{
   errors: {
      message
      ... on ValidationError {
        path
      }
   }
}

2) For validation error create a new type

type ValidationError implements ErrorInterface {
  message: String
  path: String
}

As you can see here we don't have arrays in the message field but the mongoose validation error has. So I suggest split them in the top-level. If some path has 2 errors, just create two separate ValidationError objects with the same path. So the response from graphql server can be the following form:

{
   errors: [
      { __typename: 'ValidationError', message: 'Path `n` is required.', path: 'n' },
      { __typename: 'ValidationError', message: 'this is a validate message', path: 'n' },
      // and we can provide also such errors
      { __typename:  "MongoError", "code":11000, message: "insertDocument :: caused by :: 11000 E11000 duplicate key error index: mydb.users.$email_1  dup key: { : null }" },
      { __typename:  "RuntimeError", message: "Cannot save file" },
   ]
}

This approach helps to keep the simplicity and extendability of the errors field.

[RobertLowe]

Yeah, this makes sense.

What your proposed solution for ...Many resolvers? As say validation errors will need to reference the document with issues. Should we overload the record in records?

records: [
  // record may or may not have an id (createMany vs updateMany)
  {_id?: ..., errors: [...]}
]

Or something like:

records: [...]
errors: [
  // which document do I belong to? keep in mind sometimes _id could be null
  { ... } 
]

[RobertLowe]

Okay, this uses ErrorInterface 👍 and flattens array level 👍

[nodkz]

@RobertLowe good catch for ...Many resolvers.

I think that if a client provides for us an array with input data then we should return some array with result data in the payload. I suppose that records field which is an array of shape for singular response is quite a good and reusable solution (in the clients code developers can write helper functions which read singular mutations and reuse them in iterators for plural resolvers).

type xxxManyPayload {
  records: xxxManyPayloadRecords
  hasErrors: Boolean!  # <--- need to find a good name for this field
}

type xxxManyPayloadRecords { # <---- try to keep the same payload shape of *One resolvers
  recordId: ID
  record: Entity
  errors: ErrorInterface
}

About hasErrors field: we should provide a convenient way to frontenders for checking do they have errors for some operation or not. And I think that the most convenient way to determine is there an error or not - check some field rather than traverse some arrays. And if this field has error flag, then start traversing errors if needed to display them somehow to end-users. But I not sure hasErrors is a good name or not. What are your thoughts?

[RobertLowe]

@nodkz thanks, also just resolved tests.

How about? Just errors, hasErrors sound better, but if we just send errors back as boolean it's more uniform to the other APIs

type xxxManyPayload {
  records: xxxManyPayloadRecords
  errors: Boolean!  # <--- need to find a good name for this field
}

type xxxManyPayloadRecords { # <---- try to keep the same payload shape of *One resolvers
  recordId: ID
  record: Entity
  errors: ErrorInterface
}

Yeah, this is what I was thinking too.

[nodkz]

Nope, just errors name is a bad idea for Boolean field. Clients may be confused by such query:

mutation {
  userCreateMany(...) {
     records {
       errors # <--- real array with errors' messages
     }
     errors # <--- just boolean value
  }
}

[RobertLowe]

Yeah, I'm cool with hasErrors. However, what do we do in the case of runtime errors, etc an exception like no mongo connection, would that just appear in all sub-documents.

Maybe something like:

type xxxManyPayload {
  records: xxxManyPayloadRecords
  hasRecordErrors: Boolean
  error(s): [ErrorInterface] # <- runtime/ hard errors
}

type xxxManyPayloadRecords {
  recordId: ID
  record: Entity
  errors: ErrorInterface # <-- nested validation, runtime, etc errors
}

[RobertLowe]

Follow up, what should be finished on this PR to close it?

[nodkz]

I've just improved createOne resolver – if errors field is not requested in graphql query then throw top-level error. See the following test suite da05d7a#diff-db6bb76152461b1e807ccac62fe81271

What need to do

• improve other resolvers with throwing error on top-level (maybe need to write a common wrapper for resolvers which will check resolveParams.projection.errors and throw Error or just return error inside the result. For now, we have duplicate code in resolvers).
• decide what to to with async validators (because validateSync skips async checks)
• write additional tests
  • with async validator
  • with runtime error for checking that it returns in mutation payload (if errors fields is requested in query)
  • with runtime error for checking that it throws in top-level (if errors fields is not requested in query)
• implement errors payload for all rest mutation resolvers (eg. updateById, updateOne)

[nodkz]

Also I found that ValidationError has value property from client which causes the validation error. So I think it will be nice to return it da05d7a#diff-dd2f0072f8e0f435a2b858dcba4c6aafR113

[RobertLowe]

Thanks for the improvements!

[nodkz]

As an example of a common wrapper for resolvers

In one of our internal app, we are using such wrapper for our mutations

• status: String - it's a flag about the operation status (we are using hasError this is much better solution)
• query: Query - it allows to request data from graph after mutation in one request (more details https://graphql-rules.com/rules/mutation-payload-query)

So in such manner we can catch errors from resolve method and decide what to do - return it in the payload or throw to the top-level.

import {
  TContext,
  Extensions,
  GraphQLFieldResolver,
  ObjectTypeComposer,
  ObjectTypeComposerFieldConfigAsObjectDefinition,
  ObjectTypeComposerAsObjectDefinition,
  ObjectTypeComposerFieldConfig,
  ObjectTypeComposerArgumentConfigMapDefinition,
  schemaComposer,
} from '@pskz/app-server';
import { GraphQLResolveInfo } from 'graphql';

export interface PayloadTypeConfig {
  name: string;
  fields: {
    [key: string]: ObjectTypeComposerFieldConfig<any, TContext>;
  };
}

export interface PayloadData {
  recordId: string;
  status?: string;
}

export interface PayloadConfig<TArgs, TFields = any> {
  type: ObjectTypeComposerAsObjectDefinition<any, TContext>;
  args?: ObjectTypeComposerArgumentConfigMapDefinition<TArgs>;
  resolve?: GraphQLFieldResolver<TArgs, TFields>;
  subscribe?: GraphQLFieldResolver<TArgs, TFields>;
  deprecationReason?: string | null;
  description?: string | null;
  extensions?: Extensions;
  [key: string]: any;
}

export function generateMutationPayload<TArgs>(fc: PayloadConfig<TArgs>) {
  let type: ObjectTypeComposer;
  try {
    type = schemaComposer.createObjectTC(fc.type);
  } catch (e) {
    throw new Error('Cannot wrap mutation payload cause it returns non-object type.');
  }
  type.addFields({
    status: 'String',
    query: 'Query!',
  });

  const oldResolve =
    fc.resolve ||
    async function (_source: any, _args: TArgs, _context: TContext, _info: GraphQLResolveInfo) {
      return {};
    };
  fc.resolve = async (source, args, context, info) => {
    const result = await oldResolve(source, args, context, info);
    result['query'] = {};
    if (!result['status']) result['status'] = 'Ok';
    return result;
  };
  (fc as ObjectTypeComposerFieldConfigAsObjectDefinition<any, TContext, TArgs>).type = type;
  return fc as ObjectTypeComposerFieldConfigAsObjectDefinition<any, TContext, TArgs>;
}

For today I need to go. It's midnight already. See you tomorrow.

[RobertLowe]

Thanks for the effort, sponsored the project, enjoy a few 🍻 or what you like!

[RobertLowe]

33f6659

• Ports changes from createOne to updateById & updateOne
• Use async validate over validateSync

In future we may want to allow for control over validate options. https://mongoosejs.com/docs/api/document.html#document_Document-validate ex: validateModifiedOnly

Midnight for me as well, see you tomorrow morning, haha.

[nodkz]

Thanks for the effort, sponsored the project, enjoy a few 🍻 or what you like!

Wow! Thanks a lot! 🙏
I'll take a look at your changes tomorrow... Nope, I'm not spending money on 🍻🍻🍻 😜. I had a quite terrible day by updating dependencies and resolving tons of conflicts and broken builds for our internal client package. For now, I exhausted and cannot concentrate on your PR.

[nodkz]

I changed the payload.errors: [ErrorInterface] to payload.error: ErrorInterface according to the following reasons:

• I need somehow to catch runtime errors inside the resolve method and return them to the user. And JS return just one error object. And wrap it in an array looks quite strange 😅
• Clients will get one error in most cases, so why they need Arrays?! And I'm sure, that just one error is more obvious and easy to use field. errors field on top-level in GraphQL response was created that exceptions may occur in different resolvers when graphql query is executed. BUT in our case, we have just one resolver. So no needs in an array 💃

According to current graphql schema design we return just one error. So what do you think about how to deal with many validation errors? Yep, just create ValidationError with some field inside for an array of sub-errors. And of course, return combined error msg in message field.

@RobertLowe what do you think?

[RobertLowe]

Thrown errors still happen at top level and are an array, ex:

{
  "errors": [
    {
      "message": "Authentication Error",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "getAuthToken"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "payload": [
            {
              "field": "password",
              "message": "Please enter a valid password."
            }
          ],
          "stacktrace": [
            "Error: Authentication Error",
            "    at GraphQLLocalStrategy.verify (../src/index.js:46:21)",
            "    at processTicksAndRejections (internal/process/task_queues.js:97:5)"
          ]
        }
      }
    }
  ],
  "data": {
    "getAuthToken": null
  }
}

Where validation errors happen at the data level:

"data": {
  "usersCreateOne": {
    record: ..., recordId: ..., "errors": [{ "path": "email", "value": "There is already a user with this email." }],
    hasErrors: true
  }
}

Another idea would be to use UserInputError https://www.apollographql.com/docs/apollo-server/data/errors/ However that feels like a lot for a frontend developer to dig through as well as it doesn't really work well for ...Many operators.

My proposal is this:

"data": {
  "usersCreateMany": {
    items: [
    {record: ..., recordId: ..., "errors": [{ "path": "email", "value": "There is already a user with this email." }]},
    {record: ..., recordId: ..., "errors": [{ "path": "email", "value": "There is already a user with this email." }]}, 
   ],
   hasErrors: true
  }
}

Makes the most sense as it follows what the singular version would do:

"data": {
  "usersCreateOne": {
    record: ..., recordId: ..., "errors": [{ "path": "email", "value": "There is already a user with this email." }],
    hasErrors: true
  }
}

The benefit is your errors are related to their origin, it does create one more level, but I think that's just the cost of it.

Runtime, and MongoError's should be top-level. and not inside the data level.

This is the same as what graphql-ruby does: https://graphql-ruby.org/mutations/mutation_errors

Thanks again, and I'll defer to you as you know more than I.

[nodkz]

First of all, we should differentiate errors in Queries and Mutations:

• errors in Query are well described by Sasha Solomon in her last talk about error handling. They are specific to Data Domains and it's quite hard to generate something reasonable according to just mongoose schema. So, for now, we do not consider such kinds of errors in graphql-compose-mongoose.
• errors in Mutation. The mutation we should consider as some Operation on data changing. You wrote about validation errors happen at the data level – but better to say operation level, data level its quite abstract thing.

So any operation on data changing can be finished with to statuses

• success with data,
• or it may be failed by some exception. Programming languages allow us to throw just one error. And you may encapsulate as much additional information inside the error object as you need.

Eg. mongoose's validationError is a good example of such cumulative error. You have message field with common error message – User validation failed: n: Path n is required., valid: this is a validate message:

and you may traverse errors field if you want to get specific parts of validation errors:

If we think about Mutations in terms of operation (in other words - function call, or transaction execution), so it has just two variants - success or fail. And according to graphql query language, we should provide the most convenient way for the consumption of these operation results eg. { record: Entity, error: ErrorInterface }. Such payload structure is very simple (KISS). And allows to us return an error in the payload (if the user requested error field in graphql query) or otherwise throw it to top-level errors field. When we decided to provide the array of errors in payload, we didn't consider how to provide this array on top-level – we can throw just one error, but cannot throw an array of errors (javascript & graphql disallows such thing). So providing singular ErrorInterface in the payload is a good constraint – easy to implement in code for us, easy to read any kind of errors on client-side.

Runtime, and MongoError's should be top-level. and not inside the data level.

As i wrote above I consider mutations from operation level not data level. Any errors inside operation execution belong to operation. Operations halted by some reason, so we should answer to the user why it happens in that way how it request. If it asks us to return error field in payload, then any error from operation should be returned in this field. And no matter what it was some validation error, or mongodb error, or some tricky business logic inside resolver.

When we consider just data level so we restricted to throw just validation errors. So in such way we must rename our errors field to more expressive name - validationErrors. And think in what form we can throw them to top-level if user doesn't request this field in graphql query.

Moreover, if you watch Sasha's video they distinguish request parsing errors for top-level errors field (something like an incorrect request from the client) and operation errors for data field (something happens according to data domain and data itself). So email already taken, password is too short, Mongo: E11000 duplicate id, 'no db connection' I feel that they all are operational level errors. Because the client wants to execute some operation for data changing, server accepted the query, started execution, but something went wrong and operation was halted. What if the client sends several mutations inside one request and some of them were halted?!

PS. What to do with batch resolvers I try to think about this evening.

[RobertLowe]

Okay, right, this all makes sense. We can still dig into error for error.errors.

Thanks for these explanations, I know I must be absorbing your valuable time but I'm learning a great amount and the talk was fun to watch.

Yes, batch will be more tricky because they aren't done as a transaction but a save per entity meaning only some could complete. I'm not sure how things process/halt there of hand.

All said, we should end up with results like?:

"data": {
  "usersCreateOne": {
    record: ..., 
    recordId: ..., 
    "error": {
      __typename: "ValidationError", 
      message: "Validation failed, some longer message", 
      errors: [{ "path": "email", "value": "There is already a user with this email." }]
    }
  }
}

"data": {
  "usersCreateMany": {
    items: [
    {record: ..., recordId: ..., "error": { ... }},
    {record: ..., recordId: ..., "error": { ... }}, 
   ]
  }
}

[nodkz]

Yep, for singular resolvers we can use almost the same shape as mongoose returns for us (without transformation):

"data": {
  "usersCreateOne": {
    record: ..., 
    recordId: ..., 
    "error": {
      __typename: "ValidationError", 
      message: "Validation failed, some longer message", 
      errors: [{ "path": "email", "value": "There is already a user with this email." }]
    }
  }
}

For "batch" resolvers, I think, that we should keep the same shape for payload { recordS: ..., error: ...} to know that some errors happen - eg. { message: 'Operation has 5 errors: Here go joined messages from 5 errors.' }.

And also provide granularly error for every record in records field. How I wrote above I suppose that records field which is an array of shape for singular response is quite a good and reusable solution (in the clients code developers can write helper functions which read singular mutations and reuse them in iterators for plural resolvers).

So for "batch" resolvers the graphql query can be the following form:

mutation {
  userCreateMany {
    error {
      message # String with a cumulative error message (which will be thrown to top-level if this field isn't requested)
    }
    records {
      record
      recordId
      error { # error object for specific record like in singular mutation resolvers
        message
        ... on ValidationError {
          path
          value
          message
        }
      }
    }
  }
}

What we can do more for "cumulative error"?
If we have only validation errors, we can combine all validation errors from different records with path modification and return regular ValidationError type. For example, if 3rd record has error then change path: "n" to path: "3.n" where 3 is an index of record from the input array. In such way we can throw on top-level correct validation error as we did for singular mutations.

[RobertLowe]

For the cumulative error message I think something like KISS like"<number> of document(s) has issues." is enough, but it's up to the developer to use the nested errors for more useful information, re: MongoError / ValidationError for batch operations

[github-actions]

🎉 This PR is included in version 9.0.0-alpha.1 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nodkz]

@RobertLowe please check 9.0.0-alpha.1.

I refactored code, most things were simplified. And during the refactor, I found that your suggestion in #248 (comment) (add index to errors) the most simple solution from all above that we discussed:

• less code in resolvers & helpers
• less types in graphql just one ErrorInterface (no need in ManyErrorInterface)
• less memory consumption (errors: [ { idx: 3, message: '', path: ''} ] better than errors: [ null, null, null, { message: '', errors: [{ message: '', path: ''}] } ]

If you want you may follow my last 8 commits for more information. As a summary, I can provide the following test case which demonstrates how validation errors result returned by createMany resolver:

 it('check validation for createMany', async () => {
    const res = await graphql.graphql({
      schema,
      source: `
        mutation { 
          createMany(records: [{ name: "Ok"}, { name: "John", someStrangeField: "Test" }]) { 
            records { 
              name
            }
            error {
              __typename
              message
              ... on ValidationError {
                errors {
                  message
                  path
                  value
                  idx
                }
              }
            }
          }
        }
      `,
    });

    expect(res).toEqual({
      data: {
        createMany: {
          records: null,
          error: {
            __typename: 'ValidationError',
            message: 'Nothing has been saved. Some documents contain validation errors',
            errors: [
              {
                message: 'this is a validate message',
                path: 'someStrangeField',
                value: 'Test',
                idx: 1, // <---------------------------- JUST ADDED `idx` field.
              },
            ],
          },
        },
      },
    });
  });

And such result when error field is not requested and resolver's error throws to top-level errors with extensions:

 it('check validation for createMany with root-level errors & extension', async () => {
    const res = await graphql.graphql({
      schema,
      source: `
        mutation { 
          createMany(records: [{ name: "Ok"}, { name: "John", someStrangeField: "Test" }]) { 
            records { 
              name
            }
          }
        }
      `,
    });

    expect(res).toEqual({
      data: { createMany: null },
      errors: [
        expect.objectContaining({
          message: 'Nothing has been saved. Some documents contain validation errors',
          extensions: {
            name: 'ValidationError',
            errors: [
              {
                idx: 1,
                message: 'this is a validate message',
                path: 'someStrangeField',
                value: 'Test',
              },
            ],
          },
          path: ['createMany'],
        }),
      ],
    });
  });

--

If you have any questions about current implementation – feel free to ask here.
If any additions (eg. what to do with updateMany) – please open a new one.
This issue became too big.

Thanks a lot for help with these amazing improvements 🙏

[github-actions]

🎉 This PR is included in version 9.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀