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

Fragments not populated #10

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

mattleff
Copy link
Contributor

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
Copy link
Collaborator

koskimas commented Jul 29, 2017

@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
Copy link
Contributor Author

@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 (Columns for virtual attributes not selected #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
Copy link
Collaborator

koskimas commented Jul 31, 2017

@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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

koskimas commented Aug 1, 2017

Looks good! Thank you!

@koskimas koskimas merged commit f337845 into Vincit:master Aug 1, 2017
@mattleff mattleff deleted the fragments-not-populated branch August 1, 2017 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants