Skip to content

Query introspection in resolvers #124

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
weiznich opened this issue Jan 3, 2018 · 12 comments
Closed

Query introspection in resolvers #124

weiznich opened this issue Jan 3, 2018 · 12 comments
Labels
enhancement Improvement of existing features or bugfix

Comments

@weiznich
Copy link
Contributor

weiznich commented Jan 3, 2018

Short version:

I've tried to write a small wrapper layer for a database using diesel. A straight forward implementations leads to N+1 query problems.

Long version:

Given the following database schema:

table! {
    heros {
        id -> Integer,
        name -> Text,
        species -> Integer,
    }
}

table! {
    species {
        id -> Integer,
        name -> Text,
    }
}

joinable!(hero -> species(species));
allow_tables_to_appear_in_same_query!(hero, species);

It should be possible to map this directly to the following graphql schema.

type Character {
  id: Int!
  name: String!
  species: Species!
}

type Species {
  id: Int!
  name: String!
  heros: [Character!]!
}

type Query {
  heros(id: Int, name: String): Character
  species(id: Int, name: String): Species
}

Implementing this graphql api in a straight forward way using juniper leads to the following code

struct DbConnection(Mutex<SqliteConnection>);

#[derive(Queryable, Debug, GraphQLObject)]
struct Character {
    id: i32,
    name: String,
    species: Species,
}

#[derive(Queryable, Debug)]
struct Species {
    id: i32,
    name: String,
}


graphql_object!(Species: DbConnection |&self| {
    field id() -> i32 {
        self.id
    }
    field name() -> &String {
        &self.name
    }
    field heros(&executor) -> FieldResult<Vec<Character>> {
        let conn = &*executor.context().0.lock()?;
        // This results in N+1 queries
        heros::table.filter(heros::species.eq(self.id)).load(conn).map_err(Into::into)
    }
});

struct Query;

