Ensure Specification(June 2018) Compliance

[jmpunkt]

Motivation

Juniper currently produces code which is not allowed by the GraphQL specification. To ensure that these errors are fixed, a test suite is required. The test suite ensures that configurations which are not compliant to the specification, can not successfully compile.

Implementation

Each GraphQL type has a Type Validation chapter. Therefore, each of the constraints must be checked. An isolated Rust source snippet is created for each of these constraints. An external library compiles these snippets. The library ensures that the output of the compilation process is equal to an expected output. With the process it is ensured that these problems can not reoccur silently in the future.

Goals (Progress)

• Provider better error messages
• Ensure that everything is specification conform
• Implement GraphQLTypeAsync for missing parts (required for the test suite)

Related

• GraphQLObject generates not specified code #630
• Union behavior is different to GraphQL spec #627

[jmpunkt]

Before I continue, I would like to get some feedback for the implemented error handling (@LegNeato). I wonder if the current design is acceptable or are there any problems?

Why?

A change is preferable because panics messages are not precise. Instead syn::Error allows specifying a context for an error. The following two errors are a result of that.

error: #[derive(GraphQLUnion)] expects at least one field/variant
 --> $DIR/derive_no_fields.rs:2:1
  |
2 | enum Character {}
  | ^^^^^^^^^^^^^^^^^

error: #[derive(GraphQLObject)] does not allow multiple fields/variants with the same name. There is at least one other field with the same name `test`
 --> $DIR/derive_fields_unique.rs:4:5
  |
4 | /     #[graphql(name = "test")]
5 | |     test2: String,
  | |_________________^

Since there are errors shared between the different implementations, there are shortcuts for shared errors (including no duplicates, not empty, etc.). Errors which are unique are also propagated through the error framework.

To let the error generator know what kind of macro is used, all macros must provide a context at the beginning. This context is used to generate prefixes like #[derive(GraphQLObject)]. Maybe this is not required at all since the current errors are pointing directly to the error.

[LegNeato]

This is perfect and very close to what I was planning on doing when I got some time. Thank you!

I wonder if instead of referring to the derive we refer to "GraphQL" in general as most of these aren't requirement of the derive but instead a spec requirement.

So

error: #[derive(GraphQLObject)] does not allow multiple fields/variants with the same name. There is at least one other field with the same name `test`
 --> $DIR/derive_fields_unique.rs:4:5

Becomes

error: GraphQL objects must not contain multiple fields/variants with the same name. There is at least one other field with the same name `test`
 --> $DIR/derive_fields_unique.rs:4:5
  |

What do you think?

[LegNeato]

And maybe an optional link to the spec section?

[jmpunkt]

Thanks for the response. That is a good idea.

I wonder if we should use https://docs.rs/proc-macro-error/1.0.2/proc_macro_error/struct.Diagnostic.html for errors. This allows to specify notes and help information. The underlying mechanism is not on stable yet. Therefore, not everything is supported. Information about the used GraphQL specification could be put in a note.

[LegNeato]

That sounds like a good plan, we already were thinking about it:

#435

[jmpunkt]

The proc-macro-error crate is nice to use. With your previous feedback, an error looks like the following.

error: GraphQL object expects at least one field
  --> juniper_benchmarks/src/lib.rs:97:1
   |
97 | impl Mutation {}
   | ^^^^^^^^^^^^^^^^
   |
   = note: https://spec.graphql.org/June2018/#sec-Objects

As you can see there are errors located in the benchmark module which tries to implement empty objects. What should be done here? Remove the non-compiling snippets?

The current implementation wraps all parsed attributes into a SpanContainer<T> which contains additional Span information for that attribute. This is required because all these information are lost during the parsing. Not sure if the SpanContainer<T> is the best approach. Maybe you have a better idea.

[LegNeato]

I think benches is just an oversight. We can use EmptyMutation and EmptySubscription to fix those (and probably the "no fields on object" error should suggest those).

For User, we can just remove the empty proc macro and use the derive instead.

[LegNeato]

SpanContainer sounds like a good solution for now. The error message looks great!

[jmpunkt]

@LegNeato maybe you can resolve this question. Resolver functions for objects, e.g., test(&self) -> String can have various signatures. What is the smallest possible variant of this construct. The reason for this question is that after some refactoring in favor of the error messages, I found ~10 resolver functions which do not use &self. So which of the following functions must compile?

• fn test() -> String
• fn test(&self) -> String
• fn test(&self, context: &C) -> String
• fn test(context: &C) -> String

Edit: Looks like there are more functions than the ~10 functions, I previously found. I assume that all four functions should be accepted?!

[LegNeato]

Oh weird! AFAIK we should have tests for this. The two middle should compile, the first and last should not. &self is required and context is optional (as is the executor) but if it is there it needs to be a reference.

[LegNeato]

