Discussion: Generic error codes in the spec

[pranshuchittora]

Abstract I am researching and planning to build a generic test suite to check the compliance of the various GraphQL libraries The project idea is given by the GraphQL Foundation for the Google Summer of Code 2020. There are various implementations of GraphQL and all of them are throwing error in various ways. As all errors are thrown in plain text(string). There is no way to test these implementations for such errors.

Say we have given an incorrect query (with circular fragment) to a GraphQL server and we are expecting an error. If the error comes with a specific error code then it would be great to test the libraries and provides and assuring the actual cause of the error, therefore, improving the DX as well.

For example imagine a well known error place that can hold a "spec rule number" that is indicative of what validation rule you have caused an error message to fail on

errors : [ { message : "any old message", extensions : [ __graphqlspec : { reason : "March2018-5.1.2.4" }}}}

Follow this discussion -> https://github.com/graphql-java/graphql-java/issues/1826#issuecomment-601550014

Motivation I am really impressed by the standardisation of the error codes in the following projects.

• React error codes -> https://github.com/facebook/react/blob/master/scripts/error-codes/codes.json
• Rust lang -> https://doc.rust-lang.org/error-index.html
• TypeScript -> https://github.com/microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json

What can be done It would be great if we can come up with such standard error codes in the GraphQL spec.

[sungam3r]

OK. My thoughts:

1. No breaking changes. Good.
2. Easy to implement.
3. It should be explicitly stated in spec (if we decide to supplement the spec) that attaching the error information about code is optional for the server.
4. I propose to use __graphqlspec as a conventional prefix for all such things and not as full property name. Full property name for error code may be __graphqlspecErrorCode

[shobhitchittora]

Well @sungam3r IMO the whole point of adding such error codes is to enforce / streamline error codes across implementations. Having it optional would defeat the whole purpose. What are your thoughts on this. What effort do you think would go in this route?

[sungam3r]

That is simple. It is impossible to do such error codes as required properties since

1. Breaking changes to existing implementations.
2. Extensions property by design has no structure defined.

[sungam3r]

See https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md#guiding-principles

Future changes should not change the meaning of existing schema or queries or in any other way cause an existing compliant GraphQL service to become non-compliant for prior versions of the spec.

[markokr]

How about a optional "catagory" key, so clients can make broad decision and servers are testable whether right check triggered?

Enumerating individual errors seems lot of work, for dubious benefit.

Main disadvantage would be that some implementations opt out from it if it's too bothersome.

[sungam3r]

Well, category or code are the same things. GraphQL-dotnet for example already has error codes in response https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL.NewtonsoftJson/ExecutionResultJsonConverter.cs#L98

[markokr]

code is fine as a key. The question is can other implementations reproduce them.

Main goal is spec regression tests that can be run over several implementations.

It's fine to say that this testsuite requires support for code.

[pranshuchittora]

Well, category or code are the same things. GraphQL-dotnet for example already has error codes in response https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL.NewtonsoftJson/ExecutionResultJsonConverter.cs#L98

That looks interesting, can you point me to the exact listing of these error codes.

[michaelstaib]

Code is already used in many implementations so I would go with that. We also use code in Hot Chocolate. The more important issue is to specify error codes for spec errors.

[sungam3r]

That looks interesting, can you point me to the exact listing of these error codes.

Errors codes are aligned with paragraph numbers from the spec. 5.4.2.1 for http://spec.graphql.org/June2018/#sec-Required-Arguments for example.

[markokr]

Hardwiring chapter numbers as errors seems wrong, both implementations side and also spec side: one-chapter-per-error may be annoying to maintain, especially if you are not allowed to refactor.

Named errors seems good enough, or if you really want numbers, then OID-like tree maintained in spec.

[michaelstaib]

I also would not bind them to the chapters. I personally also prefer named errors. But that might be a personal preference I would also be fine with something else.

[dariuszkuc]

Similar to "error code", we are using system events for all our exception handling but we expose it in error extension block (as per spec).

