Codegen reimplementation for GraphQL unions

[tyranron]

Fixes #549
As side effect, also fixes #663
Also, related to #295

Background and motivation

TL;DR: current codegen for GraphQL unions is not much flexible and is too magic.

• #[derive(GraphQLUnion)] macro:
  • Applicable only to enums.
  • Doesn't support generics on type, even if they're not required for type resolution.
  • Cannot work with multiple #[graphql] attributes, just pickups first and silently ignores others.
  • Not cleverly enough generates From implementations, which leads to painful conflicts.
• #[graphql_union] macro:
  • The code being fed into macro looks like a valid Rust code, but actually is not.
  • Operates with types/traits which don't really exist (GraphQLUnion), and declares impl blocks which don't exist after macro expansion too.

We definitelly want less magic, applying custom behavior in an ergonomic and convenient way and operate with valid Rust code for better IDE experience.

New design

In a nutshell, it provides a single #[derive(GraphQLUnion)] macro which allows to modify resolution logic with custom functions. This is the same what serde does with #[derive(Serialize)] macro and #[serde(with = "my_func")] attribute.

#[derive(GraphQLUnion, derive_more::From)]
#[graphql(name = "TheCharacter", desc = "Superb character")]
#[graphql(context = MyContext)]  // <-- if not specified, then Context is `()`
#[graphql(scalar = juniper::DefaultScajarValue)] // <-- if not specified, then generated code is parametrized by `__S: ScalarValue`
#[graphql(on Jedi = Character::as_jedi)]
#[graphql(on Jedi = Character::as_better_jedi)]  // <-- emits compile-time duplication error
#[graphql(on Sith = Character::as_sith)]  // <-- resolution logic doesn't require the type to be a part of enum
pub enum Character<T> {
    One(Human),  // <-- will be trivially resolved as expected

    #[graphql(with = Character::as_droid)]
    Two(Droid),  // <-- will be resolved with `Character::as_droid` function

    #[graphql(ignore)]
    #[from(ignore)]
    Three<T>,   // <-- will be skipped

    #[graphql(with = Character::as_jedi)]  // <-- emits compile-time duplication error
    #[from(ignore)]
    Jedi,  // <-- if custom resolver specified, the variant doesn't have to containt the single type value
}

impl<T> Character<T> {
    // We can use in fuctions as context any type which implements `FromContext<MyContext>`.
    fn as_droid(&self, _: &()) -> Option<&Droid> {
        if let Self::Two(droid) = self {
            Some(droid)
        } else {
            None
        }
    }

    fn as_jedi(&self, ctx: &MyContext) -> Option<&Droid> {
       ctx.can_i_haz_the_jedi_please()
    }

    fn as_sith(&self, ctx: &SithContext) -> Option<&Sith> {
       ctx.deploy_new_sith_here()
    }
}

All the described above is applicable in the same manner to struct types too.

#[derive(GraphQLUnion]
#[graphql(name = "TheCharacter", desc = "Superb character")]
#[graphql(context = MyContext)]  // <-- if not specified, then Context is `()`
#[graphql(scalar = juniper::DefaultScajarValue)] // <-- if not specified, then generated code is parametrized by `__S: ScalarValue`
#[graphql(on Jedi = Character::resolve_jedi, on Sith = Character::deploy_sith)]`
struct TheCharacter<T>(PhantomData<T>);

impl<T> Character<T> {
    fn resolve_jedi(&self, ctx: &Context) -> Option<&Jedi> {
        ctx.can_i_haz_the_jedi_please()
    }

    fn deploy_sith(&self, ctx: &SithContext) -> Option<&Sith> {
        ctx.deploy_new_sith_here()
    }
}

Also, for enums, the innerhood From implementations are removed. It's better to used derive_more::From for that purpose, which does stuff in much more clever way, and will preserve a bit more of SRP for our macro.

However, to support traits, we still need to provide #[graphql_union] macro, as proc_macro_derive cannot be placed on trait declarations. And to avoid collisions with #[graphql] helper attribute (which will result in compilation error), we shoud use #[graphql_union] even for methods.

