Merge branch 'blackst0ne-rails5-fix-type-cast-for-from-database' into 'master'blackst0ne-rails5-update-skip-befor-action-in-authenticates-with-two-factor-concern

[Rails5] Update `type_cast_*_database`  methods

See merge request gitlab-org/gitlab-ce!18188

6 files changed, 66 insertions, 34 deletions

diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb
index b75387e236e..1c2e57bb01f 100644
--- a/app/models/merge_request_diff_commit.rb
+++ b/app/models/merge_request_diff_commit.rb
@@ -17,7 +17,7 @@ class MergeRequestDiffCommit < ActiveRecord::Base
       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), # rubocop:disable Cop/ActiveRecordSerialize
         authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]),
         committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date])
       )
diff --git a/config/initializers/active_record_array_type_casting.rb b/config/initializers/active_record_array_type_casting.rb
index d94d592add6..a149e048ee2 100644
--- a/config/initializers/active_record_array_type_casting.rb
+++ b/config/initializers/active_record_array_type_casting.rb
@@ -1,20 +1,23 @@
-module ActiveRecord
-  class PredicateBuilder
-    class ArrayHandler
-      module TypeCasting
-        def call(attribute, value)
-          # This is necessary because by default ActiveRecord does not respect
-          # custom type definitions (like our `ShaAttribute`) when providing an
-          # array in `where`, like in `where(commit_sha: [sha1, sha2, sha3])`.
-          model = attribute.relation&.engine
-          type = model.user_provided_columns[attribute.name] if model
-          value = value.map { |value| type.type_cast_for_database(value) } if type
+# Remove this initializer when upgraded to Rails 5.0
+unless Gitlab.rails5?
+  module ActiveRecord
+    class PredicateBuilder
+      class ArrayHandler
+        module TypeCasting
+          def call(attribute, value)
+            # This is necessary because by default ActiveRecord does not respect
+            # custom type definitions (like our `ShaAttribute`) when providing an
+            # array in `where`, like in `where(commit_sha: [sha1, sha2, sha3])`.
+            model = attribute.relation&.engine
+            type = model.user_provided_columns[attribute.name] if model
+            value = value.map { |value| type.type_cast_for_database(value) } if type
 
-          super(attribute, value)
+            super(attribute, value)
+          end
         end
-      end
 
-      prepend TypeCasting
+        prepend TypeCasting
+      end
     end
   end
 end
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 d9400e04b83..b2d8ee81977 100644
--- a/lib/gitlab/database/sha_attribute.rb
+++ b/lib/gitlab/database/sha_attribute.rb
@@ -1,12 +1,20 @@
 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
-                    ActiveRecord::Type::Binary
-                  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
+        # 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 removing Gitlab.rails5? code.
+        if Gitlab.rails5?
+          ActiveModel::Type::Binary
+        else
+          ActiveRecord::Type::Binary
+        end
+      end
 
     # Class for casting binary data to hexadecimal SHA1 hashes (and vice-versa).
     #
@@ -16,18 +24,39 @@ 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 internal methods.
+      # Remove this method when removing Gitlab.rails5? code.
       def type_cast_from_database(value)
-        value = super
+        unpack_sha(super)
+      end
+
+      # It is called from activerecord-4.2.10/lib/active_record internal methods.
+      # Remove this method when removing Gitlab.rails5? code.
+      def type_cast_for_database(value)
+        serialize(value)
+      end
 
+      # It is called from activerecord-5.0.6/lib/active_record/attribute.rb
+      # Remove this method when removing Gitlab.rails5? code..
+      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)` removing Gitlab.rails5? code.
+      # Casts binary data to a SHA1 in hexadecimal.
+      def unpack_sha(value)
+        # Uncomment this line when removing Gitlab.rails5? code.
+        # 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
 
-        super(arg)
+        Gitlab.rails5? ? super(arg) : method(:type_cast_for_database).super_method.call(arg)
       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 7709cf43200..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": sha_attribute.type_cast_for_database('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": sha_attribute.type_cast_for_database('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": sha_attribute.type_cast_for_database('ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69')
+          "sha": sha_attribute.serialize("ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69")
         }]
       end