From 366bbdf1603ea601189958ec33a37915a7b4a5f9 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 12 Aug 2026 18:28:07 +0200 Subject: [PATCH 1/8] feat(pylon): client endpoints for accounts, contacts, users and teams Adds search/list/fetch methods for the four new resources, routed through shared private helpers; search_issues and fetch_issue now delegate to the same helpers with unchanged behavior. Co-Authored-By: Claude Fable 5 --- .../forest_admin_datasource_pylon/client.rb | 86 ++++++- .../client_spec.rb | 216 ++++++++++++++++++ 2 files changed, 294 insertions(+), 8 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb index 08678e409..80026c695 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client.rb @@ -1,5 +1,7 @@ module ForestAdminDatasourcePylon - class Client + # Long by line count only: the public surface is one explicit method per Pylon + # endpoint, each delegating to the shared helpers below. + class Client # rubocop:disable Metrics/ClassLength MAX_SEARCH_LIMIT = 1000 # `next_cursor` is nil as soon as Pylon stops advertising a next page, so @@ -19,22 +21,90 @@ def me # POST /issues/search accepts an empty body and then returns the most recent # issues, ordered by `created_at` descending. def search_issues(limit:, cursor: nil, filter: nil, search_text: nil) + search_resource('issues/search', limit: limit, cursor: cursor, filter: filter, search_text: search_text) + end + + # Accepts either the UUID or the issue number. + def fetch_issue(id) + fetch_resource('issues', id) + end + + def search_accounts(limit:, cursor: nil, filter: nil, search_text: nil) + search_resource('accounts/search', limit: limit, cursor: cursor, filter: filter, search_text: search_text) + end + + def list_accounts(limit:, cursor: nil) + list_resource('accounts', limit: limit, cursor: cursor) + end + + # Accepts either the Pylon UUID or the account's external id. + def fetch_account(id) + fetch_resource('accounts', id) + end + + def search_contacts(limit:, cursor: nil, filter: nil, search_text: nil) + search_resource('contacts/search', limit: limit, cursor: cursor, filter: filter, search_text: search_text) + end + + # GET /contacts is paginated exactly like GET /accounts even though the + # OpenAPI spec forgets to document its query parameters. + def list_contacts(limit:, cursor: nil) + list_resource('contacts', limit: limit, cursor: cursor) + end + + def fetch_contact(id) + fetch_resource('contacts', id) + end + + # GET /users is unpaginated. Deactivated agents are included by default so + # that assignees of older issues stay resolvable. + def fetch_users(include_deactivated: true) + fetch_all('users', 'include_deactivated' => include_deactivated) + end + + def fetch_user(id) + fetch_resource('users', id) + end + + # GET /teams is unpaginated and takes no parameter. + def fetch_teams + fetch_all('teams') + end + + def fetch_team(id) + fetch_resource('teams', id) + end + + private + + def search_resource(path, limit:, cursor: nil, filter: nil, search_text: nil) body = { 'limit' => clamp_limit(limit) } body['cursor'] = cursor unless blank?(cursor) body['filter'] = filter unless filter.nil? body['search_text'] = search_text unless blank?(search_text) - must_succeed('issues/search') { to_search_page(connection.post('issues/search', body).body) } + must_succeed(path) { to_search_page(connection.post(path, body).body) } end - # Accepts either the UUID or the issue number. The id comes from - # operator-supplied filter values, so it is escaped before joining the path. - def fetch_issue(id) - path = "issues/#{Faraday::Utils.escape(id)}" - must_succeed(path) { extract_data(connection.get(path).body) } + # `limit` is mandatory on the paginated GET endpoints, unlike their POST + # /search counterparts which default it server-side. + def list_resource(path, limit:, cursor: nil) + params = { 'limit' => clamp_limit(limit) } + params['cursor'] = cursor unless blank?(cursor) + + must_succeed(path) { to_search_page(connection.get(path, params).body) } end - private + def fetch_all(path, params = {}) + must_succeed(path) { Array(extract_data(connection.get(path, params).body)) } + end + + # The id comes from operator-supplied filter values, so it is escaped before + # being joined to the path. + def fetch_resource(resource, id) + path = "#{resource}/#{Faraday::Utils.escape(id)}" + must_succeed(path) { extract_data(connection.get(path).body) } + end def clamp_limit(limit) value = limit.to_i diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb index 08dbc0c48..f0766e309 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client_spec.rb @@ -250,6 +250,222 @@ def json(payload, status = 200) end end + describe '#search_accounts' do + it 'posts the full search envelope and returns the records' do + stub_request(:post, "#{base}/accounts/search").to_return(json('data' => [{ 'id' => 'a1' }])) + filter = { 'field' => 'name', 'operator' => 'equals', 'values' => ['Acme'] } + + page = client.search_accounts(limit: 2, cursor: 'c1', filter: filter, search_text: 'acme') + + expect(page.records).to eq([{ 'id' => 'a1' }]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: { 'limit' => 2, 'cursor' => 'c1', 'filter' => filter, 'search_text' => 'acme' }) + end + + it 'omits cursor, filter and search_text when they are not provided' do + stub_request(:post, "#{base}/accounts/search").to_return(json('data' => [])) + + client.search_accounts(limit: 5, cursor: nil, filter: nil, search_text: '') + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with(body: { 'limit' => 5 }) + end + + it 'wraps a failure in an APIError naming the endpoint' do + body = { 'message' => 'bad filter', 'request_id' => 'req_7' } + stub_request(:post, "#{base}/accounts/search").to_return(json(body, 400)) + + expect { client.search_accounts(limit: 1) }.to raise_error(ForestAdminDatasourcePylon::APIError) { |error| + expect(error.status).to eq(400) + expect(error.message) + .to eq('Pylon API call failed: accounts/search: HTTP 400 bad filter (request_id: req_7)') + } + end + end + + describe '#search_contacts' do + it 'posts to the contacts endpoint, clamps the limit and exposes the next cursor' do + stub_request(:post, "#{base}/contacts/search") + .to_return(json('data' => [{ 'id' => 'ct1' }], 'pagination' => { 'cursor' => 'c2', 'has_next_page' => true })) + + page = client.search_contacts(limit: 99_999) + + expect(page.records).to eq([{ 'id' => 'ct1' }]) + expect(page.next_cursor).to eq('c2') + expect(WebMock).to have_requested(:post, "#{base}/contacts/search") + .with(body: { 'limit' => described_class::MAX_SEARCH_LIMIT }) + end + end + + describe '#list_accounts' do + # Unlike POST /accounts/search, the paginated GET rejects a request that + # does not carry a limit. + it 'sends the mandatory limit as a query parameter' do + stub_request(:get, "#{base}/accounts").with(query: { 'limit' => '2' }) + .to_return(json('data' => [{ 'id' => 'a1' }])) + + page = client.list_accounts(limit: 2) + + expect(page.records).to eq([{ 'id' => 'a1' }]) + expect(page.next_cursor).to be_nil + end + + it 'forwards the cursor and clamps the limit' do + stub_request(:get, "#{base}/accounts").with(query: { 'limit' => '1000', 'cursor' => 'c1' }) + .to_return(json('data' => [])) + + client.list_accounts(limit: 99_999, cursor: 'c1') + + expect(WebMock).to have_requested(:get, "#{base}/accounts") + .with(query: { 'limit' => '1000', 'cursor' => 'c1' }) + end + + it 'omits an empty cursor' do + stub_request(:get, "#{base}/accounts").with(query: { 'limit' => '5' }).to_return(json('data' => [])) + + client.list_accounts(limit: 5, cursor: '') + + expect(WebMock).to have_requested(:get, "#{base}/accounts").with(query: { 'limit' => '5' }) + end + + it 'exposes the cursor when a next page is advertised' do + stub_request(:get, "#{base}/accounts").with(query: { 'limit' => '1' }) + .to_return(json('data' => [], + 'pagination' => { + 'cursor' => 'c2', 'has_next_page' => true + })) + + expect(client.list_accounts(limit: 1).next_cursor).to eq('c2') + end + + it 'reports no next cursor when has_next_page is false' do + stub_request(:get, "#{base}/accounts").with(query: { 'limit' => '1' }) + .to_return(json('data' => [], + 'pagination' => { + 'cursor' => 'c2', 'has_next_page' => false + })) + + expect(client.list_accounts(limit: 1).next_cursor).to be_nil + end + + it 'wraps a failure in an APIError carrying status, body and request_id' do + body = { 'message' => 'limit is required', 'request_id' => 'req_9' } + stub_request(:get, "#{base}/accounts").with(query: { 'limit' => '1' }).to_return(json(body, 400)) + + expect { client.list_accounts(limit: 1) }.to raise_error(ForestAdminDatasourcePylon::APIError) { |error| + expect(error.status).to eq(400) + expect(error.body).to eq(body) + expect(error.message) + .to eq('Pylon API call failed: accounts: HTTP 400 limit is required (request_id: req_9)') + } + end + end + + describe '#list_contacts' do + # The OpenAPI spec omits the query parameters of GET /contacts, but the + # endpoint paginates exactly like GET /accounts. + it 'paginates like the accounts listing' do + stub_request(:get, "#{base}/contacts").with(query: { 'limit' => '2', 'cursor' => 'c1' }) + .to_return(json('data' => [{ 'id' => 'ct1' }], + 'pagination' => { + 'cursor' => 'c2', 'has_next_page' => true + })) + + page = client.list_contacts(limit: 2, cursor: 'c1') + + expect(page.records).to eq([{ 'id' => 'ct1' }]) + expect(page.next_cursor).to eq('c2') + end + end + + describe '#fetch_account' do + it 'unwraps the account' do + stub_request(:get, "#{base}/accounts/a1").to_return(json('data' => { 'id' => 'a1', 'name' => 'Acme' })) + + expect(client.fetch_account('a1')).to eq('id' => 'a1', 'name' => 'Acme') + end + + it 'accepts an external id as well as a uuid' do + stub_request(:get, "#{base}/accounts/ext%2F42").to_return(json('data' => { 'id' => 'a1' })) + + expect(client.fetch_account('ext/42')).to eq('id' => 'a1') + end + + it 'wraps a missing account in a 404 APIError naming the endpoint' do + stub_request(:get, "#{base}/accounts/nope").to_return(json({ 'message' => 'not found' }, 404)) + + expect { client.fetch_account('nope') }.to raise_error(ForestAdminDatasourcePylon::APIError) { |error| + expect(error.status).to eq(404) + expect(error.message).to match(%r{accounts/nope: HTTP 404 not found}) + } + end + end + + describe '#fetch_contact' do + it 'unwraps the contact' do + stub_request(:get, "#{base}/contacts/ct1").to_return(json('data' => { 'id' => 'ct1', 'email' => 'a@b.c' })) + + expect(client.fetch_contact('ct1')).to eq('id' => 'ct1', 'email' => 'a@b.c') + end + end + + describe '#fetch_users' do + it 'includes deactivated users by default' do + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'true' }) + .to_return(json('data' => [{ 'id' => 'u1' }, { 'id' => 'u2' }])) + + expect(client.fetch_users).to eq([{ 'id' => 'u1' }, { 'id' => 'u2' }]) + end + + it 'can ask for active users only' do + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'false' }) + .to_return(json('data' => [])) + + client.fetch_users(include_deactivated: false) + + expect(WebMock).to have_requested(:get, "#{base}/users").with(query: { 'include_deactivated' => 'false' }) + end + + it 'returns an empty array when the payload carries no data' do + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'true' }) + .to_return(json('data' => nil)) + + expect(client.fetch_users).to eq([]) + end + + it 'wraps a failure in an APIError naming the endpoint' do + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'true' }) + .to_return(json({ 'message' => 'boom' }, 500)) + + expect { client.fetch_users } + .to raise_error(ForestAdminDatasourcePylon::APIError, /users: HTTP 500 boom/) + end + end + + describe '#fetch_user' do + it 'unwraps the user' do + stub_request(:get, "#{base}/users/u1").to_return(json('data' => { 'id' => 'u1', 'name' => 'Ada' })) + + expect(client.fetch_user('u1')).to eq('id' => 'u1', 'name' => 'Ada') + end + end + + describe '#fetch_teams' do + it 'returns every team without sending any query parameter' do + stub_request(:get, "#{base}/teams").to_return(json('data' => [{ 'id' => 't1' }])) + + expect(client.fetch_teams).to eq([{ 'id' => 't1' }]) + expect(WebMock).to have_requested(:get, "#{base}/teams") + end + end + + describe '#fetch_team' do + it 'unwraps the team' do + stub_request(:get, "#{base}/teams/t1").to_return(json('data' => { 'id' => 't1', 'name' => 'Support' })) + + expect(client.fetch_team('t1')).to eq('id' => 't1', 'name' => 'Support') + end + end + describe 'rate limiting' do it 'retries a 429 and returns the eventual success' do stub_request(:get, "#{base}/me") From 1fba1a83f27f28ff20769662c87403ccc07a6a73 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 12 Aug 2026 18:28:24 +0200 Subject: [PATCH 2/8] feat(pylon): shared read pipeline and issue relations Extracts the cursor-walk search flow, sort warning and operator maps into BaseCollection and shared modules; declares the four ManyToOne relations on PylonIssue and adds the schema-driven RelationEmbedder. Co-Authored-By: Claude Fable 5 --- .../collections/base_collection.rb | 78 ++++++++- .../collections/issue.rb | 52 ++---- .../collections/issue/api_filters.rb | 71 ++------ .../collections/issue/schema_definition.rb | 30 +++- .../collections/issue/serializer.rb | 21 +-- .../collections/record_serialization.rb | 29 ++++ .../collections/relation_embedder.rb | 56 +++++++ .../query/operator_maps.rb | 70 ++++++++ .../collections/base_collection_spec.rb | 155 +++++++++++++++++- .../collections/issue_spec.rb | 39 ++++- 10 files changed, 476 insertions(+), 125 deletions(-) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/record_serialization.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb index 9c3d30d63..3e7fea06d 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb @@ -2,6 +2,8 @@ module ForestAdminDatasourcePylon module Collections class BaseCollection < ForestAdminDatasourceToolkit::Collection ColumnSchema = ForestAdminDatasourceToolkit::Schema::ColumnSchema + ManyToOneSchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema + OneToManySchema = ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators Branch = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch Leaf = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf @@ -41,6 +43,17 @@ def initialize(datasource, name, custom_fields: [], searchable: false, countable enable_count if countable end + # How a collection another one points at with a ManyToOne is read in bulk: + # the serialized records of `ids`, indexed by id, missing ids left out. + # + # Public because the caller is the pointing collection, a different object: + # going through the collection rather than through the client is what keeps + # a related record serialized by the collection owning its shape, instead + # of by a second field list kept in the embedder. + def records_indexed_by_id(_ids) + raise NotImplementedError, "#{self.class} did not implement records_indexed_by_id" + end + protected # Pylon has no `id` filter operator on /issues/search, so collections @@ -79,6 +92,33 @@ def ensure_searchless_lookup!(filter) 'Clear the search or drop the id condition.' end + # Forest asks for an offset/limit window, Pylon hands out cursor pages: the + # walker bridges the two, `search_page` performs one call, and the records + # it collected are serialized by the collection. + def search_records(caller, filter) + pylon_filter = build_pylon_filter(caller, filter) + search_text = filter&.search + offset, limit = translate_page(filter&.page) + + records = walker.walk(offset: offset, limit: limit) do |batch, cursor| + search_page(limit: batch, cursor: cursor, filter: pylon_filter, search_text: search_text) + end + records.map { |record| serialize(record) } + end + + # One page of the walk, as a Client::SearchPage: the endpoint and its + # parameter names belong to the collection, the walk does not. + def search_page(limit:, cursor:, filter:, search_text:) + raise NotImplementedError, "#{self.class} did not implement search_page" + end + + # Sliced after the lookup, not before, so ids that resolved to nothing + # (404) do not eat into the requested window. + def page_window(records, filter) + offset, limit = translate_page(filter&.page) + records[offset, limit] || [] + end + def build_pylon_filter(caller, filter) tree = filter&.condition_tree ensure_no_stray_id!(tree) @@ -90,6 +130,26 @@ def api_filters {} end + # An order no endpoint honours is reported rather than silently swallowed: + # the rows come back in whatever order the API imposes. + def warn_unsortable(sort) + return if sort.nil? || sort.empty? || default_pk_sort?(sort) + return if translate_sort(sort, sortable_fields).first + + ForestAdminDatasourcePylon.logger.warn(unsortable_warning) + end + + # Overridden by collections whose endpoint can sort server-side. + def sortable_fields + {} + end + + # Overridden to name the order the endpoint imposes instead, which is what + # tells the operator what they got in place of the order they asked for. + def unsortable_warning + "[forest_admin_datasource_pylon] #{name} cannot honour the requested order." + end + # An unknown field silently disables sorting: a Pylon endpoint only honours # the fixed allow-list its collection declares. def translate_sort(sort, allow_list) @@ -167,6 +227,10 @@ def allowed_custom_field_operators def define_schema = raise(NotImplementedError, "#{self.class} did not implement define_schema") def define_relations = raise(NotImplementedError, "#{self.class} did not implement define_relations") + def walker + @walker ||= Pagination::CursorWalker.new + end + def id_values(node) return nil unless node.is_a?(Leaf) && node.field == 'id' return nil unless [Operators::EQUAL, Operators::IN].include?(node.operator) @@ -179,13 +243,17 @@ def and_branch?(node) end # An `id` the short-circuit could not take out of the tree has no - # translation left: Pylon filters no id server-side, and an id under an OR - # cannot be narrowed to a lookup because the other side of the union would - # bring in records the lookup never fetched. The UI does offer both an - # `id equals` filter and the or/and toggle, so this is worth an error an + # translation left: the endpoint filters no id server-side, and an id under + # an OR cannot be narrowed to a lookup because the other side of the union + # would bring in records the lookup never fetched. The UI does offer both + # an `id equals` filter and the or/and toggle, so this is worth an error an # operator can act on rather than the translator's "add it to api_filters". + # + # A collection whose endpoint does filter id declares it in `api_filters` + # and never short-circuits, so the translator handles its ids like any + # other field and there is nothing to refuse. def ensure_no_stray_id!(node) - return if node.nil? + return if node.nil? || api_filters.key?('id') return unless node.some_leaf { |leaf| leaf.field == 'id' } raise UnsupportedOperatorError, diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb index 4f6f62658..ac962fe8d 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb @@ -3,6 +3,7 @@ module Collections class Issue < BaseCollection include SchemaDefinition include Serializer + include RelationEmbedder # `/issues/search` exposes no sort parameter, so the allow-list is empty and # every requested order is reported instead of being silently swallowed. @@ -22,7 +23,10 @@ def initialize(datasource, custom_fields: []) end def list(caller, filter, projection) - fetch_records(caller, filter).map { |record| project(record, projection) } + records = fetch_records(caller, filter) + rows = records.map { |record| project(record, projection) } + embed_relations(records, rows, projection) + rows end protected @@ -41,6 +45,19 @@ def allowed_custom_field_operators ApiFilters::CUSTOM_FIELD_OPS.keys end + def sortable_fields + PYLON_SORTABLE + end + + def unsortable_warning + '[forest_admin_datasource_pylon] PylonIssue cannot honour the requested order; ' \ + 'POST /issues/search always returns issues from the most recent to the oldest.' + end + + def search_page(limit:, cursor:, filter:, search_text:) + datasource.client.search_issues(limit: limit, cursor: cursor, filter: filter, search_text: search_text) + end + private def fetch_records(caller, filter) @@ -52,18 +69,6 @@ def fetch_records(caller, filter) page_window(records_by_id(caller, lookup), filter) end - def search_records(caller, filter) - pylon_filter = build_pylon_filter(caller, filter) - search_text = filter&.search - offset, limit = translate_page(filter&.page) - - issues = walker.walk(offset: offset, limit: limit) do |batch, cursor| - datasource.client.search_issues(limit: batch, cursor: cursor, - filter: pylon_filter, search_text: search_text) - end - issues.map { |issue| serialize(issue) } - end - # The records are already narrowed to the ids the filter asked for, so # applying the conditions left over by the short-circuit in memory cannot # return a record the API would have excluded. The reverse — dropping a @@ -76,13 +81,6 @@ def records_by_id(caller, lookup) lookup.residual.apply(records, self, timezone_for(caller)) end - # Sliced after the lookup, not before, so ids that resolved to nothing - # (404) do not eat into the requested window. - def page_window(records, filter) - offset, limit = translate_page(filter&.page) - records[offset, limit] || [] - end - def fetch_by_ids(ids) wanted = ids.first(MAX_ID_LOOKUPS) warn_truncated_lookup(ids.size) if ids.size > wanted.size @@ -107,20 +105,6 @@ def warn_truncated_lookup(asked) 'Narrow the selection to reach the records past this point.' ) end - - def warn_unsortable(sort) - return if sort.nil? || sort.empty? || default_pk_sort?(sort) - return if translate_sort(sort, PYLON_SORTABLE).first - - ForestAdminDatasourcePylon.logger.warn( - '[forest_admin_datasource_pylon] PylonIssue cannot honour the requested order; ' \ - 'POST /issues/search always returns issues from the most recent to the oldest.' - ) - end - - def walker - @walker ||= Pagination::CursorWalker.new - end end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb index 1dd108831..3f1524dfd 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb @@ -9,69 +9,30 @@ class Issue < BaseCollection # every column's `filter_operators` from it, so the schema cannot # advertise a filter the translator would then refuse. module ApiFilters - Operators = BaseCollection::Operators + Maps = Query::OperatorMaps - # `not_equal` is deliberately absent: the toolkit derives it from - # `not_in`, so declaring it would only add a second spelling. - EQUALITY = { Operators::EQUAL => 'equals', - Operators::IN => 'in', - Operators::NOT_IN => 'not_in' }.freeze + extend Maps::Table - PRESENCE = { Operators::PRESENT => 'is_set', - Operators::BLANK => 'is_unset' }.freeze - - # Declaring the bare comparisons rather than before/after is what lets - # the toolkit rewrite Today / PreviousWeek / ... into a pair of bounds, - # which is also why `time_range` never has to be emitted. - TIME = { Operators::GREATER_THAN => 'time_is_after', - Operators::LESS_THAN => 'time_is_before' }.freeze - - # Pylon exposes a single substring operator per direction and documents - # no case semantics for either, so both Forest spellings map onto the - # one operator -- in both directions, or the UI would offer a - # case-insensitive "contains" with no way to negate it. - FULL_TEXT = { Operators::CONTAINS => 'string_contains', - Operators::I_CONTAINS => 'string_contains', - Operators::NOT_CONTAINS => 'string_does_not_contain', - Operators::NOT_I_CONTAINS => 'string_does_not_contain' }.freeze - - # `tags` holds a list: `contains` asks whether one tag belongs to it, - # while `in` matches it against several candidates at once. - MEMBERSHIP = { Operators::CONTAINS => 'contains', - Operators::NOT_CONTAINS => 'does_not_contain', - Operators::IN => 'in', - Operators::NOT_IN => 'not_in' }.freeze + CUSTOM_FIELD_OPS = Maps::CUSTOM_FIELD_OPS # `param` carries the read-to-filter renames: an issue is read with # `type` / `resolution_time` / `latest_message_time` but filtered on # `issue_type` / `resolved_at` / `latest_message_activity_at`. API_FILTERS = { - 'state' => { ops: EQUALITY }, - 'type' => { param: 'issue_type', ops: EQUALITY.merge(PRESENCE) }, - 'account_id' => { ops: EQUALITY.merge(PRESENCE) }, - 'requester_id' => { ops: EQUALITY.merge(PRESENCE) }, - 'assignee_id' => { ops: EQUALITY.merge(PRESENCE) }, - 'team_id' => { ops: EQUALITY }, - 'title' => { ops: FULL_TEXT }, - 'body_html' => { ops: FULL_TEXT }, - 'tags' => { ops: MEMBERSHIP }, - 'created_at' => { ops: TIME }, - 'updated_at' => { ops: TIME }, - 'resolution_time' => { param: 'resolved_at', ops: TIME }, - 'latest_message_time' => { param: 'latest_message_activity_at', ops: TIME } + 'state' => { ops: Maps::EQUALITY }, + 'type' => { param: 'issue_type', ops: Maps::EQUALITY.merge(Maps::PRESENCE) }, + 'account_id' => { ops: Maps::EQUALITY.merge(Maps::PRESENCE) }, + 'requester_id' => { ops: Maps::EQUALITY.merge(Maps::PRESENCE) }, + 'assignee_id' => { ops: Maps::EQUALITY.merge(Maps::PRESENCE) }, + 'team_id' => { ops: Maps::EQUALITY }, + 'title' => { ops: Maps::FULL_TEXT }, + 'body_html' => { ops: Maps::FULL_TEXT }, + 'tags' => { ops: Maps::MEMBERSHIP }, + 'created_at' => { ops: Maps::TIME }, + 'updated_at' => { ops: Maps::TIME }, + 'resolution_time' => { param: 'resolved_at', ops: Maps::TIME }, + 'latest_message_time' => { param: 'latest_message_activity_at', ops: Maps::TIME } }.freeze - - # A custom field is filtered through its slug, so the operators come - # from the column the integrator declared rather than from a table. - CUSTOM_FIELD_OPS = EQUALITY.merge(PRESENCE).merge(TIME).merge(FULL_TEXT).freeze - - def self.forest_operators(field) - API_FILTERS.dig(field, :ops)&.keys || [] - end - - def self.for_custom_field(schema) - { ops: CUSTOM_FIELD_OPS.slice(*Array(schema&.filter_operators)) } - end end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb index ca59e19ec..526aa55ad 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb @@ -12,8 +12,9 @@ class Issue < BaseCollection # column missing from that table gets no operator, so the UI never offers # a filter Pylon would refuse. module SchemaDefinition - ColumnSchema = BaseCollection::ColumnSchema - Operators = BaseCollection::Operators + ColumnSchema = BaseCollection::ColumnSchema + ManyToOneSchema = BaseCollection::ManyToOneSchema + Operators = BaseCollection::Operators private @@ -24,9 +25,23 @@ def define_schema define_time_fields end - # Relations are declared in a later story, once the Account / Contact / - # User / Team collections exist to point at. - def define_relations; end + # The four parties of an issue, each pointing at the collection owning its + # shape: the flattened `*_id` columns stay, as the keys the relation is + # read through and as the columns the search endpoint filters. + # + # RelationEmbedder resolves them at read time, in bulk. The reverse sides + # are declared by the collections they belong to; `/issues/search` + # filters every one of these keys, so they are answered server-side. + def define_relations + add_field('account', ManyToOneSchema.new(foreign_collection: 'PylonAccount', + foreign_key: 'account_id', foreign_key_target: 'id')) + add_field('requester', ManyToOneSchema.new(foreign_collection: 'PylonContact', + foreign_key: 'requester_id', foreign_key_target: 'id')) + add_field('assignee', ManyToOneSchema.new(foreign_collection: 'PylonUser', + foreign_key: 'assignee_id', foreign_key_target: 'id')) + add_field('team', ManyToOneSchema.new(foreign_collection: 'PylonTeam', + foreign_key: 'team_id', foreign_key_target: 'id')) + end def define_identity_fields # Only equal/in: these are the two the primary-key short-circuit can @@ -53,8 +68,9 @@ def define_content_fields add_column('number_of_touches', 'Number') end - # Flattened from the nested `{id: …}` objects Pylon returns. They stay - # plain columns until the story that adds the related collections. + # Flattened from the nested `{id: …}` objects Pylon returns, and kept as + # columns next to the relations they are the keys of: they are what the + # search endpoint filters, on this side and on the reverse one. def define_party_fields %w[account_id requester_id assignee_id team_id].each { |field| add_column(field, 'String') } end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb index 16a8bc511..1294dba90 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb @@ -2,6 +2,8 @@ module ForestAdminDatasourcePylon module Collections class Issue < BaseCollection module Serializer + include RecordSerialization + PARTY_FIELDS = { 'account_id' => 'account', 'requester_id' => 'requester', 'assignee_id' => 'assignee', 'team_id' => 'team' }.freeze @@ -20,25 +22,6 @@ def serialize(issue) add_custom_field_values(record, attrs['custom_fields']) record end - - def nested_id(value) - value['id'] if value.is_a?(Hash) - end - - def add_custom_field_values(record, values) - custom_fields.each do |cf| - entry = values.is_a?(Hash) ? values[cf[:column_name]] : nil - record[cf[:column_name]] = custom_field_value(entry) - end - end - - # Pylon spells a custom field as `slug => {"slug": ..., "value": ...}`, - # with `"values": [...]` instead of `"value"` for multi-value fields. - def custom_field_value(entry) - return entry unless entry.is_a?(Hash) - - entry.key?('value') ? entry['value'] : entry['values'] - end end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/record_serialization.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/record_serialization.rb new file mode 100644 index 000000000..4024ab7fe --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/record_serialization.rb @@ -0,0 +1,29 @@ +module ForestAdminDatasourcePylon + module Collections + # The two parts of a Pylon payload every collection reads the same way: the + # nested `{ id: ... }` objects it flattens into foreign-key columns, and the + # custom fields the organization defined, which are columns of their own. + module RecordSerialization + private + + def nested_id(value) + value['id'] if value.is_a?(Hash) + end + + def add_custom_field_values(record, values) + custom_fields.each do |cf| + entry = values.is_a?(Hash) ? values[cf[:column_name]] : nil + record[cf[:column_name]] = custom_field_value(entry) + end + end + + # Pylon spells a custom field as `slug => {"slug": ..., "value": ...}`, + # with `"values": [...]` instead of `"value"` for multi-value fields. + def custom_field_value(entry) + return entry unless entry.is_a?(Hash) + + entry.key?('value') ? entry['value'] : entry['values'] + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb new file mode 100644 index 000000000..ac947d388 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb @@ -0,0 +1,56 @@ +module ForestAdminDatasourcePylon + module Collections + # Forest asks for a ManyToOne relation as `relation:field` entries in the + # projection and expects the related record nested under the relation name on + # every row. Pylon has no join and no include parameter, so the records are + # read from the foreign collection — in bulk, from the foreign keys the + # serialized records already carry, never one request per row. + # + # What to embed comes from the schema rather than from a list kept here: a + # collection embeds whatever ManyToOne relations it declares, and a relation + # the projection does not ask for costs no request at all. + module RelationEmbedder + ManyToOneSchema = BaseCollection::ManyToOneSchema + + private + + # `records` are the serialized records, carrying the foreign keys `project` + # strips off the rows; `rows` are the projected rows, in the same order. + def embed_relations(records, rows, projection) + projected_relations(projection).group_by { |_name, relation| relation.foreign_collection } + .each { |foreign, group| embed_foreign(foreign, group, records, rows) } + end + + # Grouped by foreign collection, so two relations pointing at the same one + # are answered by a single read and their ids are deduped together. + # + # A relation the projection asked for is written on every row, whether or + # not it resolved: a null foreign key, and a record the operator can no + # longer reach, both read as "no related record" rather than as a row + # missing the field. + def embed_foreign(foreign_collection, relations, records, rows) + ids = foreign_ids(records, relations) + foreign = ids.empty? ? {} : datasource.get_collection(foreign_collection).records_indexed_by_id(ids) + relations.each do |name, relation| + rows.each_with_index { |row, index| row[name] = foreign[records[index][relation.foreign_key]] } + end + end + + # A null foreign key asks for nothing, and the same id is asked for once + # however many rows point at it. + def foreign_ids(records, relations) + keys = relations.map { |_name, relation| relation.foreign_key } + records.flat_map { |record| keys.map { |key| record[key] } }.compact.uniq + end + + # `account:name` asks for the `account` relation; a projected column, and a + # relation that is not a ManyToOne, name no field to embed. + def projected_relations(projection) + Array(projection).map { |field| field.to_s.split(':').first }.uniq.filter_map do |name| + relation = schema[:fields][name] + [name, relation] if relation.is_a?(ManyToOneSchema) + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb new file mode 100644 index 000000000..ea52c11bc --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb @@ -0,0 +1,70 @@ +module ForestAdminDatasourcePylon + module Query + # The operator maps the Pylon search endpoints share. A map spells each + # Forest operator as the Pylon operator honouring it; a collection's + # `API_FILTERS` table then assembles, field by field, the maps its endpoint + # accepts according to the API reference. + # + # Sharing the maps is what keeps those tables readable as the allow-lists + # they transcribe, and keeps one wire spelling from being fixed in one + # collection and left wrong in the next. + module OperatorMaps + Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators + + # `not_equal` is deliberately absent: the toolkit derives it from + # `not_in`, so declaring it would only add a second spelling. + EQUALITY = { Operators::EQUAL => 'equals', + Operators::IN => 'in', + Operators::NOT_IN => 'not_in' }.freeze + + PRESENCE = { Operators::PRESENT => 'is_set', + Operators::BLANK => 'is_unset' }.freeze + + # Declaring the bare comparisons rather than before/after is what lets + # the toolkit rewrite Today / PreviousWeek / ... into a pair of bounds, + # which is also why `time_range` never has to be emitted. + TIME = { Operators::GREATER_THAN => 'time_is_after', + Operators::LESS_THAN => 'time_is_before' }.freeze + + # Pylon exposes a single substring operator and documents no case + # semantics for it, so both Forest spellings map onto that one operator -- + # or the UI would offer a case-sensitive "contains" and a case-insensitive + # one behaving identically. + SUBSTRING = { Operators::CONTAINS => 'string_contains', + Operators::I_CONTAINS => 'string_contains' }.freeze + + # The same, for the endpoints that also accept the negation. A field + # filtered through SUBSTRING alone must not advertise it: Pylon rejects + # `string_does_not_contain` where it is not documented. + FULL_TEXT = SUBSTRING.merge(Operators::NOT_CONTAINS => 'string_does_not_contain', + Operators::NOT_I_CONTAINS => 'string_does_not_contain').freeze + + # A list-valued field -- `tags`, `domains`: `contains` asks whether one + # value belongs to it, while `in` matches it against several candidates at + # once. + MEMBERSHIP = { Operators::CONTAINS => 'contains', + Operators::NOT_CONTAINS => 'does_not_contain', + Operators::IN => 'in', + Operators::NOT_IN => 'not_in' }.freeze + + # A custom field is filtered through its slug, so its operators come from + # the column the integrator declared rather than from a table; every + # search endpoint accepts this same set on one. + CUSTOM_FIELD_OPS = EQUALITY.merge(PRESENCE).merge(TIME).merge(FULL_TEXT).freeze + + # Extended by a collection's `ApiFilters` module, whose `API_FILTERS` is + # the single source of truth for what its endpoint filters: the schema + # derives every column's `filter_operators` from it, so it cannot + # advertise a filter the translator would then refuse. + module Table + def forest_operators(field) + self::API_FILTERS.dig(field, :ops)&.keys || [] + end + + def for_custom_field(schema) + { ops: CUSTOM_FIELD_OPS.slice(*Array(schema&.filter_operators)) } + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb index 99e769d89..9dd6e3957 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb @@ -10,8 +10,10 @@ def branch(aggregator, conditions) .new(aggregator, conditions) end - def filter(condition_tree: nil, search: nil) - ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree, search: search) + def filter(condition_tree: nil, search: nil, page: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, search: search, page: page + ) end def page(offset, limit) @@ -46,12 +48,48 @@ def define_relations; end public :extract_id_lookup, :project, :translate_page, :add_custom_fields, :translate_sort, :timezone_for, :build_pylon_filter, :api_filters, :default_pk_sort?, - :ensure_searchless_lookup! + :ensure_searchless_lookup!, :search_records, :page_window, :warn_unsortable + end + end + + # Implements the two hooks the read pipeline leaves to the collection: one + # page of the cursor walk, and the serialization of what it collected. + let(:searching_subclass) do + Class.new(subclass) do + attr_accessor :pages + attr_reader :calls + + # A field the endpoint filters, so the translated filter handed to + # `search_page` can be observed. + def api_filters + operators = Collections::BaseCollection::Operators + { 'state' => { ops: { operators::EQUAL => 'equals' } } } + end + + protected + + def search_page(limit:, cursor:, filter:, search_text:) + @calls ||= [] + @calls << { limit: limit, cursor: cursor, filter: filter, search_text: search_text } + @pages.shift || Client::SearchPage.new(records: [], next_cursor: nil) + end + + private + + def serialize(record) = record.merge('serialized' => true) end end let(:collection) { subclass.new(datasource, 'X') } + def searching(*pages) + searching_subclass.new(datasource, 'X').tap { |collection| collection.pages = pages } + end + + def search_page(records, next_cursor = nil) + Client::SearchPage.new(records: records, next_cursor: next_cursor) + end + describe 'subclass contract' do it 'raises NotImplementedError naming define_schema when the hook is missing' do expect { Class.new(described_class).new(datasource, 'X') } @@ -63,6 +101,17 @@ def define_relations; end expect { incomplete.new(datasource, 'X') }.to raise_error(NotImplementedError, /define_relations/) end + + it 'raises NotImplementedError naming search_page when the walk reaches the endpoint hook' do + expect { collection.search_records(nil, filter) }.to raise_error(NotImplementedError, /search_page/) + end + + # Reached only by a collection declaring a ManyToOne to this one: an + # unresolvable relation names the missing hook rather than embedding nil. + it 'raises NotImplementedError naming records_indexed_by_id when a relation points here' do + expect { collection.records_indexed_by_id(%w[uuid-1]) } + .to raise_error(NotImplementedError, /records_indexed_by_id/) + end end describe 'search/count flags' do @@ -302,6 +351,106 @@ def define_relations; end expect { collection.build_pylon_filter(nil, filter(condition_tree: node)) } .to raise_error(UnsupportedOperatorError, /has to be combined with 'and' conditions only/) end + + # A collection whose endpoint filters id server-side never short-circuits, + # so there is nothing an `or` could widen: id is translated like any other + # field, including under an aggregator. + it 'translates an id the collection declares in api_filters, even inside an or' do + filtering = Class.new(subclass) do + def api_filters + operators = Collections::BaseCollection::Operators + { 'id' => { ops: { operators::EQUAL => 'equals' } }, + 'state' => { ops: { operators::EQUAL => 'equals' } } } + end + end.new(datasource, 'X') + node = branch('Or', [leaf('id', operators::EQUAL, 'uuid-1'), leaf('state', operators::EQUAL, 'new')]) + + expect(filtering.build_pylon_filter(nil, filter(condition_tree: node))).to eq( + 'operator' => 'or', + 'subfilters' => [{ 'field' => 'id', 'operator' => 'equals', 'value' => 'uuid-1' }, + { 'field' => 'state', 'operator' => 'equals', 'value' => 'new' }] + ) + end + end + + describe '#search_records' do + it 'walks a single page and serializes what it collected' do + collection = searching(search_page([{ 'id' => 'a' }, { 'id' => 'b' }])) + + expect(collection.search_records(nil, filter)) + .to eq([{ 'id' => 'a', 'serialized' => true }, { 'id' => 'b', 'serialized' => true }]) + expect(collection.calls) + .to eq([{ limit: Client::MAX_SEARCH_LIMIT, cursor: nil, filter: nil, search_text: nil }]) + end + + # The walker asks for the window still missing and hands back the cursor of + # the previous page; the filter and the search stay the same throughout. + it 'follows the cursor until the requested window is covered' do + collection = searching(search_page([{ 'id' => 'a' }, { 'id' => 'b' }], 'c1'), + search_page([{ 'id' => 'c' }])) + query = filter(condition_tree: leaf('state', operators::EQUAL, 'new'), search: 'boom', page: page(2, 1)) + + expect(collection.search_records(nil, query)).to eq([{ 'id' => 'c', 'serialized' => true }]) + expect(collection.calls).to eq( + [{ limit: 3, cursor: nil, filter: { 'field' => 'state', 'operator' => 'equals', 'value' => 'new' }, + search_text: 'boom' }, + { limit: 1, cursor: 'c1', filter: { 'field' => 'state', 'operator' => 'equals', 'value' => 'new' }, + search_text: 'boom' }] + ) + end + + it 'refuses a predicate the endpoint cannot express instead of searching unfiltered' do + collection = searching(search_page([{ 'id' => 'a' }])) + + expect { collection.search_records(nil, filter(condition_tree: leaf('type', operators::EQUAL, 'x'))) } + .to raise_error(UnsupportedOperatorError, /cannot filter on 'type'/) + expect(collection.calls).to be_nil + end + end + + describe '#page_window' do + let(:records) { [{ 'id' => 'a' }, { 'id' => 'b' }, { 'id' => 'c' }] } + + it 'slices the requested window out of the records' do + expect(collection.page_window(records, filter(page: page(1, 1)))).to eq([{ 'id' => 'b' }]) + end + + it 'returns every record when Forest asks for no page' do + expect(collection.page_window(records, nil)).to eq(records) + end + + it 'reports an empty window rather than nil past the last record' do + expect(collection.page_window(records, filter(page: page(10, 5)))).to eq([]) + end + end + + describe '#warn_unsortable' do + before { allow(ForestAdminDatasourcePylon.logger).to receive(:warn) } + + # The empty default matches an endpoint exposing no sort parameter: the + # order is reported instead of being silently swallowed. + it 'reports a chosen order the collection cannot honour, naming it' do + collection.warn_unsortable(sort('state')) + + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with('[forest_admin_datasource_pylon] X cannot honour the requested order.') + end + + it 'stays quiet on no order and on the default primary-key sort the agent injects' do + collection.warn_unsortable(nil) + collection.warn_unsortable([]) + collection.warn_unsortable(sort('id')) + + expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) + end + + it 'stays quiet on an order the endpoint does sort by' do + sorting = Class.new(subclass) { def sortable_fields = { 'state' => 'state' } }.new(datasource, 'X') + + sorting.warn_unsortable(sort('state')) + + expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) + end end describe '#project' do diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb index 53c3296a0..90dd61ff9 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb @@ -44,6 +44,11 @@ def issue_payload(id, overrides = {}) }.merge(overrides) end + # Relations are fields too; the assertions on the columns select them out. + def columns + collection.fields.select { |_name, field| field.type == 'Column' } + end + let(:datasource) { ForestAdminDatasourcePylon::Datasource.new(api_key: 'k') } let(:collection) { datasource.get_collection('PylonIssue') } let(:base) { datasource.configuration.url } @@ -79,8 +84,8 @@ def issue_payload(id, overrides = {}) # /issues/search exposes no sort parameter, and writes land in a later story. it 'declares every column read-only and non-sortable' do - expect(collection.fields.values.map(&:is_read_only).uniq).to eq([true]) - expect(collection.fields.values.map(&:is_sortable).uniq).to eq([false]) + expect(columns.values.map(&:is_read_only).uniq).to eq([true]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end # `search_text` is native on /issues/search, while Pylon exposes neither a @@ -116,6 +121,36 @@ def issue_payload(id, overrides = {}) end end + describe 'relations' do + let(:many_to_one) { ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema } + + it 'declares a ManyToOne for each of the four parties of an issue' do + expect(collection.fields.values_at('account', 'requester', 'assignee', 'team')) + .to all(be_a(many_to_one)) + end + + it 'points each one at the collection owning its shape, through the flattened foreign key' do + expect(collection.fields['account']) + .to have_attributes(foreign_collection: 'PylonAccount', foreign_key: 'account_id', + foreign_key_target: 'id') + expect(collection.fields['requester']) + .to have_attributes(foreign_collection: 'PylonContact', foreign_key: 'requester_id', + foreign_key_target: 'id') + expect(collection.fields['assignee']) + .to have_attributes(foreign_collection: 'PylonUser', foreign_key: 'assignee_id', + foreign_key_target: 'id') + expect(collection.fields['team']) + .to have_attributes(foreign_collection: 'PylonTeam', foreign_key: 'team_id', + foreign_key_target: 'id') + end + + # The key is what `/issues/search` filters, on this side and on the reverse + # one, so it stays a column of its own next to the relation. + it 'keeps the foreign keys as columns' do + expect(columns.keys).to include('account_id', 'requester_id', 'assignee_id', 'team_id') + end + end + describe '#list' do it 'searches for the most recent issues and serializes them' do stub_request(:post, "#{base}/issues/search").to_return(json('data' => [issue_payload('i1')])) From 8f5a0c22223321fa92e0b0fe5fa883f598c201b5 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 12 Aug 2026 18:29:06 +0200 Subject: [PATCH 3/8] feat(pylon): account and contact collections Cursor-paginated read-only collections with server-side filters, free-text search, a single-id fast path on the record endpoint and their relations to issues, contacts and account. Co-Authored-By: Claude Fable 5 --- .../collections/account.rb | 33 + .../collections/account/api_filters.rb | 41 ++ .../collections/account/schema_definition.rb | 85 +++ .../collections/account/serializer.rb | 21 + .../collections/contact.rb | 33 + .../collections/contact/api_filters.rb | 36 ++ .../collections/contact/schema_definition.rb | 84 +++ .../collections/contact/serializer.rb | 20 + .../collections/cursor_collection.rb | 142 +++++ .../collections/account_spec.rb | 570 ++++++++++++++++++ .../collections/contact_spec.rb | 474 +++++++++++++++ 11 files changed, 1539 insertions(+) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/serializer.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/serializer.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account.rb new file mode 100644 index 000000000..0f5a09f0f --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account.rb @@ -0,0 +1,33 @@ +module ForestAdminDatasourcePylon + module Collections + class Account < CursorCollection + include SchemaDefinition + include Serializer + + def initialize(datasource, custom_fields: []) + super(datasource, 'PylonAccount', custom_fields: custom_fields, searchable: true) + end + + protected + + def filter_table = ApiFilters + + def unsortable_warning + '[forest_admin_datasource_pylon] PylonAccount cannot honour the requested order; neither GET /accounts ' \ + 'nor POST /accounts/search takes a sort parameter, so accounts come back in the order the API imposes.' + end + + def search_page(limit:, cursor:, filter:, search_text:) + datasource.client.search_accounts(limit: limit, cursor: cursor, filter: filter, search_text: search_text) + end + + def list_page(limit:, cursor:) + datasource.client.list_accounts(limit: limit, cursor: cursor) + end + + def fetch_one(id) + datasource.client.fetch_account(id) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb new file mode 100644 index 000000000..eba88466d --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb @@ -0,0 +1,41 @@ +module ForestAdminDatasourcePylon + module Collections + class Account < CursorCollection + # The allow-list of `POST /accounts/search`, transcribed from the API + # reference: a field absent from this table cannot be filtered at all, and + # an operator absent from a field's map is rejected by Pylon. + # + # It is the single source of truth for filtering — `define_schema` derives + # every column's `filter_operators` from it, so the schema cannot + # advertise a filter the translator would then refuse. + module ApiFilters + Maps = Query::OperatorMaps + + extend Maps::Table + + CUSTOM_FIELD_OPS = Maps::CUSTOM_FIELD_OPS + + # `id` is filtered server-side here, which is what spares this + # collection the primary-key short-circuit Issue needs. + # + # `name` gets SUBSTRING rather than FULL_TEXT: the endpoint accepts + # `string_contains` but no negation of it. `external_ids` is left out + # entirely although the endpoint filters it — the API matches the bare + # external-id strings while the column shows `{external_id, label}` + # objects, so the filter would run on something the operator cannot see. + # The account read endpoint accepts an external id in place of the + # primary key, which is the way to reach a record by one. + # + # No time field is filterable: `created_at`, `updated_at` and + # `latest_customer_activity_time` are absent from the allow-list. + API_FILTERS = { + 'id' => { ops: Maps::EQUALITY }, + 'name' => { ops: Maps::EQUALITY.merge(Maps::SUBSTRING) }, + 'domains' => { ops: Maps::MEMBERSHIP }, + 'tags' => { ops: Maps::MEMBERSHIP }, + 'owner_id' => { ops: Maps::EQUALITY.merge(Maps::PRESENCE) } + }.freeze + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb new file mode 100644 index 000000000..50bc32f89 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb @@ -0,0 +1,85 @@ +module ForestAdminDatasourcePylon + module Collections + class Account < CursorCollection + # Every column is read-only in this story: writes land in a later one. No + # column is sortable either — neither `GET /accounts` nor + # `POST /accounts/search` exposes a sort parameter, so advertising a + # sortable column would let the UI ask for an order the API cannot honour. + # + # Filter operators are not chosen here: they come from + # `ApiFilters::API_FILTERS`, which mirrors the allow-list of the API. A + # column missing from that table gets no operator, so the UI never offers + # a filter Pylon would refuse. + module SchemaDefinition + ColumnSchema = BaseCollection::ColumnSchema + OneToManySchema = BaseCollection::OneToManySchema + + private + + def define_schema + define_identity_fields + define_domain_fields + define_ownership_fields + define_integration_fields + define_time_fields + end + + # The reverse sides of the two ManyToOne relations pointing here. Both + # `/issues/search` and `/contacts/search` filter `account_id` + # server-side, so a related list is one request and no in-memory pass. + # + # `owner_id` stays a plain column: it does point at a PylonUser, and the + # embedder would resolve it like any other key, but nothing in the panel + # asks for the owner of an account yet. + def define_relations + add_field('issues', OneToManySchema.new(foreign_collection: 'PylonIssue', + origin_key: 'account_id', origin_key_target: 'id')) + add_field('contacts', OneToManySchema.new(foreign_collection: 'PylonContact', + origin_key: 'account_id', origin_key_target: 'id')) + end + + def define_identity_fields + add_field('id', ColumnSchema.new(column_type: 'String', + filter_operators: ApiFilters.forest_operators('id'), + is_primary_key: true, is_read_only: true)) + add_column('name', 'String') + # Left as String rather than Enum: Pylon ships customer / partner / + # prospect but lets an organization define its own account types. + add_column('type', 'String') + add_column('is_disabled', 'Boolean') + end + + # `domain` and `primary_domain` carry the same value; both are kept + # because Pylon returns both, and only the `domains` list is filterable. + def define_domain_fields + add_column('domain', 'String') + add_column('primary_domain', 'String') + add_column('domains', 'Json') + add_column('tags', 'Json') + end + + def define_ownership_fields + # Flattened from the nested `{ id: ..., email: ... }` object Pylon + # returns; a plain column, see `define_relations` above. + add_column('owner_id', 'String') + add_column('external_ids', 'Json') + end + + def define_integration_fields + add_column('channels', 'Json') + add_column('crm_settings', 'Json') + end + + def define_time_fields + %w[created_at updated_at latest_customer_activity_time].each { |field| add_column(field, 'Date') } + end + + def add_column(name, type) + add_field(name, ColumnSchema.new(column_type: type, + filter_operators: ApiFilters.forest_operators(name), + is_read_only: true)) + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/serializer.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/serializer.rb new file mode 100644 index 000000000..431c53123 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/serializer.rb @@ -0,0 +1,21 @@ +module ForestAdminDatasourcePylon + module Collections + class Account < CursorCollection + module Serializer + NATIVE_FIELDS = %w[id name type is_disabled domain primary_domain domains tags external_ids + channels crm_settings created_at updated_at + latest_customer_activity_time].freeze + + private + + def serialize(account) + attrs = account.is_a?(Hash) ? account : {} + record = NATIVE_FIELDS.to_h { |field| [field, attrs[field]] } + record['owner_id'] = nested_id(attrs['owner']) + add_custom_field_values(record, attrs['custom_fields']) + record + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact.rb new file mode 100644 index 000000000..822df0797 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact.rb @@ -0,0 +1,33 @@ +module ForestAdminDatasourcePylon + module Collections + class Contact < CursorCollection + include SchemaDefinition + include Serializer + + def initialize(datasource, custom_fields: []) + super(datasource, 'PylonContact', custom_fields: custom_fields, searchable: true) + end + + protected + + def filter_table = ApiFilters + + def unsortable_warning + '[forest_admin_datasource_pylon] PylonContact cannot honour the requested order; neither GET /contacts ' \ + 'nor POST /contacts/search takes a sort parameter, so contacts come back in the order the API imposes.' + end + + def search_page(limit:, cursor:, filter:, search_text:) + datasource.client.search_contacts(limit: limit, cursor: cursor, filter: filter, search_text: search_text) + end + + def list_page(limit:, cursor:) + datasource.client.list_contacts(limit: limit, cursor: cursor) + end + + def fetch_one(id) + datasource.client.fetch_contact(id) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb new file mode 100644 index 000000000..5207b2751 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb @@ -0,0 +1,36 @@ +module ForestAdminDatasourcePylon + module Collections + class Contact < CursorCollection + # The allow-list of `POST /contacts/search`, transcribed from the API + # reference: a field absent from this table cannot be filtered at all, and + # an operator absent from a field's map is rejected by Pylon. + # + # It is the single source of truth for filtering — `define_schema` derives + # every column's `filter_operators` from it, so the schema cannot + # advertise a filter the translator would then refuse. + module ApiFilters + Maps = Query::OperatorMaps + + extend Maps::Table + + CUSTOM_FIELD_OPS = Maps::CUSTOM_FIELD_OPS + + # `id` is filtered server-side here, which is what spares this + # collection the primary-key short-circuit Issue needs. + # + # `name` and `email` get SUBSTRING rather than FULL_TEXT: the endpoint + # accepts `string_contains` but no negation of it. `email` filters the + # primary address only, not the `emails` list. + # + # The contacts search offers nothing else: no presence check, no + # filter on the phone numbers, the portal role or the external ids. + API_FILTERS = { + 'id' => { ops: Maps::EQUALITY }, + 'name' => { ops: Maps::EQUALITY.merge(Maps::SUBSTRING) }, + 'email' => { ops: Maps::EQUALITY.merge(Maps::SUBSTRING) }, + 'account_id' => { ops: Maps::EQUALITY } + }.freeze + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb new file mode 100644 index 000000000..f84ccfeaf --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb @@ -0,0 +1,84 @@ +module ForestAdminDatasourcePylon + module Collections + class Contact < CursorCollection + # Every column is read-only in this story: writes land in a later one. No + # column is sortable either — neither `GET /contacts` nor + # `POST /contacts/search` exposes a sort parameter, so advertising a + # sortable column would let the UI ask for an order the API cannot honour. + # + # Filter operators are not chosen here: they come from + # `ApiFilters::API_FILTERS`, which mirrors the allow-list of the API. A + # column missing from that table gets no operator, so the UI never offers + # a filter Pylon would refuse. A contact carries no timestamp at all — + # Pylon returns none. + module SchemaDefinition + ColumnSchema = BaseCollection::ColumnSchema + ManyToOneSchema = BaseCollection::ManyToOneSchema + OneToManySchema = BaseCollection::OneToManySchema + + private + + def define_schema + define_identity_fields + define_contact_fields + define_portal_fields + end + + # `account_id` is both the key of the relation and a column the contacts + # search filters, which is what lets the account side list its contacts + # server-side and the embedder resolve the account of a page of contacts + # in one request. + # + # `requested_issues` rather than `issues`: a contact is the requester of + # an issue, never its assignee — that side belongs to PylonUser. + def define_relations + add_field('account', ManyToOneSchema.new(foreign_collection: 'PylonAccount', + foreign_key: 'account_id', foreign_key_target: 'id')) + add_field('requested_issues', OneToManySchema.new(foreign_collection: 'PylonIssue', + origin_key: 'requester_id', origin_key_target: 'id')) + end + + def define_identity_fields + add_field('id', ColumnSchema.new(column_type: 'String', + filter_operators: ApiFilters.forest_operators('id'), + is_primary_key: true, is_read_only: true)) + add_column('name', 'String') + # Flattened from the nested `{ id: ..., external_ids: ... }` object + # Pylon returns, and kept as a column next to the `account` relation + # it is the key of: the search endpoint filters it. + add_column('account_id', 'String') + # Read-only Json, and deliberately unfilterable although the search + # endpoint does not offer it either: the API matches bare external-id + # strings while the column shows `{external_id, label}` objects, so a + # filter would run on something the operator cannot see. + add_column('external_ids', 'Json') + end + + # `email` and `primary_phone_number` carry the primary value; the lists + # hold every address and number, and neither list is filterable. + def define_contact_fields + add_column('email', 'String') + add_column('emails', 'Json') + add_column('primary_phone_number', 'String') + add_column('phone_numbers', 'Json') + add_column('avatar_url', 'String') + end + + def define_portal_fields + # Left as String rather than Enum: Pylon documents no_access / member + # / admin, but an organization can define its own portal roles, which + # is what `portal_role_id` points at. + add_column('portal_role', 'String') + add_column('portal_role_id', 'String') + add_column('integration_user_ids', 'Json') + end + + def add_column(name, type) + add_field(name, ColumnSchema.new(column_type: type, + filter_operators: ApiFilters.forest_operators(name), + is_read_only: true)) + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/serializer.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/serializer.rb new file mode 100644 index 000000000..fc644edea --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/serializer.rb @@ -0,0 +1,20 @@ +module ForestAdminDatasourcePylon + module Collections + class Contact < CursorCollection + module Serializer + NATIVE_FIELDS = %w[id name email emails primary_phone_number phone_numbers avatar_url + portal_role portal_role_id external_ids integration_user_ids].freeze + + private + + def serialize(contact) + attrs = contact.is_a?(Hash) ? contact : {} + record = NATIVE_FIELDS.to_h { |field| [field, attrs[field]] } + record['account_id'] = nested_id(attrs['account']) + add_custom_field_values(record, attrs['custom_fields']) + record + end + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb new file mode 100644 index 000000000..aa0ff30d4 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb @@ -0,0 +1,142 @@ +module ForestAdminDatasourcePylon + module Collections + # Base for the collections Pylon exposes through three endpoints: a plain + # cursor-paginated listing (`GET /accounts`, `GET /contacts`: 60 requests per + # minute), a search over the same pages (`POST /accounts/search`: 20) and a + # single record (`GET /accounts/{id}`: 60). + # + # Their search endpoint filters `id` server-side, so — unlike Issue — they + # declare it in `api_filters` and never need the primary-key short-circuit: + # every predicate, `id` included and under an `or` as well, is translated and + # answered by one search request. The routing below is therefore only about + # spending the cheapest budget that answers the question exactly, never about + # what Pylon can express. + # + # None of these endpoints takes a sort parameter, so `sortable_fields` stays + # the empty default of the base and each collection names, through + # `unsortable_warning`, the order it got instead of the one it asked for. + class CursorCollection < BaseCollection + include RecordSerialization + include RelationEmbedder + + # Pylon documents no maximum number of values on an `in` filter; the chunk + # keeps the request body and the page answering it bounded. + ID_CHUNK_SIZE = 100 + + def list(caller, filter, projection) + records = fetch_records(caller, filter) + rows = records.map { |record| project(record, projection) } + embed_relations(records, rows, projection) + rows + end + + # The search endpoint filters `id` server-side, which is what lets a whole + # page of foreign keys be read in one request per chunk. + def records_indexed_by_id(ids) + ids.each_slice(ID_CHUNK_SIZE).with_object({}) do |chunk, indexed| + search_by_ids(chunk).each { |record| indexed[record['id']] = record } + end + end + + protected + + # The `ApiFilters` module of the collection, whose table is the single + # source of truth for what its search endpoint filters. + def filter_table = raise(NotImplementedError, "#{self.class} did not implement filter_table") + + # One page of the listing endpoint, as a Client::SearchPage. + def list_page(limit:, cursor:) = raise(NotImplementedError, "#{self.class} did not implement list_page") + + # One record straight from its own endpoint. + def fetch_one(id) = raise(NotImplementedError, "#{self.class} did not implement fetch_one") + + # A custom field is filtered through its Pylon slug, with the operators the + # integrator declared on the column. + def api_filters + @api_filters ||= custom_fields.each_with_object(filter_table::API_FILTERS.dup) do |cf, filters| + filters[cf[:column_name]] = filter_table.for_custom_field(cf[:schema]) + end + end + + # Declarations outside this list are dropped at registration, so the + # schema never advertises an operator the translator would refuse. + def allowed_custom_field_operators + filter_table::CUSTOM_FIELD_OPS.keys + end + + private + + # The `id` filter goes through the translator rather than being written by + # hand, so the shape on the wire is the one this collection's `api_filters` + # produce — one spelling of an id filter, not two to keep in step. The + # cursor is followed defensively: a chunk is asked for as a single page, + # and Pylon is free to answer it over several. + def search_by_ids(ids) + pylon_filter = Query::ConditionTreeTranslator.call(Leaf.new('id', Operators::IN, ids), + api_filters: api_filters) + records = walker.walk(offset: 0, limit: ids.size) do |batch, cursor| + search_page(limit: batch, cursor: cursor, filter: pylon_filter, search_text: nil) + end + records.map { |record| serialize(record) } + end + + def fetch_records(caller, filter) + warn_unsortable(filter&.sort) + return listed_records(filter) if browsing?(filter) + + id = single_id_lookup(filter) + return page_window(records_by_id(id), filter) if id + + search_records(caller, filter) + end + + # Nothing to filter and nothing to search: the listing endpoint returns + # the same records for a budget three times larger than the search one. + def browsing?(filter) + return true if filter.nil? + + filter.condition_tree.nil? && no_search?(filter) + end + + # The walk of `search_records`, over the listing endpoint: it hands out + # cursor pages just the same, it only takes no filter. + def listed_records(filter) + offset, limit = translate_page(filter&.page) + + records = walker.walk(offset: offset, limit: limit) { |batch, cursor| list_page(limit: batch, cursor: cursor) } + records.map { |record| serialize(record) } + end + + # A record detail is `id equals X` alone: reading it through the record + # endpoint keeps the search budget for the pages that need it. + # + # Only a bare leaf takes that path. An `and` also carrying a scope is left + # to the search endpoint, which filters the id and the rest server-side in + # one request — where a lookup would have to apply the leftovers in memory, + # and would refuse the ones it cannot evaluate there. + def single_id_lookup(filter) + tree = filter.condition_tree + return nil unless tree.is_a?(Leaf) && no_search?(filter) + + ids = extract_id_lookup(tree)&.ids + ids&.one? ? ids.first : nil + end + + # A record the operator can no longer reach — deleted, or outside the + # token's scope — reads as "no record" rather than as a failed page. + def records_by_id(id) + record = fetch_one(id) + record.nil? ? [] : [serialize(record)] + rescue APIError => e + raise unless e.status == 404 + + [] + end + + # The search box sends an empty string once the operator clears it. + def no_search?(filter) + filter.search.to_s.strip.empty? + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb new file mode 100644 index 000000000..260d895dd --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb @@ -0,0 +1,570 @@ +module ForestAdminDatasourcePylon + RSpec.describe Collections::Account do + def filter(condition_tree: nil, search: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, search: search, page: page, sort: sort + ) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def branch(aggregator, conditions) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + .new(aggregator, conditions) + end + + def id_leaf(operator, value) + leaf('id', operator, value) + end + + def page(offset, limit) + ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) + end + + def sort_on(field, ascending: true) + ForestAdminDatasourceToolkit::Components::Query::Sort.new([{ field: field, ascending: ascending }]) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + # Trimmed to the shape observed on the API: the owner is a nested object + # carrying an id, and unset values come back as null rather than absent. + def account_payload(id, overrides = {}) + { + 'id' => id, 'name' => 'Acme', 'type' => 'customer', 'is_disabled' => false, + 'domain' => 'acme.com', 'primary_domain' => 'acme.com', 'domains' => %w[acme.com acme.io], + 'tags' => %w[vip], 'owner' => { 'id' => 'usr-1', 'email' => 'ada@acme.com' }, + 'external_ids' => [{ 'external_id' => 'crm-1', 'label' => 'salesforce' }], + 'channels' => [{ 'channel_id' => 'C1', 'source' => 'slack', 'is_primary' => true }], + 'crm_settings' => { 'details' => [{ 'id' => 'crm-1', 'source' => 'salesforce' }] }, + 'custom_fields' => {}, 'created_at' => '2026-08-07T13:06:22Z', 'updated_at' => '2026-08-10T09:00:00Z', + 'latest_customer_activity_time' => nil + }.merge(overrides) + end + + # Relations are fields too; the assertions on the columns select them out. + def columns + collection.fields.select { |_name, field| field.type == 'Column' } + end + + let(:datasource) { ForestAdminDatasourcePylon::Datasource.new(api_key: 'k') } + let(:collection) { described_class.new(datasource) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + def stub_list(query, payload) + stub_request(:get, "#{base}/accounts").with(query: query).to_return(json(payload)) + end + + def stub_search(payload = { 'data' => [account_payload('acc-1')] }) + stub_request(:post, "#{base}/accounts/search").to_return(json(payload)) + end + + describe 'schema' do + it 'is named PylonAccount' do + expect(collection.name).to eq('PylonAccount') + end + + it 'declares id as the primary key' do + expect(collection.fields['id'].is_primary_key).to be(true) + end + + # No short-circuit to serve here: /accounts/search filters id itself, so + # the column advertises every operator the endpoint accepts on it. + it 'advertises the id operators the search endpoint filters server-side' do + expect(collection.fields['id'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN]) + end + + it 'exposes the native columns observed on the API' do + expect(collection.fields.keys).to include( + 'name', 'type', 'is_disabled', 'domain', 'primary_domain', 'domains', 'tags', + 'owner_id', 'external_ids', 'channels', 'crm_settings', + 'created_at', 'updated_at', 'latest_customer_activity_time' + ) + end + + it 'flattens the owner into a foreign-key column instead of exposing the nested object' do + expect(collection.fields.keys).not_to include('owner') + end + + it 'types the lists as Json and the times as dates' do + expect(collection.fields['domains'].column_type).to eq('Json') + expect(collection.fields['channels'].column_type).to eq('Json') + expect(collection.fields['is_disabled'].column_type).to eq('Boolean') + expect(collection.fields['latest_customer_activity_time'].column_type).to eq('Date') + end + + # Neither endpoint exposes a sort parameter, and writes land in a later story. + it 'declares every column read-only and non-sortable' do + expect(columns.values.map(&:is_read_only).uniq).to eq([true]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) + end + + # `search_text` is native on /accounts/search, while Pylon exposes neither a + # count endpoint nor a total, so Count stays out until it can be throttled. + it 'enables search and leaves count disabled' do + expect(collection.is_searchable?).to be(true) + expect(collection.is_countable?).to be(false) + end + + it 'advertises only the operators the search allow-list accepts' do + expect(collection.fields['name'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::CONTAINS, operators::I_CONTAINS]) + expect(collection.fields['owner_id'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::PRESENT, operators::BLANK]) + expect(collection.fields['tags'].filter_operators) + .to eq([operators::CONTAINS, operators::NOT_CONTAINS, operators::IN, operators::NOT_IN]) + expect(collection.fields['domains'].filter_operators) + .to eq([operators::CONTAINS, operators::NOT_CONTAINS, operators::IN, operators::NOT_IN]) + end + + # /accounts/search accepts `string_contains` on a name but documents no + # negation of it, so the UI must not offer one. + it 'offers no negated substring on name' do + expect(collection.fields['name'].filter_operators) + .not_to include(operators::NOT_CONTAINS, operators::NOT_I_CONTAINS) + end + + # The endpoint does filter external_ids, on the bare id strings, while the + # column shows { external_id, label } objects: the filter would run on + # something the operator cannot see. + it 'advertises no operator on external_ids' do + expect(collection.fields['external_ids'].filter_operators).to eq([]) + end + + # Unlike /issues/search, the accounts search takes no time filter at all. + it 'advertises no operator on the time columns' do + %w[created_at updated_at latest_customer_activity_time].each do |field| + expect(collection.fields[field].filter_operators).to eq([]) + end + end + + it 'advertises no operator on the other columns Pylon cannot filter' do + %w[domain primary_domain type is_disabled channels crm_settings].each do |field| + expect(collection.fields[field].filter_operators).to eq([]) + end + end + end + + describe 'relations' do + let(:one_to_many) { ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema } + + # The reverse sides of the ManyToOne relations Issue and Contact declare. + # Both endpoints filter `account_id` server-side, so a related list is one + # request and no in-memory pass. + it 'declares the issues and the contacts of an account as OneToMany relations' do + expect(collection.fields.values_at('issues', 'contacts')).to all(be_a(one_to_many)) + expect(collection.fields['issues']) + .to have_attributes(foreign_collection: 'PylonIssue', origin_key: 'account_id', + origin_key_target: 'id') + expect(collection.fields['contacts']) + .to have_attributes(foreign_collection: 'PylonContact', origin_key: 'account_id', + origin_key_target: 'id') + end + + # It points at a PylonUser, but nothing in the panel asks for the owner of + # an account yet. + it 'leaves the owner a plain column' do + expect(collection.fields['owner_id'].type).to eq('Column') + expect(collection.fields.keys).not_to include('owner') + end + end + + # No condition tree and no search: the listing endpoint returns the same + # records for 60 requests per minute where the search endpoint allows 20. + describe '#list without a filter' do + it 'browses the listing endpoint and serializes what it returns' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + rows = collection.list(nil, filter, nil) + + expect(rows.size).to eq(1) + expect(rows.first).to include('id' => 'acc-1', 'name' => 'Acme', 'type' => 'customer', + 'domains' => %w[acme.com acme.io], 'tags' => %w[vip], + 'is_disabled' => false, 'latest_customer_activity_time' => nil) + end + + it 'flattens the nested owner into a foreign-key column' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + expect(collection.list(nil, filter, nil).first).to include('owner_id' => 'usr-1') + end + + it 'keeps the nested object out of the serialized record' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + expect(collection.list(nil, filter, nil).first.keys).not_to include('owner') + end + + it 'reports no owner rather than raising when the account has none' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1', 'owner' => nil)]) + + expect(collection.list(nil, filter, nil).first).to include('owner_id' => nil) + end + + it 'restricts the record to the projection' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + expect(collection.list(nil, filter, %w[id name])).to eq([{ 'id' => 'acc-1', 'name' => 'Acme' }]) + end + + it 'never spends the search budget' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + collection.list(nil, filter, %w[id]) + + expect(WebMock).not_to have_requested(:post, "#{base}/accounts/search") + end + + # The search box sends an empty string once the operator clears it, which + # is not a search and must not cost a search request. + it 'browses when the search is blank' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + expect(collection.list(nil, filter(search: ' '), %w[id])).to eq([{ 'id' => 'acc-1' }]) + end + + it 'browses when Forest sends no filter at all' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + expect(collection.list(nil, nil, %w[id])).to eq([{ 'id' => 'acc-1' }]) + end + + it 'forwards the requested page as the listing limit' do + stub_list({ 'limit' => '1' }, 'data' => [account_payload('acc-1')]) + + collection.list(nil, filter(page: page(0, 1)), nil) + + expect(WebMock).to have_requested(:get, "#{base}/accounts").with(query: { 'limit' => '1' }) + end + + it 'walks the cursor until the requested window is covered' do + stub_list({ 'limit' => '3' }, + 'data' => [account_payload('acc-1'), account_payload('acc-2')], + 'pagination' => { 'cursor' => 'c1', 'has_next_page' => true }) + stub_list({ 'limit' => '1', 'cursor' => 'c1' }, 'data' => [account_payload('acc-3')]) + + expect(collection.list(nil, filter(page: page(2, 1)), %w[id])).to eq([{ 'id' => 'acc-3' }]) + end + + it 'returns an empty list when the organization has no account' do + stub_list({ 'limit' => '1000' }, 'data' => []) + + expect(collection.list(nil, filter, nil)).to eq([]) + end + end + + describe '#list with a filter' do + before { stub_search } + + it 'sends the translated condition tree to the search endpoint' do + collection.list(nil, filter(condition_tree: leaf('name', operators::I_CONTAINS, 'acm')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: { 'limit' => Client::MAX_SEARCH_LIMIT, + 'filter' => { 'field' => 'name', 'operator' => 'string_contains', 'value' => 'acm' } } + ) + end + + it 'translates a membership filter on a list column' do + collection.list(nil, filter(condition_tree: leaf('tags', operators::CONTAINS, 'vip')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including('filter' => { 'field' => 'tags', 'operator' => 'contains', 'value' => 'vip' }) + ) + end + + it 'translates a presence filter with no value' do + collection.list(nil, filter(condition_tree: leaf('owner_id', operators::BLANK)), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including('filter' => { 'field' => 'owner_id', 'operator' => 'is_unset' }) + ) + end + + it 'sends a free-text search as search_text, intersected with the filter' do + query = filter(condition_tree: leaf('tags', operators::IN, %w[vip]), search: 'acme') + + collection.list(nil, query, %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: { 'limit' => Client::MAX_SEARCH_LIMIT, 'search_text' => 'acme', + 'filter' => { 'field' => 'tags', 'operator' => 'in', 'values' => %w[vip] } } + ) + end + + # The listing endpoint cannot search, so a search alone is worth the + # search endpoint even with nothing to filter. + it 'searches on a free-text search alone' do + collection.list(nil, filter(search: 'acme'), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: { 'limit' => Client::MAX_SEARCH_LIMIT, 'search_text' => 'acme' }) + expect(WebMock).not_to have_requested(:get, "#{base}/accounts") + end + + # `id` is a filter field of this endpoint, so it needs no short-circuit and + # an `or` cannot widen anything: it is translated like any other field. + it 'translates an id filter server-side, even under an or' do + tree = branch('Or', [id_leaf(operators::EQUAL, 'acc-1'), leaf('name', operators::EQUAL, 'Acme')]) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including( + 'filter' => { 'operator' => 'or', + 'subfilters' => [{ 'field' => 'id', 'operator' => 'equals', 'value' => 'acc-1' }, + { 'field' => 'name', 'operator' => 'equals', 'value' => 'Acme' }] } + ) + ) + end + + # The whole point of translating rather than dropping: a predicate Pylon + # cannot express fails loudly instead of returning unfiltered rows. + it 'raises rather than returning unfiltered rows for a field Pylon cannot filter' do + query = filter(condition_tree: leaf('created_at', operators::GREATER_THAN, '2026-01-01T00:00:00Z')) + + expect { collection.list(nil, query, %w[id]) } + .to raise_error(UnsupportedOperatorError, /cannot filter on 'created_at'/) + expect(WebMock).not_to have_requested(:post, "#{base}/accounts/search") + end + + it 'raises rather than searching for an operator the endpoint refuses on a field' do + expect { collection.list(nil, filter(condition_tree: leaf('name', operators::NOT_CONTAINS, 'a')), %w[id]) } + .to raise_error(UnsupportedOperatorError, /not supported on field 'name'/) + end + + it 'keeps the same filter across every page of the walk' do + stub_request(:post, "#{base}/accounts/search") + .with(body: hash_including('limit' => 3)) + .to_return(json('data' => [account_payload('acc-1'), account_payload('acc-2')], + 'pagination' => { 'cursor' => 'c1', 'has_next_page' => true })) + stub_request(:post, "#{base}/accounts/search") + .with(body: hash_including('cursor' => 'c1')) + .to_return(json('data' => [account_payload('acc-3')])) + query = filter(condition_tree: leaf('tags', operators::CONTAINS, 'vip'), page: page(2, 1)) + + expect(collection.list(nil, query, %w[id])).to eq([{ 'id' => 'acc-3' }]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: hash_including('filter' => { 'field' => 'tags', 'operator' => 'contains', + 'value' => 'vip' })).twice + end + end + + # A record detail is `id equals X` alone: reading it through GET + # /accounts/{id} spends the 60 requests/minute budget instead of the 20 of + # the search endpoint. + describe '#list on a single-id filter' do + it 'reads the account through its own endpoint instead of searching' do + stub_request(:get, "#{base}/accounts/acc-1").to_return(json('data' => account_payload('acc-1'))) + + rows = collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1')), %w[id name]) + + expect(rows).to eq([{ 'id' => 'acc-1', 'name' => 'Acme' }]) + expect(WebMock).not_to have_requested(:post, "#{base}/accounts/search") + end + + it 'still reads by id when the search is empty' do + stub_request(:get, "#{base}/accounts/acc-1").to_return(json('data' => account_payload('acc-1'))) + query = filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1'), search: '') + + expect(collection.list(nil, query, %w[id])).to eq([{ 'id' => 'acc-1' }]) + end + + it 'reports no record when the account no longer exists' do + stub_request(:get, "#{base}/accounts/gone").to_return(json({ 'message' => 'not found' }, 404)) + + expect(collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'gone')), %w[id])).to eq([]) + end + + # A blank body would otherwise serialize into a record whose every column, + # id included, is null: a row the panel shows and cannot open. + it 'reports no record rather than a blank one when Pylon answers with no data' do + stub_request(:get, "#{base}/accounts/acc-1").to_return(status: 200, body: '') + + expect(collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1')), %w[id])).to eq([]) + end + + it 'propagates a failure that is not a missing record' do + stub_request(:get, "#{base}/accounts/acc-1").to_return(json({ 'message' => 'boom' }, 500)) + + expect { collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1')), %w[id]) } + .to raise_error(APIError) + end + + it 'applies the requested page to the record it read' do + stub_request(:get, "#{base}/accounts/acc-1").to_return(json('data' => account_payload('acc-1'))) + query = filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1'), page: page(1, 1)) + + expect(collection.list(nil, query, %w[id])).to eq([]) + end + + # One search request answers several ids exactly, where one GET per id + # would burn the budget of the whole agent. + it 'searches instead when several ids are asked for' do + stub_search('data' => [account_payload('acc-1'), account_payload('acc-2')]) + + rows = collection.list(nil, filter(condition_tree: id_leaf(operators::IN, %w[acc-1 acc-2])), %w[id]) + + expect(rows).to eq([{ 'id' => 'acc-1' }, { 'id' => 'acc-2' }]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including('filter' => { 'field' => 'id', 'operator' => 'in', + 'values' => %w[acc-1 acc-2] }) + ) + end + + # Forest sends `AND(id equal X, )` on a record detail as soon as a + # scope or a segment is set. The search endpoint filters both server-side, + # so nothing has to be applied in memory -- and a scope on a list column, + # which no in-memory pass could evaluate, is answered rather than refused. + it 'searches instead when the filter carries more conditions' do + stub_search + tree = branch('And', [id_leaf(operators::EQUAL, 'acc-1'), leaf('tags', operators::CONTAINS, 'vip')]) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to eq([{ 'id' => 'acc-1' }]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including( + 'filter' => { 'operator' => 'and', + 'subfilters' => [{ 'field' => 'id', 'operator' => 'equals', 'value' => 'acc-1' }, + { 'field' => 'tags', 'operator' => 'contains', 'value' => 'vip' }] } + ) + ) + end + + # Both are honoured at once here, unlike on the collections whose endpoint + # cannot filter an id: the search intersects the filter server-side. + it 'searches instead when a search is combined with the id' do + stub_search + query = filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1'), search: 'acme') + + expect(collection.list(nil, query, %w[id])).to eq([{ 'id' => 'acc-1' }]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: { 'limit' => Client::MAX_SEARCH_LIMIT, 'search_text' => 'acme', + 'filter' => { 'field' => 'id', 'operator' => 'equals', 'value' => 'acc-1' } } + ) + end + + it 'searches instead when the id is filtered out rather than in' do + stub_search + collection.list(nil, filter(condition_tree: id_leaf(operators::NOT_IN, %w[acc-9])), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including('filter' => { 'field' => 'id', 'operator' => 'not_in', 'values' => %w[acc-9] }) + ) + end + end + + describe 'custom fields' do + let(:column) do + ForestAdminDatasourceToolkit::Schema::ColumnSchema.new(column_type: 'String', + filter_operators: [operators::EQUAL]) + end + let(:collection) do + described_class.new(datasource, custom_fields: [{ column_name: 'tier', schema: column }, + { column_name: 'zones', schema: column }]) + end + + it 'serializes single- and multi-value custom fields' do + fields = { 'tier' => { 'slug' => 'tier', 'value' => 'gold' }, + 'zones' => { 'slug' => 'zones', 'values' => %w[eu us] } } + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1', 'custom_fields' => fields)]) + + expect(collection.list(nil, filter, nil).first).to include('tier' => 'gold', 'zones' => %w[eu us]) + end + + it 'yields nil for a custom field the account does not carry' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1', 'custom_fields' => nil)]) + + expect(collection.list(nil, filter, nil).first).to include('tier' => nil, 'zones' => nil) + end + + # Pylon accepts a custom-field slug as a filter field, with the operators + # the integrator declared on the column. + it 'filters a custom field through its slug' do + stub_search('data' => []) + + collection.list(nil, filter(condition_tree: leaf('tier', operators::EQUAL, 'gold')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: hash_including('filter' => { 'field' => 'tier', 'operator' => 'equals', + 'value' => 'gold' })) + end + + it 'refuses an operator the custom field does not declare' do + expect { collection.list(nil, filter(condition_tree: leaf('tier', operators::CONTAINS, 'go')), %w[id]) } + .to raise_error(UnsupportedOperatorError, /not supported on field 'tier'/) + end + + # Clamped at registration, so the schema never advertises an operator the + # translator would refuse at query time. + it 'drops a declared operator Pylon cannot honour on a custom field and warns' do + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + declared = ForestAdminDatasourceToolkit::Schema::ColumnSchema + .new(column_type: 'String', filter_operators: [operators::EQUAL, operators::STARTS_WITH]) + + clamped = described_class.new(datasource, custom_fields: [{ column_name: 'tier', schema: declared }]) + + expect(clamped.fields['tier'].filter_operators).to eq([operators::EQUAL]) + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with(/cannot honour on a custom field \(starts_with\)/) + end + end + + describe '#list with a sort' do + before { allow(ForestAdminDatasourcePylon.logger).to receive(:warn) } + + # No Pylon endpoint of this collection takes a sort parameter, so the + # order is reported instead of being silently swallowed. + it 'warns that the requested order cannot be honoured, naming what happens instead' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + collection.list(nil, filter(sort: sort_on('name')), %w[id]) + + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with(/PylonAccount cannot honour the requested order.+order the API imposes/) + end + + it 'stays quiet when Forest asks for no order' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + collection.list(nil, filter, %w[id]) + + expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) + end + + # The agent injects an ascending primary-key sort whenever the request asks + # for no order; only an order someone actually chose is reported. + it 'stays quiet on the default primary-key sort the agent injects' do + stub_list({ 'limit' => '1000' }, 'data' => [account_payload('acc-1')]) + + collection.list(nil, filter(sort: sort_on('id')), %w[id]) + + expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) + end + + it 'warns on the search path too' do + stub_search + + collection.list(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'Acme'), + sort: sort_on('id', ascending: false)), %w[id]) + + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn).with(/cannot honour the requested order/) + end + + it 'warns on the record path too' do + stub_request(:get, "#{base}/accounts/acc-1").to_return(json('data' => account_payload('acc-1'))) + + collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1'), sort: sort_on('name')), + %w[id]) + + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn).with(/cannot honour the requested order/) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb new file mode 100644 index 000000000..0b2393f4f --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb @@ -0,0 +1,474 @@ +module ForestAdminDatasourcePylon + RSpec.describe Collections::Contact do + def filter(condition_tree: nil, search: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, search: search, page: page, sort: sort + ) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def branch(aggregator, conditions) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + .new(aggregator, conditions) + end + + def id_leaf(operator, value) + leaf('id', operator, value) + end + + def page(offset, limit) + ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) + end + + def sort_on(field, ascending: true) + ForestAdminDatasourceToolkit::Components::Query::Sort.new([{ field: field, ascending: ascending }]) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + # Trimmed to the shape observed on the API: the account is a nested object + # carrying an id, and unset values come back as null rather than absent. + def contact_payload(id, overrides = {}) + { + 'id' => id, 'name' => 'Ada Lovelace', 'email' => 'ada@acme.com', + 'emails' => %w[ada@acme.com ada@acme.io], 'account' => { 'id' => 'acc-1', 'external_ids' => nil }, + 'avatar_url' => 'https://usepylon.com/ada.png', 'portal_role' => 'admin', 'portal_role_id' => 'role-1', + 'primary_phone_number' => '+33100000000', 'phone_numbers' => %w[+33100000000], + 'external_ids' => [{ 'external_id' => 'crm-9', 'label' => 'hubspot' }], + 'integration_user_ids' => [{ 'id' => 'U1', 'source' => 'slack' }], 'custom_fields' => {} + }.merge(overrides) + end + + # Relations are fields too; the assertions on the columns select them out. + def columns + collection.fields.select { |_name, field| field.type == 'Column' } + end + + let(:datasource) { ForestAdminDatasourcePylon::Datasource.new(api_key: 'k') } + let(:collection) { described_class.new(datasource) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + def stub_list(query, payload) + stub_request(:get, "#{base}/contacts").with(query: query).to_return(json(payload)) + end + + def stub_search(payload = { 'data' => [contact_payload('con-1')] }) + stub_request(:post, "#{base}/contacts/search").to_return(json(payload)) + end + + describe 'schema' do + it 'is named PylonContact' do + expect(collection.name).to eq('PylonContact') + end + + it 'declares id as the primary key' do + expect(collection.fields['id'].is_primary_key).to be(true) + end + + # No short-circuit to serve here: /contacts/search filters id itself, so + # the column advertises every operator the endpoint accepts on it. + it 'advertises the id operators the search endpoint filters server-side' do + expect(collection.fields['id'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN]) + end + + it 'exposes the native columns observed on the API' do + expect(collection.fields.keys).to include( + 'name', 'email', 'emails', 'account_id', 'avatar_url', 'portal_role', 'portal_role_id', + 'primary_phone_number', 'phone_numbers', 'external_ids', 'integration_user_ids' + ) + end + + # The nested object Pylon returns becomes the key column; the `account` + # field of the schema is the relation read through that key. + it 'flattens the account into a foreign-key column instead of exposing the nested object' do + expect(columns.keys).to include('account_id') + expect(columns.keys).not_to include('account') + end + + # Pylon returns no timestamp at all on a contact. + it 'declares no time column' do + expect(columns.values.map(&:column_type)).not_to include('Date') + end + + it 'types the lists as Json' do + expect(collection.fields['emails'].column_type).to eq('Json') + expect(collection.fields['phone_numbers'].column_type).to eq('Json') + expect(collection.fields['integration_user_ids'].column_type).to eq('Json') + end + + # Neither endpoint exposes a sort parameter, and writes land in a later story. + it 'declares every column read-only and non-sortable' do + expect(columns.values.map(&:is_read_only).uniq).to eq([true]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) + end + + it 'enables search and leaves count disabled' do + expect(collection.is_searchable?).to be(true) + expect(collection.is_countable?).to be(false) + end + + it 'advertises only the operators the search allow-list accepts' do + expect(collection.fields['name'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::CONTAINS, operators::I_CONTAINS]) + expect(collection.fields['email'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::CONTAINS, operators::I_CONTAINS]) + expect(collection.fields['account_id'].filter_operators) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN]) + end + + # /contacts/search accepts `string_contains` but documents no negation of + # it, so the UI must not offer one. + it 'offers no negated substring on name and email' do + expect(collection.fields['name'].filter_operators) + .not_to include(operators::NOT_CONTAINS, operators::NOT_I_CONTAINS) + expect(collection.fields['email'].filter_operators) + .not_to include(operators::NOT_CONTAINS, operators::NOT_I_CONTAINS) + end + + # The endpoint filters no external id on a contact, and the column shows + # { external_id, label } objects the filter would not match anyway. + it 'advertises no operator on external_ids' do + expect(collection.fields['external_ids'].filter_operators).to eq([]) + end + + # `email` filters the primary address only: the lists, the phone numbers + # and the portal role are absent from the allow-list. + it 'advertises no operator on the columns Pylon cannot filter' do + %w[emails phone_numbers primary_phone_number avatar_url portal_role portal_role_id + integration_user_ids].each do |field| + expect(collection.fields[field].filter_operators).to eq([]) + end + end + end + + describe 'relations' do + it 'points at the account of a contact through the flattened foreign key' do + expect(collection.fields['account']) + .to be_a(ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema) + .and have_attributes(foreign_collection: 'PylonAccount', foreign_key: 'account_id', + foreign_key_target: 'id') + end + + # `requested_issues` rather than `issues`: a contact is the requester of an + # issue, never its assignee -- that side belongs to PylonUser. + it 'declares the issues a contact requested, read through requester_id' do + expect(collection.fields['requested_issues']) + .to be_a(ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema) + .and have_attributes(foreign_collection: 'PylonIssue', origin_key: 'requester_id', + origin_key_target: 'id') + end + end + + # No condition tree and no search: the listing endpoint returns the same + # records for 60 requests per minute where the search endpoint allows 20. + describe '#list without a filter' do + it 'browses the listing endpoint and serializes what it returns' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + rows = collection.list(nil, filter, nil) + + expect(rows.size).to eq(1) + expect(rows.first).to include('id' => 'con-1', 'name' => 'Ada Lovelace', 'email' => 'ada@acme.com', + 'emails' => %w[ada@acme.com ada@acme.io], 'portal_role' => 'admin', + 'primary_phone_number' => '+33100000000') + end + + it 'flattens the nested account into a foreign-key column' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + expect(collection.list(nil, filter, nil).first).to include('account_id' => 'acc-1') + end + + it 'keeps the nested object out of the serialized record' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + expect(collection.list(nil, filter, nil).first.keys).not_to include('account') + end + + it 'reports no account rather than raising when the contact has none' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1', 'account' => nil)]) + + expect(collection.list(nil, filter, nil).first).to include('account_id' => nil) + end + + it 'restricts the record to the projection' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + expect(collection.list(nil, filter, %w[id email])) + .to eq([{ 'id' => 'con-1', 'email' => 'ada@acme.com' }]) + end + + it 'never spends the search budget' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + collection.list(nil, filter, %w[id]) + + expect(WebMock).not_to have_requested(:post, "#{base}/contacts/search") + end + + it 'walks the cursor until the requested window is covered' do + stub_list({ 'limit' => '3' }, + 'data' => [contact_payload('con-1'), contact_payload('con-2')], + 'pagination' => { 'cursor' => 'c1', 'has_next_page' => true }) + stub_list({ 'limit' => '1', 'cursor' => 'c1' }, 'data' => [contact_payload('con-3')]) + + expect(collection.list(nil, filter(page: page(2, 1)), %w[id])).to eq([{ 'id' => 'con-3' }]) + end + + it 'returns an empty list when the organization has no contact' do + stub_list({ 'limit' => '1000' }, 'data' => []) + + expect(collection.list(nil, filter, nil)).to eq([]) + end + end + + describe '#list with a filter' do + before { stub_search } + + it 'sends the translated condition tree to the search endpoint' do + collection.list(nil, filter(condition_tree: leaf('email', operators::I_CONTAINS, '@acme')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: { 'limit' => Client::MAX_SEARCH_LIMIT, + 'filter' => { 'field' => 'email', 'operator' => 'string_contains', 'value' => '@acme' } } + ) + end + + it 'translates a filter on the account the contact belongs to' do + collection.list(nil, filter(condition_tree: leaf('account_id', operators::IN, %w[acc-1 acc-2])), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: hash_including('filter' => { 'field' => 'account_id', 'operator' => 'in', + 'values' => %w[acc-1 acc-2] }) + ) + end + + it 'sends a free-text search as search_text, intersected with the filter' do + query = filter(condition_tree: leaf('name', operators::EQUAL, 'Ada Lovelace'), search: 'ada') + + collection.list(nil, query, %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: { 'limit' => Client::MAX_SEARCH_LIMIT, 'search_text' => 'ada', + 'filter' => { 'field' => 'name', 'operator' => 'equals', 'value' => 'Ada Lovelace' } } + ) + end + + # The listing endpoint cannot search, so a search alone is worth the + # search endpoint even with nothing to filter. + it 'searches on a free-text search alone' do + collection.list(nil, filter(search: 'ada'), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search") + .with(body: { 'limit' => Client::MAX_SEARCH_LIMIT, 'search_text' => 'ada' }) + expect(WebMock).not_to have_requested(:get, "#{base}/contacts") + end + + # `id` is a filter field of this endpoint, so it needs no short-circuit and + # an `or` cannot widen anything: it is translated like any other field. + it 'translates an id filter server-side, even under an or' do + tree = branch('Or', [id_leaf(operators::EQUAL, 'con-1'), leaf('email', operators::EQUAL, 'ada@acme.com')]) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: hash_including( + 'filter' => { 'operator' => 'or', + 'subfilters' => [{ 'field' => 'id', 'operator' => 'equals', 'value' => 'con-1' }, + { 'field' => 'email', 'operator' => 'equals', + 'value' => 'ada@acme.com' }] } + ) + ) + end + + # The whole point of translating rather than dropping: a predicate Pylon + # cannot express fails loudly instead of returning unfiltered rows. + it 'raises rather than returning unfiltered rows for a field Pylon cannot filter' do + query = filter(condition_tree: leaf('portal_role', operators::EQUAL, 'admin')) + + expect { collection.list(nil, query, %w[id]) } + .to raise_error(UnsupportedOperatorError, /cannot filter on 'portal_role'/) + expect(WebMock).not_to have_requested(:post, "#{base}/contacts/search") + end + + it 'raises rather than searching for an operator the endpoint refuses on a field' do + query = filter(condition_tree: leaf('email', operators::NOT_I_CONTAINS, '@acme')) + + expect { collection.list(nil, query, %w[id]) } + .to raise_error(UnsupportedOperatorError, /not supported on field 'email'/) + end + end + + # A record detail is `id equals X` alone: reading it through GET + # /contacts/{id} spends the 60 requests/minute budget instead of the 20 of + # the search endpoint. + describe '#list on a single-id filter' do + it 'reads the contact through its own endpoint instead of searching' do + stub_request(:get, "#{base}/contacts/con-1").to_return(json('data' => contact_payload('con-1'))) + + rows = collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'con-1')), %w[id name]) + + expect(rows).to eq([{ 'id' => 'con-1', 'name' => 'Ada Lovelace' }]) + expect(WebMock).not_to have_requested(:post, "#{base}/contacts/search") + end + + it 'reports no record when the contact no longer exists' do + stub_request(:get, "#{base}/contacts/gone").to_return(json({ 'message' => 'not found' }, 404)) + + expect(collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'gone')), %w[id])).to eq([]) + end + + it 'propagates a failure that is not a missing record' do + stub_request(:get, "#{base}/contacts/con-1").to_return(json({ 'message' => 'boom' }, 500)) + + expect { collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'con-1')), %w[id]) } + .to raise_error(APIError) + end + + it 'applies the requested page to the record it read' do + stub_request(:get, "#{base}/contacts/con-1").to_return(json('data' => contact_payload('con-1'))) + query = filter(condition_tree: id_leaf(operators::EQUAL, 'con-1'), page: page(1, 1)) + + expect(collection.list(nil, query, %w[id])).to eq([]) + end + + # One search request answers several ids exactly, where one GET per id + # would burn the budget of the whole agent. + it 'searches instead when several ids are asked for' do + stub_search('data' => [contact_payload('con-1'), contact_payload('con-2')]) + + rows = collection.list(nil, filter(condition_tree: id_leaf(operators::IN, %w[con-1 con-2])), %w[id]) + + expect(rows).to eq([{ 'id' => 'con-1' }, { 'id' => 'con-2' }]) + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: hash_including('filter' => { 'field' => 'id', 'operator' => 'in', + 'values' => %w[con-1 con-2] }) + ) + end + + # Forest sends `AND(id equal X, )` on a record detail as soon as a + # scope or a segment is set; the search endpoint filters both server-side. + it 'searches instead when the filter carries more conditions' do + stub_search + tree = branch('And', [id_leaf(operators::EQUAL, 'con-1'), leaf('account_id', operators::EQUAL, 'acc-1')]) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to eq([{ 'id' => 'con-1' }]) + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: hash_including( + 'filter' => { 'operator' => 'and', + 'subfilters' => [{ 'field' => 'id', 'operator' => 'equals', 'value' => 'con-1' }, + { 'field' => 'account_id', 'operator' => 'equals', + 'value' => 'acc-1' }] } + ) + ) + end + + it 'searches instead when a search is combined with the id' do + stub_search + query = filter(condition_tree: id_leaf(operators::EQUAL, 'con-1'), search: 'ada') + + expect(collection.list(nil, query, %w[id])).to eq([{ 'id' => 'con-1' }]) + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").with( + body: { 'limit' => Client::MAX_SEARCH_LIMIT, 'search_text' => 'ada', + 'filter' => { 'field' => 'id', 'operator' => 'equals', 'value' => 'con-1' } } + ) + end + end + + describe 'custom fields' do + let(:column) do + ForestAdminDatasourceToolkit::Schema::ColumnSchema.new(column_type: 'String', + filter_operators: [operators::EQUAL]) + end + let(:collection) do + described_class.new(datasource, custom_fields: [{ column_name: 'seniority', schema: column }, + { column_name: 'products', schema: column }]) + end + + it 'serializes single- and multi-value custom fields' do + fields = { 'seniority' => { 'slug' => 'seniority', 'value' => 'champion' }, + 'products' => { 'slug' => 'products', 'values' => %w[api portal] } } + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1', 'custom_fields' => fields)]) + + expect(collection.list(nil, filter, nil).first) + .to include('seniority' => 'champion', 'products' => %w[api portal]) + end + + it 'yields nil for a custom field the contact does not carry' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1', 'custom_fields' => nil)]) + + expect(collection.list(nil, filter, nil).first).to include('seniority' => nil, 'products' => nil) + end + + it 'filters a custom field through its slug' do + stub_search('data' => []) + + collection.list(nil, filter(condition_tree: leaf('seniority', operators::EQUAL, 'champion')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search") + .with(body: hash_including('filter' => { 'field' => 'seniority', 'operator' => 'equals', + 'value' => 'champion' })) + end + + it 'refuses an operator the custom field does not declare' do + query = filter(condition_tree: leaf('seniority', operators::CONTAINS, 'cham')) + + expect { collection.list(nil, query, %w[id]) } + .to raise_error(UnsupportedOperatorError, /not supported on field 'seniority'/) + end + + # Clamped at registration, so the schema never advertises an operator the + # translator would refuse at query time. + it 'drops a declared operator Pylon cannot honour on a custom field and warns' do + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) + declared = ForestAdminDatasourceToolkit::Schema::ColumnSchema + .new(column_type: 'String', filter_operators: [operators::EQUAL, operators::STARTS_WITH]) + + clamped = described_class.new(datasource, custom_fields: [{ column_name: 'seniority', schema: declared }]) + + expect(clamped.fields['seniority'].filter_operators).to eq([operators::EQUAL]) + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with(/cannot honour on a custom field \(starts_with\)/) + end + end + + describe '#list with a sort' do + before { allow(ForestAdminDatasourcePylon.logger).to receive(:warn) } + + it 'warns that the requested order cannot be honoured, naming what happens instead' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + collection.list(nil, filter(sort: sort_on('email')), %w[id]) + + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with(/PylonContact cannot honour the requested order.+order the API imposes/) + end + + it 'stays quiet when Forest asks for no order and on the default primary-key sort' do + stub_list({ 'limit' => '1000' }, 'data' => [contact_payload('con-1')]) + + collection.list(nil, filter, %w[id]) + collection.list(nil, filter(sort: sort_on('id')), %w[id]) + + expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) + end + + it 'warns on the search path too' do + stub_search + + collection.list(nil, filter(condition_tree: leaf('email', operators::EQUAL, 'ada@acme.com'), + sort: sort_on('name')), %w[id]) + + expect(ForestAdminDatasourcePylon.logger).to have_received(:warn).with(/cannot honour the requested order/) + end + end + end +end From ad725d5f5f3647170052223a020e696786c4580e Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 12 Aug 2026 18:29:22 +0200 Subject: [PATCH 4/8] feat(pylon): user and team collections Fetch-all read-only collections: the endpoints return the complete dataset, so filtering, sorting and pagination run in memory over operators proven evaluable by the toolkit equivalence machinery. Co-Authored-By: Claude Fable 5 --- .../collections/fetch_all_collection.rb | 135 +++++++++ .../collections/team.rb | 43 +++ .../collections/user.rb | 63 ++++ .../collections/fetch_all_collection_spec.rb | 281 ++++++++++++++++++ .../collections/team_spec.rb | 216 ++++++++++++++ .../collections/user_spec.rb | 265 +++++++++++++++++ 6 files changed, 1003 insertions(+) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/team_spec.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/user_spec.rb diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb new file mode 100644 index 000000000..b2577dd17 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb @@ -0,0 +1,135 @@ +module ForestAdminDatasourcePylon + module Collections + # Base for the collections whose Pylon endpoint hands back the whole + # collection of the organization in a single response: `GET /users` and + # `GET /teams` take no cursor, no filter and no sort at all. + # + # Filtering, sorting and paginating that response in memory is exact rather + # than approximate: the records in hand ARE every record Pylon holds, so a + # window cut out of them carries the same rows a server-side query would + # have returned. This is what keeps the in-memory pass out of the trap this + # datasource refuses elsewhere — a result that looks filtered without being + # filtered — which only arises when a single page of a larger dataset is all + # one has. The cost is bandwidth, not correctness. + # + # Each `list` re-reads the endpoint, so what the operator sees is what Pylon + # holds now; the 60 requests/minute these endpoints allow is a budget the + # throttling story (EXT-13) owns. + class FetchAllCollection < BaseCollection + # The filters a column may advertise, per column type. Restricted to the + # operators `ConditionTreeLeaf#match` evaluates natively or + # `ConditionTreeEquivalent` rewrites into that native set, because the + # in-memory pass is the only pass there is here: an operator with no + # equivalence makes `match` return nil, which `apply` reads as "no match" + # and would silently empty the page instead of filtering it. + OPERATOR_CANDIDATES = { + 'String' => [Operators::EQUAL, Operators::NOT_EQUAL, Operators::IN, Operators::NOT_IN, + Operators::PRESENT, Operators::BLANK, Operators::CONTAINS, Operators::I_CONTAINS, + Operators::NOT_CONTAINS, Operators::STARTS_WITH, Operators::ENDS_WITH], + 'Boolean' => [Operators::EQUAL, Operators::NOT_EQUAL, Operators::IN, Operators::NOT_IN, + Operators::PRESENT, Operators::BLANK] + }.freeze + + # Candidates are re-checked against the toolkit rather than trusted, so an + # equivalence the toolkit stops providing takes the filter out of the + # schema instead of turning every page using it into an empty one. + def self.operators_for(column_type) + Array(OPERATOR_CANDIDATES[column_type]).select do |operator| + Equivalent.equivalent_tree?(operator, IN_MEMORY_OPERATORS, column_type) + end + end + + def list(caller, filter, projection) + records = fetch_all.map { |entity| serialize(entity) } + records = filter_in_memory(records, caller, filter) + records = sort_in_memory(records, filter&.sort) + + page_window(records, filter).map { |record| project(record, projection) } + end + + # One request answers any number of ids: the endpoint hands back the + # complete dataset, so the ids only pick rows out of it. Read again on + # every pass, like `list` — the freshness this collection trades bandwidth + # for is not worth losing to a cache of related records. + def records_indexed_by_id(ids) + fetch_all.map { |entity| serialize(entity) }.to_h { |record| [record['id'], record] }.slice(*ids) + end + + protected + + # Every column is read-only in this story: writes land in a later one. + # Scalar columns are sortable because the in-memory sort honours any order + # asked of them; a Json column is neither sortable nor filterable, as it + # holds a list whose Pylon semantics have no in-memory counterpart — the + # same reason the primary-key residual guard refuses one. + def add_column(name, type, is_primary_key: false) + add_field(name, ColumnSchema.new(column_type: type, + filter_operators: self.class.operators_for(type), + is_primary_key: is_primary_key, + is_sortable: type != 'Json', + is_read_only: true)) + end + + # The complete collection, straight from its unpaginated endpoint. + def fetch_all = raise(NotImplementedError, "#{self.class} did not implement fetch_all") + + # One Pylon entity flattened into a record matching the schema. + def serialize(_entity) = raise(NotImplementedError, "#{self.class} did not implement serialize") + + private + + # The tree is applied over the complete dataset, so the rows it keeps are + # the rows Pylon would have kept. `guard_nil_comparisons` is still worth + # its cost: nothing in the schema advertises a bare comparison, but a + # scope, a segment or a customizer can send one, and it would otherwise + # raise on the nulls Pylon returns for an unset column. + def filter_in_memory(records, caller, filter) + tree = guard_nil_comparisons(filter&.condition_tree) + return records if tree.nil? + + tree.apply(records, self, timezone_for(caller)) + end + + # Every requested order is honoured, including the ascending primary-key + # sort the agent injects when the request asks for none, so there is no + # unsortable order to report. + # + # Neither Ruby's `sort` nor the toolkit's `Sort#apply` can be used as is: + # `sort` is not stable, and `<=>` answers nil on a null, on two booleans + # and on mixed types, which leaves the comparator undefined and the order + # arbitrary. Ties therefore fall back to the position the API returned the + # record in, and values are compared by `compare_values`. + def sort_in_memory(records, sort) + clauses = normalized_sort_clauses(sort) + return records if clauses.empty? + + records.each_with_index.sort do |(left, left_index), (right, right_index)| + compare_clauses(left, right, clauses).nonzero? || (left_index <=> right_index) + end.map(&:first) + end + + def compare_clauses(left, right, clauses) + clauses.each do |field, ascending| + comparison = compare_values(left[field], right[field]) + next if comparison.zero? + + return ascending ? comparison : -comparison + end + + 0 + end + + # Nulls sort last on an ascending order and first on a descending one, the + # way a database orders them; values `<=>` cannot compare — two booleans, + # for one — are compared through their string form rather than left + # undefined, which puts `false` before `true`, again like a database. + def compare_values(left, right) + return 0 if left.nil? && right.nil? + return 1 if left.nil? + return -1 if right.nil? + + (left <=> right) || (left.to_s <=> right.to_s) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb new file mode 100644 index 000000000..889061a23 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb @@ -0,0 +1,43 @@ +module ForestAdminDatasourcePylon + module Collections + class Team < FetchAllCollection + def initialize(datasource, custom_fields: []) + super(datasource, 'PylonTeam', custom_fields: custom_fields) + end + + protected + + def fetch_all + datasource.client.fetch_teams + end + + # `users` is flattened to the ids of the members: the emails Pylon nests + # there belong to PylonUser, which is where they stay up to date. + def serialize(team) + attrs = team.is_a?(Hash) ? team : {} + + { 'id' => attrs['id'], 'name' => attrs['name'], + 'user_ids' => Array(attrs['users']).filter_map { |user| user['id'] if user.is_a?(Hash) } } + end + + private + + # `/issues/search` filters `team_id` server-side, so the issues assigned to + # a team are listed by one request. `user_ids` gets no relation: Pylon + # nests the members here rather than pointing at the team from a user, so + # the membership is a ManyToMany with no join collection to declare it on. + def define_relations + add_field('issues', OneToManySchema.new(foreign_collection: 'PylonIssue', + origin_key: 'team_id', origin_key_target: 'id')) + end + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('name', 'String') + # A list, so neither filterable nor sortable, and no relation either: + # see `define_relations` above. + add_column('user_ids', 'Json') + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb new file mode 100644 index 000000000..88768bed0 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb @@ -0,0 +1,63 @@ +module ForestAdminDatasourcePylon + module Collections + class User < FetchAllCollection + NATIVE_FIELDS = %w[id name email emails avatar_url status role_id is_deactivated].freeze + + def initialize(datasource, custom_fields: []) + super(datasource, 'PylonUser', custom_fields: custom_fields) + end + + protected + + # `include_deactivated` is left at the client default of true on purpose: + # a deactivated agent stays the assignee and the author of the issues they + # handled, and a record the rest of the panel points at has to stay + # readable. `is_deactivated` is exposed as a column so an operator can + # filter them out when they want to. + def fetch_all + datasource.client.fetch_users + end + + # `role` is flattened to its name only: its id is already carried by + # `role_id`, and the name is what an operator recognises. Its slug is left + # out — Pylon derives it from the name. + def serialize(user) + attrs = user.is_a?(Hash) ? user : {} + role = attrs['role'] + + NATIVE_FIELDS.to_h { |field| [field, attrs[field]] } + .merge('role_name' => role.is_a?(Hash) ? role['name'] : nil) + end + + private + + # `/issues/search` filters `assignee_id` server-side, so the issues of an + # agent are listed by one request. + # + # The teams of a user are left out: Pylon nests the members inside a team + # and exposes no team id on a user, so that side is a ManyToMany with no + # key column to build it on. + def define_relations + add_field('assigned_issues', OneToManySchema.new(foreign_collection: 'PylonIssue', + origin_key: 'assignee_id', origin_key_target: 'id')) + end + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('name', 'String') + add_column('email', 'String') + # The other addresses of the same agent: a list, so it is neither + # filterable nor sortable. `email` carries the primary one. + add_column('emails', 'Json') + add_column('avatar_url', 'String') + # Left as String rather than Enum: Pylon documents active / away / + # out_of_office on the update endpoint, but does not promise the read + # side is limited to them. + add_column('status', 'String') + add_column('role_id', 'String') + add_column('role_name', 'String') + add_column('is_deactivated', 'Boolean') + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb new file mode 100644 index 000000000..95680fc95 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb @@ -0,0 +1,281 @@ +module ForestAdminDatasourcePylon + RSpec.describe Collections::FetchAllCollection do + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def filter(condition_tree: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, page: page, sort: sort + ) + end + + def page(offset, limit) + ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) + end + + def sort(*clauses) + ForestAdminDatasourceToolkit::Components::Query::Sort.new(clauses) + end + + def by(field, ascending: true) + { field: field, ascending: ascending } + end + + def ids(records) + records.map { |record| record['id'] } + end + + # Mirrors `residual_leaf_appliable?`: an operator is evaluable in memory when + # `ConditionTreeLeaf#match` handles it natively or the toolkit can rewrite it + # into operators that it does. + def appliable?(operator, column_type) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent + .equivalent_tree?(operator, Collections::BaseCollection::IN_MEMORY_OPERATORS, column_type) + end + + def advertised(collection) + collection.fields.flat_map do |name, column| + column.filter_operators.map { |operator| [name, operator, column.column_type] } + end + end + + # A filter value of the shape the operator expects, so every advertised + # operator can be run for real over the dataset. + def value_for(operator, column_type) + return nil if [operators::PRESENT, operators::BLANK].include?(operator) + + sample = column_type == 'Boolean' ? true : 'Bob' + [operators::IN, operators::NOT_IN].include?(operator) ? [sample] : sample + end + + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + let(:datasource) { instance_double(ForestAdminDatasourcePylon::Datasource) } + + # A collection over an in-memory dataset: the endpoint hook hands back what + # the example set, so the shared flow can be observed without the HTTP layer. + let(:subclass) do + Class.new(described_class) do + attr_accessor :entities + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('name', 'String') + add_column('flag', 'Boolean') + add_column('resolved_at', 'Date') + add_column('list', 'Json') + end + + def define_relations; end + + protected + + def fetch_all + @entities + end + + def serialize(entity) + entity + end + end + end + + # Deliberately out of order, with a record whose every value is null: the + # API imposes no order, and Pylon spells an unset value as null. + let(:entities) do + [{ 'id' => 'u2', 'name' => 'Bob', 'flag' => false, 'resolved_at' => nil, 'list' => %w[a] }, + { 'id' => 'u1', 'name' => 'alice', 'flag' => true, 'resolved_at' => '2026-01-02T00:00:00Z', 'list' => [] }, + { 'id' => 'u3', 'name' => nil, 'flag' => nil, 'resolved_at' => nil, 'list' => nil }] + end + + let(:collection) { subclass.new(datasource, 'X').tap { |instance| instance.entities = entities } } + + describe 'subclass contract' do + let(:schema_only) do + Class.new(described_class) do + def define_schema; end + def define_relations; end + end + end + + it 'names fetch_all when the endpoint hook is missing' do + expect { schema_only.new(datasource, 'X').list(nil, nil, nil) } + .to raise_error(NotImplementedError, /did not implement fetch_all/) + end + + it 'names serialize when the serialization hook is missing' do + fetching = Class.new(schema_only) do + protected + + def fetch_all + [{ 'id' => 'u1' }] + end + end + + expect { fetching.new(datasource, 'X').list(nil, nil, nil) } + .to raise_error(NotImplementedError, /did not implement serialize/) + end + + # Neither endpoint carries a search parameter, and Pylon exposes no count. + it 'leaves search and count disabled' do + expect(collection.is_searchable?).to be(false) + expect(collection.is_countable?).to be(false) + end + end + + # How a collection pointing here with a ManyToOne resolves its foreign keys. + # The endpoint hands back the complete dataset, so the ids only pick rows out + # of it and any number of them costs the same single read. + describe '#records_indexed_by_id' do + it 'indexes the wanted records by id' do + expect(collection.records_indexed_by_id(%w[u3 u1])) + .to eq('u1' => entities[1], 'u3' => entities[2]) + end + + it 'leaves out an id the endpoint no longer returns rather than indexing a blank record' do + expect(collection.records_indexed_by_id(%w[u1 gone]).keys).to eq(%w[u1]) + end + end + + describe '.operators_for' do + it 'advertises the string filters the in-memory pass can evaluate' do + expect(described_class.operators_for('String')) + .to eq([operators::EQUAL, operators::NOT_EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK, operators::CONTAINS, operators::I_CONTAINS, + operators::NOT_CONTAINS, operators::STARTS_WITH, operators::ENDS_WITH]) + end + + it 'advertises the boolean filters the in-memory pass can evaluate' do + expect(described_class.operators_for('Boolean')) + .to eq([operators::EQUAL, operators::NOT_EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK]) + end + + # A Json column holds a list whose Pylon semantics have no in-memory + # counterpart, and a type with no candidate is not guessed at: both + # advertise nothing rather than a filter that could answer wrongly. + it 'advertises no filter on a json column nor on a type it has no candidate for' do + expect(described_class.operators_for('Json')).to eq([]) + expect(described_class.operators_for('Date')).to eq([]) + end + + # The schema is the contract the UI builds its filters from: an operator + # in it that memory cannot evaluate would silently empty the page. + it 'advertises only operators the in-memory pass can evaluate' do + unappliable = advertised(collection).reject { |_field, operator, type| appliable?(operator, type) } + + expect(unappliable).to be_empty + end + + it 'evaluates every advertised operator over the dataset, nulls included' do + failures = advertised(collection).filter_map do |field, operator, type| + collection.list(nil, filter(condition_tree: leaf(field, operator, value_for(operator, type))), nil) + nil + rescue StandardError => e + "#{field} #{operator}: #{e.class}: #{e.message}" + end + + expect(failures).to be_empty + end + end + + describe '#list' do + it 'serializes every record the endpoint returned when nothing is filtered' do + expect(ids(collection.list(nil, filter, nil))).to eq(%w[u2 u1 u3]) + expect(collection.list(nil, nil, nil).size).to eq(3) + end + + it 'restricts the records to the projection' do + expect(collection.list(nil, filter(sort: sort(by('id'))), %w[id name])) + .to eq([{ 'id' => 'u1', 'name' => 'alice' }, { 'id' => 'u2', 'name' => 'Bob' }, + { 'id' => 'u3', 'name' => nil }]) + end + + # Cut out of the complete, ordered dataset, so the window holds the rows a + # server-side query would have returned for it. + it 'slices the requested page out of the ordered records' do + expect(ids(collection.list(nil, filter(sort: sort(by('id')), page: page(1, 1)), nil))).to eq(%w[u2]) + end + end + + describe '#list with a filter' do + def filtered(field, operator, value = nil) + ids(collection.list(nil, filter(condition_tree: leaf(field, operator, value)), nil)) + end + + it 'keeps the records the condition tree matches, in the order the API returned them' do + expect(filtered('id', operators::IN, %w[u1 u2])).to eq(%w[u2 u1]) + expect(filtered('name', operators::CONTAINS, 'li')).to eq(%w[u1]) + expect(filtered('flag', operators::EQUAL, true)).to eq(%w[u1]) + end + + # The default search the agent builds for a non-searchable collection is a + # case-insensitive contains, which is why the operator is advertised. + it 'matches a contains regardless of case through i_contains' do + expect(filtered('name', operators::I_CONTAINS, 'ALI')).to eq(%w[u1]) + expect(filtered('name', operators::CONTAINS, 'ALI')).to eq([]) + end + + it 'reads a null column as blank rather than as a value' do + expect(filtered('name', operators::BLANK)).to eq(%w[u3]) + expect(filtered('name', operators::PRESENT)).to eq(%w[u2 u1]) + expect(filtered('flag', operators::BLANK)).to eq(%w[u3]) + end + + # `ConditionTreeLeaf#match` compares with a bare `>`, which raises on the + # null Pylon returns for an unset column. Nothing in the schema advertises + # the comparison, but a scope or a customizer can still send one. + it 'excludes a null column from a comparison instead of raising' do + expect(filtered('resolved_at', operators::GREATER_THAN, '2026-01-01T00:00:00Z')).to eq(%w[u1]) + end + end + + describe '#list with a sort' do + def sorted(*clauses) + ids(collection.list(nil, filter(sort: sort(*clauses)), nil)) + end + + it 'honours an ascending and a descending order' do + expect(sorted(by('name'))).to eq(%w[u2 u1 u3]) + expect(sorted(by('name', ascending: false))).to eq(%w[u3 u1 u2]) + end + + # The agent injects this exact order whenever the request asks for none, + # and unlike the endpoints Pylon orders itself, it is honoured here. + it 'honours the ascending primary-key sort the agent injects' do + expect(sorted(by('id'))).to eq(%w[u1 u2 u3]) + end + + # `false <=> true` is nil in Ruby: the order would be arbitrary without a + # comparator of its own. + it 'orders a boolean column, false first, the way a database does' do + expect(sorted(by('flag'))).to eq(%w[u2 u1 u3]) + expect(sorted(by('flag', ascending: false))).to eq(%w[u3 u1 u2]) + end + + it 'moves on to the next clause for the records the first cannot tell apart' do + collection.entities = [{ 'id' => 'u2', 'flag' => true, 'name' => 'Bob' }, + { 'id' => 'u1', 'flag' => true, 'name' => 'alice' }, + { 'id' => 'u3', 'flag' => false, 'name' => 'Zoe' }] + + expect(sorted(by('flag'), by('name'))).to eq(%w[u3 u2 u1]) + end + + # Ruby's `sort` is not stable, so the position the API returned the record + # in breaks the ties: the same request cannot answer in a different order. + it 'keeps the order the API returned for records the sort cannot tell apart' do + collection.entities = [{ 'id' => 'u3', 'name' => 'same' }, { 'id' => 'u1', 'name' => 'same' }, + { 'id' => 'u2', 'name' => 'same' }] + + expect(sorted(by('name'))).to eq(%w[u3 u1 u2]) + end + + it 'keeps two null columns in place rather than comparing them' do + collection.entities = [{ 'id' => 'u2', 'name' => nil }, { 'id' => 'u1', 'name' => nil }] + + expect(sorted(by('name'))).to eq(%w[u2 u1]) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/team_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/team_spec.rb new file mode 100644 index 000000000..02817a8ca --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/team_spec.rb @@ -0,0 +1,216 @@ +module ForestAdminDatasourcePylon + RSpec.describe Collections::Team do + def filter(condition_tree: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, page: page, sort: sort + ) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def page(offset, limit) + ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) + end + + def sort(field, ascending: true) + ForestAdminDatasourceToolkit::Components::Query::Sort.new([{ field: field, ascending: ascending }]) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + # Pylon nests the members as `{id, email}` objects, and a team with none + # comes back with a null rather than an empty list. + def team_payload(id, overrides = {}) + { 'id' => id, 'name' => 'Support', + 'users' => [{ 'id' => 'u1', 'email' => 'alice@acme.io' }, + { 'id' => 'u2', 'email' => 'bob@acme.io' }] }.merge(overrides) + end + + def stub_teams(*payloads) + stub_request(:get, "#{base}/teams").to_return(json('data' => payloads)) + end + + def ids(records) + records.map { |record| record['id'] } + end + + # Mirrors `residual_leaf_appliable?`: an operator is evaluable in memory when + # `ConditionTreeLeaf#match` handles it natively or the toolkit can rewrite it + # into operators that it does. + def appliable?(operator, column_type) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent + .equivalent_tree?(operator, Collections::BaseCollection::IN_MEMORY_OPERATORS, column_type) + end + + # Relations are fields too; the assertions on the columns select them out. + def columns + collection.fields.select { |_name, field| field.type == 'Column' } + end + + let(:datasource) { ForestAdminDatasourcePylon::Datasource.new(api_key: 'k') } + let(:collection) { described_class.new(datasource) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + describe 'schema' do + it 'is named PylonTeam' do + expect(collection.name).to eq('PylonTeam') + end + + it 'exposes the columns observed on the API, with the members flattened to their ids' do + expect(columns.keys).to eq(%w[id name user_ids]) + expect(collection.fields['user_ids'].column_type).to eq('Json') + expect(collection.fields['id'].column_type).to eq('String') + end + + it 'declares id as the primary key' do + expect(collection.fields['id'].is_primary_key).to be(true) + end + + # Writes land in a later story; the order is honoured in memory over the + # complete dataset, so both scalar columns can be sorted on. + it 'declares every column read-only and both scalar columns sortable' do + expect(columns.values.map(&:is_read_only).uniq).to eq([true]) + expect(columns.except('user_ids').values.map(&:is_sortable).uniq).to eq([true]) + expect(collection.fields['user_ids'].is_sortable).to be(false) + end + + # GET /teams carries neither a search nor a filter parameter, and Pylon + # exposes no count. + it 'leaves search and count disabled' do + expect(collection.is_searchable?).to be(false) + expect(collection.is_countable?).to be(false) + end + + it 'advertises the string filters on the string columns' do + expect(collection.fields['name'].filter_operators) + .to eq([operators::EQUAL, operators::NOT_EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK, operators::CONTAINS, operators::I_CONTAINS, + operators::NOT_CONTAINS, operators::STARTS_WITH, operators::ENDS_WITH]) + expect(collection.fields['id'].filter_operators).to eq(collection.fields['name'].filter_operators) + end + + # The membership holds a list, whose Pylon semantics have no in-memory + # counterpart -- and no relation either, see the relations below. + it 'advertises no filter on the membership' do + expect(collection.fields['user_ids'].filter_operators).to eq([]) + end + + # Every filter of the schema is answered in memory: one the in-memory pass + # cannot evaluate would silently empty the page instead of filtering it. + it 'advertises only operators the in-memory pass can evaluate' do + unappliable = columns.flat_map do |name, column| + column.filter_operators.reject { |operator| appliable?(operator, column.column_type) } + .map { |operator| "#{name}: #{operator}" } + end + + expect(unappliable).to be_empty + end + end + + describe 'relations' do + # `/issues/search` filters `team_id` server-side, so the issues assigned to + # a team are listed by one request. + it 'declares the issues assigned to a team, read through team_id' do + expect(collection.fields['issues']) + .to be_a(ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema) + .and have_attributes(foreign_collection: 'PylonIssue', origin_key: 'team_id', + origin_key_target: 'id') + end + + # Pylon nests the members here rather than pointing at the team from a + # user, so the membership is a ManyToMany with no join collection to + # declare it on: `user_ids` stays a plain column. + it 'declares no relation for the membership' do + expect(collection.fields.keys).to eq(%w[id name user_ids issues]) + end + end + + describe '#list' do + it 'reads every team of the organization and serializes them' do + stub_teams(team_payload('t1'), team_payload('t2', 'name' => 'Billing')) + + rows = collection.list(nil, filter, nil) + + expect(ids(rows)).to eq(%w[t1 t2]) + expect(rows.first).to eq('id' => 't1', 'name' => 'Support', 'user_ids' => %w[u1 u2]) + end + + it 'flattens the members to their ids and keeps the nested objects out' do + stub_teams(team_payload('t1'), team_payload('t2', 'users' => nil)) + + rows = collection.list(nil, filter(sort: sort('id')), nil) + + expect(rows.map { |row| row['user_ids'] }).to eq([%w[u1 u2], []]) + expect(rows.first.keys).not_to include('users') + end + + it 'restricts the record to the projection' do + stub_teams(team_payload('t1')) + + expect(collection.list(nil, filter, %w[id name])).to eq([{ 'id' => 't1', 'name' => 'Support' }]) + end + + it 'returns an empty list when the organization has no team' do + stub_teams + + expect(collection.list(nil, filter, nil)).to eq([]) + end + + # Freshness over rate-limit thrift: one call per list, and no record kept + # from the previous one. + it 'reads the endpoint once per list, and again on the next one' do + stub_teams(team_payload('t1')) + + 2.times { collection.list(nil, filter, nil) } + + expect(WebMock).to have_requested(:get, "#{base}/teams").twice + end + + it 'propagates the API error rather than answering with no team' do + stub_request(:get, "#{base}/teams").to_return(json({ 'message' => 'boom' }, 500)) + + expect { collection.list(nil, filter, %w[id]) }.to raise_error(APIError) + end + end + + describe '#list with a filter, a sort and a page' do + before do + stub_teams(team_payload('t2', 'name' => 'Billing'), team_payload('t1'), + team_payload('t3', 'name' => nil, 'users' => [])) + end + + def filtered(field, operator, value = nil) + ids(collection.list(nil, filter(condition_tree: leaf(field, operator, value)), nil)) + end + + # The single response holds every team, so the filter running in memory + # answers exactly what a server-side filter would have. + it 'filters the complete dataset in memory' do + expect(filtered('name', operators::CONTAINS, 'ill')).to eq(%w[t2]) + expect(filtered('id', operators::IN, %w[t1 t3])).to eq(%w[t1 t3]) + expect(filtered('name', operators::BLANK)).to eq(%w[t3]) + end + + it 'honours the requested order in memory, ascending and descending' do + expect(ids(collection.list(nil, filter(sort: sort('name')), nil))).to eq(%w[t2 t1 t3]) + expect(ids(collection.list(nil, filter(sort: sort('name', ascending: false)), nil))).to eq(%w[t3 t1 t2]) + end + + it 'honours the ascending primary-key sort the agent injects when nothing is asked for' do + expect(ids(collection.list(nil, filter(sort: sort('id')), nil))).to eq(%w[t1 t2 t3]) + end + + it 'slices the requested window out of the ordered records' do + query = filter(sort: sort('id'), page: page(2, 5)) + + expect(ids(collection.list(nil, query, %w[id]))).to eq(%w[t3]) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/user_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/user_spec.rb new file mode 100644 index 000000000..2125ac7ad --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/user_spec.rb @@ -0,0 +1,265 @@ +module ForestAdminDatasourcePylon + RSpec.describe Collections::User do + def filter(condition_tree: nil, page: nil, sort: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, page: page, sort: sort + ) + end + + def leaf(field, operator, value = nil) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new(field, operator, value) + end + + def page(offset, limit) + ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) + end + + def sort(field, ascending: true) + ForestAdminDatasourceToolkit::Components::Query::Sort.new([{ field: field, ascending: ascending }]) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + # Trimmed to the shape observed on the API: `role` is a nested object and an + # unset value comes back as null rather than absent. + def user_payload(id, overrides = {}) + { + 'id' => id, 'name' => 'Alice', 'email' => 'alice@acme.io', 'emails' => %w[alice@acme.io a@acme.io], + 'avatar_url' => 'https://cdn.usepylon.com/alice.png', 'status' => 'active', 'role_id' => 'role-1', + 'role' => { 'id' => 'role-1', 'name' => 'Admin', 'slug' => 'admin' }, 'is_deactivated' => false + }.merge(overrides) + end + + def stub_users(*payloads) + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'true' }) + .to_return(json('data' => payloads)) + end + + def ids(records) + records.map { |record| record['id'] } + end + + # Mirrors `residual_leaf_appliable?`: an operator is evaluable in memory when + # `ConditionTreeLeaf#match` handles it natively or the toolkit can rewrite it + # into operators that it does. + def appliable?(operator, column_type) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent + .equivalent_tree?(operator, Collections::BaseCollection::IN_MEMORY_OPERATORS, column_type) + end + + # Relations are fields too; the assertions on the columns select them out. + def columns + collection.fields.select { |_name, field| field.type == 'Column' } + end + + let(:datasource) { ForestAdminDatasourcePylon::Datasource.new(api_key: 'k') } + let(:collection) { described_class.new(datasource) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + describe 'schema' do + it 'is named PylonUser' do + expect(collection.name).to eq('PylonUser') + end + + it 'exposes the columns observed on the API, with the role flattened to its name' do + expect(columns.keys) + .to eq(%w[id name email emails avatar_url status role_id role_name is_deactivated]) + end + + it 'declares id as the primary key' do + expect(collection.fields['id'].is_primary_key).to be(true) + end + + it 'types the list of addresses as json and the deactivation flag as a boolean' do + expect(collection.fields['emails'].column_type).to eq('Json') + expect(collection.fields['is_deactivated'].column_type).to eq('Boolean') + expect(columns.except('emails', 'is_deactivated').values.map(&:column_type).uniq) + .to eq(['String']) + end + + # Writes land in a later story; the order is honoured in memory over the + # complete dataset, so every scalar column can be sorted on. + it 'declares every column read-only and every scalar column sortable' do + expect(columns.values.map(&:is_read_only).uniq).to eq([true]) + expect(columns.except('emails').values.map(&:is_sortable).uniq).to eq([true]) + expect(collection.fields['emails'].is_sortable).to be(false) + end + + # GET /users carries no search parameter, and Pylon exposes no count. + it 'leaves search and count disabled' do + expect(collection.is_searchable?).to be(false) + expect(collection.is_countable?).to be(false) + end + + it 'advertises the string filters on the string columns' do + expect(collection.fields['name'].filter_operators) + .to eq([operators::EQUAL, operators::NOT_EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK, operators::CONTAINS, operators::I_CONTAINS, + operators::NOT_CONTAINS, operators::STARTS_WITH, operators::ENDS_WITH]) + expect(collection.fields['id'].filter_operators).to eq(collection.fields['name'].filter_operators) + end + + it 'advertises the boolean filters on the deactivation flag' do + expect(collection.fields['is_deactivated'].filter_operators) + .to eq([operators::EQUAL, operators::NOT_EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK]) + end + + # The list of addresses has no in-memory filter that would mean what an + # operator expects, so it advertises none. + it 'advertises no filter on the list of addresses' do + expect(collection.fields['emails'].filter_operators).to eq([]) + end + + # Every filter of the schema is answered in memory: one the in-memory pass + # cannot evaluate would silently empty the page instead of filtering it. + it 'advertises only operators the in-memory pass can evaluate' do + unappliable = columns.flat_map do |name, column| + column.filter_operators.reject { |operator| appliable?(operator, column.column_type) } + .map { |operator| "#{name}: #{operator}" } + end + + expect(unappliable).to be_empty + end + end + + describe 'relations' do + # `/issues/search` filters `assignee_id` server-side, so the issues of an + # agent are listed by one request. + it 'declares the issues assigned to a user, read through assignee_id' do + expect(collection.fields['assigned_issues']) + .to be_a(ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema) + .and have_attributes(foreign_collection: 'PylonIssue', origin_key: 'assignee_id', + origin_key_target: 'id') + end + + # Pylon nests the members inside a team and exposes no team id on a user, + # so that side is a ManyToMany with no key column to build it on. + it 'declares no relation to the teams a user belongs to' do + expect(collection.fields.keys - columns.keys).to eq(%w[assigned_issues]) + end + end + + describe '#list' do + it 'reads every user of the organization and serializes them' do + stub_users(user_payload('u1'), user_payload('u2', 'name' => 'Bob')) + + rows = collection.list(nil, filter, nil) + + expect(ids(rows)).to eq(%w[u1 u2]) + expect(rows.first).to eq('id' => 'u1', 'name' => 'Alice', 'email' => 'alice@acme.io', + 'emails' => %w[alice@acme.io a@acme.io], 'status' => 'active', + 'avatar_url' => 'https://cdn.usepylon.com/alice.png', + 'role_id' => 'role-1', 'role_name' => 'Admin', 'is_deactivated' => false) + end + + # A deactivated agent stays the assignee of the issues they handled, so the + # record the rest of the panel points at has to stay readable. + it 'asks for the deactivated users too' do + stub_users(user_payload('u1')) + + collection.list(nil, filter, nil) + + expect(WebMock).to have_requested(:get, "#{base}/users") + .with(query: { 'include_deactivated' => 'true' }) + end + + it 'flattens the nested role and keeps the object out of the record' do + stub_users(user_payload('u1'), user_payload('u2', 'role' => nil)) + + rows = collection.list(nil, filter(sort: sort('id')), nil) + + expect(rows.map { |row| row['role_name'] }).to eq(['Admin', nil]) + expect(rows.first.keys).not_to include('role') + end + + it 'restricts the record to the projection' do + stub_users(user_payload('u1')) + + expect(collection.list(nil, filter, %w[id name])).to eq([{ 'id' => 'u1', 'name' => 'Alice' }]) + end + + it 'returns an empty list when the organization has no user' do + stub_users + + expect(collection.list(nil, filter, nil)).to eq([]) + end + + # Freshness over rate-limit thrift: one call per list, and no record kept + # from the previous one. + it 'reads the endpoint once per list, and again on the next one' do + stub_users(user_payload('u1')) + + 2.times { collection.list(nil, filter, nil) } + + expect(WebMock).to have_requested(:get, "#{base}/users") + .with(query: { 'include_deactivated' => 'true' }).twice + end + end + + describe '#list with a filter' do + before do + stub_users(user_payload('u1'), user_payload('u2', 'name' => 'Bob', 'is_deactivated' => true), + user_payload('u3', 'name' => nil, 'role' => nil, 'avatar_url' => nil)) + end + + def filtered(field, operator, value = nil) + ids(collection.list(nil, filter(condition_tree: leaf(field, operator, value)), nil)) + end + + # The single response holds every user, so the filter running in memory + # answers exactly what a server-side filter would have. + it 'filters the complete dataset in memory' do + expect(filtered('name', operators::CONTAINS, 'li')).to eq(%w[u1]) + expect(filtered('is_deactivated', operators::EQUAL, true)).to eq(%w[u2]) + expect(filtered('id', operators::IN, %w[u1 u3])).to eq(%w[u1 u3]) + end + + it 'reads the columns Pylon leaves null as blank instead of crashing' do + expect(filtered('name', operators::BLANK)).to eq(%w[u3]) + expect(filtered('name', operators::CONTAINS, 'li')).to eq(%w[u1]) + expect(filtered('role_name', operators::PRESENT)).to eq(%w[u1 u2]) + expect(filtered('avatar_url', operators::NOT_CONTAINS, 'alice')).to eq(%w[u3]) + end + end + + describe '#list with a sort and a page' do + before do + stub_users(user_payload('u2', 'name' => 'Zoe'), user_payload('u1'), + user_payload('u3', 'name' => 'Carol', 'is_deactivated' => true)) + end + + it 'honours the requested order in memory, ascending and descending' do + expect(ids(collection.list(nil, filter(sort: sort('name')), nil))).to eq(%w[u1 u3 u2]) + expect(ids(collection.list(nil, filter(sort: sort('name', ascending: false)), nil))).to eq(%w[u2 u3 u1]) + end + + it 'honours the ascending primary-key sort the agent injects when nothing is asked for' do + expect(ids(collection.list(nil, filter(sort: sort('id')), nil))).to eq(%w[u1 u2 u3]) + end + + it 'orders the deactivation flag, false first, the way a database does' do + expect(ids(collection.list(nil, filter(sort: sort('is_deactivated')), nil))).to eq(%w[u2 u1 u3]) + end + + it 'slices the requested window out of the ordered records' do + query = filter(sort: sort('id'), page: page(1, 2)) + + expect(ids(collection.list(nil, query, %w[id]))).to eq(%w[u2 u3]) + end + end + + describe '#list when the endpoint fails' do + it 'propagates the API error rather than answering with no user' do + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'true' }) + .to_return(json({ 'message' => 'boom' }, 500)) + + expect { collection.list(nil, filter, %w[id]) }.to raise_error(APIError) + end + end + end +end From e3a03669d5b196ecac76da8070a90ada4a41696e Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 12 Aug 2026 18:29:26 +0200 Subject: [PATCH 5/8] feat(pylon): register collections and embed relations Registers the five collections on the datasource and covers the relation embedding end to end: bulk id search per foreign collection, chunking, dedup and missing-record handling. Co-Authored-By: Claude Fable 5 --- .../datasource.rb | 7 + .../collections/relation_embedder_spec.rb | 336 ++++++++++++++++++ .../datasource_spec.rb | 20 +- 3 files changed, 361 insertions(+), 2 deletions(-) create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/datasource.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/datasource.rb index c7574c05b..446eb45a5 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/datasource.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/datasource.rb @@ -12,8 +12,15 @@ def initialize(api_key:, **options) private + # The five collections are registered together: each one declares relations + # pointing at the others, and a relation whose foreign collection is missing + # is a schema the agent refuses to boot on. def register_collections add_collection(Collections::Issue.new(self)) + add_collection(Collections::Account.new(self)) + add_collection(Collections::Contact.new(self)) + add_collection(Collections::User.new(self)) + add_collection(Collections::Team.new(self)) end end end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb new file mode 100644 index 000000000..cd473acbb --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb @@ -0,0 +1,336 @@ +module ForestAdminDatasourcePylon + # Observed through the two list paths that embed: PylonIssue, whose four + # ManyToOne relations reach both kinds of foreign collection, and PylonContact, + # which embeds through the cursor-paginated pipeline. + RSpec.describe Collections::RelationEmbedder do + def filter(condition_tree: nil, search: nil, page: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new( + condition_tree: condition_tree, search: search, page: page + ) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + # Trimmed to what the embedder reads: the nested parties. The columns left + # out serialize to nil, which is what Pylon returns for them anyway. + def issue_payload(id, overrides = {}) + { 'id' => id, 'title' => 'Boom', 'account' => { 'id' => 'acc-1' }, + 'requester' => { 'id' => 'con-1' }, 'assignee' => { 'id' => 'usr-1' }, + 'team' => { 'id' => 'team-1' } }.merge(overrides) + end + + def account_payload(id, overrides = {}) + { 'id' => id, 'name' => 'Acme', 'domains' => %w[acme.com], + 'owner' => { 'id' => 'usr-9', 'email' => 'ada@acme.com' } }.merge(overrides) + end + + def contact_payload(id, overrides = {}) + { 'id' => id, 'name' => 'Ada', 'email' => 'ada@acme.com', + 'account' => { 'id' => 'acc-1' } }.merge(overrides) + end + + def user_payload(id, overrides = {}) + { 'id' => id, 'name' => 'Alice', 'role' => { 'id' => 'role-1', 'name' => 'Admin' } }.merge(overrides) + end + + def team_payload(id, overrides = {}) + { 'id' => id, 'name' => 'Support', 'users' => [{ 'id' => 'usr-1' }] }.merge(overrides) + end + + def id_filter(values) + { 'field' => 'id', 'operator' => 'in', 'values' => values } + end + + def stub_issues(*payloads) + stub_request(:post, "#{base}/issues/search").to_return(json('data' => payloads)) + end + + def stub_accounts(*payloads) + stub_request(:post, "#{base}/accounts/search").to_return(json('data' => payloads)) + end + + def stub_contact_search(*payloads) + stub_request(:post, "#{base}/contacts/search").to_return(json('data' => payloads)) + end + + def stub_contact_list(*payloads) + stub_request(:get, "#{base}/contacts").with(query: { 'limit' => '1000' }) + .to_return(json('data' => payloads)) + end + + def stub_users(*payloads) + stub_request(:get, "#{base}/users").with(query: { 'include_deactivated' => 'true' }) + .to_return(json('data' => payloads)) + end + + def stub_teams(*payloads) + stub_request(:get, "#{base}/teams").to_return(json('data' => payloads)) + end + + # The columns the foreign collection declares, which is what its serializer + # is expected to fill in on the embedded record. + def columns_of(name) + datasource.get_collection(name).fields.select { |_field, schema| schema.type == 'Column' }.keys + end + + let(:datasource) { ForestAdminDatasourcePylon::Datasource.new(api_key: 'k') } + let(:issues) { datasource.get_collection('PylonIssue') } + let(:contacts) { datasource.get_collection('PylonContact') } + let(:base) { datasource.configuration.url } + + describe 'what the projection asks for' do + before { stub_issues(issue_payload('i1')) } + + it 'resolves only the relations the projection names' do + stub_accounts(account_payload('acc-1')) + + row = issues.list(nil, filter, %w[id account:name]).first + + expect(row['account']).to include('id' => 'acc-1', 'name' => 'Acme') + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + expect(WebMock).not_to have_requested(:post, "#{base}/contacts/search") + expect(WebMock).not_to have_requested(:get, "#{base}/users") + expect(WebMock).not_to have_requested(:get, "#{base}/teams") + end + + it 'reads no foreign collection when the projection holds columns only' do + rows = issues.list(nil, filter, %w[id title account_id]) + + expect(rows).to eq([{ 'id' => 'i1', 'title' => 'Boom', 'account_id' => 'acc-1' }]) + expect(WebMock).not_to have_requested(:post, "#{base}/accounts/search") + end + + # A nil projection returns every column and no relation: `list` is also + # called that way by a count or an export. + it 'reads no foreign collection when the projection is nil' do + expect(issues.list(nil, filter, nil).first).not_to have_key('account') + expect(WebMock).not_to have_requested(:post, "#{base}/accounts/search") + end + + # `account_id` is a column of the projection, not a relation prefix. + it 'embeds nothing for a projected column bearing a relation name' do + expect(issues.list(nil, filter, %w[account_id]).first.keys).to eq(%w[account_id]) + end + end + + describe 'the ids it asks for' do + it 'asks for an id once however many rows point at it' do + stub_issues(issue_payload('i1'), issue_payload('i2'), issue_payload('i3', 'account' => { 'id' => 'acc-2' })) + stub_accounts(account_payload('acc-1'), account_payload('acc-2')) + + rows = issues.list(nil, filter, %w[id account:name]) + + expect(rows.map { |row| row['account']['id'] }).to eq(%w[acc-1 acc-1 acc-2]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: hash_including('filter' => id_filter(%w[acc-1 acc-2]))).once + end + + # Pylon documents no maximum on an `in` filter; the chunk keeps the request + # body and the page answering it bounded. + it 'chunks the ids, and asks for no more records than the chunk holds' do + chunk = Collections::CursorCollection::ID_CHUNK_SIZE + ids = (1..(chunk + 50)).map { |index| "acc-#{index}" } + stub_issues(*ids.map { |id| issue_payload("i-#{id}", 'account' => { 'id' => id }) }) + stub_accounts + + issues.list(nil, filter, %w[id account:name]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: { 'limit' => chunk, 'filter' => id_filter(ids.first(chunk)) }) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: { 'limit' => 50, 'filter' => id_filter(ids.last(50)) }) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").twice + end + + it 'sends no request at all when every foreign key of the page is null' do + stub_issues(issue_payload('i1', 'team' => nil)) + + expect(issues.list(nil, filter, %w[id team:name]).first).to eq('id' => 'i1', 'team' => nil) + expect(WebMock).not_to have_requested(:get, "#{base}/teams") + end + + it 'leaves the null key out of the ids it asks for' do + stub_issues(issue_payload('i1'), issue_payload('i2', 'account' => nil)) + stub_accounts(account_payload('acc-1')) + + rows = issues.list(nil, filter, %w[id account:name]) + + expect(rows.map { |row| row['account'] }).to eq([rows.first['account'], nil]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: hash_including('filter' => id_filter(%w[acc-1]))) + end + + # Deleted, merged, or outside the scope of the token: the row says so + # rather than carrying a blank record the panel would offer to open. + it 'embeds no record for a foreign key the endpoint no longer answers' do + stub_issues(issue_payload('i1')) + stub_accounts + + expect(issues.list(nil, filter, %w[id account:name]).first).to eq('id' => 'i1', 'account' => nil) + end + end + + describe 'how it reads the foreign collections' do + before { stub_issues(issue_payload('i1'), issue_payload('i2', 'assignee' => { 'id' => 'usr-2' })) } + + # `GET /users` and `GET /teams` hand back the complete dataset, so the ids + # only pick rows out of one response. + it 'reads an unpaginated collection once and indexes it by id' do + stub_users(user_payload('usr-1'), user_payload('usr-2', 'name' => 'Bob')) + + rows = issues.list(nil, filter, %w[id assignee:name]) + + expect(rows.map { |row| row['assignee']['name'] }).to eq(%w[Alice Bob]) + expect(WebMock).to have_requested(:get, "#{base}/users") + .with(query: { 'include_deactivated' => 'true' }).once + end + + # A chunk is asked for as a single page, and Pylon is free to answer it over + # several: the ids left out of the first page are read from the next one + # rather than reported as records that no longer exist. + it 'follows the cursor when Pylon answers a chunk over several pages' do + stub_issues(issue_payload('i1'), issue_payload('i2', 'account' => { 'id' => 'acc-2' })) + stub_request(:post, "#{base}/accounts/search").with(body: hash_including('limit' => 2)) + .to_return(json('data' => [account_payload('acc-1')], + 'pagination' => { 'cursor' => 'c1', + 'has_next_page' => true })) + stub_request(:post, "#{base}/accounts/search").with(body: hash_including('cursor' => 'c1')) + .to_return(json('data' => [account_payload('acc-2')])) + + rows = issues.list(nil, filter, %w[id account:name]) + + expect(rows.map { |row| row['account']['id'] }).to eq(%w[acc-1 acc-2]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").twice + end + + # One read per foreign collection, and one collection per relation here: + # nothing is shared, so the four are read side by side. + it 'reads each foreign collection the projection reaches' do + stub_accounts(account_payload('acc-1')) + stub_contact_search(contact_payload('con-1')) + stub_users(user_payload('usr-1'), user_payload('usr-2')) + stub_teams(team_payload('team-1')) + + row = issues.list(nil, filter, %w[id account:name requester:name assignee:name team:name]).first + + expect(row.keys).to eq(%w[id account requester assignee team]) + expect(row['requester']).to include('id' => 'con-1', 'name' => 'Ada') + expect(row['team']).to include('id' => 'team-1', 'name' => 'Support') + end + end + + describe 'the shape of the embedded record' do + before { stub_issues(issue_payload('i1')) } + + # Serialized by the foreign collection itself, not by a second field list + # kept here: an account carries every column PylonAccount declares. + it 'carries the columns of the foreign collection, and nothing else' do + stub_accounts(account_payload('acc-1')) + + embedded = issues.list(nil, filter, %w[id account:name]).first['account'] + + expect(embedded.keys).to match_array(columns_of('PylonAccount')) + end + + it 'flattens the nested objects of the foreign record the way its own list does' do + stub_accounts(account_payload('acc-1')) + stub_users(user_payload('usr-1')) + + row = issues.list(nil, filter, %w[id account:name assignee:name]).first + + expect(row['account']).to include('owner_id' => 'usr-9') + expect(row['account']).not_to have_key('owner') + expect(row['assignee']).to include('role_name' => 'Admin') + expect(row['assignee']).not_to have_key('role') + end + + # The projection of the relation is not applied here: the agent picks the + # fields it asked for out of the record. + it 'embeds the whole foreign record rather than the projected fields of it' do + stub_teams(team_payload('team-1')) + + expect(issues.list(nil, filter, %w[id team:name]).first['team']) + .to eq('id' => 'team-1', 'name' => 'Support', 'user_ids' => %w[usr-1]) + end + end + + describe 'the order of the rows' do + it 'writes each related record on its own row' do + stub_issues(issue_payload('i1', 'account' => { 'id' => 'acc-2' }), + issue_payload('i2', 'account' => nil), + issue_payload('i3')) + stub_accounts(account_payload('acc-1', 'name' => 'First'), account_payload('acc-2', 'name' => 'Second')) + + rows = issues.list(nil, filter, %w[id account:name]) + + expect(rows.map { |row| row['id'] }).to eq(%w[i1 i2 i3]) + expect(rows.map { |row| row['account']&.fetch('name') }).to eq(['Second', nil, 'First']) + end + + # The window is cut out before the relations are read, so the ids asked for + # are those of the rows the operator sees. + it 'reads the relations of the requested page only' do + stub_issues(issue_payload('i1'), issue_payload('i2', 'account' => { 'id' => 'acc-2' })) + stub_accounts(account_payload('acc-2')) + page = ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: 1, limit: 1) + + rows = issues.list(nil, filter(page: page), %w[id account:name]) + + expect(rows.map { |row| row['id'] }).to eq(%w[i2]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: hash_including('filter' => id_filter(%w[acc-2]))) + end + end + + # The cursor-paginated pipeline embeds the same way, on all three of its + # paths: the listing endpoint, the search endpoint and the record endpoint. + describe 'PylonContact#list' do + it 'embeds the account of a browsed page' do + stub_contact_list(contact_payload('con-1')) + stub_accounts(account_payload('acc-1')) + + row = contacts.list(nil, filter, %w[id account:name]).first + + expect(row).to eq('id' => 'con-1', 'account' => row['account']) + expect(row['account']).to include('id' => 'acc-1', 'name' => 'Acme') + end + + it 'embeds the account of a searched page' do + stub_contact_search(contact_payload('con-1')) + stub_accounts(account_payload('acc-1')) + + row = contacts.list(nil, filter(search: 'ada'), %w[id account:name]).first + + expect(row['account']).to include('id' => 'acc-1') + end + + it 'embeds the account of a record read through its own endpoint' do + stub_request(:get, "#{base}/contacts/con-1").to_return(json('data' => contact_payload('con-1'))) + stub_accounts(account_payload('acc-1')) + tree = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('id', ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators::EQUAL, 'con-1') + + row = contacts.list(nil, filter(condition_tree: tree), %w[id account:name]).first + + expect(row['account']).to include('id' => 'acc-1') + end + end + + # A OneToMany is not embedded: the agent lists the far side with a query of + # its own, filtered on the origin key -- which every reverse side here is + # filterable on server-side. PylonUser and PylonTeam declare nothing else, so + # they embed nothing at all. + describe 'a collection declaring no ManyToOne' do + it 'embeds nothing, and reads nothing besides its own endpoint' do + stub_users(user_payload('usr-1')) + + rows = datasource.get_collection('PylonUser').list(nil, filter, %w[id assigned_issues:id]) + + expect(rows).to eq([{ 'id' => 'usr-1' }]) + expect(WebMock).not_to have_requested(:post, "#{base}/issues/search") + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/datasource_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/datasource_spec.rb index 568251689..be80cd1af 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/datasource_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/datasource_spec.rb @@ -17,9 +17,25 @@ expect(datasource.client).to be_a(ForestAdminDatasourcePylon::Client) end - it 'registers the issue collection' do - expect(datasource.collections.keys).to eq(['PylonIssue']) + it 'registers the five collections Pylon exposes' do + expect(datasource.collections.keys) + .to eq(%w[PylonIssue PylonAccount PylonContact PylonUser PylonTeam]) expect(datasource.get_collection('PylonIssue')).to be_a(ForestAdminDatasourcePylon::Collections::Issue) + expect(datasource.get_collection('PylonAccount')).to be_a(ForestAdminDatasourcePylon::Collections::Account) + expect(datasource.get_collection('PylonContact')).to be_a(ForestAdminDatasourcePylon::Collections::Contact) + expect(datasource.get_collection('PylonUser')).to be_a(ForestAdminDatasourcePylon::Collections::User) + expect(datasource.get_collection('PylonTeam')).to be_a(ForestAdminDatasourcePylon::Collections::Team) + end + + # Every relation declared by one of them points at another: a foreign + # collection left unregistered is a schema the agent refuses to boot on. + it 'registers a collection for every foreign collection its relations point at' do + relations = datasource.collections.values.flat_map do |collection| + collection.fields.values.reject { |field| field.type == 'Column' } + end + + expect(relations).not_to be_empty + expect(relations.map(&:foreign_collection).uniq - datasource.collections.keys).to be_empty end it 'refuses to build without an api key' do From 086179c33c819eff49d59c44711ce1c1b29b114c Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 18 Aug 2026 11:58:11 +0200 Subject: [PATCH 6/8] fix(pylon): address the collections and relations review - refuse an absence filter Pylon cannot answer, with a message naming it, and map missing onto is_unset where the API takes it - report a refused filter as a ValidationError, so the agent answers 400 with the message rather than a 500 "Unexpected error" - refuse a filter on a related field, naming the key to use instead - drop a record answered under an alias of the primary key - declare no column groupable, and answer an aggregation in memory on the two collections holding every record Pylon has - take no custom field on those two, where nothing reads one - leave a blank foreign key out of the relation reads, like a null - serialize only the records a relation asked for - hoist filter_table, api_filters and add_column into BaseCollection Also fixes SortCollectionDecorator#refine_schema, which marked every column of every datasource sortable instead of the ones a sort was registered for, and rewrote the schema of its child while doing so. Co-Authored-By: Claude Opus 5 (1M context) --- .../sort/sort_collection_decorator.rb | 30 ++++-- .../sort/sort_collection_decorator_spec.rb | 26 +++++ .../lib/forest_admin_datasource_pylon.rb | 8 +- .../collections/account/api_filters.rb | 6 +- .../collections/account/schema_definition.rb | 17 +--- .../collections/base_collection.rb | 96 +++++++++++++++++-- .../collections/contact/api_filters.rb | 6 +- .../collections/contact/schema_definition.rb | 17 +--- .../collections/cursor_collection.rb | 24 ++--- .../collections/fetch_all_collection.rb | 54 +++++++++-- .../collections/issue.rb | 28 +++--- .../collections/issue/api_filters.rb | 6 +- .../collections/issue/schema_definition.rb | 14 +-- .../collections/issue/serializer.rb | 2 - .../collections/relation_embedder.rb | 11 ++- .../collections/team.rb | 4 +- .../collections/user.rb | 4 +- .../query/condition_tree_translator.rb | 31 ++++++ .../query/filter_value.rb | 2 +- .../query/operator_maps.rb | 37 ++++++- .../collections/account_spec.rb | 21 +++- .../collections/base_collection_spec.rb | 56 ++++++++++- .../collections/contact_spec.rb | 7 ++ .../collections/fetch_all_collection_spec.rb | 72 ++++++++++++++ .../collections/issue_spec.rb | 41 +++++++- .../collections/relation_embedder_spec.rb | 20 ++++ .../query/condition_tree_translator_spec.rb | 34 +++++-- 27 files changed, 558 insertions(+), 116 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator.rb index 8b955d728..dee0d510d 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator.rb @@ -51,15 +51,33 @@ def list(caller, filter = nil, projection = nil) projection.apply(records) end + # Only a field registered through `emulate_field_sorting` or + # `replace_field_sorting` becomes sortable here: those are the ones + # `list` above knows how to order, either by emulating the sort over the + # whole collection or by rewriting it into an equivalent one. Every other + # field keeps the flag its datasource declared -- a clause on it is + # handed straight to `child_collection.list`, so marking it sortable + # would let the UI ask for an order nothing honours, and the records + # would come back in whatever order the datasource imposes. + # + # `@sorts` holds nil as the value of an emulated field, so membership is + # read with `key?`, the way `emulated?` reads it. + # + # `CollectionDecorator#schema` only shallow-copies the schema it hands + # over, so the fields hash and the ColumnSchema objects in it are the + # ones of the collection below: both are copied before the flag is set, + # or the decorator would rewrite the schema of its own child. def refine_schema(child_schema) - child_schema[:fields].each do |name, schema| - if schema.type == 'Column' - schema.is_sortable = true if @sorts[name].nil? - child_schema[:fields][name] = schema - end + schema = child_schema.dup + schema[:fields] = child_schema[:fields].dup + + schema[:fields].each do |name, field| + next unless field.type == 'Column' && @sorts.key?(name) + + schema[:fields][name] = field.dup.tap { |sortable| sortable.is_sortable = true } end - child_schema + schema end def rewrite_plain_sort_clause(clause) diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator_spec.rb index b7d639a3e..72a33e4ea 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/sort/sort_collection_decorator_spec.rb @@ -105,6 +105,32 @@ module Sort expect { @decorated_book.replace_field_sorting('author_id', nil) }.to raise_error(ForestException, 'A new sorting method should be provided to replace field sorting') end + # A clause on a field nothing was registered for is handed straight to + # `child_collection.list`, so the flag the datasource declared is the + # truth about it: marking it sortable would let the UI ask for an order + # nothing honours, and the records would come back in the order the + # datasource imposes with no signal that the sort was dropped. + it 'leaves the sortability of the fields it cannot order as the datasource declared it' do + expect(@decorated_book.schema[:fields]['title'].is_sortable).to be false + expect(@decorated_book.schema[:fields]['author_id'].is_sortable).to be false + expect(@decorated_book.schema[:fields]['id'].is_sortable).to be false + end + + it 'marks a field sortable once its sorting is replaced' do + @decorated_book.replace_field_sorting('author_id', [{ field: 'id', ascending: true }]) + + expect(@decorated_book.schema[:fields]['author_id'].is_sortable).to be true + end + + # `CollectionDecorator#schema` shallow-copies what it refines, so the + # fields hash and the columns in it belong to the collection below. + it 'leaves the schema of the collection below untouched' do + @decorated_book.emulate_field_sorting('title') + + expect(@decorated_book.schema[:fields]['title'].is_sortable).to be true + expect(@collection_book.schema[:fields]['title'].is_sortable).to be false + end + context 'when emulating sort on book.title (no relations)' do before do @decorated_book.emulate_field_sorting('title') diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon.rb index 16adf3521..24aeadbba 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon.rb @@ -12,7 +12,13 @@ module ForestAdminDatasourcePylon class Error < StandardError; end class ConfigurationError < Error; end - class UnsupportedOperatorError < Error; end + + # A filter Pylon cannot express. It descends from the toolkit's ValidationError + # rather than from the package's own Error so the agent answers 400 carrying + # the message instead of a 500 "Unexpected error": every one of these names a + # condition the operator set and can change, and the message is the only place + # they learn which one. + class UnsupportedOperatorError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end # Raised when a Pylon API call fails. Carries the HTTP status and the # (parsed) response body so callers — smart actions in particular — can diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb index eba88466d..5a866dc08 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/api_filters.rb @@ -6,8 +6,10 @@ class Account < CursorCollection # an operator absent from a field's map is rejected by Pylon. # # It is the single source of truth for filtering — `define_schema` derives - # every column's `filter_operators` from it, so the schema cannot - # advertise a filter the translator would then refuse. + # every column's `filter_operators` from it, so this collection declares no + # filter the translator would then refuse; the absence family the agent + # derives on top of it is the exception `Query::OperatorMaps::Table` + # describes. module ApiFilters Maps = Query::OperatorMaps diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb index 50bc32f89..00bcc089f 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb @@ -8,10 +8,11 @@ class Account < CursorCollection # # Filter operators are not chosen here: they come from # `ApiFilters::API_FILTERS`, which mirrors the allow-list of the API. A - # column missing from that table gets no operator, so the UI never offers - # a filter Pylon would refuse. + # column missing from that table gets no operator, so the UI offers no + # filter of this collection's own that Pylon would refuse — the absence + # family the agent derives above the datasource being the exception + # `Query::OperatorMaps::Table` describes. module SchemaDefinition - ColumnSchema = BaseCollection::ColumnSchema OneToManySchema = BaseCollection::OneToManySchema private @@ -39,9 +40,7 @@ def define_relations end def define_identity_fields - add_field('id', ColumnSchema.new(column_type: 'String', - filter_operators: ApiFilters.forest_operators('id'), - is_primary_key: true, is_read_only: true)) + add_column('id', 'String', is_primary_key: true) add_column('name', 'String') # Left as String rather than Enum: Pylon ships customer / partner / # prospect but lets an organization define its own account types. @@ -73,12 +72,6 @@ def define_integration_fields def define_time_fields %w[created_at updated_at latest_customer_activity_time].each { |field| add_column(field, 'Date') } end - - def add_column(name, type) - add_field(name, ColumnSchema.new(column_type: type, - filter_operators: ApiFilters.forest_operators(name), - is_read_only: true)) - end end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb index 3e7fea06d..6f41a4403 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb @@ -54,6 +54,21 @@ def records_indexed_by_id(_ids) raise NotImplementedError, "#{self.class} did not implement records_indexed_by_id" end + # Pylon exposes no aggregate endpoint, and the pages of a cursor walk are + # not the dataset: a count or a group computed over them would look exact + # while answering a fraction. Every column is registered with + # `is_groupable: false` so the UI never offers one, and a chart built + # through the API anyway is refused here rather than through the + # contract's NotImplementedError, which reads as an oversight. + # + # FetchAllCollection, which does hold every record Pylon has, overrides + # this and answers exactly. + def aggregate(_caller, _filter, _aggregation, _limit = nil) + raise UnsupportedOperatorError, + "#{name} cannot be aggregated: Pylon exposes no aggregate endpoint, and counting or grouping the " \ + 'pages the agent walked would answer a fraction of the collection as if it were the whole of it.' + end + protected # Pylon has no `id` filter operator on /issues/search, so collections @@ -64,6 +79,7 @@ def records_indexed_by_id(_ids) # `AND(id equal X, )` on a record detail as soon as a scope or a # segment is set, and `id` is not a field Pylon can filter on. def extract_id_lookup(node) + ensure_no_relation_leaf!(node) ids = id_values(node) return IdLookup.new(ids: ids, residual: nil) if ids return nil unless and_branch?(node) @@ -121,13 +137,49 @@ def page_window(records, filter) def build_pylon_filter(caller, filter) tree = filter&.condition_tree + ensure_no_relation_leaf!(tree) ensure_no_stray_id!(tree) Query::ConditionTreeTranslator.call(tree, api_filters: api_filters, timezone: timezone_for(caller)) end - # Overridden by collections whose endpoint can filter server-side. + # The `ApiFilters` module of the collection, whose table is the single + # source of truth for what its endpoint filters. The empty table is the + # default: a collection read whole and filtered in memory filters nothing + # server-side. + def filter_table = Query::OperatorMaps::EmptyTable + + # What the endpoint filters server-side: the table of the collection, plus + # one entry per custom field — filtered through the very Pylon slug it is + # read by, with the operators the integrator declared on the column. def api_filters - {} + @api_filters ||= custom_fields.each_with_object(filter_table::API_FILTERS.dup) do |cf, filters| + filters[cf[:column_name]] = filter_table.for_custom_field(cf[:schema]) + end + end + + # A native column: read-only in this story — writes land in a later one — + # and never groupable, as no Pylon endpoint aggregates. It is not sortable + # either, the ColumnSchema default, because no search endpoint takes a sort + # parameter. Filter operators are not chosen here: they come from + # `filter_table`, which mirrors the allow-list of the API, so a column + # missing from it gets none and the UI offers no filter Pylon would refuse. + def add_column(name, type, is_primary_key: false) + add_field(name, ColumnSchema.new(column_type: type, + filter_operators: filter_table.forest_operators(name), + is_primary_key: is_primary_key, + is_groupable: false, + is_read_only: true)) + end + + # A record read through the endpoint of an id that is not the primary key + # it answered with. `GET /accounts/{id}` takes an external id and + # `GET /issues/{id}` an issue number, so the record a lookup hands back + # can carry an `id` other than the one the filter asked for: keeping it + # would answer `id equals ` with a row that does not match, where + # the same filter combined with a scope — which goes through the search + # endpoint instead — answers nothing at all. + def matches_id?(record, id) + record['id'].to_s == id.to_s end # An order no endpoint honours is reported rather than silently swallowed: @@ -215,11 +267,13 @@ def add_custom_fields(custom_fields) end end - # Operators a custom field may advertise. The empty default matches the - # empty `api_filters`: a collection that filters nothing server-side - # must not advertise custom-field filters either. + # Operators a custom field may advertise: the ones the endpoint accepts on + # one, read off the same table the native columns come from. Declarations + # outside this list are dropped at registration, so the schema never + # advertises an operator the translator would refuse — and the empty table + # of a collection filtering nothing server-side advertises none. def allowed_custom_field_operators - [] + filter_table::CUSTOM_FIELD_OPS.keys end private @@ -262,6 +316,36 @@ def ensure_no_stray_id!(node) 'silently widen. Rewrite the filter with `and`, or filter on another field.' end + # Forest offers a filter on a related field as soon as a ManyToOne is + # declared, and sends it as a `relation:field` leaf. Pylon has no join and + # no include parameter, so there is nothing to translate it into: the + # matching records would have to be read from the foreign collection and + # their keys matched here, which is a read of its own, not a filter. + # + # Refused with the foreign key of the relation, which is the filter the + # operator can set instead — and the one the reverse side is listed by. + def ensure_no_relation_leaf!(node) + return if node.nil? + + field = nil + node.some_leaf { |leaf| field = leaf.field if leaf.field.to_s.include?(':') } + raise_unfilterable_relation(field) if field + end + + def raise_unfilterable_relation(field) + relation = schema[:fields][field.to_s.split(':').first] + instead = if relation.respond_to?(:foreign_key) + "Filter on '#{relation.foreign_key}' instead, or set the filter from the " \ + "#{relation.foreign_collection} list." + else + 'Filter on a column of this collection instead.' + end + + raise UnsupportedOperatorError, + "Pylon cannot filter on the related field '#{field}': it has no join, so a condition on a " \ + "relation has no server-side translation. #{instead}" + end + def clamp_custom_field_operators(column_name, schema) declared = Array(schema.filter_operators) dropped = declared - allowed_custom_field_operators diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb index 5207b2751..af2379e8f 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/api_filters.rb @@ -6,8 +6,10 @@ class Contact < CursorCollection # an operator absent from a field's map is rejected by Pylon. # # It is the single source of truth for filtering — `define_schema` derives - # every column's `filter_operators` from it, so the schema cannot - # advertise a filter the translator would then refuse. + # every column's `filter_operators` from it, so this collection declares no + # filter the translator would then refuse; the absence family the agent + # derives on top of it is the exception `Query::OperatorMaps::Table` + # describes. module ApiFilters Maps = Query::OperatorMaps diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb index f84ccfeaf..176a83a08 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb @@ -8,11 +8,12 @@ class Contact < CursorCollection # # Filter operators are not chosen here: they come from # `ApiFilters::API_FILTERS`, which mirrors the allow-list of the API. A - # column missing from that table gets no operator, so the UI never offers - # a filter Pylon would refuse. A contact carries no timestamp at all — + # column missing from that table gets no operator, so the UI offers no + # filter of this collection's own that Pylon would refuse — the absence + # family the agent derives above the datasource being the exception + # `Query::OperatorMaps::Table` describes. A contact carries no timestamp at all — # Pylon returns none. module SchemaDefinition - ColumnSchema = BaseCollection::ColumnSchema ManyToOneSchema = BaseCollection::ManyToOneSchema OneToManySchema = BaseCollection::OneToManySchema @@ -39,9 +40,7 @@ def define_relations end def define_identity_fields - add_field('id', ColumnSchema.new(column_type: 'String', - filter_operators: ApiFilters.forest_operators('id'), - is_primary_key: true, is_read_only: true)) + add_column('id', 'String', is_primary_key: true) add_column('name', 'String') # Flattened from the nested `{ id: ..., external_ids: ... }` object # Pylon returns, and kept as a column next to the `account` relation @@ -72,12 +71,6 @@ def define_portal_fields add_column('portal_role_id', 'String') add_column('integration_user_ids', 'Json') end - - def add_column(name, type) - add_field(name, ColumnSchema.new(column_type: type, - filter_operators: ApiFilters.forest_operators(name), - is_read_only: true)) - end end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb index aa0ff30d4..f476d0e8a 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb @@ -40,8 +40,9 @@ def records_indexed_by_id(ids) protected - # The `ApiFilters` module of the collection, whose table is the single - # source of truth for what its search endpoint filters. + # Every collection read this way has a search endpoint, hence a table of + # its own: the empty default of the base would silently turn each of its + # filters into a refusal. def filter_table = raise(NotImplementedError, "#{self.class} did not implement filter_table") # One page of the listing endpoint, as a Client::SearchPage. @@ -50,20 +51,6 @@ def list_page(limit:, cursor:) = raise(NotImplementedError, "#{self.class} did n # One record straight from its own endpoint. def fetch_one(id) = raise(NotImplementedError, "#{self.class} did not implement fetch_one") - # A custom field is filtered through its Pylon slug, with the operators the - # integrator declared on the column. - def api_filters - @api_filters ||= custom_fields.each_with_object(filter_table::API_FILTERS.dup) do |cf, filters| - filters[cf[:column_name]] = filter_table.for_custom_field(cf[:schema]) - end - end - - # Declarations outside this list are dropped at registration, so the - # schema never advertises an operator the translator would refuse. - def allowed_custom_field_operators - filter_table::CUSTOM_FIELD_OPS.keys - end - private # The `id` filter goes through the translator rather than being written by @@ -126,7 +113,10 @@ def single_id_lookup(filter) # token's scope — reads as "no record" rather than as a failed page. def records_by_id(id) record = fetch_one(id) - record.nil? ? [] : [serialize(record)] + return [] if record.nil? + + serialized = serialize(record) + matches_id?(serialized, id) ? [serialized] : [] rescue APIError => e raise unless e.status == 404 diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb index b2577dd17..a477a161d 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb @@ -40,36 +40,68 @@ def self.operators_for(column_type) end def list(caller, filter, projection) - records = fetch_all.map { |entity| serialize(entity) } - records = filter_in_memory(records, caller, filter) - records = sort_in_memory(records, filter&.sort) + records = sort_in_memory(filtered_records(caller, filter), filter&.sort) page_window(records, filter).map { |record| project(record, projection) } end + # Exact, like the filter and the sort above it: the records in hand are + # every record Pylon holds, so a count or a group computed over them is + # the one a server-side aggregation would have answered — which is why + # these columns stay groupable where every other Pylon column is not. + # + # The rows are keyed with strings because that is how the agent reads + # them, while `Aggregation#apply` hands them back keyed with symbols. + def aggregate(caller, filter, aggregation, limit = nil) + aggregation.apply(filtered_records(caller, filter), timezone_for(caller), limit) + .map { |row| { 'group' => row[:group], 'value' => row[:value] } } + end + # One request answers any number of ids: the endpoint hands back the # complete dataset, so the ids only pick rows out of it. Read again on # every pass, like `list` — the freshness this collection trades bandwidth # for is not worth losing to a cache of related records. + # + # Only the wanted entities are serialized: a page of a pointing collection + # asks for a handful of ids, against every record the organization has. def records_indexed_by_id(ids) - fetch_all.map { |entity| serialize(entity) }.to_h { |record| [record['id'], record] }.slice(*ids) + wanted = Array(ids) + + fetch_all.each_with_object({}) do |entity, indexed| + next unless entity.is_a?(Hash) && wanted.include?(entity['id']) + + indexed[entity['id']] = serialize(entity) + end end protected # Every column is read-only in this story: writes land in a later one. - # Scalar columns are sortable because the in-memory sort honours any order - # asked of them; a Json column is neither sortable nor filterable, as it - # holds a list whose Pylon semantics have no in-memory counterpart — the - # same reason the primary-key residual guard refuses one. + # Scalar columns are sortable and groupable because the in-memory sort and + # aggregation honour anything asked of them; a Json column is none of the + # three, as it holds a list whose Pylon semantics have no in-memory + # counterpart — the same reason the primary-key residual guard refuses one. def add_column(name, type, is_primary_key: false) add_field(name, ColumnSchema.new(column_type: type, filter_operators: self.class.operators_for(type), is_primary_key: is_primary_key, is_sortable: type != 'Json', + is_groupable: type != 'Json', is_read_only: true)) end + # Pylon defines custom fields on issues, accounts and contacts only, so + # neither collection read this way has any. Refused rather than ignored: + # `serialize` has no hook here to read a custom-field value with, and the + # in-memory pass no table to clamp the declared operators against, so a + # declaration would register a column reading nil on every row forever. + def add_custom_fields(custom_fields) + return [] if custom_fields.empty? + + raise ConfigurationError, + "#{name} takes no custom field: Pylon defines them on issues, accounts and contacts only." + end + # The complete collection, straight from its unpaginated endpoint. def fetch_all = raise(NotImplementedError, "#{self.class} did not implement fetch_all") @@ -78,6 +110,12 @@ def serialize(_entity) = raise(NotImplementedError, "#{self.class} did not imple private + # The complete dataset, serialized and narrowed to the rows the filter + # keeps: what `list` pages and what `aggregate` counts are the same rows. + def filtered_records(caller, filter) + filter_in_memory(fetch_all.map { |entity| serialize(entity) }, caller, filter) + end + # The tree is applied over the complete dataset, so the rows it keeps are # the rows Pylon would have kept. `guard_nil_comparisons` is still worth # its cost: nothing in the schema advertises a bare comparison, but a diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb index ac962fe8d..fd629e905 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb @@ -2,6 +2,7 @@ module ForestAdminDatasourcePylon module Collections class Issue < BaseCollection include SchemaDefinition + include RecordSerialization include Serializer include RelationEmbedder @@ -31,19 +32,7 @@ def list(caller, filter, projection) protected - # A custom field is filtered through its Pylon slug, with the operators the - # integrator declared on the column. - def api_filters - @api_filters ||= custom_fields.each_with_object(ApiFilters::API_FILTERS.dup) do |cf, filters| - filters[cf[:column_name]] = ApiFilters.for_custom_field(cf[:schema]) - end - end - - # Declarations outside this list are dropped at registration, so the - # schema never advertises an operator the translator would refuse. - def allowed_custom_field_operators - ApiFilters::CUSTOM_FIELD_OPS.keys - end + def filter_table = ApiFilters def sortable_fields PYLON_SORTABLE @@ -75,17 +64,26 @@ def fetch_records(caller, filter) # record over a condition memory evaluates differently from Pylon — is # ruled out by `extract_id_lookup`, which refuses such residuals. def records_by_id(caller, lookup) - records = fetch_by_ids(lookup.ids).map { |issue| serialize(issue) } + records = fetch_by_ids(lookup.ids) return records if lookup.residual.nil? lookup.residual.apply(records, self, timezone_for(caller)) end + # `GET /issues/{id}` accepts the issue number as well as the UUID, so a + # record answering with an id other than the one asked for is dropped: + # see `matches_id?`. def fetch_by_ids(ids) wanted = ids.first(MAX_ID_LOOKUPS) warn_truncated_lookup(ids.size) if ids.size > wanted.size - wanted.filter_map { |id| fetch_issue(id) } + wanted.filter_map do |id| + record = fetch_issue(id) + next if record.nil? + + serialized = serialize(record) + serialized if matches_id?(serialized, id) + end end # A record the operator can no longer reach — deleted, or outside the diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb index 3f1524dfd..07b878767 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/api_filters.rb @@ -6,8 +6,10 @@ class Issue < BaseCollection # an operator absent from a field's map is rejected by Pylon. # # It is the single source of truth for filtering — `define_schema` derives - # every column's `filter_operators` from it, so the schema cannot - # advertise a filter the translator would then refuse. + # every column's `filter_operators` from it, so this collection declares no + # filter the translator would then refuse; the absence family the agent + # derives on top of it is the exception `Query::OperatorMaps::Table` + # describes. module ApiFilters Maps = Query::OperatorMaps diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb index 526aa55ad..924d82da5 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/schema_definition.rb @@ -9,8 +9,10 @@ class Issue < BaseCollection # # Filter operators are not chosen here: they come from # `ApiFilters::API_FILTERS`, which mirrors the allow-list of the API. A - # column missing from that table gets no operator, so the UI never offers - # a filter Pylon would refuse. + # column missing from that table gets no operator, so the UI offers no + # filter of this collection's own that Pylon would refuse — the absence + # family the agent derives above the datasource being the exception + # `Query::OperatorMaps::Table` describes. module SchemaDefinition ColumnSchema = BaseCollection::ColumnSchema ManyToOneSchema = BaseCollection::ManyToOneSchema @@ -49,7 +51,7 @@ def define_identity_fields # search allow-list, so it never reaches the translator. add_field('id', ColumnSchema.new(column_type: 'String', filter_operators: [Operators::EQUAL, Operators::IN], - is_primary_key: true, is_read_only: true)) + is_primary_key: true, is_groupable: false, is_read_only: true)) add_column('number', 'Number') add_column('link', 'String') end @@ -83,12 +85,6 @@ def define_time_fields add_column(field, 'Json') end end - - def add_column(name, type) - add_field(name, ColumnSchema.new(column_type: type, - filter_operators: ApiFilters.forest_operators(name), - is_read_only: true)) - end end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb index 1294dba90..932df6202 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue/serializer.rb @@ -2,8 +2,6 @@ module ForestAdminDatasourcePylon module Collections class Issue < BaseCollection module Serializer - include RecordSerialization - PARTY_FIELDS = { 'account_id' => 'account', 'requester_id' => 'requester', 'assignee_id' => 'assignee', 'team_id' => 'team' }.freeze diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb index ac947d388..f1c067553 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/relation_embedder.rb @@ -36,11 +36,16 @@ def embed_foreign(foreign_collection, relations, records, rows) end end - # A null foreign key asks for nothing, and the same id is asked for once - # however many rows point at it. + # A foreign key Pylon left empty asks for nothing — a blank one no more + # than a null one, and it would reach the `in` filter of the read below, + # which refuses a blank inside a list and would fail the whole page over + # one malformed key. The same id is asked for once however many rows point + # at it. def foreign_ids(records, relations) keys = relations.map { |_name, relation| relation.foreign_key } - records.flat_map { |record| keys.map { |key| record[key] } }.compact.uniq + records.flat_map { |record| keys.map { |key| record[key] } } + .reject { |id| id.nil? || id.to_s.empty? } + .uniq end # `account:name` asks for the `account` relation; a projected column, and a diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb index 889061a23..d4e2aca49 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb @@ -1,8 +1,8 @@ module ForestAdminDatasourcePylon module Collections class Team < FetchAllCollection - def initialize(datasource, custom_fields: []) - super(datasource, 'PylonTeam', custom_fields: custom_fields) + def initialize(datasource) + super(datasource, 'PylonTeam') end protected diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb index 88768bed0..7b90eedfe 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb @@ -3,8 +3,8 @@ module Collections class User < FetchAllCollection NATIVE_FIELDS = %w[id name email emails avatar_url status role_id is_deactivated].freeze - def initialize(datasource, custom_fields: []) - super(datasource, 'PylonUser', custom_fields: custom_fields) + def initialize(datasource) + super(datasource, 'PylonUser') end protected diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/condition_tree_translator.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/condition_tree_translator.rb index cf35126e2..8b3e0cf86 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/condition_tree_translator.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/condition_tree_translator.rb @@ -80,6 +80,7 @@ def translate_leaf(leaf) operator = spec[:ops][leaf.operator] raise_unsupported_operator(leaf, spec) unless operator + ensure_filterable_absence!(leaf, spec) with_value({ 'field' => (spec[:param] || leaf.field).to_s, 'operator' => operator }, operator, leaf) end @@ -91,6 +92,36 @@ def with_value(filter, operator, leaf) filter.merge('value' => @value.single(leaf)) end + # `present`, `blank` and `missing` are advertised on every field carrying + # an equality or a membership filter: the agent derives them from those + # above the datasource and rewrites them into a comparison with an empty + # value. Only a field the API reference documents `is_set` / `is_unset` on + # can answer one, and it answers it through those operators, never through + # the rewritten comparison -- which Pylon would match against the empty + # value as if it were a value of its own. + # + # Refused here rather than in FilterValue, which sees the empty value but + # not whether the field has a presence filter to answer it with. + def ensure_filterable_absence!(leaf, spec) + return unless absence_condition?(leaf) + return if spec[:ops].values.any? { |candidate| VALUELESS_OPERATORS.include?(candidate) } + + raise UnsupportedOperatorError, + "Pylon cannot filter '#{leaf.field}' for absence: the field carries no is_set / is_unset filter " \ + 'in the Pylon API reference, so a present, blank or missing condition on it cannot be translated. ' \ + 'Filter for absence on a field that does, or filter on a value instead.' + end + + # The shape the absence operators are rewritten into: a nil value, or a + # list holding nothing but blanks. An empty list is not one of them -- it + # comes from a filter carrying no value at all, which FilterValue reports. + def absence_condition?(leaf) + return true if leaf.value.nil? + return false unless leaf.value.is_a?(Array) && leaf.value.any? + + leaf.value.all? { |value| value.nil? || value.to_s.empty? } + end + def raise_too_deep(depth) raise UnsupportedOperatorError, "Pylon rejects a filter nested deeper than #{MAX_DEPTH} levels (reached #{depth}); " \ diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/filter_value.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/filter_value.rb index 144df8b6d..9d53b3b7e 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/filter_value.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/filter_value.rb @@ -22,8 +22,8 @@ def single(leaf) # translating to a filter matching everything. def list(leaf) values = Array(leaf.value) - raise_blank_in_list(leaf) if values.any? { |value| value.nil? || value.to_s.empty? } raise_empty_list(leaf) if values.empty? + raise_blank_in_list(leaf) if values.any? { |value| value.nil? || value.to_s.empty? } values.map { |value| format(value) } end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb index ea52c11bc..2d8998682 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb @@ -17,8 +17,13 @@ module OperatorMaps Operators::IN => 'in', Operators::NOT_IN => 'not_in' }.freeze + # MISSING is mapped as well although Pylon spells absence one way only: + # the toolkit rewrites it into `equal nil` when it is left out, which the + # translator cannot express, so a field the API does check for absence + # would refuse the very filter it can answer. PRESENCE = { Operators::PRESENT => 'is_set', - Operators::BLANK => 'is_unset' }.freeze + Operators::BLANK => 'is_unset', + Operators::MISSING => 'is_unset' }.freeze # Declaring the bare comparisons rather than before/after is what lets # the toolkit rewrite Today / PreviousWeek / ... into a pair of bounds, @@ -54,17 +59,41 @@ module OperatorMaps # Extended by a collection's `ApiFilters` module, whose `API_FILTERS` is # the single source of truth for what its endpoint filters: the schema - # derives every column's `filter_operators` from it, so it cannot - # advertise a filter the translator would then refuse. + # derives every column's `filter_operators` from it, so no collection + # declares a filter the translator would then refuse. + # + # One family escapes those tables. The agent derives `present`, `blank` and + # `missing` from an equality or a membership filter, above the datasource, + # and rewrites them into a comparison with an empty value. Only a field + # carrying PRESENCE can answer one -- Pylon matches an absent value through + # `is_set` / `is_unset` alone -- so on every other field the translator + # refuses the rewritten condition and names the filter to change, rather + # than sending a comparison Pylon would answer as if the empty value were + # a value of its own. module Table def forest_operators(field) self::API_FILTERS.dig(field, :ops)&.keys || [] end + # Read off the extending module rather than off this one, so the + # `CUSTOM_FIELD_OPS` a collection declares is the single source both + # this spelling and `allowed_custom_field_operators` come from: an + # endpoint accepting less on a custom field narrows one constant. def for_custom_field(schema) - { ops: CUSTOM_FIELD_OPS.slice(*Array(schema&.filter_operators)) } + { ops: self::CUSTOM_FIELD_OPS.slice(*Array(schema&.filter_operators)) } end end + + # The table of a collection whose endpoint filters nothing server-side: no + # field, and no operator on a custom field either. It is the default of + # `BaseCollection#filter_table`, so a collection read whole and filtered + # in memory needs no table of its own. + module EmptyTable + extend Table + + API_FILTERS = {}.freeze + CUSTOM_FIELD_OPS = {}.freeze + end end end end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb index 260d895dd..7c5737cc3 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb @@ -106,6 +106,13 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end + # No Pylon endpoint aggregates, and the pages of a cursor walk are not the + # dataset: a chart grouped by one of these columns would answer a fraction + # as if it were the whole collection. + it 'declares no column groupable' do + expect(columns.values.map(&:is_groupable).uniq).to eq([false]) + end + # `search_text` is native on /accounts/search, while Pylon exposes neither a # count endpoint nor a total, so Count stays out until it can be throttled. it 'enables search and leaves count disabled' do @@ -117,7 +124,8 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) expect(collection.fields['name'].filter_operators) .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::CONTAINS, operators::I_CONTAINS]) expect(collection.fields['owner_id'].filter_operators) - .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::PRESENT, operators::BLANK]) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK, operators::MISSING]) expect(collection.fields['tags'].filter_operators) .to eq([operators::CONTAINS, operators::NOT_CONTAINS, operators::IN, operators::NOT_IN]) expect(collection.fields['domains'].filter_operators) @@ -391,6 +399,17 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) expect(collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'acc-1')), %w[id])).to eq([]) end + # `GET /accounts/{id}` accepts an external id in place of the primary key + # and answers with the account carrying its own UUID. The row would not + # match the filter that asked for it, and the same filter combined with a + # scope — which goes through the search endpoint — answers nothing. + it 'reports no record when the endpoint answered an alias of the primary key' do + stub_request(:get, "#{base}/accounts/crm-1").to_return(json('data' => account_payload('acc-1'))) + query = filter(condition_tree: id_leaf(operators::EQUAL, 'crm-1')) + + expect(collection.list(nil, query, %w[id name])).to eq([]) + end + it 'propagates a failure that is not a missing record' do stub_request(:get, "#{base}/accounts/acc-1").to_return(json({ 'message' => 'boom' }, 500)) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb index 9dd6e3957..211b69502 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb @@ -42,9 +42,17 @@ def define_schema add_field('type', column.new(column_type: 'String')) add_field('tags', column.new(column_type: 'Json')) add_field('resolved_at', column.new(column_type: 'Date')) + add_field('account_id', column.new(column_type: 'String')) end - def define_relations; end + # One ManyToOne, so the relation guard can be observed both on a relation + # the collection declares and on a prefix naming none. + def define_relations + add_field('account', Collections::BaseCollection::ManyToOneSchema.new( + foreign_collection: 'PylonAccount', foreign_key: 'account_id', + foreign_key_target: 'id' + )) + end public :extract_id_lookup, :project, :translate_page, :add_custom_fields, :translate_sort, :timezone_for, :build_pylon_filter, :api_filters, :default_pk_sort?, @@ -130,6 +138,52 @@ def search_page(records, next_cursor = nil) end end + describe 'refusals' do + # The agent answers 400 carrying the message for a ValidationError, and 500 + # "Unexpected error" for anything else. Every refusal of this datasource + # names a filter the operator set and can change, and the message is the + # only place they learn which one. + it 'refuses through an error the agent answers 400 for' do + expect(UnsupportedOperatorError.new('nope')) + .to be_a(ForestAdminDatasourceToolkit::Exceptions::ValidationError) + end + + # No Pylon endpoint aggregates, and a count over the pages the agent walked + # would answer a fraction of the collection as if it were all of it. The + # contract's NotImplementedError would read as an oversight instead. + it 'refuses to aggregate, naming the collection' do + expect { collection.aggregate(nil, nil, nil) } + .to raise_error(UnsupportedOperatorError, /X cannot be aggregated/) + end + + # Forest offers a filter on a related field as soon as a ManyToOne is + # declared; Pylon has no join to answer it with, and the foreign key does + # the same job on the side the operator is already on. + it 'refuses a filter on a related field, naming the foreign key to use instead' do + query = filter(condition_tree: leaf('account:name', operators::EQUAL, 'Acme')) + + expect { collection.build_pylon_filter(nil, query) } + .to raise_error(UnsupportedOperatorError, + /related field 'account:name'.*Filter on 'account_id' instead.*PylonAccount list/m) + end + + it 'refuses one whose prefix names no relation of the collection' do + query = filter(condition_tree: leaf('nope:name', operators::EQUAL, 'Acme')) + + expect { collection.build_pylon_filter(nil, query) } + .to raise_error(UnsupportedOperatorError, /Filter on a column of this collection instead/) + end + + # Before the short-circuit reads it as a residual it cannot evaluate: the + # message names the relation rather than the in-memory pass. + it 'refuses one carried alongside an id' do + conditions = [leaf('id', operators::EQUAL, 'uuid-1'), leaf('account:name', operators::EQUAL, 'Acme')] + + expect { collection.extract_id_lookup(branch('And', conditions)) } + .to raise_error(UnsupportedOperatorError, /related field 'account:name'/) + end + end + describe '#extract_id_lookup' do it 'extracts a single id from an equality leaf' do lookup = collection.extract_id_lookup(leaf('id', operators::EQUAL, 'uuid-1')) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb index 0b2393f4f..5e66406dc 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb @@ -110,6 +110,13 @@ def stub_search(payload = { 'data' => [contact_payload('con-1')] }) expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end + # No Pylon endpoint aggregates, and the pages of a cursor walk are not the + # dataset: a chart grouped by one of these columns would answer a fraction + # as if it were the whole collection. + it 'declares no column groupable' do + expect(columns.values.map(&:is_groupable).uniq).to eq([false]) + end + it 'enables search and leaves count disabled' do expect(collection.is_searchable?).to be(true) expect(collection.is_countable?).to be(false) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb index 95680fc95..4cc21b991 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb @@ -15,6 +15,12 @@ def page(offset, limit) ForestAdminDatasourceToolkit::Components::Query::Page.new(offset: offset, limit: limit) end + def aggregation(operation, field: nil, groups: []) + ForestAdminDatasourceToolkit::Components::Query::Aggregation.new( + operation: operation, field: field, groups: groups + ) + end + def sort(*clauses) ForestAdminDatasourceToolkit::Components::Query::Sort.new(clauses) end @@ -122,6 +128,24 @@ def fetch_all expect(collection.is_searchable?).to be(false) expect(collection.is_countable?).to be(false) end + + # A group is computed over the complete dataset here, unlike every other + # Pylon collection: a chart grouped by a scalar column is exact. + it 'declares the scalar columns groupable and the json one not' do + expect(collection.fields['name'].is_groupable).to be(true) + expect(collection.fields['list'].is_groupable).to be(false) + end + + # Pylon defines custom fields on issues, accounts and contacts only, and + # nothing here reads a custom-field value nor clamps its operators: the + # column would read nil on every row forever. + it 'refuses a custom field rather than registering a column nothing fills' do + declared = { column_name: 'tier', + schema: ForestAdminDatasourceToolkit::Schema::ColumnSchema.new(column_type: 'String') } + + expect { subclass.new(datasource, 'X', custom_fields: [declared]) } + .to raise_error(ConfigurationError, /takes no custom field/) + end end # How a collection pointing here with a ManyToOne resolves its foreign keys. @@ -136,6 +160,54 @@ def fetch_all it 'leaves out an id the endpoint no longer returns rather than indexing a blank record' do expect(collection.records_indexed_by_id(%w[u1 gone]).keys).to eq(%w[u1]) end + + # A page of the pointing collection asks for a handful of ids against every + # record the organization has: serializing the whole dataset to slice it + # afterwards would pay for all of them, on every page. + it 'serializes the wanted records only' do + counting = Class.new(subclass) do + attr_reader :serialized + + protected + + def serialize(entity) + (@serialized ||= []) << entity['id'] + entity + end + end.new(datasource, 'X') + counting.entities = entities + + counting.records_indexed_by_id(%w[u1]) + + expect(counting.serialized).to eq(%w[u1]) + end + end + + # Exact where every other Pylon collection has to refuse: the records in + # hand are every record Pylon holds, so a count over them is the count a + # server-side aggregation would have answered. + describe '#aggregate' do + it 'counts the records the filter keeps' do + expect(collection.aggregate(nil, filter, aggregation('Count'))) + .to eq([{ 'group' => {}, 'value' => 3 }]) + expect(collection.aggregate(nil, filter(condition_tree: leaf('name', operators::PRESENT)), + aggregation('Count'))) + .to eq([{ 'group' => {}, 'value' => 2 }]) + end + + it 'groups by a column' do + rows = collection.aggregate(nil, filter, aggregation('Count', groups: [{ field: 'flag' }])) + + expect(rows).to contain_exactly({ 'group' => { 'flag' => false }, 'value' => 1 }, + { 'group' => { 'flag' => true }, 'value' => 1 }, + { 'group' => { 'flag' => nil }, 'value' => 1 }) + end + + it 'honours the limit the chart asks for' do + rows = collection.aggregate(nil, filter, aggregation('Count', groups: [{ field: 'name' }]), 1) + + expect(rows.size).to eq(1) + end end describe '.operators_for' do diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb index 90dd61ff9..2bfc0491e 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb @@ -88,6 +88,13 @@ def columns expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end + # No Pylon endpoint aggregates, and the pages of a cursor walk are not the + # dataset: a chart grouped by one of these columns would answer a fraction + # as if it were the whole collection. + it 'declares no column groupable' do + expect(columns.values.map(&:is_groupable).uniq).to eq([false]) + end + # `search_text` is native on /issues/search, while Pylon exposes neither a # count endpoint nor a total, so Count stays out until it can be throttled. it 'enables search and leaves count disabled' do @@ -99,7 +106,8 @@ def columns expect(collection.fields['state'].filter_operators).to eq([operators::EQUAL, operators::IN, operators::NOT_IN]) expect(collection.fields['assignee_id'].filter_operators) - .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::PRESENT, operators::BLANK]) + .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, + operators::PRESENT, operators::BLANK, operators::MISSING]) expect(collection.fields['title'].filter_operators) .to eq([operators::CONTAINS, operators::I_CONTAINS, operators::NOT_CONTAINS, operators::NOT_I_CONTAINS]) end @@ -276,6 +284,15 @@ def columns expect { collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, 'i1')), %w[id]) } .to raise_error(APIError) end + + # `GET /issues/{id}` accepts the issue number as well as the UUID and + # answers with the issue carrying its own id: keeping it would answer + # `id equals 42` with a row whose id is not 42. + it 'reports no record when the endpoint answered an alias of the primary key' do + stub_request(:get, "#{base}/issues/42").to_return(json('data' => issue_payload('i1'))) + + expect(collection.list(nil, filter(condition_tree: id_leaf(operators::EQUAL, '42')), %w[id])).to eq([]) + end end describe 'custom fields' do @@ -366,6 +383,28 @@ def columns expect(WebMock).not_to have_requested(:post, "#{base}/issues/search") end + # Declaring the four ManyToOne relations is what makes a filter on a + # related field reachable, and Pylon has no join to answer it with. The + # refusal names the foreign key, which is the filter that does the same + # job on the side the operator is already on. + it 'refuses a filter on a related field, naming the foreign key to use instead' do + query = filter(condition_tree: leaf('account:name', operators::CONTAINS, 'Acme')) + + expect { collection.list(nil, query, %w[id]) } + .to raise_error(UnsupportedOperatorError, + /related field 'account:name'.*Filter on 'account_id' instead.*PylonAccount list/m) + expect(WebMock).not_to have_requested(:post, "#{base}/issues/search") + end + + # The same filter alongside an id: the short-circuit sees the tree first, + # and would otherwise report the relation as a residual it cannot evaluate. + it 'refuses a filter on a related field combined with an id' do + conditions = [id_leaf(operators::EQUAL, 'i1'), leaf('account:name', operators::CONTAINS, 'Acme')] + + expect { collection.list(nil, filter(condition_tree: branch('And', conditions)), %w[id]) } + .to raise_error(UnsupportedOperatorError, /related field 'account:name'/) + end + it 'keeps the same filter across every page of the walk' do stub_request(:post, "#{base}/issues/search") .with(body: hash_including('limit' => 3)) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb index cd473acbb..27d973559 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/relation_embedder_spec.rb @@ -162,6 +162,26 @@ def columns_of(name) .with(body: hash_including('filter' => id_filter(%w[acc-1]))) end + # A blank key would otherwise reach the `in` filter of the read, which + # refuses a blank inside a list: one malformed key would fail the page. + it 'leaves a blank key out of the ids it asks for, like a null one' do + stub_issues(issue_payload('i1'), issue_payload('i2', 'account' => { 'id' => '' })) + stub_accounts(account_payload('acc-1')) + + rows = issues.list(nil, filter, %w[id account:name]) + + expect(rows.map { |row| row['account'] }).to eq([rows.first['account'], nil]) + expect(WebMock).to have_requested(:post, "#{base}/accounts/search") + .with(body: hash_including('filter' => id_filter(%w[acc-1]))) + end + + it 'sends no request at all when every foreign key of the page is blank' do + stub_issues(issue_payload('i1', 'team' => { 'id' => '' })) + + expect(issues.list(nil, filter, %w[id team:name]).first).to eq('id' => 'i1', 'team' => nil) + expect(WebMock).not_to have_requested(:get, "#{base}/teams") + end + # Deleted, merged, or outside the scope of the token: the row says so # rather than carrying a blank record the panel would offer to open. it 'embeds no record for a foreign key the endpoint no longer answers' do diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb index 8b1099862..0743f135e 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb @@ -63,6 +63,14 @@ def translate(tree, api_filters: default_filters, timezone: nil) .to eq('field' => 'assignee_id', 'operator' => 'is_unset') end + # Left out of the map, `missing` is rewritten by the toolkit into + # `equal nil`, which no Pylon operator expresses: the field would refuse + # the very absence check it is one of the few to accept. + it 'translates missing to the same is_unset as blank' do + expect(translate(leaf('assignee_id', operators::MISSING))) + .to eq('field' => 'assignee_id', 'operator' => 'is_unset') + end + # Pylon lists is_set / is_unset for the party ids and issue_type only. it 'refuses presence on a field Pylon does not accept it for' do expect { translate(leaf('state', operators::PRESENT)) } @@ -239,17 +247,29 @@ def translate(tree, api_filters: default_filters, timezone: nil) .to raise_error(UnsupportedOperatorError, /holding a blank value/) end - # `blank` on a String column is rewritten by the toolkit into `in [nil, - # '']`, so the message has to point at the presence operators rather than - # blame the caller for an empty list. - it 'points a list of blanks at the presence operators' do + # The agent advertises `present`, `blank` and `missing` on every field + # carrying an equality filter, and rewrites them into `not_in [nil, '']`, + # `in [nil, '']` and `equal nil`. On a field Pylon accepts no is_set / + # is_unset on, the refusal has to name the absence filter the operator set + # rather than the empty value it was rewritten into. + it 'refuses an absence filter on a field carrying no presence operator' do expect { translate(leaf('state', operators::IN, [nil, ''])) } - .to raise_error(UnsupportedOperatorError, /PRESENT or BLANK operator/) + .to raise_error(UnsupportedOperatorError, /cannot filter 'state' for absence/) + expect { translate(leaf('state', operators::NOT_IN, [nil, ''])) } + .to raise_error(UnsupportedOperatorError, /cannot filter 'state' for absence/) + expect { translate(leaf('state', operators::EQUAL, nil)) } + .to raise_error(UnsupportedOperatorError, /cannot filter 'state' for absence/) end - it 'refuses a nil value and points at the presence operators' do - expect { translate(leaf('state', operators::EQUAL, nil)) } + # `assignee_id` does carry is_set / is_unset, so a nil reaching the value + # comes from a scope or a segment written in Ruby rather than from a + # rewritten absence filter: the message names the operators to write it + # with instead. + it 'points a nil on a presence-filtered field at the presence operators' do + expect { translate(leaf('assignee_id', operators::EQUAL, nil)) } .to raise_error(UnsupportedOperatorError, /use the PRESENT or BLANK operator/) + expect { translate(leaf('assignee_id', operators::IN, [nil, ''])) } + .to raise_error(UnsupportedOperatorError, /holding a blank value/) end it 'refuses every filter when the collection declares none' do From cee51dd5639f611760abc875db0a18fb1f7938f8 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 18 Aug 2026 15:16:25 +0200 Subject: [PATCH 7/8] fix(pylon): resolve relation filters and stop truncating page-less reads page_window no longer slices when the filter carries no page, or a page naming no limit. FetchAllCollection was answering a page-less read with the first 1000 records of a larger set, which left SortCollectionDecorator without a position for the rest. domains and tags no longer advertise contains / not_contains: the columns are typed Json, on which the toolkit refuses every substring operator, so the UI offered a filter that could only error. Typing them as a String array is not a way out either, the validator has no branch for an array column type. A relation:field leaf is resolved into the foreign_key in [...] the endpoint filters, by reading the foreign collection for the keys of the records matching it, instead of being refused while the schema advertises the relation as filterable. Bounded at 500 keys and refused rather than truncated past that, and answered with no record and no request when no foreign record matched. Resolved at the entry of the read, so the primary-key lookup route benefits too and the tree is walked once instead of twice. Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/base_collection.rb | 133 +++++++++++++++--- .../collections/cursor_collection.rb | 11 +- .../collections/fetch_all_collection.rb | 9 +- .../collections/issue.rb | 11 +- .../query/operator_maps.rb | 21 ++- .../collections/account_spec.rb | 32 +++-- .../collections/base_collection_spec.rb | 130 ++++++++++++++--- .../collections/fetch_all_collection_spec.rb | 21 +++ .../collections/issue_spec.rb | 57 ++++++-- .../query/condition_tree_translator_spec.rb | 17 ++- 10 files changed, 366 insertions(+), 76 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb index 6f41a4403..e6f58b733 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb @@ -10,6 +10,9 @@ class BaseCollection < ForestAdminDatasourceToolkit::Collection ConditionTreeFactory = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeFactory Equivalent = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent SortFactory = ForestAdminDatasourceToolkit::Components::Query::SortUtils::SortFactory + Filter = ForestAdminDatasourceToolkit::Components::Query::Filter + Page = ForestAdminDatasourceToolkit::Components::Query::Page + Projection = ForestAdminDatasourceToolkit::Components::Query::Projection # `residual` holds the conditions left over once the primary-key leaf has # been taken out of the tree, for the caller to apply in memory. @@ -28,6 +31,18 @@ class BaseCollection < ForestAdminDatasourceToolkit::Collection # test `is_a?(String)` first. They need the guard added here. NIL_UNSAFE_OPERATORS = [Operators::LESS_THAN, Operators::GREATER_THAN, Operators::INCLUDES_ALL].freeze + # How many foreign keys a resolved relation condition may carry. Past this + # the condition is refused rather than truncated: keeping the first keys + # would answer a narrower question than the one asked, which is the very + # thing this datasource refuses — a result that looks filtered and is not. + MAX_RELATION_KEYS = 500 + + # What a resolved relation condition leaves behind when no foreign record + # matched it. It is not expressible as a filter — `FilterValue` refuses an + # empty `in`, whose Pylon meaning is undocumented and would read as "match + # everything" — so it travels as a marker the read answers with no record. + MATCHES_NOTHING = :pylon_matches_nothing + attr_reader :custom_fields # Template method: subclasses implement `define_schema` and @@ -79,7 +94,6 @@ def aggregate(_caller, _filter, _aggregation, _limit = nil) # `AND(id equal X, )` on a record detail as soon as a scope or a # segment is set, and `id` is not a field Pylon can filter on. def extract_id_lookup(node) - ensure_no_relation_leaf!(node) ids = id_values(node) return IdLookup.new(ids: ids, residual: nil) if ids return nil unless and_branch?(node) @@ -108,6 +122,23 @@ def ensure_searchless_lookup!(filter) 'Clear the search or drop the id condition.' end + # Yields the filter with every relation condition resolved into a condition + # on this collection's own columns, so the routes below — the search and + # the primary-key lookup, which applies its leftovers in memory — never see + # a `relation:field` leaf. + # + # Answers with no record at all, and no request, when the resolution found + # nothing to match: see `MATCHES_NOTHING`. + def with_resolved_relations(caller, filter) + tree = filter&.condition_tree + return yield(filter) unless tree&.some_leaf { |leaf| leaf.field.to_s.include?(':') } + + resolved = resolve_relation_conditions(caller, tree) + return [] if resolved == MATCHES_NOTHING + + yield(filter.override(condition_tree: resolved)) + end + # Forest asks for an offset/limit window, Pylon hands out cursor pages: the # walker bridges the two, `search_page` performs one call, and the records # it collected are serialized by the collection. @@ -130,14 +161,26 @@ def search_page(limit:, cursor:, filter:, search_text:) # Sliced after the lookup, not before, so ids that resolved to nothing # (404) do not eat into the requested window. + # + # A filter carrying no page — or a page naming no limit — asks for every + # record it matched, and the records are already in hand: there is no + # window to cut. The `MAX_SEARCH_LIMIT` fallback of `translate_page` is a + # cap on how far a walk of the API goes, which is a different question, + # and applying it here would answer a page-less read with the first + # thousand records of a larger set as if they were all of it. def page_window(records, filter) - offset, limit = translate_page(filter&.page) + page = filter&.page + return records if page.nil? + + offset = page.offset.to_i.clamp(0, nil) + limit = page.limit.to_i + return records.drop(offset) unless limit.positive? + records[offset, limit] || [] end def build_pylon_filter(caller, filter) tree = filter&.condition_tree - ensure_no_relation_leaf!(tree) ensure_no_stray_id!(tree) Query::ConditionTreeTranslator.call(tree, api_filters: api_filters, timezone: timezone_for(caller)) end @@ -316,20 +359,69 @@ def ensure_no_stray_id!(node) 'silently widen. Rewrite the filter with `and`, or filter on another field.' end - # Forest offers a filter on a related field as soon as a ManyToOne is - # declared, and sends it as a `relation:field` leaf. Pylon has no join and - # no include parameter, so there is nothing to translate it into: the - # matching records would have to be read from the foreign collection and - # their keys matched here, which is a read of its own, not a filter. + def resolve_relation_conditions(caller, node) + return resolve_relation_branch(caller, node) if node.is_a?(Branch) + return node unless node.field.to_s.include?(':') + + resolve_relation_leaf(caller, node) + end + + # An unmatchable condition empties an `and` and drops out of an `or`, which + # is how it would behave had it been sent as a condition on a column no + # record answers. + def resolve_relation_branch(caller, branch) + resolved = Array(branch.conditions).map { |condition| resolve_relation_conditions(caller, condition) } + return MATCHES_NOTHING if and_branch?(branch) && resolved.include?(MATCHES_NOTHING) + return Branch.new(branch.aggregator, resolved) if and_branch?(branch) + + kept = resolved.reject { |condition| condition == MATCHES_NOTHING } + kept.empty? ? MATCHES_NOTHING : Branch.new(branch.aggregator, kept) + end + + # Pylon has neither a join nor an include parameter, so a `relation:field` + # leaf — which Forest offers as soon as a ManyToOne is declared, and which + # the schema therefore advertises as filterable — has no translation as it + # stands. It is resolved instead, the way `RelationCollectionDecorator` + # resolves one on a relation the customizer added: the foreign collection + # is read for the keys of the records matching the condition, and the leaf + # becomes the `foreign_key in [...]` this collection does filter. # - # Refused with the foreign key of the relation, which is the filter the - # operator can set instead — and the one the reverse side is listed by. - def ensure_no_relation_leaf!(node) - return if node.nil? + # The read is the foreign collection's own, so its endpoint, its operators + # and its refusals apply — and a relation no condition names costs nothing, + # only a filter mentioning it triggers the read. + def resolve_relation_leaf(caller, leaf) + relation = schema[:fields][leaf.field.to_s.split(':').first] + raise_unfilterable_relation(leaf.field) unless resolvable_relation?(relation) + + keys = foreign_keys_matching(caller, relation, leaf) + keys.empty? ? MATCHES_NOTHING : Leaf.new(relation.foreign_key, Operators::IN, keys) + end + + # A relation is resolvable when its foreign key is a column this collection + # filters with `in` — server-side through `api_filters` for the collections + # that search, in memory for the ones read whole. Nothing else is: a + # OneToMany would have to be matched the other way round, which the schema + # never advertises as filterable, and a leaf reaching further than one + # relation is left to the foreign collection, which resolves its own. + def resolvable_relation?(relation) + return false unless relation.is_a?(ManyToOneSchema) - field = nil - node.some_leaf { |leaf| field = leaf.field if leaf.field.to_s.include?(':') } - raise_unfilterable_relation(field) if field + column = schema[:fields][relation.foreign_key] + column.is_a?(ColumnSchema) && column.filter_operators.include?(Operators::IN) + end + + # One record past the cap is asked for, so an overflow is seen rather than + # guessed from a full page. + def foreign_keys_matching(caller, relation, leaf) + foreign = datasource.get_collection(relation.foreign_collection) + target = relation.foreign_key_target + query = Filter.new(condition_tree: leaf.unnest, + page: Page.new(offset: 0, limit: MAX_RELATION_KEYS + 1)) + + records = foreign.list(caller, query, Projection.new([target])) + raise_too_many_relation_keys(leaf.field, relation) if records.size > MAX_RELATION_KEYS + + records.filter_map { |record| record[target] }.uniq end def raise_unfilterable_relation(field) @@ -343,7 +435,16 @@ def raise_unfilterable_relation(field) raise UnsupportedOperatorError, "Pylon cannot filter on the related field '#{field}': it has no join, so a condition on a " \ - "relation has no server-side translation. #{instead}" + 'relation is answered by reading the foreign collection for its keys, which this relation ' \ + "does not allow. #{instead}" + end + + def raise_too_many_relation_keys(field, relation) + raise UnsupportedOperatorError, + "The filter on '#{field}' matches more than #{MAX_RELATION_KEYS} #{relation.foreign_collection} " \ + 'records: Pylon has no join, so the condition travels as the list of their keys, and a list this ' \ + 'long is one the endpoint cannot carry. Narrow the condition on the related field, or filter on ' \ + "'#{relation.foreign_key}' directly." end def clamp_custom_field_operators(column_name, schema) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb index f476d0e8a..797f8e6ed 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb @@ -69,12 +69,15 @@ def search_by_ids(ids) def fetch_records(caller, filter) warn_unsortable(filter&.sort) - return listed_records(filter) if browsing?(filter) - id = single_id_lookup(filter) - return page_window(records_by_id(id), filter) if id + with_resolved_relations(caller, filter) do |query| + next listed_records(query) if browsing?(query) - search_records(caller, filter) + id = single_id_lookup(query) + next page_window(records_by_id(id), query) if id + + search_records(caller, query) + end end # Nothing to filter and nothing to search: the listing endpoint returns diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb index a477a161d..9cac206d3 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb @@ -112,8 +112,15 @@ def serialize(_entity) = raise(NotImplementedError, "#{self.class} did not imple # The complete dataset, serialized and narrowed to the rows the filter # keeps: what `list` pages and what `aggregate` counts are the same rows. + # + # Relation conditions are resolved first, like everywhere else: neither + # collection read this way declares a ManyToOne today, so what this refuses + # is a condition on the reverse side, which `match` would otherwise read as + # a missing column and answer by dropping every row. def filtered_records(caller, filter) - filter_in_memory(fetch_all.map { |entity| serialize(entity) }, caller, filter) + with_resolved_relations(caller, filter) do |query| + filter_in_memory(fetch_all.map { |entity| serialize(entity) }, caller, query) + end end # The tree is applied over the complete dataset, so the rows it keeps are diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb index fd629e905..e6261cad8 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/issue.rb @@ -51,11 +51,14 @@ def search_page(limit:, cursor:, filter:, search_text:) def fetch_records(caller, filter) warn_unsortable(filter&.sort) - lookup = extract_id_lookup(filter&.condition_tree) - return search_records(caller, filter) unless lookup - ensure_searchless_lookup!(filter) - page_window(records_by_id(caller, lookup), filter) + with_resolved_relations(caller, filter) do |query| + lookup = extract_id_lookup(query&.condition_tree) + next search_records(caller, query) unless lookup + + ensure_searchless_lookup!(query) + page_window(records_by_id(caller, lookup), query) + end end # The records are already narrowed to the ids the filter asked for, so diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb index 2d8998682..3a4230fd8 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/query/operator_maps.rb @@ -44,12 +44,21 @@ module OperatorMaps FULL_TEXT = SUBSTRING.merge(Operators::NOT_CONTAINS => 'string_does_not_contain', Operators::NOT_I_CONTAINS => 'string_does_not_contain').freeze - # A list-valued field -- `tags`, `domains`: `contains` asks whether one - # value belongs to it, while `in` matches it against several candidates at - # once. - MEMBERSHIP = { Operators::CONTAINS => 'contains', - Operators::NOT_CONTAINS => 'does_not_contain', - Operators::IN => 'in', + # A list-valued field -- `tags`, `domains`: `in` matches it against + # several candidates at once. + # + # Pylon also accepts `contains` / `does_not_contain` on such a field, and + # they are deliberately left out: the columns are typed `Json`, the only + # type the toolkit has for a list, and `Rules` allows a Json column the + # base and array operators alone. A declared `contains` would be refused + # by `ConditionTreeValidator` on the way in -- "the given operator + # 'contains' is not allowed with the columnType schema: 'Json'" -- so the + # UI would offer a filter that errors instead of one Pylon answers. + # Typing the columns `['String']` is not the way out either: no branch of + # `get_allowed_operators_for_column_type` reads an array type, and the + # validator raises a NoMethodError on it. Reaching those two operators + # takes a toolkit change, which is not this datasource's to make here. + MEMBERSHIP = { Operators::IN => 'in', Operators::NOT_IN => 'not_in' }.freeze # A custom field is filtered through its slug, so its operators come from diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb index 7c5737cc3..d3533424e 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb @@ -127,9 +127,23 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) .to eq([operators::EQUAL, operators::IN, operators::NOT_IN, operators::PRESENT, operators::BLANK, operators::MISSING]) expect(collection.fields['tags'].filter_operators) - .to eq([operators::CONTAINS, operators::NOT_CONTAINS, operators::IN, operators::NOT_IN]) + .to eq([operators::IN, operators::NOT_IN]) expect(collection.fields['domains'].filter_operators) - .to eq([operators::CONTAINS, operators::NOT_CONTAINS, operators::IN, operators::NOT_IN]) + .to eq([operators::IN, operators::NOT_IN]) + end + + # `POST /accounts/search` does filter a list column with `contains`, and + # the schema still must not advertise it: the column is typed Json, the + # only type the toolkit has for a list, and it allows no substring + # operator on one -- the filter would be refused on the way in. + it 'advertises no substring operator on a list column' do + allowed = ForestAdminDatasourceToolkit::Validations::Rules.get_allowed_operators_for_column_type('Json') + + %w[domains tags].each do |field| + expect(collection.fields[field].filter_operators) + .not_to include(operators::CONTAINS, operators::NOT_CONTAINS, operators::I_CONTAINS) + expect(allowed).to include(*collection.fields[field].filter_operators) + end end # /accounts/search accepts `string_contains` on a name but documents no @@ -281,10 +295,10 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) end it 'translates a membership filter on a list column' do - collection.list(nil, filter(condition_tree: leaf('tags', operators::CONTAINS, 'vip')), %w[id]) + collection.list(nil, filter(condition_tree: leaf('tags', operators::IN, %w[vip])), %w[id]) expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( - body: hash_including('filter' => { 'field' => 'tags', 'operator' => 'contains', 'value' => 'vip' }) + body: hash_including('filter' => { 'field' => 'tags', 'operator' => 'in', 'values' => %w[vip] }) ) end @@ -356,12 +370,12 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) stub_request(:post, "#{base}/accounts/search") .with(body: hash_including('cursor' => 'c1')) .to_return(json('data' => [account_payload('acc-3')])) - query = filter(condition_tree: leaf('tags', operators::CONTAINS, 'vip'), page: page(2, 1)) + query = filter(condition_tree: leaf('tags', operators::IN, %w[vip]), page: page(2, 1)) expect(collection.list(nil, query, %w[id])).to eq([{ 'id' => 'acc-3' }]) expect(WebMock).to have_requested(:post, "#{base}/accounts/search") - .with(body: hash_including('filter' => { 'field' => 'tags', 'operator' => 'contains', - 'value' => 'vip' })).twice + .with(body: hash_including('filter' => { 'field' => 'tags', 'operator' => 'in', + 'values' => %w[vip] })).twice end end @@ -444,14 +458,14 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) # which no in-memory pass could evaluate, is answered rather than refused. it 'searches instead when the filter carries more conditions' do stub_search - tree = branch('And', [id_leaf(operators::EQUAL, 'acc-1'), leaf('tags', operators::CONTAINS, 'vip')]) + tree = branch('And', [id_leaf(operators::EQUAL, 'acc-1'), leaf('tags', operators::IN, %w[vip])]) expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to eq([{ 'id' => 'acc-1' }]) expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( body: hash_including( 'filter' => { 'operator' => 'and', 'subfilters' => [{ 'field' => 'id', 'operator' => 'equals', 'value' => 'acc-1' }, - { 'field' => 'tags', 'operator' => 'contains', 'value' => 'vip' }] } + { 'field' => 'tags', 'operator' => 'in', 'values' => %w[vip] }] } ) ) end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb index 211b69502..2f2d05420 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb @@ -42,21 +42,29 @@ def define_schema add_field('type', column.new(column_type: 'String')) add_field('tags', column.new(column_type: 'Json')) add_field('resolved_at', column.new(column_type: 'Date')) - add_field('account_id', column.new(column_type: 'String')) + add_field('account_id', column.new(column_type: 'String', + filter_operators: [Collections::BaseCollection::Operators::IN])) + add_field('owner_id', column.new(column_type: 'String')) end - # One ManyToOne, so the relation guard can be observed both on a relation - # the collection declares and on a prefix naming none. + # Two ManyToOne, so the resolution can be observed on a relation whose + # foreign key this collection filters and on one whose foreign key it + # does not -- alongside a prefix naming no relation at all. def define_relations add_field('account', Collections::BaseCollection::ManyToOneSchema.new( foreign_collection: 'PylonAccount', foreign_key: 'account_id', foreign_key_target: 'id' )) + add_field('owner', Collections::BaseCollection::ManyToOneSchema.new( + foreign_collection: 'PylonUser', foreign_key: 'owner_id', + foreign_key_target: 'id' + )) end public :extract_id_lookup, :project, :translate_page, :add_custom_fields, :translate_sort, :timezone_for, :build_pylon_filter, :api_filters, :default_pk_sort?, - :ensure_searchless_lookup!, :search_records, :page_window, :warn_unsortable + :ensure_searchless_lookup!, :search_records, :page_window, :warn_unsortable, + :with_resolved_relations end end @@ -156,31 +164,117 @@ def search_page(records, next_cursor = nil) .to raise_error(UnsupportedOperatorError, /X cannot be aggregated/) end - # Forest offers a filter on a related field as soon as a ManyToOne is - # declared; Pylon has no join to answer it with, and the foreign key does - # the same job on the side the operator is already on. - it 'refuses a filter on a related field, naming the foreign key to use instead' do - query = filter(condition_tree: leaf('account:name', operators::EQUAL, 'Acme')) + # A relation whose foreign key this collection does not filter cannot be + # resolved: the keys read from the foreign collection would have nothing + # to be matched against. + it 'refuses a filter on a relation whose foreign key it cannot filter' do + query = filter(condition_tree: leaf('owner:name', operators::EQUAL, 'Bob')) - expect { collection.build_pylon_filter(nil, query) } + expect { collection.with_resolved_relations(nil, query) { |q| q } } .to raise_error(UnsupportedOperatorError, - /related field 'account:name'.*Filter on 'account_id' instead.*PylonAccount list/m) + /related field 'owner:name'.*Filter on 'owner_id' instead.*PylonUser list/m) end it 'refuses one whose prefix names no relation of the collection' do query = filter(condition_tree: leaf('nope:name', operators::EQUAL, 'Acme')) - expect { collection.build_pylon_filter(nil, query) } + expect { collection.with_resolved_relations(nil, query) { |q| q } } .to raise_error(UnsupportedOperatorError, /Filter on a column of this collection instead/) end + end + + # Pylon has no join, so a condition on a related field is answered by reading + # the foreign collection for the keys matching it. + describe '#with_resolved_relations' do + let(:foreign) { instance_double(Collections::Account) } + + before { allow(datasource).to receive(:get_collection).with('PylonAccount').and_return(foreign) } + + def resolved(tree) + collection.with_resolved_relations(nil, filter(condition_tree: tree), &:condition_tree) + end + + def returning(*ids) + allow(foreign).to receive(:list) { ids.map { |id| { 'id' => id } } } + end + + it 'leaves a filter naming no relation untouched, without reading anything' do + tree = leaf('state', operators::EQUAL, 'new') + + expect(resolved(tree)).to be(tree) + expect(datasource).not_to have_received(:get_collection) + end + + it 'rewrites the leaf into the foreign keys of the matching records' do + returning('acc-1', 'acc-2') + + expect(resolved(leaf('account:name', operators::EQUAL, 'Acme')).to_h) + .to eq(field: 'account_id', operator: operators::IN, value: %w[acc-1 acc-2]) + end + + # The condition reaches the foreign collection unnested, asking only for + # the key it is matched against, and bounded so an overflow is seen. + it 'reads the foreign collection for the target key alone' do + returning('acc-1') + resolved(leaf('account:name', operators::EQUAL, 'Acme')) + + expect(foreign).to have_received(:list) do |_caller, query, projection| + expect(query.condition_tree.to_h).to eq(field: 'name', operator: operators::EQUAL, value: 'Acme') + expect(query.page.to_h).to eq(offset: 0, limit: Collections::BaseCollection::MAX_RELATION_KEYS + 1) + expect(projection).to eq(['id']) + end + end + + it 'resolves a relation nested inside a branch, leaving the other conditions in place' do + returning('acc-1') + tree = branch('And', [leaf('state', operators::EQUAL, 'new'), leaf('account:name', operators::EQUAL, 'Acme')]) + + expect(resolved(tree).to_h[:conditions].last) + .to eq(field: 'account_id', operator: operators::IN, value: %w[acc-1]) + end + + # No foreign record matched, so no record of this collection can: answered + # without a request, rather than with an empty `in` reading as "everything". + it 'answers with no record when nothing matched, without running the read' do + returning + ran = false + query = filter(condition_tree: leaf('account:name', operators::EQUAL, 'Acme')) + + expect(collection.with_resolved_relations(nil, query) { ran = true }).to eq([]) + expect(ran).to be(false) + end + + it 'empties an and whose relation matched nothing' do + returning + tree = branch('And', [leaf('state', operators::EQUAL, 'new'), leaf('account:name', operators::EQUAL, 'Acme')]) + + expect(collection.with_resolved_relations(nil, filter(condition_tree: tree)) { |q| q }).to eq([]) + end + + # The other side of the union still selects records, so the unmatchable + # branch drops out instead of emptying the read. + it 'drops an unmatchable branch out of an or' do + returning + tree = branch('Or', [leaf('state', operators::EQUAL, 'new'), leaf('account:name', operators::EQUAL, 'Acme')]) + + expect(resolved(tree).to_h) + .to eq(aggregator: 'Or', conditions: [{ field: 'state', operator: operators::EQUAL, value: 'new' }]) + end + + it 'answers with no record when every branch of an or matched nothing' do + returning + tree = branch('Or', [leaf('account:name', operators::EQUAL, 'A'), leaf('account:name', operators::EQUAL, 'B')]) + + expect(collection.with_resolved_relations(nil, filter(condition_tree: tree)) { |q| q }).to eq([]) + end - # Before the short-circuit reads it as a residual it cannot evaluate: the - # message names the relation rather than the in-memory pass. - it 'refuses one carried alongside an id' do - conditions = [leaf('id', operators::EQUAL, 'uuid-1'), leaf('account:name', operators::EQUAL, 'Acme')] + # Truncating would answer a narrower question than the one asked, without + # saying so. + it 'refuses to truncate a relation matching more records than the cap' do + returning(*Array.new(Collections::BaseCollection::MAX_RELATION_KEYS + 1) { |i| "acc-#{i}" }) - expect { collection.extract_id_lookup(branch('And', conditions)) } - .to raise_error(UnsupportedOperatorError, /related field 'account:name'/) + expect { resolved(leaf('account:name', operators::EQUAL, 'Acme')) } + .to raise_error(UnsupportedOperatorError, /matches more than 500 PylonAccount records/) end end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb index 4cc21b991..0c9148d1f 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/fetch_all_collection_spec.rb @@ -269,6 +269,27 @@ def serialize(entity) it 'slices the requested page out of the ordered records' do expect(ids(collection.list(nil, filter(sort: sort(by('id')), page: page(1, 1)), nil))).to eq(%w[u2]) end + + # A read asking for no page asks for the whole dataset, which is the read + # `SortCollectionDecorator` performs to build its reference order: capping + # it at the search limit would drop every record past the thousandth and + # leave the decorator without a position for them. + it 'answers a page-less read with every record, past the search limit' do + collection.entities = Array.new(Client::MAX_SEARCH_LIMIT + 5) { |i| { 'id' => "u#{i}" } } + + expect(collection.list(nil, filter, nil).size).to eq(Client::MAX_SEARCH_LIMIT + 5) + expect(collection.list(nil, nil, nil).size).to eq(Client::MAX_SEARCH_LIMIT + 5) + end + + # The same read with an offset and no limit -- which `Page#apply` reads as + # "to the end of the records" too: the offset is honoured, the tail is not + # cut. + it 'honours an offset carrying no limit without capping the tail' do + collection.entities = Array.new(Client::MAX_SEARCH_LIMIT + 5) { |i| { 'id' => "u#{i}" } } + query = filter(page: page(2, nil)) + + expect(collection.list(nil, query, nil).size).to eq(Client::MAX_SEARCH_LIMIT + 3) + end end describe '#list with a filter' do diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb index 2bfc0491e..1792b932c 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/issue_spec.rb @@ -384,25 +384,56 @@ def columns end # Declaring the four ManyToOne relations is what makes a filter on a - # related field reachable, and Pylon has no join to answer it with. The - # refusal names the foreign key, which is the filter that does the same - # job on the side the operator is already on. - it 'refuses a filter on a related field, naming the foreign key to use instead' do - query = filter(condition_tree: leaf('account:name', operators::CONTAINS, 'Acme')) + # related field reachable, and the schema advertises it as filterable. + # Pylon has no join, so it is answered by reading the accounts matching + # the condition and sending their ids as the `account_id` filter the + # issues endpoint does take. + it 'answers a filter on a related field with the keys of the matching records' do + stub_request(:post, "#{base}/accounts/search") + .to_return(json('data' => [{ 'id' => 'acc-1' }, { 'id' => 'acc-2' }])) + stub_request(:post, "#{base}/issues/search").to_return(json('data' => [])) - expect { collection.list(nil, query, %w[id]) } - .to raise_error(UnsupportedOperatorError, - /related field 'account:name'.*Filter on 'account_id' instead.*PylonAccount list/m) + collection.list(nil, filter(condition_tree: leaf('account:name', operators::CONTAINS, 'Acme')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts/search").with( + body: hash_including('filter' => { 'field' => 'name', 'operator' => 'string_contains', + 'value' => 'Acme' }) + ) + expect(WebMock).to have_requested(:post, "#{base}/issues/search").with( + body: hash_including('filter' => { 'field' => 'account_id', 'operator' => 'in', + 'values' => %w[acc-1 acc-2] }) + ) + end + + # No account matched, so no issue can: answered without asking the issues + # endpoint, where an empty `in` would have read as no filter at all. + it 'answers with no issue when no related record matched, without searching' do + stub_request(:post, "#{base}/accounts/search").to_return(json('data' => [])) + + expect(collection.list(nil, filter(condition_tree: leaf('account:name', operators::CONTAINS, 'Acme')), + %w[id])).to eq([]) expect(WebMock).not_to have_requested(:post, "#{base}/issues/search") end - # The same filter alongside an id: the short-circuit sees the tree first, - # and would otherwise report the relation as a residual it cannot evaluate. - it 'refuses a filter on a related field combined with an id' do + # Resolved before the short-circuit reads the tree, so the relation + # becomes an `account_id in [...]` the id lookup can apply in memory. + it 'resolves a filter on a related field combined with an id' do + stub_request(:post, "#{base}/accounts/search").to_return(json('data' => [{ 'id' => 'acc-1' }])) + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) conditions = [id_leaf(operators::EQUAL, 'i1'), leaf('account:name', operators::CONTAINS, 'Acme')] - expect { collection.list(nil, filter(condition_tree: branch('And', conditions)), %w[id]) } - .to raise_error(UnsupportedOperatorError, /related field 'account:name'/) + expect(collection.list(nil, filter(condition_tree: branch('And', conditions)), %w[id])) + .to eq([{ 'id' => 'i1' }]) + end + + # A relation the filter never names costs nothing: no foreign read is + # triggered by declaring it. + it 'reads no foreign collection for a filter naming none' do + stub_request(:post, "#{base}/issues/search").to_return(json('data' => [])) + + collection.list(nil, filter(condition_tree: leaf('state', operators::EQUAL, 'new')), %w[id]) + + expect(WebMock).not_to have_requested(:post, "#{base}/accounts/search") end it 'keeps the same filter across every page of the walk' do diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb index 0743f135e..19dbfa1e1 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/query/condition_tree_translator_spec.rb @@ -101,14 +101,21 @@ def translate(tree, api_filters: default_filters, timezone: nil) .to raise_error(UnsupportedOperatorError, /not supported on field 'title'/) end - # `tags` holds a list, so membership has its own pair of operators. + # `tags` holds a list, so membership is matched against candidates rather + # than compared. it 'translates tag membership with the list operators' do - expect(translate(leaf('tags', operators::CONTAINS, 'urgent'))) - .to eq('field' => 'tags', 'operator' => 'contains', 'value' => 'urgent') - expect(translate(leaf('tags', operators::NOT_CONTAINS, 'urgent'))) - .to eq('field' => 'tags', 'operator' => 'does_not_contain', 'value' => 'urgent') expect(translate(leaf('tags', operators::IN, %w[urgent vip]))) .to eq('field' => 'tags', 'operator' => 'in', 'values' => %w[urgent vip]) + expect(translate(leaf('tags', operators::NOT_IN, %w[urgent]))) + .to eq('field' => 'tags', 'operator' => 'not_in', 'values' => %w[urgent]) + end + + # Pylon accepts `contains` on a list field, and the map leaves it out: the + # column is typed Json, on which the toolkit refuses the operator before + # the translator ever sees it. + it 'refuses the substring operators on a list column' do + expect { translate(leaf('tags', operators::CONTAINS, 'urgent')) } + .to raise_error(UnsupportedOperatorError, /not supported on field 'tags'/) end end From f4440457b7b45933c5651d138798441cdb62e61a Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Tue, 18 Aug 2026 16:14:23 +0200 Subject: [PATCH 8/8] fix(pylon): stop truncating page-less reads at the walk page_window stopped slicing a page-less read in cee51dd, which fixed FetchAllCollection, where the records are already in hand. The cursor collections take theirs from the walk, and the walk was still bounded: translate_page answered a missing page with MAX_SEARCH_LIMIT, so a read asking for every record travelled as a read asking for a thousand. The walker could not tell the two apart, broke out on records.size >= needed before reaching its cap check, and answered a larger set with its first thousand records without logging a truncation. translate_page now answers a missing page, or a page naming no positive limit, with a nil limit, and walk reads nil as "every record past the offset": it follows the cursor until Pylon reports no page left or a cap stops it, and a cap stopping it is a truncation the warning now names. batch_size bounds itself on the record budget alone when there is no window, and the final slice becomes a drop. A window the caller did ask for keeps its exact cost and its silence. Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/base_collection.rb | 18 ++++---- .../pagination/cursor_walker.rb | 30 ++++++++++---- .../collections/account_spec.rb | 12 ++++++ .../collections/base_collection_spec.rb | 21 ++++++++-- .../pagination/cursor_walker_spec.rb | 41 +++++++++++++++++++ 5 files changed, 103 insertions(+), 19 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb index e6f58b733..f6b209458 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb @@ -164,10 +164,8 @@ def search_page(limit:, cursor:, filter:, search_text:) # # A filter carrying no page — or a page naming no limit — asks for every # record it matched, and the records are already in hand: there is no - # window to cut. The `MAX_SEARCH_LIMIT` fallback of `translate_page` is a - # cap on how far a walk of the API goes, which is a different question, - # and applying it here would answer a page-less read with the first - # thousand records of a larger set as if they were all of it. + # window to cut. How far the walk that collected them went is a different + # question, answered by `translate_page` and the caps of the walker. def page_window(records, filter) page = filter&.page return records if page.nil? @@ -280,11 +278,17 @@ def project(record, projection) wanted.to_h { |k| [k, record[k]] } end + # A filter carrying no page — or a page naming no limit — asks for every + # record it matched, and travels to the walk as no limit at all rather than + # as `MAX_SEARCH_LIMIT`: a limit standing in for "everything" is one the + # walk cannot tell from a window the caller asked for, so it would stop at + # a thousand records having answered a larger set, and stop silently — the + # truncation warning only fires on a walk that knows it was cut short. def translate_page(page) - return [0, Client::MAX_SEARCH_LIMIT] if page.nil? + return [0, nil] if page.nil? - limit = page.limit.to_i.positive? ? page.limit.to_i : Client::MAX_SEARCH_LIMIT - [page.offset.to_i.clamp(0, nil), limit] + limit = page.limit.to_i + [page.offset.to_i.clamp(0, nil), limit.positive? ? limit : nil] end # Adds custom fields, skipping any whose column name collides with a diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/pagination/cursor_walker.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/pagination/cursor_walker.rb index ea086ace9..d68cab2ad 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/pagination/cursor_walker.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/pagination/cursor_walker.rb @@ -16,10 +16,20 @@ def initialize(max_pages: MAX_PAGES, max_records: MAX_RECORDS) end # Yields `(limit, cursor)` and expects a Client::SearchPage back. + # + # A nil limit asks for every record past the offset: the walk then runs + # until Pylon says there is no page left, or until a cap stops it. That + # distinction is the whole point of accepting nil rather than a limit + # standing in for "everything": a walk told to collect a thousand records + # stops at a thousand having covered the window it was given, and reports + # nothing, while a walk told to collect everything and stopped by a cap + # knows it is handing back less than it was asked for, and says so. def walk(offset:, limit:) - return [] unless limit.to_i.positive? + offset = offset.to_i.clamp(0, nil) + limit = limit&.to_i + return [] if limit && !limit.positive? - needed = offset.to_i + limit.to_i + needed = limit && (offset + limit) records = [] cursor = nil pages = 0 @@ -29,7 +39,8 @@ def walk(offset:, limit:) records.concat(page.records) pages += 1 - break if stop?(page, cursor) || records.size >= needed + break if stop?(page, cursor) + break if needed && records.size >= needed if capped?(pages, records.size) log_truncation(offset: offset, limit: limit, pages: pages, collected: records.size) @@ -39,7 +50,7 @@ def walk(offset:, limit:) cursor = page.next_cursor end - records[offset.to_i, limit.to_i] || [] + limit ? (records[offset, limit] || []) : records.drop(offset) end private @@ -55,16 +66,19 @@ def capped?(pages, collected) pages >= @max_pages || collected >= @max_records end - # Bounded by the window still missing and by the record budget left, so - # the walk never collects past @max_records. + # Bounded by the record budget left, and by the window still missing when + # there is one, so the walk never collects past @max_records. def batch_size(needed, collected) - [needed - collected, @max_records - collected].min.clamp(1, Client::MAX_SEARCH_LIMIT) + budget = @max_records - collected + budget = [needed - collected, budget].min if needed + budget.clamp(1, Client::MAX_SEARCH_LIMIT) end def log_truncation(offset:, limit:, pages:, collected:) + window = limit ? "offset=#{offset} limit=#{limit}" : "every record past offset=#{offset}" ForestAdminDatasourcePylon.logger.warn( "[forest_admin_datasource_pylon] Stopped paginating after #{pages} page(s) / #{collected} record(s) " \ - "while fetching offset=#{offset} limit=#{limit}; results are truncated. " \ + "while fetching #{window}; results are truncated. " \ 'Narrow the filter to reach records past this point.' ) end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb index d3533424e..1a5b7764c 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb @@ -275,6 +275,18 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) expect(collection.list(nil, filter(page: page(2, 1)), %w[id])).to eq([{ 'id' => 'acc-3' }]) end + # Browsing carries no page when a decorator above asks for the whole + # collection — an emulated sort, an emulated operator — and the walk has to + # answer with the collection rather than with its first page. + it 'walks every page when the read carries no page at all' do + stub_list({ 'limit' => '1000' }, + 'data' => [account_payload('acc-1')], + 'pagination' => { 'cursor' => 'c1', 'has_next_page' => true }) + stub_list({ 'limit' => '1000', 'cursor' => 'c1' }, 'data' => [account_payload('acc-2')]) + + expect(collection.list(nil, filter, %w[id])).to eq([{ 'id' => 'acc-1' }, { 'id' => 'acc-2' }]) + end + it 'returns an empty list when the organization has no account' do stub_list({ 'limit' => '1000' }, 'data' => []) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb index 2f2d05420..607b2d93f 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/base_collection_spec.rb @@ -531,6 +531,19 @@ def api_filters .to eq([{ limit: Client::MAX_SEARCH_LIMIT, cursor: nil, filter: nil, search_text: nil }]) end + # The regression guarded here: a page-less read used to travel to the walk + # as `limit: MAX_SEARCH_LIMIT`, which the walk could not tell from a window + # the caller asked for — so it stopped at the first full page and answered a + # larger set with its first thousand records, and did so without a warning. + it 'follows the cursor past the first page when the filter carries no page' do + collection = searching(search_page([{ 'id' => 'a' }], 'c1'), + search_page([{ 'id' => 'b' }], 'c2'), + search_page([{ 'id' => 'c' }])) + + expect(collection.search_records(nil, filter).map { |record| record['id'] }).to eq(%w[a b c]) + expect(collection.calls.map { |call| call[:cursor] }).to eq([nil, 'c1', 'c2']) + end + # The walker asks for the window still missing and hands back the cursor of # the previous page; the filter and the search stay the same throughout. it 'follows the cursor until the requested window is covered' do @@ -622,16 +635,16 @@ def api_filters end describe '#translate_page' do - it 'defaults to a single full-size page when Forest sends none' do - expect(collection.translate_page(nil)).to eq([0, Client::MAX_SEARCH_LIMIT]) + it 'asks for every record when Forest sends no page' do + expect(collection.translate_page(nil)).to eq([0, nil]) end it 'passes the offset and limit through' do expect(collection.translate_page(page(10, 25))).to eq([10, 25]) end - it 'falls back to the maximum limit when the page carries none' do - expect(collection.translate_page(page(0, nil))).to eq([0, Client::MAX_SEARCH_LIMIT]) + it 'asks for every record when the page carries no limit' do + expect(collection.translate_page(page(0, nil))).to eq([0, nil]) end it 'clamps a negative offset to zero' do diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/pagination/cursor_walker_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/pagination/cursor_walker_spec.rb index 116d4a897..527c43138 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/pagination/cursor_walker_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/pagination/cursor_walker_spec.rb @@ -62,6 +62,32 @@ def walk(pages, offset:, limit:, walker: described_class.new) expect(calls).to be_empty end + describe 'a nil limit' do + it 'walks every page the API hands out instead of stopping at one window' do + pages = [search_page(%w[a b], 'c1'), search_page(%w[c d], 'c2'), search_page(%w[e], nil)] + + expect(walk(pages, offset: 0, limit: nil).size).to eq(5) + expect(calls.size).to eq(3) + end + + it 'asks for the whole record budget on each page' do + walk([search_page(%w[a], nil)], offset: 0, limit: nil) + + expect(calls.first[:limit]).to eq(ForestAdminDatasourcePylon::Client::MAX_SEARCH_LIMIT) + end + + it 'still drops the offset' do + pages = [search_page(%w[a b c], 'c1'), search_page(%w[d], nil)] + + expect(walk(pages, offset: 2, limit: nil)).to eq([{ 'id' => 'c' }, { 'id' => 'd' }]) + end + + it 'costs a single request when the first page is the last' do + expect(walk([search_page(%w[a b], nil)], offset: 0, limit: nil).size).to eq(2) + expect(calls.size).to eq(1) + end + end + describe 'truncation' do before { allow(ForestAdminDatasourcePylon.logger).to receive(:warn) } @@ -93,6 +119,21 @@ def walk(pages, offset:, limit:, walker: described_class.new) expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) end + + it 'warns when a cap cuts a walk that asked for every record' do + pages = Array.new(5) { |i| search_page(%W[a#{i} b#{i}], "c#{i}") } + + expect(walk(pages, offset: 0, limit: nil, walker: described_class.new(max_records: 4)).size).to eq(4) + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with(/every record past offset=0; results are truncated/) + end + + it 'does not warn when a window the caller asked for is covered exactly' do + pages = [search_page(%w[a b], 'c1'), search_page(%w[c d], 'c2')] + + expect(walk(pages, offset: 0, limit: 2).size).to eq(2) + expect(ForestAdminDatasourcePylon.logger).not_to have_received(:warn) + end end describe 'defensive stops' do