graphql-rust / juniper

GraphQL server library for Rust
Other
5.71k stars 425 forks source link

Query introspection in resolvers #124

Closed weiznich closed 6 years ago

weiznich commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

In rust: https://github.com/cksac/dataloader-rs

weiznich commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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>
}
pyrossh commented 6 years ago

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 commented 6 years ago

@pyros2097 #136 basically implements exactly this.

brandur commented 6 years ago

@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 commented 6 years ago

Yes, I did

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