Skip to content

FieldResult should implement GraphQLType #320

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

Closed
jgillich opened this issue Jan 31, 2019 · 6 comments
Closed

FieldResult should implement GraphQLType #320

jgillich opened this issue Jan 31, 2019 · 6 comments

Comments

@jgillich
Copy link
Contributor

I wanted to return Option<FieldResult<Foo>> from a field, but I can't because FieldResult doesn't implement GraphQLType. Thoughts? I could probably do the PR (assuming this is even possible, didn't really look into it).

@LegNeato
Copy link
Member

I'm not sure that would make sense. FieldResult is analogous to Result. You might actually want FieldResult<Option<Foo>>, which means that the field can possibly fail to resolve (the FieldResult bit) but if it does resolve it either returns nothing (None in the Option) or Foo (Some in the Option).

Let me know if that isn't what you are trying to do or if I misunderstand.

@jgillich
Copy link
Contributor Author

jgillich commented Jan 31, 2019

I have a field that can either:

  • Not have a value
  • Have a value that is a link to another object - but then the resolver might fail

So the most correct return type would be Option<FieldResult>. However, I didn't think of trying FieldResult<Option>, and that's fine as well. Makes no difference in the actual API anyway. Thank you!

@JeanMertz
Copy link
Contributor

I ran into this issue while researching a problem I was facing.

I'm not a fan of using too many macro's in my code-base (for a lack of clarity, and usually a lack of informative compiler error messages), so I wanted to implement GraphQLType myself.

The documentation states "Manually deriving an object is straightforward but tedious".

I ran into an issue where I wanted to manually derive a FieldResult<T>. I was unable to do so using something like:

registry.field<FieldResult<Vec<&MyT>>>("my_t", info)

As it returned:

the trait `GraphQLType` is not implemented for `Result<Vec<&MyT>, FieldError>`

I had to dig into the graphql_object macro to find that the macro in fact uses field_convert to make this work, which is a hidden (but public) API.

Am I doing something wrong here, or is this the correct way to use a FieldResult<T> in a manually derived GraphQLTYpe?

@theduke
Copy link
Member

theduke commented May 14, 2019

@JeanMertz if you are manually implementing resolve_field anyway, you can just return the ExecutionError in case the execution fails, no? field_convert doesn't add any type of conversion to the resolver code, it's just for convenience.

Also: the API is really not intended for implementing GraphQLType manually. It's really error prone, since you lose all type safety in your resolvers and can easily return a value with a wrong type, especially after making changes to the api structure.

You might be interested in the just merged #333.
Your code still goes through a macro, but it looks just like regular Rust code now:

struct Query;

#[juniper::object]
impl Query {
    fn add(&self, a: i32, b: i32) -> i32 {
        a + b
    }
}

See the updated master book: https://graphql-rust.github.io/juniper/master

@JeanMertz
Copy link
Contributor

Ah thanks @theduke, that's great, and it works flawlessly, thank you for this 👍

(documentation is still a bit lacking, I had to dig through the tests to find how to rename a field, as it only showed how to rename the object itself, but other than that, it worked as expected)

@theduke
Copy link
Member

theduke commented May 14, 2019

There is some more extensive documentation on the juniper::object macro (cargo doc --open, it's not online yet).

But thanks for pointing it out, I'll add that to the book.

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

No branches or pull requests

4 participants