From f3052c0de6797e02c2d13d550caf9ac9e1761e9c Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Sun, 17 Oct 2021 14:16:25 +0100 Subject: [PATCH 1/2] GithubImporter: Refactoring representation layer This is the first part of a bigger plan to improve the GithubImporter Representation layer. The GitHub importer uses a variation of the ETL architecture, where: - Extractions happens on Importers (with pluralized resource names); - Transformations happens in Representation layer; - Loading happens in Importers (with singular resource names); Currently some transformations are not happening exclusively in the Representation objects, instead, some transformations are leaking to the Loading layer. This is happening because some transformations depends on the context of the import, like the project being imported. This first step adds more context, project being imported and GitHub client being used, to the Representation classes. The next step is move the transformations that are happening out side the Representations layer back in to these classes. The end goal is to move representations (transformations) out of the Import (load) layer. Related to: #330331 MR: !72429 Changelog: changed --- .../gitlab/github_import/object_importer.rb | 27 ++----- lib/gitlab/github_import/context.rb | 21 ++++++ .../github_import/parallel_scheduling.rb | 14 +++- lib/gitlab/github_import/representation.rb | 25 ------- .../github_import/representation/base.rb | 74 +++++++++++++++++++ .../github_import/representation/diff_note.rb | 30 ++------ .../github_import/representation/issue.rb | 41 +++------- .../representation/lfs_object.rb | 22 +----- .../github_import/representation/note.rb | 31 ++------ .../representation/pull_request.rb | 44 ++++------- .../representation/pull_request_review.rb | 29 ++------ .../github_import/representation/user.rb | 27 ++----- 12 files changed, 167 insertions(+), 218 deletions(-) create mode 100644 lib/gitlab/github_import/context.rb delete mode 100644 lib/gitlab/github_import/representation.rb create mode 100644 lib/gitlab/github_import/representation/base.rb diff --git a/app/workers/concerns/gitlab/github_import/object_importer.rb b/app/workers/concerns/gitlab/github_import/object_importer.rb index e1f404b250d500..f2ce98872efe0b 100644 --- a/app/workers/concerns/gitlab/github_import/object_importer.rb +++ b/app/workers/concerns/gitlab/github_import/object_importer.rb @@ -23,29 +23,18 @@ module ObjectImporter # client - An instance of `Gitlab::GithubImport::Client` # hash - A Hash containing the details of the object to import. def import(project, client, hash) - object = representation_class.from_json_hash(hash) + context = Context.new(project: project, client: client) + + self.representation = representation_class.new(context) + representation.deserialize(hash) - # To better express in the logs what object is being imported. - self.github_identifiers = object.github_identifiers info(project.id, message: 'starting importer') - importer_class.new(object, project, client).execute + importer_class.new(representation, project, client).execute Gitlab::GithubImport::ObjectCounter.increment(project, object_type, :imported) info(project.id, message: 'importer finished') - rescue NoMethodError => e - # This exception will be more useful in development when a new - # Representation is created but the developer forgot to add a - # `:github_identifiers` field. - Gitlab::Import::ImportFailureService.track( - project_id: project.id, - error_source: importer_class.name, - exception: e, - fail_import: true - ) - - raise(e) rescue StandardError => e Gitlab::Import::ImportFailureService.track( project_id: project.id, @@ -59,7 +48,7 @@ def object_type end # Returns the representation class to use for the object. This class must - # define the class method `from_json_hash`. + # define the class method `deserialize`. def representation_class raise NotImplementedError end @@ -71,7 +60,7 @@ def importer_class private - attr_accessor :github_identifiers + attr_accessor :representation def info(project_id, extra = {}) Logger.info(log_attributes(project_id, extra)) @@ -81,7 +70,7 @@ def log_attributes(project_id, extra = {}) extra.merge( project_id: project_id, importer: importer_class.name, - github_identifiers: github_identifiers + github_identifiers: representation.github_identifiers ) end end diff --git a/lib/gitlab/github_import/context.rb b/lib/gitlab/github_import/context.rb new file mode 100644 index 00000000000000..b6958bf0d2d2b1 --- /dev/null +++ b/lib/gitlab/github_import/context.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module GithubImport + class Context + def initialize(project:, client:) + @attributes = ActiveSupport::HashWithIndifferentAccess.new + @attributes[:project] = project + @attributes[:client] = client + end + + def [](key) + @attributes[key] + end + + def []=(key, value) + @attributes[key] = value + end + end + end +end diff --git a/lib/gitlab/github_import/parallel_scheduling.rb b/lib/gitlab/github_import/parallel_scheduling.rb index a8e006ea082974..014cdf048cca62 100644 --- a/lib/gitlab/github_import/parallel_scheduling.rb +++ b/lib/gitlab/github_import/parallel_scheduling.rb @@ -63,9 +63,9 @@ def execute # Imports all the objects in sequence in the current thread. def sequential_import each_object_to_import do |object| - repr = representation_class.from_api_response(object) + representation.parse(object) - importer_class.new(repr, project, client).execute + importer_class.new(representation, project, client).execute end end @@ -75,10 +75,10 @@ def parallel_import waiter = JobWaiter.new each_object_to_import do |object| - repr = representation_class.from_api_response(object) + representation.parse(object) sidekiq_worker_class - .perform_async(project.id, repr.to_hash, waiter.key) + .perform_async(project.id, representation.to_hash, waiter.key) waiter.jobs_remaining += 1 end @@ -155,6 +155,12 @@ def representation_class raise NotImplementedError end + def representation + @representation ||= representation_class.new( + Context.new(project: project, client: client) + ) + end + # The class to use for importing objects when importing them sequentially. def importer_class raise NotImplementedError diff --git a/lib/gitlab/github_import/representation.rb b/lib/gitlab/github_import/representation.rb deleted file mode 100644 index 639477ef2a2b60..00000000000000 --- a/lib/gitlab/github_import/representation.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module GithubImport - module Representation - TIMESTAMP_KEYS = %i[created_at updated_at merged_at].freeze - - # Converts a Hash with String based keys to one that can be used by the - # various Representation classes. - # - # Example: - # - # Representation.symbolize_hash('number' => 10) # => { number: 10 } - def self.symbolize_hash(raw_hash = nil) - hash = raw_hash.deep_symbolize_keys - - TIMESTAMP_KEYS.each do |key| - hash[key] = Time.parse(hash[key]) if hash[key].is_a?(String) - end - - hash - end - end - end -end diff --git a/lib/gitlab/github_import/representation/base.rb b/lib/gitlab/github_import/representation/base.rb new file mode 100644 index 00000000000000..ddd31fcd0b5adb --- /dev/null +++ b/lib/gitlab/github_import/representation/base.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +module Gitlab + module GithubImport + module Representation + # Representation Objects have two main functions: + # + # - serialize/deserialize Github responses to/from import jobs; + # - transform Github data to expect Gitlab format; + class Base + include ToHash + include ExposeAttribute + + TIMESTAMP_KEYS = %i[created_at updated_at merged_at submitted_at].freeze + + # attributes - A Hash containing the user details. The keys of this + # Hash (and any nested hashes) must be symbols. + def initialize(context) + @context = context + end + + # Parse the original github response + # + # response - Sawyer::Resource + def parse(response) + assign_attributes(response.to_hash) + end + + # Load a hash of attributes, created from Representation#to_hash + # to the current Representation object + def deserialize(hash) + assign_attributes(hash) + end + + # Set of relevant github identifier used to log the current object + def github_identifiers + raise NotImplementedError + end + + private + + attr_reader :context, :attributes + + def assign_attributes(attributes) + @attributes = symbolize_hash(attributes) + end + + def parse_with(klass, object) + klass.new(context).tap { |representer| representer.parse(object) } + end + + def deserialize_with(klass, hash) + klass.new(context).tap { |representer| representer.deserialize(hash) } + end + + # Converts a Hash with String based keys to one that can be used by the + # various Representation classes. + # + # Example: + # + # Representation.symbolize_hash('number' => 10) # => { number: 10 } + def symbolize_hash(raw_hash = nil) + hash = raw_hash.deep_symbolize_keys + + TIMESTAMP_KEYS.each do |key| + hash[key] = Time.parse(hash[key]).in_time_zone if hash[key].is_a?(String) + end + + hash + end + end + end + end +end diff --git a/lib/gitlab/github_import/representation/diff_note.rb b/lib/gitlab/github_import/representation/diff_note.rb index a3dcd2e380c6ce..ed9f3c455378f0 100644 --- a/lib/gitlab/github_import/representation/diff_note.rb +++ b/lib/gitlab/github_import/representation/diff_note.rb @@ -3,22 +3,14 @@ module Gitlab module GithubImport module Representation - class DiffNote - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class DiffNote < Base expose_attribute :noteable_type, :noteable_id, :commit_id, :file_path, :diff_hunk, :author, :note, :created_at, :updated_at, :original_commit_id, :note_id NOTEABLE_ID_REGEX = %r{/pull/(?\d+)}i.freeze - # Builds a diff note from a GitHub API response. - # - # note - An instance of `Sawyer::Resource` containing the note details. - def self.from_api_response(note) + def parse(note) matches = note.html_url.match(NOTEABLE_ID_REGEX) unless matches @@ -28,7 +20,7 @@ def self.from_api_response(note) ) end - user = Representation::User.from_api_response(note.user) if note.user + user = parse_with(Representation::User, note.user) if note.user hash = { noteable_type: 'MergeRequest', noteable_id: matches[:iid].to_i, @@ -45,21 +37,13 @@ def self.from_api_response(note) start_line: note.start_line } - new(hash) + super(hash) end - # Builds a new note using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - hash = Representation.symbolize_hash(raw_hash) - hash[:author] &&= Representation::User.from_json_hash(hash[:author]) - - new(hash) - end + def deserialize(hash) + super(hash) - # attributes - A Hash containing the raw note details. The keys of this - # Hash must be Symbols. - def initialize(attributes) - @attributes = attributes + attributes[:author] &&= deserialize_with(Representation::User, attributes[:author]) end def line_code diff --git a/lib/gitlab/github_import/representation/issue.rb b/lib/gitlab/github_import/representation/issue.rb index db4a8188c0333d..e5b351007a6ceb 100644 --- a/lib/gitlab/github_import/representation/issue.rb +++ b/lib/gitlab/github_import/representation/issue.rb @@ -3,25 +3,13 @@ module Gitlab module GithubImport module Representation - class Issue - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class Issue < Base expose_attribute :iid, :title, :description, :milestone_number, :created_at, :updated_at, :state, :assignees, :label_names, :author - # Builds an issue from a GitHub API response. - # - # issue - An instance of `Sawyer::Resource` containing the issue - # details. - def self.from_api_response(issue) - user = - if issue.user - Representation::User.from_api_response(issue.user) - end + def parse(issue) + user = parse_with(Representation::User, issue.user) if issue.user hash = { iid: issue.number, @@ -30,7 +18,7 @@ def self.from_api_response(issue) milestone_number: issue.milestone&.number, state: issue.state == 'open' ? :opened : :closed, assignees: issue.assignees.map do |u| - Representation::User.from_api_response(u) + parse_with(Representation::User, u) end, label_names: issue.labels.map(&:name), author: user, @@ -39,24 +27,15 @@ def self.from_api_response(issue) pull_request: issue.pull_request ? true : false } - new(hash) + super(hash) end - # Builds a new issue using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - hash = Representation.symbolize_hash(raw_hash) - - hash[:state] = hash[:state].to_sym - hash[:assignees].map! { |u| Representation::User.from_json_hash(u) } - hash[:author] &&= Representation::User.from_json_hash(hash[:author]) - - new(hash) - end + def deserialize(hash) + super(hash) - # attributes - A hash containing the raw issue details. The keys of this - # Hash (and any nested hashes) must be symbols. - def initialize(attributes) - @attributes = attributes + attributes[:state] = attributes[:state].to_sym + attributes[:assignees].map! { |u| deserialize_with(Representation::User, u) } + attributes[:author] &&= deserialize_with(Representation::User, attributes[:author]) end def truncated_title diff --git a/lib/gitlab/github_import/representation/lfs_object.rb b/lib/gitlab/github_import/representation/lfs_object.rb index 18737bfcde3852..aeccf4f84b18ef 100644 --- a/lib/gitlab/github_import/representation/lfs_object.rb +++ b/lib/gitlab/github_import/representation/lfs_object.rb @@ -3,34 +3,18 @@ module Gitlab module GithubImport module Representation - class LfsObject - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class LfsObject < Base expose_attribute :oid, :link, :size # Builds a lfs_object - def self.from_api_response(lfs_object) - new( + def self.parse(lfs_object) + super( oid: lfs_object.oid, link: lfs_object.link, size: lfs_object.size ) end - # Builds a new lfs_object using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - new(Representation.symbolize_hash(raw_hash)) - end - - # attributes - A Hash containing the raw lfs_object details. The keys of this - # Hash must be Symbols. - def initialize(attributes) - @attributes = attributes - end - def github_identifiers { oid: oid diff --git a/lib/gitlab/github_import/representation/note.rb b/lib/gitlab/github_import/representation/note.rb index bcdb1a5459b463..cadb929c07717c 100644 --- a/lib/gitlab/github_import/representation/note.rb +++ b/lib/gitlab/github_import/representation/note.rb @@ -3,21 +3,13 @@ module Gitlab module GithubImport module Representation - class Note - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class Note < Base expose_attribute :noteable_id, :noteable_type, :author, :note, :created_at, :updated_at, :note_id NOTEABLE_TYPE_REGEX = %r{/(?(pull|issues))/(?\d+)}i.freeze - # Builds a note from a GitHub API response. - # - # note - An instance of `Sawyer::Resource` containing the note details. - def self.from_api_response(note) + def parse(note) matches = note.html_url.match(NOTEABLE_TYPE_REGEX) if !matches || !matches[:type] @@ -34,7 +26,7 @@ def self.from_api_response(note) 'Issue' end - user = Representation::User.from_api_response(note.user) if note.user + user = parse_with(Representation::User, note.user) if note.user hash = { noteable_type: noteable_type, noteable_id: matches[:iid].to_i, @@ -45,22 +37,13 @@ def self.from_api_response(note) note_id: note.id } - new(hash) + super(hash) end - # Builds a new note using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - hash = Representation.symbolize_hash(raw_hash) - - hash[:author] &&= Representation::User.from_json_hash(hash[:author]) - - new(hash) - end + def deserialize(hash) + super(hash) - # attributes - A Hash containing the raw note details. The keys of this - # Hash must be Symbols. - def initialize(attributes) - @attributes = attributes + attributes[:author] &&= deserialize_with(Representation::User, attributes[:author]) end alias_method :issuable_type, :noteable_type diff --git a/lib/gitlab/github_import/representation/pull_request.rb b/lib/gitlab/github_import/representation/pull_request.rb index 82bcdee8b2bffe..62265f5d73aac9 100644 --- a/lib/gitlab/github_import/representation/pull_request.rb +++ b/lib/gitlab/github_import/representation/pull_request.rb @@ -3,25 +3,17 @@ module Gitlab module GithubImport module Representation - class PullRequest - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class PullRequest < Base expose_attribute :iid, :title, :description, :source_branch, :source_branch_sha, :target_branch, :target_branch_sha, :milestone_number, :author, :assignee, :created_at, :updated_at, :merged_at, :source_repository_id, :target_repository_id, :source_repository_owner, :merged_by - # Builds a PR from a GitHub API response. - # - # issue - An instance of `Sawyer::Resource` containing the PR details. - def self.from_api_response(pr) - assignee = Representation::User.from_api_response(pr.assignee) if pr.assignee - user = Representation::User.from_api_response(pr.user) if pr.user - merged_by = Representation::User.from_api_response(pr.merged_by) if pr.merged_by + def parse(pr) + assignee = parse_with(Representation::User, pr.assignee) if pr.assignee + user = parse_with(Representation::User, pr.user) if pr.user + merged_by = parse_with(Representation::User, pr.merged_by) if pr.merged_by hash = { iid: pr.number, @@ -44,28 +36,18 @@ def self.from_api_response(pr) merged_by: merged_by } - new(hash) + super(hash) end - # Builds a new PR using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - hash = Representation.symbolize_hash(raw_hash) + def deserialize(hash) + super(hash) - hash[:state] = hash[:state].to_sym - hash[:author] &&= Representation::User.from_json_hash(hash[:author]) - - # Assignees are optional so we only convert it from a Hash if one was - # set. - hash[:assignee] &&= Representation::User.from_json_hash(hash[:assignee]) - hash[:merged_by] &&= Representation::User.from_json_hash(hash[:merged_by]) - - new(hash) - end + attributes[:state] = attributes[:state].to_sym + attributes[:author] &&= deserialize_with(Representation::User, attributes[:author]) - # attributes - A Hash containing the raw PR details. The keys of this - # Hash (and any nested hashes) must be symbols. - def initialize(attributes) - @attributes = attributes + # Assignees are optional so we only convert it from a attributes if one was set. + attributes[:assignee] &&= deserialize_with(Representation::User, attributes[:assignee]) + attributes[:merged_by] &&= deserialize_with(Representation::User, attributes[:merged_by]) end def truncated_title diff --git a/lib/gitlab/github_import/representation/pull_request_review.rb b/lib/gitlab/github_import/representation/pull_request_review.rb index 70c1e51ffddc5e..3c06a66142b790 100644 --- a/lib/gitlab/github_import/representation/pull_request_review.rb +++ b/lib/gitlab/github_import/representation/pull_request_review.rb @@ -3,18 +3,13 @@ module Gitlab module GithubImport module Representation - class PullRequestReview - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class PullRequestReview < Base expose_attribute :author, :note, :review_type, :submitted_at, :merge_request_id, :review_id - def self.from_api_response(review) - user = Representation::User.from_api_response(review.user) if review.user + def parse(review) + user = parse_with(Representation::User, review.user) if review.user - new( + super( merge_request_id: review.merge_request_id, author: user, note: review.body, @@ -24,20 +19,10 @@ def self.from_api_response(review) ) end - # Builds a new note using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - hash = Representation.symbolize_hash(raw_hash) - - hash[:author] &&= Representation::User.from_json_hash(hash[:author]) - hash[:submitted_at] = Time.parse(hash[:submitted_at]).in_time_zone if hash[:submitted_at].present? - - new(hash) - end + def deserialize(hash) + super(hash) - # attributes - A Hash containing the raw note details. The keys of this - # Hash must be Symbols. - def initialize(attributes) - @attributes = attributes + attributes[:author] &&= deserialize_with(Representation::User, attributes[:author]) end def approval? diff --git a/lib/gitlab/github_import/representation/user.rb b/lib/gitlab/github_import/representation/user.rb index fac8920a3f2429..43fa1deee6d8d2 100644 --- a/lib/gitlab/github_import/representation/user.rb +++ b/lib/gitlab/github_import/representation/user.rb @@ -3,33 +3,20 @@ module Gitlab module GithubImport module Representation - class User - include ToHash - include ExposeAttribute - - attr_reader :attributes - + class User < Base expose_attribute :id, :login - # Builds a user from a GitHub API response. - # - # user - An instance of `Sawyer::Resource` containing the user details. - def self.from_api_response(user) - new( + def parse(user) + super( id: user.id, login: user.login ) end - # Builds a user using a Hash that was built from a JSON payload. - def self.from_json_hash(raw_hash) - new(Representation.symbolize_hash(raw_hash)) - end - - # attributes - A Hash containing the user details. The keys of this - # Hash (and any nested hashes) must be symbols. - def initialize(attributes) - @attributes = attributes + def github_identifiers + { + id: id + } end end end -- GitLab From e9f08e4b3963bbdb9b0a0241b004892bfc3b0772 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Sun, 17 Oct 2021 20:31:03 +0100 Subject: [PATCH 2/2] WIP --- .../importer/pull_request_importer.rb | 60 ++++-------- .../github_import/representation/base.rb | 8 ++ .../representation/pull_request.rb | 93 ++++++++++++++----- 3 files changed, 96 insertions(+), 65 deletions(-) diff --git a/lib/gitlab/github_import/importer/pull_request_importer.rb b/lib/gitlab/github_import/importer/pull_request_importer.rb index 3c17ea1195e434..fd53a7714699cf 100644 --- a/lib/gitlab/github_import/importer/pull_request_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_importer.rb @@ -6,33 +6,28 @@ module Importer class PullRequestImporter include Gitlab::Import::MergeRequestHelpers - attr_reader :pull_request, :project, :client, :user_finder, - :milestone_finder, :issuable_finder - - # pull_request - An instance of - # `Gitlab::GithubImport::Representation::PullRequest`. - # project - An instance of `Project` - # client - An instance of `Gitlab::GithubImport::Client` + # pull_request - An instance of `Gitlab::GithubImport::Representation::PullRequest`. + # project - An instance of `Project` + # client - An instance of `Gitlab::GithubImport::Client` def initialize(pull_request, project, client) @pull_request = pull_request @project = project @client = client - @user_finder = GithubImport::UserFinder.new(project, client) - @milestone_finder = MilestoneFinder.new(project) - @issuable_finder = - GithubImport::IssuableFinder.new(project, pull_request) end def execute mr, already_exists = create_merge_request if mr - set_merge_request_assignees(mr) insert_git_data(mr, already_exists) issuable_finder.cache_database_id(mr.id) end end + private + + attr_reader :pull_request, :project + # Creates the merge request and returns its ID. # # This method will return `nil` if the merge request could not be @@ -42,30 +37,11 @@ def execute # 1. A MergeRequest instance. # 2. A boolean indicating if the MR already exists. def create_merge_request - author_id, author_found = user_finder.author_id_for(pull_request) - - description = MarkdownText.format(pull_request.description, pull_request.author, author_found) - - attributes = { - iid: pull_request.iid, - title: pull_request.truncated_title, - description: description, - source_project_id: project.id, - target_project_id: project.id, - source_branch: pull_request.formatted_source_branch, - target_branch: pull_request.target_branch, - state_id: ::MergeRequest.available_states[pull_request.state], - milestone_id: milestone_finder.id_for(pull_request), - author_id: author_id, - created_at: pull_request.created_at, - updated_at: pull_request.updated_at - } - - create_merge_request_without_hooks(project, attributes, pull_request.iid) - end - - def set_merge_request_assignees(merge_request) - merge_request.assignee_ids = [user_finder.assignee_id_for(pull_request)] + create_merge_request_without_hooks( + project, + pull_request.attributes_to_insert, + pull_request.iid + ) end def insert_git_data(merge_request, already_exists) @@ -88,17 +64,21 @@ def insert_git_data(merge_request, already_exists) def create_source_branch_if_not_exists(merge_request) return unless merge_request.open? - source_branch = pull_request.formatted_source_branch + pull_request.source_branch - return if project.repository.branch_exists?(source_branch) + return if project.repository.branch_exists?(pull_request.source_branch) - project.repository.add_branch(project.creator, source_branch, pull_request.source_branch_sha) + project.repository.add_branch(project.creator, pull_request.source_branch, pull_request.source_branch_sha) rescue Gitlab::Git::CommandError => e Gitlab::ErrorTracking.track_exception(e, - source_branch: source_branch, + source_branch: pull_request.source_branch, project_id: merge_request.project.id, merge_request_id: merge_request.id) end + + def issuable_finder + @issuable_finder ||= GithubImport::IssuableFinder.new(project, pull_request) + end end end end diff --git a/lib/gitlab/github_import/representation/base.rb b/lib/gitlab/github_import/representation/base.rb index ddd31fcd0b5adb..b0d472a7d14e05 100644 --- a/lib/gitlab/github_import/representation/base.rb +++ b/lib/gitlab/github_import/representation/base.rb @@ -68,6 +68,14 @@ def symbolize_hash(raw_hash = nil) hash end + + def project + @project ||= context[:project] + end + + def client + @client ||= context[:client] + end end end end diff --git a/lib/gitlab/github_import/representation/pull_request.rb b/lib/gitlab/github_import/representation/pull_request.rb index 62265f5d73aac9..657f881d1bc8fb 100644 --- a/lib/gitlab/github_import/representation/pull_request.rb +++ b/lib/gitlab/github_import/representation/pull_request.rb @@ -4,10 +4,9 @@ module Gitlab module GithubImport module Representation class PullRequest < Base - expose_attribute :iid, :title, :description, :source_branch, - :source_branch_sha, :target_branch, :target_branch_sha, - :milestone_number, :author, :assignee, :created_at, - :updated_at, :merged_at, :source_repository_id, + expose_attribute :iid, :title, :description, :source_branch_sha, :target_branch, + :target_branch_sha, :milestone_number, :author, :assignee, + :created_at, :updated_at, :merged_at, :source_repository_id, :target_repository_id, :source_repository_owner, :merged_by def parse(pr) @@ -40,9 +39,8 @@ def parse(pr) end def deserialize(hash) - super(hash) + super - attributes[:state] = attributes[:state].to_sym attributes[:author] &&= deserialize_with(Representation::User, attributes[:author]) # Assignees are optional so we only convert it from a attributes if one was set. @@ -50,51 +48,96 @@ def deserialize(hash) attributes[:merged_by] &&= deserialize_with(Representation::User, attributes[:merged_by]) end - def truncated_title - title.truncate(255) + def attributes_to_insert + @author_id, @author_found = user_finder.author_id_for(self) + + { + iid: iid, + title: title, + description: description, + source_project_id: project.id, + target_project_id: project.id, + source_branch: source_branch, + target_branch: attributes[:target_branch], + state_id: state_id, + milestone_id: milestone_id, + assignee_id: assignee_id, + author_id: author_id, + created_at: attributes[:created_at], + updated_at: attributes[:updated_at] + } end # Returns a formatted source branch. # # For cross-project pull requests the branch name will be in the format # `github/fork/owner-name/branch-name`. - def formatted_source_branch - if cross_project? && source_repository_owner - "github/fork/#{source_repository_owner}/#{source_branch}" - elsif source_branch == target_branch + def source_branch + if cross_project? && attributes[:source_repository_owner] + "github/fork/#{attributes[:source_repository_owner]}/#{attributes[:source_branch]}" + elsif attributes[:source_branch] == attributes[:target_branch] # Sometimes the source and target branch are the same, but GitLab # doesn't support this. This can happen when both the user and # source repository have been deleted, and the PR was submitted from # the fork's master branch. - "#{source_branch}-#{iid}" + "#{attributes[:source_branch]}-#{iid}" else - source_branch + attributes[:source_branch] end end + # Required by IssuableFinder + def issuable_type + 'MergeRequest' + end + + def github_identifiers + { + iid: iid, + issuable_type: issuable_type + } + end + + private + + attr_reader :author_id, :author_found + + def title + attributes[:title].truncate(255) + end + + def description + MarkdownText.format(attributes[:description], author, author_found) + end + + def state_id + ::MergeRequest.available_states[state] + end + def state if merged_at :merged else - attributes[:state] + attributes[:state].to_sym end end - def cross_project? - return true unless source_repository_id + def assignee_id + user_finder.assignee_id_for(self) + end - source_repository_id != target_repository_id + def milestone_id + MilestoneFinder.new(project).id_for(self) end - def issuable_type - 'MergeRequest' + def cross_project? + return true unless attributes[:source_repository_id] + + attributes[:source_repository_id] != attributes[:target_repository_id] end - def github_identifiers - { - iid: iid, - issuable_type: issuable_type - } + def user_finder + @user_finder ||= GithubImport::UserFinder.new(project, client) end end end -- GitLab