juniper icon indicating copy to clipboard operation
juniper copied to clipboard

Make graphql_object macro supports deriving object field resolvers

Open zhenwenc opened this issue 4 years ago • 6 comments

This PR enhance the graphql_object macro to optionally derive field resolvers for fields defined in the object type struct.

#[derive(GraphQLObjectInfo)]
#[graphql(scalar = DefaultScalarValue)]
struct Obj {
    regular_field: bool,
}

#[juniper::graphql_object(derive_fields)]
impl Obj {
    fn custom_field_resolve() -> bool {
        true
    }
}

I am pretty new to rust, please let me know if I have done anything incorrect. And I am not a native speaker of english, feel free to correct the wording of my comments. Cheers!


Related: #553

zhenwenc avatar May 05 '20 04:05 zhenwenc

Not sure how to add reviewers, would be great if anyone can help. Thanks!

^^ @LegNeato @jmpunkt @Victor-Savu


While I was working on the PR, I also noticed a few potential issues. But I am not sure who to talk to or maybe they are actually intended or work-in-progress features:

  1. Looks like its a typo? https://github.com/graphql-rust/juniper/blob/d13305f2029c12b11844b2fdf941f081e835a366/juniper_codegen/src/util/mod.rs#L649

  2. Derive GraphQLObject macro for struct with lifetime doesn't work. The following struct found in a test suite, which can reproduce the issue, but seems its not been used in any test case. https://github.com/graphql-rust/juniper/blob/d13305f2029c12b11844b2fdf941f081e835a366/integration_tests/juniper_tests/src/codegen/derive_object.rs#L63-L66

  3. Derive GraphQLObject macro requires scalar attribute, otherwise generic __S will be used instead of the default DefaultScalarValue. For instance

#[derive(GraphQLObject)]
struct Object {
    value: i32,
}

should be the same as

#[derive(GraphQLObject)]
#[graphql(scalar = juniper::DefaultScalarValue)] // <---- here
struct Object {
    value: i32,
}

zhenwenc avatar May 05 '20 22:05 zhenwenc

  1. You can fix the typo.
  2. Do you know why this does not work? Is the lifetime parameter missing at the generics definition?
  3. There is an issue related to this (https://github.com/graphql-rust/juniper/issues/580). Not sure how to deal with this, I would ignore it for now. A workaround for this is to specify #[graphql(Scalar = juniper::DefaultScalarValue)]

jmpunkt avatar May 06 '20 11:05 jmpunkt

Thanks @jmpunkt !

Re 2:

Given the following code:

mod test {
    #[derive(juniper::GraphQLObject)]
    #[graphql(scalar = juniper::DefaultScalarValue)]
    struct WithLifetime<'a> {
        value: &'a str,
    }

    struct Query;

    #[juniper::graphql_object(scalar = juniper::DefaultScalarValue)]
    impl<'a> Query {
        fn with_lifetime(&self) -> WithLifetime<'a> {
            WithLifetime { value: "blub" }
        }
    }
}

Here is the compile error, which I think it is related to the async resolver (but can be wrong):

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> integration_tests/juniper_tests/src/lib.rs:20:5
   |
20 |     #[juniper::graphql_object(scalar = juniper::DefaultScalarValue)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the impl at 21:10...
  --> integration_tests/juniper_tests/src/lib.rs:21:10
   |
21 |     impl<'a> Query {
   |          ^^
note: ...so that the expression is assignable
  --> integration_tests/juniper_tests/src/lib.rs:20:5
   |
20 |     #[juniper::graphql_object(scalar = juniper::DefaultScalarValue)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected  `foo::WithLifetime<'_>`
              found  `foo::WithLifetime<'a>`
note: but, the lifetime must be valid for the lifetime `'b` as defined on the method body at 20:5...
  --> integration_tests/juniper_tests/src/lib.rs:20:5
   |
20 |     #[juniper::graphql_object(scalar = juniper::DefaultScalarValue)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...so that the expression is assignable
  --> integration_tests/juniper_tests/src/lib.rs:20:5
   |
20 |     #[juniper::graphql_object(scalar = juniper::DefaultScalarValue)]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected  `std::pin::Pin<std::boxed::Box<(dyn core::future::future::Future<Output = std::result::Result<juniper::value::Value, juniper::executor::FieldError>> + std::marker::Send + 'b)>>`
              found  `std::pin::Pin<std::boxed::Box<dyn core::future::future::Future<Output = std::result::Result<juniper::value::Value, juniper::executor::FieldError>> + std::marker::Send>>`

