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.

Sign up for GitHub

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

Jump to bottom

GraphQL Error for hasMany relation where some records are denied read access, but others are not #9236

Open
6TELOIV opened this issue Nov 15, 2024 · 3 comments · May be fixed by #9307
Open

GraphQL Error for hasMany relation where some records are denied read access, but others are not #9236

6TELOIV opened this issue Nov 15, 2024 · 3 comments · May be fixed by #9307
Labels
status: needs-triage Possible bug which hasn't been reproduced yet

Comments

@6TELOIV
Copy link

6TELOIV commented Nov 15, 2024

Describe the Bug

When using access control to restrict access to a collection, the GraphQL api will give errors and not return the correct data if some but not all records are denied readability in a relation field with many related records.

If all are read-allowed, it works fine. If all are denied, it's also fine. But if some are allowed and some are not, it returns null for the entire field.

Link to the code that reproduces this issue

https://github.com/6TELOIV/graphql-null-relations

Reproduction Steps

  1. Run the server on your local machine with a local PostgreSQL DB

  2. On the DemoGlobal, add multiple related Demo items, checking visible on some but not others.

  3. Visit localhost:3000/api/graphql-playground in a private window/logged out context

  4. Input the following query:

    query demo {
  DemoGlobal {
    demoRelationship {
      id
    }
  }
}

  5. Observe the error:

    Error: Cannot return null for non-nullable field DemoGlobal.demoRelationship.

    {
  "errors": [
    {
      "extensions": {
        "name": "Error",
        "statusCode": 500
      },
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "message": "Something went wrong.",
      "path": [
        "DemoGlobal",
        "demoRelationship",
        1
      ]
    }
  ],
  "data": {
    "DemoGlobal": {
      "demoRelationship": null
    }
  }
}

Which area(s) are affected? (Select all that apply)

area: core, db-postgres

Environment Info

Payload: 3.0.0-beta.130
Node: 22.11.0
NextJS: 15.0.0
@6TELOIV 6TELOIV added status: needs-triage Possible bug which hasn't been reproduced yet v3 validate-reproduction labels Nov 15, 2024
@6TELOIV
Copy link
Author

6TELOIV commented Nov 15, 2024

Initial investigation and details present in this discord thread: https://discord.com/channels/967097582721572934/1307023111056785418

It seems to maybe be an inconsistent issue, but that min repro seems to consistently have the issue.

@6TELOIV
Copy link
Author

6TELOIV commented Nov 18, 2024

The plot thickens.

on my test environment (the min repro attached), I have 4 Demo records stored in the demoRelationship field.
Image

If there is a contiguous group of visibile records, followed by a contiguous group of not-visible records, the query works. In all other cases, the error occurs. For example, if 1 and 2 are visible, but 3 and 4 are not, the query works. Same behavior occurs with more records.

I tested reordering them as well, and what matters is the position in the relationship, not the ID of the record. So if you reordered the above to 3, 1, 4, 2, then visibility where the query works are:

  • All visible
  • 3, 1, 4 visible
  • 3, 1 visible
  • 3 visible
  • none visible

All other visibility settings for that order will fail with Error: Cannot return null for non-nullable field DemoGlobal.demoRelationship

@6TELOIV
Copy link
Author

6TELOIV commented Nov 18, 2024

https://github.com/payloadcms/payload/blob/main/packages/graphql/src/schema/buildObjectType.ts#L432-L471

The issue seems to be here. The field is supposed to be an array of non-null values, but it inserts false-y values into the array (results[i] = result on L467).

A simple solutution is to modify L480 to return results.filter(result => result !== null). I'm not sure if there would be a more efficient way to do this in parallel. If you changed createPopulationPromise to push, then you cannot guarantee the order. I think a syncronous once-over to filter out a constant value (null) shouldn't be too much of a performance impact.

I can implement this change, create some tests, and submit a PR if this solution sounds good to the maintainers.

@6TELOIV 6TELOIV linked a pull request Nov 18, 2024 that will close this issue
Open
@denolfe denolfe removed the v3 label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: needs-triage Possible bug which hasn't been reproduced yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(graphql): gql relationship has many null values 6TELOIV/payload
2 participants
@denolfe @6TELOIV