From f81b6d92369937c6f8e0b0e4ae65961b4586cb25 Mon Sep 17 00:00:00 2001 From: Shuya Honda Date: Mon, 8 Sep 2025 15:06:11 +0900 Subject: [PATCH 1/2] Enhance SCIM mixin documentation and add support for find_all_with method --- app/models/scimitar/resources/mixin.rb | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/app/models/scimitar/resources/mixin.rb b/app/models/scimitar/resources/mixin.rb index d11ce1b..16ef902 100644 --- a/app/models/scimitar/resources/mixin.rb +++ b/app/models/scimitar/resources/mixin.rb @@ -145,7 +145,8 @@ module Resources # display: :full_name # <-- i.e. Team.users[n].full_name # }, # class: Team, # Optional; see below - # find_with: -> (scim_list_entry) {...} # See below + # find_with: -> (scim_list_entry) {...}, # See below + # find_all_with: -> (scim_list_entries) {...} # Optional, See below # } # ], # #... @@ -165,6 +166,11 @@ module Resources # Scimitar::EngineConfiguration::schema_list_from_attribute_mappings is # defined; see documentation of that option for more information. # + # To avoid N+1 queries when resolving many entries (e.g. Group members + # during PATCH), you can instead provide ":find_all_with" which is passed + # the entire Array of SCIM entries and should return an Array of resolved + # model instances. If both are provided, ":find_all_with" is preferred. + # # Note that you can only use either: # # * One or more static maps where each matches some other piece of source @@ -315,7 +321,7 @@ def scim_mutable_attributes enum.each do | static_or_dynamic_mapping | if static_or_dynamic_mapping.key?(:match) # Static extractor.call(static_or_dynamic_mapping[:using]) - elsif static_or_dynamic_mapping.key?(:find_with) # Dynamic + elsif static_or_dynamic_mapping.key?(:find_with) || static_or_dynamic_mapping.key?(:find_all_with) # Dynamic @scim_mutable_attributes << static_or_dynamic_mapping[:list] end end @@ -839,9 +845,17 @@ def from_scim_backend!( method = "#{mapped_array_entry[:list]}=" if (attribute&.mutability == 'readWrite' || attribute&.mutability == 'writeOnly') && self.respond_to?(method) - find_with_proc = mapped_array_entry[:find_with] + find_all_with_proc = mapped_array_entry[:find_all_with] + find_with_proc = mapped_array_entry[:find_with] + + if find_all_with_proc.respond_to?(:call) + scim_entries = (scim_hash_or_leaf_value || []) + mapped_list = find_all_with_proc.call(scim_entries) || [] - unless find_with_proc.nil? + mapped_list.compact! + + self.public_send(method, mapped_list) + elsif find_with_proc.respond_to?(:call) mapped_list = (scim_hash_or_leaf_value || []).map do | source_list_entry | find_with_proc.call(source_list_entry) end From fd833645aa471ff514c2d6692836e507962c01cb Mon Sep 17 00:00:00 2001 From: Shuya Honda Date: Mon, 8 Sep 2025 15:14:33 +0900 Subject: [PATCH 2/2] Add MockBatchGroupsController and routes for batch processing; implement find_all_with in MockGroupBatch --- .../mock_batch_groups_controller.rb | 13 ++++++++ .../apps/dummy/app/models/mock_group_batch.rb | 20 +++++++++++++ spec/apps/dummy/config/routes.rb | 5 ++++ ...record_backed_resources_controller_spec.rb | 30 +++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 spec/apps/dummy/app/controllers/mock_batch_groups_controller.rb create mode 100644 spec/apps/dummy/app/models/mock_group_batch.rb diff --git a/spec/apps/dummy/app/controllers/mock_batch_groups_controller.rb b/spec/apps/dummy/app/controllers/mock_batch_groups_controller.rb new file mode 100644 index 0000000..b6731d8 --- /dev/null +++ b/spec/apps/dummy/app/controllers/mock_batch_groups_controller.rb @@ -0,0 +1,13 @@ +class MockBatchGroupsController < Scimitar::ActiveRecordBackedResourcesController + + protected + + def storage_class + MockGroupBatch + end + + def storage_scope + MockGroupBatch.all + end + +end diff --git a/spec/apps/dummy/app/models/mock_group_batch.rb b/spec/apps/dummy/app/models/mock_group_batch.rb new file mode 100644 index 0000000..dc26bec --- /dev/null +++ b/spec/apps/dummy/app/models/mock_group_batch.rb @@ -0,0 +1,20 @@ +class MockGroupBatch < MockGroup + def self.scim_attributes_map + { + id: :id, + externalId: :scim_uid, + displayName: :display_name, + members: [ + { + list: :scim_users_and_groups, + using: { value: :id }, + # Minimal mock: assume user-only entries (type omitted => User) + find_all_with: -> (entries) do + ids = entries.map { |e| e['value'] } + users = MockUser.where(primary_key: ids).to_a + end + } + ] + } + end +end diff --git a/spec/apps/dummy/config/routes.rb b/spec/apps/dummy/config/routes.rb index 8e1d03f..becec67 100644 --- a/spec/apps/dummy/config/routes.rb +++ b/spec/apps/dummy/config/routes.rb @@ -17,6 +17,11 @@ get 'Groups/:id', to: 'mock_groups#show' patch 'Groups/:id', to: 'mock_groups#update' + # Batch lookup variant for testing find_all_with + get 'BatchGroups', to: 'mock_batch_groups#index' + get 'BatchGroups/:id', to: 'mock_batch_groups#show' + patch 'BatchGroups/:id', to: 'mock_batch_groups#update' + # For testing blocks passed to ActiveRecordBackedResourcesController#create, # #update, #replace and #destroy. # diff --git a/spec/requests/active_record_backed_resources_controller_spec.rb b/spec/requests/active_record_backed_resources_controller_spec.rb index e9e3e1d..47eb147 100644 --- a/spec/requests/active_record_backed_resources_controller_spec.rb +++ b/spec/requests/active_record_backed_resources_controller_spec.rb @@ -840,6 +840,36 @@ context '#update' do shared_examples 'an updater' do | force_upper_case: | + context "when updating group members using :find_all_with" do + it "uses :find_all_with to batch-resolve users and updates associations" do + payload = { + schemas: [ 'urn:ietf:params:scim:api:messages:2.0:PatchOp' ], + Operations: [ + { + op: 'add', + path: 'members', + value: [ + { 'value' => @u1.primary_key }, + { 'value' => @u2.primary_key } + ] + } + ] + } + + patch "/BatchGroups/#{@g1.id}", params: payload.merge({ format: :scim }) + + expect(response.status).to eql(200) + + # Verify membership updated + get "/BatchGroups/#{@g1.id}", params: { format: :scim } + expect(response.status).to eql(200) + result = JSON.parse(response.body) + + values = result.fetch('members', []).map { |m| m['value'] } + expect(values).to include(@u1.primary_key.to_s) + expect(values).to include(@u2.primary_key.to_s) + end + end it 'which patches regular attributes' do payload = { Operations: [