graphql_object!(Species: DbConnection |&self| {
    field heros(&executor, id: Option<i32>, name: Option<String>) -> FieldResult<Vec<Character>> {
        let conn = &*executor.context().0.lock()?;

       let q = heros::table.inner_join(species::table)
           .select((heros::id, heros::name, (species::id, species::name)));

       match (id, name) {
            (Some(id), Some(name)) => q.filter(heros::id.eq(id).and(heros::name.eq(name)).load(conn).map_err(Into::into),
            (Some(id), None) => q.filter(heros::id.eq(id)).load(conn).map_err(Into::into),
            (None, Some(name)) => q.filter(heros::name.eq(name)).load(conn).map_err(Into::into),
            (None, None) => q.load(conn).map_err(Into::into),
       }
    }

    field species(&executor, id: Option<i32>, name: Option<String>) ->  FieldResult<Vec<Species>> {
       let conn = &*executor.context().0.lock()?;

       let q = species::table;

       match (id, name) {
            (Some(id), Some(name)) => q.filter(species::id.eq(id).and(species::name.eq(name)).load(conn).map_err(Into::into),
            (Some(id), None) => q.filter(species::id.eq(id)).load(conn).map_err(Into::into),
            (None, Some(name)) => q.filter(species::name.eq(name)).load(conn).map_err(Into::into),
            (None, None) => q.load(conn).map_err(Into::into),
       }
    }
});

Diesel offers BelongingToDsl + GroupedByDsl to solve exactly this problem, but I do not see a place to incorporate this into my current code. A solution would be to allow implementations of GraphQLType to access the requested selection for the current query. It should be possible to build optimized queries.

@theduke
Copy link
Member

theduke commented Jan 4, 2018

I agree that this is a problem, that get's much worse the bigger your schema becomes.

Basing your DB requests on the request schema works for simple schemas, but breaks down the more complicated and nested your schema becomes.

The only real solution for this, IMO, is a design like https://github.com/facebook/dataloader, which is also the approach FB recommends.

This will require tokio and async support, though, which is planned.

@LegNeato
Copy link
Member

@weiznich
Copy link
Contributor Author

I think this should be solvable without tokio, async support and dataloader. As stated in the orginal bugreport I would help if each method in GraphQLType had access to the simplified version of ast::Selection containing basically the query tree at this stage. For each node it should be possible to access the name, the alias and the passed options.

(Basically I'm looking for something like the LookAhead feature in Graphile)

@theduke
Copy link
Member

theduke commented Jan 16, 2018

That's very reasonable and I've actually wanted this myself. Let's put it on the roadmap.

If it's ok with you I'll repurpose the issue and add a description.

@mhallin
Copy link
Member

mhallin commented Jan 16, 2018

This is very interesting and could certainly pave the way for a more in-depth integration with e.g. Diesel on the backend of Juniper.

The Executor already contains a private struct field for the currently resolving selection set, so if we think today's AST is stable enough to release into the public API, we would basically only need a method to expose this field.

To make it useful though, we'd probably need a few utility methods that can determine whether a specific field on a specific type is accessed in a selection, etc.

@weiznich
Copy link
Contributor Author

The Executor already contains a private struct field for the currently resolving selection set, so if we think today's AST is stable enough to release into the public API, we would basically only need a method to expose this field.

I think we should not expose the AST for this, because the AST contains many informations that are not required to resolve the actual query.
In my (very incomplete) understanding of GraphQL I think something like the following may server:

struct Argument{
    name: String,
    value: Value,
}

struct Selection {
    name: String,
    alias: Option<String>,
    arguments: Vec<Argument>,
   childs: Vec<Selection>
}

Notably fragments should be already inlined.

(I'm willing to work on this, but as written above my understanding of GraphQL is currently quite limited, so some help might be required.)

@mhallin
Copy link
Member

mhallin commented Jan 16, 2018

That was my initial reaction as well, but unfortunately I don't think you can inline fragments in the general case - there might be type conditions for values that haven't been resolved yet. For example, if you've got the query

{
  hero {
    ... on Droid { primaryFunction }
    ... on Human { homePlanet }
  }
}

Here, the executor can't inline the fragments before the hero field has been resolved since the concrete type is unknown.

@weiznich
Copy link
Contributor Author

Ok, seems like I missed this case. (I really need to read a full GraphQL guide soon…)

At leas this case should be solvable by adding informations about the type to the child_list
So we could have something like:

struct Argument {
    name: String,
    value: Value,
}

struct ChildSelection {
    inner: Selection,
// Maybe this needs a distinct enum type with better names instead of Option
    applies_for: Option<String>,
}

struct Selection {
    name: String,
    alias: Option<String>,
    arguments: Vec<Argument>,
   childs: Vec<ChildSelection>
}

@ghost
Copy link

ghost commented Mar 28, 2018

Allowing what fields are selected for the resolver using either selectionSets or exposing the ast source to the resolver will help in solving N+1 query problems right now easily.
Example:

type Author {
   id: String
   name: String
}

type Book {
   id: String
   title: String
  author: Author
}

type Query {
  getBooks: [Book]
}

Like this O(1)

fn getBooks(selectionSet) ->FieldResult<Vec<Book>> {
  let q = getBooksQuery();
  if (selectionSet.contains('author') {
      q.joinAuthors();
  }
  return diesel.query(q)
}

instead of this N + 1;

graphql_object!(Book: Context |&self| {
  field author(&executor) -> FieldResult<Author> {
    Ok(diesel::get_author(&*executor.context().conn, self.id, self.id)?)
  }
});

This way we don't need to wait for async/tokio to come by.

@weiznich
Copy link
Contributor Author

@pyros2097 #136 basically implements exactly this.

@theduke theduke changed the title Trait design possibly leads to inefficient implementations Query introspection in resolvers Apr 22, 2018
@theduke theduke added the enhancement Improvement of existing features or bugfix label Apr 22, 2018
@brandur
Copy link

brandur commented Jun 1, 2018

@weiznich Did you end up putting together a Diesel-powered project that uses this somewhere? I'm kind of curious to see your implementation.

@weiznich
Copy link
Contributor Author

weiznich commented Jun 1, 2018

Yes, I did

Also I close this issue now because it is already implemented by #136.

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

No branches or pull requests

5 participants