Skip to content

Releases create service associates all milestones if `milestones: nil`

The following discussion from !46263 (merged) should be addressed:

  • @nfriend started a discussion: (+4 comments)

    I'm being extra careful that milestones is never passed nil, because strangely enough this results in the release being associated to all milestones.

Summary

If Releases::CreateService#execute is used to create a release with a milestones: nil parameter, a release is created that is associated to all project milestones.

Expected behavior

Passing milestones: nil to #execute behaves the same as if milestones: [] was passed: a release is created with no milestone associations.

Actual behavior

Passing milestones: nil to #execute associates the newly-created release to all project milestones.

Steps to reproduce

  1. Apply this Git patch, which adds a new test case for this scenario:

    diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb
    index b9294182883..de346bcf6d7 100644
    --- a/spec/services/releases/create_service_spec.rb
    +++ b/spec/services/releases/create_service_spec.rb
    @@ -167,28 +167,45 @@
          end
        end
    
    -    context 'when no milestone is passed in' do
    -      it 'creates a release without a milestone tied to it' do
    -        expect(params.key?(:milestones)).to be_falsey
    +    context 'no milestone association behavior' do
    +      let(:title_1) { 'v1.0' }
    +      let(:title_2) { 'v1.0-rc' }
    +      let!(:milestone_1) { create(:milestone, :active, project: project, title: title_1) }
    +      let!(:milestone_2) { create(:milestone, :active, project: project, title: title_2) }
    
    -        service.execute
    -        release = project.releases.last
    +      context 'when no milestones parameter is passed' do
    +        it 'creates a release without a milestone tied to it' do
    +          expect(params.key?(:milestones)).to be_falsey
    +
    +          service.execute
    +          release = project.releases.last
    +
    +          expect(release.milestones).to be_empty
    +        end
    
    -        expect(release.milestones).to be_empty
    +        it 'does not create any new MilestoneRelease object' do
    +          expect { service.execute }.not_to change { MilestoneRelease.count }
    +        end
          end
    
    -      it 'does not create any new MilestoneRelease object' do
    -        expect { service.execute }.not_to change { MilestoneRelease.count }
    +      context 'when an empty array is passed as the milestones parameter' do
    +        it 'creates a release without a milestone tied to it' do
    +          service = described_class.new(project, user, params.merge!({ milestones: [] }))
    +          service.execute
    +          release = project.releases.last
    +
    +          expect(release.milestones).to be_empty
    +        end
          end
    -    end
    
    -    context 'when an empty value is passed as a milestone' do
    -      it 'creates a release without a milestone tied to it' do
    -        service = described_class.new(project, user, params.merge!({ milestones: [] }))
    -        service.execute
    -        release = project.releases.last
    +      context 'when nil is passed as the milestones parameter' do
    +        it 'creates a release without a milestone tied to it' do
    +          service = described_class.new(project, user, params.merge!({ milestones: nil }))
    +          service.execute
    +          release = project.releases.last
    
    -        expect(release.milestones).to be_empty
    +          expect(release.milestones).to be_empty
    +        end
          end
        end
      end
  2. Run the test:

    bundle exec bin/rspec spec/services/releases/create_service_spec.rb

The new test case will fail.

Edited by Nathan Friend