When following code first approach, schema generation process should not allow generation of invalid schema. Your runtime should already run valid schema that produces correct responses so I am unsure how often you would end up with something non-compliant with the spec. I believe you need this sort of validation when creating the schema but if that is the case then I am unsure why you would need this error information in GraphQL response as the server should not start with invalid schema.

[sungam3r]

@dariuszkuc The talk is about query validation errors as well.

[sungam3r]

@markokr We can use different properties together- code like UNIQUE_ARGUMENT, number like 1.2.3.4 , even helpUrl to spec like http://spec.graphql.org/June2018/#sec-Argument-Names

[sungam3r]

URLs convenient because paragraph numbers can vary from version to version of specification.

[sungam3r]

@pranshuchittora From my comment it follows that separate test suites are required for each version of the specification.

[markokr]

The testsuite itself would indeed be tied to specific spec version, so instead it makes sense for tests to contain spec section numbers and even links to spec.

I still cannot see how making implementation organize code around spec layout is useful - it may be natural to share some error handling code, so why force it track spec layout?

[pranshuchittora]

@sungam3r I think creating a separate section for the error spec can be one of the approaches. The section will contain a whole bunch of error code like GQ0001 (inspired from rust-lang), and these codes can internally reference their corresponding section i.e. 1.2.3.4.

This approach can be fruitful in the long run as it is decoupled with the entire structure of the spec.

[michaelstaib]

@pranshuchittora this sounds quite good.

[michaelstaib]

@markokr @sungam3r

We implement features once they hit draft. Meaning organizing a test suite around a specific spec version can be count-productive. I like the approaches that web browser suites go where you can see what is implemented and what not.

@pranshuchittora when I talked with @IvanGoncharov about building a replacement for GraphQL-CATS we first wanted to focus on parser tests. What is now the general idea.

We had CATS already implemented for Hot Chocolate but it was quite difficult back then. One of the issues were error codes. But there are other problems to solve. Do you already have a broader idea how to go about it?

[pranshuchittora]

@michaelstaib I am in the process of writing the proposal and I would love to get it reviewed by you, once it gets completed.

As of now, we can only test the implementations for basic input-output, hence not covering the edge case. The idea of standardising the error codes is one of the effort for testing the implementations more deeply. We can improve the proposal prioritizing the aspects of the spec which seem most valuable to be covered with centralized tests.

[michaelstaib]

@pranshuchittora excellent... love that this finally gets some traction.

[pranshuchittora]

Hello there, With several iterations in the design, and the help of @michaelstaib. I have created a rough architecture for the compliance test suite.

---

Design of the App Server and the test runner. (Docker Approach)

---

Events and lifecycle

For detailed info, you can read my GSoC proposal -> https://docs.google.com/document/d/1D0PD2nwU97aL-hTc_XLL8H5MYNqBzkIBJeDeh_UJ0yo/edit?usp=sharing

[shobhitchittora]

@michaelstaib , @sungam3r The above sounds quite good. Wanted to also check on the suggestions around Error spec for gql - https://github.com/graphql/graphql-spec/issues/698#issuecomment-602242983

@sungam3r I think creating a separate section for the error spec can be one of the approaches. The section will contain a whole bunch of error code like GQ0001 (inspired from rust-lang), and these codes can internally reference their corresponding section i.e. 1.2.3.4.

This approach can be fruitful in the long run as it is decoupled with the entire structure of the spec.

Let me know if we can start some discussion around this, and write an initial spec / RFC around this.

[sungam3r]

I'm doing other things now, I can help with wordings and review.

[Fontinalis]

@shobhitchittora I’m happy to hop in and start some discussion on this. Currently building a GraphQL server in Go and would love to add these generic error codes as early as possible, would make testing much easier later.

@pranshuchittora Awesome work, nice proposal.

[michaelstaib]

@shobhitchittora I would really like to get this going since it would at a lot of value. So, I am in.

[shobhitchittora]

Thanks for the positive vibe here. Let me take some time and start a RFC around this.

Thanks all.

[shobhitchittora]

@michaelstaib @Fontinalis @sungam3r @markokr @pranshuchittora @IvanGoncharov Hope you're all safe and well. Please check out #708. Would love all suggestions and feedback for improvements.