Skip to content

Fix 500 errors when discussion notes reference commits in forks

username-removed-23026 requested to merge atavakoli/gitlab-ce:fix/nil_noteable_in_discussion into master

In app/models/note.rb, in def noteable, if the noteable_type == 'Commit' but the commit_id is not found in the note's project's repo, then it returns Nil for the noteable:

  # override to return commits, which are not active record
  def noteable
    if for_commit?
      project.repository.commit(commit_id)
    else
      super
    end
  # Temp fix to prevent app crash
  # if note commit id doesn't exist
  rescue
    nil
  end

This results in 500 errors when discussions are rendered (e.g. in MRs involving forked projects, if a commit in the source project has discussion notes on it and that commit is not in the destination project). Looking up that commit's ID in the destination repo will fail, causing note.noteable to be Nil, and note.noteable.short_id to throw a NoMethodError.

This is a better temporary fix, to just fall back to displaying note.commit_id[0...8] (which will exist for these discussion notes) in case noteable is empty, instead of trying to generate a link.

The long-term fix should be to detect if this is a commit on a forked repo and link to that commit; only if there is no such commit, fall back to just displaying the commit ID (though if this view is being called on a commit that doesn't exist, there is a bigger problem to be solved).

UPDATE 1: 2015-02-23 - The app/models/note.rb def noteable implementation has changed, so that instead of calling rescue, it returns nil instead. This is better, but still has the same issue in rendering the page.

I've rebased my branch to the latest master to resolve any conflicts. Please consider for acceptance, or please let me know how I can improve this MR.

UPDATE 2: 2015-02-23 - My mistake, the implementation for def noteable has not change; the same bug is present. The fix should still apply.

Merge request reports