Moving an issue to another project sometimes fails

Related note: I spotted the following notifications in our slack channel for issues raised:

Notice the internal ids for the issues are the same

We are also seeing this on our self-hosted instance. We have a few issues in a project (that was changed from internal to private, although this is not necessarily related) that cannot be moved to another one.

This is the response from the server with details removed:

{"method":"PUT","path":"/<removed>/Overview/issues/3.json","format":"json","controller":"Projects::IssuesController","action":"update","status":500,"error":"TypeError: no implicit conversion of nil into String","duration":449.93,"view":0.0,"db":50.84,"time":"2017-09-15T08:10:07.585Z","params":{"title":"<removed>","confidential":false,"description":"<removed>","lockedWarningVisible":false,"move_to_project_id":115,"updateLoading":true,"namespace_id":"<removed>","project_id":"Overview","id":"3","issue":{"title":"<removed>","description":"<removed>","confidential":false}},"remote_ip":"<removed>","user_id":6,"username":"ossi"}

And the full stack trace

TypeError (no implicit conversion of nil into String):
  lib/gitlab/gfm/reference_rewriter.rb:64:in `+'
  lib/gitlab/gfm/reference_rewriter.rb:64:in `unfold_reference'
  lib/gitlab/gfm/reference_rewriter.rb:44:in `block in rewrite'
  lib/gitlab/gfm/reference_rewriter.rb:43:in `gsub'
  lib/gitlab/gfm/reference_rewriter.rb:43:in `rewrite'
  app/services/issues/move_service.rb:106:in `block in rewrite_content'
  app/services/issues/move_service.rb:104:in `each'
  app/services/issues/move_service.rb:104:in `inject'
  app/services/issues/move_service.rb:104:in `rewrite_content'
  app/services/issues/move_service.rb:82:in `block in rewrite_notes'
  app/services/issues/move_service.rb:79:in `rewrite_notes'
  app/services/issues/move_service.rb:26:in `block in execute'
  app/services/issues/move_service.rb:21:in `execute'
  app/controllers/projects/issues_controller.rb:136:in `update'
  lib/gitlab/i18n.rb:46:in `with_locale'
  lib/gitlab/i18n.rb:52:in `with_user_locale'
  app/controllers/application_controller.rb:326:in `set_locale'
  lib/gitlab/middleware/multipart.rb:93:in `call'
  lib/gitlab/request_profiler/middleware.rb:14:in `call'
  lib/gitlab/middleware/go.rb:17:in `call'
  lib/gitlab/etag_caching/middleware.rb:11:in `call'
  lib/gitlab/request_context.rb:18:in `call'
  lib/gitlab/metrics/requests_rack_middleware.rb:27:in `call'

/cc @smcgivern

Is that possible that we can have referables with nil reference?

Exception is being raised from

new_text = before + cross_reference + after

in

      def unfold_reference(reference, match, target_project)
        before = @text[0...match.begin(0)]
        after = @text[match.end(0)..-1]

        referable = find_referable(reference)
        return reference unless referable

        cross_reference = build_cross_reference(referable, target_project)
        return reference if reference == cross_reference

        new_text = before + cross_reference + after
        substitution_valid?(new_text) ? cross_reference : reference
      end

Let's add a milestone here, because I believe we should fix that in 10.0 as this is a regression.

changed milestone to %10.0

@ossiv Can you share what referables do you have mentioned in the issue that causes problems when moving? Do you have a group milestone there, referenced by a ID?

I managed to reproduce it by adding a following test:

diff --git a/spec/lib/gitlab/gfm/reference_rewriter_spec.rb b/spec/lib/gitlab/gfm/reference_rewriter_spec.rb
index a3d323f..5be186a 100644
--- a/spec/lib/gitlab/gfm/reference_rewriter_spec.rb
+++ b/spec/lib/gitlab/gfm/reference_rewriter_spec.rb
@@ -2,8 +2,9 @@ require 'spec_helper'
 
 describe Gitlab::Gfm::ReferenceRewriter do
   let(:text) { 'some text' }
-  let(:old_project) { create(:project, name: 'old-project') }
-  let(:new_project) { create(:project, name: 'new-project') }
+  let(:group) { create(:group) }
+  let(:old_project) { create(:project, name: 'old-project', group: group) }
+  let(:new_project) { create(:project, name: 'new-project', group: group) }
   let(:user) { create(:user) }
 
   before do
@@ -106,6 +107,13 @@ describe Gitlab::Gfm::ReferenceRewriter do
 
         it { is_expected.to eq text }
       end
+
+      context 'when referring to group milestone' do
+        let!(:milestone) { create(:milestone, group: group) }
+        let(:text) { "some " + milestone.to_reference + " text" }
+
+        it { is_expected.to eq '#1' }
+      end
     end
   end
 end

I need to learn why do we allow nil references for group milestones:

  def to_reference(from_project = nil, format: :iid, full: false)
    return if group_milestone? && format != :name

    format_reference = milestone_format_reference(format)
    reference = "#{self.class.reference_prefix}#{format_reference}"

    if project
      "#{project.to_reference(from_project, full: full)}#{reference}"
    else
      reference
    end
  end

I see that @smcgivern changed this line in 149528f4 which was originally introduced by @felipe_artur in https://gitlab.com/gitlab-org/gitlab-ce/commit/b5f596c3ffb655b6e4fee127fa9336c829198b5b.

I would like to fix that, but I'm not sure what are the business rules behind these changes.

What happens if someone moves an issue to a different project within the same group? How should we rewrite a group milestone reference? Currently it raises an error, because default milestone format is :iid, but you can't refer to a group milestone by it. We probably should not rewrite the reference at all.
What happens if someone moves an issue with a group milestone reference to a project inside a different group? Should we rewrite the reference? I guess that we should not.

assigned to @grzesiek

mentioned in merge request !14294 (merged)

I just opened a merge request, which for now only contains a test that makes this problem reproducible. Waiting for Sean's and Felipe's answers, to decide on how to fix this problem.

Thank you @grzesiek for digging into this problem.

What happens if someone moves an issue to a different project within the same group? How should we rewrite a group milestone reference? Currently it raises an error, because default milestone format is :iid, but you can't refer to a group milestone by it. We probably should not rewrite the reference at all.

If you move an issue from one project to another project within the same group, and the original issue had a group milestone, than the new issue should have the same group milestone.

What happens if someone moves an issue with a group milestone reference to a project inside a different group? Should we rewrite the reference? I guess that we should not.

In this case, look at the new project's group. If that group has a group milestone with the same name as the previous one, then use that. Otherwise, don't have a milestone.

There's more specific cases. All of those cases are listed here (end of the description): https://gitlab.com/gitlab-org/gitlab-ce/issues/30126

What is still not clear for me, from mostly the technical point of view, is why do we forbid to reference a group milestone by an internal ID number, do you maybe know @victorwu?

@grzesiek : That level of technical detail I don't know what it was chosen that way. Sean is out currently. Let's wait for @felipe_artur to respond. Thanks!

@victorwu Okay, thanks!

@grzesiek Thanks for digging into this!!!

I first added that line because in the first iteration we did not support references for group milestones.

I think @smcgivern decided to let the reference be made by name because Group and Projects milestones names are unique in group scope.

If we need to use iid there is no problem to change, but group and project milestones are in different tables so we will have duplicated iids which is not ideal. I suggest keep using names.

@felipe_artur We can use a name, but it has to be a default. Currently default format is format: :iid. Can we safely change it to :name?

Asked @eReGeBe on Slack to check if we can make referring to a milestone by name a default behavior, since Alejandro seems to be the author of milestone references https://gitlab.com/gitlab-org/gitlab-ce/commit/077f9a4eeef3c64c5f3e9cc5df5442c8817ee1d6.

@grzesiek Sorry for the late response!
I am not 100% sure we have to investigate. Lets see what @eReGeBe says.

@eReGeBe Said on Slack that it is fine to use name format as a default, and this should resolve all concerns here, and using it should fix the issue!

added Deliverable workflowin dev labels

added Stretch and removed Deliverable labels

Confirmed that this is a global milestone problem, encountered this while moving https://gitlab.com/gitlab-org/gitlab-ce/issues/37533, https://sentry.gitlap.com/gitlab/gitlabcom/issues/47305/. There is one comment with a global milestone there.

added workflowin review and removed workflowin dev labels

closed via merge request !14294 (merged)

mentioned in commit e7237a5c

mentioned in commit 96bb281c

Thanks @grzesiek for the quick fix.

added devopsplan label

Moving an issue to another project sometimes fails

Designs

Child items ...

Activity