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

Nested relationship filters #272

Merged
merged 7 commits into from
Aug 28, 2018
Merged

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented Aug 28, 2018

Fixes #210

You can now do queries like so:

query {
  allUsers {
    id
    posts(
      where: { title_contains: 'React' }
      first: 3
    ) {
      id
      title
    }
  }
}

It accepts all the same filtering arguments as a top-level allPosts query would 😍

I've left comments in line below for your guidance.

@@ -140,45 +139,6 @@ describe('Access control package tests', () => {
expect(() => parseFieldAccess({ access: 10 })).toThrow(Error);
});

test('mergeWhereClause', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shifted these tests (and the associated function) out to @keystonejs/utils because they're not access-control specific, and it's a generic function.

@@ -272,6 +270,7 @@ module.exports = class List {
id_in: [ID!]
id_not_in: [ID!]
${queryArgs}
AND: [${this.gqlNames.whereInputName}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This enables the AND query syntax. (see the tests)

@@ -300,15 +299,18 @@ module.exports = class List {
return types;
}

getAdminGraphqlQueries() {
const commonArgs = `
getGraphqlFilterFragment() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this its own function allows the Relationship type to pull in the same input types for nested fields.


if (many) {
return `${this.path}(
${this.getListByKey(ref).getGraphqlFilterFragment()}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the nested relationship input type. ie;

author(UserWhereInput): [User]

): [${ref}]`;
}

return `${this.path}: ${ref}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to-one relationships can't have any filters, etc, applied.

We can revisit this in the future if we think it's useful to add.


let server;

beforeAll(() => {
server = setupServer({
name: 'Tests queries and relationships',
name: `ks5-testdb-${cuid()}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally used the same name in two different test files, and got weird failures until I realised that the dropDatabase command was nuking the DB mid-test from another file (because of the way jest parallelises tests). To protect agains that silly mistake in the future, I've made each test have a unique database name.

resolveAllKeys(
afterAll(async () => {
// clean the db
await resolveAllKeys(mapKeys(server.keystone.adapters, adapter => adapter.dropDatabase()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes that random database name, so your local mongo instance isn't full of cruft.


test('explicitly filters when given a `where` clause', () => {});

test('nested to-single relationships can be filtered within AND clause', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that these tests are more robust query tests. They are not nested query tests (see further below for those). Included here as I wanted to cover some more edge cases to be certain I hadn't broken anything.

id
posts (where: {
content_contains: "hi",
}){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh, Aaaaahh.

notes {
id
content
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Access Control means these can't be read (rightfully so).

Copy link
Contributor

@dominikwilkowski dominikwilkowski left a comment

Choose a reason for hiding this comment

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

nice!

@jesstelford
Copy link
Contributor Author

🎉 Will merge once CI passes

@jesstelford jesstelford merged commit 137432e into master Aug 28, 2018
@jesstelford
Copy link
Contributor Author

Oh god that feels good.

@jesstelford jesstelford deleted the nested-relationship-filters branch August 28, 2018 06:35
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