Refactor

5 files changed, 49 insertions, 37 deletions

diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb
index 9f21b927118..0fa06032897 100644
--- a/app/models/merge_request_diff_commit.rb
+++ b/app/models/merge_request_diff_commit.rb
@@ -14,13 +14,10 @@ class MergeRequestDiffCommit < ActiveRecord::Base
       commit_hash = commit.to_hash.except(:parent_ids)
       sha = commit_hash.delete(:id)
 
-      # rubocop:disable Cop/ActiveRecordSerialize
-      sha_serialized = Gitlab.rails5? ? sha_attribute.serialize(sha) : sha_attribute.type_cast_for_database(sha)
-
       commit_hash.merge(
         merge_request_diff_id: merge_request_diff_id,
         relative_order: index,
-        sha: sha_serialized,
+        sha: sha_attribute.serialize(sha),
         authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]),
         committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date])
       )
diff --git a/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits.rb b/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits.rb
index fd5cbf76e47..a357538a885 100644
--- a/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits.rb
+++ b/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits.rb
@@ -96,7 +96,7 @@ module Gitlab
           commit_hash.merge(
             merge_request_diff_id: merge_request_diff.id,
             relative_order: index,
-            sha: sha_attribute.type_cast_for_database(sha)
+            sha: sha_attribute.serialize(sha)
           )
         end
 
diff --git a/lib/gitlab/database/sha_attribute.rb b/lib/gitlab/database/sha_attribute.rb
index 400bdd01c2f..882e31e711f 100644
--- a/lib/gitlab/database/sha_attribute.rb
+++ b/lib/gitlab/database/sha_attribute.rb
@@ -1,19 +1,13 @@
 module Gitlab
   module Database
-    BINARY_TYPE = if Gitlab::Database.postgresql?
-                    # PostgreSQL defines its own class with slightly different
-                    # behaviour from the default Binary type.
-                    ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Bytea
-                  else
-                    if Gitlab.rails5?
-                      # In Rails 5.0 `Type` has been moved from `ActiveRecord` to `ActiveModel`
-                      # https://github.com/rails/rails/commit/9cc8c6f3730df3d94c81a55be9ee1b7b4ffd29f6#diff-f8ba7983a51d687976e115adcd95822b
-                      # Remove this if-condition and leave just `ActiveModel::Type::Binary` when upgraded to rails 5.0
-                      ActiveModel::Type::Binary
-                    else
-                      ActiveRecord::Type::Binary
-                    end
-                  end
+    BINARY_TYPE =
+      if Gitlab::Database.postgresql?
+        # PostgreSQL defines its own class with slightly different
+        # behaviour from the default Binary type.
+        ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Bytea
+      else
+        type_binary
+      end
 
     # Class for casting binary data to hexadecimal SHA1 hashes (and vice-versa).
     #
@@ -23,18 +17,46 @@ module Gitlab
     class ShaAttribute < BINARY_TYPE
       PACK_FORMAT = 'H*'.freeze
 
-      # Casts binary data to a SHA1 in hexadecimal.
+      # It is called from activerecord-4.2.10/lib/active_record/attribute.rb
+      # Remove this method when upgraded to rails 5.0
       def type_cast_from_database(value)
-        value = Gitlab.rails5? ? deserialize(value) : super
+        unpack_sha(super)
+      end
+
+      # It is called from activerecord-5.0.6/lib/active_record/attribute.rb
+      # Remove this method when upgraded to rails 5.0.
+      def deserialize(value)
+        value = Gitlab.rails5? ? super : method(:type_cast_from_database).super_method.call(value)
+
+        unpack_sha(value)
+      end
 
+      # Rename this method to `deserialize(value)` when upgraded to rails 5.0
+      # Casts binary data to a SHA1 in hexadecimal.
+      def unpack_sha(value)
+        # Uncomment this line when upgraded to rails 5.0
+        # value = super
         value ? value.unpack(PACK_FORMAT)[0] : nil
       end
 
       # Casts a SHA1 in hexadecimal to the proper binary format.
