From 1796936abc8873c1fcc38061fe3d8b44acda3ffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Mon, 30 Oct 2017 20:21:56 -0300 Subject: [PATCH] Handle large values on `MergeRequestDiffCommit` dates --- app/models/merge_request_diff_commit.rb | 4 +- lib/gitlab/database.rb | 8 ++ spec/lib/gitlab/database_spec.rb | 22 +++++ spec/models/merge_request_diff_commit_spec.rb | 83 ++++++++++++++++++- 4 files changed, 114 insertions(+), 3 deletions(-) diff --git a/app/models/merge_request_diff_commit.rb b/app/models/merge_request_diff_commit.rb index 670b26d4ca3..b75387e236e 100644 --- a/app/models/merge_request_diff_commit.rb +++ b/app/models/merge_request_diff_commit.rb @@ -17,7 +17,9 @@ 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.type_cast_for_database(sha), + authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]), + committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date]) ) end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 357f16936c6..43a00d6cedb 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -4,6 +4,10 @@ module Gitlab # https://www.postgresql.org/docs/9.2/static/datatype-numeric.html # http://dev.mysql.com/doc/refman/5.7/en/integer-types.html MAX_INT_VALUE = 2147483647 + # The max value between MySQL's TIMESTAMP and PostgreSQL's timestampz: + # https://www.postgresql.org/docs/9.1/static/datatype-datetime.html + # https://dev.mysql.com/doc/refman/5.7/en/datetime.html + MAX_TIMESTAMP_VALUE = Time.at((1 << 31) - 1).freeze def self.config ActiveRecord::Base.configurations[Rails.env] @@ -120,6 +124,10 @@ module Gitlab EOF end + def self.sanitize_timestamp(timestamp) + MAX_TIMESTAMP_VALUE > timestamp ? timestamp : MAX_TIMESTAMP_VALUE.dup + end + # pool_size - The size of the DB pool. # host - An optional host name to use instead of the default one. def self.create_connection_pool(pool_size, host = nil) diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 5fa94999d25..7aeb85b8f5a 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -256,4 +256,26 @@ describe Gitlab::Database do expect(described_class.false_value).to eq 0 end end + + describe '#sanitize_timestamp' do + let(:max_timestamp) { Time.at((1 << 31) - 1) } + + subject { described_class.sanitize_timestamp(timestamp) } + + context 'with a timestamp smaller than MAX_TIMESTAMP_VALUE' do + let(:timestamp) { max_timestamp - 10.years } + + it 'returns the given timestamp' do + expect(subject).to eq(timestamp) + end + end + + context 'with a timestamp larger than MAX_TIMESTAMP_VALUE' do + let(:timestamp) { max_timestamp + 1.second } + + it 'returns MAX_TIMESTAMP_VALUE' do + expect(subject).to eq(max_timestamp) + end + end + end end diff --git a/spec/models/merge_request_diff_commit_spec.rb b/spec/models/merge_request_diff_commit_spec.rb index 9d4a0ecf8c0..7709cf43200 100644 --- a/spec/models/merge_request_diff_commit_spec.rb +++ b/spec/models/merge_request_diff_commit_spec.rb @@ -2,14 +2,93 @@ require 'rails_helper' describe MergeRequestDiffCommit do let(:merge_request) { create(:merge_request) } - subject { merge_request.commits.first } + let(:project) { merge_request.project } describe '#to_hash' do + subject { merge_request.commits.first } + it 'returns the same results as Commit#to_hash, except for parent_ids' do - commit_from_repo = merge_request.project.repository.commit(subject.sha) + commit_from_repo = project.repository.commit(subject.sha) commit_from_repo_hash = commit_from_repo.to_hash.merge(parent_ids: []) expect(subject.to_hash).to eq(commit_from_repo_hash) end end + + describe '.create_bulk' do + let(:sha_attribute) { Gitlab::Database::ShaAttribute.new } + let(:merge_request_diff_id) { merge_request.merge_request_diff.id } + let(:commits) do + [ + project.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e'), + project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') + ] + end + let(:rows) do + [ + { + "message": "Add submodule from gitlab.com\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n", + "authored_date": "2014-02-27T10:01:38.000+01:00".to_time, + "author_name": "Dmitriy Zaporozhets", + "author_email": "dmitriy.zaporozhets@gmail.com", + "committed_date": "2014-02-27T10:01:38.000+01:00".to_time, + "committer_name": "Dmitriy Zaporozhets", + "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') + }, + { + "message": "Change some files\n\nSigned-off-by: Dmitriy Zaporozhets \u003cdmitriy.zaporozhets@gmail.com\u003e\n", + "authored_date": "2014-02-27T09:57:31.000+01:00".to_time, + "author_name": "Dmitriy Zaporozhets", + "author_email": "dmitriy.zaporozhets@gmail.com", + "committed_date": "2014-02-27T09:57:31.000+01:00".to_time, + "committer_name": "Dmitriy Zaporozhets", + "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') + } + ] + end + + subject { described_class.create_bulk(merge_request_diff_id, commits) } + + it 'inserts the commits into the database en masse' do + expect(Gitlab::Database).to receive(:bulk_insert) + .with(described_class.table_name, rows) + + subject + end + + context 'with dates larger than the DB limit' do + let(:commits) do + # This commit's date is "Sun Aug 17 07:12:55 292278994 +0000" + [project.commit('ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69')] + end + let(:timestamp) { Time.at((1 << 31) - 1) } + let(:rows) do + [{ + "message": "Weird commit date\n", + "authored_date": timestamp, + "author_name": "Alejandro Rodríguez", + "author_email": "alejorro70@gmail.com", + "committed_date": timestamp, + "committer_name": "Alejandro Rodríguez", + "committer_email": "alejorro70@gmail.com", + "merge_request_diff_id": merge_request_diff_id, + "relative_order": 0, + "sha": sha_attribute.type_cast_for_database('ba3343bc4fa403a8dfbfcab7fc1a8c29ee34bd69') + }] + end + + it 'uses a sanitized date' do + expect(Gitlab::Database).to receive(:bulk_insert) + .with(described_class.table_name, rows) + + subject + end + end + end end