Here is the generated GraphQLTypeAsync for Query:

    impl<'a> juniper::GraphQLTypeAsync<juniper::DefaultScalarValue> for Query
    where
        juniper::DefaultScalarValue: Send + Sync,
        Self: Send + Sync,
    {
        fn resolve_field_async<'b>(
            &'b self,
            info: &'b Self::TypeInfo,
            field: &'b str,
            args: &'b juniper::Arguments<juniper::DefaultScalarValue>,
            executor: &'b juniper::Executor<Self::Context, juniper::DefaultScalarValue>,
        ) -> juniper::BoxFuture<'b, juniper::ExecutionResult<juniper::DefaultScalarValue>>
        where
            juniper::DefaultScalarValue: Send + Sync,
        {
            use futures::future;
            use juniper::GraphQLType;
            match field {
                "withLifetime" => {
                    let res: WithLifetime<'a> = (|| WithLifetime { value: "blub" })();
                    let res2 = juniper::IntoResolvable::into(res, executor.context());
                    let f = async move {
                        match res2 {
                            Ok(Some((ctx, r))) => {
                                let sub = executor.replaced_context(ctx);
                                sub.resolve_with_ctx_async(&(), &r).await
                            }
                            Ok(None) => Ok(juniper::Value::null()),
                            Err(e) => Err(e),
                        }
                    };
                    use futures::future;
                    future::FutureExt::boxed(f)
                }
                _ => {
                    {
                        ::std::rt::begin_panic_fmt(&::core::fmt::Arguments::new_v1(
                            &["Field ", " not found on type "],
                            &match (
                                &field,
                                &<Self as juniper::GraphQLType<juniper::DefaultScalarValue>>::name(
                                    info,
                                ),
                            ) {
                                (arg0, arg1) => [
                                    ::core::fmt::ArgumentV1::new(arg0, ::core::fmt::Display::fmt),
                                    ::core::fmt::ArgumentV1::new(arg1, ::core::fmt::Debug::fmt),
                                ],
                            },
                        ))
                    };
                }
            }
        }
    }

zhenwenc avatar May 06 '20 22:05 zhenwenc

While I like the idea of this in general, I think it massively complicates the UX for folks to save on some typing. This reminds me a lot of Rust accessors and transparent newtypes. Rust itself requires boilerplate (self.0) but people who mind the repetition build a layer on top with proc macros and there are multiple libraries that do it based on various tradeoffs. I always assumed people would just use the proc macro with a bit of boilerplate or use juniper_codegen to make their own macros with their preferred tradeoffs if this was an issue.

Personally, I like how simple and explicit the UX is right now--either graphql projection is trivial and the derive suffices or it is complex and everything is right there in the impl.

I'm happy to entertain a patch if we can keep this simple and explicit. I'm not sure we can get there with rust proc macros as-is though 🤷

LegNeato avatar May 10 '20 00:05 LegNeato

@LegNeato Thanks for your reply, I understand your concern and agree that Juniper should not be opinionated.

However, since juniper_codegen already achieved a great effort on collecting necessary information to build GraphQL schema (such as fields for object / input types, field resolvers, etc), is it possible to expose these collected information to the user instead of generating the GraphQLType directly, so that user can use these information in their own macro to generate GraphQLType.

For example juniper can generate GraphQLTypeInfo that holds such information, similar to what this PR does. Then I can build my own graphql_object macro with the functionalities I need very easily.

Let me know what you think. Thanks!

zhenwenc avatar May 12 '20 00:05 zhenwenc

can we have multiple derive_fields? it would be awesome if we can.

file1:

#[derive(GraphQLObjectInfo)]
#[graphql(scalar = DefaultScalarValue)]
struct Obj {
    regular_field: bool,
}

#[juniper::graphql_object(derive_fields)]
impl Obj {
    fn custom_field_resolve() -> bool {
        true
    }
}

file2:

#[juniper::graphql_object(derive_fields)]
impl Obj {
    fn other_field_resolve() -> bool {
        true
    }
}

smmoosavi avatar Sep 12 '20 19:09 smmoosavi