#[graphq_union(name = "Character", desc = "Traited character")]
#[graphq_union(context = MyContext)]  // <-- if not specified, then auto-inferred from methods, if possible, otherwise is `()`
#[graphq_union(scalar = juniper::DefaultScajarValue)] // <-- if not specified, then generated code is parametrized by `__S: ScalarValue`
#[graphq_union(on Jedi = custom_resolve_as_jedi)]  // <-- it's possible even for traits!
trait Character {  // <-- trait, however, needs to be object-safe as dynamic dispatch is involved
    fn as_human(&self) -> Option<&Human> { None } // <-- we can omit context here if we want
    fn as_droid(&self, _: &Context) -> Option<&Droid> { None }
    #[graphq_union(ignore)]
    fn unrelated_trait_method(&self) { }
}

impl Character for Human {
    fn as_human(&self) -> Option<&Human> {
        Some(self)
    }
}

impl Character for Droid {
    fn as_droid(&self, _: &Context) -> Option<&Droid> {
        Some(self)
    }
}

fn custom_resolve_as_jedi(ch: &dyn Character, ctx: &MyContext) -> Option<&Jedi> {
     ctx.can_i_haz_the_jedi_please(ch)
}

Also, this PR:

• Adds juniper::marker::GraphQLUnion trait, to not reffer in the code to unexistent trait/types.
• Relaxes Sized requirement on GraphQLType trait to allow implement it for bare trait objects.

Features

• Fully valid Rust code. No magic at all. All macro are additive and only add new code without transforming/removing existing.
• Idiomatic and familiar way for handling custom logic in proc macros: #[attr(with = func_name)].
• No need to choose between macros. There is a single macro for each situation, which still allows tweaking with custom behavior/logic.
• Multiple #[graphql] attributes are supported.
• Generics are supported.

Checklist

• Support parsing multiple #[graphql] attributes.
  • Duplication check inside single attribute.
  • Duplication check across multiple attributes.
