From 85475f46df20597ef8009950e13163fe719e8d92 Mon Sep 17 00:00:00 2001 From: Alex Sherman Date: Mon, 30 Aug 2021 13:28:26 +0500 Subject: [PATCH] Fix possible transaction retries by setting idempotency --- Gemfile | 1 + Gemfile.lock | 4 ++- .../domains/bulk_renew/single_domain_renew.rb | 28 +++++++++++++++++-- app/models/epp/domain.rb | 15 +--------- .../repp/v1/domains/renews_test.rb | 7 ++++- test/test_helper.rb | 1 + 6 files changed, 38 insertions(+), 18 deletions(-) diff --git a/Gemfile b/Gemfile index 43dfb5299..c50efe25d 100644 --- a/Gemfile +++ b/Gemfile @@ -92,6 +92,7 @@ group :test do gem 'database_cleaner' gem 'minitest', '~> 5.14' gem 'simplecov', '0.17.1', require: false # CC last supported v0.17 + gem 'spy' gem 'webdrivers' gem 'webmock' end diff --git a/Gemfile.lock b/Gemfile.lock index f84d16005..1f54d141e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -456,6 +456,7 @@ GEM actionpack (>= 4.0) activesupport (>= 4.0) sprockets (>= 3.0.0) + spy (1.0.1) swd (1.2.0) activesupport (>= 3) attr_required (>= 0.0.5) @@ -568,6 +569,7 @@ DEPENDENCIES sidekiq simplecov (= 0.17.1) simpleidn (= 0.2.1) + spy truemail (~> 2.4) uglifier validates_email_format_of (= 1.6.3) @@ -577,4 +579,4 @@ DEPENDENCIES wkhtmltopdf-binary (~> 0.12.5.1) BUNDLED WITH - 2.2.24 + 2.2.26 diff --git a/app/interactions/domains/bulk_renew/single_domain_renew.rb b/app/interactions/domains/bulk_renew/single_domain_renew.rb index 5951ef9ec..5ae3826cd 100644 --- a/app/interactions/domains/bulk_renew/single_domain_renew.rb +++ b/app/interactions/domains/bulk_renew/single_domain_renew.rb @@ -8,9 +8,12 @@ module Domains object :registrar def execute + renewed_expire_time = prepare_renewed_expire_time in_transaction_with_retries do check_balance - success = domain.renew(domain.valid_to, period, unit) + success = domain.renew(renewed_expire_time: renewed_expire_time, + period: period, + unit: unit) if success check_balance reduce_balance @@ -42,7 +45,8 @@ module Domains else transaction_wrapper { yield } end - rescue ActiveRecord::StatementInvalid + rescue ActiveRecord::StatementInvalid => e + log_error e sleep rand / 100 retry end @@ -59,6 +63,26 @@ module Domains domain.add_epp_error(2104, nil, nil, I18n.t(:domain_renew_error_for_domain)) errors.add(:domain, I18n.t('domain_renew_error_for_domain', domain: domain.name)) end + + def prepare_renewed_expire_time + int_period = period.to_i + plural_period_unit_name = (unit == 'm' ? 'months' : 'years').to_sym + renewed_expire_time = domain.valid_to.advance(plural_period_unit_name => int_period.to_i) + + max_reg_time = 11.years.from_now + + if renewed_expire_time >= max_reg_time + domain.add_epp_error('2105', nil, nil, I18n.t('epp.domains.object_is_not_eligible_for_renewal', + max_date: max_reg_time.to_date.to_s(:db))) + end + renewed_expire_time + end + + def log_error(error) + message = (["#{self.class} - #{error.class}: #{error.message}"] + error.backtrace) + .join("\n") + Rails.logger.error message + end end end end diff --git a/app/models/epp/domain.rb b/app/models/epp/domain.rb index c5ce69776..0b3f1ad7f 100644 --- a/app/models/epp/domain.rb +++ b/app/models/epp/domain.rb @@ -173,26 +173,13 @@ class Epp::Domain < Domain ### RENEW ### - def renew(cur_exp_date, period, unit = 'y') + def renew(renewed_expire_time:, period:, unit:) @is_renewal = true - validate_exp_dates(cur_exp_date) add_renew_epp_errors unless renewable? return false if errors.any? - period = period.to_i - plural_period_unit_name = (unit == 'm' ? 'months' : 'years').to_sym - renewed_expire_time = valid_to.advance(plural_period_unit_name => period.to_i) - - max_reg_time = 11.years.from_now - - if renewed_expire_time >= max_reg_time - add_epp_error('2105', nil, nil, I18n.t('epp.domains.object_is_not_eligible_for_renewal', - max_date: max_reg_time.to_date.to_s(:db))) - return false if errors.any? - end - self.expire_time = renewed_expire_time self.outzone_at = nil self.delete_date = nil diff --git a/test/integration/repp/v1/domains/renews_test.rb b/test/integration/repp/v1/domains/renews_test.rb index c3e73669a..3949f49dd 100644 --- a/test/integration/repp/v1/domains/renews_test.rb +++ b/test/integration/repp/v1/domains/renews_test.rb @@ -13,6 +13,9 @@ class ReppV1DomainsRenewsTest < ActionDispatch::IntegrationTest def test_domain_can_be_renewed original_valid_to = @domain.valid_to travel_to Time.zone.parse('2010-07-05') + domain_spy = Spy.on_instance_method(Epp::Domain, :renew).and_call_through + renew_spy = Spy.on_instance_method(Domains::BulkRenew::SingleDomainRenew, + :prepare_renewed_expire_time).and_call_through @auth_headers['Content-Type'] = 'application/json' payload = { renew: { period: 1, period_unit: 'y', exp_date: original_valid_to } } @@ -24,6 +27,8 @@ class ReppV1DomainsRenewsTest < ActionDispatch::IntegrationTest assert_equal 'Command completed successfully', json[:message] assert @domain.valid_to, original_valid_to + 1.year + assert domain_spy.has_been_called? + assert renew_spy.has_been_called? end def test_domain_renew_pricing_error @@ -31,7 +36,7 @@ class ReppV1DomainsRenewsTest < ActionDispatch::IntegrationTest travel_to Time.zone.parse('2010-07-05') @auth_headers['Content-Type'] = 'application/json' - payload = { renew: { period: 100, period_unit: 'y', exp_date: original_valid_to } } + payload = { renew: { period: 10, period_unit: 'y', exp_date: original_valid_to } } post "/repp/v1/domains/#{@domain.name}/renew", headers: @auth_headers, params: payload.to_json json = JSON.parse(response.body, symbolize_names: true) diff --git a/test/test_helper.rb b/test/test_helper.rb index efdd982ce..98a7d0f39 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -19,6 +19,7 @@ require 'capybara/minitest' require 'webmock/minitest' require 'support/assertions/epp_assertions' require 'sidekiq/testing' +require 'spy/integration' Sidekiq::Testing.fake!