-      def type_cast_for_database(value)
+      def serialize(value)
         arg = value ? [value].pack(PACK_FORMAT) : nil
 
-        Gitlab.rails5? ? serialize(arg) : super(arg)
+        Gitlab.rails5? ? super(arg) : type_cast_for_database(arg)
+      end
+    end
+
+    private
+
+    # In Rails 5.0 `Type` has been moved from `ActiveRecord` to `ActiveModel`
+    # https://github.com/rails/rails/commit/9cc8c6f3730df3d94c81a55be9ee1b7b4ffd29f6#diff-f8ba7983a51d687976e115adcd95822b
+    # Remove this method and leave just `ActiveModel::Type::Binary` when upgraded to rails 5.0
+    def type_binary
+      if Gitlab.rails5?
+        ActiveModel::Type::Binary
+      else
+        ActiveRecord::Type::Binary
       end
     end
   end
diff --git a/spec/lib/gitlab/database/sha_attribute_spec.rb b/spec/lib/gitlab/database/sha_attribute_spec.rb
index 62c1d37ea1c..778bfa2cc47 100644
--- a/spec/lib/gitlab/database/sha_attribute_spec.rb
+++ b/spec/lib/gitlab/database/sha_attribute_spec.rb
@@ -19,15 +19,15 @@ describe Gitlab::Database::ShaAttribute do
 
   let(:attribute) { described_class.new }
 
-  describe '#type_cast_from_database' do
+  describe '#deserialize' do
     it 'converts the binary SHA to a String' do
-      expect(attribute.type_cast_from_database(binary_from_db)).to eq(sha)
+      expect(attribute.deserialize(binary_from_db)).to eq(sha)
     end
   end
 
-  describe '#type_cast_for_database' do
+  describe '#serialize' do
     it 'converts a SHA String to binary data' do
-      expect(attribute.type_cast_for_database(sha).to_s).to eq(binary_sha)
+      expect(attribute.serialize(sha).to_s).to eq(binary_sha)
     end
   end
 end
diff --git a/spec/models/merge_request_diff_commit_spec.rb b/spec/models/merge_request_diff_commit_spec.rb
index c8f3bbdac48..8c01a7ac18f 100644
--- a/spec/models/merge_request_diff_commit_spec.rb
+++ b/spec/models/merge_request_diff_commit_spec.rb
@@ -36,7 +36,7 @@ describe MergeRequestDiffCommit do
           "committer_email": "dmitriy.zaporozhets@gmail.com",
           "merge_request_diff_id": merge_request_diff_id,
           "relative_order": 0,
-          "sha": serialize_sha("5937ac0a7beb003549fc5fd26fc247adbce4a52e")
+          "sha": sha_attribute.serialize("5937ac0a7beb003549fc5fd26fc247adbce4a52e")
         },
         {
           "message": "Change some files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n",
@@ -48,7 +48,7 @@ describe MergeRequestDiffCommit do
           "committer_email": "dmitriy.zaporozhets@gmail.com",
           "merge_request_diff_id": merge_request_diff_id,
           "relative_order": 1,
-          "sha": serialize_sha("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
+          "sha": sha_attribute.serialize("570e7b2abdd848b95f2f578043fc23bd6f6fd24d")
         }
       ]
     end
@@ -79,7 +79,7 @@ describe MergeRequestDiffCommit do
           "committer_email": "alejorro70@gmail.com",
           "merge_request_diff_id": merge_request_diff_id,
           "relative_order": 0,
-          "sha": serialize_sha("ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69")
+          "sha": sha_attribute.serialize("ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69")
         }]
       end
 
@@ -90,12 +90,5 @@ describe MergeRequestDiffCommit do
         subject
       end
     end
-
-    private
-
-    # Remove this method when upgraded to rails 5.0.
-    def serialize_sha(sha)
-      Gitlab.rails5? ? sha_attribute.serialize(sha) : sha_attribute.type_cast_for_database(sha)
-    end
   end
 end