Skip to content

fix: Apply left outer join to retrieve optional associations [WIP] #105

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

Merged
merged 8 commits into from
Apr 28, 2019

Conversation

igdianov
Copy link
Collaborator

@igdianov igdianov commented Apr 7, 2019

This PR fixes #101

Given optional to-one association, i.e. favoriteDroid:

@Entity(name = "Human")
@Data
@EqualsAndHashCode(callSuper=true)
public class Human extends Character {

    String homePlanet;

    @ManyToOne(fetch = FetchType.LAZY, optional = true)
    @JoinColumn(name = "favorite_droid_id")
    Droid favoriteDroid;

}

The GraphQL schema will generate optional argument with default value derived from JPA's optional attribute value definition.

Type Humans {
   ...
   favoriteDroid(where: DroidsCriteriaExpression, optional: Boolean = true): Droid
   ...
}

The JPQL query will now apply left outer join to all queries against entity graph with optional associations, i.e. given the following query

    Humans {
        select {
            id
            name
            homePlanet
            favoriteDroid {
              name
            }
        }
    }

And return all records with optional favoriteDroid association null value

{
  "data": {
    "Humans": {
      "select": [
        {
          "id": "1000",
          "name": "Luke Skywalker",
          "homePlanet": "Tatooine",
          "favoriteDroid": {
            "name": "C-3PO"
          }
        },
        {
          "id": "1001",
          "name": "Darth Vader",
          "homePlanet": "Tatooine",
          "favoriteDroid": {
            "name": "R2-D2"
          }
        },
        {
          "id": "1002",
          "name": "Han Solo",
          "homePlanet": null,
          "favoriteDroid": null
        },
        {
          "id": "1003",
          "name": "Leia Organa",
          "homePlanet": "Alderaan",
          "favoriteDroid": null
        },
        {
          "id": "1004",
          "name": "Wilhuff Tarkin",
          "homePlanet": null,
          "favoriteDroid": null
        }
      ]
    }
  }
}

It will possible to override optional strategy with explicit value, i.e.

query {
  Humans {
    select {
      id
      name
      homePlanet
      favoriteDroid(optional: false) {
        name
      }
    }
  }
}

Will return the following result:

{
  "data": {
    "Humans": {
      "select": [
        {
          "id": "1000",
          "name": "Luke Skywalker",
          "homePlanet": "Tatooine",
          "favoriteDroid": {
            "name": "C-3PO"
          }
        },
        {
          "id": "1001",
          "name": "Darth Vader",
          "homePlanet": "Tatooine",
          "favoriteDroid": {
            "name": "R2-D2"
          }
        }
      ]
    }
  }
}

@igdianov igdianov self-assigned this Apr 7, 2019
@igdianov igdianov added the bug label Apr 7, 2019
@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #105 into master will increase coverage by 0.09%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #105      +/-   ##
============================================
+ Coverage     65.73%   65.83%   +0.09%     
- Complexity      366      369       +3     
============================================
  Files            37       37              
  Lines          2014     2014              
  Branches        296      296              
============================================
+ Hits           1324     1326       +2     
  Misses          562      562              
+ Partials        128      126       -2
Impacted Files Coverage Δ Complexity Δ
...a/query/schema/impl/QraphQLJpaBaseDataFetcher.java 72.64% <75%> (+0.58%) 114 <0> (+3) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d2f5d1...5a1c0b5. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #105 into master will increase coverage by 0.58%.
The diff coverage is 83.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master   #105      +/-   ##
==========================================
+ Coverage     66.42%    67%   +0.58%     
- Complexity      383    400      +17     
==========================================
  Files            34     33       -1     
  Lines          2031   2061      +30     
  Branches        301    304       +3     
==========================================
+ Hits           1349   1381      +32     
- Misses          552    555       +3     
+ Partials        130    125       -5
Impacted Files Coverage Δ Complexity Δ
...ures/graphql/jpa/query/schema/model/book/Book.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...graphql/jpa/query/schema/model/starwars/Droid.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...jpa/query/schema/impl/GraphQLJpaSchemaBuilder.java 88.05% <100%> (+0.75%) 119 <6> (+4) ⬆️
...a/query/schema/impl/QraphQLJpaBaseDataFetcher.java 73.75% <76.19%> (+0.68%) 130 <12> (+13) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e9da5b...ea84a00. Read the comment docs.

