Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make graphql_object macro supports deriving object field resolvers #652

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhenwenc
Copy link

@zhenwenc zhenwenc commented May 5, 2020

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 added 2 commits May 4, 2020 09:44
My editor uses `rustfmt` to format on save, but `rustfmt` doesn't pickup edition
specified in `Cargo.toml` properly.
@zhenwenc zhenwenc marked this pull request as ready for review May 5, 2020 22:01
@zhenwenc
Copy link
Author

zhenwenc commented May 5, 2020

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?

    pub struct GraphQLTypeDefiniton {

  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.

    #[derive(GraphQLObject, Debug, PartialEq)]
    struct WithLifetime<'a> {
    regular_field: &'a i32,
    }

  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,
}

@jmpunkt
Copy link
Contributor

jmpunkt commented May 6, 2020

  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 (#[juniper::graphql_object] != #[derive(juniper::GraphQLObject)] #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)]

@zhenwenc
Copy link
Author

zhenwenc commented May 6, 2020

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),
                                ],
                            },
                        ))
                    };
                }
            }
        }
    }

@LegNeato
Copy link
Member

LegNeato commented May 10, 2020

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 🤷

@zhenwenc
Copy link
Author

@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!

@smmoosavi
Copy link

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
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants