Epics::BaseService#initialize first argument is a group

- Follow-on to previous commit, to keep the changes related
  to Epics::BaseService hierarchy constructor in a cohesive
  commit, to make it easier to identify the scope of changes
  for future potential refactors.
- Add metaprogramming conditional logic to account for the
  different potential first argument names.
- Add TODO notes indicating that checking types or method signatures
  in logic is an antipattern, but the root cause is probably
  that inheritance may not be appropriate here.

app/services/issuable/bulk_update_service.rb

@@ -57,7 +57,7 @@ module Issuable
       items.each do |issuable|
         next unless can?(current_user, :"update_#{type}", issuable)
 
-        update_class.new(container: issuable.issuing_parent, current_user: current_user, params: params).execute(issuable)
+        update_class.new(**update_class.constructor_container_arg(issuable.issuing_parent), current_user: current_user, params: params).execute(issuable)
       end
 
       items

app/services/issuable_base_service.rb

@@ -353,9 +353,13 @@ class IssuableBaseService < ::BaseProjectService
   def change_state(issuable)
     case params.delete(:state_event)
     when 'reopen'
-      reopen_service.new(container: project, current_user: current_user).execute(issuable)
+      service_class = reopen_service
     when 'close'
-      close_service.new(container: project, current_user: current_user).execute(issuable)
+      service_class = close_service
+    end
+
+    if service_class
+      service_class.new(**service_class.constructor_container_arg(project), current_user: current_user).execute(issuable)
     end
   end
 

app/services/merge_requests/create_from_issue_service.rb

@@ -2,6 +2,18 @@
 
 module MergeRequests
   class CreateFromIssueService < MergeRequests::CreateService
+    # TODO: This constructor does not use the "params:" argument from the superclass,
+    #   but instead has a custom "mr_params:" argument. This is because historically,
+    #   prior to named arguments being introduced to the constructor, it never passed
+    #   along the third positional argument when calling `super`.
+    #   This should be changed, in order to be consistent (all subclasses should pass
+    #   along all of the arguments to the superclass, otherwise it is probably not an
+    #   "is a" relationship). However, we need to be sure that passing the params
+    #   argument to `super` (especially target_project_id) will not cause any unexpected
+    #   behavior in the superclass. Since the addition of the named arguments is
+    #   intended to be a low-risk pure refactor, we will defer this fix
+    #   to this follow-on issue:
+    #   https://gitlab.com/gitlab-org/gitlab/-/issues/328726
     def initialize(project:, current_user:, mr_params: {})
       # branch - the name of new branch
       # ref    - the source of new branch.
@@ -11,7 +23,6 @@ module MergeRequests
       @ref               = mr_params[:ref]
       @target_project_id = mr_params[:target_project_id]
 
-      # NOTE: We intentionally do not pass along the params
       super(project: project, current_user: current_user)
     end
 

app/services/notes/quick_actions_service.rb

@@ -24,12 +24,12 @@ module Notes
       UPDATE_SERVICES
     end
 
-    def self.noteable_update_service(note)
+    def self.noteable_update_service_class(note)
       update_services[note.noteable_type]
     end
 
     def self.supported?(note)
-      !!noteable_update_service(note)
+      !!noteable_update_service_class(note)
     end
 
     def supported?(note)
@@ -55,7 +55,21 @@ module Notes
         update_params[:spend_time][:note_id] = note.id
       end
 
-      self.class.noteable_update_service(note).new(container: note.resource_parent, current_user: current_user, params: update_params).execute(note.noteable)
+      noteable_update_service_class = self.class.noteable_update_service_class(note)
+
+      # TODO: This conditional is necessary because we have not fully converted all possible
+      #   noteable_update_service_class classes to use named arguments. See more details
+      #   on the partial conversion at https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59182
+      #   Follow-on issue to address this is here:
+      #   https://gitlab.com/gitlab-org/gitlab/-/issues/328734
+      service =
+        if noteable_update_service_class.respond_to?(:constructor_container_arg)
+          noteable_update_service_class.new(**noteable_update_service_class.constructor_container_arg(note.resource_parent), current_user: current_user, params: update_params)
+        else
+          noteable_update_service_class.new(note.resource_parent, current_user, update_params)
+        end
+
+      service.execute(note.noteable)
     end
   end
 end

