From bb1fe3865d406859a1d0a9478459129c1c79ca6a Mon Sep 17 00:00:00 2001 From: Jose Rafael Coello Alba Date: Fri, 26 Nov 2021 11:40:40 -0800 Subject: [PATCH 1/5] add eager load option + added eager load to default locators (in locate and locate_many methods) with backwards compatibility + added tests cases with includes method in use. + created a new stub class for mocking the includes behavior ~ modified Person::Child class to have a relationship called parent. --- README.md | 23 +++++++++++++-- lib/global_id/locator.rb | 48 +++++++++++++++++++++++-------- test/cases/global_locator_test.rb | 35 +++++++++++++++++++++- test/models/person.rb | 43 ++++++++++++++++++++++++++- 4 files changed, 133 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 550a6d6..27b6074 100644 --- a/README.md +++ b/README.md @@ -161,6 +161,25 @@ GlobalID::Locator.locate_many gids Note the order is maintained in the returned results. +### Options + +Either `GlobalID::Locator.locate` or `GlobalID::Locator.locate_many` supports a hash of options as second parameter. The supported options are: + +* :includes - A Symbol, Array, Hash or combination of them + The same structure you would pass into a `includes` method of Active Record. + See [Active Record eager loading associations](https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations) + If present, `locate` or `locate_many` will eager load all the relationships specified here. + Note: It only works if all the gids models have that relationships. +* :only - A class, module or Array of classes and/or modules that are + allowed to be located. Passing one or more classes limits instances of returned + classes to those classes or their subclasses. Passing one or more modules in limits + instances of returned classes to those including that module. If no classes or + modules match, +nil+ is returned. +* :ignore_missing (Only for `locate_many`) - By default, `locate_many` will call `#find` on the model to locate the + ids extracted from the GIDs. In Active Record (and other data stores following the same pattern), + `#find` will raise an exception if a named ID can't be found. When you set this option to true, + we will use `#where(id: ids)` instead, which does not raise on missing records. + ### Custom App Locator A custom locator can be set for an app by calling `GlobalID::Locator.use` and providing an app locator to use for that app. @@ -172,7 +191,7 @@ A custom locator can either be a block or a class. Using a block: ```ruby -GlobalID::Locator.use :foo do |gid| +GlobalID::Locator.use :foo do |gid, options| FooRemote.const_get(gid.model_name).find(gid.model_id) end ``` @@ -182,7 +201,7 @@ Using a class: ```ruby GlobalID::Locator.use :bar, BarLocator.new class BarLocator - def locate(gid) + def locate(gid, options = {}) @search_client.search name: gid.model_name, id: gid.model_id end end diff --git a/lib/global_id/locator.rb b/lib/global_id/locator.rb index f4dec07..0576b18 100644 --- a/lib/global_id/locator.rb +++ b/lib/global_id/locator.rb @@ -8,15 +8,20 @@ class << self # Takes either a GlobalID or a string that can be turned into a GlobalID # # Options: + # * :includes - A Symbol, Array, Hash or combination of them + # The same structure you would pass into a includes method of Active Record. + # @see https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations + # If present, locate will load all the relationships specified here. # * :only - A class, module or Array of classes and/or modules that are # allowed to be located. Passing one or more classes limits instances of returned # classes to those classes or their subclasses. Passing one or more modules in limits # instances of returned classes to those including that module. If no classes or # modules match, +nil+ is returned. def locate(gid, options = {}) - if gid = GlobalID.parse(gid) - locator_for(gid).locate gid if find_allowed?(gid.model_class, options[:only]) - end + gid = GlobalID.parse(gid) + return unless gid && find_allowed?(gid.model_class, options[:only]) + + locator_for(gid).locate(gid, options.except(:only)) end # Takes an array of GlobalIDs or strings that can be turned into a GlobalIDs. @@ -30,6 +35,11 @@ def locate(gid, options = {}) # per model class, but still interpolate the results to match the order in which the gids were passed. # # Options: + # * :includes - A Symbol, Array, Hash or combination of them + # The same structure you would pass into a includes method of Active Record. + # @see https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations + # If present, locate_many will load all the relationships specified here. + # Note: It only works if all the gids models have that relationships. # * :only - A class, module or Array of classes and/or modules that are # allowed to be located. Passing one or more classes limits instances of returned # classes to those classes or their subclasses. Passing one or more modules in limits @@ -51,6 +61,10 @@ def locate_many(gids, options = {}) # Takes either a SignedGlobalID or a string that can be turned into a SignedGlobalID # # Options: + # * :includes - A Symbol, Array, Hash or combination of them + # The same structure you would pass into a includes method of Active Record. + # @see https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations + # If present, locate_signed will load all the relationships specified here. # * :only - A class, module or Array of classes and/or modules that are # allowed to be located. Passing one or more classes limits instances of returned # classes to those classes or their subclasses. Passing one or more modules in limits @@ -68,6 +82,11 @@ def locate_signed(sgid, options = {}) # the results to match the order in which the gids were passed. # # Options: + # * :includes - A Symbol, Array, Hash or combination of them + # The same structure you would pass into a includes method of Active Record. + # @see https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations + # If present, locate_many_signed will load all the relationships specified here. + # Note: It only works if all the gids models have that relationships. # * :only - A class, module or Array of classes and/or modules that are # allowed to be located. Passing one or more classes limits instances of returned # classes to those classes or their subclasses. Passing one or more modules in limits @@ -84,7 +103,7 @@ def locate_many_signed(sgids, options = {}) # # Using a block: # - # GlobalID::Locator.use :foo do |gid| + # GlobalID::Locator.use :foo do |gid, options| # FooRemote.const_get(gid.model_name).find(gid.model_id) # end # @@ -93,7 +112,7 @@ def locate_many_signed(sgids, options = {}) # GlobalID::Locator.use :bar, BarLocator.new # # class BarLocator - # def locate(gid) + # def locate(gid, options = {}) # @search_client.search name: gid.model_name, id: gid.model_id # end # end @@ -127,9 +146,12 @@ def normalize_app(app) @locators = {} class BaseLocator - def locate(gid) + def locate(gid, options = {}) return unless model_id_is_valid?(gid) - gid.model_class.find gid.model_id + model_class = gid.model_class + model_class = model_class.includes(options[:includes]) if options[:includes] + + model_class.find gid.model_id end def locate_many(gids, options = {}) @@ -143,7 +165,7 @@ def locate_many(gids, options = {}) records_by_model_name_and_id = {} ids_by_model.each do |model, ids| - records = find_records(model, ids, ignore_missing: options[:ignore_missing]) + records = find_records(model, ids, ignore_missing: options[:ignore_missing], includes: options[:includes]) records_by_id = records.index_by do |record| record.id.is_a?(Array) ? record.id.map(&:to_s) : record.id.to_s @@ -157,6 +179,8 @@ def locate_many(gids, options = {}) private def find_records(model_class, ids, options) + model_class = model_class.includes(options[:includes]) if options[:includes] + if options[:ignore_missing] model_class.where(model_class.primary_key => ids) else @@ -170,7 +194,7 @@ def model_id_is_valid?(gid) end class UnscopedLocator < BaseLocator - def locate(gid) + def locate(gid, options = {}) unscoped(gid.model_class) { super } end @@ -194,12 +218,12 @@ def initialize(block) @locator = block end - def locate(gid) - @locator.call(gid) + def locate(gid, options = {}) + @locator.call(gid, options) end def locate_many(gids, options = {}) - gids.map { |gid| locate(gid) } + gids.map { |gid| locate(gid, options) } end end end diff --git a/test/cases/global_locator_test.rb b/test/cases/global_locator_test.rb index ac3b8ea..fadadbf 100644 --- a/test/cases/global_locator_test.rb +++ b/test/cases/global_locator_test.rb @@ -64,6 +64,23 @@ class GlobalLocatorTest < ActiveSupport::TestCase assert_equal @gid.model_id, found.id end + test 'by GID with eager loading' do + assert_equal Person::Child.new('1', Person.new('1')), + GlobalID::Locator.locate( + Person::Child.new('1', Person.new('1')).to_gid, + includes: :parent + ) + end + + test 'by GID trying to eager load an unexisting relationship' do + assert_raises StandardError do + GlobalID::Locator.locate( + Person::Child.new('1', Person.new('1')).to_gid, + includes: :some_non_existent_relationship + ) + end + end + test 'by many GIDs of one class' do assert_equal [ Person.new('1'), Person.new('2') ], GlobalID::Locator.locate_many([ Person.new('1').to_gid, Person.new('2').to_gid ]) @@ -91,6 +108,22 @@ class GlobalLocatorTest < ActiveSupport::TestCase GlobalID::Locator.locate_many([ Person.new('1').to_gid, Person::Child.new('1').to_gid, Person.new('2').to_gid ], only: Person::Child) end + test 'by many GIDs with eager loading' do + assert_equal [ Person::Child.new('1', Person.new('1')), Person::Child.new('2', Person.new('2')) ], + GlobalID::Locator.locate_many( + [ Person::Child.new('1', Person.new('1')).to_gid, Person::Child.new('2', Person.new('2')).to_gid ], + includes: :parent + ) + end + + test 'by many GIDs trying to eager load an unexisting relationship' do + assert_raises StandardError do + GlobalID::Locator.locate_many( + [ Person::Child.new('1', Person.new('1')).to_gid, Person::Child.new('2', Person.new('2')).to_gid ], + includes: :some_non_existent_relationship + ) + end + end test 'by SGID' do found = GlobalID::Locator.locate_signed(@sgid) @@ -236,7 +269,7 @@ class GlobalLocatorTest < ActiveSupport::TestCase test 'use locator with class' do class BarLocator - def locate(gid); :bar; end + def locate(gid, options = {}); :bar; end def locate_many(gids, options = {}); gids.map(&:model_id); end end diff --git a/test/models/person.rb b/test/models/person.rb index ebf3d5e..1a4d546 100644 --- a/test/models/person.rb +++ b/test/models/person.rb @@ -53,4 +53,45 @@ def self.find(*) end end -class Person::Child < Person; end +class Person::ChildWithParent < Person + def self.find(id_or_ids) + if id_or_ids.is_a? Array + ids = id_or_ids + ids.collect { |id| find(id) } + else + id = id_or_ids + + if id == HARDCODED_ID_FOR_MISSING_PERSON + raise 'Person missing' + else + Person::Child.new(id, Person.new(id)) + end + end + end + + def self.where(conditions) + (conditions[:id] - [HARDCODED_ID_FOR_MISSING_PERSON]).collect do |id| + Person::Child.new(id, Person.new(id)) + end + end +end + +class Person::Child < Person + attr_accessor :parent + + def initialize(id = 1, parent = nil) + @id = id + @parent = parent + end + + def self.includes(relationships) + return Person::ChildWithParent if relationships == :parent + return self if relationships.nil? + + raise StandardError, 'Relationship does not exist' + end + + def ==(other) + other.is_a?(self.class) && id == other.try(:id) && parent == other.parent + end +end From 3bdfc3ae39095474ad68bb4129599ee694618b04 Mon Sep 17 00:00:00 2001 From: Jose Rafael Coello Alba Date: Thu, 12 Jan 2023 10:58:07 -0800 Subject: [PATCH 2/5] deprecate locate with one argument --- Gemfile.lock | 3 +++ lib/global_id/locator.rb | 7 ++++++- test/cases/global_locator_test.rb | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index cb2cfbb..e1cd4e5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -39,6 +39,8 @@ GEM method_source (1.0.0) mini_portile2 (2.8.4) minitest (5.19.0) + nokogiri (1.14.0-aarch64-linux) + racc (~> 1.4) nokogiri (1.15.4) mini_portile2 (~> 2.8.2) racc (~> 1.4) @@ -67,6 +69,7 @@ GEM zeitwerk (2.6.11) PLATFORMS + aarch64-linux ruby DEPENDENCIES diff --git a/lib/global_id/locator.rb b/lib/global_id/locator.rb index 0576b18..ec8d69e 100644 --- a/lib/global_id/locator.rb +++ b/lib/global_id/locator.rb @@ -21,7 +21,12 @@ def locate(gid, options = {}) gid = GlobalID.parse(gid) return unless gid && find_allowed?(gid.model_class, options[:only]) - locator_for(gid).locate(gid, options.except(:only)) + if locator_for(gid).method(:locate).arity == 1 + ActiveSupport::Deprecation.warn "Calling `locate(gid)' is deprecated. Please use `locate(gid, options)' instead." + locator_for(gid).locate(gid) + else + locator_for(gid).locate(gid, options.except(:only)) + end end # Takes an array of GlobalIDs or strings that can be turned into a GlobalIDs. diff --git a/test/cases/global_locator_test.rb b/test/cases/global_locator_test.rb index fadadbf..cba1831 100644 --- a/test/cases/global_locator_test.rb +++ b/test/cases/global_locator_test.rb @@ -281,6 +281,21 @@ def locate_many(gids, options = {}); gids.map(&:model_id); end end end + test 'use locator with class and single argument' do + class DeprecatedBarLocator + def locate(gid); :bar; end + def locate_many(gids, options = {}); gids.map(&:model_id); end + end + + GlobalID::Locator.use :bar, DeprecatedBarLocator.new + + with_app 'bar' do + assert_output(nil, /.*Calling `locate\(gid\)' is deprecated\. Please use `locate\(gid, options\)' instead\..*/) { GlobalID::Locator.locate('gid://bar/Person/1') } + assert_equal :bar, GlobalID::Locator.locate('gid://bar/Person/1') + assert_equal ['1', '2'], GlobalID::Locator.locate_many(['gid://bar/Person/1', 'gid://bar/Person/2']) + end + end + test 'app locator is case insensitive' do GlobalID::Locator.use :insensitive do |gid| :insensitive From 6016b0e7f5d291fe1e9e49cd3d6b005347b0d754 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 30 Aug 2023 18:46:46 +0000 Subject: [PATCH 3/5] Define GlobalID deprecator --- lib/global_id.rb | 4 ++++ lib/global_id/locator.rb | 11 +++++++---- test/cases/global_locator_test.rb | 13 +++++++------ 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/global_id.rb b/lib/global_id.rb index eeb013f..7692b90 100644 --- a/lib/global_id.rb +++ b/lib/global_id.rb @@ -16,4 +16,8 @@ def self.eager_load! super require 'global_id/signed_global_id' end + + def self.deprecator # :nodoc: + @deprecator ||= ActiveSupport::Deprecation.new("2.1", "GlobalID") + end end diff --git a/lib/global_id/locator.rb b/lib/global_id/locator.rb index ec8d69e..0072a33 100644 --- a/lib/global_id/locator.rb +++ b/lib/global_id/locator.rb @@ -19,13 +19,16 @@ class << self # modules match, +nil+ is returned. def locate(gid, options = {}) gid = GlobalID.parse(gid) + return unless gid && find_allowed?(gid.model_class, options[:only]) - if locator_for(gid).method(:locate).arity == 1 - ActiveSupport::Deprecation.warn "Calling `locate(gid)' is deprecated. Please use `locate(gid, options)' instead." - locator_for(gid).locate(gid) + locator = locator_for(gid) + + if locator.method(:locate).arity == 1 + GlobalID.deprecator.warn "It seems your locator is defining the `locate` method only with one argument. Please make sure your locator is receiving the options argument as well, like `locate(gid, options = {})`." + locator.locate(gid) else - locator_for(gid).locate(gid, options.except(:only)) + locator.locate(gid, options.except(:only)) end end diff --git a/test/cases/global_locator_test.rb b/test/cases/global_locator_test.rb index cba1831..955a57c 100644 --- a/test/cases/global_locator_test.rb +++ b/test/cases/global_locator_test.rb @@ -283,16 +283,17 @@ def locate_many(gids, options = {}); gids.map(&:model_id); end test 'use locator with class and single argument' do class DeprecatedBarLocator - def locate(gid); :bar; end + def locate(gid); :deprecated; end def locate_many(gids, options = {}); gids.map(&:model_id); end end - GlobalID::Locator.use :bar, DeprecatedBarLocator.new + GlobalID::Locator.use :deprecated, DeprecatedBarLocator.new - with_app 'bar' do - assert_output(nil, /.*Calling `locate\(gid\)' is deprecated\. Please use `locate\(gid, options\)' instead\..*/) { GlobalID::Locator.locate('gid://bar/Person/1') } - assert_equal :bar, GlobalID::Locator.locate('gid://bar/Person/1') - assert_equal ['1', '2'], GlobalID::Locator.locate_many(['gid://bar/Person/1', 'gid://bar/Person/2']) + with_app 'deprecated' do + assert_deprecated(nil, GlobalID.deprecator) do + assert_equal :deprecated, GlobalID::Locator.locate('gid://deprecated/Person/1') + end + assert_equal ['1', '2'], GlobalID::Locator.locate_many(['gid://deprecated/Person/1', 'gid://deprecated/Person/2']) end end From 18fd60cc0c9546abc2334614bb5b622249c54847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 30 Aug 2023 18:47:11 +0000 Subject: [PATCH 4/5] Copy edit documentation --- lib/global_id/locator.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/global_id/locator.rb b/lib/global_id/locator.rb index 0072a33..ca9dfe6 100644 --- a/lib/global_id/locator.rb +++ b/lib/global_id/locator.rb @@ -8,10 +8,10 @@ class << self # Takes either a GlobalID or a string that can be turned into a GlobalID # # Options: - # * :includes - A Symbol, Array, Hash or combination of them - # The same structure you would pass into a includes method of Active Record. - # @see https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations + # * :includes - A Symbol, Array, Hash or combination of them. + # The same structure you would pass into a +includes+ method of Active Record. # If present, locate will load all the relationships specified here. + # See https://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations. # * :only - A class, module or Array of classes and/or modules that are # allowed to be located. Passing one or more classes limits instances of returned # classes to those classes or their subclasses. Passing one or more modules in limits From 98e02beccf795ed54bd5bf7bf5f7ffd359871267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 30 Aug 2023 18:48:48 +0000 Subject: [PATCH 5/5] Register global_id deprecator in the app --- lib/global_id/railtie.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/global_id/railtie.rb b/lib/global_id/railtie.rb index f02e1fd..49e0dd3 100644 --- a/lib/global_id/railtie.rb +++ b/lib/global_id/railtie.rb @@ -42,6 +42,10 @@ class Railtie < Rails::Railtie # :nodoc: send :extend, GlobalID::FixtureSet end end + + initializer "web_console.deprecator" do |app| + app.deprecators[:global_id] = GlobalID.deprecator if app.respond_to?(:deprecators) + end end end