diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index a6d8d4fdb..332f94dd6 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -330,11 +330,16 @@ def usage(action, collection, path) end def collect_search_usages(collection, search, search_extended, usages) - return if search.nil? || !collection.respond_to?(:searched_fields) + # The stack discards a blank search instead of running it, so there is nothing to authorize. + return if search.nil? || search.strip.empty? - searched = collection.searched_fields(search, search_extended) + searched = collection.searched_fields(search, search_extended) if collection.respond_to?(:searched_fields) - return if searched.nil? + if searched.nil? + assert_extended_search_checkable(collection, search_extended) + + return + end published = collection.datasource.collections @@ -344,6 +349,25 @@ def collect_search_usages(collection, search, search_extended, usages) end end + # Only a callable `replace_search` is a footprint this stack could have described and did not; a + # native child search, or no search decorator at all, builds no condition here. A plain search is + # served in either case — the extended flag is the caller's own, so the exemption stops there. + def assert_extended_search_checkable(collection, search_extended) + return unless search_extended + return unless describes_own_search?(collection) + return unless permission_system? + + raise ForbiddenError, + "You cannot run an extended search on the '#{collection.name}' collection: the fields " \ + 'it reaches cannot be determined, so they cannot be checked against your permissions.' + end + + # Reaches the search decorator only through `CollectionDecorator#search_handler?`: drop that + # delegation and this answers false for every collection. + def describes_own_search?(collection) + collection.respond_to?(:search_handler?) && collection.search_handler? + end + # `searched_fields` answers below the publication layer — deliberately, so a field hidden by # renaming above it is still checked — so it can name a collection `remove_collection` took out # of the API. An extended search does reach through to it: the condition is built below 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 07301f4b0..4b6bb0492 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 @@ -10,6 +10,7 @@ class QueryStringParser DEFAULT_ITEMS_PER_PAGE = '15'.freeze DEFAULT_PAGE_TO_SKIP = '1'.freeze POLYMORPHIC_TARGET_WILDCARD = '*'.freeze + FALSY_SEARCH_EXTENDED = [nil, false, 0, '0', 'false', ''].freeze def self.parse_condition_tree(collection, args) filters = begin @@ -230,21 +231,43 @@ def self.parse_export_pagination(limit) Page.new(offset: 0, limit: limit&.to_i) end + # Presence, not truth: with +||+ a +false+ in the select-all body would silently lose to the + # query string. An explicit +null+ or +''+ there is read as absent rather than as "no search", + # so neither can widen a result set by discarding the term the URL carried. + def self.subset_or_query(args, key) + subset = begin + args.dig(:params, :data, :attributes, :all_records_subset_query) + rescue StandardError + nil + end + + return subset[key] if subset.is_a?(Hash) && !subset[key].nil? && subset[key] != '' + + begin + args.dig(:params, key) + rescue StandardError + nil + end + end + def self.parse_search(collection, args) - search = args.dig(:params, :data, :attributes, :all_records_subset_query, :search) || args.dig(:params, :search) + search = subset_or_query(args, :search) + + return nil if search.nil? - raise BadRequestError, 'Collection is not searchable' if search && !collection.is_searchable? + raise BadRequestError, 'Collection is not searchable' unless collection.is_searchable? - search + unless search.is_a?(String) || search.is_a?(Numeric) + raise BadRequestError, 'Search must be a string or a number' + end + + search.to_s end def self.parse_search_extended(args) - extended = args.dig(:params, :data, :attributes, :all_records_subset_query, - :searchExtended) || args.dig(:params, :searchExtended) - - return false if extended.nil? + extended = subset_or_query(args, :searchExtended) - extended != '0' + !FALSY_SEARCH_EXTENDED.include?(extended.is_a?(String) ? extended.downcase : extended) end def self.parse_sort(collection, args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb index 89d1bb663..4751140fd 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb @@ -55,7 +55,15 @@ module Resources ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count count.handle_request(args) - expect(read_guard_calls[:query_fields]).to eq([{ collection: 'user', applies: %i[filter search] }]) + expect(read_guard_calls[:query_fields]).to eq([{ collection: 'user', applies: %i[filter search search_extended] }]) + end + + it 'hands the guard the extended flag it parsed, not a default' do + ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count + args[:params][:searchExtended] = '1' + count.handle_request(args) + + expect(read_guard_calls[:search_extended]).to eq([true]) end context 'when collection is countable' do diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb index b1e0b0fc6..cacdc8c7b 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb @@ -66,11 +66,18 @@ module Resources csv.handle_request(args) expect(read_guard_calls[:query_fields]).to eq( - [{ collection: 'user', applies: %i[filter sort search] }] + [{ collection: 'user', applies: %i[filter sort search search_extended] }] ) expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: false }]) end + it 'hands the guard the extended flag it parsed, not a default' do + args[:params][:searchExtended] = '1' + csv.handle_request(args) + + expect(read_guard_calls[:search_extended]).to eq([true]) + end + context 'when call csv' do it 'returns a streaming export csv' do # Create a mock enumerator that yields CSV data diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb index 13d0529eb..c9f4eadfd 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb @@ -79,10 +79,17 @@ module Resources list.handle_request(args) expect(read_guard_calls[:query_fields]).to eq( - [{ collection: 'user', applies: %i[filter sort search] }] + [{ collection: 'user', applies: %i[filter sort search search_extended] }] ) end + it 'hands the guard the extended flag it parsed, not a default' do + args[:params][:searchExtended] = '1' + list.handle_request(args) + + expect(read_guard_calls[:search_extended]).to eq([true]) + end + it 'refuses a projection the caller named on its own collection' do args[:params][:fields] = { 'user' => 'id,first_name' } list.handle_request(args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 7c6b2f241..a1529a341 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -248,13 +248,13 @@ def leaf(field) Nodes::ConditionTreeLeaf.new(field, Operators::EQUAL, 'FR76') end - def searchable_cards(searched) + def searchable_cards(searched, search_handler: false) double = instance_double( - ForestAdminDatasourceToolkit::Decorators::CollectionDecorator, + ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator, name: 'cards', datasource: datasource ) - allow(double).to receive(:searched_fields).and_return(searched) + allow(double).to receive_messages(searched_fields: searched, search_handler?: search_handler) double end @@ -353,12 +353,46 @@ def searchable_cards(searched) ) end - # A replaced search: the handler picks the fields, the caller only supplies the text. + # A replaced search: the block picks the fields, the caller only supplies the text. it 'serves the request when the stack cannot say what a search reaches' do permissions = build_permissions([]) - expect { permissions.assert_can_read_query_fields(searchable_cards(nil), search: 'martin') } - .not_to raise_error + expect do + permissions.assert_can_read_query_fields( + searchable_cards(nil, search_handler: true), search: 'martin' + ) + end.not_to raise_error + end + + # The flag is the caller's: the same term with it off and on differs by exactly the rows + # matched through a relation, so an unverifiable traversal is refused where a plain search is + # served. + it 'refuses the extended half of that same search' do + permissions = build_permissions([]) + + expect do + permissions.assert_can_read_query_fields( + searchable_cards(nil, search_handler: true), search: 'martin', search_extended: true + ) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot run an extended search on the 'cards' collection: the fields it reaches " \ + 'cannot be determined, so they cannot be checked against your permissions.' + ) + end + + # `refine_filter` discards a blank search instead of running it, so refusing one would 403 a + # request that searches nothing. + ['', ' '].each do |blank| + it "serves #{blank.inspect} with the extended flag on, which runs no search at all" do + permissions = build_permissions([]) + + expect do + permissions.assert_can_read_query_fields( + searchable_cards(nil, search_handler: true), search: blank, search_extended: true + ) + end.not_to raise_error + end end it 'checks nothing on a collection that cannot answer what a search reaches' do @@ -367,6 +401,24 @@ def searchable_cards(searched) expect { permissions.assert_can_read_query_fields(cards, search: 'martin') }.not_to raise_error end + # Refusing here would 403 every collection the search decorator does not sit on. + it 'serves the extended search of a collection that cannot answer at all' do + permissions = build_permissions([]) + + expect { permissions.assert_can_read_query_fields(cards, search: 'martin', search_extended: true) } + .not_to raise_error + end + + # `permission_system?` fetches `/liana/v4/permissions/environment` cold; `describes_own_search?` + # is local. Every extended search on a nil footprint reached that fetch. + it 'does not reach the permission system for a search it will not refuse' do + permissions = build_permissions([]) + + permissions.assert_can_read_query_fields(cards, search: 'martin', search_extended: true) + + expect(permissions).not_to have_received(:permission_system?) + end + it 'accepts a filter once the collection it reaches is readable' do permissions = build_permissions(%w[accounts]) @@ -381,6 +433,125 @@ def searchable_cards(searched) expect { permissions.assert_can_read_query_fields(cards) }.not_to raise_error end + + # The examples above hand the guard a stubbed footprint to pin its policy. These drive a real + # search decorator instead, so what the decorator reports and what the guard does with it are + # checked together. + describe 'through a real replace_search field selection' do + def cards_searching(replacer) + decorator = ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.new( + cards, datasource + ) + decorator.replace_search(replacer) + + decorator + end + + it 'refuses an included relation path the caller cannot read' do + permissions = build_permissions([]) + collection = cards_searching({ include_fields: ['account:iban'] }) + + expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') } + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot search on 'account:iban': you are not allowed to read the 'accounts' collection." + ) + end + + it 'serves the same search once the collection the path reaches is readable' do + permissions = build_permissions(%w[accounts]) + collection = cards_searching({ include_fields: ['account:iban'] }) + + expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') }.not_to raise_error + end + + # What a field selection buys over a callable: the callable names no field, so a plain search + # reads the same column unchecked for a role that cannot read the collection it belongs to, + # and its extended half is refused outright rather than checked. + it 'serves a plain search a callable describes, which names no field to check' do + permissions = build_permissions([]) + collection = cards_searching( + ->(value, _extended, _context) { { field: 'account:iban', operator: Operators::EQUAL, value: value } } + ) + + expect { permissions.assert_can_read_query_fields(collection, search: 'FR76') }.not_to raise_error + end + + it 'refuses the extended search of that callable, where the selection is checked instead' do + permissions = build_permissions(%w[accounts]) + collection = cards_searching( + ->(value, _extended, _context) { { field: 'account:iban', operator: Operators::EQUAL, value: value } } + ) + + expect do + permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + /You cannot run an extended search on the 'cards' collection/ + ) + end + + it 'serves the extended search of the equivalent field selection' do + permissions = build_permissions(%w[accounts]) + collection = cards_searching({ include_fields: ['account:iban'] }) + + expect do + permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) + end.not_to raise_error + end + + # Refusing here would take extended search off every natively searchable datasource. + it 'serves an extended search the child collection runs natively' do + permissions = build_permissions([]) + native = datasource.get_collection('cards') + allow(native).to receive(:schema).and_return(native.schema.merge(searchable: true)) + collection = ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator.new( + native, datasource + ) + + expect(collection.searched_fields('martin', true)).to be_nil + expect do + permissions.assert_can_read_query_fields(collection, search: 'martin', search_extended: true) + end.not_to raise_error + end + + # The only example that goes through the stack the routes hand the guard: without it, the + # delegation going missing looks exactly like the refusal being correctly narrowed. + it 'refuses a callable extended search through a booted customizer stack' do + permissions = build_permissions([]) + customizer = ForestAdminDatasourceCustomizer::DatasourceCustomizer.new + customizer.add_datasource(datasource, {}) + customizer.customize_collection('cards') do |collection| + collection.replace_search do |value, _extended, _context| + { field: 'pan_last4', operator: Operators::EQUAL, value: value } + end + end + top = customizer.datasource({}).get_collection('cards') + + expect(top).not_to be_a(ForestAdminDatasourceCustomizer::Decorators::Search::SearchCollectionDecorator) + expect(top.searched_fields('martin', true)).to be_nil + expect do + permissions.assert_can_read_query_fields(top, search: 'martin', search_extended: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + /You cannot run an extended search on the 'cards' collection/ + ) + end + + # `can?` allows everything without a permission system, so a refusal there would be the one + # denial no grant could lift. + it 'serves the callable extended search when no permission system is enabled' do + permissions = described_class.new(caller) + allow(permissions).to receive(:permission_system?).and_return(false) + collection = cards_searching( + ->(value, _extended, _context) { { field: 'pan_last4', operator: Operators::EQUAL, value: value } } + ) + + expect do + permissions.assert_can_read_query_fields(collection, search: 'FR76', search_extended: true) + end.not_to raise_error + end + end end describe '#read_permissions' do 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 cd7ee0144..e7f115fac 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 @@ -1197,10 +1197,79 @@ module Utils expect(described_class.parse_search(collection_user, args)).to eq('searched argument') end - it 'converts the query search parameter as string' do - args = { params: { search: 1234 } } + [[1234, '1234'], [12.5, '12.5']].each do |value, expected| + it "converts #{value.inspect} to #{expected.inspect}" do + args = { params: { search: value } } - expect(described_class.parse_search(collection_user, args)).to eq(1234) + expect(described_class.parse_search(collection_user, args)).to eq(expected) + end + end + + it 'falls back to the query string when the subset query does not name a search' do + args = { params: { search: 'searched argument', data: { attributes: { all_records_subset_query: {} } } } } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + # An explicit null there must not discard the term the URL carried: dropping a search widens + # the result set. + it 'reads an explicit null in the subset query as absent, not as no search' do + args = { + params: { + search: 'searched argument', + data: { attributes: { all_records_subset_query: { search: nil } } } + } + } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + # Honouring it would discard the term the URL carried, which widens the result set — the same + # reason an explicit null is read as absent. + it 'reads an empty string in the subset query as absent too' do + args = { + params: { + search: 'searched argument', + data: { attributes: { all_records_subset_query: { search: '' } } } + } + } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + it 'falls back to the query string when the subset query is not a hash' do + args = { params: { search: 'searched argument', data: { attributes: { all_records_subset_query: 'nope' } } } } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + it 'answers nil rather than raising when the body is shaped unexpectedly' do + args = { params: { search: 'searched argument', data: { attributes: [1] } } } + + expect(described_class.parse_search(collection_user, args)).to eq('searched argument') + end + + # Every consumer strips it, so a value that cannot be one term is a request error rather than + # a `NoMethodError` deeper in the stack — or a search for the string an Array prints as. + [['x'], { a: '1' }, true, false].each do |value| + it "refuses #{value.inspect}, which cannot be a search term" do + args = { params: { search: value } } + + expect { described_class.parse_search(collection_user, args) } + .to raise_error(Http::Exceptions::BadRequestError, 'Search must be a string or a number') + end + end + + it 'refuses a body value the query string would otherwise mask' do + args = { + params: { + search: 'searched argument', + data: { attributes: { all_records_subset_query: { search: false } } } + } + } + + expect { described_class.parse_search(collection_user, args) } + .to raise_error(Http::Exceptions::BadRequestError, 'Search must be a string or a number') end it 'works when passed in the body (actions)' do @@ -1232,6 +1301,37 @@ module Utils expect(described_class.parse_search_extended(args)).to be(false) end + + [false, 0, 'false', 'FALSE', ''].each do |falsy| + it "reads #{falsy.inspect} as not extended" do + expect(described_class.parse_search_extended({ params: { searchExtended: falsy } })).to be(false) + end + end + + it 'reads an absent parameter as not extended' do + expect(described_class.parse_search_extended({ params: {} })).to be(false) + end + + it 'keeps reading an unrecognised value as extended, as it always did' do + expect(described_class.parse_search_extended({ params: { searchExtended: 'yes' } })).to be(true) + end + + it 'lets the subset query say no while the query string still carries the flag' do + args = { + params: { + searchExtended: '1', + data: { attributes: { all_records_subset_query: { searchExtended: false } } } + } + } + + expect(described_class.parse_search_extended(args)).to be(false) + end + + it 'falls back to the query string when the subset query does not name the flag' do + args = { params: { searchExtended: '1', data: { attributes: { all_records_subset_query: {} } } } } + + expect(described_class.parse_search_extended(args)).to be(true) + end end describe 'parse_sort' do diff --git a/packages/forest_admin_agent/spec/spec_helper.rb b/packages/forest_admin_agent/spec/spec_helper.rb index edf93e0f9..c659ee7fe 100644 --- a/packages/forest_admin_agent/spec/spec_helper.rb +++ b/packages/forest_admin_agent/spec/spec_helper.rb @@ -44,10 +44,12 @@ # `applies` is derived from the keys the route actually passed, so it cannot drift from the query the # route builds: a component it drops is one it does not pass. -READ_GUARD_QUERY_KEYS = { filter: :condition_tree, sort: :sort, search: :search }.freeze +READ_GUARD_QUERY_KEYS = { + filter: :condition_tree, sort: :sort, search: :search, search_extended: :search_extended +}.freeze RSpec.shared_context 'with readable related collections' do - let(:read_guard_calls) { { query_fields: [], projections: [] } } + let(:read_guard_calls) { { query_fields: [], projections: [], search_extended: [] } } before do allow(permissions).to receive(:assert_can_read_query_fields) do |collection, **options| @@ -55,6 +57,8 @@ collection: collection.name, applies: READ_GUARD_QUERY_KEYS.select { |_name, key| options.key?(key) }.keys } + # `applies` only says the key was passed; the refusal turns on its value. + read_guard_calls[:search_extended] << options[:search_extended] if options.key?(:search_extended) end allow(permissions).to receive(:assert_can_read_usages) allow(permissions).to receive(:redact_projection) do |collection, projection, **options| diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb index 7cdab3d46..bce6492de 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/collection_customizer.rb @@ -31,8 +31,33 @@ def disable_count push_customization { @stack.schema.get_collection(@name).override_schema(countable: false) } end - def replace_search(&definition) - push_customization { @stack.search.get_collection(@name).replace_search(definition) } + # On a natively searchable datasource, a selection does not narrow that search — it replaces it + # with the agent's own per-column one. + # + # A relation path into a collection the role cannot read is refused; own columns carry no check, + # there being no field-level permissions. A block names nothing, so its extended half is refused + # where permissions are enabled. + # + # An included relation path is read on a plain search too, so +searchExtended=0+ no longer means + # no relation traversal; with +only_fields+, +extended+ becomes inert entirely. + # + # Names resolve against the collection below this layer, and +rename_field+ sits above it: a field + # renamed +name+ -> +title+ is named +name+ here. Resolved in declaration order, at boot, so put + # +replace_search+ after the +add_field+ / +add_relation+ calls it depends on. + # + # Example: + # collection.replace_search(include_fields: ['project:name'], exclude_fields: ['description']) + # collection.replace_search { |value, _extended, _context| { field: 'name', operator: Operators::CONTAINS, value: value } } + def replace_search(include_fields: nil, exclude_fields: nil, only_fields: nil, &definition) + selection = { + include_fields: include_fields, + exclude_fields: exclude_fields, + only_fields: only_fields + }.compact + + assert_search_replacement(selection, definition, only_fields: only_fields) + + push_customization { @stack.search.get_collection(@name).replace_search(definition || selection) } end # Disable the search bar @@ -310,6 +335,30 @@ def override_delete(&handler) private + def assert_search_replacement(selection, definition, only_fields:) + if definition && selection.any? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts either a block or a field selection, not both' + end + + if definition.nil? && selection.empty? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' + end + + empty = selection.select { |_name, names| Array(names).empty? } + if empty.any? + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + "replace_search cannot take an empty #{empty.keys.join(" or ")}: use disable_search to " \ + 'turn the search bar off' + end + + return unless only_fields && selection.keys != [:only_fields] + + raise ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or exclude_fields' + end + def push_customization(&customization) @stack.queue_customization(customization) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 775d34567..311f78b81 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -4,6 +4,7 @@ module Search class SearchCollectionDecorator < ForestAdminDatasourceToolkit::Decorators::CollectionDecorator include ForestAdminDatasourceToolkit::Schema include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + include ForestAdminDatasourceToolkit::Exceptions POLYMORPHIC_TYPES = %w[PolymorphicManyToOne PolymorphicOneToOne].freeze TO_ONE_RELATIONS = %w[ManyToOne OneToOne].freeze @@ -20,8 +21,10 @@ def disable_search end def replace_search(replacer) + assert_selection_resolves(replacer) @replacer = replacer @disabled_search = false + warn_extended_search_refused if handler mark_schema_as_dirty end @@ -33,27 +36,20 @@ def refine_filter(caller, filter) # Search string is not significant return filter.override({ search: nil }) if !filter || !filter.search || filter.search.strip&.empty? - # Implement search ourselves - if @replacer || !@child_collection.schema[:searchable] - ctx = ForestAdminDatasourceCustomizer::Context::CollectionCustomizationContext.new(self, caller) - tree = default_replacer(filter.search, filter.search_extended) - - if @replacer - plain_tree = @replacer.call(filter.search, filter.search_extended, ctx) - tree = ConditionTreeFactory.from_plain_object(plain_tree) - end - - # Note that if no fields are searchable with the provided searchString, the conditions - # array might be empty, which will create a condition returning zero records - # (this is the desired behavior). - return filter.override({ - condition_tree: ConditionTreeFactory.intersect([filter.condition_tree, tree]), - search: nil - }) - end - # Let sub-collection deal with the search - filter + return filter unless implements_search? + + tree = if handler + ctx = ForestAdminDatasourceCustomizer::Context::CollectionCustomizationContext.new(self, caller) + ConditionTreeFactory.from_plain_object(handler.call(filter.search, filter.search_extended, ctx)) + else + search_condition_tree(filter.search, filter.search_extended) + end + + filter.override({ + condition_tree: ConditionTreeFactory.intersect([filter.condition_tree, tree]), + search: nil + }) end # Answers against +@child_collection+, which is what the search actually reads: a field @@ -63,21 +59,72 @@ def refine_filter(caller, filter) # field the search cannot match — a number column for a word, a uuid column for anything # else — is left out rather than reported as reached. # - # +nil+ whenever this layer does not choose the fields — a replacer is installed, or the - # child collection searches natively — because then no enumeration made here is true. + # +nil+ whenever this layer does not choose the fields — a callable replacer is installed, or + # the child collection searches natively — because then no enumeration made here is true. def searched_fields(search, extended) return nil unless enumerable_search? return [] if insignificant_search?(search) - get_fields(extended).filter_map do |path, schema| + searchable_fields(extended).filter_map do |path, schema| searched_field(path) if build_condition(path, schema, search) end end + def search_handler? + !handler.nil? + end + private + # Warned at boot, not left to the first caller who trips the 403: blocks installed before this + # version were served. Nil-guarded because the RPC agent runs its own +AgentFactory+ subclass, + # so the base facade's container is never built there — a customization must not become a boot + # failure over a warning. + def warn_extended_search_refused + logger = ForestAdminAgent::Facades::Container.logger + + return if logger.nil? + + logger.log( + 'Warn', + "An extended search on #{name} is refused where permissions are enabled: a " \ + '`replace_search` block names no field, so the agent cannot check what it reads against ' \ + "the caller's permissions. Declaring the search with " \ + '`replace_search(include_fields: [...])` makes it checkable.' + ) + end + + def handler + @replacer.respond_to?(:call) ? @replacer : nil + end + + def field_selection + @replacer.respond_to?(:call) ? nil : @replacer + end + + def implements_search? + !@replacer.nil? || !@child_collection.schema[:searchable] + end + def enumerable_search? - @replacer.nil? && !@child_collection.schema[:searchable] + handler.nil? && implements_search? + end + + def assert_selection_resolves(replacer) + return if replacer.nil? || replacer.respond_to?(:call) + + selected = field_paths(replacer[:only_fields]) + field_paths(replacer[:include_fields]) + excluded = field_paths(replacer[:exclude_fields]) + + selected.each { |path| selected_field(path) } + excluded.each { |path| excluded_field(path) } + + overlap = selected.select { |path| excluded?(path, excluded) } + + return if overlap.empty? + + raise ForestException, + "Cannot both search and exclude #{overlap.map { |path| "'#{path}'" }.join(", ")}" end def insignificant_search?(search) @@ -93,16 +140,95 @@ def searched_field(path) } end - def default_replacer(search, extended) - searchable_fields = get_fields(extended) - - conditions = searchable_fields.map do |field, schema| + def search_condition_tree(search, extended) + conditions = searchable_fields(extended).filter_map do |field, schema| build_condition(field, schema, search) end + return ConditionTreeFactory.match_none if conditions.empty? + ConditionTreeFactory.union(conditions) end + # Both the condition tree +refine_filter+ builds and the footprint +searched_fields+ reports + # come from here: a path the search reads without appearing in the footprint is a column read + # unchecked. + def searchable_fields(extended) + selection = field_selection || {} + only_fields = selection[:only_fields] + + defaults = only_fields ? {} : get_fields(extended).to_h + selected = field_paths(only_fields) + field_paths(selection[:include_fields]) + + excluded = field_paths(selection[:exclude_fields]) + + defaults + .merge(selected.to_h { |path| resolved_field(path) }) + .reject { |path, _schema| excluded?(path, excluded) } + end + + def excluded?(path, excluded) + excluded.any? { |name| path == name || path.start_with?("#{name}:") } + end + + # Unlike a selected path, a bare to-one relation is legal here: it drops every path through it, + # where naming the target's columns would have to be revisited each time it gains one. + # + # A name the search does not read is reported rather than refused. An exclusion states an + # intent that only becomes more true as the schema moves — a column turning unsearchable + # satisfies "never search this" — so refusing it would stop the agent booting over a + # configuration that was defensive on purpose. A typo still raises, from +get_field_schema+. + def excluded_field(path) + schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) + + unless excludable?(path, schema) + ForestAdminAgent::Facades::Container.logger&.log( + 'Debug', + "Excluding '#{path}' from the search on #{name} changes nothing: the search does not read it" + ) + end + + schema + end + + def excludable?(path, schema) + return path.count(':') <= 1 && searchable_field?(schema) if schema.type == 'Column' + + !path.include?(':') && TO_ONE_RELATIONS.include?(schema.type) + end + + def field_paths(names) + Array(names).map(&:to_s) + end + + # Strict where an end-user term would be interpreted: this list is written by the developer, + # so a name that names nothing, or names a relation the search cannot compare a term to, is a + # mistake to report. + def resolved_field(path) + schema = ForestAdminDatasourceToolkit::Utils::Collection.get_field_schema(@child_collection, path) + + unless schema.type == 'Column' + raise ForestException, "Cannot search on '#{path}': a #{schema.type} is not a column" + end + + [path, schema] + end + + # A Number, Enum or UUID column reaching `build_condition` gets an EQUAL leaf its datasource + # never declared, and `get_fields` skips it silently — so a named one is refused instead. + def selected_field(path) + resolved = resolved_field(path) + schema = resolved.last + + unless searchable_field?(schema) + raise ForestException, + "Cannot search on '#{path}': its #{schema.column_type} column declares no filter " \ + 'operator a search term can use' + end + + resolved + end + def build_condition(field, schema, search_string) column_type = schema.column_type enum_values = schema.enum_values @@ -151,7 +277,7 @@ def get_fields(extended) fields.push([name, field]) if field.type == 'Column' && searchable_field?(field) if POLYMORPHIC_TYPES.include?(field.type) && extended - ForestAdminAgent::Facades::Container.logger.log( + ForestAdminAgent::Facades::Container.logger&.log( 'Debug', "We're not searching through #{self.name}.#{name} because it's a polymorphic relation. " \ "You can override the default search behavior with 'replace_search'. " \ diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb index b236e143c..bf3ff27fc 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/collection_customizer_spec.rb @@ -200,6 +200,99 @@ module ForestAdminDatasourceCustomizer expect(search_collection.instance_variable_get(:@replacer)).to eq(condition) end + + it 'hands the search decorator the field selection it was given' do + stack = @datasource_customizer.stack + stack.apply_queued_customizations({}) + + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + customizer.replace_search(include_fields: ['title'], exclude_fields: ['id']) + stack.apply_queued_customizations({}) + + search_collection = @datasource_customizer.stack.search.get_collection('book') + + expect(search_collection.instance_variable_get(:@replacer)) + .to eq({ include_fields: ['title'], exclude_fields: ['id'] }) + end + + # `extended` is the caller's, and comes from the request: a customization pinning it would + # decide for every caller. + it 'refuses a key that is not one of the three field lists' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(extended: true) }.to raise_error(ArgumentError, /extended/) + end + + it 'refuses a block and a field selection at once, which would leave the winner implicit' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: ['title']) { |value| value } } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts either a block or a field selection, not both' + ) + end + + it 'refuses neither, which would replace the search with nothing' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search needs a block, or one of include_fields, exclude_fields, only_fields' + ) + end + + # It also re-enables a bar `disable_search` had switched off. + it 'refuses an empty field list, which says nothing and disables nothing' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: []) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search cannot take an empty only_fields: use disable_search to turn the search bar off' + ) + end + + it 'refuses only_fields alongside include_fields, where only would stop meaning only' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: ['title'], include_fields: ['id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or exclude_fields' + ) + end + + it 'refuses only_fields alongside exclude_fields, which could empty the selected set' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(only_fields: ['title'], exclude_fields: ['title']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search accepts only_fields on its own, not alongside include_fields or exclude_fields' + ) + end + + it 'refuses an empty include_fields too, not only an empty only_fields' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(include_fields: []) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'replace_search cannot take an empty include_fields: use disable_search to turn the search bar off' + ) + end + + it 'names every empty list it was given, not just the first' do + customizer = described_class.new(@datasource_customizer, @datasource_customizer.stack, 'book') + + expect { customizer.replace_search(include_fields: [], exclude_fields: []) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + /empty include_fields or exclude_fields/ + ) + end end context 'when using disable_search' do diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb index 46fb7afcd..b98f5766d 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator_spec.rb @@ -219,13 +219,18 @@ module Search end end - context 'when the given field is a column' do - it 'adds a condition to return records matching the search value' do + context 'when the collection carries no searchable column' do + # Used to answer an empty filter, which `intersect` reads as no restriction at all: every + # row came back for a term that matched nothing. + it 'matches nothing rather than every record' do filter = Filter.new(search: 'a search value') search_collection_decorator = described_class.new(@collection_user, datasource) refined_filter = search_collection_decorator.refine_filter(caller, filter) - expect(refined_filter.to_h).to eq(Filter.new.to_h) + expect(refined_filter).to have_attributes( + search: nil, + condition_tree: have_attributes(aggregator: 'Or', conditions: []) + ) end end @@ -600,7 +605,7 @@ module Search refined_filter = search_collection_decorator.refine_filter(caller, filter) expect(refined_filter).to have_attributes( search: nil, - condition_tree: nil + condition_tree: have_attributes(aggregator: 'Or', conditions: []) ) end end diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index 24142426e..7b0996ddc 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -9,6 +9,8 @@ module Search describe SearchCollectionDecorator do subject(:decorated) { described_class.new(datasource.get_collection('cards'), datasource) } + let(:caller) { instance_double(ForestAdminDatasourceToolkit::Components::Caller) } + let(:datasource) do build_datasource_with_collections( [ @@ -82,6 +84,396 @@ module Search expect(described_class.new(child, datasource).searched_fields('martin', true)).to be_nil end end + + describe '#searched_fields when a field selection narrows the default search' do + it 'answers the footprint instead of refusing to tell' do + decorated.replace_search(exclude_fields: ['pan_last4']) + + expect(decorated.searched_fields('martin', true)).to contain_exactly( + { path: 'holder:national_id', collections: ['holders'] } + ) + end + + it 'reports an included relation path on a plain search too, not only an extended one' do + decorated.replace_search(include_fields: ['holder:national_id']) + + expect(decorated.searched_fields('martin', false)).to contain_exactly( + { path: 'pan_last4', collections: ['cards'] }, + { path: 'holder:national_id', collections: ['holders'] } + ) + end + + it 'reports only the replaced set once only_fields is given' do + decorated.replace_search(only_fields: ['holder:national_id']) + + expect(decorated.searched_fields('martin', true)).to eq( + [{ path: 'holder:national_id', collections: ['holders'] }] + ) + end + + it 'reports an included path once, where it overlaps a default field' do + decorated.replace_search(include_fields: ['pan_last4']) + + expect(decorated.searched_fields('martin', false).map { |field| field[:path] }) + .to eq(['pan_last4']) + end + + it 'answers the footprint even when the datasource searches natively, which it replaces' do + child = datasource.get_collection('cards') + allow(child).to receive(:schema).and_return(child.schema.merge(searchable: true)) + decorator = described_class.new(child, datasource) + decorator.replace_search(only_fields: ['pan_last4']) + + expect(decorator.searched_fields('martin', true)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + + it 'still answers nothing it can be sure of when a callable chooses the fields' do + decorated.replace_search(->(search, _extended, _context) { { field: 'id', operator: 'equal', value: search } }) + + expect(decorated.searched_fields('martin', true)).to be_nil + end + end + + # The invariant the permission check rests on: a path the search reads without appearing in + # the footprint is a column read unchecked. + describe '#searched_fields against what the search actually reads' do + def paths_read(selection, search: 'martin', extended: true) + decorated.replace_search(**selection) + refined = decorated.refine_filter( + caller, + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + search: search, search_extended: extended + ) + ) + + refined.condition_tree.projection.uniq + end + + it 'covers every path an included relation path makes the search read' do + read = paths_read({ include_fields: ['holder:national_id'] }) + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + + expect(read).to include('holder:national_id') + expect(footprint).to include(*read) + end + + it 'covers every path the search reads once exclude_fields dropped one' do + read = paths_read({ exclude_fields: ['pan_last4'] }) + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + + expect(read).not_to include('pan_last4') + expect(read).to include('holder:national_id') + expect(footprint).to include(*read) + end + + it 'covers every path the search reads once only_fields replaced the set' do + read = paths_read({ only_fields: ['holder:national_id'] }) + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + + expect(read).to eq(['holder:national_id']) + expect(footprint).to include(*read) + end + + it 'reads nothing outside the footprint for a numeric term, which matches more columns' do + read = paths_read({ include_fields: ['holder:national_id'] }, search: '42') + footprint = decorated.searched_fields('42', true).map { |field| field[:path] } + + expect(read).to include('id', 'holder:id') + expect(footprint).to include(*read) + end + end + + describe 'when the collection carries a polymorphic relation' do + let(:datasource) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'pan_last4' => build_column(column_type: 'String', filter_operators: [Operators::I_CONTAINS]), + 'holder_id' => build_column(column_type: 'Number'), + 'holder_type' => build_column(column_type: 'String'), + 'holder' => Relations::PolymorphicManyToOneSchema.new( + foreign_key: 'holder_id', + foreign_key_type_field: 'holder_type', + foreign_collections: %w[persons companies], + foreign_key_targets: { 'persons' => 'id', 'companies' => 'id' } + ) + } + } + ), + build_collection( + name: 'persons', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'national_id' => build_column(column_type: 'String', filter_operators: [Operators::I_CONTAINS]) + } + } + ), + build_collection( + name: 'companies', + schema: { fields: { 'id' => build_numeric_primary_key } } + ) + ] + ) + end + + before do + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return( + instance_double(ForestAdminAgent::Services::LoggerService, log: nil) + ) + end + + it 'refuses a path crossing it, which the search cannot follow' do + expect { decorated.replace_search(include_fields: ['holder:national_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'Unexpected field type PolymorphicManyToOne: cards.holder' + ) + end + + it 'refuses it named bare, which carries no term to compare' do + expect { decorated.replace_search(only_fields: ['holder']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot search on 'holder': a PolymorphicManyToOne is not a column" + ) + end + + it 'refuses excluding a path crossing it, rather than excluding nothing' do + expect { decorated.replace_search(exclude_fields: ['holder:national_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + 'Unexpected field type PolymorphicManyToOne: cards.holder' + ) + end + + # The request path, not the boot path: an RPC agent resolves nothing from the base facade, so + # an extended search reaching this relation must not 500 over a Debug line. + it 'searches on where no logger is resolvable' do + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(nil) + + expect(decorated.searched_fields('martin', true).map { |field| field[:path] }) + .to eq(['pan_last4']) + end + + # Its targets stay out of both, so no footprint entry ever needs the several collections + # `leaf_collection_names` would answer for a polymorphic leaf. + it 'keeps its columns out of the footprint and out of what the search reads' do + decorated.replace_search(include_fields: ['pan_last4']) + + footprint = decorated.searched_fields('martin', true).map { |field| field[:path] } + refined = decorated.refine_filter( + caller, + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + search: 'martin', search_extended: true + ) + ) + + expect(footprint).to eq(['pan_last4']) + expect(refined.condition_tree.projection.uniq).to eq(['pan_last4']) + end + end + + describe '#refine_filter when the selection leaves no field searchable' do + def refined(selection) + decorated.replace_search(selection) + decorated.refine_filter( + caller, + ForestAdminDatasourceToolkit::Components::Query::Filter.new(search: 'martin') + ) + end + + # An empty union is nil, and `intersect` drops a nil: the search would carry no condition + # at all and the request would answer every row instead of none. + it 'matches nothing rather than everything once only_fields is empty' do + expect(refined({ only_fields: [] }).condition_tree) + .to have_attributes(aggregator: 'Or', conditions: []) + end + + it 'matches nothing rather than everything once every searchable field is excluded' do + expect(refined({ exclude_fields: %w[id pan_last4 holder:id holder:national_id] }).condition_tree) + .to have_attributes(aggregator: 'Or', conditions: []) + end + end + + # The fixture above has no searchable column whose name the relation only prefixes, so the + # `:` boundary in the exclusion cannot be told apart there. + describe '#replace_search excluding a relation whose name prefixes a column' do + let(:datasource) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'holder_note' => build_column(column_type: 'String', + filter_operators: [Operators::I_CONTAINS]), + 'holder_id' => build_column(column_type: 'Number'), + 'holder' => build_many_to_one(foreign_collection: 'holders', foreign_key: 'holder_id') + } + } + ), + build_collection( + name: 'holders', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'national_id' => build_column(column_type: 'String', + filter_operators: [Operators::I_CONTAINS]) + } + } + ) + ] + ) + end + + it 'keeps a sibling column the relation name only prefixes' do + decorated.replace_search(exclude_fields: ['holder']) + + expect(decorated.searched_fields('martin', true)).to eq( + [{ path: 'holder_note', collections: ['cards'] }] + ) + end + end + + describe '#replace_search with a callable replacer' do + let(:logger) { instance_double(ForestAdminAgent::Services::LoggerService, log: nil) } + + before { allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) } + + it 'warns at boot that an extended search there is refused where permissions are on' do + decorated.replace_search(->(value, _extended, _context) { { field: 'pan_last4', value: value } }) + + expect(logger).to have_received(:log).with( + 'Warn', + 'An extended search on cards is refused where permissions are enabled: a ' \ + '`replace_search` block names no field, so the agent cannot check what it reads against ' \ + "the caller's permissions. Declaring the search with " \ + '`replace_search(include_fields: [...])` makes it checkable.' + ) + end + + # The RPC agent runs its own `AgentFactory` subclass, so the base facade's container is never + # built there and `logger` answers nil. + it 'installs the block anyway where no logger is resolvable' do + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(nil) + + expect { decorated.replace_search(->(value, _extended, _context) { { field: 'pan_last4', value: value } }) } + .not_to raise_error + expect(decorated.searched_fields('martin', true)).to be_nil + end + + it 'says nothing for a field selection, which is checkable' do + decorated.replace_search(include_fields: ['holder:national_id']) + + expect(logger).not_to have_received(:log) + end + end + + describe '#replace_search with an unresolvable field selection' do + it 'names the field it cannot resolve rather than searching nothing for good' do + expect { decorated.replace_search(only_fields: ['pan_last_four']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + /Column not found cards.pan_last_four/ + ) + end + + # The list is written by the developer, so a name that names nothing is a mistake to + # report rather than one to guess at. + it 'refuses a name spelt in another case rather than resolving it fuzzily' do + expect { decorated.replace_search(include_fields: ['panLast4']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /panLast4/) + end + + it 'accepts a symbol, which resolves to the same column as the string' do + decorated.replace_search(only_fields: [:pan_last4]) + + expect(decorated.searched_fields('martin', false)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + + it 'checks the excluded names too, which would otherwise exclude nothing' do + expect { decorated.replace_search(exclude_fields: ['nope']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) + end + + it 'refuses a path reaching through a relation the search cannot follow' do + expect { decorated.replace_search(include_fields: ['holder:nope']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) + end + + # `get_field_schema` only requires a to-one relation of the segments it crosses, so a bare + # relation resolves to a RelationSchema and reached `build_condition`, which asks it for a + # `column_type` it does not have. + it 'refuses a bare relation, which carries no term to compare' do + expect { decorated.replace_search(only_fields: ['holder']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot search on 'holder': a ManyToOne is not a column" + ) + end + + it 'refuses a column no search term can match, which the defaults already skip' do + expect { decorated.replace_search(include_fields: ['holder_id']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot search on 'holder_id': its Number column declares no filter operator a search term can use" + ) + end + + it 'excludes every path through a relation named bare, which no column list could keep up with' do + decorated.replace_search(exclude_fields: ['holder']) + + expect(decorated.searched_fields('martin', true)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + + # Refusing it would stop the agent booting the day a column named defensively turns + # unsearchable — the one configuration that only became more correct. + it 'reports rather than refuses excluding a column the search never reads' do + logger = instance_double(ForestAdminAgent::Services::LoggerService, log: nil) + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) + + expect { decorated.replace_search(exclude_fields: ['holder_id']) }.not_to raise_error + expect(logger).to have_received(:log).with( + 'Debug', + "Excluding 'holder_id' from the search on cards changes nothing: the search does not read it" + ) + end + + it 'still raises on a name that resolves to nothing, which is a typo' do + expect { decorated.replace_search(exclude_fields: ['holder_ids']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /holder_ids/) + end + + it 'refuses searching and excluding the same path, rather than letting one win silently' do + expect { decorated.replace_search(include_fields: ['holder:national_id'], exclude_fields: ['holder']) } + .to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Cannot both search and exclude 'holder:national_id'" + ) + end + + it 'leaves the previous selection in place when it refuses a new one' do + decorated.replace_search(only_fields: ['pan_last4']) + + expect { decorated.replace_search(only_fields: ['nope']) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /nope/) + expect(decorated.searched_fields('martin', false)).to eq( + [{ path: 'pan_last4', collections: ['cards'] }] + ) + end + end end end end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb index b6ffd92c4..50271df64 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb @@ -85,6 +85,12 @@ def searched_fields(search, extended) @child_collection.searched_fields(search, extended) end + def search_handler? + return false unless @child_collection.is_a?(CollectionDecorator) + + @child_collection.search_handler? + end + protected def mark_schema_as_dirty diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb index 2117d3e4f..c7657dcb8 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/decorators/collection_decorator_spec.rb @@ -18,6 +18,24 @@ module Decorators datasource.add_collection(@collection_book) end + # The permission layer probes this with `respond_to?` on the top of a 26-layer stack, so a + # missing link answers "no customization" instead of raising. Owned here, tested here. + context 'when search_handler? is called' do + it 'forwards to the child decorator, which is where the search layer answers' do + child = described_class.new(@collection_book, @collection_book.datasource) + allow(child).to receive(:search_handler?).and_return(true) + decorator = described_class.new(child, @collection_book.datasource) + + expect(decorator.search_handler?).to be true + end + + it 'answers false at the bottom, where the child is a plain collection' do + decorator = described_class.new(@collection_book, @collection_book.datasource) + + expect(decorator.search_handler?).to be false + end + end + context 'when native_driver is called' do it 'returns the native driver' do allow(@collection_book).to receive(:native_driver).and_return('a native driver')