Adapter-knex: Wrong handling of 'where' parameter for relationships in QueryBuilder._addWheres

[stefankirchfeld]

Bug report

Describe the bug

Non-many relationships get wrong "where" condition object passed to recursive call in _addWheres, which later leads to the fact that the fieldAdapter is not found and the code goes into the "Many relationships" codeblock, leading finally to the error:

"GraphQL error: Cannot destructure property `rel` of 'undefined' or 'null'. GraphQL error: Cannot destructure property `rel` of 'undefined' or 'null'. GraphQL error: Cannot destructure property `rel` of 'undefined' or 'null'."

This error is directly displayed in the Admin UI in such cases (see screenshot below).

To Reproduce

1. Create a simple app with 'User" list and any object which has an "owner" property:

// User object as in example app
keystone.createList('User', {
    fields: {
      // the usual
    },
    // List-level access controls
    access: {
      read: access.userIsAdminOrOwner,
      update: access.userIsAdminOrOwner,
      create: access.userIsAdmin,
      delete: access.userIsAdmin,
      auth: true,
    },
  })

// declare access check which returns a GraphQL query on the related User list
// (as described in tutorials)
const userOwnsItem = ({ authentication: { item: user } }) => {
  if (!user) {
    return false
  }

  // Instead of a boolean, you can return a GraphQL query:
  // https://www.keystonejs.com/api/access-control#graphqlwhere
  return { owner: user.id }
}

// Create another list with "owner" access control
keystone.createList('SomeList', {
    fields: {
      owner: { type: AuthedRelationship, ref: 'User', isRequired: true },
      // other fields
    },
    // List-level access controls
    access: {
      read: userOwnsItem, // use access check returning subquery
      update: userOwnsItem,
      create: true,
      delete: userOwnsItem,
    },
  })

2.) Start admin UI and try to access the "SomeList". You will see above error, because the

{ owner: user.id } access query triggers the bug described above.

Expected behaviour

Fix where condition

Looking through the code, I believe the code needs to be changed as:

adapter-knex.js

   line 769:  { id: where[path] },    // instead of just where[path]

because where = { owner: "1" }, so 'where[path]' will just evaluate to "1", which is not a valid input for the next _addWheres call. It needs to be wrapped into a proper where condition. Assuming that the name of the primary key column is always "id", this should be as given above.

Fix tableAlias parameter

Apart from that, I think also the following needs to be changed:

adapter-knex.js

  line 683: path => !this._getQueryConditionByPath(listAdapter, path, tableAlias) // add 'tableAlias' to parameters

The tableAlias was missing here, which sometimes lead to 'undefined.' alias declarations down the line

Screenshots

System information

• OS: MacOs
• Node 12

[timleslie]

Thank you for the detailed bug report and investigation. At first glance your suggested changes look very reasonable. My plan is to write some test cases which can trigger the bug and then verify that your suggestions fix the issue.

[stefankirchfeld]

Hi Tim, thanks, sounds good. Looking forward to the fix.

[timleslie]

Hi @stefankirchfeld, I've drilled into this and I've found the problem:

  return { owner: user.id }

should be:

  return { owner: { id: user.id } }

If we try to perform the same query directly against the graphQL API:

{
  allThings(where: { owner: "1" }) {
    id
    name
  }
}

we get the error:

"Expected type UserWhereInput, found \"1\".",

Unfortunately during the access control phase we circumvent the GraphQL type checking, which means the invalid type ends up deep in the resolver and triggers the error messages that you saw.

So I don't think this is technically a bug, but it does highlight a less than ideal developer experience. We should probably try to solve this by pushing this category of access control through the server side of the graphQL so that we can get consistent error messaging, rather than directly accessing the resolvers, which can have rather obscure failure modes when given unexpected data.

[stefankirchfeld]

Hi Tim,

ok, I see, thanks! Yes, then indeed better error messages would be helpful here.

To me it was not obvious that in the access query I have to wrap the "owner" id in another object reflecting the actual referenced entity structure. I guess this is because of the field type "Relationship"/"AuthedRelationship"?

[stefankirchfeld]

What about the other change about the "tableAlias" ? I did not really observe an actual bug because of it, but I saw in the debugger table aliases being constructed with "undefined.", so that did not look too great ;)

[timleslie]

Those undefined tableAlias values are actually intentional (although that might not be immediately obvious).

[stale[bot]]

It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :)

[bladey]

Keystone 5 has officially moved into active maintenance mode as we push towards the next major new version Keystone Next, you can find out more information about this transition here.

In an effort to sustain the project going forward, we're cleaning up and closing old issues such as this one. If you feel this issue is still relevant for Keystone Next, please let us know.