• Re-export futures crate inside juniper with #[doc(hidden)] for internal use in macros.
• Make GraphQLUnion a real marker trait.
• Relax Sized requirement on GraphQLType trait.
• Enums (#[derive(GraphQLUnion)]):
  • infer trivial implementation for enum variants;
  • support ignoring variant with #[graphq(ignore)] attribute;
  • support generics in type declaration;
  • support custom context type with #[graphql(context = Type)] attribute;
  • support custom ScalarValue type #[graphql(scalar = Type)] attribute;
  • support custom GraphQL name and description with #[graphql(name = "Type")] and #[graphql(description = "A Type desc.")] attributes;
  • support custom resolvers on type itself with #[grapqh(on Type = resolver_fn)] attribute;
  • support custom resolver on a type variant with #[grapqh(with = resolver_fn)] attribute;
    • duplication check between #[grapqh(on Type = resolver_fn)] and #[grapqh(with = resolver_fn)] attributes for the same type;
  • remove auto-generated From implementations for enum variants.
• Structs (#[derive(GraphQLUnion)]):
  • support custom resolvers on type itself with #[grapqh(on Type = resolver_fn)] attribute;
  • support generics in type declaration;
  • support custom context type with #[graphql(context = Type)] attribute;
  • support custom ScalarValue type #[graphql(scalar = Type)] attribute;
  • support custom GraphQL name and description with #[graphql(name = "Type")] and #[graphql(description = "A Type desc.")] attributes.
• Traits (#[graphql_union]):
  • use trait methods with return Option<&Type> as union variant resolvers;
    • allow omitting Context in methods;
    • work correctly with parenthesized types;
    • allow full import paths as types;
    • validate signature for good error messages;
  • support multiple attributes
    • remove duplicated attributes on first expansion
  • support custom resolvers on type itself with #[graphql_union(on Type = resolver_fn)] attribute;
  • support custom context type with #[graphql_union(context = Type)] attribute;
  • support custom ScalarValue type #[graphql_union(scalar = Type)] attribute;
  • support custom GraphQL name and description with #[graphql_union(name = "Type")] and #[graphql_union(description = "A Type desc.")] attributes;
  • ignore trait methods with #[graphql_union(ignore)] attribute.
• Review tests and provide rich coverage of all situations
• Documentation and Book are updated
• Changelog is updated

[tyranron]

And no... PR number doesn't have any subtle meanings 🙃

[LegNeato]

I'll take a closer look but this looks awesome!

[tyranron]

@LegNeato

Why is this on needed when below there is a with?

The on is chosen to repeat the GraphQL syntax for dispatching union variants in queries.

How do these two attributes differ:

• #[graphql(on UnionVariantType = resolve_fn)] is always placed on top of the whole type declaration and always specifies the UnionVariantType that is intented to be resolved with the custom resolve_fn. This allows to specify custom resolvers for types, that don't exist in enum type declaration at all. That's why this attribute is applicable to structs and traits too:

  #[derive(GraphQLUnion]
  #[graphql(on Jedi = Character::resolve_jedi, on Sith = Character::deploy_sith)]`
  struct Character; // <-- no fields at all here
  
  impl Character {
      fn resolve_jedi(&self, ctx: &Context) -> Option<&Jedi> {
          ctx.can_i_haz_the_jedi_please()
      }
  
      fn deploy_sith(&self, ctx: &SithContext) -> Option<&Sith> {
          ctx.deploy_new_sith_here()
      }
  }

• #[graphql(with = resolve_fn)] is applicable only to enum variants, where the resolving UnionVariantType is part of enum type declaration. It's just a short, but ergonomic and expected form, applicable to enum variants only.

  #[derive(GraphQLUnion)]
  enum Character {
      First(Human),
      #[graphql(with = custom_droid_resolver)]
      Second(Droid),
  }

I've put two variants into example, to show that both are possible, and there is a duplication check for them.

[tyranron]

@LegNeato ok, I've stuck a bit with proc_macro_attribute and can't decide what is better. I need an advise.

Synopsis

It turns out that the following is totally OK when using proc_macro_derive, as it allows to register helper graphql attribute, but is impossible when using proc_macro_attribute:

#[graphq_union]
#[graphql(name = "MyCharacter", desc = "Traited character")]
trait Character {
    fn as_human(&self) -> Option<&Human>;
}

The problem is that proc_macro_attribute has no ability to register helper attributes, so #[graphql(...)] is unknown and compiler complains:

error: cannot find attribute `graphql` in this scope
  --> integration_tests/juniper_tests/src/codegen/union_attr.rs:19:3
   |
19 | #[graphql(description = "A Collection of things")]
   |   ^^^^^^^

We could have been define such attribute separately, but it raises 2 other major downsides:

• library user should import graphql attribute (via use juniper::graphql), which is not obvious and ergonomic;
• when proc_macro_derive and proc_macro_attribute are used in the same module, the graphql attributes will conflict.

  error[E0659]: `graphql` is ambiguous (derive helper attribute vs any other name)
     --> integration_tests/juniper_tests/src/codegen/union_derive.rs:169:3
      |
  169 | #[graphql(Context = CustomContext)]
      |   ^^^^^^^ ambiguous name
      |
  

I see the following possible solutions...

1. Use only graphql_union attribute

use juniper::graphql_union;

#[graphql_union(name = "MyCharacter", desc = "Traited character")]
#[graphql_union(scalar = juniper::DefaultScajarValue)]
#[graphql_union(on Jedi = custom_resolve_as_jedi)]
trait Character {
    fn as_human(&self) -> Option<&Human>;
    #[graphql_union(ignore)]
    fn some();
}

• 👍 No ambiguity on attribute names.
• 👍 No additional imports required.
• 👎 Differs too much with #[derive(GraphQLUnion)].
• 👎 graphql_union is quite long name, which makes definition noisy.

2. Provide another helper attribute

use juniper::{graphql_union, gql};

#[graphql_union]
#[gql(name = "MyCharacter", desc = "Traited character")]
#[gql(scalar = juniper::DefaultScajarValue)]
#[gql(on Jedi = custom_resolve_as_jedi)]
trait Character {
    fn as_human(&self) -> Option<&Human>;
    #[gql(ignore)]
    fn some();
}

• 👍 More laconic naming, so cleaner definition, similar to the one we have with proc_macro_derive.
• 👎 Necessity to additionally import gql macro.
• 👎 Too much different namings provided by juniper crate.

---

Writing it up, I can conclude that the option 1 is the most sane one (despite it feels not perfect one).

But comments, considerations and ideas are welcome!

[tyranron]

I've bootstrapped traits support basing on option 1.

As an another side effect:
Previous graphql_union! implementation was generating code for &'a dynTrait trait object. I've relaxed Sized requirement on GraphQLType and now it generates implementation directly for dyn Trait + 'a.

The description is updated properly.

[LegNeato]

Ugh, that's a bummer. Neither are ideal. Let me play around and see if I can come up with something.

[LegNeato]

rust-lang/rust#65823

[LegNeato]

That issue seems to think they should be supported by the attribute proc macro and not throwing an error?

[tyranron]

@LegNeato

That issue seems to think they should be supported by the attribute proc macro and not throwing an error?

Yup. If we could have something like proposed in rust-lang/rust#65823, the described above would be non-issue, and the initial design would be possible.

At the moment I think the option 1 is quite enough. It's not ideal, but does the job relatively obviously without subtleties. The most regular form (90% cases) in the code will be something like that:

#[graphq_union(
    name = "Character", 
    description = "Traited character",
    context = MyContext,
)]
trait Character {
    fn as_human(&self) -> Option<&Human> { None }
    fn as_droid(&self, _: &Context) -> Option<&Droid> { None }
}

I think users will hit the necessity to use multiple attributes for traits quite rare, in practice.

And, also, once Rust will land something like rust-lang/rust#65823 (or allow proc_macro_derive on traits), it will be quite easy to refactor.

[jmpunkt]

For me it is not clear what happens in the following case.

#[derive(GraphQLUnion)]
pub enum Character {
    One(Human), 
    Two(Droid), 
    #[graphql(ignore)]
    Three(Three),
}

#[graphql_object]
impl Test {
     fn test(&self) -> Character {
         Character::Three(todo!()) // ignore the todo!()
     }
}

The resolver would fail and return an error? If Three should not be part of the scheme, then why is it inside of the enum. Personally Im not a huge fan of skipping variants within enums. Enums should be only contain variants which are relevant. If the user accidental did something wrong, then this error will only be visible if the it is queried which seems odd when using enums especially in the context of Rust. Or is there something Im missing?

Im wondering if it is possible to have a custom resolver with the signature fn (&self, _: &Ctx) -> Result<A, B>?

[tyranron]

@jmpunkt

The resolver would fail and return an error?

At the moment it will panic. This is inherited from the existing implmentation, and, I'm afraid, is not possible to change without changing the the design of GraphQLType itself.

If Three should not be part of the scheme, then why is it inside of the enum. Personally Im not a huge fan of skipping variants within enums. Enums should be only contain variants which are relevant.

It shouldn't. If you don't want to make it a part of enum, then you're not pushed to.

The reason of having #[graphql(ignore)] attribute is not to advertise bad practices, but to give the library user more codegen abilities and the flexibility in the cases where he may have no other options.

I can quite easily imagine the situation, when library user should deal with some enum or trait, where not all methods should be exposed in schema, but they need to be there to express some additional invariants or something. Making the user in such situation to roll out manual GraphQLType implementation with all the nuances will be a huge ergonomic cost. Instead, we give him the flexibility to exclude undesired things and have all the shiny generated code with an effort of one additional helper attribute only.

If the user accidental did something wrong, then this error will only be visible if the it is queried which seems odd when using enums especially in the context of Rust.

The point is that this is "opt-in" action, not "opt-out". So to do this accidentially, the library user should explicitly put the #[graphql(ignore)] attribute. To make sure that "he knows what he's doing" I think we can describe this issue in docs clearly with a huge warning.

Im wondering if it is possible to have a custom resolver with the signature fn (&self, _: &Ctx) -> Result<A, B>?

At the moment, no. But, in theory, we could have been support fn (&self, _: &Ctx) -> Result<Option<&A>, B> signature. Option<&UnionVariantType> is mandatory due to current GraphQLType and resolving design, which uses that Option to understand whether variant should be resolved or not.

In this PR I'm trying to re-introduce capabilities of existing code generation in a better usage form. To extend those capabilities, I think we should go with a separate PR, as there are a lot questions to bikeshed and discuss.

[tyranron]

@LegNeato to sum up the current progress:

• Basically, it's done and ready. All the declared features are supported: structs, enums, traits and custom resolvers for them. You may see tests (attr, derive) for usage examples (still cumbersome, though).
• It's only left to polish, provide more tests and document everything quite well.
• The PR has growed a bit further than I have been planning. But for a reason: working with traits, I've found a juniper being unnecessarily too restrictive with Sized trait here and there, which made trait objects almost unusable to integrage with juniper. I had to relax those bounds in many places for trait objects "just to work". I'm quite happy that this was even possible, but to do it really well, it would require a separate PR with a focus on refactoring and Sized trait relaxing, as I've changed only the places required for this PR. The same refactoring may be done for Send/Sync to relax unnecessary Sync bounds (BoxFuture requires only Send, btw). Doing both will expand a much bigger flexibility, and in near future iterations we will be able to do even something like that (which, in turn, will allow much more flexibility to test the schema implementations, if one needs):

  #[graphql_object]
  trait Human {
    fn id(&self) -> String;
    fn home_planet(&self) -> &str;
  }

[LegNeato]

Awesome work, I'll take a look in-depth today.

I too am a bit suspect about ignore and would prefer to leave it out unless we have a concrete usecase. For example, with skip right away we knew folks would likely have a pasword_hash in a user struct they would not want to expose. I'd also prefer to rename ignore to skip to stay consistent with derives and serde if we keep the functionality.

[tyranron]

@LegNeato both skip and ignore are supported, they are synonyms. I've provided support for both, because ignore is more widely used in Rust ecosystem (derive_more, for example), and skip is used by juniper for a long time.

As for other macros, I'm planning to refactor/rework them too in near future. So, at the end, with a new release they will be fully consistent, I swear 😉

I too am a bit suspect about ignore and would prefer to leave it out unless we have a concrete usecase.

Actually, I've provided that, because there is a use case for ignoring in our closed codebase. This usecase involves binding some type parameters, which are not really used, but required for comfortable type-level "magic".
Something like that:

enum UserCreationResult<T> {
    Ok(User),
    Err(UserCreationError),
    #[graphql(ignore)]
    _State(PhatomData<T>),
}

where T is used for expressing some type state patterns in resolvers code.

[LegNeato]

Cool, makes sense. As long as we have a real usecase.

[LegNeato]

Looks like the CI failures are real.

[tyranron]

@LegNeato thanks for the corrections, and sorry for my bad English 🙃

Looks like the CI failures are real.

Yup, tests aren't there yet. Working on it...

[jmpunkt]

While reading the documentation I found the behavior of the following example odd.

#[derive(GraphQLUnion)]
#[graphql(Context = CustomContext)]
#[graphql(on Ewok = Character::ewok_from_context)]
enum Character {
    Human(Human),
    Droid(Droid),
}
impl Character {
    fn ewok_from_context<'c>(&self, ctx: &'c CustomContext) -> Option<&'c Ewok> {
        Some(&ctx.ewok)
    }
}

