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

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

Open
Occhioverde opened this issue Aug 26, 2022 · 3 comments · May be fixed by #1072
Assignees
Labels
enhancement Improvement of existing features or bugfix
Milestone

Comments

@Occhioverde
Copy link

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 Occhioverde added bug Something isn't working needs-triage labels Aug 26, 2022
@Occhioverde
Copy link
Author

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

@ilslv
Copy link
Member

ilslv commented Aug 29, 2022

@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 ilslv added enhancement Improvement of existing features or bugfix good-first-issue and removed bug Something isn't working needs-triage labels Aug 29, 2022
@ilslv ilslv added this to the 0.16.0 milestone Aug 29, 2022
@tyranron tyranron linked a pull request Aug 29, 2022 that will close this issue
26 tasks
@tyranron tyranron linked a pull request Aug 29, 2022 that will close this issue
26 tasks
@tyranron
Copy link
Member

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 tyranron self-assigned this Aug 29, 2022
@tyranron tyranron modified the milestones: 0.16.0, 0.17.0 Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants