26378511fe
In https://gitlab.com/gitlab-org/release/framework/issues/28 we found that this method was changed a lot over the years: 43 times if our calculations were correct. Looking at the method, it had quite a few branches going on: def create_or_update_import_data(data: nil, credentials: nil) return if data.nil? && credentials.nil? project_import_data = import_data || build_import_data if data project_import_data.data ||= {} project_import_data.data = project_import_data.data.merge(data) end if credentials project_import_data.credentials ||= {} project_import_data.credentials = project_import_data.credentials.merge(credentials) end project_import_data end If we turn the || and ||= operators into regular if statements, we can see a bit more clearly that this method has quite a lot of branches in it: def create_or_update_import_data(data: nil, credentials: nil) if data.nil? && credentials.nil? return else project_import_data = if import_data import_data else build_import_data end if data if project_import_data.data # nothing else project_import_data.data = {} end project_import_data.data = project_import_data.data.merge(data) end if credentials if project_import_data.credentials # nothing else project_import_data.credentials = {} end project_import_data.credentials = project_import_data.credentials.merge(credentials) end project_import_data end end The number of if statements and branches here makes it easy to make mistakes. To resolve this, we refactor this code in such a way that we can get rid of all but the first `if data.nil? && credentials.nil?` statement. We can do this by simply sending `to_h` to `nil` in the right places, which removes the need for statements such as `if data`. Since this data gets written to a database, in ProjectImportData we do make sure to not write empty Hash values. This requires an `unless` (which is really a `if !`), but the resulting code is still very easy to read.
33 lines
991 B
Ruby
33 lines
991 B
Ruby
# frozen_string_literal: true
|
|
|
|
require 'carrierwave/orm/activerecord'
|
|
|
|
class ProjectImportData < ActiveRecord::Base
|
|
belongs_to :project, inverse_of: :import_data
|
|
attr_encrypted :credentials,
|
|
key: Settings.attr_encrypted_db_key_base,
|
|
marshal: true,
|
|
encode: true,
|
|
mode: :per_attribute_iv_and_salt,
|
|
insecure_mode: true,
|
|
algorithm: 'aes-256-cbc'
|
|
|
|
serialize :data, JSON # rubocop:disable Cop/ActiveRecordSerialize
|
|
|
|
validates :project, presence: true
|
|
|
|
before_validation :symbolize_credentials
|
|
|
|
def symbolize_credentials
|
|
# bang doesn't work here - attr_encrypted makes it not to work
|
|
self.credentials = self.credentials.deep_symbolize_keys unless self.credentials.blank?
|
|
end
|
|
|
|
def merge_data(hash)
|
|
self.data = data.to_h.merge(hash) unless hash.empty?
|
|
end
|
|
|
|
def merge_credentials(hash)
|
|
self.credentials = credentials.to_h.merge(hash) unless hash.empty?
|
|
end
|
|
end
|