Makes sense that we have custom resolver. But in this case it is not possible to have Ewok resolved ever, since it is not part of the enum. I was wondering if it is better to fail if there is no matching variant? The same structure which should compile would be

#[derive(GraphQLUnion)]
#[graphql(Context = CustomContext)]
#[graphql(on Ewok = Character::ewok_from_context)]
enum Character {
    Human(Human),
    Droid(Droid),
    Ewok,
}
impl Character {
    fn ewok_from_context<'c>(&self, ctx: &'c CustomContext) -> Option<&'c Ewok> {
        Some(&ctx.ewok)
    }
}

[tyranron]

@jmpunkt thanks! Makes sense. I've changed the example to reflect that.

[tyranron]

@LegNeato @jmpunkt I've also added "ScalarValue considerations" section to the union docs in the Book, because this nuance is quite unobvious for newcomers, which are not familiar with Juniper architecture, which results in confusions like this one.

[tyranron]

@LegNeato @jmpunkt at this point, I'm pretty statisfied with docs in Book and Rust sources.

Returning to tests to finish this...

[tyranron]

Phew... almost there! 🎉

Existing tests are fixed and renewed.

Now, it's only the matter of adding new tests.

[tyranron]

@LegNeato finally, done! Please review, and I'll squash-merge it once approved.

[LegNeato]

Some small tweaks but let's land this!