Commit 465e0e48 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix-pipeline-for-empty-merge-request-diff' into 'master'

Fix pipeline error when trying to read empty merge request diff

When a user pushed something which resulted an empty merge request diff, `st_commits` would be `nil`. Therefore we also need to check if there exists `st_commits`.

We could tell this from:

``` ruby
def commits
  @commits ||= load_commits(st_commits || [])
end
```

and

``` ruby
def save_commits
  new_attributes = {}

  commits = compare.commits

  if commits.present?
    commits = Commit.decorate(commits, merge_request.source_project).reverse
    new_attributes[:st_commits] = dump_commits(commits)
  end

  update_columns_serialized(new_attributes)
end
```

Closes #22438

See merge request !6470
parent b48d3893
...@@ -30,6 +30,10 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -30,6 +30,10 @@ class MergeRequestDiff < ActiveRecord::Base
select(column_names - ['st_diffs']) select(column_names - ['st_diffs'])
end end
def st_commits
super || []
end
# Collect information about commits and diff from repository # Collect information about commits and diff from repository
# and save it to the database as serialized data # and save it to the database as serialized data
def save_git_content def save_git_content
...@@ -83,7 +87,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -83,7 +87,7 @@ class MergeRequestDiff < ActiveRecord::Base
end end
def commits def commits
@commits ||= load_commits(st_commits || []) @commits ||= load_commits(st_commits)
end end
def reload_commits def reload_commits
......
...@@ -64,5 +64,27 @@ describe MergeRequestDiff, models: true do ...@@ -64,5 +64,27 @@ describe MergeRequestDiff, models: true do
end end
end end
end end
describe '#commits_sha' do
shared_examples 'returning all commits SHA' do
it 'returns all commits SHA' do
commits_sha = subject.commits_sha
expect(commits_sha).to eq(subject.commits.map(&:sha))
end
end
context 'when commits were loaded' do
before do
subject.commits
end
it_behaves_like 'returning all commits SHA'
end
context 'when commits were not loaded' do
it_behaves_like 'returning all commits SHA'
end
end
end end
end end
...@@ -517,7 +517,7 @@ describe MergeRequest, models: true do ...@@ -517,7 +517,7 @@ describe MergeRequest, models: true do
context 'with multiple irrelevant merge_request_diffs' do context 'with multiple irrelevant merge_request_diffs' do
before do before do
subject.update(target_branch: 'markdown') subject.update(target_branch: 'v1.0.0')
end end
it_behaves_like 'returning pipelines with proper ordering' it_behaves_like 'returning pipelines with proper ordering'
...@@ -544,13 +544,28 @@ describe MergeRequest, models: true do ...@@ -544,13 +544,28 @@ describe MergeRequest, models: true do
subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq subject.merge_request_diffs.flat_map(&:commits).map(&:sha).uniq
end end
before do shared_examples 'returning all SHA' do
subject.update(target_branch: 'markdown') it 'returns all SHA from all merge_request_diffs' do
expect(subject.merge_request_diffs.size).to eq(2)
expect(subject.all_commits_sha).to eq(all_commits_sha)
end
end
context 'with a completely different branch' do
before do
subject.update(target_branch: 'v1.0.0')
end
it_behaves_like 'returning all SHA'
end end
it 'returns all SHA from all merge_request_diffs' do context 'with a branch having no difference' do
expect(subject.merge_request_diffs.size).to eq(2) before do
expect(subject.all_commits_sha).to eq(all_commits_sha) subject.update(target_branch: 'v1.1.0')
subject.reload # make sure commits were not cached
end
it_behaves_like 'returning all SHA'
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment