GraphQL: getting MR diff discussions often returns 500
Summary
When we ask GraphQL for discussions (including position
) on an MR where the source project is a fork, the API call fails with 500 Internal Server Error
.
Steps to reproduce
In production
Try to get discussions on gitlab-vscode-extension!155 (merged) gitlab-vscode-extension!157 (closed)
Copy-paste GraphQL query
put the following query to https://gitlab.com/-/graphql-explorer
fragment discussions on DiscussionConnection {
pageInfo {
hasNextPage
endCursor
}
nodes {
replyId
createdAt
notes {
pageInfo {
hasNextPage
endCursor
}
nodes {
id
createdAt
system
author {
avatarUrl
name
username
webUrl
}
body
bodyHtml
position {
diffRefs {
baseSha
headSha
}
filePath
positionType
newLine
oldLine
newPath
oldPath
positionType
}
}
}
}
}
query GetMrDiscussions{
project(fullPath: "gitlab-org/gitlab-vscode-extension") {
id
mergeRequest(iid: "155") {
id
discussions {
...discussions
}
}
}
}
Locally
- Create an MR
- Create a comment on the "Overview page" (not on the diff)
- Edit the text of this comment (e.g. add a few characters at the end)
- Now try to use the above GraqhQL query to get discussions on this MR
The issue is caused in the part that gets position
for a note. When you remove the position
part from the GraphQL query, the query returns successfully.
What is the current bug behavior?
API returns 500
error.
What is the expected correct behavior?
I'd at least see the discussions without a position, but ideally, I'd see the normal response including position.
Relevant logs and/or screenshots
Looking through production logs I see:
stacktrace:
app/graphql/types/base_field.rb:46:in `block (2 levels) in resolve_field', app/graphql/types/base_field.rb:35:in `block in resolve_field', app/graphql/types/base_field.rb:31:in `resolve_field', lib/gitlab/graphql/calls_gitaly/instrumentation.rb:18:in `block in instrument', lib/gitlab/graphql/generic_tracing.rb:40:in `with_labkit_tracing', lib/gitlab/graphql/generic_tracing.rb:30:in `platform_trace', lib/gitlab/graphql/generic_tracing.rb:40:in `with_labkit_tracing', lib/gitlab/graphql/generic_tracing.rb:30:in `platform_trace', lib/gitlab/graphql/generic_tracing.rb:40:in `with_labkit_tracing', lib/gitlab/graphql/generic_tracing.rb:30:in `platform_trace', app/graphql/gitlab_schema.rb:43:in `multiplex', app/graphql/gitlab_schema.rb:50:in `execute', app/controllers/graphql_controller.rb:75:in `execute_query', app/controllers/graphql_controller.rb:32:in `execute', ee/lib/gitlab/ip_address_state.rb:10:in `with', ee/app/controllers/ee/application_controller.rb:44:in `set_current_ip_address', app/controllers/application_controller.rb:482:in `set_current_admin', lib/gitlab/session.rb:11:in `with_session', app/controllers/application_controller.rb:473:in `set_session_storage', lib/gitlab/i18n.rb:73:in `with_locale', lib/gitlab/i18n.rb:79:in `with_user_locale', app/controllers/application_controller.rb:467:in `set_locale', lib/gitlab/error_tracking.rb:52:in `with_context', app/controllers/application_controller.rb:532:in `sentry_context', app/controllers/application_controller.rb:460:in `block in set_current_context', lib/gitlab/application_context.rb:56:in `block in use', lib/gitlab/application_context.rb:56:in `use', lib/gitlab/application_context.rb:22:in `with_context', app/controllers/application_controller.rb:451:in `set_current_context', ee/lib/omni_auth/strategies/group_saml.rb:41:in `other_phase', lib/gitlab/metrics/elasticsearch_rack_middleware.rb:16:in `call', lib/gitlab/middleware/rails_queue_duration.rb:33:in `call', lib/gitlab/metrics/rack_middleware.rb:16:in `block in call', lib/gitlab/metrics/transaction.rb:56:in `run', lib/gitlab/metrics/rack_middleware.rb:16:in `call', lib/gitlab/request_profiler/middleware.rb:17:in `call', ee/lib/gitlab/database/load_balancing/rack_middleware.rb:39:in `call', lib/gitlab/jira/middleware.rb:19:in `call', lib/gitlab/middleware/go.rb:20:in `call', lib/gitlab/etag_caching/middleware.rb:21:in `call', lib/gitlab/middleware/multipart.rb:234:in `call', lib/gitlab/middleware/read_only/controller.rb:50:in `call', lib/gitlab/middleware/read_only.rb:18:in `call', lib/gitlab/middleware/same_site_cookies.rb:27:in `call', lib/gitlab/middleware/handle_malformed_strings.rb:21:in `call', lib/gitlab/middleware/basic_health_check.rb:25:in `call', lib/gitlab/middleware/handle_ip_spoof_attack_error.rb:25:in `call', lib/gitlab/middleware/request_context.rb:23:in `call', config/initializers/fix_local_cache_middleware.rb:9:in `call', lib/gitlab/metrics/requests_rack_middleware.rb:76:in `call', lib/gitlab/middleware/release_env.rb:12:in `call'
error message:
RuntimeError
Failed to implement DiffPosition.diffRefs, tried:
- `Types::Notes::DiffPositionType#diff_refs`, which did not exist
- `String#diff_refs`, which did not exist
- Looking up hash key `:diff_refs` or `"diff_refs"` on `{}`, but it wasn't a Hash
To implement this field, define one of the methods above (and check for typos)
Output of checks
This bug happens on GitLab.com
Results of GitLab environment info
Expand for output related to GitLab environment info
(For installations with omnibus-gitlab package run and paste the output of: `sudo gitlab-rake gitlab:env:info`) (For installations from source run and paste the output of: `sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)
Results of GitLab application Check
Expand for output related to the GitLab application check
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true
)(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true
)(we will only investigate if the tests are passing)
Possible fixes
frontend was sending position: "{}"
in their request and backend was storing this value without validating it. This has already been resolved from the frontend perspective but backend still needs to:
-
Either reject the note if position is set to anything but nil or force it to be nil if it's a regular Note. But since DiffNote extends Note, we have to be careful that the validation only occurs on Note not DiffNote. -
Run a migration to set all existing "{}"
tonil
to fix already broken discussions