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

GraphQL: DX Relational Where Query #6255

Merged
merged 7 commits into from
Dec 5, 2019

Conversation

Moumouls
Copy link
Member

@Moumouls Moumouls commented Dec 3, 2019

Allow to execute complexe relational queries like:

Query: Find countries containing companies that contains at least one employee named "John"

query {
  countries(where: {
    companies: { employees: { name: { equalTo: "John" } } },
  }) {
    edges {
      node {
        id
        objectId
        companies {
          edges {
            node {
              id
              objectId
              name
              employees {
                edges {
                  node {
                    name
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

This system works for Pointer and Relation Parse fields

@Moumouls Moumouls requested a review from davimacedo December 3, 2019 17:50
@Moumouls Moumouls changed the title GraphQL: Relational Where Query GraphQL: DX Relational Where Query Dec 3, 2019
@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #6255 into master will decrease coverage by 0.09%.
The diff coverage is 93.65%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6255     +/-   ##
=========================================
- Coverage   94.01%   93.92%   -0.1%     
=========================================
  Files         169      169             
  Lines       11529    11564     +35     
=========================================
+ Hits        10839    10861     +22     
- Misses        690      703     +13
Impacted Files Coverage Δ
src/GraphQL/loaders/parseClassQueries.js 97.95% <ø> (+0.08%) ⬆️
src/GraphQL/loaders/parseClassTypes.js 94.23% <100%> (-0.65%) ⬇️
src/GraphQL/helpers/objectsQueries.js 90.35% <100%> (ø) ⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js 97.03% <100%> (ø) ⬆️
src/GraphQL/transformers/constraintType.js 86.95% <75%> (-8.05%) ⬇️
src/GraphQL/transformers/query.js 76.74% <93.75%> (+2.83%) ⬆️
src/middlewares.js 97.17% <0%> (-1.12%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.74% <0%> (-0.71%) ⬇️
... and 7 more

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 1f54d02...e1be5ae. Read the comment docs.

@Moumouls
Copy link
Member Author

Moumouls commented Dec 3, 2019

Everything should be okay now 🚀

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I have few comments.

@@ -964,8 +921,6 @@ const STRING_WHERE_INPUT = new GraphQLInputObjectType({
in: inOp(GraphQLString),
notIn: notIn(GraphQLString),
exists,
inQueryKey,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not remove these options from string, float, id, etc. With these options, the users are currently able to do something like:

countries(where: { continent: { inQueryKey: { query: { className: "Country", where: { name: { equalTo: "US" } } }, key: "continent" } } }) {
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hummm, from my point of view it's hard to understand this query without reading a documentation. The inQueryKey feature is interesting to implement but i think we need first to re-design the input before re-implementing this feature. 😄 ? like playing with Enum for key and className, but this is not the purpose of this PR... We need to avoid people using inputs that can be deprecated in a near futur 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think it would be better keep it here while we don't improve. Someone can be already using this feature and they will have no alternative.

src/GraphQL/loaders/parseClassTypes.js Show resolved Hide resolved
src/GraphQL/transformers/constraintType.js Show resolved Hide resolved
src/GraphQL/transformers/query.js Show resolved Hide resolved
@Moumouls
Copy link
Member Author

Moumouls commented Dec 4, 2019

So, to avoid some confusion between regular where and relational where, and improve DX complex relational queries i choose to use key have & haveNot

I added support of exists on Relation too, via $inQuery

Result:

{
  countries(
    where: { companies: { haveNot: { name: { equalTo: "imACompany1" } }, } }
  ) {
    edges {
      node {
        name
        president {
          name
        }
        companies {
          edges {
            node {
              name
            }
          }
        }
      }
    }
  }
}

@Moumouls
Copy link
Member Author

Moumouls commented Dec 4, 2019

Tests are okay (postgres tested with my local set up), just (again) a Postgres fail...

@dplewis
Copy link
Member

dplewis commented Dec 4, 2019

@vitaly-t We have a flaky test with Postgres on Travis-CI. here

could not write to file "pg_xlog/xlogtemp.8833": No space left on device

Is there a way to turn off WAL or logging? Any help would be appreciated.

@davimacedo
Copy link
Member

davimacedo commented Dec 4, 2019

@Moumouls great job with the have, haveNot, and exists!

I think have (and haveNot) are perfect names for the pointer:

persons(where: { spouse: { have: { name: { equalTo: "John" } } } }) { ... }

But for relation is little bit strange to me:

persons(where: { children: { have: { name: { equalTo: "John" } } } }) { ... }

It looks to me that it should only return persons whose ALL children have name equal to "John". But that's not the case, right?

I can't think a better name though. Maybe existWith? I am not good in giving names :) so if you prefer I am also okay in moving forward with the current ones.

Another minor comment. For relation, it should probably be better exist instead of exists:

countries(where: { companies: { exist: true } }) { ... }

@davimacedo
Copy link
Member

davimacedo commented Dec 4, 2019

@dplewis Postgres tests are hanging in many branches. Sometimes it works for all of them then it stops working for all of them. I guess it is a Travis issue or at least some misconfiguration from our side...

@dplewis
Copy link
Member

dplewis commented Dec 4, 2019

@davimacedo I opened a PR to address this. Hopefully it works

@vitaly-t
Copy link
Contributor

vitaly-t commented Dec 4, 2019

@dplewis I'm not sure about logging, as it does not seem relevant to the pg-promise library, which does not log those things...

Especially for the error related to writing something to a file. The library never writes anything to a file.

@dplewis
Copy link
Member

dplewis commented Dec 4, 2019

@Moumouls The build should be good now.

@Moumouls
Copy link
Member Author

Moumouls commented Dec 4, 2019

@Moumouls great job with the have, haveNot, and exists!

I think have (and haveNot) are perfect names for the pointer:

persons(where: { spouse: { have: { name: { equalTo: "John" } } } }) { ... }

But for relation is little bit strange to me:

persons(where: { children: { have: { name: { equalTo: "John" } } } }) { ... }

It looks to me that it should only return persons whose ALL children have name equal to "John". But that's not the case, right?

I can't think a better name though. Maybe existWith? I am not good in giving names :) so if you prefer I am also okay in moving forward with the current ones.

Another minor comment. For relation, it should probably be better exist instead of exists:

countries(where: { companies: { exist: true } }) { ... }

I think we will keep the have, need more documentation on the field maybe.

@davimacedo
Copy link
Member

Ok. No problem. Can you please only address my last open conversation and let's go to merge?

@Moumouls
Copy link
Member Author

Moumouls commented Dec 5, 2019

Seems to have a random should work either with global id or object id test

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! LGTM!

@davimacedo davimacedo merged commit 5d76b2f into parse-community:master Dec 5, 2019
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* DX Relational Where Query

* Remove WherePointer & fix tests

* Add have, haveNot, exists on Pointer/Relation where input

* Merge branch 'master' into gql-relational-query

* Enable inQueryKey

* better descrption
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.

4 participants