From 16caa04e8db836605ea69e94595b871c9e45313e Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 29 Mar 2024 11:30:40 -0400 Subject: [PATCH 01/18] DEV-1096 Rights API: translate OFFSET queries into inequalities - Add `QueryOptimizer` class which operates mainly on a `QueryParser` and emits a modified `offset` and additional `where` clauses. - Add `Cache` class to record last model object found, from which the new `where` can be constructed. - `default_order` changed to Array and always applied even when redundant, enforcing an intrinsic order. FIXME: - When items age out of the cache, removal is O(n) operation. - `QueryOptimizer` unit tests are a dismal placeholder. --- Gemfile | 6 +- Gemfile.lock | 6 ++ lib/rights_api.rb | 3 + lib/rights_api/cache.rb | 32 ++++++++ lib/rights_api/model_extensions.rb | 9 ++- lib/rights_api/models/rights_current.rb | 21 +++++- lib/rights_api/models/rights_log.rb | 21 +++++- lib/rights_api/query.rb | 23 +++--- lib/rights_api/query_optimizer.rb | 73 +++++++++++++++++++ lib/rights_api/query_parser.rb | 4 +- lib/rights_api/services.rb | 6 ++ spec/integration/rights_log_spec.rb | 36 +++++++-- spec/integration/rights_spec.rb | 40 +++++++--- spec/unit/cache_spec.rb | 27 +++++++ spec/unit/models/access_profile_spec.rb | 4 +- spec/unit/models/access_statement_map_spec.rb | 4 +- spec/unit/models/access_statement_spec.rb | 4 +- spec/unit/models/attribute_spec.rb | 4 +- spec/unit/models/reason_spec.rb | 4 +- spec/unit/models/rights_current_spec.rb | 4 +- spec/unit/models/rights_log_spec.rb | 4 +- spec/unit/models/source_spec.rb | 4 +- spec/unit/query_optimizer_spec.rb | 14 ++++ spec/unit/query_parser_spec.rb | 3 - 24 files changed, 300 insertions(+), 56 deletions(-) create mode 100644 lib/rights_api/cache.rb create mode 100644 lib/rights_api/query_optimizer.rb create mode 100644 spec/unit/cache_spec.rb create mode 100644 spec/unit/query_optimizer_spec.rb diff --git a/Gemfile b/Gemfile index 22b9afe..35863ad 100644 --- a/Gemfile +++ b/Gemfile @@ -3,6 +3,7 @@ source "https://rubygems.org" gem "canister" gem "climate_control" gem "json" +gem "json-canonicalization" gem "mysql2" gem "puma" gem "sequel" @@ -10,10 +11,11 @@ gem "sinatra" gem "sinatra-contrib" group :development, :test do + gem "ffi-icu" gem "pry" - gem "standard" - gem "rspec" gem "rack-test" + gem "rspec" gem "simplecov" gem "simplecov-lcov" + gem "standard" end diff --git a/Gemfile.lock b/Gemfile.lock index 501e8df..e3ba948 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -9,7 +9,11 @@ GEM coderay (1.1.3) diff-lcs (1.5.0) docile (1.4.0) + ffi (1.16.3) + ffi-icu (0.5.3) + ffi (~> 1.0, >= 1.0.9) json (2.6.3) + json-canonicalization (1.0.0) language_server-protocol (3.17.0.3) lint_roller (1.1.0) method_source (1.0.0) @@ -110,7 +114,9 @@ PLATFORMS DEPENDENCIES canister climate_control + ffi-icu json + json-canonicalization mysql2 pry puma diff --git a/lib/rights_api.rb b/lib/rights_api.rb index cb5c811..839eb7e 100644 --- a/lib/rights_api.rb +++ b/lib/rights_api.rb @@ -4,8 +4,11 @@ module RightsAPI end require "rights_api/app" +require "rights_api/cache" require "rights_api/database" require "rights_api/query" +require "rights_api/query_optimizer" +require "rights_api/query_parser" require "rights_api/result" require "rights_api/result/error_result" require "rights_api/schema" diff --git a/lib/rights_api/cache.rb b/lib/rights_api/cache.rb new file mode 100644 index 0000000..1187afd --- /dev/null +++ b/lib/rights_api/cache.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module RightsAPI + class Cache + DEFAULT_CACHE_SIZE = 1000 + + def initialize(size: DEFAULT_CACHE_SIZE) + @size = size + # Model name + canonicalized params MD5 in order of oldest to newest + @keys = [] + @data = {} + end + + def add(key:, data:) + if @data.key?(key) + # FIXME: inefficient O(n) bahavior when bumped to newest. + idx = @keys.index key + @keys.delete_at idx + elsif @keys.length >= @size + oldest = @keys[0] + @keys.delete_at 0 + @data.delete oldest + end + @data[key] = data + @keys.push key + end + + def [](key) + @data[key] + end + end +end diff --git a/lib/rights_api/model_extensions.rb b/lib/rights_api/model_extensions.rb index 75c9a75..af0e138 100644 --- a/lib/rights_api/model_extensions.rb +++ b/lib/rights_api/model_extensions.rb @@ -19,9 +19,14 @@ def default_key end # For use in ORDER BY clause. - # @return [Sequel::SQL::QualifiedIdentifier] + # @return [Array] def default_order - query_for_field field: default_key + [query_for_field(field: default_key)] + end + + # Use the offset optimizer to minimize use of OFFSET? + def optimize? + false end # @param field [String, Symbol] diff --git a/lib/rights_api/models/rights_current.rb b/lib/rights_api/models/rights_current.rb index a6f78f1..c3a373e 100644 --- a/lib/rights_api/models/rights_current.rb +++ b/lib/rights_api/models/rights_current.rb @@ -30,10 +30,25 @@ def self.query_for_field(field:) super field: field end - # rights_current and rights_log should order by timestamp - # @return [Sequel::SQL::Expression] + # rights_current and rights_log should order by htid, timestamp + # @return [Array] def self.default_order - qualify field: :time + [ + qualify(field: :namespace), + qualify(field: :id), + qualify(field: :time) + ] + end + + def self.optimize? + true + end + + # @return [Array] + def self.optimizer_query(value:) + lhs = Sequel[default_order] + rhs = Sequel[[value[:namespace], value[:id], value[:time]]] + [lhs > rhs] end def to_h diff --git a/lib/rights_api/models/rights_log.rb b/lib/rights_api/models/rights_log.rb index 3330436..7a6a488 100644 --- a/lib/rights_api/models/rights_log.rb +++ b/lib/rights_api/models/rights_log.rb @@ -10,7 +10,7 @@ class RightsLog < Sequel::Model(:rights_log) many_to_one :attribute_obj, class: :"RightsAPI::Attribute", key: :attr many_to_one :reason_obj, class: :"RightsAPI::Reason", key: :reason many_to_one :source_obj, class: :"RightsAPI::Source", key: :source - set_primary_key [:namespace, :id] + set_primary_key [:namespace, :id, :time] # Maybe TOO eager. This makes us partially responsible for the fact that rights_current.source # has an embedded access_profile. @@ -32,9 +32,24 @@ def self.query_for_field(field:) end # rights_current and rights_log should order by timestamp - # @return [Sequel::SQL::Expression] + # @return [Array] def self.default_order - qualify field: :time + [ + qualify(field: :namespace), + qualify(field: :id), + qualify(field: :time) + ] + end + + def self.optimize? + true + end + + # @return [Array] + def self.optimizer_query(value:) + lhs = Sequel[default_order] + rhs = Sequel[[value[:namespace], value[:id], value[:time]]] + [lhs > rhs] end def to_h diff --git a/lib/rights_api/query.rb b/lib/rights_api/query.rb index 7ab8c52..0b451bd 100644 --- a/lib/rights_api/query.rb +++ b/lib/rights_api/query.rb @@ -1,16 +1,15 @@ # frozen_string_literal: true require "benchmark" -require "cgi" require_relative "error" +require_relative "query_optimizer" require_relative "query_parser" require_relative "result" -require_relative "services" module RightsAPI class Query - attr_reader :model, :params, :parser, :total, :dataset + attr_reader :model, :params, :parser, :total # @param model [Class] Sequel::Model subclass for the table being queried # @param params [Hash] CGI parameters submitted to the Sinatra frontend @@ -19,29 +18,33 @@ def initialize(model:, params: {}) @params = params @parser = QueryParser.new(model: model) @total = 0 - @dataset = nil end # @return [Result] def run + dataset = nil # This may raise QueryParserError parser.parse(params: params) + optimizer = QueryOptimizer.new(parser: parser) time_delta = Benchmark.realtime do - @dataset = model.base_dataset + dataset = model.base_dataset parser.where.each do |where| - @dataset = dataset.where(where) + dataset = dataset.where(where) end + dataset = dataset.order(*parser.order) # Save this here because offset and limit may alter the count. @total = dataset.count - @dataset = dataset.order(*parser.order) - .offset(parser.offset) - .limit(parser.limit) - .all + optimizer.where.each do |where| + dataset = dataset.where(where) + end + dataset = dataset.offset(optimizer.offset) if optimizer.offset.positive? + dataset = dataset.limit(parser.limit).all end result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta) dataset.each do |row| result.add! row: row.to_h end + optimizer.add_to_cache(dataset: dataset) result end end diff --git a/lib/rights_api/query_optimizer.rb b/lib/rights_api/query_optimizer.rb new file mode 100644 index 0000000..a35de41 --- /dev/null +++ b/lib/rights_api/query_optimizer.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require "digest/md5" +require "json/canonicalization" + +require_relative "services" + +module RightsAPI + class QueryOptimizer + attr_reader :parser, :offset, :where + + # A class that translates parsed query and optimizes it to + # avoid large OFFSETS into large tables when previous (cached) queries + # may enable the use of smaller or zero OFFSET. + # Doing this requires a model class that returns `true` for `#optimize?` + # and implements `.optimizer_query`, the results of which are used to derive + # additional `where` clause(s) that reduce or eliminate the OFFSET. + # + # This is only used with rights_current and rights_log. + # + # Provides a modified `#offset` that supersedes the query parser's version, + # and `#where` clause(s) that should be applied after the parser's `#where` + # that should replace or augment the desired OFFSET. + def initialize(parser:) + @parser = parser + @where = [] + @offset = parser.offset + optimize + end + + def add_to_cache(dataset:) + return if ENV["RIGHTS_API_DISABLE_OFFSET_OPTIMIZER"] + return if dataset.count.zero? + return unless dataset.last.class.optimize? + + data = { + offset: parser.offset, + limit: parser.limit, + last: dataset.last.to_h + } + Services[:cache].add(key: model_params_md5, data: data) + end + + private + + def optimize + return if ENV["RIGHTS_API_DISABLE_OFFSET_OPTIMIZER"] + return if offset.zero? + + cached = Services[:cache][model_params_md5] + return if cached.nil? + + # Next page: cached value located cached limit or more items earlier than desired offset + # We are counting across the cached page and starting after the last result. + if cached[:offset] <= @offset - cached[:limit] + @where = parser.model.optimizer_query(value: cached[:last]) + @offset -= cached[:limit] + cached[:offset] + Services[:logger].info "\e[31mCACHE HIT: NEW OFFSET #{@offset}\e[0m" + end + end + + # Canonicalization ensures that reordering query params in the URL won't affect + # the ability to get cache hits on subsequent queries. + # We don't want to be sensitive to OFFSET or LIMIT because we keep those as part + # of the cache value. + def model_params_md5 + canonical = parser.params.clone + canonical.delete "offset" + canonical.delete "limit" + Digest::MD5.hexdigest(parser.model.to_s + canonical.to_json_c14n) + end + end +end diff --git a/lib/rights_api/query_parser.rb b/lib/rights_api/query_parser.rb index 8a3cde4..6791a9a 100644 --- a/lib/rights_api/query_parser.rb +++ b/lib/rights_api/query_parser.rb @@ -36,7 +36,9 @@ def parse(params: {}) parse_parameter(key: key, values: values) end end - @order = [model.default_order] if @order.empty? + # Always tack on the default order even if it is redundant. + # The offset optimizer requires that there be an intrinsic order. + @order += model.default_order self end diff --git a/lib/rights_api/services.rb b/lib/rights_api/services.rb index db47c11..4b2ee92 100644 --- a/lib/rights_api/services.rb +++ b/lib/rights_api/services.rb @@ -3,10 +3,16 @@ require "canister" require "logger" +require_relative "cache" require_relative "database" module RightsAPI Services = Canister.new + + Services.register(:cache) do + Cache.new + end + Services.register(:database) do Database.new end diff --git a/spec/integration/rights_log_spec.rb b/spec/integration/rights_log_spec.rb index a58d663..c227e02 100644 --- a/spec/integration/rights_log_spec.rb +++ b/spec/integration/rights_log_spec.rb @@ -1,10 +1,15 @@ # frozen_string_literal: true +require "climate_control" +require "ffi-icu" require "rack/test" require "shared_examples" +require "pry" RSpec.describe "/rights_log" do include Rack::Test::Methods + # Use ICU collator to try to approximate Sequel's collation of underscore vs Ruby's + let(:collator) { ICU::Collation::Collator.new("en") } describe "/rights_log/HTID" do context "with a valid HTID" do @@ -16,16 +21,33 @@ before(:each) { get(rights_api_endpoint + "rights_log/bogus.no_such_id") } it_behaves_like "empty response" end + end - context "with no HTID" do - before(:each) { get(rights_api_endpoint + "rights_log") } - it_behaves_like "nonempty rights response" + context "with no HTID" do + before(:each) { get(rights_api_endpoint + "rights_log") } + it_behaves_like "nonempty rights response" + + it "obeys default sort order `namespace, id, time`" do + response = parse_json(last_response.body) + sorted = response[:data].sort do |a, b| + a[:namespace] <=> b[:namespace] || + collator.compare(a[:id], b[:id]) || + a[:time] <=> b[:time] + end + expect(response[:data]).to eq(sorted) + end + end + + context "with an OFFSET" do + before(:each) { get(rights_api_endpoint + "rights_log?limit=2") } - it "obeys default sort order `time ASC`" do - response = parse_json(last_response.body) - sorted = response[:data].sort_by { |a| a[:time] } - expect(response[:data]).to eq(sorted) + it "produces the same results with or without optimizer" do + optimized = parse_json get(rights_api_endpoint + "rights_log?limit=2&offset=2").body + unoptimized = nil + ClimateControl.modify(RIGHTS_API_DISABLE_OFFSET_OPTIMIZER: "1") do + unoptimized = parse_json get(rights_api_endpoint + "rights_log?limit=2&offset=2").body end + expect(optimized[:data]).to eq(unoptimized[:data]) end end end diff --git a/spec/integration/rights_spec.rb b/spec/integration/rights_spec.rb index 3097e4c..6439ecb 100644 --- a/spec/integration/rights_spec.rb +++ b/spec/integration/rights_spec.rb @@ -1,29 +1,51 @@ # frozen_string_literal: true +require "ffi-icu" require "rack/test" require "shared_examples" RSpec.describe "/rights" do include Rack::Test::Methods + # Use ICU collator to try to approximate Sequel's collation of underscore vs Ruby's + let(:collator) { ICU::Collation::Collator.new("en") } - context "with a valid HTID" do - before(:each) { get(rights_api_endpoint + "rights/test.pd_google") } - it_behaves_like "nonempty rights response" - end + describe "/rights_log/HTID" do + context "with a valid HTID" do + before(:each) { get(rights_api_endpoint + "rights/test.pd_google") } + it_behaves_like "nonempty rights response" + end - context "with an invalid HTID" do - before(:each) { get(rights_api_endpoint + "rights/bogus.no_such_id") } - it_behaves_like "empty response" + context "with an invalid HTID" do + before(:each) { get(rights_api_endpoint + "rights/bogus.no_such_id") } + it_behaves_like "empty response" + end end context "with no HTID" do before(:each) { get(rights_api_endpoint + "rights") } it_behaves_like "nonempty rights response" - it "obeys default sort order `time ASC`" do + it "obeys default sort order `namespace, id, time`" do response = parse_json(last_response.body) - sorted = response[:data].sort_by { |a| a[:time] } + sorted = response[:data].sort do |a, b| + a[:namespace] <=> b[:namespace] || + collator.compare(a[:id], b[:id]) || + a[:time] <=> b[:time] + end expect(response[:data]).to eq(sorted) end end + + context "with an OFFSET" do + before(:each) { get(rights_api_endpoint + "rights?limit=2") } + + it "produces the same results with or without optimizer" do + optimized = parse_json get(rights_api_endpoint + "rights?limit=2&offset=2").body + unoptimized = nil + ClimateControl.modify(RIGHTS_API_DISABLE_OFFSET_OPTIMIZER: "1") do + unoptimized = parse_json get(rights_api_endpoint + "rights?limit=2&offset=2").body + end + expect(optimized[:data]).to eq(unoptimized[:data]) + end + end end diff --git a/spec/unit/cache_spec.rb b/spec/unit/cache_spec.rb new file mode 100644 index 0000000..d57f679 --- /dev/null +++ b/spec/unit/cache_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module RightsAPI + RSpec.describe(Cache) do + let(:cache) { described_class.new(size: 2) } + + describe "#add" do + it "adds data" do + cache.add(key: "key", data: "data") + expect(cache["key"]).to eq "data" + end + + it "replaces existing data" do + cache.add(key: "key", data: "data") + cache.add(key: "key", data: "different data") + expect(cache["key"]).to eq "different data" + end + + it "replaces oldest data" do + cache.add(key: "key1", data: "data1") + cache.add(key: "key2", data: "data2") + cache.add(key: "key3", data: "data3") + expect(cache["key1"]).to be_nil + end + end + end +end diff --git a/spec/unit/models/access_profile_spec.rb b/spec/unit/models/access_profile_spec.rb index 9d2524a..95c465c 100644 --- a/spec/unit/models/access_profile_spec.rb +++ b/spec/unit/models/access_profile_spec.rb @@ -11,8 +11,8 @@ module RightsAPI end describe "#default_order" do - it "returns SQL expression" do - expect(described_class.default_order).to be_a(Sequel::SQL::QualifiedIdentifier) + it "returns Array" do + expect(described_class.default_order).to be_a(Array) end end diff --git a/spec/unit/models/access_statement_map_spec.rb b/spec/unit/models/access_statement_map_spec.rb index 83051ae..a7f9ce3 100644 --- a/spec/unit/models/access_statement_map_spec.rb +++ b/spec/unit/models/access_statement_map_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns SQL expression" do - expect(described_class.default_order).to be_a(Sequel::SQL::Expression) + it "returns Array" do + expect(described_class.default_order).to be_a(Array) end end diff --git a/spec/unit/models/access_statement_spec.rb b/spec/unit/models/access_statement_spec.rb index 741f167..bb4df2a 100644 --- a/spec/unit/models/access_statement_spec.rb +++ b/spec/unit/models/access_statement_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns SQL expression" do - expect(described_class.default_order).to be_a(Sequel::SQL::QualifiedIdentifier) + it "returns Array" do + expect(described_class.default_order).to be_a(Array) end end diff --git a/spec/unit/models/attribute_spec.rb b/spec/unit/models/attribute_spec.rb index ec16dc5..b611bbe 100644 --- a/spec/unit/models/attribute_spec.rb +++ b/spec/unit/models/attribute_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns SQL expression" do - expect(described_class.default_order).to be_a(Sequel::SQL::QualifiedIdentifier) + it "returns Array" do + expect(described_class.default_order).to be_a(Array) end end diff --git a/spec/unit/models/reason_spec.rb b/spec/unit/models/reason_spec.rb index b4726a1..ba021e5 100644 --- a/spec/unit/models/reason_spec.rb +++ b/spec/unit/models/reason_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns SQL expression" do - expect(described_class.default_order).to be_a(Sequel::SQL::QualifiedIdentifier) + it "returns Array" do + expect(described_class.default_order).to be_a(Array) end end diff --git a/spec/unit/models/rights_current_spec.rb b/spec/unit/models/rights_current_spec.rb index c9a43a5..42b3d82 100644 --- a/spec/unit/models/rights_current_spec.rb +++ b/spec/unit/models/rights_current_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns timestamp" do - expect(described_class.default_order).to be_a(Sequel::SQL::Expression) + it "returns Array with timestamp" do + expect(described_class.default_order.first).to be_a(Sequel::SQL::Expression) end end diff --git a/spec/unit/models/rights_log_spec.rb b/spec/unit/models/rights_log_spec.rb index 416233e..24f3220 100644 --- a/spec/unit/models/rights_log_spec.rb +++ b/spec/unit/models/rights_log_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns timestamp" do - expect(described_class.default_order).to be_a(Sequel::SQL::Expression) + it "returns Array with timestamp" do + expect(described_class.default_order.first).to be_a(Sequel::SQL::Expression) end end diff --git a/spec/unit/models/source_spec.rb b/spec/unit/models/source_spec.rb index 4746655..ad6d3ba 100644 --- a/spec/unit/models/source_spec.rb +++ b/spec/unit/models/source_spec.rb @@ -17,8 +17,8 @@ module RightsAPI end describe "#default_order" do - it "returns SQL expression" do - expect(described_class.default_order).to be_a(Sequel::SQL::QualifiedIdentifier) + it "returns Array" do + expect(described_class.default_order).to be_a(Array) end end diff --git a/spec/unit/query_optimizer_spec.rb b/spec/unit/query_optimizer_spec.rb new file mode 100644 index 0000000..f4279e7 --- /dev/null +++ b/spec/unit/query_optimizer_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module RightsAPI + RSpec.describe(QueryOptimizer) do + let(:query_parser) { QueryParser.new(model: RightsCurrent) } + let(:query_optimizer) { described_class.new(parser: query_parser) } + + describe "#initialize" do + it "creates RightsAPI::QueryOptimizer instance" do + expect(query_optimizer).to be_a(RightsAPI::QueryOptimizer) + end + end + end +end diff --git a/spec/unit/query_parser_spec.rb b/spec/unit/query_parser_spec.rb index 3c03cd2..185cdef 100644 --- a/spec/unit/query_parser_spec.rb +++ b/spec/unit/query_parser_spec.rb @@ -1,11 +1,8 @@ # frozen_string_literal: true -require "sequel" - module RightsAPI RSpec.describe(QueryParser) do let(:query_parser) { described_class.new(model: Attribute) } - # let(:id_query) { described_class.new(params: "id=some+id", table_name: "rights") } describe ".new" do it "creates a #{described_class}" do From 447a612f5ff6b421f937110744a363da3b549bb6 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 14:59:06 -0400 Subject: [PATCH 02/18] Use lru_redux cache instead of homebrew --- Gemfile | 1 + Gemfile.lock | 2 ++ lib/rights_api.rb | 1 - lib/rights_api/cache.rb | 32 ------------------------------- lib/rights_api/query_optimizer.rb | 2 +- lib/rights_api/services.rb | 6 ++++-- spec/unit/cache_spec.rb | 27 -------------------------- 7 files changed, 8 insertions(+), 63 deletions(-) delete mode 100644 lib/rights_api/cache.rb delete mode 100644 spec/unit/cache_spec.rb diff --git a/Gemfile b/Gemfile index 35863ad..18760d2 100644 --- a/Gemfile +++ b/Gemfile @@ -9,6 +9,7 @@ gem "puma" gem "sequel" gem "sinatra" gem "sinatra-contrib" +gem "lru_redux" group :development, :test do gem "ffi-icu" diff --git a/Gemfile.lock b/Gemfile.lock index e3ba948..d6ec587 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -16,6 +16,7 @@ GEM json-canonicalization (1.0.0) language_server-protocol (3.17.0.3) lint_roller (1.1.0) + lru_redux (1.1.0) method_source (1.0.0) multi_json (1.15.0) mustermann (3.0.0) @@ -117,6 +118,7 @@ DEPENDENCIES ffi-icu json json-canonicalization + lru_redux mysql2 pry puma diff --git a/lib/rights_api.rb b/lib/rights_api.rb index 839eb7e..62d3b00 100644 --- a/lib/rights_api.rb +++ b/lib/rights_api.rb @@ -4,7 +4,6 @@ module RightsAPI end require "rights_api/app" -require "rights_api/cache" require "rights_api/database" require "rights_api/query" require "rights_api/query_optimizer" diff --git a/lib/rights_api/cache.rb b/lib/rights_api/cache.rb deleted file mode 100644 index 1187afd..0000000 --- a/lib/rights_api/cache.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -module RightsAPI - class Cache - DEFAULT_CACHE_SIZE = 1000 - - def initialize(size: DEFAULT_CACHE_SIZE) - @size = size - # Model name + canonicalized params MD5 in order of oldest to newest - @keys = [] - @data = {} - end - - def add(key:, data:) - if @data.key?(key) - # FIXME: inefficient O(n) bahavior when bumped to newest. - idx = @keys.index key - @keys.delete_at idx - elsif @keys.length >= @size - oldest = @keys[0] - @keys.delete_at 0 - @data.delete oldest - end - @data[key] = data - @keys.push key - end - - def [](key) - @data[key] - end - end -end diff --git a/lib/rights_api/query_optimizer.rb b/lib/rights_api/query_optimizer.rb index a35de41..59a978e 100644 --- a/lib/rights_api/query_optimizer.rb +++ b/lib/rights_api/query_optimizer.rb @@ -38,7 +38,7 @@ def add_to_cache(dataset:) limit: parser.limit, last: dataset.last.to_h } - Services[:cache].add(key: model_params_md5, data: data) + Services[:cache][model_params_md5] = data end private diff --git a/lib/rights_api/services.rb b/lib/rights_api/services.rb index 4b2ee92..4ea3376 100644 --- a/lib/rights_api/services.rb +++ b/lib/rights_api/services.rb @@ -2,15 +2,17 @@ require "canister" require "logger" +require "lru_redux" -require_relative "cache" require_relative "database" +DEFAULT_CACHE_SIZE = 1000 + module RightsAPI Services = Canister.new Services.register(:cache) do - Cache.new + LruRedux::Cache.new(DEFAULT_CACHE_SIZE) end Services.register(:database) do diff --git a/spec/unit/cache_spec.rb b/spec/unit/cache_spec.rb deleted file mode 100644 index d57f679..0000000 --- a/spec/unit/cache_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module RightsAPI - RSpec.describe(Cache) do - let(:cache) { described_class.new(size: 2) } - - describe "#add" do - it "adds data" do - cache.add(key: "key", data: "data") - expect(cache["key"]).to eq "data" - end - - it "replaces existing data" do - cache.add(key: "key", data: "data") - cache.add(key: "key", data: "different data") - expect(cache["key"]).to eq "different data" - end - - it "replaces oldest data" do - cache.add(key: "key1", data: "data1") - cache.add(key: "key2", data: "data2") - cache.add(key: "key3", data: "data3") - expect(cache["key1"]).to be_nil - end - end - end -end From 5962ff7971d816f3b64724939ce4d98b52a8dff3 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 15:00:04 -0400 Subject: [PATCH 03/18] Add QueryOptimizer#add_to_cache spec --- spec/unit/query_optimizer_spec.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/spec/unit/query_optimizer_spec.rb b/spec/unit/query_optimizer_spec.rb index f4279e7..56d7aea 100644 --- a/spec/unit/query_optimizer_spec.rb +++ b/spec/unit/query_optimizer_spec.rb @@ -2,12 +2,22 @@ module RightsAPI RSpec.describe(QueryOptimizer) do - let(:query_parser) { QueryParser.new(model: RightsCurrent) } + let(:query_parser) { QueryParser.new(model: RightsCurrent).parse } let(:query_optimizer) { described_class.new(parser: query_parser) } describe "#initialize" do it "creates RightsAPI::QueryOptimizer instance" do expect(query_optimizer).to be_a(RightsAPI::QueryOptimizer) + expect(query_optimizer.parser).to be_a(RightsAPI::QueryParser) + expect(query_optimizer.offset).to be_a(Integer) + expect(query_optimizer.where).to be_an(Array) + end + end + + describe "#add_to_cache" do + it "adds an entry to the cache" do + query_optimizer.add_to_cache(dataset: RightsCurrent.base_dataset.all) + expect(Services[:cache].count).to be > 0 end end end From 328f8b62d1a3f55c8b35e070a7b80d05b7d45f68 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 15:02:19 -0400 Subject: [PATCH 04/18] Fix ''undefined method 'parse' for CGI:Class'' error --- lib/rights_api/app.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/rights_api/app.rb b/lib/rights_api/app.rb index 547e20e..d987c22 100644 --- a/lib/rights_api/app.rb +++ b/lib/rights_api/app.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require "cgi" require "sinatra" require "sinatra/json" require "sinatra/reloader" if development? From bfbb08a88575db92e3d13b4b34eaf84185561e5c Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 15:03:15 -0400 Subject: [PATCH 05/18] Include cached true/false in output JSON --- lib/rights_api/query.rb | 5 ++++- lib/rights_api/result.rb | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/rights_api/query.rb b/lib/rights_api/query.rb index 0b451bd..6493557 100644 --- a/lib/rights_api/query.rb +++ b/lib/rights_api/query.rb @@ -23,6 +23,7 @@ def initialize(model:, params: {}) # @return [Result] def run dataset = nil + cached = false # This may raise QueryParserError parser.parse(params: params) optimizer = QueryOptimizer.new(parser: parser) @@ -36,11 +37,13 @@ def run @total = dataset.count optimizer.where.each do |where| dataset = dataset.where(where) + cached = true end dataset = dataset.offset(optimizer.offset) if optimizer.offset.positive? dataset = dataset.limit(parser.limit).all end - result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta) + result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta, + cached: cached) dataset.each do |row| result.add! row: row.to_h end diff --git a/lib/rights_api/result.rb b/lib/rights_api/result.rb index e2d1303..0e304e9 100644 --- a/lib/rights_api/result.rb +++ b/lib/rights_api/result.rb @@ -9,10 +9,11 @@ class Result # @param offset [Integer] The offset=x URL parameter. # @param total [Integer] The total number of results, regardless of paging. - def initialize(offset: 0, total: 0, milliseconds: 0.0) + def initialize(offset: 0, total: 0, milliseconds: 0.0, cached: false) @offset = offset @total = total @milliseconds = milliseconds + @cached = cached @start = 0 @end = 0 @data = [] @@ -39,6 +40,7 @@ def to_h "start" => @start, "end" => @end, "milliseconds" => @milliseconds, + "cached" => @cached, "data" => @data } finalize h From af2a36f498744db776c42322b29ebd2d271a528c Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 15:04:39 -0400 Subject: [PATCH 06/18] Add 'cached' to list of required result fields --- spec/unit/result_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/result_spec.rb b/spec/unit/result_spec.rb index b24fb18..4424b63 100644 --- a/spec/unit/result_spec.rb +++ b/spec/unit/result_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module RightsAPI - REQUIRED_HASH_KEYS = %w[total start end milliseconds data] + REQUIRED_HASH_KEYS = %w[total start end milliseconds cached data] RSpec.describe(Result) do let(:result) { described_class.new } let(:test_row) { {key1: "value1", key2: "value2"} } From 21153294dd65b01a52f7dee2debcc5f95b9e3251 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 20:34:00 -0400 Subject: [PATCH 07/18] Remove unneeded 'require pry' --- spec/integration/rights_log_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/integration/rights_log_spec.rb b/spec/integration/rights_log_spec.rb index c227e02..26feec2 100644 --- a/spec/integration/rights_log_spec.rb +++ b/spec/integration/rights_log_spec.rb @@ -4,7 +4,6 @@ require "ffi-icu" require "rack/test" require "shared_examples" -require "pry" RSpec.describe "/rights_log" do include Rack::Test::Methods From 279295cbe0e74969ffb5a6626f45785b3f58c845 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Fri, 12 Apr 2024 20:34:25 -0400 Subject: [PATCH 08/18] Add build workflow --- .github/workflows/build.yml | 51 +++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 .github/workflows/build.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml new file mode 100644 index 0000000..9a72b66 --- /dev/null +++ b/.github/workflows/build.yml @@ -0,0 +1,51 @@ +name: Build + +on: + workflow_run: + workflows: ['Run Tests'] + branches: ['main'] + types: [completed] + + workflow_dispatch: + inputs: + img_tag: + description: Docker Image Tag + ref: + description: Revision or Branch to build + default: main + push_latest: + description: Set True if the build is for the latest version + type: boolean + required: false + default: false + platforms: + description: Platforms to build for + type: choice + default: linux/amd64,linux/arm64 + options: + - linux/amd64,linux/arm64 + - linux/amd64 + - linux/arm64 + rebuild: + description: Rebuild this image? + type: boolean + default: false + +jobs: + build-image: + runs-on: ubuntu-latest + permissions: + contents: read + packages: write + + steps: + - name: Build Image + uses: hathitrust/github_actions/build@v1.4.0 + with: + image: ghcr.io/${{ github.repository }}-unstable + dockerfile: Dockerfile + img_tag: ${{ inputs.img_tag }} + tag: ${{ inputs.ref }} + push_latest: ${{ inputs.push_latest}} + registry_token: ${{ github.token }} + rebuild: ${{ inputs.rebuild }} From 95d5c0e5887eb77c970452fe6b21a39d7ced8c60 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Mon, 15 Apr 2024 14:34:43 -0400 Subject: [PATCH 09/18] Remove Build workflow to be added in a separate branch. --- .github/workflows/build.yml | 51 ------------------------------------- 1 file changed, 51 deletions(-) delete mode 100644 .github/workflows/build.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml deleted file mode 100644 index 9a72b66..0000000 --- a/.github/workflows/build.yml +++ /dev/null @@ -1,51 +0,0 @@ -name: Build - -on: - workflow_run: - workflows: ['Run Tests'] - branches: ['main'] - types: [completed] - - workflow_dispatch: - inputs: - img_tag: - description: Docker Image Tag - ref: - description: Revision or Branch to build - default: main - push_latest: - description: Set True if the build is for the latest version - type: boolean - required: false - default: false - platforms: - description: Platforms to build for - type: choice - default: linux/amd64,linux/arm64 - options: - - linux/amd64,linux/arm64 - - linux/amd64 - - linux/arm64 - rebuild: - description: Rebuild this image? - type: boolean - default: false - -jobs: - build-image: - runs-on: ubuntu-latest - permissions: - contents: read - packages: write - - steps: - - name: Build Image - uses: hathitrust/github_actions/build@v1.4.0 - with: - image: ghcr.io/${{ github.repository }}-unstable - dockerfile: Dockerfile - img_tag: ${{ inputs.img_tag }} - tag: ${{ inputs.ref }} - push_latest: ${{ inputs.push_latest}} - registry_token: ${{ github.token }} - rebuild: ${{ inputs.rebuild }} From 0f26c92105ac692f094dd073b33523e389a94447 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Tue, 23 Apr 2024 17:03:22 -0400 Subject: [PATCH 10/18] Cursor implementation; allow order= parameters --- Gemfile | 3 - Gemfile.lock | 8 -- lib/rights_api.rb | 3 +- lib/rights_api/cursor.rb | 97 +++++++++++++++++++ lib/rights_api/model_extensions.rb | 11 +-- lib/rights_api/models/access_statement_map.rb | 7 ++ lib/rights_api/models/rights_current.rb | 19 +--- lib/rights_api/models/rights_log.rb | 21 +--- lib/rights_api/order.rb | 21 ++++ lib/rights_api/query.rb | 31 +++--- lib/rights_api/query_optimizer.rb | 73 -------------- lib/rights_api/query_parser.rb | 49 ++++++++-- lib/rights_api/result.rb | 17 +++- lib/rights_api/services.rb | 5 - spec/integration/rights_log_spec.rb | 17 +--- spec/integration/rights_spec.rb | 17 +--- spec/integration/support_table_spec.rb | 36 +++---- spec/spec_helper.rb | 6 ++ spec/unit/cursor_spec.rb | 53 ++++++++++ spec/unit/database_spec.rb | 70 ++++++------- spec/unit/models/rights_current_spec.rb | 7 +- spec/unit/models/rights_log_spec.rb | 7 +- spec/unit/order_spec.rb | 34 +++++++ spec/unit/query_optimizer_spec.rb | 24 ----- spec/unit/query_parser_spec.rb | 18 ++-- spec/unit/result_spec.rb | 28 +++++- 26 files changed, 410 insertions(+), 272 deletions(-) create mode 100644 lib/rights_api/cursor.rb create mode 100644 lib/rights_api/order.rb delete mode 100644 lib/rights_api/query_optimizer.rb create mode 100644 spec/unit/cursor_spec.rb create mode 100644 spec/unit/order_spec.rb delete mode 100644 spec/unit/query_optimizer_spec.rb diff --git a/Gemfile b/Gemfile index 18760d2..cee4b83 100644 --- a/Gemfile +++ b/Gemfile @@ -3,16 +3,13 @@ source "https://rubygems.org" gem "canister" gem "climate_control" gem "json" -gem "json-canonicalization" gem "mysql2" gem "puma" gem "sequel" gem "sinatra" gem "sinatra-contrib" -gem "lru_redux" group :development, :test do - gem "ffi-icu" gem "pry" gem "rack-test" gem "rspec" diff --git a/Gemfile.lock b/Gemfile.lock index d6ec587..501e8df 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -9,14 +9,9 @@ GEM coderay (1.1.3) diff-lcs (1.5.0) docile (1.4.0) - ffi (1.16.3) - ffi-icu (0.5.3) - ffi (~> 1.0, >= 1.0.9) json (2.6.3) - json-canonicalization (1.0.0) language_server-protocol (3.17.0.3) lint_roller (1.1.0) - lru_redux (1.1.0) method_source (1.0.0) multi_json (1.15.0) mustermann (3.0.0) @@ -115,10 +110,7 @@ PLATFORMS DEPENDENCIES canister climate_control - ffi-icu json - json-canonicalization - lru_redux mysql2 pry puma diff --git a/lib/rights_api.rb b/lib/rights_api.rb index 62d3b00..435c7a7 100644 --- a/lib/rights_api.rb +++ b/lib/rights_api.rb @@ -4,9 +4,10 @@ module RightsAPI end require "rights_api/app" +require "rights_api/cursor" require "rights_api/database" +require "rights_api/order" require "rights_api/query" -require "rights_api/query_optimizer" require "rights_api/query_parser" require "rights_api/result" require "rights_api/result/error_result" diff --git a/lib/rights_api/cursor.rb b/lib/rights_api/cursor.rb new file mode 100644 index 0000000..2291d7b --- /dev/null +++ b/lib/rights_api/cursor.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require "base64" +require "json" +require "sequel" + +# Given a list of sort fields -- Array of symbols +# and a list of field -> value mappings (the last result) +# create a string that can be decoded into +# a WHERE list + +# Tries to store just enough information so the next query can +# pick up where the current one left off. + +# Relies on the search parameters being unchanged between queries, +# if they do then the results are undefined. + +# To create the semi-opaque cursor value for the result set, +# creates an array containing the current offset and one value for each +# sort parameter (explicit or default). + +# This may be a module and not a class +module RightsAPI + class Cursor + OFFSET_KEY = "off" + LAST_ROW_KEY = "last" + attr_reader :values, :offset + + # @param cursor_string [String] the URL parameter to decode + # @return [Array] of the form [offset, "val1", "val2" ...] + def self.decode(cursor_string) + JSON.parse(Base64.urlsafe_decode64(cursor_string)) + end + + def self.encode(arg) + Base64.urlsafe_encode64(JSON.generate(arg)) + end + + def initialize(cursor_string: nil) + @offset = 0 + @values = [] + if cursor_string + @values = self.class.decode cursor_string + @offset = @values.shift + end + end + + # @param order [Array] + # @param rows [Sequel::Dataset] + def encode(order:, rows:) + data = [offset + rows.count] + row = rows.last + order.each do |ord| + data << row[ord.column] + end + Services[:logger].info "to encode: #{data}" + self.class.encode data + end + + # Generate zero or one WHERE clauses that will generate a pseudo-OFFSET + # based on ORDER BY parameters. + # FIXME: should cursor be a required parameter? (require "*" in order to get back a + # cursor value?) + # TODO: Can we shorten long cursors by calculating longest common initial substring + # based on last value in current window and first value of next? + # ORDER BY a, b, c TRANSLATES TO + # WHERE (a > 1) + # OR (a = 1 AND b > 2) + # OR (a = 1 AND b = 2 AND c > 3) + def where(model:, order:) + return [] if values.empty? + + # Create one OR clause for each ORDER. + # Each OR clause is a series of AND clauses. + # The last element of each AND clause is < or >, the others are = + # The first AND clause has only the first ORDER parameter. + # Each subsequent one adds one ORDER PARAMETER. + or_clause = [] + order.count.times do |order_index| + # Take a slice of ORDER of size order_index + 1 + and_clause = order[0, order_index + 1].each_with_index.map do |ord, i| + # in which each element is a "col op val" string + # and the last is an inequality + op = if i == order_index + ord.asc? ? ">" : "<" + else + "=" + end + "#{model.table_name}.#{ord.column}#{op}'#{values[i]}'" + end + or_clause << "(" + and_clause.join(" AND ") + ")" + end + res = Sequel.lit or_clause.join(" OR ") + [res] + end + end +end diff --git a/lib/rights_api/model_extensions.rb b/lib/rights_api/model_extensions.rb index af0e138..136e12e 100644 --- a/lib/rights_api/model_extensions.rb +++ b/lib/rights_api/model_extensions.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require_relative "order" + module RightsAPI module ModelExtensions # Overridden by classes that want to do some kind of #eager or #eager_graph @@ -19,14 +21,9 @@ def default_key end # For use in ORDER BY clause. - # @return [Array] + # @return [Array] def default_order - [query_for_field(field: default_key)] - end - - # Use the offset optimizer to minimize use of OFFSET? - def optimize? - false + [Order.new(column: default_key)] end # @param field [String, Symbol] diff --git a/lib/rights_api/models/access_statement_map.rb b/lib/rights_api/models/access_statement_map.rb index f2cd142..9d127df 100644 --- a/lib/rights_api/models/access_statement_map.rb +++ b/lib/rights_api/models/access_statement_map.rb @@ -9,6 +9,13 @@ def self.default_key :attr_access_id end + def self.default_order + [ + Order.new(column: :a_attr), + Order.new(column: :a_access_profile) + ] + end + # @param [String, Symbol] field # @return [Sequel::SQL::Expression] def self.query_for_field(field:) diff --git a/lib/rights_api/models/rights_current.rb b/lib/rights_api/models/rights_current.rb index c3a373e..11ef80b 100644 --- a/lib/rights_api/models/rights_current.rb +++ b/lib/rights_api/models/rights_current.rb @@ -31,26 +31,15 @@ def self.query_for_field(field:) end # rights_current and rights_log should order by htid, timestamp - # @return [Array] + # @return [Array] def self.default_order [ - qualify(field: :namespace), - qualify(field: :id), - qualify(field: :time) + Order.new(column: :namespace), + Order.new(column: :id), + Order.new(column: :time) ] end - def self.optimize? - true - end - - # @return [Array] - def self.optimizer_query(value:) - lhs = Sequel[default_order] - rhs = Sequel[[value[:namespace], value[:id], value[:time]]] - [lhs > rhs] - end - def to_h { namespace: namespace, diff --git a/lib/rights_api/models/rights_log.rb b/lib/rights_api/models/rights_log.rb index 7a6a488..5c3c0ac 100644 --- a/lib/rights_api/models/rights_log.rb +++ b/lib/rights_api/models/rights_log.rb @@ -31,27 +31,16 @@ def self.query_for_field(field:) super field: field end - # rights_current and rights_log should order by timestamp - # @return [Array] + # rights_current and rights_log should order by htid, timestamp + # @return [Array] def self.default_order [ - qualify(field: :namespace), - qualify(field: :id), - qualify(field: :time) + Order.new(column: :namespace), + Order.new(column: :id), + Order.new(column: :time) ] end - def self.optimize? - true - end - - # @return [Array] - def self.optimizer_query(value:) - lhs = Sequel[default_order] - rhs = Sequel[[value[:namespace], value[:id], value[:time]]] - [lhs > rhs] - end - def to_h { namespace: namespace, diff --git a/lib/rights_api/order.rb b/lib/rights_api/order.rb new file mode 100644 index 0000000..9ad14af --- /dev/null +++ b/lib/rights_api/order.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# A class that encapsulates the field and ASC/DESC properties of a +# single ORDER BY argument. + +module RightsAPI + class Order + attr_reader :column + # @param column [Symbol] the field to ORDER BY + # @param asc [Boolean] true if ASC, false if DESC + def initialize(column:, asc: true) + @column = column + @asc = asc + end + + # @return [Boolean] is the order direction ASC? + def asc? + @asc + end + end +end diff --git a/lib/rights_api/query.rb b/lib/rights_api/query.rb index 6493557..ea87ac8 100644 --- a/lib/rights_api/query.rb +++ b/lib/rights_api/query.rb @@ -2,8 +2,8 @@ require "benchmark" +require_relative "cursor" require_relative "error" -require_relative "query_optimizer" require_relative "query_parser" require_relative "result" @@ -23,32 +23,37 @@ def initialize(model:, params: {}) # @return [Result] def run dataset = nil - cached = false # This may raise QueryParserError parser.parse(params: params) - optimizer = QueryOptimizer.new(parser: parser) time_delta = Benchmark.realtime do dataset = model.base_dataset parser.where.each do |where| dataset = dataset.where(where) end - dataset = dataset.order(*parser.order) - # Save this here because offset and limit may alter the count. + dataset = dataset.order(*(parser.order.map { |order| order_to_sequel(order: order, model: model) })) + # Save this here because limit may alter the count. @total = dataset.count - optimizer.where.each do |where| - dataset = dataset.where(where) - cached = true - end - dataset = dataset.offset(optimizer.offset) if optimizer.offset.positive? dataset = dataset.limit(parser.limit).all end - result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta, - cached: cached) + result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta) dataset.each do |row| result.add! row: row.to_h end - optimizer.add_to_cache(dataset: dataset) + if result.more? + cursor = parser.cursor.encode(order: parser.order, rows: dataset) + result.cursor = cursor + end result end + + private + + def order_to_sequel(order:, model:) + if order.asc? + Sequel.asc(model.qualify(field: order.column)) + else + Sequel.desc(model.qualify(field: order.column)) + end + end end end diff --git a/lib/rights_api/query_optimizer.rb b/lib/rights_api/query_optimizer.rb deleted file mode 100644 index 59a978e..0000000 --- a/lib/rights_api/query_optimizer.rb +++ /dev/null @@ -1,73 +0,0 @@ -# frozen_string_literal: true - -require "digest/md5" -require "json/canonicalization" - -require_relative "services" - -module RightsAPI - class QueryOptimizer - attr_reader :parser, :offset, :where - - # A class that translates parsed query and optimizes it to - # avoid large OFFSETS into large tables when previous (cached) queries - # may enable the use of smaller or zero OFFSET. - # Doing this requires a model class that returns `true` for `#optimize?` - # and implements `.optimizer_query`, the results of which are used to derive - # additional `where` clause(s) that reduce or eliminate the OFFSET. - # - # This is only used with rights_current and rights_log. - # - # Provides a modified `#offset` that supersedes the query parser's version, - # and `#where` clause(s) that should be applied after the parser's `#where` - # that should replace or augment the desired OFFSET. - def initialize(parser:) - @parser = parser - @where = [] - @offset = parser.offset - optimize - end - - def add_to_cache(dataset:) - return if ENV["RIGHTS_API_DISABLE_OFFSET_OPTIMIZER"] - return if dataset.count.zero? - return unless dataset.last.class.optimize? - - data = { - offset: parser.offset, - limit: parser.limit, - last: dataset.last.to_h - } - Services[:cache][model_params_md5] = data - end - - private - - def optimize - return if ENV["RIGHTS_API_DISABLE_OFFSET_OPTIMIZER"] - return if offset.zero? - - cached = Services[:cache][model_params_md5] - return if cached.nil? - - # Next page: cached value located cached limit or more items earlier than desired offset - # We are counting across the cached page and starting after the last result. - if cached[:offset] <= @offset - cached[:limit] - @where = parser.model.optimizer_query(value: cached[:last]) - @offset -= cached[:limit] + cached[:offset] - Services[:logger].info "\e[31mCACHE HIT: NEW OFFSET #{@offset}\e[0m" - end - end - - # Canonicalization ensures that reordering query params in the URL won't affect - # the ability to get cache hits on subsequent queries. - # We don't want to be sensitive to OFFSET or LIMIT because we keep those as part - # of the cache value. - def model_params_md5 - canonical = parser.params.clone - canonical.delete "offset" - canonical.delete "limit" - Digest::MD5.hexdigest(parser.model.to_s + canonical.to_json_c14n) - end - end -end diff --git a/lib/rights_api/query_parser.rb b/lib/rights_api/query_parser.rb index 6791a9a..da4d059 100644 --- a/lib/rights_api/query_parser.rb +++ b/lib/rights_api/query_parser.rb @@ -2,7 +2,9 @@ require "sequel" +require_relative "cursor" require_relative "error" +require_relative "order" # Processes the Hash of URL parameters passed to the API into an # Array of WHERE constraints, as well as LIMIT, and OFFSET values. @@ -11,16 +13,15 @@ module RightsAPI class QueryParser DEFAULT_LIMIT = 1000 - DEFAULT_OFFSET = 0 - attr_reader :params, :model, :where, :order, :offset, :limit + attr_reader :params, :model, :order, :limit # @param model [Class] Sequel::Model subclass for the table being queried def initialize(model:) @model = model @where = [] @order = [] + @cursor = nil @limit = DEFAULT_LIMIT - @offset = DEFAULT_OFFSET end def parse(params: {}) @@ -28,20 +29,34 @@ def parse(params: {}) params.each do |key, values| key = key.to_sym case key - when :offset - parse_offset(values: values) + when :cursor + parse_cursor(values: values) when :limit parse_limit(values: values) + when :order + parse_order(values: values) else parse_parameter(key: key, values: values) end end # Always tack on the default order even if it is redundant. - # The offset optimizer requires that there be an intrinsic order. + # The cursor implementation requires that there be an intrinsic order. @order += model.default_order self end + def where + @where + cursor.where(model: model, order: order) + end + + def offset + cursor.offset + end + + def cursor + @cursor || Cursor.new + end + private # Parses a general search parameter and appends the resulting Sequel @@ -57,9 +72,25 @@ def parse_parameter(key:, values:) end end - # Extract a single integer that can be passed to dataset.offset. - def parse_offset(values:) - @offset = parse_int_value(values: values, type: "OFFSET") + # Raturn Array that can be passed as params to .order + def parse_order(values:) + values.each do |value| + column, dir = value.split(/\s+/, 2) + @order << Order.new(column: column, asc: dir.nil? || dir.downcase == "asc") + end + end + + # Parse cursor value into an auxiliary WHERE clause + def parse_cursor(values:) + # Services[:logger].info "parse_cursor #{values}" + if values.count > 1 + raise QueryParserError, "multiple cursor values" + end + begin + @cursor = Cursor.new(cursor_string: values.first) + rescue ArgumentError => e + raise QueryParserError, "cannot decode cursor: #{e.message}" + end end # Extract a single integer that can be passed to dataset.limit. diff --git a/lib/rights_api/result.rb b/lib/rights_api/result.rb index 0e304e9..5c49325 100644 --- a/lib/rights_api/result.rb +++ b/lib/rights_api/result.rb @@ -5,15 +5,15 @@ # and then data is populated according to the requested offset and limit. module RightsAPI class Result - attr_reader :offset, :total, :start, :end, :milliseconds, :data + attr_reader :offset, :total, :start, :end, :milliseconds, :cursor, :data # @param offset [Integer] The offset=x URL parameter. # @param total [Integer] The total number of results, regardless of paging. - def initialize(offset: 0, total: 0, milliseconds: 0.0, cached: false) + def initialize(offset: 0, total: 0, milliseconds: 0.0) @offset = offset @total = total @milliseconds = milliseconds - @cached = cached + @cursor = nil @start = 0 @end = 0 @data = [] @@ -33,6 +33,15 @@ def add!(row:) self end + # @return [Boolean] are there any more results after this one? + def more? + @end < @total + end + + def cursor=(arg) + @cursor = arg unless arg.nil? + end + # @return [Hash] def to_h h = { @@ -40,9 +49,9 @@ def to_h "start" => @start, "end" => @end, "milliseconds" => @milliseconds, - "cached" => @cached, "data" => @data } + h["cursor"] = @cursor unless @cursor.nil? finalize h end diff --git a/lib/rights_api/services.rb b/lib/rights_api/services.rb index 4ea3376..cb0d0cd 100644 --- a/lib/rights_api/services.rb +++ b/lib/rights_api/services.rb @@ -2,7 +2,6 @@ require "canister" require "logger" -require "lru_redux" require_relative "database" @@ -11,10 +10,6 @@ module RightsAPI Services = Canister.new - Services.register(:cache) do - LruRedux::Cache.new(DEFAULT_CACHE_SIZE) - end - Services.register(:database) do Database.new end diff --git a/spec/integration/rights_log_spec.rb b/spec/integration/rights_log_spec.rb index 26feec2..b6b2c4f 100644 --- a/spec/integration/rights_log_spec.rb +++ b/spec/integration/rights_log_spec.rb @@ -1,14 +1,14 @@ # frozen_string_literal: true require "climate_control" -require "ffi-icu" +# require "ffi-icu" require "rack/test" require "shared_examples" RSpec.describe "/rights_log" do include Rack::Test::Methods # Use ICU collator to try to approximate Sequel's collation of underscore vs Ruby's - let(:collator) { ICU::Collation::Collator.new("en") } + # let(:collator) { ICU::Collation::Collator.new("en") } describe "/rights_log/HTID" do context "with a valid HTID" do @@ -36,17 +36,4 @@ expect(response[:data]).to eq(sorted) end end - - context "with an OFFSET" do - before(:each) { get(rights_api_endpoint + "rights_log?limit=2") } - - it "produces the same results with or without optimizer" do - optimized = parse_json get(rights_api_endpoint + "rights_log?limit=2&offset=2").body - unoptimized = nil - ClimateControl.modify(RIGHTS_API_DISABLE_OFFSET_OPTIMIZER: "1") do - unoptimized = parse_json get(rights_api_endpoint + "rights_log?limit=2&offset=2").body - end - expect(optimized[:data]).to eq(unoptimized[:data]) - end - end end diff --git a/spec/integration/rights_spec.rb b/spec/integration/rights_spec.rb index 6439ecb..1ffa2b6 100644 --- a/spec/integration/rights_spec.rb +++ b/spec/integration/rights_spec.rb @@ -1,13 +1,13 @@ # frozen_string_literal: true -require "ffi-icu" +# require "ffi-icu" require "rack/test" require "shared_examples" RSpec.describe "/rights" do include Rack::Test::Methods # Use ICU collator to try to approximate Sequel's collation of underscore vs Ruby's - let(:collator) { ICU::Collation::Collator.new("en") } + # let(:collator) { ICU::Collation::Collator.new("en") } describe "/rights_log/HTID" do context "with a valid HTID" do @@ -35,17 +35,4 @@ expect(response[:data]).to eq(sorted) end end - - context "with an OFFSET" do - before(:each) { get(rights_api_endpoint + "rights?limit=2") } - - it "produces the same results with or without optimizer" do - optimized = parse_json get(rights_api_endpoint + "rights?limit=2&offset=2").body - unoptimized = nil - ClimateControl.modify(RIGHTS_API_DISABLE_OFFSET_OPTIMIZER: "1") do - unoptimized = parse_json get(rights_api_endpoint + "rights?limit=2&offset=2").body - end - expect(optimized[:data]).to eq(unoptimized[:data]) - end - end end diff --git a/spec/integration/support_table_spec.rb b/spec/integration/support_table_spec.rb index f982b92..2fd1343 100644 --- a/spec/integration/support_table_spec.rb +++ b/spec/integration/support_table_spec.rb @@ -42,24 +42,24 @@ end # With OFFSET - SUPPORT_TABLES.each do |table| - describe "/#{table}/offset=" do - context "with a valid offset value" do - before(:each) { get(rights_api_endpoint + table + "?offset=1") } - it_behaves_like "nonempty #{table} response" - - it "returns data set starting at index 2" do - response = parse_json(last_response.body) - expect(response[:start]).to eq(2) - end - end - - context "with a bogus offset value" do - before(:each) { get(rights_api_endpoint + table + "?offset=x") } - it_behaves_like "400 response" - end - end - end + # SUPPORT_TABLES.each do |table| + # describe "/#{table}/offset=" do + # context "with a valid offset value" do + # before(:each) { get(rights_api_endpoint + table + "?offset=1") } + # it_behaves_like "nonempty #{table} response" + # + # it "returns data set starting at index 2" do + # response = parse_json(last_response.body) + # expect(response[:start]).to eq(2) + # end + # end + # + # context "with a bogus offset value" do + # before(:each) { get(rights_api_endpoint + table + "?offset=x") } + # it_behaves_like "400 response" + # end + # end + # end # With LIMIT SUPPORT_TABLES.each do |table| diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b235fbf..2916534 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -33,6 +33,12 @@ def rights_api_endpoint "/v1/" end +module RightsAPI + VALID_CURSOR = "WzIsMl0=" + ANOTHER_VALID_CURSOR = "WzQsNF0=" + INVALID_CURSOR = "%" +end + # BEGIN RSPEC BOILERPLATE # This file was generated by the `rspec --init` command. Conventionally, all diff --git a/spec/unit/cursor_spec.rb b/spec/unit/cursor_spec.rb new file mode 100644 index 0000000..74b10ab --- /dev/null +++ b/spec/unit/cursor_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module RightsAPI + RSpec.describe Cursor do + describe ".encode" do + context "with an object" do + it "returns a string" do + expect(described_class.encode({})).to be_a(String) + end + end + + context "with nil" do + it "returns a string" do + expect(described_class.encode(nil)).to be_a(String) + end + end + end + + describe ".decode" do + context "with a valid cursor string" do + it "returns an object" do + expect(described_class.decode(VALID_CURSOR)).to be_a(Array) + end + end + + context "with a bogus cursor string" do + it "raises ArgumentError" do + expect { described_class.decode(INVALID_CURSOR) }.to raise_error(ArgumentError) + end + end + end + + describe "#initialize" do + context "with a valid cursor string" do + it "creates RightsAPI::Cursor instance" do + expect(described_class.new(cursor_string: VALID_CURSOR)).to be_a(RightsAPI::Cursor) + end + end + + context "with an empty cursor string" do + it "creates RightsAPI::Cursor instance" do + expect(described_class.new).to be_a(RightsAPI::Cursor) + end + end + + context "with a bogus cursor string" do + it "raises ArgumentError" do + expect { described_class.new(INVALID_CURSOR) }.to raise_error(ArgumentError) + end + end + end + end +end diff --git a/spec/unit/database_spec.rb b/spec/unit/database_spec.rb index 0b599a0..e45206c 100644 --- a/spec/unit/database_spec.rb +++ b/spec/unit/database_spec.rb @@ -2,46 +2,48 @@ require "climate_control" -RSpec.describe RightsAPI::Database do - describe "#initialize" do - it "creates RightsAPI::Database instance" do - expect(described_class.new).to be_a(RightsAPI::Database) +module RightsAPI + RSpec.describe Database do + describe "#initialize" do + it "creates RightsAPI::Database instance" do + expect(described_class.new).to be_a(RightsAPI::Database) + end end - end - describe "#connect" do - it "connects with built-in connection string" do - expect(described_class.new).not_to be nil - end + describe "#connect" do + it "connects with built-in connection string" do + expect(described_class.new).not_to be nil + end - it "connects with explicit connection string" do - expect(described_class.new.connect(ENV["RIGHTS_API_DATABASE_CONNECTION_STRING"])).not_to be nil - end + it "connects with explicit connection string" do + expect(described_class.new.connect(ENV["RIGHTS_API_DATABASE_CONNECTION_STRING"])).not_to be nil + end - it "connects with connection arguments" do - ClimateControl.modify(RIGHTS_API_DATABASE_CONNECTION_STRING: nil) do - args = { - user: "ht_rights", - password: "ht_rights", - host: "mariadb", - database: "ht", - adapter: "mysql2" - } - expect(described_class.new.connect(**args)).not_to be nil + it "connects with connection arguments" do + ClimateControl.modify(RIGHTS_API_DATABASE_CONNECTION_STRING: nil) do + args = { + user: "ht_rights", + password: "ht_rights", + host: "mariadb", + database: "ht", + adapter: "mysql2" + } + expect(described_class.new.connect(**args)).not_to be nil + end end - end - it "connects with ENV variables" do - env = { - RIGHTS_API_DATABASE_CONNECTION_STRING: nil, - RIGHTS_API_DATABASE_USER: "ht_rights", - RIGHTS_API_DATABASE_PASSWORD: "ht_rights", - RIGHTS_API_DATABASE_HOST: "mariadb", - RIGHTS_API_DATABASE_DATABASE: "ht", - RIGHTS_API_DATABASE_ADAPTER: "mysql2" - } - ClimateControl.modify(**env) do - expect(described_class.new.connect).not_to be nil + it "connects with ENV variables" do + env = { + RIGHTS_API_DATABASE_CONNECTION_STRING: nil, + RIGHTS_API_DATABASE_USER: "ht_rights", + RIGHTS_API_DATABASE_PASSWORD: "ht_rights", + RIGHTS_API_DATABASE_HOST: "mariadb", + RIGHTS_API_DATABASE_DATABASE: "ht", + RIGHTS_API_DATABASE_ADAPTER: "mysql2" + } + ClimateControl.modify(**env) do + expect(described_class.new.connect).not_to be nil + end end end end diff --git a/spec/unit/models/rights_current_spec.rb b/spec/unit/models/rights_current_spec.rb index 42b3d82..0925277 100644 --- a/spec/unit/models/rights_current_spec.rb +++ b/spec/unit/models/rights_current_spec.rb @@ -17,8 +17,11 @@ module RightsAPI end describe "#default_order" do - it "returns Array with timestamp" do - expect(described_class.default_order.first).to be_a(Sequel::SQL::Expression) + it "returns Array of RightsAPI::Order" do + expect(described_class.default_order).to be_a(Array) + described_class.default_order.each do |order| + expect(order).to be_a(RightsAPI::Order) + end end end diff --git a/spec/unit/models/rights_log_spec.rb b/spec/unit/models/rights_log_spec.rb index 24f3220..9083d6d 100644 --- a/spec/unit/models/rights_log_spec.rb +++ b/spec/unit/models/rights_log_spec.rb @@ -17,8 +17,11 @@ module RightsAPI end describe "#default_order" do - it "returns Array with timestamp" do - expect(described_class.default_order.first).to be_a(Sequel::SQL::Expression) + it "returns Array of RightsAPI::Order" do + expect(described_class.default_order).to be_a(Array) + described_class.default_order.each do |order| + expect(order).to be_a(RightsAPI::Order) + end end end diff --git a/spec/unit/order_spec.rb b/spec/unit/order_spec.rb new file mode 100644 index 0000000..8a773fd --- /dev/null +++ b/spec/unit/order_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module RightsAPI + RSpec.describe Order do + let(:order) { described_class.new(column: :id) } + let(:order_desc) { described_class.new(column: :id, asc: false) } + + describe ".new" do + it "creates a #{described_class}" do + expect(order).to be_a(described_class) + end + + it "has the expected attribute reader" do + %i[column].each do |reader| + expect(order.send(reader)).not_to be_nil + end + end + end + + describe "#asc?" do + context "with default direction" do + it "returns true" do + expect(order.asc?).to eq true + end + end + + context "with asc false" do + it "returns false" do + expect(order_desc.asc?).to eq false + end + end + end + end +end diff --git a/spec/unit/query_optimizer_spec.rb b/spec/unit/query_optimizer_spec.rb deleted file mode 100644 index 56d7aea..0000000 --- a/spec/unit/query_optimizer_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module RightsAPI - RSpec.describe(QueryOptimizer) do - let(:query_parser) { QueryParser.new(model: RightsCurrent).parse } - let(:query_optimizer) { described_class.new(parser: query_parser) } - - describe "#initialize" do - it "creates RightsAPI::QueryOptimizer instance" do - expect(query_optimizer).to be_a(RightsAPI::QueryOptimizer) - expect(query_optimizer.parser).to be_a(RightsAPI::QueryParser) - expect(query_optimizer.offset).to be_a(Integer) - expect(query_optimizer.where).to be_an(Array) - end - end - - describe "#add_to_cache" do - it "adds an entry to the cache" do - query_optimizer.add_to_cache(dataset: RightsCurrent.base_dataset.all) - expect(Services[:cache].count).to be > 0 - end - end - end -end diff --git a/spec/unit/query_parser_spec.rb b/spec/unit/query_parser_spec.rb index 185cdef..c843c20 100644 --- a/spec/unit/query_parser_spec.rb +++ b/spec/unit/query_parser_spec.rb @@ -10,7 +10,7 @@ module RightsAPI end it "has the expected attribute readers" do - %i[model where order offset limit].each do |reader| + %i[model where order limit cursor].each do |reader| expect(query_parser.send(reader)).not_to be_nil end end @@ -25,8 +25,16 @@ module RightsAPI expect(query_parser.parse(params: {id: ["1"]}).where.count).to eq(1) end - it "parses OFFSET query" do - expect(query_parser.parse(params: {offset: ["1"]}).offset).to eq(1) + it "parses cursor" do + expect(query_parser.parse(params: {cursor: [VALID_CURSOR]}).cursor).to be_a(RightsAPI::Cursor) + end + + it "raises on bogus cursor" do + expect { query_parser.parse(params: {cursor: [INVALID_CURSOR]}) }.to raise_error(QueryParserError) + end + + it "raises on multiple cursors" do + expect { query_parser.parse(params: {cursor: [VALID_CURSOR, ANOTHER_VALID_CURSOR]}) }.to raise_error(QueryParserError) end it "parses LIMIT query" do @@ -36,10 +44,6 @@ module RightsAPI it "raises on bogus LIMIT queries" do expect { query_parser.parse(params: {limit: ["a"]}) }.to raise_error(QueryParserError) end - - it "raises on bogus OFFSET queries" do - expect { query_parser.parse(params: {offset: ["a"]}) }.to raise_error(QueryParserError) - end end end end diff --git a/spec/unit/result_spec.rb b/spec/unit/result_spec.rb index 4424b63..8816c4a 100644 --- a/spec/unit/result_spec.rb +++ b/spec/unit/result_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module RightsAPI - REQUIRED_HASH_KEYS = %w[total start end milliseconds cached data] + REQUIRED_HASH_KEYS = %w[total start end milliseconds data] RSpec.describe(Result) do let(:result) { described_class.new } let(:test_row) { {key1: "value1", key2: "value2"} } @@ -47,6 +47,32 @@ module RightsAPI end end + describe "#more?" do + context "with a total greater than rows added" do + it "returns true" do + res = described_class.new(total: 4) + 2.times { |i| res.add! row: {} } + expect(res.more?).to be true + end + end + + context "with a total equal to rows added" do + it "returns false" do + res = described_class.new(total: 4) + 4.times { |i| res.add! row: {} } + expect(res.more?).to be false + end + end + end + + describe "#cursor=" do + it "sets the cursor" do + res = described_class.new(total: 1) + res.cursor = "cursor" + expect(res.cursor).to eq "cursor" + end + end + describe "#to_h" do it "returns a hash" do expect(result.to_h).to be_a(Hash) From 7561192c1bcd4bcf68040226f785293eff830ba5 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Wed, 1 May 2024 17:44:08 -0400 Subject: [PATCH 11/18] Get rid of deprecated version tag --- docker-compose.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index ec24f63..86487c5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,6 +1,4 @@ --- -version: '3' - x-condition-healthy: &healthy condition: service_healthy From abd73c4815294fdd1f4db402f535ace526c74311 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Wed, 1 May 2024 17:45:12 -0400 Subject: [PATCH 12/18] Restore total test coverage to 100% --- lib/rights_api/cursor.rb | 10 +++------- lib/rights_api/order.rb | 9 +++++++++ lib/rights_api/query.rb | 12 +----------- lib/rights_api/query_parser.rb | 12 ++---------- spec/unit/cursor_spec.rb | 31 ++++++++++++++++++++++++++++++- spec/unit/order_spec.rb | 16 ++++++++++++++++ spec/unit/query_spec.rb | 8 ++++++++ 7 files changed, 69 insertions(+), 29 deletions(-) diff --git a/lib/rights_api/cursor.rb b/lib/rights_api/cursor.rb index 2291d7b..e975c8f 100644 --- a/lib/rights_api/cursor.rb +++ b/lib/rights_api/cursor.rb @@ -53,20 +53,17 @@ def encode(order:, rows:) order.each do |ord| data << row[ord.column] end - Services[:logger].info "to encode: #{data}" self.class.encode data end # Generate zero or one WHERE clauses that will generate a pseudo-OFFSET # based on ORDER BY parameters. - # FIXME: should cursor be a required parameter? (require "*" in order to get back a - # cursor value?) - # TODO: Can we shorten long cursors by calculating longest common initial substring - # based on last value in current window and first value of next? # ORDER BY a, b, c TRANSLATES TO # WHERE (a > 1) # OR (a = 1 AND b > 2) # OR (a = 1 AND b = 2 AND c > 3) + # @param model [Class] Sequel::Model subclass for the table being queried + # @param order [Array] def where(model:, order:) return [] if values.empty? @@ -79,8 +76,7 @@ def where(model:, order:) order.count.times do |order_index| # Take a slice of ORDER of size order_index + 1 and_clause = order[0, order_index + 1].each_with_index.map do |ord, i| - # in which each element is a "col op val" string - # and the last is an inequality + # in which each element is a "col op val" string and the last is an inequality op = if i == order_index ord.asc? ? ">" : "<" else diff --git a/lib/rights_api/order.rb b/lib/rights_api/order.rb index 9ad14af..b66a9cd 100644 --- a/lib/rights_api/order.rb +++ b/lib/rights_api/order.rb @@ -17,5 +17,14 @@ def initialize(column:, asc: true) def asc? @asc end + + # @return [Sequel::SQL::OrderedExpression] + def to_sequel(model:) + if asc? + Sequel.asc(model.qualify(field: column)) + else + Sequel.desc(model.qualify(field: column)) + end + end end end diff --git a/lib/rights_api/query.rb b/lib/rights_api/query.rb index ea87ac8..a5ca5fc 100644 --- a/lib/rights_api/query.rb +++ b/lib/rights_api/query.rb @@ -30,7 +30,7 @@ def run parser.where.each do |where| dataset = dataset.where(where) end - dataset = dataset.order(*(parser.order.map { |order| order_to_sequel(order: order, model: model) })) + dataset = dataset.order(*(parser.order.map { |order| order.to_sequel(model: model) })) # Save this here because limit may alter the count. @total = dataset.count dataset = dataset.limit(parser.limit).all @@ -45,15 +45,5 @@ def run end result end - - private - - def order_to_sequel(order:, model:) - if order.asc? - Sequel.asc(model.qualify(field: order.column)) - else - Sequel.desc(model.qualify(field: order.column)) - end - end end end diff --git a/lib/rights_api/query_parser.rb b/lib/rights_api/query_parser.rb index da4d059..eafc728 100644 --- a/lib/rights_api/query_parser.rb +++ b/lib/rights_api/query_parser.rb @@ -33,8 +33,6 @@ def parse(params: {}) parse_cursor(values: values) when :limit parse_limit(values: values) - when :order - parse_order(values: values) else parse_parameter(key: key, values: values) end @@ -72,14 +70,6 @@ def parse_parameter(key:, values:) end end - # Raturn Array that can be passed as params to .order - def parse_order(values:) - values.each do |value| - column, dir = value.split(/\s+/, 2) - @order << Order.new(column: column, asc: dir.nil? || dir.downcase == "asc") - end - end - # Parse cursor value into an auxiliary WHERE clause def parse_cursor(values:) # Services[:logger].info "parse_cursor #{values}" @@ -105,6 +95,8 @@ def parse_limit(values:) # @param type [String] "OFFSET" or "LIMIT", used only for reporting errors. # @return [Integer] def parse_int_value(values:, type:) + return values.last if values.last.is_a? Integer + value = values.last.to_i # Make sure the offset can make a round-trip conversion between Int and String # https://stackoverflow.com/a/1235891 diff --git a/spec/unit/cursor_spec.rb b/spec/unit/cursor_spec.rb index 74b10ab..6f44126 100644 --- a/spec/unit/cursor_spec.rb +++ b/spec/unit/cursor_spec.rb @@ -2,6 +2,8 @@ module RightsAPI RSpec.describe Cursor do + let(:empty_cursor) { described_class.new } + describe ".encode" do context "with an object" do it "returns a string" do @@ -39,7 +41,7 @@ module RightsAPI context "with an empty cursor string" do it "creates RightsAPI::Cursor instance" do - expect(described_class.new).to be_a(RightsAPI::Cursor) + expect(empty_cursor).to be_a(RightsAPI::Cursor) end end @@ -49,5 +51,32 @@ module RightsAPI end end end + + describe "#encode" do + it "returns the expected cursor string" do + expected = described_class.encode([1, "test", "cc-by-3.0_google", "2009-01-01 05:00:00 +0000"]) + row = {namespace: "test", id: "cc-by-3.0_google", time: "2009-01-01 05:00:00 +0000"} + expect(empty_cursor.encode(order: RightsCurrent.default_order, rows: [row])).to eq(expected) + end + end + + describe "#where" do + context "with no results" do + it "returns an empty Array" do + expect(empty_cursor.where(model: RightsCurrent, order: [])).to match_array([]) + end + end + + it "produces a one-element Array with a Sequel literal" do + cursor_string = described_class.encode([0, "test", "cc-by-3.0_google", "2009-01-01 05:00:00 +0000"]) + cursor = described_class.new(cursor_string: cursor_string) + expected = <<~END.delete("\n") + (rights_current.namespace>'test') OR + (rights_current.namespace='test' AND rights_current.id>'cc-by-3.0_google') OR + (rights_current.namespace='test' AND rights_current.id='cc-by-3.0_google' AND rights_current.time>'2009-01-01 05:00:00 +0000') + END + expect(cursor.where(model: RightsCurrent, order: RightsCurrent.default_order).first.to_s).to eq(expected) + end + end end end diff --git a/spec/unit/order_spec.rb b/spec/unit/order_spec.rb index 8a773fd..0550e63 100644 --- a/spec/unit/order_spec.rb +++ b/spec/unit/order_spec.rb @@ -30,5 +30,21 @@ module RightsAPI end end end + + describe "#to_sequel" do + context "with default direction" do + it "returns Sequel::SQL::OrderedExpression with #descending == false" do + expect(order.to_sequel(model: RightsCurrent)).to be_a(Sequel::SQL::OrderedExpression) + expect(order.to_sequel(model: RightsCurrent).descending).to eq(false) + end + end + + context "with asc false" do + it "returns Sequel::SQL::OrderedExpression with #descending == true" do + expect(order_desc.to_sequel(model: RightsCurrent)).to be_a(Sequel::SQL::OrderedExpression) + expect(order_desc.to_sequel(model: RightsCurrent).descending).to eq(true) + end + end + end end end diff --git a/spec/unit/query_spec.rb b/spec/unit/query_spec.rb index b5b2994..ebda2a1 100644 --- a/spec/unit/query_spec.rb +++ b/spec/unit/query_spec.rb @@ -6,6 +6,7 @@ module RightsAPI RSpec.describe(Query) do let(:query) { described_class.new(model: Attribute) } let(:query_with_params) { described_class.new(model: Attribute, params: {id: [1]}) } + let(:query_with_limit) { described_class.new(model: Attribute, params: {limit: [2]}) } describe ".new" do it "creates a Query" do @@ -25,6 +26,13 @@ module RightsAPI expect(query.run).to be_a_kind_of(Result) end end + + context "with a limit" do + it "returns a Result with a Cursor" do + expect(query_with_limit.run).to be_a_kind_of(Result) + expect(query_with_limit.run.cursor).to be_a(String) + end + end end end end From dc738b3ae9ba553ff6866c0edfc9b6f70668797b Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 2 May 2024 10:11:59 -0400 Subject: [PATCH 13/18] Revert docker-compose.yml for GH issue #15 -- rein in scope creep --- docker-compose.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docker-compose.yml b/docker-compose.yml index 86487c5..ec24f63 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,4 +1,6 @@ --- +version: '3' + x-condition-healthy: &healthy condition: service_healthy From e0665ba67c54c359eaf085de0871d760e2fe49d9 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 2 May 2024 10:14:04 -0400 Subject: [PATCH 14/18] Undo scope creep: un-alphabetize gem list --- Gemfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index cee4b83..22b9afe 100644 --- a/Gemfile +++ b/Gemfile @@ -11,9 +11,9 @@ gem "sinatra-contrib" group :development, :test do gem "pry" - gem "rack-test" + gem "standard" gem "rspec" + gem "rack-test" gem "simplecov" gem "simplecov-lcov" - gem "standard" end From 843542fbecea7779e653a74479304cdb9370eed1 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 2 May 2024 11:27:11 -0400 Subject: [PATCH 15/18] Moar comments --- lib/rights_api/cursor.rb | 67 ++++++++++++++++++++++------------ lib/rights_api/query.rb | 2 +- lib/rights_api/query_parser.rb | 6 +-- spec/unit/cursor_spec.rb | 4 +- 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/lib/rights_api/cursor.rb b/lib/rights_api/cursor.rb index e975c8f..dd4dfae 100644 --- a/lib/rights_api/cursor.rb +++ b/lib/rights_api/cursor.rb @@ -10,16 +10,30 @@ # a WHERE list # Tries to store just enough information so the next query can -# pick up where the current one left off. +# pick up where the current one left off. The Base64-encoded cursor string +# is a serialized array containing the current offset and one value for each +# sort parameter (explicit or default) used in the query. -# Relies on the search parameters being unchanged between queries, -# if they do then the results are undefined. +# CURSOR LIFECYCLE +# - At the beginning of the query a Cursor is created with the "cursor" URL parameter, or +# nil if none was supplied (indicating first page of results, i.e. no previous query). +# - Query calls `cursor.where` to create a WHERE clause based on the decoded values +# from the previous result (in effect saying "WHERE fields > last_result") +# - Query calls `cursor.offset` for the current page of results. +# - Query calls `cursor.encode` to calculate a new offset and new last_result values +# dictated by the current ORDER BY. -# To create the semi-opaque cursor value for the result set, -# creates an array containing the current offset and one value for each -# sort parameter (explicit or default). +# IVAR SEMANTICS +# - `offset` the (zero-based) offset into the overall results set produced by `where`, or perhaps +# "give me the results at offset N (by using values X Y and Z)" +# - `values` the X Y and Z from above, these are the relevant values from the previous result +# if there was one. +# It is possibly counterintuitive that X Y and Z are NOT at offset N. Offset N is the location +# of the NEXT record. -# This may be a module and not a class +# CAVEATS +# Relies on the search parameters being unchanged between queries, +# if they do change then the results are undefined. module RightsAPI class Cursor OFFSET_KEY = "off" @@ -32,6 +46,9 @@ def self.decode(cursor_string) JSON.parse(Base64.urlsafe_decode64(cursor_string)) end + # JSON-encode and Base64-encode an object + # @param arg [Object] a serializable object (always an Array in this class) + # @return [String] def self.encode(arg) Base64.urlsafe_encode64(JSON.generate(arg)) end @@ -45,25 +62,16 @@ def initialize(cursor_string: nil) end end - # @param order [Array] - # @param rows [Sequel::Dataset] - def encode(order:, rows:) - data = [offset + rows.count] - row = rows.last - order.each do |ord| - data << row[ord.column] - end - self.class.encode data - end - # Generate zero or one WHERE clauses that will generate a pseudo-OFFSET # based on ORDER BY parameters. # ORDER BY a, b, c TRANSLATES TO # WHERE (a > 1) # OR (a = 1 AND b > 2) # OR (a = 1 AND b = 2 AND c > 3) - # @param model [Class] Sequel::Model subclass for the table being queried - # @param order [Array] + # @param model [Class] Sequel::Model subclass for the table being queried, + # only used for qualifying column names in the WHERE clause. + # @param order [Array] the current query's ORDER BY + # @return [Array] zero or one Sequel literals def where(model:, order:) return [] if values.empty? @@ -71,7 +79,7 @@ def where(model:, order:) # Each OR clause is a series of AND clauses. # The last element of each AND clause is < or >, the others are = # The first AND clause has only the first ORDER parameter. - # Each subsequent one adds one ORDER PARAMETER. + # Each subsequent one adds one ORDER parameter. or_clause = [] order.count.times do |order_index| # Take a slice of ORDER of size order_index + 1 @@ -86,8 +94,21 @@ def where(model:, order:) end or_clause << "(" + and_clause.join(" AND ") + ")" end - res = Sequel.lit or_clause.join(" OR ") - [res] + [Sequel.lit(or_clause.join(" OR "))] + end + + # Encode the offset and the relevant values from the last result row + # (i.e. those used in the current ORDER BY) + # @param order [Array] the current query's ORDER BY + # @param rows [Sequel::Dataset] the result of the current query + # @return [String] + def encode(order:, rows:) + data = [offset + rows.count] + row = rows.last + order.each do |ord| + data << row[ord.column] + end + self.class.encode data end end end diff --git a/lib/rights_api/query.rb b/lib/rights_api/query.rb index a5ca5fc..8e684ac 100644 --- a/lib/rights_api/query.rb +++ b/lib/rights_api/query.rb @@ -35,7 +35,7 @@ def run @total = dataset.count dataset = dataset.limit(parser.limit).all end - result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta) + result = Result.new(offset: parser.cursor.offset, total: total, milliseconds: 1000 * time_delta) dataset.each do |row| result.add! row: row.to_h end diff --git a/lib/rights_api/query_parser.rb b/lib/rights_api/query_parser.rb index eafc728..7610105 100644 --- a/lib/rights_api/query_parser.rb +++ b/lib/rights_api/query_parser.rb @@ -47,12 +47,8 @@ def where @where + cursor.where(model: model, order: order) end - def offset - cursor.offset - end - def cursor - @cursor || Cursor.new + @cursor ||= Cursor.new end private diff --git a/spec/unit/cursor_spec.rb b/spec/unit/cursor_spec.rb index 6f44126..249620d 100644 --- a/spec/unit/cursor_spec.rb +++ b/spec/unit/cursor_spec.rb @@ -72,8 +72,8 @@ module RightsAPI cursor = described_class.new(cursor_string: cursor_string) expected = <<~END.delete("\n") (rights_current.namespace>'test') OR - (rights_current.namespace='test' AND rights_current.id>'cc-by-3.0_google') OR - (rights_current.namespace='test' AND rights_current.id='cc-by-3.0_google' AND rights_current.time>'2009-01-01 05:00:00 +0000') + (rights_current.namespace='test' AND rights_current.id>'cc-by-3.0_google') OR + (rights_current.namespace='test' AND rights_current.id='cc-by-3.0_google' AND rights_current.time>'2009-01-01 05:00:00 +0000') END expect(cursor.where(model: RightsCurrent, order: RightsCurrent.default_order).first.to_s).to eq(expected) end From fa1d68804351164b62197dc19cbbb75104d09560 Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 2 May 2024 11:54:55 -0400 Subject: [PATCH 16/18] Moar tests --- lib/rights_api/query.rb | 6 +++++- lib/rights_api/query_parser.rb | 9 ++------- spec/integration/rights_log_spec.rb | 22 +++++++++++++++++++--- spec/integration/rights_spec.rb | 22 +++++++++++++++++++--- spec/unit/cursor_spec.rb | 4 ++-- spec/unit/query_parser_spec.rb | 16 +++++++--------- 6 files changed, 54 insertions(+), 25 deletions(-) diff --git a/lib/rights_api/query.rb b/lib/rights_api/query.rb index 8e684ac..1c0b9e9 100644 --- a/lib/rights_api/query.rb +++ b/lib/rights_api/query.rb @@ -31,8 +31,12 @@ def run dataset = dataset.where(where) end dataset = dataset.order(*(parser.order.map { |order| order.to_sequel(model: model) })) - # Save this here because limit may alter the count. + # Save this here because limit and cursor would otherwise alter the count. @total = dataset.count + # Apply the cursor to get to the offset we want + parser.cursor.where(model: model, order: parser.order).each do |where| + dataset = dataset.where(where) + end dataset = dataset.limit(parser.limit).all end result = Result.new(offset: parser.cursor.offset, total: total, milliseconds: 1000 * time_delta) diff --git a/lib/rights_api/query_parser.rb b/lib/rights_api/query_parser.rb index 7610105..3b352cb 100644 --- a/lib/rights_api/query_parser.rb +++ b/lib/rights_api/query_parser.rb @@ -13,7 +13,7 @@ module RightsAPI class QueryParser DEFAULT_LIMIT = 1000 - attr_reader :params, :model, :order, :limit + attr_reader :params, :model, :where, :order, :limit # @param model [Class] Sequel::Model subclass for the table being queried def initialize(model:) @@ -43,10 +43,6 @@ def parse(params: {}) self end - def where - @where + cursor.where(model: model, order: order) - end - def cursor @cursor ||= Cursor.new end @@ -68,9 +64,8 @@ def parse_parameter(key:, values:) # Parse cursor value into an auxiliary WHERE clause def parse_cursor(values:) - # Services[:logger].info "parse_cursor #{values}" if values.count > 1 - raise QueryParserError, "multiple cursor values" + raise QueryParserError, "multiple cursor values (#{values})" end begin @cursor = Cursor.new(cursor_string: values.first) diff --git a/spec/integration/rights_log_spec.rb b/spec/integration/rights_log_spec.rb index b6b2c4f..9098f68 100644 --- a/spec/integration/rights_log_spec.rb +++ b/spec/integration/rights_log_spec.rb @@ -1,14 +1,11 @@ # frozen_string_literal: true require "climate_control" -# require "ffi-icu" require "rack/test" require "shared_examples" RSpec.describe "/rights_log" do include Rack::Test::Methods - # Use ICU collator to try to approximate Sequel's collation of underscore vs Ruby's - # let(:collator) { ICU::Collation::Collator.new("en") } describe "/rights_log/HTID" do context "with a valid HTID" do @@ -36,4 +33,23 @@ expect(response[:data]).to eq(sorted) end end + + context "with a cursor" do + before(:each) { get(rights_api_endpoint + "rights_log?limit=2") } + it_behaves_like "nonempty rights response" + + it "returns a cursor" do + response = parse_json(last_response.body) + expect(response[:cursor]).to be_a(String) + end + + it "produces next page of results when cursor is used" do + response_1 = parse_json(last_response.body) + cursor = response_1[:cursor] + get(rights_api_endpoint + "rights_log?limit=2&cursor=#{cursor}") + response_2 = parse_json(last_response.body) + expect(response_2[:total]).to eq(response_1[:total]) + expect(response_2[:start]).to eq(3) + end + end end diff --git a/spec/integration/rights_spec.rb b/spec/integration/rights_spec.rb index 1ffa2b6..7c0ecd8 100644 --- a/spec/integration/rights_spec.rb +++ b/spec/integration/rights_spec.rb @@ -1,13 +1,10 @@ # frozen_string_literal: true -# require "ffi-icu" require "rack/test" require "shared_examples" RSpec.describe "/rights" do include Rack::Test::Methods - # Use ICU collator to try to approximate Sequel's collation of underscore vs Ruby's - # let(:collator) { ICU::Collation::Collator.new("en") } describe "/rights_log/HTID" do context "with a valid HTID" do @@ -35,4 +32,23 @@ expect(response[:data]).to eq(sorted) end end + + context "with a cursor" do + before(:each) { get(rights_api_endpoint + "rights?limit=2") } + it_behaves_like "nonempty rights response" + + it "returns a cursor" do + response = parse_json(last_response.body) + expect(response[:cursor]).to be_a(String) + end + + it "produces next page of results when cursor is used" do + response_1 = parse_json(last_response.body) + cursor = response_1[:cursor] + get(rights_api_endpoint + "rights?limit=2&cursor=#{cursor}") + response_2 = parse_json(last_response.body) + expect(response_2[:total]).to eq(response_1[:total]) + expect(response_2[:start]).to eq(3) + end + end end diff --git a/spec/unit/cursor_spec.rb b/spec/unit/cursor_spec.rb index 249620d..6f44126 100644 --- a/spec/unit/cursor_spec.rb +++ b/spec/unit/cursor_spec.rb @@ -72,8 +72,8 @@ module RightsAPI cursor = described_class.new(cursor_string: cursor_string) expected = <<~END.delete("\n") (rights_current.namespace>'test') OR - (rights_current.namespace='test' AND rights_current.id>'cc-by-3.0_google') OR - (rights_current.namespace='test' AND rights_current.id='cc-by-3.0_google' AND rights_current.time>'2009-01-01 05:00:00 +0000') + (rights_current.namespace='test' AND rights_current.id>'cc-by-3.0_google') OR + (rights_current.namespace='test' AND rights_current.id='cc-by-3.0_google' AND rights_current.time>'2009-01-01 05:00:00 +0000') END expect(cursor.where(model: RightsCurrent, order: RightsCurrent.default_order).first.to_s).to eq(expected) end diff --git a/spec/unit/query_parser_spec.rb b/spec/unit/query_parser_spec.rb index c843c20..44d307c 100644 --- a/spec/unit/query_parser_spec.rb +++ b/spec/unit/query_parser_spec.rb @@ -10,21 +10,13 @@ module RightsAPI end it "has the expected attribute readers" do - %i[model where order limit cursor].each do |reader| + %i[model where order limit].each do |reader| expect(query_parser.send(reader)).not_to be_nil end end end describe "#parse" do - it "parses empty query into zero WHERE clauses" do - expect(query_parser.parse.where.count).to eq(0) - end - - it "parses id query into one WHERE clause" do - expect(query_parser.parse(params: {id: ["1"]}).where.count).to eq(1) - end - it "parses cursor" do expect(query_parser.parse(params: {cursor: [VALID_CURSOR]}).cursor).to be_a(RightsAPI::Cursor) end @@ -45,5 +37,11 @@ module RightsAPI expect { query_parser.parse(params: {limit: ["a"]}) }.to raise_error(QueryParserError) end end + + describe "#cursor" do + it "returns a RightsAPI::Cursor" do + expect(query_parser.cursor).to be_a(RightsAPI::Cursor) + end + end end end From 10684bf3420035838817d7844fe59ec78441cdbd Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 2 May 2024 11:58:57 -0400 Subject: [PATCH 17/18] Back unneeded changes to services.rb --- lib/rights_api/services.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/rights_api/services.rb b/lib/rights_api/services.rb index cb0d0cd..db47c11 100644 --- a/lib/rights_api/services.rb +++ b/lib/rights_api/services.rb @@ -5,11 +5,8 @@ require_relative "database" -DEFAULT_CACHE_SIZE = 1000 - module RightsAPI Services = Canister.new - Services.register(:database) do Database.new end From eab1c00cfa2d3436972143d651f853513dc26e2a Mon Sep 17 00:00:00 2001 From: Brian Moses Hall Date: Thu, 2 May 2024 12:04:57 -0400 Subject: [PATCH 18/18] Back out scope creep on database spec --- spec/unit/database_spec.rb | 70 ++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/spec/unit/database_spec.rb b/spec/unit/database_spec.rb index e45206c..0b599a0 100644 --- a/spec/unit/database_spec.rb +++ b/spec/unit/database_spec.rb @@ -2,48 +2,46 @@ require "climate_control" -module RightsAPI - RSpec.describe Database do - describe "#initialize" do - it "creates RightsAPI::Database instance" do - expect(described_class.new).to be_a(RightsAPI::Database) - end +RSpec.describe RightsAPI::Database do + describe "#initialize" do + it "creates RightsAPI::Database instance" do + expect(described_class.new).to be_a(RightsAPI::Database) end + end - describe "#connect" do - it "connects with built-in connection string" do - expect(described_class.new).not_to be nil - end + describe "#connect" do + it "connects with built-in connection string" do + expect(described_class.new).not_to be nil + end - it "connects with explicit connection string" do - expect(described_class.new.connect(ENV["RIGHTS_API_DATABASE_CONNECTION_STRING"])).not_to be nil - end + it "connects with explicit connection string" do + expect(described_class.new.connect(ENV["RIGHTS_API_DATABASE_CONNECTION_STRING"])).not_to be nil + end - it "connects with connection arguments" do - ClimateControl.modify(RIGHTS_API_DATABASE_CONNECTION_STRING: nil) do - args = { - user: "ht_rights", - password: "ht_rights", - host: "mariadb", - database: "ht", - adapter: "mysql2" - } - expect(described_class.new.connect(**args)).not_to be nil - end + it "connects with connection arguments" do + ClimateControl.modify(RIGHTS_API_DATABASE_CONNECTION_STRING: nil) do + args = { + user: "ht_rights", + password: "ht_rights", + host: "mariadb", + database: "ht", + adapter: "mysql2" + } + expect(described_class.new.connect(**args)).not_to be nil end + end - it "connects with ENV variables" do - env = { - RIGHTS_API_DATABASE_CONNECTION_STRING: nil, - RIGHTS_API_DATABASE_USER: "ht_rights", - RIGHTS_API_DATABASE_PASSWORD: "ht_rights", - RIGHTS_API_DATABASE_HOST: "mariadb", - RIGHTS_API_DATABASE_DATABASE: "ht", - RIGHTS_API_DATABASE_ADAPTER: "mysql2" - } - ClimateControl.modify(**env) do - expect(described_class.new.connect).not_to be nil - end + it "connects with ENV variables" do + env = { + RIGHTS_API_DATABASE_CONNECTION_STRING: nil, + RIGHTS_API_DATABASE_USER: "ht_rights", + RIGHTS_API_DATABASE_PASSWORD: "ht_rights", + RIGHTS_API_DATABASE_HOST: "mariadb", + RIGHTS_API_DATABASE_DATABASE: "ht", + RIGHTS_API_DATABASE_ADAPTER: "mysql2" + } + ClimateControl.modify(**env) do + expect(described_class.new.connect).not_to be nil end end end