ee/app/controllers/groups/epics_controller.rb

@@ -40,7 +40,7 @@ class Groups::EpicsController < Groups::ApplicationController
   end
 
   def create
-    @epic = ::Epics::CreateService.new(container: @group, current_user: current_user, params: epic_params).execute
+    @epic = ::Epics::CreateService.new(group: @group, current_user: current_user, params: epic_params).execute
 
     if @epic.persisted?
       render json: {
@@ -102,7 +102,7 @@ class Groups::EpicsController < Groups::ApplicationController
   end
 
   def update_service
-    ::Epics::UpdateService.new(container: @group, current_user: current_user, params: epic_params.to_h)
+    ::Epics::UpdateService.new(group: @group, current_user: current_user, params: epic_params.to_h)
   end
 
   def finder_type

ee/app/graphql/mutations/epics/create.rb

@@ -21,7 +21,7 @@ module Mutations
         validate_arguments!(args)
 
         group = authorized_find!(group_path: group_path)
-        epic = ::Epics::CreateService.new(container: group, current_user: current_user, params: args).execute
+        epic = ::Epics::CreateService.new(group: group, current_user: current_user, params: args).execute
 
         response_object = epic if epic.valid?
 

ee/app/graphql/mutations/epics/update.rb

@@ -21,7 +21,7 @@ module Mutations
         validate_arguments!(args)
 
         epic = authorized_find!(group_path: group_path, iid: epic_iid)
-        epic = ::Epics::UpdateService.new(container: epic.group, current_user: current_user, params: args).execute(epic)
+        epic = ::Epics::UpdateService.new(group: epic.group, current_user: current_user, params: args).execute(epic)
 
         {
           epic: epic.reset,

ee/app/services/boards/epics/move_service.rb

@@ -8,7 +8,7 @@ module Boards
       private
 
       def update(epic, epic_modification_params)
-        ::Epics::UpdateService.new(container: epic.group, current_user: current_user, params: epic_modification_params).execute(epic)
+        ::Epics::UpdateService.new(group: epic.group, current_user: current_user, params: epic_modification_params).execute(epic)
       end
 
       def board

ee/app/services/epics/base_service.rb

@@ -4,10 +4,51 @@ module Epics
   class BaseService < IssuableBaseService
     extend ::Gitlab::Utils::Override
 
+    def self.constructor_container_arg(value)
+      # TODO: Dynamically determining the type of a constructor arg based on the class is an antipattern,
+      # but the root cause is that Epics::BaseService has some issues that inheritance may not be the
+      # appropriate pattern. See more details in comments at the top of Epics::BaseService#initialize.
+      # Follow on issue to address this:
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/328438
+
+      { group: value }
+    end
+
     attr_reader :group, :parent_epic, :child_epic
 
-    def initialize(container:, current_user:, params: {})
-      @group = container
+    # TODO: This constructor does NOT call `super`, because it has
+    # no `project` associated. Thus, the first argument is named
+    # `group`, even though it only a `group` in this sub-hierarchy of `IssuableBaseClass`,
+    # but is a `project` everywhere else.  This is because named arguments
+    # were added after the class was already in use. We use `.constructor_container_arg`
+    # to determine the correct keyword to use.
+    #
+    # This is revealing an inconsistency which already existed,
+    # where sometimes a `project` is passed as the first argument but ignored.  For example,
+    # in `IssuableBaseService#change_state` method, as well as many others.
+    #
+    # This is a form of violation of the Liskov Substitution Principle
+    # (https://en.wikipedia.org/wiki/Liskov_substitution_principle),
+    # in that we cannot determine which form of the constructor to call without
+    # knowing what the type of subclass is.
+    #
+    # This implies that inheritance may not be the proper relationship to "issuable",
+    # because it may not be an "is a" relationship.
+    #
+    # All other `IssuableBaseService` subclasses are in the context of a
+    # project, and take the project as the first argument to the constructor.
+    #
+    # Instead, is seems like there is are some concerns such as state management, and
+    # having notes, which are applicable to "epic" services, but not necessarily all aspects
+    # of "issuable" services.
+    #
+    # See the following links for more context:
+    # - Original discussion thread: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59182#note_555401711
+    # - Issue to address inheritance problems: https://gitlab.com/gitlab-org/gitlab/-/issues/328438
+    def initialize(group:, current_user:, params: {})
+      # NOTE: this does NOT call `super`! See details in comment above.
+
+      @group = group
       @current_user = current_user
       @params = params
     end

ee/app/services/epics/issue_promote_service.rb

@@ -39,7 +39,7 @@ module Epics
     end
 
     def create_new_entity
-      @new_entity = Epics::CreateService.new(container: parent_group, current_user: current_user, params: params).execute
+      @new_entity = Epics::CreateService.new(group: parent_group, current_user: current_user, params: params).execute
     end
 
     def update_old_entity

ee/app/services/epics/transfer_service.rb

@@ -46,7 +46,7 @@ module Epics
       epic_params = epic.attributes
                         .slice('title', 'description', 'start_date', 'end_date', 'confidential')
 
-      CreateService.new(container: project.group, current_user: current_user, params: epic_params).execute
+      CreateService.new(group: project.group, current_user: current_user, params: epic_params).execute
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

ee/lib/api/epic_links.rb

@@ -85,7 +85,7 @@ module API
         confidential = params[:confidential].nil? ? epic.confidential : params[:confidential]
         create_params = { parent_id: epic.id, title: params[:title], confidential: confidential }
 
-        child_epic = ::Epics::CreateService.new(container: user_group, current_user: current_user, params: create_params).execute
+        child_epic = ::Epics::CreateService.new(group: user_group, current_user: current_user, params: create_params).execute
 
         if child_epic.valid?
           present child_epic, with: EE::API::Entities::LinkedEpic, user: current_user
@@ -101,7 +101,7 @@ module API
       delete ':id/(-/)epics/:epic_iid/epics/:child_epic_id' do
         authorize_can_destroy_epic_link!
 
-        updated_epic = ::Epics::UpdateService.new(container: user_group, current_user: current_user, params: { parent: nil }).execute(child_epic)
+        updated_epic = ::Epics::UpdateService.new(group: user_group, current_user: current_user, params: { parent: nil }).execute(child_epic)
 
         present updated_epic, with: EE::API::Entities::Epic
       end

ee/lib/api/epics.rb

@@ -87,7 +87,7 @@ module API
         # Setting created_at is allowed only for admins and owners
         params.delete(:created_at) unless current_user.can?(:set_epic_created_at, user_group)
 
-        epic = ::Epics::CreateService.new(container: user_group, current_user: current_user, params: declared_params(include_missing: false)).execute
+        epic = ::Epics::CreateService.new(group: user_group, current_user: current_user, params: declared_params(include_missing: false)).execute
         if epic.valid?
           present epic, epic_options
         else
@@ -125,7 +125,7 @@ module API
         update_params = declared_params(include_missing: false)
         update_params.delete(:epic_iid)
 
-        result = ::Epics::UpdateService.new(container: user_group, current_user: current_user, params: update_params).execute(epic)
+        result = ::Epics::UpdateService.new(group: user_group, current_user: current_user, params: update_params).execute(epic)
 
         if result.valid?
           present result, epic_options

ee/spec/services/epics/close_service_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Epics::CloseService do
   let_it_be(:epic, reload: true) { create(:epic, group: group) }
 
   describe '#execute' do
-    subject { described_class.new(container: group, current_user: user) }
+    subject { described_class.new(group: group, current_user: user) }
 
     context 'when epics are disabled' do
       before do

ee/spec/services/epics/create_service_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Epics::CreateService do
 
   let(:params) { { title: 'new epic', description: 'epic description', parent_id: parent_epic.id, confidential: true } }
 
-  subject { described_class.new(container: group, current_user: user, params: params).execute }
+  subject { described_class.new(group: group, current_user: user, params: params).execute }
 
   describe '#execute' do
     it 'creates one epic correctly' do
@@ -96,7 +96,7 @@ RSpec.describe Epics::CreateService do
             description = "/parent_epic #{parent_epic.to_reference}"
             params = { title: 'New epic with parent', description: description }
 
-            epic = described_class.new(container: group, current_user: user, params: params).execute
+            epic = described_class.new(group: group, current_user: user, params: params).execute
 
             expect(epic.parent).to eq(parent_epic)
           end
@@ -108,7 +108,7 @@ RSpec.describe Epics::CreateService do
               description = "/parent_epic #{parent_epic.to_reference(group)}"
               params = { title: 'New epic with parent', description: description }
 
-              epic = described_class.new(container: group, current_user: user, params: params).execute
+              epic = described_class.new(group: group, current_user: user, params: params).execute
 
               expect(epic.parent).to eq(nil)
             end
@@ -121,7 +121,7 @@ RSpec.describe Epics::CreateService do
             description = "/child_epic #{child_epic.to_reference}"
             params = { title: 'New epic with child', description: description }
 
-            epic = described_class.new(container: group, current_user: user, params: params).execute
+            epic = described_class.new(group: group, current_user: user, params: params).execute
 
             expect(epic.reload.children).to include(child_epic)
           end
@@ -133,7 +133,7 @@ RSpec.describe Epics::CreateService do
               description = "/child_epic #{child_epic.to_reference(group)}"
               params = { title: 'New epic with child', description: description }
 
-              epic = described_class.new(container: group, current_user: user, params: params).execute
+              epic = described_class.new(group: group, current_user: user, params: params).execute
 
               expect(epic.reload.children).to be_empty
             end

ee/spec/services/epics/reopen_service_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Epics::ReopenService do
   let_it_be(:epic, reload: true) { create(:epic, group: group, state: :closed, closed_at: Date.today, closed_by: user) }
 
   describe '#execute' do
-    subject { described_class.new(container: group, current_user: user) }
+    subject { described_class.new(group: group, current_user: user) }
 
     context 'when epics are disabled' do
       before do

ee/spec/services/epics/update_service_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe Epics::UpdateService do
     end
 
     def update_epic(opts)
-      described_class.new(container: group, current_user: user, params: opts).execute(epic)
+      described_class.new(group: group, current_user: user, params: opts).execute(epic)
     end
 
     context 'multiple values update' do
@@ -324,7 +324,7 @@ RSpec.describe Epics::UpdateService do
 
       it_behaves_like 'updating a single task' do
         def update_issuable(opts)
-          described_class.new(container: group, current_user: user, params: opts).execute(epic)
+          described_class.new(group: group, current_user: user, params: opts).execute(epic)
         end
       end
 

ee/spec/support/shared_examples/services/scoped_label_shared_examples.rb

@@ -29,7 +29,7 @@ RSpec.shared_examples 'new issuable with scoped labels' do
       context 'when using label_ids parameter' do
         it 'adds only last selected exclusive scoped label' do
           issuable = described_class.new(
-            container: parent, current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] }
+            **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] }
           ).execute
 
           expect(issuable.labels).to match_array([label1, label2])
