500 error on MR approvers edit page
Observed on gitlab.com.
worker2 production.log: Completed 500 Internal Server Error in 287ms (ActiveRecord: 11.7ms)
worker2 production.log:
worker2 production.log: ActionView::Template::Error (undefined method `name' for nil:NilClass):
worker2 production.log: 54: %ul.well-list
worker2 production.log: 55: - @project.approvers.each do |approver|
worker2 production.log: 56: %li
worker2 production.log: 57: = link_to approver.user.name, approver.user
worker2 production.log: 58: .pull-right
worker2 production.log: 59: = link_to namespace_project_approver_path(@project.namespace, @project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove approver' do
worker2 production.log: 60: = icon("sign-out")
worker2 production.log: app/views/projects/_merge_request_settings.html.haml:57:in `block in _app_views_projects__merge_request_settings_html_haml___545752523034962009_57962180'
I checked the Rails console, this project has a number of approvers but the user ID of the last one is 0.
Designs
- Show closed items
Related branches 7
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- Developer
/cc @JobV
- Contributor
Yeah, that's already fixed in my branch. I'm working on tests and will push it.
1 - Contributor
The problem is that this code a bit differs in master and stable. So I think I will need to create a separate fix for stable.
- Developer
@vsizov OK, consider fixing in master and them changing the code when you resolve the merge request.
- Contributor
The first part of fix https://dev.gitlab.org/gitlab/gitlab-ee/merge_requests/467. Should be merged to master
- Contributor
@jacobvosmaer I observed this bug in the master then I fixed it. But I could not reproduce it in the stable. Do you have some more information about it? For example, how many approvers records do we have with user_id 0 (SELECT * FROM approvers WHERE user_id = 0)
- Author Contributor
irb(main):003:0> Approver.where(user_id: 0).count => 1
- Author Contributor
That one approver is from the MR that started this issue.
@vsizov I can give you more complete data via a non-public channel, ping me on slack if you need it.
- Contributor
@jacobvosmaer I received information from you through the slack but it didn't help me. I propose to port validator for approver item from master branch to stable in next patch release. So we will know exactly where and when this bug comes up.
- Author Contributor
@vsizov do we know how to reproduce this problem?
- Contributor
@jacobvosmaer no. As I said we will know it when add validation.
- Author Contributor
@vsizov what about the one broken approver on gitlab.com? Delete it?
What if other installations have broken approvers too?
- Contributor
https://gitlab.com/gitlab-org/gitlab-ee/issues/1#note_1740590 It will be handled by MR I created above.
- Author Contributor
@vsizov great!
- Developer
@jacobvosmaer Can this be closed?
- Author Contributor
I am not sure if I understand the status of this issue.
We had a project on gitlab.com with inconsistent data associated with it (MR approvers with user ID 0). We deployed a migration that deletes existing invalid MR approver records.
But what have we done to prevent such records from being created in the future? @vsizov can you help me, I don't see it in the issue above.
- Job van der Voort mentioned in commit b616bc81
mentioned in commit b616bc81
- Shawn Debnath mentioned in issue #192 (closed)
mentioned in issue #192 (closed)
- Valery Sizov mentioned in commit cvd/gitlab-ce@cd017c88
mentioned in commit cvd/gitlab-ce@cd017c88
- Maintainer
@vsizov Can you answer @jacobvosmaer question above? Similar to #253 (closed), I have another customer with the same problem.
@jacobvosmaer Can you point me to the migration that removed invalid records?
- Maintainer
Ah, I found the migration in https://dev.gitlab.org/gitlab/gitlab-ee/merge_requests/467/diffs. I can confirm that this issue is recurring after this point so obviously we still have a validation issue somewhere.
- Maintainer
Reopened #253 (closed). Continue discussion there.
- Contributor
@dblessing @jacobvosmaer Sorry for the late answer, it looks like there were problems with notifications at that moment, I never ignore mentions :) So the answer is: Yes we have to delete invalid data (MR above) and prevent creating new invalid data, the last thing was in a separate MR and here is the actual fix - https://dev.gitlab.org/gitlab/gitlab-ee/commit/37ae86231de2dd234267443ee2e80fc23b64627b#ea3f59702ce4cbdaefcd816e6b7e53e81624d076_18_17
Edited by Valery Sizov - Maintainer
@vsizov The fixing of bad data was some time ago, right? July of 2015? I think we need to add a new migration that checks:
DELETE FROM approvers WHERE user_id NOT IN (SELECT id FROM users);
Here the issue is not that
user_id == 0
, it's that the user no longer exists. - Drew Blessing mentioned in issue #253 (closed)
mentioned in issue #253 (closed)
- Contributor
@dblessing Yes, that's another problem. The
dependent:destroy
for approvers has been added 3 months ago (https://gitlab.com/gitlab-org/gitlab-ee/commit/a5e1e352727ae51243010629845b7e1cdc92490d), so we have 3-4 months window for invalid data in approvers table.Edited by Valery Sizov - Contributor
I don't think it would hurt to add a migration that scrubs the bad data, if there is any.
Edited by Robert Speicher - Author Contributor
I don't think it would hurt to add a migration that scrubs the bad data
As long as the migration is fast! Scrubbing all data, when done wrong (for example loading each Project object into Ruby) takes way too long on large GitLab installations.
- Contributor
@jacobvosmaer Query in question is https://gitlab.com/gitlab-org/gitlab-ee/issues/1#note_4286417; that should be fast, no?
- Author Contributor
@rspeicher yes that looks fast enough.
- Contributor
@dblessing So if you think it'd be useful to add a migration for this please do so, or feel free to pass to me. We can add it to the next patch release (cc @rymai).
- Contributor
@dblessing Do you think we need a migration for this?
- Drew Blessing mentioned in merge request !506 (closed)
mentioned in merge request !506 (closed)
- Robert Speicher mentioned in commit e2692686
mentioned in commit e2692686
- Valery Sizov Mentioned in commit pfjason/gitlab-ce@cd017c88
Mentioned in commit pfjason/gitlab-ce@cd017c88
- Robert Speicher Mentioned in commit e2692686
Mentioned in commit e2692686
- Timothy Andrew mentioned in commit d71ad49f
mentioned in commit d71ad49f
- Nick Thomas mentioned in issue #1606 (closed)
mentioned in issue #1606 (closed)
- Sean McGivern mentioned in merge request !1480 (closed)
mentioned in merge request !1480 (closed)
- Brandon Bohling mentioned in issue #2245 (closed)
mentioned in issue #2245 (closed)
- Nick Thomas mentioned in issue #2360 (closed)
mentioned in issue #2360 (closed)
- macta mentioned in issue #2370 (closed)
mentioned in issue #2370 (closed)
- Chad Malchow mentioned in issue #688
mentioned in issue #688
- Joshua Lambert mentioned in issue #3046 (closed)
mentioned in issue #3046 (closed)
- Ryan McLaughlin mentioned in issue #3561 (closed)
mentioned in issue #3561 (closed)
- Sarrah Vesselov mentioned in issue #4000 (closed)
mentioned in issue #4000 (closed)
- Nick Thomas mentioned in issue #4607 (moved)
mentioned in issue #4607 (moved)
- Rémy Coutable closed via merge request !4556 (merged)
closed via merge request !4556 (merged)
- Rémy Coutable mentioned in commit 5f22c2d4
mentioned in commit 5f22c2d4
- Mark Pundsack mentioned in issue #3839 (closed)
mentioned in issue #3839 (closed)
- Sarrah Vesselov mentioned in issue #4830 (closed)
mentioned in issue #4830 (closed)
- Jeremy Watson (ex-GitLab) mentioned in issue #5674 (moved)
mentioned in issue #5674 (moved)
- Nick Thomas mentioned in issue #4517
mentioned in issue #4517
- Jabreal Johnson mentioned in issue #5053
mentioned in issue #5053
- blackst0ne mentioned in issue #6932 (closed)
mentioned in issue #6932 (closed)
- Alvaro Hernandez mentioned in issue #7420
mentioned in issue #7420
- Chris Cox mentioned in issue #8058 (moved)
mentioned in issue #8058 (moved)