From 5ce154c0eaacdde7ce86e781a25336e000db316f Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Wed, 16 Aug 2023 17:03:37 -0400 Subject: [PATCH] Allow for composite identifiers delimited by `/` This commit extends global id to allow representing models with composite identifiers. The value will be joined by `/`. For example: Given a `TravelRoute` model with `origin` and `destination` as the compsoite primary key, the global id will be represented as: ``` TravelRoute.new(origin: "Ottawa", destination: "New York").to_global_id => gid://app/TravelRoute/Ottawa/New%20York ``` Co-authored-by: Adrianna Chang Co-authored-by: Nikita Vasilevsky --- lib/global_id/global_id.rb | 9 ++-- lib/global_id/locator.rb | 38 +++++++++++++--- lib/global_id/uri/gid.rb | 37 ++++++++++++--- test/cases/global_id_test.rb | 18 +++++++- test/cases/global_locator_test.rb | 52 ++++++++++++++++++++++ test/cases/uri_gid_test.rb | 51 +++++++++++++++++++-- test/helper.rb | 1 + test/models/composite_primary_key_model.rb | 42 +++++++++++++++++ test/models/person.rb | 4 ++ test/models/person_model.rb | 4 ++ 10 files changed, 234 insertions(+), 22 deletions(-) create mode 100644 test/models/composite_primary_key_model.rb diff --git a/lib/global_id/global_id.rb b/lib/global_id/global_id.rb index 5cd055d..1082326 100644 --- a/lib/global_id/global_id.rb +++ b/lib/global_id/global_id.rb @@ -50,12 +50,13 @@ def find(options = {}) end def model_class - model = model_name.constantize + @model_class ||= begin + model = model_name.constantize - unless model <= GlobalID + if model <= GlobalID + raise ArgumentError, "GlobalID and SignedGlobalID cannot be used as model_class." + end model - else - raise ArgumentError, "GlobalID and SignedGlobalID cannot be used as model_class." end end diff --git a/lib/global_id/locator.rb b/lib/global_id/locator.rb index dcdd15b..71687a3 100644 --- a/lib/global_id/locator.rb +++ b/lib/global_id/locator.rb @@ -2,6 +2,8 @@ class GlobalID module Locator + class InvalidModelIdError < StandardError; end + class << self # Takes either a GlobalID or a string that can be turned into a GlobalID # @@ -126,27 +128,49 @@ def normalize_app(app) class BaseLocator def locate(gid) + return unless model_id_is_valid?(gid) gid.model_class.find gid.model_id end def locate_many(gids, options = {}) - models_and_ids = gids.collect { |gid| [ gid.model_class, gid.model_id ] } - ids_by_model = models_and_ids.group_by(&:first) - loaded_by_model = Hash[ids_by_model.map { |model, ids| - [ model, find_records(model, ids.map(&:last), ignore_missing: options[:ignore_missing]).index_by { |record| record.id.to_s } ] - }] + ids_by_model = Hash.new { |hash, key| hash[key] = [] } + + gids.each do |gid| + next unless model_id_is_valid?(gid) + ids_by_model[gid.model_class] << gid.model_id + end + + records_by_model_name_and_id = {} + ids_by_model.each do |model, ids| + + records = find_records(model, ids, ignore_missing: options[:ignore_missing]) + + records_by_id = records.index_by do |record| + record.id.is_a?(Array) ? record.id.map(&:to_s) : record.id.to_s + end + + records_by_model_name_and_id[model.name] = records_by_id + end - models_and_ids.collect { |(model, id)| loaded_by_model[model][id] }.compact + gids.filter_map { |gid| records_by_model_name_and_id[gid.model_name][gid.model_id] } end private def find_records(model_class, ids, options) if options[:ignore_missing] - model_class.where(id: ids) + model_class.where(model_class.primary_key => ids) else model_class.find(ids) end end + + private + def model_id_is_valid?(gid) + primary_key = Array(gid.model_class.primary_key) + primary_key_size = primary_key.size + + Array(gid.model_id).size == primary_key_size + end end class UnscopedLocator < BaseLocator diff --git a/lib/global_id/uri/gid.rb b/lib/global_id/uri/gid.rb index 3fadf4f..20a22a7 100644 --- a/lib/global_id/uri/gid.rb +++ b/lib/global_id/uri/gid.rb @@ -30,6 +30,11 @@ class GID < Generic # Raised when creating a Global ID for a model without an id class MissingModelIdError < URI::InvalidComponentError; end + class InvalidModelIdError < URI::InvalidComponentError; end + + # Maximum size of a model id segment + COMPOSITE_MODEL_ID_MAX_SIZE = 20 + COMPOSITE_MODEL_ID_DELIMITER = "/" class << self # Validates +app+'s as URI hostnames containing only alphanumeric characters @@ -83,7 +88,8 @@ def create(app, model, params = nil) def build(args) parts = Util.make_components_hash(self, args) parts[:host] = parts[:app] - parts[:path] = "/#{parts[:model_name]}/#{CGI.escape(parts[:model_id].to_s)}" + model_id_segment = Array(parts[:model_id]).map { |p| CGI.escape(p.to_s) }.join(COMPOSITE_MODEL_ID_DELIMITER) + parts[:path] = "/#{parts[:model_name]}/#{model_id_segment}" if parts[:params] && !parts[:params].empty? parts[:query] = URI.encode_www_form(parts[:params]) @@ -147,12 +153,22 @@ def check_scheme(scheme) def set_model_components(path, validate = false) _, model_name, model_id = path.split('/', 3) - validate_component(model_name) && validate_model_id(model_id, model_name) if validate - - model_id = CGI.unescape(model_id) if model_id + validate_component(model_name) && validate_model_id_section(model_id, model_name) if validate @model_name = model_name - @model_id = model_id + + if model_id + model_id_parts = model_id + .split(COMPOSITE_MODEL_ID_DELIMITER, COMPOSITE_MODEL_ID_MAX_SIZE) + .reject(&:blank?) + + model_id_parts.map! do |id| + validate_model_id(id) + CGI.unescape(id) + end + + @model_id = model_id_parts.length == 1 ? model_id_parts.first : model_id_parts + end end def validate_component(component) @@ -162,13 +178,20 @@ def validate_component(component) "Expected a URI like gid://app/Person/1234: #{inspect}" end - def validate_model_id(model_id, model_name) - return model_id unless model_id.blank? || model_id.include?('/') + def validate_model_id_section(model_id, model_name) + return model_id unless model_id.blank? raise MissingModelIdError, "Unable to create a Global ID for " \ "#{model_name} without a model id." end + def validate_model_id(model_id_part) + return unless model_id_part.include?('/') + + raise InvalidModelIdError, "Unable to create a Global ID for " \ + "#{model_name} with a malformed model id." + end + def parse_query_params(query) Hash[URI.decode_www_form(query)].with_indifferent_access if query end diff --git a/test/cases/global_id_test.rb b/test/cases/global_id_test.rb index 041c9de..44ef9a4 100644 --- a/test/cases/global_id_test.rb +++ b/test/cases/global_id_test.rb @@ -2,7 +2,7 @@ class GlobalIDTest < ActiveSupport::TestCase test 'value equality' do - assert_equal GlobalID.new('gid://app/model/id'), GlobalID.new('gid://app/model/id') + assert_equal GlobalID.new('gid://app/Person/5'), GlobalID.new('gid://app/Person/5') end test 'invalid app name' do @@ -44,6 +44,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase @person_uuid_gid = GlobalID.create(Person.new(@uuid)) @person_namespaced_gid = GlobalID.create(Person::Child.new(4)) @person_model_gid = GlobalID.create(PersonModel.new(id: 1)) + @cpk_model_gid = GlobalID.create(CompositePrimaryKeyModel.new(id: ["tenant-key-value", "id-value"])) end test 'find' do @@ -51,12 +52,14 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal Person.find(@person_uuid_gid.model_id), @person_uuid_gid.find assert_equal Person::Child.find(@person_namespaced_gid.model_id), @person_namespaced_gid.find assert_equal PersonModel.find(@person_model_gid.model_id), @person_model_gid.find + assert_equal CompositePrimaryKeyModel.find(@cpk_model_gid.model_id), @cpk_model_gid.find end test 'find with class' do assert_equal Person.find(@person_gid.model_id), @person_gid.find(only: Person) assert_equal Person.find(@person_uuid_gid.model_id), @person_uuid_gid.find(only: Person) assert_equal PersonModel.find(@person_model_gid.model_id), @person_model_gid.find(only: PersonModel) + assert_equal CompositePrimaryKeyModel.find(@cpk_model_gid.model_id), @cpk_model_gid.find(only: CompositePrimaryKeyModel) end test 'find with class no match' do @@ -64,6 +67,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_nil @person_uuid_gid.find(only: Array) assert_nil @person_namespaced_gid.find(only: String) assert_nil @person_model_gid.find(only: Float) + assert_nil @cpk_model_gid.find(only: Hash) end test 'find with subclass' do @@ -135,6 +139,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal "gid://bcx/Person/#{@uuid}", @person_uuid_gid.to_s assert_equal 'gid://bcx/Person::Child/4', @person_namespaced_gid.to_s assert_equal 'gid://bcx/PersonModel/1', @person_model_gid.to_s + assert_equal 'gid://bcx/CompositePrimaryKeyModel/tenant-key-value/id-value', @cpk_model_gid.to_s end test 'as param' do @@ -149,6 +154,10 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal 'Z2lkOi8vYmN4L1BlcnNvbk1vZGVsLzE', @person_model_gid.to_param assert_equal @person_model_gid, GlobalID.parse('Z2lkOi8vYmN4L1BlcnNvbk1vZGVsLzE') + + expected_encoded = 'Z2lkOi8vYmN4L0NvbXBvc2l0ZVByaW1hcnlLZXlNb2RlbC90ZW5hbnQta2V5LXZhbHVlL2lkLXZhbHVl' + assert_equal expected_encoded, @cpk_model_gid.to_param + assert_equal @cpk_model_gid, GlobalID.parse(expected_encoded) end test 'as URI' do @@ -156,6 +165,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal URI("gid://bcx/Person/#{@uuid}"), @person_uuid_gid.uri assert_equal URI('gid://bcx/Person::Child/4'), @person_namespaced_gid.uri assert_equal URI('gid://bcx/PersonModel/1'), @person_model_gid.uri + assert_equal URI('gid://bcx/CompositePrimaryKeyModel/tenant-key-value/id-value'), @cpk_model_gid.uri end test 'as JSON' do @@ -170,6 +180,9 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal 'gid://bcx/PersonModel/1', @person_model_gid.as_json assert_equal '"gid://bcx/PersonModel/1"', @person_model_gid.to_json + + assert_equal 'gid://bcx/CompositePrimaryKeyModel/tenant-key-value/id-value', @cpk_model_gid.as_json + assert_equal '"gid://bcx/CompositePrimaryKeyModel/tenant-key-value/id-value"', @cpk_model_gid.to_json end test 'model id' do @@ -177,6 +190,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal @uuid, @person_uuid_gid.model_id assert_equal '4', @person_namespaced_gid.model_id assert_equal '1', @person_model_gid.model_id + assert_equal ['tenant-key-value', 'id-value'], @cpk_model_gid.model_id end test 'model name' do @@ -184,6 +198,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal 'Person', @person_uuid_gid.model_name assert_equal 'Person::Child', @person_namespaced_gid.model_name assert_equal 'PersonModel', @person_model_gid.model_name + assert_equal 'CompositePrimaryKeyModel', @cpk_model_gid.model_name end test 'model class' do @@ -191,6 +206,7 @@ class GlobalIDCreationTest < ActiveSupport::TestCase assert_equal Person, @person_uuid_gid.model_class assert_equal Person::Child, @person_namespaced_gid.model_class assert_equal PersonModel, @person_model_gid.model_class + assert_equal CompositePrimaryKeyModel, @cpk_model_gid.model_class assert_raise ArgumentError do GlobalID.find 'gid://bcx/SignedGlobalID/5' end diff --git a/test/cases/global_locator_test.rb b/test/cases/global_locator_test.rb index 0a10c36..ac3b8ea 100644 --- a/test/cases/global_locator_test.rb +++ b/test/cases/global_locator_test.rb @@ -5,6 +5,9 @@ class GlobalLocatorTest < ActiveSupport::TestCase model = Person.new('id') @gid = model.to_gid @sgid = model.to_sgid + @cpk_model = CompositePrimaryKeyModel.new(id: ["tenant-key-value", "id-value"]) + @cpk_gid = @cpk_model.to_gid + @cpk_sgid = @cpk_model.to_sgid end test 'by GID' do @@ -13,6 +16,12 @@ class GlobalLocatorTest < ActiveSupport::TestCase assert_equal @gid.model_id, found.id end + test 'composite primary key model by GID' do + found = GlobalID::Locator.locate(@cpk_gid) + assert_kind_of @cpk_gid.model_class, found + assert_equal ["tenant-key-value", "id-value"], found.id + end + test 'by GID with only: restriction with match' do found = GlobalID::Locator.locate(@gid, only: Person) assert_kind_of @gid.model_class, found @@ -60,6 +69,18 @@ class GlobalLocatorTest < ActiveSupport::TestCase GlobalID::Locator.locate_many([ Person.new('1').to_gid, Person.new('2').to_gid ]) end + test '#locate_many by composite primary key GIDs of the same class' do + records = [ @cpk_model, CompositePrimaryKeyModel.new(id: ["tenant-key-value2", "id-value2"]) ] + located = GlobalID::Locator.locate_many(records.map(&:to_gid)) + assert_equal records, located + end + + test '#locate_many by composite primary key GIDs of different classes' do + records = [ @cpk_model, Person.new('1') ] + located = GlobalID::Locator.locate_many(records.map(&:to_gid)) + assert_equal records, located + end + test 'by many GIDs of mixed classes' do assert_equal [ Person.new('1'), Person::Child.new('1'), Person.new('2') ], GlobalID::Locator.locate_many([ Person.new('1').to_gid, Person::Child.new('1').to_gid, Person.new('2').to_gid ]) @@ -77,6 +98,12 @@ class GlobalLocatorTest < ActiveSupport::TestCase assert_equal @sgid.model_id, found.id end + test 'by SGID of a composite primary key model' do + found = GlobalID::Locator.locate_signed(@cpk_sgid) + assert_kind_of @cpk_sgid.model_class, found + assert_equal @cpk_sgid.model_id, found.id + end + test 'by SGID with only: restriction with match' do found = GlobalID::Locator.locate_signed(@sgid, only: Person) assert_kind_of @sgid.model_class, found @@ -124,11 +151,23 @@ class GlobalLocatorTest < ActiveSupport::TestCase GlobalID::Locator.locate_many_signed([ Person.new('1').to_sgid, Person.new('2').to_sgid ]) end + test 'by many SGIDs of the same composite primary key class' do + records = [ @cpk_model, CompositePrimaryKeyModel.new(id: ["tenant-key-value2", "id-value2"]) ] + located = GlobalID::Locator.locate_many_signed(records.map(&:to_sgid)) + assert_equal records, located + end + test 'by many SGIDs of mixed classes' do assert_equal [ Person.new('1'), Person::Child.new('1'), Person.new('2') ], GlobalID::Locator.locate_many_signed([ Person.new('1').to_sgid, Person::Child.new('1').to_sgid, Person.new('2').to_sgid ]) end + test 'by many SGIDs of composite primary key model mixed with other models' do + records = [ @cpk_model, Person.new('1') ] + located = GlobalID::Locator.locate_many_signed(records.map(&:to_sgid)) + assert_equal records, located + end + test 'by many SGIDs with only: restriction to match subclass' do assert_equal [ Person::Child.new('1') ], GlobalID::Locator.locate_many_signed([ Person.new('1').to_sgid, Person::Child.new('1').to_sgid, Person.new('2').to_sgid ], only: Person::Child) @@ -157,6 +196,12 @@ class GlobalLocatorTest < ActiveSupport::TestCase assert_equal @gid.model_id, found.id end + test 'by to_param encoding for a composite primary key model' do + found = GlobalID::Locator.locate(@cpk_gid.to_param) + assert_kind_of @cpk_gid.model_class, found + assert_equal @cpk_gid.model_id, found.id + end + test 'by non-GID returns nil' do assert_nil GlobalID::Locator.locate 'This is not a GID' end @@ -172,6 +217,13 @@ class GlobalLocatorTest < ActiveSupport::TestCase assert_nil GlobalID::Locator.locate 'gid://app/Person/1/2' end + test 'locating by a GID URI with a mismatching model_id returns nil' do + assert_nil GlobalID::Locator.locate 'gid://app/Person/1/2' + assert_nil GlobalID::Locator.locate 'gid://app/CompositePrimaryKeyModel/tenant-key-value/id-value/something_else' + assert_nil GlobalID::Locator.locate 'gid://app/CompositePrimaryKeyModel/tenant-key-value/' + assert_nil GlobalID::Locator.locate 'gid://app/CompositePrimaryKeyModel/tenant-key-value' + end + test 'use locator with block' do GlobalID::Locator.use :foo do |gid| :foo diff --git a/test/cases/uri_gid_test.rb b/test/cases/uri_gid_test.rb index 8f68151..3c3b3fa 100644 --- a/test/cases/uri_gid_test.rb +++ b/test/cases/uri_gid_test.rb @@ -4,12 +4,25 @@ class URI::GIDTest < ActiveSupport::TestCase setup do @gid_string = 'gid://bcx/Person/5' @gid = URI::GID.parse(@gid_string) + @cpk_gid_string = 'gid://bcx/CompositePrimaryKeyModel/tenant-key-value/id-value' + @cpk_gid = URI::GID.parse(@cpk_gid_string) end test 'parsed' do assert_equal @gid.app, 'bcx' assert_equal @gid.model_name, 'Person' assert_equal @gid.model_id, '5' + assert_equal ["tenant-key-value", "id-value"], @cpk_gid.model_id + end + + test 'parsed for non existing model class' do + flat_id_gid = URI::GID.parse("gid://bcx/NonExistingModel/1") + assert_equal("1", flat_id_gid.model_id) + assert_equal("NonExistingModel", flat_id_gid.model_name) + + composite_id_gid = URI::GID.parse("gid://bcx/NonExistingModel/tenant-key-value/id-value") + assert_equal(["tenant-key-value", "id-value"], composite_id_gid.model_id) + assert_equal("NonExistingModel", composite_id_gid.model_name) end test 'new returns invalid gid when not checking' do @@ -21,6 +34,11 @@ class URI::GIDTest < ActiveSupport::TestCase assert_equal @gid_string, URI::GID.create('bcx', model).to_s end + test 'create from a composite primary key model' do + model = CompositePrimaryKeyModel.new(id: ["tenant-key-value", "id-value"]) + assert_equal @cpk_gid_string, URI::GID.create('bcx', model).to_s + end + test 'build' do array = URI::GID.build(['bcx', 'Person', '5', nil]) assert array @@ -31,6 +49,22 @@ class URI::GIDTest < ActiveSupport::TestCase assert_equal array, hash end + test 'build with a composite primary key' do + array = URI::GID.build(['bcx', 'CompositePrimaryKeyModel', ["tenant-key-value", "id-value"], nil]) + assert array + + hash = URI::GID.build( + app: 'bcx', + model_name: 'CompositePrimaryKeyModel', + model_id: ["tenant-key-value", "id-value"], + params: nil + ) + assert hash + + assert_equal array, hash + assert_equal("gid://bcx/CompositePrimaryKeyModel/tenant-key-value/id-value", array.to_s) + end + test 'build with wrong ordered array creates a wrong ordered gid' do assert_not_equal @gid_string, URI::GID.build(['Person', '5', 'bcx', nil]).to_s end @@ -55,6 +89,11 @@ class URI::GIDModelIDEncodingTest < ActiveSupport::TestCase model = Person.new('John Doe-Smith/Jones') assert_equal 'gid://app/Person/John+Doe-Smith%2FJones', URI::GID.create('app', model).to_s end + + test 'every part of composite primary key is encoded' do + model = CompositePrimaryKeyModel.new(id: ["tenant key", "id value"]) + assert_equal 'gid://app/CompositePrimaryKeyModel/tenant+key/id+value', URI::GID.create('app', model).to_s + end end class URI::GIDModelIDDecodingTest < ActiveSupport::TestCase @@ -65,6 +104,11 @@ class URI::GIDModelIDDecodingTest < ActiveSupport::TestCase test 'non-alphanumeric' do assert_equal 'John Doe-Smith/Jones', URI::GID.parse('gid://app/Person/John+Doe-Smith%2FJones').model_id end + + test 'every part of composite primary key is decoded' do + gid = 'gid://app/CompositePrimaryKeyModel/tenant+key+value/id+value' + assert_equal ['tenant key value', 'id value'], URI::GID.parse(gid).model_id + end end class URI::GIDValidationTest < ActiveSupport::TestCase @@ -81,8 +125,9 @@ class URI::GIDValidationTest < ActiveSupport::TestCase assert_match(/Unable to create a Global ID for Person/, err.message) end - test 'too many model ids' do - assert_invalid_component 'gid://bcx/Person/1/2' + test 'missing model composite id' do + err = assert_raise(URI::GID::MissingModelIdError) { URI::GID.parse('gid://bcx/CompositePrimaryKeyModel') } + assert_match(/Unable to create a Global ID for CompositePrimaryKeyModel/, err.message) end test 'empty' do @@ -112,7 +157,7 @@ class URI::GIDAppValidationTest < ActiveSupport::TestCase end test 'apps containing non alphanumeric characters are invalid' do - assert_invalid_app 'foo/bar' + assert_invalid_app 'foo&bar' assert_invalid_app 'foo:bar' assert_invalid_app 'foo_bar' end diff --git a/test/helper.rb b/test/helper.rb index e93f118..3cdeb65 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -6,6 +6,7 @@ require 'global_id' require 'models/person' require 'models/person_model' +require 'models/composite_primary_key_model' require 'json' diff --git a/test/models/composite_primary_key_model.rb b/test/models/composite_primary_key_model.rb new file mode 100644 index 0000000..edd4c24 --- /dev/null +++ b/test/models/composite_primary_key_model.rb @@ -0,0 +1,42 @@ +require 'active_model' + +class CompositePrimaryKeyModel + include ActiveModel::Model + include GlobalID::Identification + + attr_accessor :id + + def self.primary_key + [:tenant_key, :id] + end + + def self.find(id_or_ids) + raise "id is not composite" unless id_or_ids.is_a?(Array) + multi_record_fetch = id_or_ids.first.is_a?(Array) + if multi_record_fetch + id_or_ids.map do |id| + raise "id doesn't match primary key #{primary_key}" if id.size != primary_key.size + new id: id + end + else + raise "id doesn't match primary key #{primary_key}" if id_or_ids.size != primary_key.size + new id: id_or_ids + end + end + + def where(conditions) + keys = conditions.keys + raise "only primary key condition was expected" if keys.size != 1 + pk = keys.first + raise "key is not the `#{primary_key}` primary key" if pk != primary_key + + conditions[pk].map do |id| + raise "id doesn't match primary key #{primary_key}" if id.size != primary_key.size + new id: id + end + end + + def ==(other) + id == other.try(:id) + end +end diff --git a/test/models/person.rb b/test/models/person.rb index b2ef1df..ebf3d5e 100644 --- a/test/models/person.rb +++ b/test/models/person.rb @@ -5,6 +5,10 @@ class Person attr_reader :id + def self.primary_key + :id + end + def self.find(id_or_ids) if id_or_ids.is_a? Array ids = id_or_ids diff --git a/test/models/person_model.rb b/test/models/person_model.rb index f955fd8..734d78b 100644 --- a/test/models/person_model.rb +++ b/test/models/person_model.rb @@ -6,6 +6,10 @@ class PersonModel attr_accessor :id + def self.primary_key + :id + end + def self.find(id) new id: id end