juniper icon indicating copy to clipboard operation
juniper copied to clipboard

Can't compile crate with "the trait `IntoResolvable<'_, _, _, _>` is not implemented for `Result<MyType, FieldError>`" error

Open Occhioverde opened this issue 2 years ago • 3 comments

Describe the bug

The compile-time the trait `IntoResolvable<'_, _, _, _>` is not implemented for `Result<GitRepo, FieldError>` error is thrown by the #[graphql_object] macro when trying to return a struct with complex fields wrapped in a FieldResul.

Example code

The following is a snippet from my project that throws the compile-time error I mentioned before. Just in case it can be meaningful, the Repo struct annotated as the type of the repo field in GitRepo is not a GraphQL Object.

The first line is marked as the one from which the error is originating:

#[graphql_object(context = GQContext)]
impl Query {
    async fn repo(path: String, context: &GQContext) -> FieldResult<GitRepo> {
        get_by_path(&path, &context.authdata, &context.db).await
    }
}

pub struct GitRepo {
    repo: Repo,
}

#[graphql_object(context = GQContext)]
impl GitRepo {
    pub fn id(&self) -> String {
        self.repo.id.to_string()
    }
}

pub async fn get_by_path(path: &String, authdata: &AuthData, db: &DbCon) -> FieldResult<GitRepo> {
    // -- snip --
}

Expected behavior

The code should compile just like the following does:

#[graphql_object(context = GQContext)]
impl Query {
    async fn repo(path: String, context: &GQContext) -> FieldResult<GitRepo> {
        get_by_path(&path, &context.authdata, &context.db).await
    }
}

#[derive(GraphQLObject)]
pub struct GitRepo {
    id: String,
}

pub async fn get_by_path(path: &String, authdata: &AuthData, db: &DbCon) -> FieldResult<GitRepo> {
    // -- snip --
}

Occhioverde avatar Aug 26 '22 18:08 Occhioverde

I think that I managed to find the solution to the problem.

While trying to write down a simpler example to attach to this issue I found myself unable to reproduce the error, so I double-checked my project in order to find in what was different. It turns out that in some previous refactoring I inadvertently removed the impl juniper::Context for Ctx {} statement, which evidently wasn't an issue until I introduced an object with complex fields.

Maybe, in order to avoid confusion in case someone else forgets to implement juniper::Context, it should be considered the option to introduce a more explanatory error message as the one I reported above looks quite obscure (at least to me).

In case anyone wants to reproduce the issue, here is a snippet that you can paste in the main.rs file of a new crate that only depends on Juniper:

use juniper::{graphql_object, EmptyMutation, EmptySubscription, FieldResult, Variables};

pub struct Ctx(i32);
// Uncomment the following line to compile the crate
//impl juniper::Context for Ctx {}

pub struct Query;

#[graphql_object(context = Ctx)]
impl Query {
    fn repo() -> FieldResult<GitRepo> {
        Ok(GitRepo)
    }
}

pub struct GitRepo;

#[graphql_object(context = Ctx)]
impl GitRepo {
    pub fn id(&self, context: &Ctx) -> FieldResult<i32> {
        Ok(1 + context.0)
    }
}

pub type Schema = juniper::RootNode<'static, Query, EmptyMutation<Ctx>, EmptySubscription<Ctx>>;

fn main() {
    let (res, _errors) = juniper::execute_sync(
        "query { repo { numid } }",
        None,
        &Schema::new(Query, EmptyMutation::new(), EmptySubscription::new()),
        &Variables::new(),
        &Ctx(1),
    )
    .unwrap();

    println!("{}", res);
}

Occhioverde avatar Aug 26 '22 20:08 Occhioverde

@Occhioverde

Maybe, in order to avoid confusion in case someone else forgets to implement juniper::Context, it should be considered the option to introduce a more explanatory error message as the one I reported above looks quite obscure (at least to me).

I agree, the error message is quite confusing. Expanding additional check on should help:

const _: fn() = || {
    fn assert_context<T: ::juniper::Context>() {}
    assert_context::<Ctx>();
};

ilslv avatar Aug 29 '22 06:08 ilslv

It seems that #1072 will eliminate this problem, as there will be no juniper::Context anymore, and IntoResolvable will be used for Result coercion only without necessity to coerce context types.

tyranron avatar Aug 29 '22 09:08 tyranron