Vincit / objection-graphql

GraphQL schema generator for objection.js
MIT License
307 stars 34 forks source link

Fragments not populated #10

Closed mattleff closed 7 years ago

mattleff commented 7 years ago

GraphQL queries using fragments are not being resolved correctly. For example:

query PersonQuery {
    ...PersonQueryFragment
}
fragment PersonQueryFragment on Query {
    person(id: 2) {
    ...PersonFragment
    }
}
fragment PersonFragment on Person {
    id
    firstName
    lastName
    movies {
    ...MovieFragment
    }
}
fragment MovieFragment on Movie {
    name
    releaseDate
}

This pull doesn't work unless select filtering is disabled (see #9). An alternative would be some code to similar to this which I've disabled because I need virtual attributes.

koskimas commented 7 years ago

@mattleff This now conflicts with the code from your other PR. This is also an excellent addition and I will merge this as soon as the conflicts are resolved. Could you resolve them?

EDIT: I only now noticed that you had disabled the select filtering. That obviously isn't an option. I can try to find a workaround for that but it will take weeks before I have time. So unless you want to fix this so that select filtering works, I cannot merge this for a while.

mattleff commented 7 years ago

@koskimas

EDIT: I only now noticed that you had disabled the select filtering. That obviously isn't an option. I can try to find a workaround for that but it will take weeks before I have time. So unless you want to fix this so that select filtering works, I cannot merge this for a while.

Did you have any thoughts on #9 regarding virtual attributes? I like the idea of select filtering but in practice that means either:

  1. Models define column maps for virtual attributes (similar to this). I don't really like that idea...
  2. Not supporting virtual attributes that use DB columns (#9 is wontfix). Obviously, that doesn't work for me either.

Would you be open to a method that could toggle select filtering (defaulting on), maybe something like:

const graphQlSchema = graphQlBuilder()
  .model(Movie)
  .model(Person)
  .model(Review)
  .selectFiltering(false)
  .build();
koskimas commented 7 years ago

@mattleff You need to list virtual attributes in Model.virtualAttributes list to make them work with objection. We can modify objection-graphql so that that array is taken into consideration when creating possible attributes. Or did you mean you don't want to use Model.virtualAttributes by saying

Models define column maps for virtual attributes (similar to this). I don't really like that idea...

EDIT: I understood what you meant 5 seconds after posting this: If a virtual attribute uses columns that you didn't explicitly select, it won't work. And only other option is to list "dependencies" for virtual attributes and I don't like that either.

An option like selectFiltering sounds like a reasonable solution to this.

koskimas commented 7 years ago

Can't this feature be implemented without the need for selectFiltering? I don't like the idea that users need to remember things like "if I use fragments, I need to call this arbitrary selectFiltering(false) method".

mattleff commented 7 years ago

@koskimas Yes, this feature can be implemented without modifying selectFiltering as I mentioned in the original post. Let me reply to the question about virtual attributes over on #9.

mattleff commented 7 years ago

@koskimas Take a look at the commit I just pushed. This doesn't deal with #9–I'll do that once we get this sorted.

koskimas commented 7 years ago

Looks good! Thank you!