From 8f1fe263c4284e6e40dadb130518ea3ea514e113 Mon Sep 17 00:00:00 2001 From: Lud Date: Tue, 18 Jul 2023 17:23:51 +0200 Subject: [PATCH] Strict Types, BigDecimal everywhere (#7) --- .github/workflows/ci.yml | 2 ++ lib/money/distributed/fetcher/base.rb | 31 ++++++++++++---- lib/money/distributed/fetcher/file.rb | 2 +- lib/money/distributed/redis.rb | 10 +++--- lib/money/distributed/storage.rb | 39 ++++++++++++++++++--- spec/money/distributed/fetcher/file_spec.rb | 29 +++++++++------ spec/money/distributed/storage_spec.rb | 12 +++---- spec/spec_helper.rb | 2 +- 8 files changed, 93 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ce96edc..c4d6db4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,6 +3,7 @@ on: [push] jobs: test: strategy: + fail-fast: false matrix: ruby: - 3.0 @@ -42,6 +43,7 @@ jobs: bundle install -j $(getconf _NPROCESSORS_ONLN) --retry 3 - name: Run RSspec + continue-on-error: true run: bundle exec rspec --format documentation - name: Run Sorbet Typechecks diff --git a/lib/money/distributed/fetcher/base.rb b/lib/money/distributed/fetcher/base.rb index 492070f..b8991bf 100644 --- a/lib/money/distributed/fetcher/base.rb +++ b/lib/money/distributed/fetcher/base.rb @@ -1,4 +1,4 @@ -# typed: true +# typed: strict # frozen_string_literal: true class Money @@ -6,31 +6,48 @@ module Distributed module Fetcher # Base class for rates fetchers module Base + extend T::Sig + extend T::Generic + + abstract! + + sig { params(bank: T.nilable(Money::Bank::VariableExchange)).void } def initialize(bank = nil) - @bank = bank || Money.default_bank + @bank = T.let(bank || Money.default_bank, Money::Bank::VariableExchange) end + sig { void } def fetch rates = exchange_rates currencies = rates.keys - currencies.each { add_rate(_1, _1, 1) } + # rate from currency to itself is always 1 + currencies.each { add_rate(_1, _1, BigDecimal('1')) } currencies.combination(2).each do |curr_1, curr_2| - rate = rates[curr_2] / rates[curr_1] + curr_1 = T.cast(curr_1, String) + curr_2 = T.cast(curr_2, String) + rate = rates.fetch(curr_2) / rates.fetch(curr_1) add_rate(curr_1, curr_2, rate) end end + sig do + params( + from_iso: String, + to_iso: String, + rate: BigDecimal, + ).void + end private def add_rate(from_iso, to_iso, rate) - @bank.add_rate(from_iso, to_iso, rate.round(4)) + @bank.add_rate(from_iso, to_iso, rate) return if from_iso == to_iso - @bank.add_rate(to_iso, from_iso, (1 / rate).round(4)) + @bank.add_rate(to_iso, from_iso, 1 / rate) end + sig { abstract.returns(T::Hash[String, BigDecimal]) } private def exchange_rates - raise NotImplementedError end end end diff --git a/lib/money/distributed/fetcher/file.rb b/lib/money/distributed/fetcher/file.rb index 9f77bbb..bf09547 100644 --- a/lib/money/distributed/fetcher/file.rb +++ b/lib/money/distributed/fetcher/file.rb @@ -24,7 +24,7 @@ def initialize(file_path, bank = nil) @file_path = file_path end - sig { returns(T::Hash[String, BigDecimal]) } + sig { override.returns(T::Hash[String, BigDecimal]) } private def exchange_rates ::File.read(@file_path).split("\n").each_with_object({}) do |line, h| code_rate = line.split diff --git a/lib/money/distributed/redis.rb b/lib/money/distributed/redis.rb index 6068e01..5894a5a 100644 --- a/lib/money/distributed/redis.rb +++ b/lib/money/distributed/redis.rb @@ -1,4 +1,4 @@ -# typed: true +# typed: strict # frozen_string_literal: true class Money @@ -9,16 +9,16 @@ class Redis sig do params( - redis: T.any(::Redis, ConnectionPool, Hash, Proc), + redis: T.any(::Redis, ConnectionPool, T::Hash[T.untyped, T.untyped], Proc), ).void end def initialize(redis) - @redis_proc = build_redis_proc(redis) + @redis_proc = T.let(build_redis_proc(redis), Proc) end sig do params( - block: T.proc.returns(T.untyped), + block: T.proc.params(redis_or_similar: T.untyped).returns(T.untyped), ).returns(T.untyped) end def exec(&block) @@ -27,7 +27,7 @@ def exec(&block) sig do params( - redis: T.any(::Redis, ConnectionPool, Hash, Proc), + redis: T.any(::Redis, ConnectionPool, T::Hash[T.untyped, T.untyped], Proc), ).returns(Proc) end private def build_redis_proc(redis) diff --git a/lib/money/distributed/storage.rb b/lib/money/distributed/storage.rb index 64cb9c6..d4407d0 100644 --- a/lib/money/distributed/storage.rb +++ b/lib/money/distributed/storage.rb @@ -5,26 +5,47 @@ class Money module Distributed # Storage for `Money::Bank::VariableExchange` that stores rates in Redis class Storage + extend T::Sig + INDEX_KEY_SEPARATOR = '_TO_' REDIS_KEY = 'money_rates' + sig do + params( + redis: T.any(::Redis, ConnectionPool, T::Hash[T.untyped, T.untyped], Proc), + cache_ttl: T.nilable(Integer), + ).void + end def initialize(redis, cache_ttl = nil) - @redis = Money::Distributed::Redis.new(redis) + @redis = T.let(Money::Distributed::Redis.new(redis), Money::Distributed::Redis) - @cache = {} + @cache = T.let({}, T::Hash[String, BigDecimal]) @cache_ttl = cache_ttl - @cache_updated_at = nil + @cache_updated_at = T.let(nil, T.nilable(Time)) @lock = Concurrent::ReentrantReadWriteLock.new end + sig do + params( + iso_from: String, + iso_to: String, + rate: BigDecimal, + ).void + end def add_rate(iso_from, iso_to, rate) @redis.exec do |r| - r.hset(REDIS_KEY, key_for(iso_from, iso_to), rate) + r.hset(REDIS_KEY, key_for(iso_from, iso_to), rate.to_s) end clear_cache end + sig do + params( + iso_from: String, + iso_to: String, + ).returns(T.nilable(BigDecimal)) + end def get_rate(iso_from, iso_to) cached_rates[key_for(iso_from, iso_to)] end @@ -45,10 +66,17 @@ def transaction yield end + sig { returns(T::Array[T.untyped]) } def marshal_dump [self.class, @cache_ttl] end + sig do + params( + iso_from: String, + iso_to: String, + ).returns(String) + end private def key_for(iso_from, iso_to) [iso_from, iso_to].join(INDEX_KEY_SEPARATOR).upcase end @@ -60,6 +88,7 @@ def cached_rates end end + sig { returns(T::Boolean) } private def cache_outdated? return false unless @cache_ttl @@ -67,12 +96,14 @@ def cached_rates @cache_updated_at < Time.now - @cache_ttl end + sig { void } def clear_cache Money::Distributed::ReadWriteLock.write(@lock) do @cache.clear end end + sig { void } private def retrieve_rates updated_cache = {} diff --git a/spec/money/distributed/fetcher/file_spec.rb b/spec/money/distributed/fetcher/file_spec.rb index 9c1016b..14a803e 100644 --- a/spec/money/distributed/fetcher/file_spec.rb +++ b/spec/money/distributed/fetcher/file_spec.rb @@ -7,19 +7,28 @@ subject { described_class.new(file_path, bank) } let(:file_path) { File.expand_path('../../../fixtures/rates.txt', __dir__) } - let(:bank) { double(add_rate: true) } + let(:bank) { Money::Bank::VariableExchange.new } + + before do + allow(bank).to receive(:add_rate).and_call_original + end it 'fetches rates from the file' do subject.fetch - expect(bank).to have_received(:add_rate).with('USD', 'USD', 1.0) - expect(bank).to have_received(:add_rate).with('AUD', 'AUD', 1.0) - expect(bank).to have_received(:add_rate).with('EUR', 'EUR', 1.0) - expect(bank).to have_received(:add_rate).with('USD', 'AUD', 1.3209) - expect(bank).to have_received(:add_rate).with('AUD', 'USD', 0.7571) - expect(bank).to have_received(:add_rate).with('USD', 'EUR', 0.9076) - expect(bank).to have_received(:add_rate).with('EUR', 'USD', 1.1018) - expect(bank).to have_received(:add_rate).with('AUD', 'EUR', 0.6871) - expect(bank).to have_received(:add_rate).with('EUR', 'AUD', 1.4554) + # trivial + expect(bank).to have_received(:add_rate).with('USD', 'USD', BigDecimal('1.0')) + expect(bank).to have_received(:add_rate).with('AUD', 'AUD', BigDecimal('1.0')) + expect(bank).to have_received(:add_rate).with('EUR', 'EUR', BigDecimal('1.0')) + + # non-trivial combinations + expect(bank).to have_received(:add_rate).with('USD', 'AUD', BigDecimal('1.320898')) + expect(bank).to have_received(:add_rate).with('AUD', 'USD', 1 / BigDecimal('1.320898')) + expect(bank).to have_received(:add_rate).with('USD', 'EUR', BigDecimal('0.907601')) + expect(bank).to have_received(:add_rate).with('EUR', 'USD', 1 / BigDecimal('0.907601')) + expect(bank).to have_received(:add_rate).with('AUD', 'EUR', BigDecimal('0.907601') / BigDecimal('1.320898')) + + # @NOTE: due to precision, 1/(a/b) != b/a + expect(bank).to have_received(:add_rate).with('EUR', 'AUD', 1 / (BigDecimal('0.907601') / BigDecimal('1.320898'))) end end diff --git a/spec/money/distributed/storage_spec.rb b/spec/money/distributed/storage_spec.rb index ec00382..823f59a 100644 --- a/spec/money/distributed/storage_spec.rb +++ b/spec/money/distributed/storage_spec.rb @@ -10,8 +10,8 @@ let(:ttl) { 3600 } it 'stores rates in redis' do - subject.add_rate 'USD', 'RUB', 60.123 - expect(redis.hget(described_class::REDIS_KEY, 'USD_TO_RUB')).to eq '60.123' + subject.add_rate 'USD', 'RUB', BigDecimal('60.123') + expect(redis.hget(described_class::REDIS_KEY, 'USD_TO_RUB')).to eq('0.60123e2') end it 'gets rates from redis' do @@ -40,14 +40,14 @@ let(:storage) { subject } let(:rates) do { - 'USD_TO_EUR' => '0.85', - 'EUR_TO_USD' => '1.18', + 'USD_TO_EUR' => BigDecimal('0.85'), + 'EUR_TO_USD' => BigDecimal('1.18'), } end before do rates.each do |key, rate| - redis.hset(described_class::REDIS_KEY, key, rate) + redis.hset(described_class::REDIS_KEY, key, rate.to_s) end end @@ -58,7 +58,7 @@ threads = [] 10.times do threads << Thread.new do - 1000.times do + 100.times do key, rate = rates.to_a.sample iso_from, iso_to = key.split(described_class::INDEX_KEY_SEPARATOR) storage.add_rate(iso_from, iso_to, rate) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3280d78..dfde1c3 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,4 +1,4 @@ -# typed: false +# typed: true # frozen_string_literal: true require 'rspec'