@@ -39,7 +39,7 @@ RSpec.shared_examples 'new issuable with scoped labels' do
       context 'when using labels parameter' do
         it 'adds only last selected exclusive scoped label' do
           issuable = described_class.new(
-            container: parent, current_user: user, params: { title: 'test', labels: [label1.title, label3.title, label4.title, label2.title] }
+            **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', labels: [label1.title, label3.title, label4.title, label2.title] }
           ).execute
 
           expect(issuable.labels).to match_array([label1, label2])
@@ -59,7 +59,7 @@ RSpec.shared_examples 'new issuable with scoped labels' do
         label4 = create_label('key::label3')
 
         issuable = described_class.new(
-          container: parent, current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] }
+          **described_class.constructor_container_arg(parent), current_user: user, params: { title: 'test', label_ids: [label1.id, label3.id, label4.id, label2.id] }
         ).execute
 
         expect(issuable.labels).to match_array([label1, label2, label3, label4])
@@ -87,7 +87,7 @@ RSpec.shared_examples 'existing issuable with scoped labels' do
           issuable.reload
 
           described_class.new(
-            container: parent, current_user: user, params: { label_ids: [label1.id, label3.id] }
+            **described_class.constructor_container_arg(parent), current_user: user, params: { label_ids: [label1.id, label3.id] }
           ).execute(issuable)
 
           expect(issuable.reload.labels).to match_array([label3])
