Skip to content

feat: add optional argument for plural attributes #138

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
May 7, 2019

Conversation

igdianov
Copy link
Collaborator

@igdianov igdianov commented May 6, 2019

This PR adds support for default optional: true argument for collections in the query selection graph, i.e.

query {
  Authors {
    select {
      id
      name
      books(optional: true) {
        id
        title
        genre
      }
    }
  }
}

With optional: true, the data fetcher will apply LEFT OUTER JOIN on books and return empty books collections:

{
  "data": {
    "Authors": {
      "select": [
        {
          "id": 1,
          "name": "Leo Tolstoy",
          "books": [
            {
              "id": 2,
              "title": "War and Peace",
              "genre": "NOVEL"
            },
            {
              "id": 3,
              "title": "Anna Karenina",
              "genre": "NOVEL"
            }
          ]
        },
        {
          "id": 4,
          "name": "Anton Chekhov",
          "books": [
            {
              "id": 5,
              "title": "The Cherry Orchard",
              "genre": "PLAY"
            },
            {
              "id": 6,
              "title": "The Seagull",
              "genre": "PLAY"
            },
            {
              "id": 7,
              "title": "Three Sisters",
              "genre": "PLAY"
            }
          ]
        },
        {
          "id": 8,
          "name": "Igor Dianov",
          "books": []
        }
      ]
    }
  }
}

Use optional: false to enforce INNER JOIN, so that

query {
  Authors {
    select {
      id
      name
      books(optional: false) {
        id
        title
        genre
      }
    }
}

Will return entity graph with only existing associations:

{
  "data": {
    "Authors": {
      "select": [
        {
          "id": 1,
          "name": "Leo Tolstoy",
          "books": [
            {
              "id": 2,
              "title": "War and Peace",
              "genre": "NOVEL"
            },
            {
              "id": 3,
              "title": "Anna Karenina",
              "genre": "NOVEL"
            }
          ]
        },
        {
          "id": 4,
          "name": "Anton Chekhov",
          "books": [
            {
              "id": 5,
              "title": "The Cherry Orchard",
              "genre": "PLAY"
            },
            {
              "id": 6,
              "title": "The Seagull",
              "genre": "PLAY"
            },
            {
              "id": 7,
              "title": "Three Sisters",
              "genre": "PLAY"
            }
          ]
        }
      ]
    }
  }
}

You can also configure default join fetch optional argument value via Api.

@igdianov igdianov self-assigned this May 6, 2019
@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #138 into master will decrease coverage by 0.04%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #138      +/-   ##
============================================
- Coverage     68.27%   68.23%   -0.05%     
- Complexity      432      437       +5     
============================================
  Files            33       33              
  Lines          2219     2238      +19     
  Branches        330      331       +1     
============================================
+ Hits           1515     1527      +12     
- Misses          571      576       +5     
- Partials        133      135       +2
Impacted Files Coverage Δ Complexity Δ
...res/graphql/jpa/query/schema/model/book/Genre.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...a/query/schema/impl/QraphQLJpaBaseDataFetcher.java 71.45% <100%> (+0.05%) 144 <2> (ø) ⬇️
...query/schema/impl/GraphQLJpaSimpleDataFetcher.java 91.3% <100%> (ø) 7 <1> (ø) ⬇️
...query/autoconfigure/GraphQLJpaQueryProperties.java 84.61% <50%> (-6.3%) 12 <1> (+1)
.../query/schema/impl/GraphQLJpaQueryDataFetcher.java 92.63% <50%> (-1.06%) 24 <0> (+1)
...jpa/query/schema/impl/GraphQLJpaSchemaBuilder.java 87.65% <72.72%> (-0.41%) 119 <0> (ø)
...ry/schema/impl/GraphQLJpaOneToManyDataFetcher.java 73.84% <81.81%> (+0.16%) 15 <3> (+2) ⬆️
...hql/jpa/query/schema/impl/JpaPredicateBuilder.java 52.22% <0%> (+0.4%) 56% <0%> (+1%) ⬆️

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 d452620...e08ab4f. Read the comment docs.

@igdianov
Copy link
Collaborator Author

igdianov commented May 6, 2019

@molexx ^^^^^

I understand why you need left outer joins, so I added optional argument for plural attributes. May be default optional should be configurable via Builder Api?

@igdianov igdianov changed the title feat: add optional argument parameter for plural attributes feat: add optional argument for plural attributes May 6, 2019
@igdianov igdianov merged commit e2ddd4d into master May 7, 2019
@igdianov igdianov deleted the igdianov-optional-one-to-many branch May 7, 2019 03:55
@igdianov igdianov restored the igdianov-optional-one-to-many branch May 7, 2019 04:06
@igdianov igdianov deleted the igdianov-optional-one-to-many branch May 7, 2019 04:11
@molexx
Copy link
Contributor

molexx commented May 7, 2019

Aha, this is great, thanks!

Yes works ok.

Allowing the optionals by default is good for me, I haven't had to add it in the query or make an API call. Adding to the builder would keep consistency with 'distinct'.

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

Successfully merging this pull request may close these issues.

2 participants