From 2fbea6cc94d60f064361c438c96e9e63b61b5060 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 16 Oct 2026 16:27:51 +0200 Subject: [PATCH 1/4] fix(agent): keep the search and segments of a select-all in delete, actions and dissociate [PRD-1667] A select-all request carries the list's search, segment and live-query segment in all_records_subset_query. Bulk delete, bulk actions and dissociate/delete-related built their filter from the condition tree, the ids and the scope only, so "Delete all" on a searched list deleted the whole collection. The three routes now build their filter through one QueryHandler#build_context_filter, as the Node agent does with ContextFilterFactory.build. parse_query_segment and parse_segment read the select-all body too. Refs: PRD-1667 Co-Authored-By: Claude Opus 5.5 --- .../routes/action/actions.rb | 14 +------ .../routes/query_handler.rb | 33 +++++++++++---- .../routes/resources/delete.rb | 12 ++---- .../resources/related/dissociate_related.rb | 12 ++---- .../utils/query_string_parser.rb | 3 +- .../routes/action/actions_spec.rb | 42 +++++++++++++++++++ .../routes/query_handler_spec.rb | 17 ++++++++ .../routes/resources/delete_spec.rb | 37 +++++++++++++++- .../related/dissociate_related_spec.rb | 38 +++++++++++++++++ 9 files changed, 170 insertions(+), 38 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/action/actions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/action/actions.rb index a7a548f4d..3a57509b2 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/action/actions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/action/actions.rb @@ -11,6 +11,7 @@ class Actions < AbstractAuthenticatedRoute include ForestAdminDatasourceToolkit::Components::Query include ForestAdminDatasourceToolkit::Components::Query::ConditionTree include ForestAdminDatasourceCustomizer::Decorators::Action + include ForestAdminAgent::Routes::QueryHandler def initialize(collection, action) @action_name = action @@ -223,18 +224,7 @@ def decode_signed_approval_request(signed_request) def get_record_selection(args, context) attributes = args.dig(:params, :data, :attributes) - # Match user filter + search + scope? + segment - scope = context.permissions.get_scope(context.collection) - filter = Filter.new( - condition_tree: ConditionTreeFactory.intersect( - [ - scope, - ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree( - context.collection, args - ) - ] - ) - ) + filter = build_context_filter(context.collection, args, context.permissions, context.caller) # Restrict the filter to the selected records for single or bulk actions if context.collection.schema[:actions][@action_name].scope != Types::ActionScope::GLOBAL diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb index fb979f08e..d13df3cfa 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb @@ -23,23 +23,25 @@ def execute_query(datasource, query, connection_name, permissions, caller, conte end def parse_query_segment(collection, args, permissions, caller) - return unless args[:params][:segmentQuery] + segment_query = QueryStringParser.subset_or_query(args, :segmentQuery) + return unless segment_query - unless args[:params][:connectionName] + connection_name = QueryStringParser.subset_or_query(args, :connectionName) + unless connection_name raise ForestAdminAgent::Http::Exceptions::UnprocessableError, 'Missing native query connection attribute' end - QueryValidator.valid?(args[:params][:segmentQuery]) + QueryValidator.valid?(segment_query) - permissions.can_execute_query_segment?(collection, args[:params][:segmentQuery], args[:params][:connectionName]) + permissions.can_execute_query_segment?(collection, segment_query, connection_name) ids = execute_query( collection.datasource, - args[:params][:segmentQuery], - args[:params][:connectionName], + segment_query, + connection_name, permissions, caller, - args[:params][:contextVariables] + QueryStringParser.subset_or_query(args, :contextVariables) ).map(&:values) condition_tree_segment = ConditionTree::ConditionTreeFactory.match_ids(collection, ids) @@ -47,6 +49,23 @@ def parse_query_segment(collection, args, permissions, caller) condition_tree_segment end + + def build_context_filter(collection, args, permissions, caller) + search = QueryStringParser.parse_search(collection, args) + + Filter.new( + condition_tree: ConditionTree::ConditionTreeFactory.intersect( + [ + permissions.get_scope(collection), + QueryStringParser.parse_condition_tree(collection, args), + parse_query_segment(collection, args, permissions, caller) + ] + ), + search: search, + search_extended: search && QueryStringParser.parse_search_extended(args), + segment: QueryStringParser.parse_segment(collection, args) + ) + end end end end diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb index 5d1c1f3cf..d757276f8 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb @@ -8,6 +8,7 @@ class Delete < AbstractAuthenticatedRoute include ForestAdminAgent::Builder include ForestAdminDatasourceToolkit::Components::Query include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + include ForestAdminAgent::Routes::QueryHandler def setup_routes add_route('forest_delete_bulk', 'delete', '/:collection_name', ->(args) { handle_request_bulk(args) }) @@ -67,14 +68,9 @@ def delete_records(args, selection_ids, context) field_schema.origin_type_field => nil }) end - filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( - condition_tree: ConditionTree::ConditionTreeFactory.intersect( - [ - Utils::QueryStringParser.parse_condition_tree(context.collection, args), - condition_tree_ids, - context.permissions.get_scope(context.collection) - ] - ) + filter = build_context_filter(context.collection, args, context.permissions, context.caller) + filter = filter.override( + condition_tree: ConditionTree::ConditionTreeFactory.intersect([filter.condition_tree, condition_tree_ids]) ) context.collection.delete(context.caller, filter) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb index fe003eaff..2d4b25b69 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb @@ -8,6 +8,7 @@ class DissociateRelated < AbstractRelatedRoute include ForestAdminAgent::Builder include ForestAdminDatasourceToolkit::Utils include ForestAdminDatasourceToolkit::Components::Query + include ForestAdminAgent::Routes::QueryHandler def setup_routes add_route( @@ -126,14 +127,9 @@ def get_base_foreign_filter(args, context) raise ForestAdminDatasourceToolkit::Exceptions::ForestException, 'Expected no empty id list' end - Filter.new( - condition_tree: ConditionTree::ConditionTreeFactory.intersect( - [ - context.permissions.get_scope(context.child_collection), - Utils::QueryStringParser.parse_condition_tree(context.child_collection, args), - selected_ids - ] - ) + filter = build_context_filter(context.child_collection, args, context.permissions, context.caller) + filter.override( + condition_tree: ConditionTree::ConditionTreeFactory.intersect([filter.condition_tree, selected_ids]) ) end end diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index d5f2c4d90..a8978f968 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -318,8 +318,7 @@ def self.parse_chart_parameters(args) end def self.parse_segment(collection, args) - segment = args.dig(:params, :data, :attributes, :all_records_subset_query, - :segment) || args.dig(:params, :segment) + segment = subset_or_query(args, :segment) return unless segment diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb index 2a42a043c..218cb18eb 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb @@ -75,6 +75,48 @@ def audited_ids end end + describe 'the records a select-all runs on' do + let(:collection) do + build_collection( + name: 'orders', + schema: { + fields: { + 'id' => ColumnSchema.new( + column_type: 'Number', is_primary_key: true, + filter_operators: [Operators::IN, Operators::EQUAL] + ) + }, + actions: { 'Refund' => double('action', scope: action_scope) }, + segments: ['unpaid'] + }, + is_searchable?: true + ) + end + let(:permissions) { instance_double(ForestAdminAgent::Services::Permissions, get_scope: nil) } + let(:context) { double('context', collection: collection, caller: build_caller, permissions: permissions) } + let(:args) do + { + params: { + data: { + attributes: { + ids: [], + all_records: true, + all_records_ids_excluded: ['4'], + all_records_subset_query: { search: 'refund me', searchExtended: 'true', segment: 'unpaid' } + } + } + } + } + end + + it 'keeps the search and the segment of the list the selection was made on' do + filter = route.send(:get_record_selection, args, context) + + expect(filter).to have_attributes(search: 'refund me', search_extended: true, segment: 'unpaid') + expect(filter.condition_tree.to_h).to eq(field: 'id', operator: Operators::NOT_EQUAL, value: 4) + end + end + describe 'auditing the run' do let(:store) { double('store', append_all: [10, 11], confirm: nil) } diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/query_handler_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/query_handler_spec.rb index 3ee91fd43..31938aaf3 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/query_handler_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/query_handler_spec.rb @@ -83,6 +83,23 @@ module Routes expect(condition_tree.to_h).to eq({ field: 'id', operator: Operators::IN, value: [1, 2] }) end + it 'reads the segment query a select-all sends in its body' do + args = { + params: { + data: { + attributes: { + all_records_subset_query: { segmentQuery: 'SELECT id from user', connectionName: 'primary' } + } + } + } + } + + condition_tree = dummy_class.parse_query_segment(collection, args, permission, caller) + expect(condition_tree.to_h).to eq({ field: 'id', operator: Operators::IN, value: [1, 2] }) + expect(permission).to have_received(:can_execute_query_segment?) + .with(collection, 'SELECT id from user', 'primary') + end + it 'work with inject context variable' do args = { params: { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb index 96254ed6b..9cc320059 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb @@ -56,7 +56,8 @@ def respond_to?(arg) ), 'first_name' => ColumnSchema.new(column_type: 'String'), 'last_name' => ColumnSchema.new(column_type: 'String') - } + }, + segments: ['active'] }, delete: true ) @@ -148,6 +149,40 @@ def respond_to?(arg) expect(filter.condition_tree.to_h).to eq(field: 'id', operator: Operators::NOT_IN, value: [1, 2, 3]) end end + + context 'when every record of a searched, segmented list is selected' do + before do + args[:params][:data][:attributes][:all_records] = true + args[:params][:data][:attributes][:all_records_ids_excluded] = %w[3] + args[:params][:data][:attributes][:all_records_subset_query].merge!( + search: 'jo', searchExtended: '1', segment: 'active' + ) + end + + it 'deletes only the records the search and the segment match, minus the excluded ones' do + delete.handle_request_bulk(args) + + expect(@datasource.get_collection('user')).to have_received(:delete) do |caller, filter| + expect(caller).to be_instance_of(Components::Caller) + expect(filter).to have_attributes( + search: 'jo', + search_extended: true, + segment: 'active', + page: nil, + sort: nil + ) + expect(filter.condition_tree.to_h).to eq(field: 'id', operator: Operators::NOT_EQUAL, value: 3) + end + end + + it 'rejects a segment the collection does not have, deleting nothing' do + args[:params][:data][:attributes][:all_records_subset_query][:segment] = 'unknown' + + expect { delete.handle_request_bulk(args) } + .to raise_error(ForestAdminAgent::Http::Exceptions::BadRequestError, 'Invalid segment: unknown') + expect(@datasource.get_collection('user')).not_to have_received(:delete) + end + end end context 'with polymorphic relations' do diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb index bcf48f52b..d23cc2a9f 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb @@ -90,6 +90,7 @@ module Related ) } ) + collection_address.add_segments(['billing']) allow(ForestAdminAgent::Builder::AgentFactory.instance).to receive(:send_schema).and_return(nil) datasource.add_collection(collection_user) @@ -417,6 +418,43 @@ module Related expect(result).to eq({ content: nil, status: 204 }) end + + it 'deletes only the related records a select-all search and segment match' do + allow(@datasource.get_collection('address')).to receive(:delete).and_return(true) + + args[:params][:delete] = true + args[:params]['relation_name'] = 'address_poly' + args[:params]['id'] = 1 + args[:params][:data] = { + attributes: { + ids: [], + all_records: true, + all_records_ids_excluded: ['2'], + all_records_subset_query: { search: 'paris', segment: 'billing' } + } + } + + dissociate.handle_request(args) + + expect(@datasource.get_collection('address')).to have_received(:delete) do |caller, filter| + expect(caller).to be_instance_of(Components::Caller) + expect(filter).to have_attributes( + condition_tree: have_attributes( + aggregator: 'And', + conditions: [ + have_attributes(field: 'id', operator: Operators::NOT_EQUAL, value: 2), + have_attributes(field: 'addressable_id', operator: Operators::EQUAL, value: 1), + have_attributes(field: 'addressable_type', operator: Operators::EQUAL, value: 'user') + ] + ), + page: nil, + search: 'paris', + search_extended: false, + segment: 'billing', + sort: nil + ) + end + end end end end From 56405938836af66211772d3278baafec9ace0e39 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 16 Oct 2026 16:32:49 +0200 Subject: [PATCH 2/4] fix(agent): nullify polymorphic references only for the records a delete matches [PRD-1667] The cleanup ran before the delete filter was built and from the request's ids, so a rejected filter still cleared references, and a select-all cleared those of the excluded records instead of the deleted ones. It now lists the records through the final filter first. A select-all body sending segment: false falls back to the query string segment again. Refs: PRD-1667 Co-Authored-By: Claude Opus 5.5 --- .../routes/resources/delete.rb | 48 ++++++++++-------- .../utils/query_string_parser.rb | 2 +- .../routes/resources/delete_spec.rb | 50 +++++++++++++++++++ .../utils/query_string_parser_spec.rb | 16 ++++++ 4 files changed, 93 insertions(+), 23 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb index d757276f8..12aa27a38 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb @@ -10,6 +10,8 @@ class Delete < AbstractAuthenticatedRoute include ForestAdminDatasourceToolkit::Components::Query::ConditionTree include ForestAdminAgent::Routes::QueryHandler + NULLIFIED_POLYMORPHIC_TYPES = %w[PolymorphicOneToOne PolymorphicOneToMany].freeze + def setup_routes add_route('forest_delete_bulk', 'delete', '/:collection_name', ->(args) { handle_request_bulk(args) }) add_route('forest_delete', 'delete', '/:collection_name/:id', ->(args) { handle_request(args) }) @@ -40,19 +42,29 @@ def delete_records(args, selection_ids, context) selection_ids[:ids]) condition_tree_ids = condition_tree_ids.inverse if selection_ids[:are_excluded] - context.collection.schema[:fields].each_value do |field_schema| - next unless ['PolymorphicOneToOne', 'PolymorphicOneToMany'].include?(field_schema.type) - next if field_schema.respond_to?(:cascade_on_delete) && field_schema.cascade_on_delete + filter = build_context_filter(context.collection, args, context.permissions, context.caller) + filter = filter.override( + condition_tree: ConditionTree::ConditionTreeFactory.intersect([filter.condition_tree, condition_tree_ids]) + ) + + nullify_polymorphic_references(context, filter) + context.collection.delete(context.caller, filter) + end + + def nullify_polymorphic_references(context, filter) + relations = context.collection.schema[:fields].values.select do |field_schema| + NULLIFIED_POLYMORPHIC_TYPES.include?(field_schema.type) && + !(field_schema.respond_to?(:cascade_on_delete) && field_schema.cascade_on_delete) + end + return if relations.empty? + + # ponytail: loads every deleted record's keys at once, page it if select-all deletes outgrow memory + records = context.collection.list(context.caller, filter, + Projection.new(relations.map(&:origin_key_target).uniq)) - origin_values = selection_ids[:ids].map do |pk_hash| - if pk_hash.is_a?(Hash) - pk_hash[field_schema.origin_key_target] - else - pk_names = ForestAdminDatasourceToolkit::Utils::Schema.primary_keys(context.collection) - index = pk_names.index(field_schema.origin_key_target) - pk_hash[index] if index - end - end + relations.each do |field_schema| + origin_values = records.map { |record| record[field_schema.origin_key_target] } + next if origin_values.empty? condition_tree = Nodes::ConditionTreeBranch.new( 'And', @@ -62,18 +74,10 @@ def delete_records(args, selection_ids, context) context.collection.name.gsub('__', '::')) ] ) - filter = Filter.new(condition_tree: condition_tree) context.datasource.get_collection(field_schema.foreign_collection) - .update(context.caller, filter, { field_schema.origin_key => nil, - field_schema.origin_type_field => nil }) + .update(context.caller, Filter.new(condition_tree: condition_tree), + { field_schema.origin_key => nil, field_schema.origin_type_field => nil }) end - - filter = build_context_filter(context.collection, args, context.permissions, context.caller) - filter = filter.override( - condition_tree: ConditionTree::ConditionTreeFactory.intersect([filter.condition_tree, condition_tree_ids]) - ) - - context.collection.delete(context.caller, filter) end end end diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index a8978f968..18a562bc7 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -318,7 +318,7 @@ def self.parse_chart_parameters(args) end def self.parse_segment(collection, args) - segment = subset_or_query(args, :segment) + segment = subset_or_query(args, :segment) || args.dig(:params, :segment) return unless segment diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb index 9cc320059..1649b0b78 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb @@ -259,6 +259,7 @@ def respond_to?(arg) @datasource = ForestAdminAgent::Facades::Container.datasource allow(@datasource.get_collection('user')).to receive(:delete) + allow(@datasource.get_collection('user')).to receive(:list).and_return([{ 'id' => 42 }]) allow(@datasource.get_collection('address')).to receive(:update) end @@ -290,6 +291,52 @@ def respond_to?(arg) end end end + + describe 'polymorphic relation cleanup on a searched select-all' do + let(:params) do + { + 'collection_name' => 'user', + 'timezone' => 'Europe/Paris', + data: { + attributes: { + ids: [], + all_records: true, + all_records_ids_excluded: ['7'], + all_records_subset_query: { search: 'jo' } + }, + type: 'action-requests' + } + } + end + + it 'nullifies the references of the records the search matched, not of the excluded ids' do + delete.handle_request_bulk(args) + + expect(@datasource.get_collection('user')).to have_received(:list) do |_caller, filter, projection| + expect(filter.search).to eq('jo') + expect(filter.condition_tree.to_h).to eq(field: 'id', operator: Operators::NOT_EQUAL, value: 7) + expect(projection).to eq(['id']) + end + expect(@datasource.get_collection('address')).to have_received(:update) do |_caller, filter, patch| + expect(filter.condition_tree.to_h).to eq( + aggregator: 'And', + conditions: [ + { field: 'addressable_id', operator: Operators::IN, value: [42] }, + { field: 'addressable_type', operator: Operators::EQUAL, value: 'user' } + ] + ) + expect(patch).to eq({ 'addressable_id' => nil, 'addressable_type' => nil }) + end + end + + it 'nullifies nothing when the select-all filter is rejected' do + args[:params][:data][:attributes][:all_records_subset_query][:segment] = 'unknown' + + expect { delete.handle_request_bulk(args) } + .to raise_error(ForestAdminAgent::Http::Exceptions::BadRequestError, 'Invalid segment: unknown') + expect(@datasource.get_collection('address')).not_to have_received(:update) + end + end end context 'with composite primary keys and polymorphic relations' do @@ -372,6 +419,9 @@ def respond_to?(arg) @datasource = ForestAdminAgent::Facades::Container.datasource allow(@datasource.get_collection('composite_model')).to receive(:delete) + allow(@datasource.get_collection('composite_model')).to receive(:list).and_return( + [{ 'key1' => 'abc', 'key2' => 1 }, { 'key1' => 'def', 'key2' => 2 }, { 'key1' => 'ghi', 'key2' => 3 }] + ) allow(@datasource.get_collection('composite_address')).to receive(:update) end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index 30325b65d..b73866b43 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1381,6 +1381,22 @@ module Utils end end + describe 'when parse_segment' do + let(:segmented) { build_collection(schema: { segments: ['active'] }) } + + it 'reads the segment of a select-all body sent as an array of ids without raising' do + args = { params: { data: [{ 'id' => 1 }], segment: 'active' } } + + expect(described_class.parse_segment(segmented, args)).to eq('active') + end + + it 'falls back to the query string segment when the select-all body sends false' do + args = { params: { segment: 'active', data: { attributes: { all_records_subset_query: { segment: false } } } } } + + expect(described_class.parse_segment(segmented, args)).to eq('active') + end + end + describe 'when parse_search_extended' do it 'returns the query searchExtended parameter' do args = { params: { searchExtended: true } } From b9b6b40cdb5e3bb26878593b62690a73a4d84b43 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 16 Oct 2026 17:50:57 +0200 Subject: [PATCH 3/4] fix(datasource_active_record): group nested condition branches, and answer the PRD-1667 review [PRD-1667] ActiveRecord chained every leaf onto the running query, so And[a, Or[x, y]] compiled as `a OR x OR y`: the search Or that select-all now carries into delete widened it to the whole collection, and already widened a searched list on main. Each branch is now built as its own relation, which ActiveRecord groups. build_context_filter checks the caller can read the fields its search and filter reach, and list, count and csv build through it too. Delete lists the polymorphic keys, deletes, then nullifies, so a failed delete leaves the children attached. Refs: PRD-1667 Co-Authored-By: Claude Opus 5.5 --- .../routes/query_handler.rb | 12 ++- .../routes/resources/count.rb | 23 +--- .../routes/resources/csv.rb | 22 +--- .../routes/resources/delete.rb | 48 +++++---- .../routes/resources/list.rb | 24 +---- .../routes/action/actions_spec.rb | 42 -------- .../routes/resources/action/actions_spec.rb | 52 ++++++++- .../routes/resources/delete_spec.rb | 28 ++++- .../related/dissociate_related_spec.rb | 18 ++-- .../utils/query_string_parser_spec.rb | 2 +- .../utils/query.rb | 100 ++++++++---------- .../utils/query_spec.rb | 60 +++++++++++ 12 files changed, 226 insertions(+), 205 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb index d13df3cfa..f65769651 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/query_handler.rb @@ -50,19 +50,25 @@ def parse_query_segment(collection, args, permissions, caller) condition_tree_segment end - def build_context_filter(collection, args, permissions, caller) + def build_context_filter(collection, args, permissions, caller, sort: nil) + condition_tree = QueryStringParser.parse_condition_tree(collection, args) search = QueryStringParser.parse_search(collection, args) + search_extended = QueryStringParser.parse_search_extended(args) + applied = { condition_tree: condition_tree, search: search, search_extended: search_extended } + applied[:sort] = sort if sort + permissions.assert_can_read_query_fields(collection, **applied) Filter.new( condition_tree: ConditionTree::ConditionTreeFactory.intersect( [ permissions.get_scope(collection), - QueryStringParser.parse_condition_tree(collection, args), + condition_tree, parse_query_segment(collection, args, permissions, caller) ] ), search: search, - search_extended: search && QueryStringParser.parse_search_extended(args), + search_extended: search_extended, + sort: sort, segment: QueryStringParser.parse_segment(collection, args) ) end diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb index 85ecee003..741008e09 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb @@ -19,28 +19,7 @@ def handle_request(args = {}) context.permissions.can?(:browse, context.collection) if context.collection.is_countable? - condition_tree = ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree( - context.collection, args - ) - search = QueryStringParser.parse_search(context.collection, args) - search_extended = QueryStringParser.parse_search_extended(args) - context.permissions.assert_can_read_query_fields( - context.collection, - condition_tree: condition_tree, search: search, search_extended: search_extended - ) - - filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( - condition_tree: ConditionTreeFactory.intersect( - [ - context.permissions.get_scope(context.collection), - parse_query_segment(context.collection, args, context.permissions, context.caller), - condition_tree - ] - ), - search: search, - search_extended: search_extended, - segment: QueryStringParser.parse_segment(context.collection, args) - ) + filter = build_context_filter(context.collection, args, context.permissions, context.caller) aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count') result = context.collection.aggregate(context.caller, filter, aggregation) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb index dd45c45fa..7f493bbd8 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb @@ -23,28 +23,8 @@ def handle_request(args = {}) context.permissions.can?(:browse, context.collection) context.permissions.can?(:export, context.collection) - condition_tree = QueryStringParser.parse_condition_tree(context.collection, args) - search = QueryStringParser.parse_search(context.collection, args) - search_extended = QueryStringParser.parse_search_extended(args) sort = QueryStringParser.parse_sort(context.collection, args) - context.permissions.assert_can_read_query_fields( - context.collection, - condition_tree: condition_tree, sort: sort, search: search, search_extended: search_extended - ) - - filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( - condition_tree: ConditionTreeFactory.intersect( - [ - context.permissions.get_scope(context.collection), - parse_query_segment(context.collection, args, context.permissions, context.caller), - condition_tree - ] - ), - search: search, - search_extended: search_extended, - sort: sort, - segment: QueryStringParser.parse_segment(context.collection, args) - ) + filter = build_context_filter(context.collection, args, context.permissions, context.caller, sort: sort) requested = QueryStringParser.parse_requested_projection(context.collection, args) projection = context.permissions.redact_projection( context.collection, diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb index 12aa27a38..5044b823c 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/delete.rb @@ -47,37 +47,39 @@ def delete_records(args, selection_ids, context) condition_tree: ConditionTree::ConditionTreeFactory.intersect([filter.condition_tree, condition_tree_ids]) ) - nullify_polymorphic_references(context, filter) + relations = nullified_polymorphic_relations(context.collection) + # ponytail: loads every deleted record's keys at once, page it if select-all deletes outgrow memory + records = if relations.any? + context.collection.list(context.caller, filter, + Projection.new(relations.map(&:origin_key_target).uniq)) + end + context.collection.delete(context.caller, filter) + relations.each { |relation| nullify_polymorphic_references(context, relation, records) } end - def nullify_polymorphic_references(context, filter) - relations = context.collection.schema[:fields].values.select do |field_schema| + def nullified_polymorphic_relations(collection) + collection.schema[:fields].values.select do |field_schema| NULLIFIED_POLYMORPHIC_TYPES.include?(field_schema.type) && !(field_schema.respond_to?(:cascade_on_delete) && field_schema.cascade_on_delete) end - return if relations.empty? - - # ponytail: loads every deleted record's keys at once, page it if select-all deletes outgrow memory - records = context.collection.list(context.caller, filter, - Projection.new(relations.map(&:origin_key_target).uniq)) + end - relations.each do |field_schema| - origin_values = records.map { |record| record[field_schema.origin_key_target] } - next if origin_values.empty? + def nullify_polymorphic_references(context, relation, records) + origin_values = records.map { |record| record[relation.origin_key_target] } + return if origin_values.empty? - condition_tree = Nodes::ConditionTreeBranch.new( - 'And', - [ - Nodes::ConditionTreeLeaf.new(field_schema.origin_key, Operators::IN, origin_values), - Nodes::ConditionTreeLeaf.new(field_schema.origin_type_field, Operators::EQUAL, - context.collection.name.gsub('__', '::')) - ] - ) - context.datasource.get_collection(field_schema.foreign_collection) - .update(context.caller, Filter.new(condition_tree: condition_tree), - { field_schema.origin_key => nil, field_schema.origin_type_field => nil }) - end + condition_tree = Nodes::ConditionTreeBranch.new( + 'And', + [ + Nodes::ConditionTreeLeaf.new(relation.origin_key, Operators::IN, origin_values), + Nodes::ConditionTreeLeaf.new(relation.origin_type_field, Operators::EQUAL, + context.collection.name.gsub('__', '::')) + ] + ) + context.datasource.get_collection(relation.foreign_collection) + .update(context.caller, Filter.new(condition_tree: condition_tree), + { relation.origin_key => nil, relation.origin_type_field => nil }) end end end diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb index f26244ab5..a6aff6ed5 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb @@ -18,29 +18,9 @@ def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.collection) - condition_tree = QueryStringParser.parse_condition_tree(context.collection, args) - search = QueryStringParser.parse_search(context.collection, args) - search_extended = QueryStringParser.parse_search_extended(args) sort = QueryStringParser.parse_sort(context.collection, args) - context.permissions.assert_can_read_query_fields( - context.collection, - condition_tree: condition_tree, sort: sort, search: search, search_extended: search_extended - ) - - filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( - condition_tree: ConditionTreeFactory.intersect( - [ - context.permissions.get_scope(context.collection), - condition_tree, - parse_query_segment(context.collection, args, context.permissions, context.caller) - ] - ), - page: QueryStringParser.parse_pagination(args), - search: search, - search_extended: search_extended, - sort: sort, - segment: QueryStringParser.parse_segment(context.collection, args) - ) + filter = build_context_filter(context.collection, args, context.permissions, context.caller, sort: sort) + .override(page: QueryStringParser.parse_pagination(args)) projection = redacted_projection_with_pks(context, context.collection, args) records = context.collection.list(context.caller, filter, projection) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb index 218cb18eb..2a42a043c 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb @@ -75,48 +75,6 @@ def audited_ids end end - describe 'the records a select-all runs on' do - let(:collection) do - build_collection( - name: 'orders', - schema: { - fields: { - 'id' => ColumnSchema.new( - column_type: 'Number', is_primary_key: true, - filter_operators: [Operators::IN, Operators::EQUAL] - ) - }, - actions: { 'Refund' => double('action', scope: action_scope) }, - segments: ['unpaid'] - }, - is_searchable?: true - ) - end - let(:permissions) { instance_double(ForestAdminAgent::Services::Permissions, get_scope: nil) } - let(:context) { double('context', collection: collection, caller: build_caller, permissions: permissions) } - let(:args) do - { - params: { - data: { - attributes: { - ids: [], - all_records: true, - all_records_ids_excluded: ['4'], - all_records_subset_query: { search: 'refund me', searchExtended: 'true', segment: 'unpaid' } - } - } - } - } - end - - it 'keeps the search and the segment of the list the selection was made on' do - filter = route.send(:get_record_selection, args, context) - - expect(filter).to have_attributes(search: 'refund me', search_extended: true, segment: 'unpaid') - expect(filter.condition_tree.to_h).to eq(field: 'id', operator: Operators::NOT_EQUAL, value: 4) - end - end - describe 'auditing the run' do let(:store) { double('store', append_all: [10, 11], confirm: nil) } diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/action/actions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/action/actions_spec.rb index 41f26e822..e651fdff7 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/action/actions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/action/actions_spec.rb @@ -43,9 +43,11 @@ module Action 'id' => ColumnSchema.new( column_type: 'Uuid', is_primary_key: true, - filter_operators: [Operators::EQUAL, Operators::IN] - ) - } + filter_operators: [Operators::EQUAL, Operators::IN, Operators::NOT_EQUAL] + ), + 'title' => ColumnSchema.new(column_type: 'String') + }, + segments: ['recent'] } ) @@ -62,6 +64,7 @@ module Action allow(permissions).to receive_messages( can_smart_action?: true, get_scope: nil, + assert_can_read_query_fields: nil, get_user_data: { id: 1, firstName: 'John', @@ -170,6 +173,49 @@ def declare_actions(actions) end end + context 'with a bulk action run on a searched, segmented select-all' do + before do + @action_collection.add_action( + 'foo', + ForestAdminDatasourceCustomizer::Decorators::Action::BaseAction.new( + scope: ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope::BULK + ) do |_context, result_builder| + result_builder.success + end + ) + args[:params][:data][:attributes].merge!( + ids: [], + all_records: true, + all_records_ids_excluded: ['123e4567-e89b-12d3-a456-426614174000'], + all_records_subset_query: { search: 'tolkien', segment: 'recent' } + ) + allow(@action_collection).to receive(:get_form).and_return([]) + end + + it 'hands the action the search and the segment of the list, minus the excluded ids' do + action.handle_hook_request(args) + + expect(@action_collection).to have_received(:get_form) do |caller, action_name, data, filter, _meta| + expect(caller).to be_instance_of(Components::Caller) + expect(action_name).to eq('foo') + expect(data).to be_nil + expect(filter).to have_attributes( + search: 'tolkien', search_extended: false, segment: 'recent', sort: nil, page: nil + ) + expect(filter.condition_tree.to_h).to eq( + { field: 'id', operator: Operators::NOT_EQUAL, value: '123e4567-e89b-12d3-a456-426614174000' } + ) + end + end + + it 'checks the caller can read the fields the search reaches' do + action.handle_hook_request(args) + + expect(permissions).to have_received(:assert_can_read_query_fields) + .with(@datasource.get_collection('book'), condition_tree: nil, search: 'tolkien', search_extended: false) + end + end + context 'with a single action used from list-view, detail-view & summary' do before do @action_collection.add_action( diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb index 1649b0b78..3328bfcd0 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/delete_spec.rb @@ -22,7 +22,7 @@ module Resources before do allow(ForestAdminAgent::Services::Permissions).to receive(:new).and_return(permissions) - allow(permissions).to receive_messages(can?: true, get_scope: nil) + allow(permissions).to receive_messages(can?: true, get_scope: nil, assert_can_read_query_fields: nil) end it 'adds the route forest_store' do @@ -312,12 +312,17 @@ def respond_to?(arg) it 'nullifies the references of the records the search matched, not of the excluded ids' do delete.handle_request_bulk(args) - expect(@datasource.get_collection('user')).to have_received(:list) do |_caller, filter, projection| - expect(filter.search).to eq('jo') + expect(@datasource.get_collection('user')).to have_received(:list) do |caller, filter, projection| + expect(caller).to be_instance_of(Components::Caller) + expect(filter).to have_attributes( + search: 'jo', search_extended: false, segment: nil, sort: nil, page: nil + ) expect(filter.condition_tree.to_h).to eq(field: 'id', operator: Operators::NOT_EQUAL, value: 7) expect(projection).to eq(['id']) end - expect(@datasource.get_collection('address')).to have_received(:update) do |_caller, filter, patch| + expect(@datasource.get_collection('address')).to have_received(:update) do |caller, filter, patch| + expect(caller).to be_instance_of(Components::Caller) + expect(filter).to have_attributes(search: nil, search_extended: nil, segment: nil, sort: nil, page: nil) expect(filter.condition_tree.to_h).to eq( aggregator: 'And', conditions: [ @@ -329,6 +334,21 @@ def respond_to?(arg) end end + it 'nullifies nothing when the search matches no record' do + allow(@datasource.get_collection('user')).to receive(:list).and_return([]) + + delete.handle_request_bulk(args) + + expect(@datasource.get_collection('address')).not_to have_received(:update) + end + + it 'keeps the references attached when the delete itself fails' do + allow(@datasource.get_collection('user')).to receive(:delete).and_raise(StandardError, 'hook refused') + + expect { delete.handle_request_bulk(args) }.to raise_error(StandardError, 'hook refused') + expect(@datasource.get_collection('address')).not_to have_received(:update) + end + it 'nullifies nothing when the select-all filter is rejected' do args[:params][:data][:attributes][:all_records_subset_query][:segment] = 'unknown' diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb index d23cc2a9f..664aae886 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb @@ -102,7 +102,7 @@ module Related @datasource = ForestAdminAgent::Facades::Container.datasource allow(ForestAdminAgent::Services::Permissions).to receive(:new).and_return(permissions) - allow(permissions).to receive_messages(can?: true, get_scope: nil) + allow(permissions).to receive_messages(can?: true, get_scope: nil, assert_can_read_query_fields: nil) end it 'adds the route forest_related_dissociate' do @@ -154,7 +154,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -188,7 +188,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -227,7 +227,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -288,7 +288,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -322,7 +322,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -340,7 +340,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -375,7 +375,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -410,7 +410,7 @@ module Related ), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index b73866b43..04d82c6e7 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -1384,7 +1384,7 @@ module Utils describe 'when parse_segment' do let(:segmented) { build_collection(schema: { segments: ['active'] }) } - it 'reads the segment of a select-all body sent as an array of ids without raising' do + it 'falls back to the query string segment when data is an array of picked ids' do args = { params: { data: [{ 'id' => 1 }], segment: 'active' } } expect(described_class.parse_segment(segmented, args)).to eq('active') diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb index 12a800f48..81228bfba 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb @@ -57,92 +57,84 @@ def apply_pagination(page) @query end - def apply_condition_tree(condition_tree, aggregator = nil) - if condition_tree.is_a? Nodes::ConditionTreeBranch - aggregator = condition_tree.aggregator.downcase.to_sym - condition_tree.conditions.each do |condition| - @query = apply_condition_tree(condition, aggregator) - @query = @query.send(aggregator, @query) - end + def apply_condition_tree(condition_tree) + query_aggregator(:and, condition_relation(condition_tree)) + end - @query - else - @query = compute_main_operator(condition_tree, aggregator || :and) - end + # Each branch is built as its own relation so ActiveRecord groups it: chaining every leaf onto + # @query compiled And[a, Or[x, y]] as `a OR x OR y`. + def condition_relation(condition_tree) + return leaf_relation(condition_tree) unless condition_tree.is_a?(Nodes::ConditionTreeBranch) + + aggregator = condition_tree.aggregator.downcase.to_sym + relations = condition_tree.conditions.map { |condition| condition_relation(condition) } + return @collection.model.where(Arel.sql(aggregator == :or ? '1 = 0' : '1 = 1')) if relations.empty? + + relations.reduce { |combined, relation| combined.send(aggregator, relation) } end - def compute_main_operator(condition_tree, aggregator) + def leaf_relation(condition_tree) resolved = resolve_field(condition_tree.field) field = resolved[:formatted] arel_attr = resolved[:arel_attr] value = condition_tree.value - aggregator = aggregator.to_sym case condition_tree.operator when Operators::PRESENT field_schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@collection, condition_tree.field) - @query = if field_schema.column_type == 'String' - query_aggregator(aggregator, @collection.model.where.not({ field => [nil, ''] })) - else - query_aggregator(aggregator, @collection.model.where.not({ field => nil })) - end + if field_schema.column_type == 'String' + @collection.model.where.not({ field => [nil, ''] }) + else + @collection.model.where.not({ field => nil }) + end when Operators::BLANK field_schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@collection, condition_tree.field) - @query = if field_schema.column_type == 'String' - query_aggregator(aggregator, @collection.model.where({ field => [nil, ''] })) - else - query_aggregator(aggregator, @collection.model.where({ field => nil })) - end + if field_schema.column_type == 'String' + @collection.model.where({ field => [nil, ''] }) + else + @collection.model.where({ field => nil }) + end when Operators::MISSING - @query = query_aggregator(aggregator, @collection.model.where({ field => nil })) + @collection.model.where({ field => nil }) when Operators::EQUAL, Operators::IN - @query = query_aggregator(aggregator, @collection.model.where({ field => value })) + @collection.model.where({ field => value }) when Operators::NOT_EQUAL, Operators::NOT_IN - @query = query_aggregator(aggregator, @collection.model.where.not({ field => value })) + @collection.model.where.not({ field => value }) when Operators::GREATER_THAN - @query = query_aggregator(aggregator, build_comparison_query(condition_tree.field, arel_attr, value, :gt)) + build_comparison_query(condition_tree.field, arel_attr, value, :gt) when Operators::GREATER_THAN_OR_EQUAL - @query = query_aggregator(aggregator, build_comparison_query(condition_tree.field, arel_attr, value, :gteq)) + build_comparison_query(condition_tree.field, arel_attr, value, :gteq) when Operators::LESS_THAN - @query = query_aggregator(aggregator, build_comparison_query(condition_tree.field, arel_attr, value, :lt)) + build_comparison_query(condition_tree.field, arel_attr, value, :lt) when Operators::LESS_THAN_OR_EQUAL - @query = query_aggregator(aggregator, build_comparison_query(condition_tree.field, arel_attr, value, :lteq)) + build_comparison_query(condition_tree.field, arel_attr, value, :lteq) when Operators::CONTAINS - @query = query_aggregator(aggregator, - @collection.model.where(arel_attr.matches("%#{value}%"))) + @collection.model.where(arel_attr.matches("%#{value}%")) when Operators::I_CONTAINS lower_field = Arel::Nodes::NamedFunction.new('LOWER', [arel_attr]) - @query = query_aggregator(aggregator, - @collection.model.where(lower_field.matches("%#{value.to_s.downcase}%"))) + @collection.model.where(lower_field.matches("%#{value.to_s.downcase}%")) when Operators::NOT_CONTAINS - @query = query_aggregator(aggregator, - @collection.model.where.not(arel_attr.matches("%#{value}%"))) + @collection.model.where.not(arel_attr.matches("%#{value}%")) when Operators::NOT_I_CONTAINS lower_field = Arel::Nodes::NamedFunction.new('LOWER', [arel_attr]) - @query = query_aggregator(aggregator, - @collection.model.where.not(lower_field.matches("%#{value.to_s.downcase}%"))) + @collection.model.where.not(lower_field.matches("%#{value.to_s.downcase}%")) when Operators::STARTS_WITH - @query = query_aggregator(aggregator, - @collection.model.where(arel_attr.matches("#{value}%"))) + @collection.model.where(arel_attr.matches("#{value}%")) when Operators::I_STARTS_WITH lower_field = Arel::Nodes::NamedFunction.new('LOWER', [arel_attr]) - @query = query_aggregator(aggregator, - @collection.model.where(lower_field.matches("#{value.to_s.downcase}%"))) + @collection.model.where(lower_field.matches("#{value.to_s.downcase}%")) when Operators::ENDS_WITH - @query = query_aggregator(aggregator, - @collection.model.where(arel_attr.matches("%#{value}"))) + @collection.model.where(arel_attr.matches("%#{value}")) when Operators::I_ENDS_WITH lower_field = Arel::Nodes::NamedFunction.new('LOWER', [arel_attr]) - @query = query_aggregator(aggregator, - @collection.model.where(lower_field.matches("%#{value.to_s.downcase}"))) + @collection.model.where(lower_field.matches("%#{value.to_s.downcase}")) when Operators::LIKE - @query = query_aggregator(aggregator, @collection.model.where(arel_attr.matches(value))) + @collection.model.where(arel_attr.matches(value)) when Operators::I_LIKE lower_field = Arel::Nodes::NamedFunction.new('LOWER', [arel_attr]) - @query = query_aggregator(aggregator, - @collection.model.where(lower_field.matches(value.to_s.downcase))) + @collection.model.where(lower_field.matches(value.to_s.downcase)) when Operators::MATCH # Match operator supports: # - Regexp objects from pattern transformations: Regexp.new("pattern") @@ -167,18 +159,16 @@ def compute_main_operator(condition_tree, aggregator) raise ArgumentError, "Match operator is not supported for database adapter '#{adapter_name}'" end - @query = query_aggregator(aggregator, @collection.model.where(regex_clause, pattern)) + @collection.model.where(regex_clause, pattern) when Operators::INCLUDES_ALL - @query = query_aggregator(aggregator, @collection.model.where(arel_attr.matches_all(value))) + @collection.model.where(arel_attr.matches_all(value)) when Operators::SHORTER_THAN length_func = Arel::Nodes::NamedFunction.new('LENGTH', [arel_attr]) - @query = query_aggregator(aggregator, @collection.model.where(length_func.lt(value))) + @collection.model.where(length_func.lt(value)) when Operators::LONGER_THAN length_func = Arel::Nodes::NamedFunction.new('LENGTH', [arel_attr]) - @query = query_aggregator(aggregator, @collection.model.where(length_func.gt(value))) + @collection.model.where(length_func.gt(value)) end - - @query end def build_select(collection, projection) diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb index 1de5322d6..6559669eb 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb @@ -375,6 +375,66 @@ module Utils end end + describe 'nested condition branches' do + let(:nodes) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + def sql_for(condition_tree) + query_builder = described_class.new(collection, nil, Filter.new(condition_tree: condition_tree)) + query_builder.build + query_builder.query.to_sql + end + + # Car's default_scope (id > 10) is still applied to filtered queries, as on main. + it 'groups an Or nested in an And, so a search cannot widen the other conditions' do + sql = sql_for(nodes::ConditionTreeBranch.new('And', [ + nodes::ConditionTreeLeaf.new('id', operators::NOT_EQUAL, 3), + nodes::ConditionTreeBranch.new('Or', [ + nodes::ConditionTreeLeaf.new('brand', operators::CONTAINS, 'a'), + nodes::ConditionTreeLeaf.new('model', operators::CONTAINS, 'b') + ]) + ])) + + expect(sql).to end_with( + 'WHERE (id > 10) AND "cars"."id" != 3 AND ("cars"."brand" LIKE \'%a%\' OR "cars"."model" LIKE \'%b%\')' + ) + end + + it 'groups an And nested in an Or' do + sql = sql_for(nodes::ConditionTreeBranch.new('Or', [ + nodes::ConditionTreeLeaf.new('id', operators::EQUAL, 1), + nodes::ConditionTreeBranch.new('And', [ + nodes::ConditionTreeLeaf.new('brand', operators::EQUAL, 'a'), + nodes::ConditionTreeLeaf.new('model', operators::EQUAL, 'b') + ]) + ])) + + expect(sql).to end_with( + 'WHERE (id > 10) AND ("cars"."id" = 1 OR "cars"."brand" = \'a\' AND "cars"."model" = \'b\')' + ) + end + + it 'keeps the join of a related field used inside a nested Or' do + sql = sql_for(nodes::ConditionTreeBranch.new('And', [ + nodes::ConditionTreeLeaf.new('id', operators::NOT_EQUAL, 3), + nodes::ConditionTreeBranch.new('Or', [ + nodes::ConditionTreeLeaf.new('brand', operators::CONTAINS, 'a'), + nodes::ConditionTreeLeaf.new('category:label', operators::CONTAINS, 'b') + ]) + ])) + + expect(sql).to include('LEFT OUTER JOIN "categories"') + expect(sql).to end_with( + 'WHERE (id > 10) AND "cars"."id" != 3 AND ("cars"."brand" LIKE \'%a%\' OR "categories"."label" LIKE \'%b%\')' + ) + end + + it 'matches nothing on an empty Or and everything on an empty And' do + expect(sql_for(nodes::ConditionTreeBranch.new('Or', []))).to end_with('WHERE (id > 10) AND (1 = 0)') + expect(sql_for(nodes::ConditionTreeBranch.new('And', []))).to end_with('WHERE (id > 10) AND (1 = 1)') + end + end + describe 'sorting on related fields' do it 'uses the related table name for sorting' do sort = [{ field: 'category:label', ascending: true }] From 76957e2baf891429292a7ff83f0ac2316a2a065f Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Tue, 20 Oct 2026 14:38:45 +0200 Subject: [PATCH 4/4] fix(agent): apply the search and segment to related list, count and csv [PRD-1667] A select-all delete on a searched related list now honours the search, but the related list, count and csv still ignored it, so the list showed records the delete would not touch. The three routes now build their filter through build_context_filter, as Node does with ContextFilterFactory.build. Related count also applies the list's filters, which it used to drop. Refs: PRD-1667 Co-Authored-By: Claude Opus 5.5 --- .../routes/resources/related/count_related.rb | 3 +- .../routes/resources/related/csv_related.rb | 14 +------ .../routes/resources/related/list_related.rb | 18 ++------- .../resources/related/count_related_spec.rb | 37 ++++++++++++++++-- .../resources/related/csv_related_spec.rb | 25 ++++++++++-- .../resources/related/list_related_spec.rb | 39 ++++++++++++++++--- 6 files changed, 96 insertions(+), 40 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/count_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/count_related.rb index a55b87602..6b5e816af 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/count_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/count_related.rb @@ -8,6 +8,7 @@ class CountRelated < AbstractRelatedRoute include ForestAdminAgent::Builder include ForestAdminDatasourceToolkit::Utils include ForestAdminDatasourceToolkit::Components::Query + include ForestAdminAgent::Routes::QueryHandler def setup_routes add_route( @@ -25,7 +26,7 @@ def handle_request(args = {}) context.permissions.can?(:browse, context.child_collection) if context.child_collection.is_countable? - filter = Filter.new(condition_tree: context.permissions.get_scope(context.child_collection)) + filter = build_context_filter(context.child_collection, args, context.permissions, context.caller) primary_key_values = Utils::Id.unpack_id(context.collection, args[:params]['id'], with_key: true) result = Collection.aggregate_relation( context.collection, diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb index 294df1d89..b77032004 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb @@ -6,6 +6,7 @@ class CsvRelated < AbstractRelatedRoute include ForestAdminAgent::Utils include ForestAdminDatasourceToolkit::Utils include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + include ForestAdminAgent::Routes::QueryHandler def setup_routes add_route( @@ -23,18 +24,7 @@ def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.child_collection) context.permissions.can?(:export, context.child_collection) - condition_tree = ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree( - context.child_collection, args - ) - context.permissions.assert_can_read_query_fields( - context.child_collection, condition_tree: condition_tree - ) - - filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( - condition_tree: ConditionTreeFactory.intersect( - [context.permissions.get_scope(context.child_collection), condition_tree] - ) - ) + filter = build_context_filter(context.child_collection, args, context.permissions, context.caller) requested = ForestAdminAgent::Utils::QueryStringParser.parse_requested_projection( context.child_collection, args ) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb index 5b906a308..0c3dfc5c8 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb @@ -8,6 +8,7 @@ class ListRelated < AbstractRelatedRoute include ForestAdminAgent::Builder include ForestAdminDatasourceToolkit::Utils include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + include ForestAdminAgent::Routes::QueryHandler def setup_routes add_route( @@ -23,21 +24,10 @@ def setup_routes def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.child_collection) - condition_tree = ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree( - context.child_collection, args - ) sort = ForestAdminAgent::Utils::QueryStringParser.parse_sort(context.child_collection, args) - context.permissions.assert_can_read_query_fields( - context.child_collection, condition_tree: condition_tree, sort: sort - ) - - filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( - condition_tree: ConditionTreeFactory.intersect( - [context.permissions.get_scope(context.child_collection), condition_tree] - ), - page: ForestAdminAgent::Utils::QueryStringParser.parse_pagination(args), - sort: sort - ) + filter = build_context_filter(context.child_collection, args, context.permissions, context.caller, + sort: sort) + .override(page: ForestAdminAgent::Utils::QueryStringParser.parse_pagination(args)) projection = redacted_projection_with_pks(context, context.child_collection, args) primary_key_values = Utils::Id.unpack_id(context.collection, args[:params]['id'], with_key: true) records = Collection.list_relation( diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/count_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/count_related_spec.rb index 742b87049..dea574f8e 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/count_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/count_related_spec.rb @@ -53,8 +53,9 @@ module Related schema: { fields: { 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), - 'label' => ColumnSchema.new(column_type: 'String') - } + 'label' => ColumnSchema.new(column_type: 'String', filter_operators: [Operators::I_CONTAINS]) + }, + segments: ['popular'] } ) allow(ForestAdminAgent::Builder::AgentFactory.instance).to receive(:send_schema).and_return(nil) @@ -64,7 +65,7 @@ module Related ForestAdminAgent::Builder::AgentFactory.instance.build allow(ForestAdminAgent::Services::Permissions).to receive(:new).and_return(permissions) - allow(permissions).to receive_messages(can?: true, get_scope: Nodes::ConditionTreeBranch.new('Or', [])) + allow(permissions).to receive_messages(can?: true, get_scope: Nodes::ConditionTreeBranch.new('Or', []), assert_can_read_query_fields: nil) end it 'adds the route forest_related_count' do @@ -92,7 +93,7 @@ module Related condition_tree: have_attributes(aggregator: 'Or', conditions: []), page: nil, search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: nil ) @@ -102,6 +103,34 @@ module Related end end + context 'when the related list is searched and segmented' do + it 'counts only the related records the search and the segment match' do + args[:params].merge!('relation_name' => 'category', 'id' => 1, search: 'bar', segment: 'popular') + ForestAdminAgent::Facades::Container.datasource.get_collection('category').enable_count + allow(ForestAdminDatasourceToolkit::Utils::Collection).to receive(:aggregate_relation) + .and_return([{ value: 1 }]) + + count.handle_request(args) + + expect(ForestAdminDatasourceToolkit::Utils::Collection).to have_received(:aggregate_relation) do + |collection, id, relation_name, caller, foreign_filter, aggregation| + expect(caller).to be_instance_of(Components::Caller) + expect(collection.name).to eq('user') + expect(id).to eq({ 'id' => 1 }) + expect(relation_name).to eq('category') + expect(foreign_filter).to have_attributes( + condition_tree: have_attributes(aggregator: 'Or', conditions: []), + page: nil, + search: 'bar', + search_extended: false, + segment: 'popular', + sort: nil + ) + expect(aggregation).to have_attributes(operation: 'Count') + end + end + end + context 'when checking permissions and scope' do it 'authorizes and scopes on the related collection, not the parent' do args[:params]['relation_name'] = 'category' diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb index 5e67841ed..bb85995dc 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb @@ -57,8 +57,9 @@ module Related schema: { fields: { 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), - 'label' => ColumnSchema.new(column_type: 'String') - } + 'label' => ColumnSchema.new(column_type: 'String', filter_operators: [Operators::I_CONTAINS]) + }, + segments: ['popular'] }, list: [Category.new(1, 'bar'), Category.new(2, 'baz'), Category.new(3, 'qux')] ) @@ -87,7 +88,7 @@ module Related csv.handle_request(args) - expect(read_guard_calls[:query_fields]).to eq([{ collection: 'category', applies: %i[filter] }]) + expect(read_guard_calls[:query_fields]).to eq([{ collection: 'category', applies: %i[filter search search_extended] }]) expect(read_guard_calls[:projections]).to eq([{ collection: 'category', named_by_caller: false }]) end @@ -150,6 +151,24 @@ module Related ) end + it 'exports only the related records the search and the segment match' do + args[:params].merge!('relation_name' => 'category', 'id' => 1, search: 'bar', segment: 'popular') + allow(csv_generator_stream).to receive(:stream).and_return(["id,label\n"].to_enum) + + csv.handle_request(args) + + expect(csv_generator_stream).to have_received(:stream) do |_header, filter, _projection, _list_records, _limit| + expect(filter).to have_attributes( + condition_tree: have_attributes(aggregator: 'Or', conditions: []), + page: nil, + search: 'bar', + search_extended: false, + segment: 'popular', + sort: nil + ) + end + end + it 'passes a lambda to stream that calls list_relation with correct parameters' do args[:params]['relation_name'] = 'category' args[:params]['id'] = 1 diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb index 30288e707..0cf86ace1 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb @@ -59,7 +59,8 @@ module Related is_primary_key: true ), 'label' => ColumnSchema.new(column_type: 'String') - } + }, + segments: ['popular'] } ) allow(ForestAdminAgent::Builder::AgentFactory.instance).to receive(:send_schema).and_return(nil) @@ -80,8 +81,8 @@ module Related expect(list.routes.length).to eq 1 end - # The only routes resolving against the foreign collection rather than their own, and a - # related listing applies no search — so neither is checked against the parent. + # The only routes resolving against the foreign collection rather than their own, so none of + # their components is checked against the parent. it 'checks the components it applies against the foreign collection' do args[:params]['relation_name'] = 'category' args[:params]['id'] = 1 @@ -90,11 +91,37 @@ module Related list.handle_request(args) expect(read_guard_calls[:query_fields]).to eq( - [{ collection: 'category', applies: %i[filter sort] }] + [{ collection: 'category', applies: %i[filter sort search search_extended] }] ) expect(read_guard_calls[:projections]).to eq([{ collection: 'category', named_by_caller: false }]) end + context 'when the related list is searched and segmented' do + it 'lists only the related records the search and the segment match' do + args[:params].merge!('relation_name' => 'category', 'id' => 1, search: 'bar', segment: 'popular') + allow(ForestAdminDatasourceToolkit::Utils::Collection).to receive(:list_relation).and_return([]) + + list.handle_request(args) + + expect(ForestAdminDatasourceToolkit::Utils::Collection).to have_received(:list_relation) do + |collection, id, relation_name, caller, foreign_filter, projection| + expect(caller).to be_instance_of(Components::Caller) + expect(collection.name).to eq('user') + expect(id).to eq({ 'id' => 1 }) + expect(relation_name).to eq('category') + expect(foreign_filter).to have_attributes( + condition_tree: have_attributes(aggregator: 'Or', conditions: []), + page: be_instance_of(ForestAdminDatasourceToolkit::Components::Query::Page), + search: 'bar', + search_extended: false, + segment: 'popular', + sort: [{ ascending: true, field: 'id' }] + ) + expect(projection).to eq(%w[id label]) + end + end + end + context 'when call without filters' do it 'call list_relation with expected args' do args[:params]['relation_name'] = 'category' @@ -112,7 +139,7 @@ module Related condition_tree: have_attributes(aggregator: 'Or', conditions: []), page: be_instance_of(ForestAdminDatasourceToolkit::Components::Query::Page), search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: [{ ascending: true, field: 'id' }] ) @@ -196,7 +223,7 @@ module Related ), page: be_instance_of(ForestAdminDatasourceToolkit::Components::Query::Page), search: nil, - search_extended: nil, + search_extended: false, segment: nil, sort: [{ ascending: true, field: 'id' }] )