Skip to content

Join Fetch applied twice for ElementCollection #198

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
vokinpirks opened this issue Oct 1, 2019 · 5 comments · Fixed by #200
Closed

Join Fetch applied twice for ElementCollection #198

vokinpirks opened this issue Oct 1, 2019 · 5 comments · Fixed by #200

Comments

@vokinpirks
Copy link

vokinpirks commented Oct 1, 2019

Hi!
I'm playing around with this library and encountered an issue.
I have pretty simple JPA model. There is element collection defined as follows:

@ElementCollection(fetch = FetchType.LAZY)
@GraphQLDescription("A set of user-defined tags")
@BatchSize(size = 16)
private Set<String> tags = new HashSet<>();

When I do NOT include this field in query everything works fine:

query {
  Tracks() {
    select {
      addedAt
      playCount
    }
  }
}

But when I do so, I get an empty result:

query {
  Tracks() {
    select {
      addedAt
      playCount
      tags
    }
  }
}

'tags' field is empty for all entities.
I checked generated JPQL/SQL queries and found out that there are two joins for tags, left and inner one. I guess I'm getting the empty result because of that inner join.
I started debugging and figured out that the second join is added at
com.introproventures.graphql.jpa.query.schema.impl.QraphQLJpaBaseDataFetcher.java:234
(root.fetches becomes [2] there).

And..join is a very inefficient way of fetching *ToMany/ElemectCollection relations. Batch fetching is a way better for that. Theoretically, is it possible to implement batch fetching in this library?

@igdianov
Copy link
Collaborator

igdianov commented Oct 1, 2019

@vokinpirks Thank you for reporting this problem. :) There should be only one fetch join generated for element collections here:

The first join is needed to fetch the data from the graphql query. The inner join should not be there unless you want to filter elements in your collection. It is probably a side effect from the predicate builder somewhere. I will try to create a test to find out and fix it.

The batch fetching is not part of JPA spec, so it cannot be used directly. I am also not sure what effect @BatchSize annotation has on the Hibernate to fetch element collections on top JPA query.

Graphql-java has something called DataLoader: https://github.com/graphql-java/java-dataloader. I have not figured out how to use it, but may be your case is a good candidate to do it.

@vokinpirks
Copy link
Author

@igdianov the first join is left whereas the second one is inner.
FYI the first join appears right after this call:

The line of code you are referring to always produces inner join despite the fact isOptional set to true. The method javax.persistence.criteria.FetchParent#fetch(java.lang.String) has an overloaded signature which takes JoinType as a parameter.
Shouldn't it be like
from.fetch(selectedField.getName(), isOptional ? JoinType.LEFT : JoinType.INNER);
?

Thanks for pointing me to DataLoader. Will take a look at that.

@igdianov
Copy link
Collaborator

igdianov commented Oct 2, 2019

@vokinpirks Yes, this is a bug. It should not be apply to fetch joins for element collections. It will be easy to fix.

igdianov added a commit that referenced this issue Oct 5, 2019
…elements (#200)

* fix: adedd support for fetching optional element collections elements

* fix(test): polished queryOptionalElementCollections

* polish(test): added tags selection field to queryOptionalElementCollections
@igdianov
Copy link
Collaborator

igdianov commented Oct 6, 2019

@vokinpirks I have fixed the issue. Let me know if it works for you.

@vokinpirks
Copy link
Author

@igdianov thanks! I'll try this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants