Revert "Merge branch 'fix-vuln-graphql-performance' into 'master'"
Purpose of revert
!116643 (merged) introduces a severity1 regression for Query.vulnerability
.
The issue is that field authorization checks against the current object and
Types::QueryType
does not have one. So, we're essentially trying to docan?(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:
- It introduces a severity1 regression (broken feature with no workaround)
- The N+1 query issue it fixes is not currently causing reliability problems
- The solution is sub-optimal
Before revert | After revert |
---|---|
Checklist
-
Create an issue to reinstate the merge request and assign it to the author of the reverted merge request. -
If the revert is to resolve a broken 'master' incident, please read through the Responsibilities of the Broken master
resolution DRI. -
If the revert involves a database migration, please read through Deleting existing migrations. -
Add the appropriate labels before the MR is created. We can skip CI/CD jobs only if the labels are added before the CI/CD pipeline is created.
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