Ah, what I remember seeing is for the resolve function:

https://github.com/graphql-rust/juniper/blob/master/juniper_codegen/src/util/parse_impl.rs#L74

It looks like we don't require &self:

https://github.com/graphql-rust/juniper/blob/master/integration_tests/juniper_tests/src/codegen/derive_object.rs#L85

I think it makes sense to always require &self even though it is a breaking change, do you agree? I'm not sure what is more idiomatic.

[jmpunkt]

Having the requirement for &self for each functions increase the consistency. For example,
https://github.com/graphql-rust/juniper/blob/master/integration_tests/async_await/src/main.rs sometimes has a &self. Therefore, introducing a &self makes it clear that the function requires an underlying object. fn() -> T could suggest that there are no requirements for the function.

An obvious downside is the breaking change which may result in changing a lot of functions. However, currently I tend to introduce the breaking but I am not fully convinced.

[LegNeato]

This release is already breaking a ton so let's just do it.

[jmpunkt]

This PR is getting a little bit larger. I had to refactor the GraphQLInputObject which allows applying my error scheme. Now the GraphQLInputObject has a similar form like GraphQLObject and the others. Since GraphQLInputObject was the last construct which used a different argument parse, it now does not allow using string scalars like "DefaultScalarValue". I mentioned this breaking change in the changelog.

One last thing, since GraphQLInputObject is missing the async trait, some of the tests fail with a different error message. So there are two possibilities:

• add GraphQLTypeAsync in this PR
• disabled tests until it is implemented

[LegNeato]

Looks like rustfmt is failing:

Diff in /home/vsts/work/1/s/integration_tests/juniper_tests/src/codegen/derive_object.rs at line 387:
 
     f((type_info, fields));
 }
-

Happy to either disable those tests and do a followup or have you do the async change here...whatever is easier for you!

[jmpunkt]

Thanks for your review.

Additionally, I added a marker trait to detect non-object within the context of unions. Now it is not possible to use non-objects as values for unions.

Besides your recommendations, I added the __ constraint for all fields, types and arguments. The only exception are internal objects, types, etc.

Finally there is support to rename an argument inside an object impl block. This was required to test the __ for arguments. Notice that the __ tests can only be tested with #[graphql] annotations. The function to_camel_case function can not return a name with a __ prefix.

[jmpunkt]

I notice that interface testing is not possible at the moment, since it is still a declarative macro. Enforcing the requirements of the specification seems not possible. I disabled the test. After the macro is replaced the test should be re-enabled.

Maybe you can take a close look at the GraphQLTypeAsync impl for input objects. The input object now implements the async type. No functions are required to implement for the async trait since input objects are constructed by the other two traits (ToInputValue, FromInputValue)?!

[LegNeato]

Looking awesome! Thank you again for your hard work.

We're having issues with the panic message tests, as the note is included on nightly but not on stable...so the tests pass on nightly but fail on stable even though everything is ok. That is kinda annoying.

We have a way to run crates in CI only on nightly (see the rocket integration, it is basically dropping a cargo-make file in the crate) so perhaps we should pull these out into their own crate under https://github.com/graphql-rust/juniper/tree/master/integration_tests and have it only run on nightly? Is there any way we can switch between expected output automatically based on what toolchain is running the tests?

[jmpunkt]

The only way to switch the expected output is to have two crates with different expected output. But testing only on nightly should be fine. This way we notice possible changes since. Stable does not implement most of the features, thus the output should not change at all (until the Diagnostic API lands).

[LegNeato]

Excellent! I think we just need to record the output again and we're good to merge!

[jmpunkt]

Implementing the constraints that arguments have the property IsInputType(arg) == True is bit tricky. I use a marker trait IsInputType which has an arbitrary method. For each argument,
<#arg_type as IsInputType<#scalar>>::mark() is generated. The compiler can check if each arg has the IsInputType implemented. This seems to work. However, this must be implemented for interfaces as well. Since interfaces are generated by declartive macros, there is no way to express the previous statement.

        $crate::__juniper_impl_trait!(
            impl<$($scalar)* $(, $lifetimes)* > GraphQLType for $name {
                fn mark() {
                    $(
                        <$return_ty as $crate::marker::IsOutputType<$($scalar)*>::mark();
                    )*
                }
            }
        );

The reason why this does not compile is because of the nested $(...)*. The only possible fix I see right now is to remove the declarative macro and replace it with a proc macro.

[LegNeato]

Yeah, the plan is to remove all macros and replace them with proc macros in this release for just that reason. We just did so a couple weeks ago with graphql_scalar.

The declarative macros were from way back from when proc macros were not available on stable.

If you want to skip that check in this PR so we can land it I'm totally fine with that.

Also, I'd love to add you as a maintainer for Juniper, let me know if you would be interested. You're doing a ton of high-quality work.

[LegNeato]

Thanks again!