@igdianov igdianov force-pushed the igdianov-use-left-outer-join branch from 5a1c0b5 to c0cc4d0 Compare April 13, 2019 04:45
@molexx
Copy link
Contributor

molexx commented Apr 15, 2019

Thanks for this! We've been running against this branch for a few days, the fix works as expected and we haven't noticed any unexpected side effects.

@igdianov
Copy link
Collaborator Author

@molexx Thank you for checking. I will refactor and apply optional association fetching after merging PR to support nested associations in where criteria expressions.

@igdianov igdianov force-pushed the igdianov-use-left-outer-join branch from c0cc4d0 to 148e129 Compare April 16, 2019 02:05
@igdianov igdianov force-pushed the igdianov-use-left-outer-join branch 6 times, most recently from b51638b to 08131be Compare April 23, 2019 01:46
@igdianov igdianov changed the title fix: Apply left outer join to retrieve optional associations fix: Apply left outer join to retrieve optional associations [WIP] Apr 24, 2019
@igdianov igdianov force-pushed the igdianov-use-left-outer-join branch from 08131be to 1c72160 Compare April 24, 2019 06:03
@igdianov igdianov force-pushed the igdianov-use-left-outer-join branch from 1c72160 to bd4427a Compare April 27, 2019 02:57
@igdianov igdianov merged commit 7ff25ae into master Apr 28, 2019
@igdianov igdianov deleted the igdianov-use-left-outer-join branch April 28, 2019 22:15
@molexx
Copy link
Contributor

molexx commented Apr 29, 2019

Thanks! :-)

@molexx
Copy link
Contributor

molexx commented May 1, 2019

I don't think this is working properly in the merge to master. I've had difficulty proving when it broke because the branch this was on is now deleted and I've not had much success trying to pull builds for commits noted above from jitpack.

I'll continue to investigate tomorrow...

@molexx
Copy link
Contributor

molexx commented May 1, 2019

Our project works in the old build from that branch but does not work with 0.3.25, yet doing a similar query with the starwars schema using the example project works fine with master.

@igdianov are there any changes to this outer join functionality between the branch and master? Did you implement the optional keyword you mentioned? I can't see any changes to the example project or starwars schema to support this? Thanks!

@igdianov igdianov restored the igdianov-use-left-outer-join branch May 1, 2019 23:56
@igdianov
Copy link
Collaborator Author

igdianov commented May 2, 2019

@molexx I had restored igdianov-use-left-outer-join-original branch so you could try to use it again and compare generated queries between initial and later commits in igdianov-use-left-outer-join.

I have made the changes only to apply left outer join for @ManyToOne and @OneToOne singular attributes. I did not make any changes to Startwars schema because JPA annotation optional attribute is true by default.

@igdianov
Copy link
Collaborator Author

igdianov commented May 2, 2019

@molexx Let me know what you find. Thank you

@molexx
Copy link
Contributor

molexx commented May 2, 2019

@igdianov thanks for restoring the branches!

c7754eb works OK whereas a6761b46 does not. The JPQL is as expected - when it works there's a left outer join between the tables whereas when it doesn't there's an inner join.

The parent entity contains a HashSet of the child records with @OneToMany(fetch = FetchType.LAZY, mappedBy=<id of parent in child>).

@igdianov
Copy link
Collaborator Author

igdianov commented May 3, 2019

@molexx I need to understand the full context of query to be able to find the solution. The changes that work for you use outer join for all associations, but it is not a good practice for performance reasons. Let me know more details...

@igdianov
Copy link
Collaborator Author

igdianov commented May 6, 2019

@molexx See #138 that adds supports for optional join fetch argument for collection attributes

@igdianov igdianov deleted the igdianov-use-left-outer-join branch October 12, 2019 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing relationship data causes parent data to not be returned
2 participants