Merge pull request #163 from Shopify/ac-gid-cpk

Allow for composite identifiers delimited by `/`

Author: rafaelfranca

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
 

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

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

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,26 +44,30 @@ 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
     assert_equal Person.find(@person_gid.model_id), @person_gid.find
     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
     assert_nil @person_gid.find(only: Hash)
     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,13 +154,18 @@ 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
     assert_equal URI('gid://bcx/Person/5'), @person_gid.uri
     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,27 +180,33 @@ 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
     assert_equal '5', @person_gid.model_id
     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
     assert_equal 'Person', @person_gid.model_name
     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
     assert_equal Person, @person_gid.model_class
     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

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