@@ -102,7 +102,7 @@ RSpec.shared_examples 'existing issuable with scoped labels' do
           issuable.reload
 
           described_class.new(
-            container: parent, current_user: user, params: { labels: [label1.title, label3.title] }
+            **described_class.constructor_container_arg(parent), current_user: user, params: { labels: [label1.title, label3.title] }
           ).execute(issuable)
 
           expect(issuable.reload.labels).to match_array([label3])
@@ -118,7 +118,7 @@ RSpec.shared_examples 'existing issuable with scoped labels' do
           issuable.reload
 
           described_class.new(
-            container: parent, current_user: user, params: { label_ids: [label2.id, label3.id] }
+            **described_class.constructor_container_arg(parent), current_user: user, params: { label_ids: [label2.id, label3.id] }
           ).execute(issuable)
 
           expect(issuable.reload.labels).to match_array([label2, label3])
@@ -138,7 +138,7 @@ RSpec.shared_examples 'existing issuable with scoped labels' do
         issuable.reload
 
         described_class.new(
-          container: parent, current_user: user, params: { label_ids: [label1.id, label2.id, label3.id] }
+          **described_class.constructor_container_arg(parent), current_user: user, params: { label_ids: [label1.id, label2.id, label3.id] }
         ).execute(issuable)
 
         expect(issuable.reload.labels).to match_array([label1, label2, label3])

spec/services/notes/quick_actions_service_spec.rb

@@ -238,25 +238,25 @@ RSpec.describe Notes::QuickActionsService do
     end
   end
 
-  describe '.noteable_update_service' do
+  describe '.noteable_update_service_class' do
     include_context 'note on noteable'
 
     it 'returns Issues::UpdateService for a note on an issue' do
       note = create(:note_on_issue, project: project)
 
-      expect(described_class.noteable_update_service(note)).to eq(Issues::UpdateService)
+      expect(described_class.noteable_update_service_class(note)).to eq(Issues::UpdateService)
     end
 
     it 'returns MergeRequests::UpdateService for a note on a merge request' do
       note = create(:note_on_merge_request, project: project)
 
-      expect(described_class.noteable_update_service(note)).to eq(MergeRequests::UpdateService)
+      expect(described_class.noteable_update_service_class(note)).to eq(MergeRequests::UpdateService)
     end
 
     it 'returns Commits::TagService for a note on a commit' do
       note = create(:note_on_commit, project: project)
 
-      expect(described_class.noteable_update_service(note)).to eq(Commits::TagService)
+      expect(described_class.noteable_update_service_class(note)).to eq(Commits::TagService)
     end
   end