Skip to content

Revert "Merge branch 'fix-vuln-graphql-performance' into 'master'"

Brian Williams requested to merge revert-4ac715e8 into master

Purpose of revert

!116643 (merged) introduces a severity1 regression for Query.vulnerability.

Summary:

The issue is that field authorization checks against the current object and Types::QueryType does not have one. So, we're essentially trying to do can?(current_user, :read_vulnerability, nil) which always returns false. The fact that the tests are passing does signal that we have a test gap.

I also really dislike the idea to fix N+1 queries by removing type authorizations. I think type authorizations are a valuable safety net. Instead, I would prefer to fix these by ensuring the project associations are preloaded.

I think we should revert this change given these considerations:

  1. It introduces a severity1 regression (broken feature with no workaround)
  2. The N+1 query issue it fixes is not currently causing reliability problems
  3. The solution is sub-optimal
Before revert After revert
Screenshot_2023-05-03_at_3.23.09_PM Screenshot_2023-05-03_at_3.22.22_PM

Checklist

Milestone info

  • I am reverting something in the current milestone. No changelog is needed, and I've added a ~"regression:*" label.
  • I am reverting something in a different milestone. A changelog is needed, and I've removed the ~"regression:*" label.

Related issues and merge requests

Edited by Brian Williams

Merge request reports