From 50cb6eca570a352a3b9799a66f77edad261763be Mon Sep 17 00:00:00 2001 From: Aleksei Lipniagov Date: Wed, 11 Sep 2019 18:32:24 +0000 Subject: Use 'gitlab_chronic_duration' gem Replace 'chronic_duration' to 'gitlab_chronic_duration', to make relevant method calls thread-safe. --- Gemfile | 2 +- Gemfile.lock | 6 +-- ...637-use-chronic-duration-in-thread-safe-way.yml | 5 +++ config/initializers/chronic_duration.rb | 2 - lib/gitlab/patch/chronic_duration.rb | 35 ---------------- lib/gitlab/time_tracking_formatter.rb | 47 +++++++++++----------- spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb | 2 +- spec/lib/gitlab/patch/chronic_duration_spec.rb | 27 ------------- 8 files changed, 34 insertions(+), 92 deletions(-) create mode 100644 changelogs/unreleased/66637-use-chronic-duration-in-thread-safe-way.yml delete mode 100644 lib/gitlab/patch/chronic_duration.rb delete mode 100644 spec/lib/gitlab/patch/chronic_duration_spec.rb diff --git a/Gemfile b/Gemfile index 87e3e42c59a..d79e97aabdd 100644 --- a/Gemfile +++ b/Gemfile @@ -265,7 +265,7 @@ gem 'fast_blank' # Parse time & duration gem 'chronic', '~> 0.10.2' -gem 'chronic_duration', '~> 0.10.6' +gem 'gitlab_chronic_duration', '~> 0.10.6.1' gem 'webpack-rails', '~> 0.9.10' gem 'rack-proxy', '~> 0.6.0' diff --git a/Gemfile.lock b/Gemfile.lock index fbdb78663e4..025542422d3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -136,8 +136,6 @@ GEM childprocess (0.9.0) ffi (~> 1.0, >= 1.0.11) chronic (0.10.2) - chronic_duration (0.10.6) - numerizer (~> 0.1.1) chunky_png (1.3.5) citrus (3.0.2) claide (1.0.3) @@ -356,6 +354,8 @@ GEM rubocop-gitlab-security (~> 0.1.0) rubocop-performance (~> 1.1.0) rubocop-rspec (~> 1.19) + gitlab_chronic_duration (0.10.6.1) + numerizer (~> 0.1.1) gitlab_omniauth-ldap (2.1.1) net-ldap (~> 0.16) omniauth (~> 1.3) @@ -1089,7 +1089,6 @@ DEPENDENCIES carrierwave (~> 1.3) charlock_holmes (~> 0.7.5) chronic (~> 0.10.2) - chronic_duration (~> 0.10.6) commonmarker (~> 0.17) concurrent-ruby (~> 1.1) connection_pool (~> 2.0) @@ -1141,6 +1140,7 @@ DEPENDENCIES gitlab-peek (~> 0.0.1) gitlab-sidekiq-fetcher (= 0.5.2) gitlab-styles (~> 2.7) + gitlab_chronic_duration (~> 0.10.6.1) gitlab_omniauth-ldap (~> 2.1.1) gon (~> 6.2) google-api-client (~> 0.23) diff --git a/changelogs/unreleased/66637-use-chronic-duration-in-thread-safe-way.yml b/changelogs/unreleased/66637-use-chronic-duration-in-thread-safe-way.yml new file mode 100644 index 00000000000..549f523076e --- /dev/null +++ b/changelogs/unreleased/66637-use-chronic-duration-in-thread-safe-way.yml @@ -0,0 +1,5 @@ +--- +title: Use `ChronicDuration` in a thread-safe way +merge_request: 32817 +author: +type: fixed diff --git a/config/initializers/chronic_duration.rb b/config/initializers/chronic_duration.rb index aa43eef434c..5528df6c660 100644 --- a/config/initializers/chronic_duration.rb +++ b/config/initializers/chronic_duration.rb @@ -1,5 +1,3 @@ # frozen_string_literal: true ChronicDuration.raise_exceptions = true - -ChronicDuration.prepend Gitlab::Patch::ChronicDuration diff --git a/lib/gitlab/patch/chronic_duration.rb b/lib/gitlab/patch/chronic_duration.rb deleted file mode 100644 index ab3cba3657f..00000000000 --- a/lib/gitlab/patch/chronic_duration.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -# Fixes a bug where parsing months doesn't take into account -# the ChronicDuration.days_per_week setting -# -# We can remove this when we do a refactor and push upstream in -# https://gitlab.com/gitlab-org/gitlab-ce/issues/66637 - -module Gitlab - module Patch - module ChronicDuration - extend ActiveSupport::Concern - - class_methods do - def duration_units_seconds_multiplier(unit) - return 0 unless duration_units_list.include?(unit) - - case unit - when 'months' - 3600 * ::ChronicDuration.hours_per_day * ::ChronicDuration.days_per_month - else - super - end - end - - # ChronicDuration#output uses 1mo = 4w as the conversion so we do the same here. - # We do need to add a special case for the default days_per_week value because - # we want to retain existing behavior for the default case - def days_per_month - ::ChronicDuration.days_per_week == 7 ? 30 : ::ChronicDuration.days_per_week * 4 - end - end - end - end -end diff --git a/lib/gitlab/time_tracking_formatter.rb b/lib/gitlab/time_tracking_formatter.rb index 302da91328a..31883527135 100644 --- a/lib/gitlab/time_tracking_formatter.rb +++ b/lib/gitlab/time_tracking_formatter.rb @@ -4,37 +4,38 @@ module Gitlab module TimeTrackingFormatter extend self - def parse(string) - with_custom_config do - string = string.sub(/\A-/, '') + # We may want to configure it through project settings in a future version. + CUSTOM_DAY_AND_WEEK_LENGTH = { hours_per_day: 8, days_per_month: 20 }.freeze - seconds = ChronicDuration.parse(string, default_unit: 'hours') rescue nil - seconds *= -1 if seconds && Regexp.last_match - seconds - end + def parse(string) + string = string.sub(/\A-/, '') + + seconds = + begin + ChronicDuration.parse( + string, + CUSTOM_DAY_AND_WEEK_LENGTH.merge(default_unit: 'hours')) + rescue + nil + end + + seconds *= -1 if seconds && Regexp.last_match + seconds end def output(seconds) - with_custom_config do - ChronicDuration.output(seconds, format: :short, limit_to_hours: limit_to_hours_setting, weeks: true) rescue nil - end + ChronicDuration.output( + seconds, + CUSTOM_DAY_AND_WEEK_LENGTH.merge( + format: :short, + limit_to_hours: limit_to_hours_setting, + weeks: true)) + rescue + nil end private - def with_custom_config - # We may want to configure it through project settings in a future version. - ChronicDuration.hours_per_day = 8 - ChronicDuration.days_per_week = 5 - - result = yield - - ChronicDuration.hours_per_day = 24 - ChronicDuration.days_per_week = 7 - - result - end - def limit_to_hours_setting Gitlab::CurrentSettings.time_tracking_limit_to_hours end diff --git a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb index c25344ec1a4..18037a5612c 100644 --- a/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/rules/rule_spec.rb @@ -1,5 +1,5 @@ require 'fast_spec_helper' -require 'chronic_duration' +require 'gitlab_chronic_duration' require 'support/helpers/stub_feature_flags' require_dependency 'active_model' diff --git a/spec/lib/gitlab/patch/chronic_duration_spec.rb b/spec/lib/gitlab/patch/chronic_duration_spec.rb deleted file mode 100644 index 541037ec1a2..00000000000 --- a/spec/lib/gitlab/patch/chronic_duration_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::Patch::ChronicDuration do - subject { ChronicDuration.parse('1mo') } - - it 'uses default conversions' do - expect(subject).to eq(2_592_000) - end - - context 'with custom conversions' do - before do - ChronicDuration.hours_per_day = 8 - ChronicDuration.days_per_week = 5 - end - - after do - ChronicDuration.hours_per_day = 24 - ChronicDuration.days_per_week = 7 - end - - it 'uses custom conversions' do - expect(subject).to eq(576_000) - end - end -end -- cgit v1.2.1