From f9841f04ab6c45a7ac30869a6b593a95b961cb82 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 4 Sep 2026 10:21:12 +0200 Subject: [PATCH 1/3] feat(datasource): intercom relations of the reference tier Declares the relations between the collections this datasource already serves, and answers them (lot 2.5, PRD-1145). Publishing a many-to-one is not free: the agent marks it filterable as soon as any column of its target is, so the interface offers `assignee:name` the moment the relation exists. The traversal is therefore the bulk of this change, not the declarations. * eight many-to-one relations towards Admin, Team, TicketState and TicketType, whose targets all shipped with lot 1; * IntercomTeamMembership, synthesized from GET /teams: Intercom carries the membership on both sides and exposes no resource for the pair, while a many-to-many needs a collection to travel through. Read-only, Intercom writing none; * admin_names and team_names replace the arrays of ids on Team and Admin, read only when a projection asks for them; * one label per object on a ticket: state_category and state_external_label are dropped, both being a hop away on the state relation and neither having ever been filterable; * a projection through a relation reads its target once per page; a condition through one is resolved by the target, exactly, and becomes a condition on the foreign key. Over fifteen matching ids, or on a key the endpoint does not filter, it is refused by name rather than approximated. Co-Authored-By: Claude Opus 5 (1M context) --- .../README.md | 82 ++++++- .../collections/admin.rb | 41 +++- .../collections/base_collection.rb | 38 +++- .../collections/conversation.rb | 34 ++- .../collections/cursor_collection.rb | 68 +++++- .../collections/fetch_all_collection.rb | 19 +- .../collections/relations.rb | 200 +++++++++++++++++ .../collections/team.rb | 40 +++- .../collections/team_membership.rb | 61 ++++++ .../collections/ticket.rb | 37 +++- .../collections/ticket/serializer.rb | 4 +- .../datasource.rb | 4 + .../collections/admin_spec.rb | 75 +++++-- .../collections/conversation_spec.rb | 58 ++++- .../collections/fetch_all_collection_spec.rb | 4 +- .../collections/team_membership_spec.rb | 206 ++++++++++++++++++ .../collections/team_spec.rb | 123 +++++++++-- .../collections/ticket_spec.rb | 201 ++++++++++++++++- .../datasource_spec.rb | 7 +- 19 files changed, 1212 insertions(+), 90 deletions(-) create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb create mode 100644 packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb create mode 100644 packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb diff --git a/packages/forest_admin_datasource_intercom/README.md b/packages/forest_admin_datasource_intercom/README.md index 38503e773..ee5d176d7 100644 --- a/packages/forest_admin_datasource_intercom/README.md +++ b/packages/forest_admin_datasource_intercom/README.md @@ -60,7 +60,9 @@ beats not running. A read-only token is enough, and is what to recommend for this lot. A permission the token lacks costs **columns or a collection, never the boot of the agent**: the ticket-type introspection -degrades to no attribute column, and a collection whose endpoint answers 403 fails its own page. +degrades to no attribute column, a collection whose endpoint answers 403 fails its own page, and a +token that cannot read `/admins` or `/teams` leaves the membership names empty without touching the +relation, which reads them from the other side. ## Collections @@ -70,12 +72,13 @@ degrades to no attribute column, and a collection whose endpoint answers 403 fai | `IntercomTicket` | `POST /tickets/search`, `GET /tickets/{id}` | cursor | yes, exactly | | `IntercomAdmin` | `GET /admins` | read whole | yes, exactly | | `IntercomTeam` | `GET /teams` | read whole | yes, exactly | +| `IntercomTeamMembership` | `GET /teams` | read whole | yes, exactly | | `IntercomTicketType` | `GET /ticket_types` | read whole | yes, exactly | | `IntercomTicketState` | `GET /ticket_states` | read whole | yes, exactly | Two tiers, and they behave differently on purpose. -**Read whole** — admins, teams, ticket types, ticket states. Their endpoints answer in one response, +**Read whole** — admins, teams, team memberships, ticket types, ticket states. Their endpoints answer in one response, so filtering, sorting, paging and counting them in memory is *exact*: the records in hand are every record Intercom holds. These are the only collections that can be filtered, sorted and grouped in this lot, and the only ones a chart may group by. The cost is bandwidth, not correctness. @@ -86,6 +89,52 @@ nothing is filtered or sorted in memory. Three routes and no fourth: no conditio Intercom's search DSL and walked through the search endpoint. What the translation cannot express is **refused by name** — see [Filtering](#filtering). +## Relations + +Intercom joins nothing: a ticket carries an assignee id, and the teammate behind it is a second read +of a second endpoint. What makes eight relations affordable is that every collection on the far end +is read whole in one request — so a relation resolves for a **whole page at the price of one read**, +never one read per row, and it resolves *exactly*. + +| Collection | Relation | Target | Filterable through | +| --- | --- | --- | --- | +| `IntercomConversation` | `admin_assignee`, `closed_by` | `IntercomAdmin` | yes | +| `IntercomConversation` | `team_assignee` | `IntercomTeam` | yes | +| `IntercomTicket` | `admin_assignee` | `IntercomAdmin` | yes | +| `IntercomTicket` | `team_assignee` | `IntercomTeam` | yes | +| `IntercomTicket` | `ticket_type` | `IntercomTicketType` | yes | +| `IntercomTicket` | `state`, `previous_state` | `IntercomTicketState` | **no** — read and navigate only | +| `IntercomTeam` | `admins` | `IntercomAdmin` | no (many-to-many) | +| `IntercomAdmin` | `teams` | `IntercomTeam` | no (many-to-many) | +| `IntercomTeamMembership` | `team`, `admin` | `IntercomTeam`, `IntercomAdmin` | yes | + +Every one of them is **read-only**: this lot writes nothing, and Intercom exposes no endpoint that +writes a team membership at all. + +**`IntercomTeamMembership` exists because Intercom's does not.** The workspace carries the +membership on the team (`admin_ids`) and on the teammate (`team_ids`) both and exposes no resource +for the pair, while a many-to-many needs a collection to travel through. It is synthesized from +`GET /teams`, one record per pair, keyed `teamId:adminId`. Without it, both sides read as an array of +ids nobody can click. + +Alongside it, a team names its teammates (`admin_names`) and a teammate its teams (`team_names`) on +the row itself, so a list view reads without a join. **Those replace the arrays of ids** the first +lots published: one readable form plus a relation to navigate, rather than two ways to read one fact. +They are read only when a projection asks for them, and a token that cannot read the other side +costs the column and nothing else — never the page, and never the relation. + +The same rule settled the ticket labels: `state_label` and `ticket_type_name` stay on the row, +`state_category` and `state_external_label` are gone — they are a hop away, on the `state` relation, +and neither was ever filterable, so no segment, scope or saved filter could rest on them. + +A relation reads its target **undecorated**, so a permission scope or a segment defined on the target +does not narrow what a relation resolves — the same way a native datasource joins a table without +applying the scopes of the collection mapped to it. + +One semantic worth stating plainly: **a row whose foreign key is null matches no relation filter**, +the way a join drops it, negated filters included. A ticket with no assignee is not "assigned to +someone other than Marie". + ## What the API cannot do, and what this does about it Where Forest asks for something Intercom has no equivalent for, this datasource **refuses with a @@ -200,6 +249,9 @@ refuses a search by name. `last_responder_name`, `last_responder_type`. They exist nowhere in Intercom; `/tickets/search` filters none of them and ignores a sort on them without a word; - **the account of a ticket** — `company_id`, refused by the endpoint itself with `invalid_field`; +- **the state of a ticket** — the measured table carries no filter on a state id, so `state_id`, + `previous_state_id` and the `state` relation are read and navigated rather than filtered. Whether + the endpoint filters one at all is one of the probe's open questions; - **the ticket attributes** — filtered as `ticket_attribute.{id}`, and the same attribute carries a different id per ticket type, so a union column has no single id to translate to. See [Tickets](#tickets); @@ -212,6 +264,30 @@ refuses a search by name. - **group-by**, on either cursor collection: there is no aggregate endpoint, and grouping over the pages a walk collected would look exact while answering a fraction. +### Through a relation + +A relation is published filterable as soon as *any* column of its target is — the agent decides that, +not this datasource — so the interface offers `admin_assignee:name` the moment the relation exists. +What Intercom is really filtered on is the foreign key: the **target says which of its records +match**, over every record it holds rather than over a page, and the ids it names become the +condition the search carries. + +That is exact, and it has three visible edges: + +- Intercom takes no membership operator on these fields, so several matches become **one equality per + match**, inside an `OR` — which counts against the fifteen conditions a group allows. A relation + condition matching more records than that is refused by name rather than sent and answered with a + 400 naming neither the limit nor the filter that hit it. +- A condition the target matched **no record** with names no row, and the DSL cannot say so: the + search is skipped entirely rather than sent as a filter that would come back with everything. +- A relation whose foreign key the endpoint does not filter — the ticket `state` — is refused with a + message saying which of the two it is: the relation is there to be read and navigated. Whether + `/tickets/search` filters a state id at all is one of the probe's open questions; the answer lands + in the table, not in an assumption. + +On the collections read whole the same condition costs nothing: they filter in memory, so the ids go +in as a plain membership and none of the DSL's limits apply. + ### The limits of a search, checked before the request leaves Intercom nests a search **two levels** deep and takes **fifteen conditions per group**. Past either @@ -327,7 +403,7 @@ Everything else is read when a collection is listed, so an agent boots whatever | Lot | What it brings | | --- | --- | | 3 | Writes and business actions: reply, close, snooze, reopen, assign, tag, convert | -| 4 | Contacts and companies, and the relations promoted from today's denormalized columns | +| 4 | Contacts and companies, and the relations towards them promoted from today's denormalized columns | | 5 | Notes, tags, segments | | 6 | Bounded group-by and the reporting export | diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb index 45704ea82..5814b8a4a 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb @@ -25,11 +25,37 @@ def serialize(admin) 'away_mode_enabled' => attrs['away_mode_enabled'], 'away_mode_reassign' => attrs['away_mode_reassign'], 'has_inbox_seat' => attrs['has_inbox_seat'], - 'team_ids' => Array(attrs['team_ids']).map { |id| stringify_id(id) } } + # Not a column: it is what the names below are read from, and what the + # membership collection turns into the relation. + 'team_ids' => Array(attrs['team_ids']).map { |id| stringify_id(id) }, + 'team_names' => nil } + end + + # The teams the teammate belongs to, by name, and only when a projection + # asked for them: one read of `/teams` for the whole page. A token that + # cannot read them costs the column and nothing else. + def enrich(records, rows, projection) + return unless Array(projection).map(&:to_s).include?('team_names') + + names = team_names + records.each_with_index do |record, index| + rows[index]['team_names'] = Array(record['team_ids']).filter_map { |id| names[id] } + end end private + def team_names + client.fetch_all('teams', list_key: 'teams') + .to_h { |team| [stringify_id(team['id']), team['name']] } + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} could not read the teams of the workspace (HTTP " \ + "#{e.status || "-"}); the names are left empty. The relation to IntercomTeam is unaffected." + ) + {} + end + def define_schema add_column('id', 'String', is_primary_key: true) add_column('name', 'String') @@ -41,11 +67,14 @@ def define_schema add_column('away_mode_enabled', 'Boolean') add_column('away_mode_reassign', 'Boolean') add_column('has_inbox_seat', 'Boolean') - # A list, so neither filterable nor sortable. It stays a plain column - # rather than a relation: Intercom carries the membership on the admin - # and on the team both, so declaring it twice would give the schema two - # sides of a many-to-many with no join collection to hold it. - add_column('team_ids', 'Json') + # A list, so neither filterable nor sortable -- as the array of ids it + # replaces was. It reads the teammate without a join; the relation below + # is what navigates it, through the membership collection that gives the + # many-to-many the join Intercom does not expose. + add_column('team_names', 'Json') + add_many_to_many('teams', foreign_collection: 'IntercomTeam', + through_collection: 'IntercomTeamMembership', + origin_key: 'admin_id', foreign_key: 'team_id') end end end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb index 9c4731f27..2ed15a3c6 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb @@ -4,11 +4,15 @@ module Collections # record is narrowed to the projection asked for, and how a window is cut # out of records already in hand. # - # Read-only for now. The writes and the business actions arrive with lot 3, - # and the relations with lot 4, once Contacts and Companies exist -- a - # relation whose target collection is missing is a schema the agent refuses - # to boot on. + # Read-only for now: the writes and the business actions arrive with lot 3. + # The relations towards Contacts and Companies wait for lot 4, those two + # collections not existing yet -- a relation whose target collection is + # missing is a schema the agent refuses to boot on. The relations between the + # collections this datasource already serves are declared and answered here, + # through `Relations`. class BaseCollection < ForestAdminDatasourceToolkit::Collection + include Relations + ColumnSchema = ForestAdminDatasourceToolkit::Schema::ColumnSchema Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators Equivalent = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::ConditionTreeEquivalent @@ -27,14 +31,24 @@ def client def define_schema = raise(NotImplementedError, "#{self.class} did not implement define_schema") - # A record narrowed to what was asked for. A projection naming a field the - # record does not carry yields nil rather than nothing at all: the agent - # asked for a column, and an absent key would read as a record missing it. + # A record narrowed to the columns that were asked for. A projection naming + # a field the record does not carry yields nil rather than nothing at all: + # the agent asked for a column, and an absent key would read as a record + # missing it. + # + # A path through a relation is not a column and is skipped here: it is + # answered by `embed_relations`, which nests a whole row under the relation + # name once the page is in hand. + # + # No projection at all asks for every column -- which is not the same as + # every key a serialized record happens to carry: a couple of them hold + # what a column is read *from*, the ids behind a membership for one, and + # publishing those would show the operator the plumbing. def project(record, projection) - fields = Array(projection) - return record if fields.empty? + asked = Array(projection).map(&:to_s) + return record.slice(*column_names) if asked.empty? - fields.to_h { |field| [field, record[field]] } + asked.reject { |field| field.include?(':') }.to_h { |field| [field, record[field]] } end # The window a list view asked for, cut out of records already in hand. @@ -54,6 +68,10 @@ def page_window(records, filter) records[offset, limit] || [] end + def column_names + @column_names ||= fields.select { |_, field| field.is_a?(ColumnSchema) }.keys + end + # The timezone in-memory date comparisons are evaluated in. The caller's, # since that is whose "today" the filter was written against. def timezone_for(caller) diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb index dbe8a53d0..380caeec9 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/conversation.rb @@ -10,7 +10,7 @@ module Collections # customers, and rendering third-party HTML inside Forest is neither safe nor # useful (R10). # Long by line count only: most of it declares the columns, one call each. - class Conversation < CursorCollection + class Conversation < CursorCollection # rubocop:disable Metrics/ClassLength include ContactIdentity include Conversation::Serializer include Conversation::Timeline @@ -69,18 +69,38 @@ def define_schema add_column('updated_at', 'Date') add_column('waiting_since', 'Date') add_column('snoozed_until', 'Date') - add_column('admin_assignee_id', 'String') - add_column('team_assignee_id', 'String') - # The conversation carries its company as a whole object, so the account - # name is free here -- unlike on a ticket, which carries the id alone. - add_column('company_id', 'String') - add_column('company_name', 'String') + define_assignment_columns define_contact_columns define_source_columns define_statistics_columns add_column('tag_names', 'Json') add_column('ai_agent_participated', 'Boolean') add_column('timeline', 'Json') + define_relations + end + + # Who the conversation sits with, and who closed it. All three targets are + # read whole in one request, and `/conversations/search` takes a filter on + # each of the three keys -- so these relations can be read, navigated and + # filtered through alike. + # + # No relation towards the company: a conversation carries its account as a + # whole object, so the name is already on the row, and the Companies + # collection arrives with lot 4. + def define_relations + add_many_to_one('admin_assignee', foreign_collection: 'IntercomAdmin', foreign_key: 'admin_assignee_id') + add_many_to_one('team_assignee', foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + add_many_to_one('closed_by', foreign_collection: 'IntercomAdmin', foreign_key: 'closed_by_id') + end + + # Who the conversation sits with, and which account it belongs to. The + # conversation carries its company as a whole object, so the account name + # is free here -- unlike on a ticket, which carries the id alone. + def define_assignment_columns + add_column('admin_assignee_id', 'String') + add_column('team_assignee_id', 'String') + add_column('company_id', 'String') + add_column('company_name', 'String') end # The contact identity is denormalized onto the row rather than declared as diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb index 17c7d3c39..52a56853b 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb @@ -16,6 +16,10 @@ module Collections # refuses by name: an unfiltered page served in answer to a filter is the # one failure this datasource is built to avoid. # + # A condition through a relation is resolved before any of that: the target + # collection is asked which of its records match, and what reaches Intercom + # is a condition on the foreign key. See `Relations`. + # # Counting is the exception that costs nothing: `total_count` is exact on # every response, filter included, so the record counter is one request. # Long by line count only: half of it is the refusals, and a refusal that @@ -39,8 +43,14 @@ def list(caller, filter, projection) warn_ignored_sort(filter&.sort) records = fetch_records(caller, filter) - rows = records.map { |record| project(serialize(record), projection) } + # Serialized whole and projected afterwards rather than the other way + # round: a projection reaching through a relation names no foreign key, + # and the key is where the relation is read from. + serialized = records.map { |record| serialize(record) } + rows = serialized.map { |record| project(record, projection) } + enrich(records, rows, projection) + embed_relations(caller, serialized, rows, projection) rows end @@ -146,7 +156,13 @@ def fetch_records(caller, filter) # having been dropped by the truncation before the window was applied. return records_by_ids(page_window(ids, filter)) if ids - listed_records(filter, translate(caller, filter)) + query = translate(caller, filter) + # A condition through a relation the target matched no record with names + # no row, and Intercom's DSL cannot say so: the read is skipped rather + # than sent as a filter that would come back with everything. + return [] if query == NOTHING + + listed_records(filter, query) end # The Intercom query a filter comes down to, or nil for a list view, which @@ -155,12 +171,53 @@ def fetch_records(caller, filter) # one tree, it is checked against the nesting Intercom allows like every # other condition, instead of adding a level nothing counted. def translate(caller, filter) - tree = combined_tree(filter) + tree = rewrite_relation_conditions(caller, combined_tree(filter)) do |key, ids, leaf| + relation_group(key, ids, leaf) + end + return NOTHING if tree == NOTHING Query::ConditionTreeTranslator.call(tree, endpoint: search_endpoint, collection: name, timezone: timezone_for(caller)) end + # The ids the target matched, written as the filter Intercom does take on + # the foreign key: a group of equalities, its DSL offering no membership + # operator on these fields. + # + # That group counts against the fifteen conditions Intercom allows, so a + # relation condition matching more records than that is refused rather than + # sent -- and refused here, where the message can name the relation the + # operator filtered on rather than the key it resolved to. + def relation_group(key, ids, leaf) + refuse_fan_out!(leaf, key, ids) if ids.size > Query::ConditionTreeTranslator::MAX_GROUP_SIZE + return Leaf.new(key, Operators::EQUAL, ids.first) if ids.size == 1 + + Branch.new('Or', ids.map { |id| Leaf.new(key, Operators::EQUAL, id) }) + end + + # A relation this endpoint filters nothing through. It is navigable all the + # same -- the read costs nothing, the target being read whole -- and saying + # which of the two it is, is the whole point of the message. + def check_relation_filterable!(leaf, relation) + key = relation.foreign_key + refuse_unfilterable_key!(leaf, key) if search_endpoint.field(key).nil? + end + + def refuse_unfilterable_key!(leaf, key) + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: the relation resolves to #{key.inspect}, on " \ + "which #{search_endpoint.path} takes no filter. The relation is there to be read and " \ + "navigated; filter on one of: #{search_endpoint.filterable_columns.join(", ")}." + end + + def refuse_fan_out!(leaf, key, ids) + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: it names #{ids.size} records, " \ + "#{search_endpoint.path} answers #{key.inspect} one value at a time, and Intercom takes " \ + "#{Query::ConditionTreeTranslator::MAX_GROUP_SIZE} conditions per group. Narrow the condition " \ + "on the relation, or filter on #{key.inspect} itself." + end + def combined_tree(filter) conditions = [filter&.condition_tree, search_condition(filter)].compact return conditions.first if conditions.size < 2 @@ -244,7 +301,10 @@ def count_records(caller, filter) ids = id_lookup(filter) return records_by_ids(ids).size if ids - page = read_page(per_page: 1, cursor: nil, query: translate(caller, filter)) + query = translate(caller, filter) + return 0 if query == NOTHING + + page = read_page(per_page: 1, cursor: nil, query: query) return page.total_count if page.total_count raise UnsupportedOperatorError, diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb index ceeefe5f5..862c349f1 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb @@ -57,8 +57,12 @@ def initialize(datasource, name) def list(caller, filter, projection) records = sort_in_memory(filtered_records(caller, filter), filter&.sort) + window = page_window(records, filter) + rows = window.map { |record| project(record, projection) } - page_window(records, filter).map { |record| project(record, projection) } + enrich(window, rows, projection) + embed_relations(caller, window, rows, projection) + rows end # Exact, like the filter and the sort above it, which is why these columns @@ -90,6 +94,12 @@ def add_column(name, type, is_primary_key: false) is_groupable: type != 'Json')) end + # Hook for what a row needs beyond the endpoint this collection reads -- + # a name held on the other side of a membership, say. Called with the rows + # of the page only, and with the projection, so a column nobody asked for + # costs no request. + def enrich(_records, _rows, _projection); end + # Every record of the collection, straight from its endpoint. def fetch_all = raise(NotImplementedError, "#{self.class} did not implement fetch_all") @@ -105,6 +115,13 @@ def filtered_records(caller, filter) tree = filter&.condition_tree return records if tree.nil? + # A condition through a relation becomes a membership on the foreign key: + # this tier filters in memory, where a list of ids costs no more than one + # id and none of Intercom's group limits apply -- they bound its search + # DSL, which nothing here goes through. + tree = rewrite_relation_conditions(caller, tree) { |key, ids, _| Leaf.new(key, Operators::IN, ids) } + return [] if tree == NOTHING + refuse_unevaluable!(tree) tree.apply(records, self, timezone_for(caller)) end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb new file mode 100644 index 000000000..51c6f76f7 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb @@ -0,0 +1,200 @@ +module ForestAdminDatasourceIntercom + module Collections + # How this datasource declares a relation, and how it answers one. + # + # Intercom joins nothing: a ticket carries an assignee id, and the teammate + # behind it is a second read of a second endpoint. What makes that affordable + # -- and exact -- is that every collection on the far end of these relations + # is read whole in one request, the tier `FetchAllCollection` serves. + # + # Declaring a relation obliges both halves below, and the second is not + # optional: a many-to-one is published filterable as soon as *any* column of + # its target is (`GeneratorField#build_many_to_one_schema`), so the interface + # offers `assignee:name` the moment the relation exists. Left unanswered, + # that filter reaches a translator refusing every traversing field -- the + # interface offering a filter the datasource then refuses, which is the one + # thing this package is built not to do. + # + # * a projection through a relation (`assignee:name`) is answered by reading + # the target once per page and nesting its row under the relation name; + # * a filter through a relation is answered by asking the target which of its + # records match, and filtering Intercom on the ids it names. + # + # The second is exact rather than approximate -- the target answers over + # every record it holds, not over a page -- with one semantic worth stating + # plainly: a row whose foreign key is null matches no relation filter, the + # way a join drops it, negated filters included. A ticket with no assignee is + # not "assigned to someone other than Marie". + module Relations + ManyToOneSchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema + ManyToManySchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema + Filter = ForestAdminDatasourceToolkit::Components::Query::Filter + Projection = ForestAdminDatasourceToolkit::Components::Query::Projection + Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators + Branch = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + Leaf = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + + # What a relation condition comes to when the target matched no record: no + # row can satisfy it. It is a value rather than an empty condition because + # the two tiers spell "match nothing" differently -- in memory an `in []` + # says it, while Intercom's search DSL has no way to. + NOTHING = :matches_nothing + + protected + + # Every relation of this package points at a collection keyed by `id` and + # is read-only: nothing in this lot writes, and an editable relation would + # offer an association the collection cannot perform. + def add_many_to_one(name, foreign_collection:, foreign_key:) + add_field(name, ManyToOneSchema.new(foreign_collection: foreign_collection, + foreign_key: foreign_key, + foreign_key_target: 'id', + is_read_only: true)) + end + + def add_many_to_many(name, foreign_collection:, through_collection:, origin_key:, foreign_key:) + add_field(name, ManyToManySchema.new(foreign_collection: foreign_collection, + through_collection: through_collection, + origin_key: origin_key, origin_key_target: 'id', + foreign_key: foreign_key, foreign_key_target: 'id', + is_read_only: true)) + end + + # The rows of one page, with the relations the projection named nested onto + # them. `records` are the same rows before projection: that is where the + # foreign keys are read, since a projection naming `assignee:name` does not + # have to name `admin_assignee_id`. + # + # One request per relation per page, never one per row. + def embed_relations(caller, records, rows, projection) + relations_asked(projection).each do |name, sub_projection| + relation = fields[name] + next unless relation.is_a?(ManyToOneSchema) + + key = relation.foreign_key + ids = records.filter_map { |record| record[key] }.uniq + targets = indexed_targets(caller, relation, ids, sub_projection) + + # Nil rather than absent when an id names no record: a teammate who + # left the workspace reads as no teammate, not as a broken row. + records.each_with_index { |record, index| rows[index][name] = targets[record[key]] } + end + end + + # The tree this tier can filter on, every `relation:field` leaf traded for + # a condition on the foreign key. The block is handed that key and the ids + # the target matched, and answers the node the tier wants: an `in` where + # the filtering is done in memory, a group of equalities where Intercom's + # DSL takes no membership operator. + # + # An `and` carrying a leaf that matches nothing matches nothing itself; an + # `or` drops that leaf and keeps its siblings. + def rewrite_relation_conditions(caller, node, &builder) + case node + when Branch then rewrite_branch(caller, node, &builder) + when Leaf then relation_leaf?(node) ? rewrite_relation_leaf(caller, node, &builder) : node + else node + end + end + + private + + def relation_leaf?(node) + node.is_a?(Leaf) && node.field.to_s.include?(':') + end + + def relations_asked(projection) + Projection.new(Array(projection).map(&:to_s)).relations + end + + # The target's own key travels with the projection whether or not it was + # asked for: it is what the rows are indexed by here, and what makes the + # nested row a link rather than a label in the interface. + def indexed_targets(caller, relation, ids, sub_projection) + return {} if ids.empty? + + target = relation.foreign_key_target + wanted = Projection.new(Array(sub_projection).map(&:to_s).union([target])) + filter = Filter.new(condition_tree: Leaf.new(target, Operators::IN, ids)) + + foreign_collection(relation).list(caller, filter, wanted) + .to_h { |row| [row[target], row] } + end + + def rewrite_branch(caller, branch, &builder) + rewritten = Array(branch.conditions).map { |node| rewrite_relation_conditions(caller, node, &builder) } + + if branch.aggregator.to_s.casecmp('or').zero? + kept = rewritten.reject { |node| node == NOTHING } + kept.empty? ? NOTHING : Branch.new(branch.aggregator, kept) + elsif rewritten.include?(NOTHING) + NOTHING + else + Branch.new(branch.aggregator, rewritten) + end + end + + def rewrite_relation_leaf(caller, leaf) + name, path = leaf.field.to_s.split(':', 2) + relation = fields[name] + refuse_unknown_relation!(leaf, name) unless relation.is_a?(ManyToOneSchema) + refuse_two_hops!(leaf) if path.include?(':') + + check_relation_filterable!(leaf, relation) + + ids = matching_ids(caller, relation, Leaf.new(path, leaf.operator, leaf.value)) + return NOTHING if ids.empty? + + yield(relation.foreign_key, ids, leaf) + end + + # Hook for a tier that cannot filter on every foreign key it declares a + # relation on. Answered before the target is read: a refusal that spends a + # request first costs exactly what it refuses to do. + def check_relation_filterable!(_leaf, _relation); end + + # Which records of the target the condition names, asked of the target + # itself: it owns what its columns can be filtered with, and it answers + # over every record Intercom holds rather than over a page of them. + def matching_ids(caller, relation, leaf) + target = relation.foreign_key_target + + foreign_collection(relation) + .list(caller, Filter.new(condition_tree: leaf), Projection.new([target])) + .filter_map { |row| row[target] } + .uniq + end + + # The target as the datasource holds it, undecorated -- so a permission + # scope or a segment defined on the target does not narrow what a relation + # resolves. That is how a native datasource behaves too: it joins the table + # without applying the scopes of the collection mapped to it. + def foreign_collection(relation) + datasource.get_collection(relation.foreign_collection) + end + + # Either a name this collection carries no relation under -- a condition + # from a scope or a segment written against another schema -- or a + # many-to-many, which is published unfilterable: resolving one would mean + # reading the collection it travels through once per value. + def refuse_unknown_relation!(leaf, relation_name) + filterable = fields.select { |_, field| field.is_a?(ManyToOneSchema) }.keys + + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: #{relation_name.inspect} is not a relation it filters " \ + "through. #{filterable.empty? ? "It has none." : "Those it does: #{filterable.join(", ")}."} Filter " \ + 'on a column of the collection next door instead.' + end + + # Two hops would mean resolving a relation of a relation, one read per + # level, and nothing in this datasource publishes a filter that deep -- a + # many-to-many is published unfilterable. It is refused by name rather than + # half-answered. + def refuse_two_hops!(leaf) + raise UnsupportedOperatorError, + "#{name} cannot filter #{leaf.field.inspect}: it reaches through two relations, and this datasource " \ + 'resolves one. Filter on a column of the collection next door instead.' + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb index 2286bfa46..59c62f85a 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb @@ -19,17 +19,49 @@ def serialize(team) { 'id' => stringify_id(attrs['id']), 'name' => attrs['name'], - 'admin_ids' => Array(attrs['admin_ids']).map { |id| stringify_id(id) } } + # Not a column: it is what the names below are read from, and what the + # membership collection turns into the relation. Intercom types these + # as numbers here and as strings on the admin itself, so they are + # stringified for both sides to carry the same id. + 'admin_ids' => Array(attrs['admin_ids']).map { |id| stringify_id(id) }, + 'admin_names' => nil } + end + + # The teammates of the team, by name, and only when a projection asked for + # them: one read of `/admins` for the whole page, never one per team. A + # token that cannot read the teammates costs the column and nothing else. + def enrich(records, rows, projection) + return unless Array(projection).map(&:to_s).include?('admin_names') + + names = admin_names + records.each_with_index do |record, index| + rows[index]['admin_names'] = Array(record['admin_ids']).filter_map { |id| names[id] } + end end private + def admin_names + client.fetch_all('admins', list_key: 'admins') + .to_h { |admin| [stringify_id(admin['id']), admin['name']] } + rescue APIError => e + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} could not read the teammates of the workspace (HTTP " \ + "#{e.status || "-"}); the names are left empty. The relation to IntercomAdmin is unaffected." + ) + {} + end + def define_schema add_column('id', 'String', is_primary_key: true) add_column('name', 'String') - # Intercom types these as numbers here and as strings on the admin - # itself; they are stringified so both sides carry the same id. - add_column('admin_ids', 'Json') + # A list, so neither filterable nor sortable -- as the array of ids it + # replaces was. It reads the team without a join; the relation below is + # what navigates it, and filtering happens on the Admins collection. + add_column('admin_names', 'Json') + add_many_to_many('admins', foreign_collection: 'IntercomAdmin', + through_collection: 'IntercomTeamMembership', + origin_key: 'team_id', foreign_key: 'admin_id') end end end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb new file mode 100644 index 000000000..9a1d662b0 --- /dev/null +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team_membership.rb @@ -0,0 +1,61 @@ +module ForestAdminDatasourceIntercom + module Collections + # The membership of the inbox teams: one record per team-and-teammate pair. + # + # It exists because Intercom's does not. The workspace carries the membership + # on the team (`admin_ids`) and on the teammate (`team_ids`) both, and offers + # no resource for the pair -- while a many-to-many needs a collection to + # travel through, whose two many-to-one relations are what + # `Utils::Collection.get_through_target` looks for. Declared here, Teams and + # Admins navigate to each other in both directions; left undeclared, both + # sides read as an array of ids. + # + # Read from `/teams` alone. The admin side names the same pairs, so reading + # it too would spend a request confirming what the first answer already said. + # Read-only, like the relations it carries: Intercom exposes no endpoint that + # writes a membership, and an editable relation would offer an association + # that could only fail. + class TeamMembership < FetchAllCollection + def initialize(datasource) + super(datasource, 'IntercomTeamMembership') + end + + protected + + def fetch_all + client.fetch_all('teams', list_key: 'teams').flat_map { |team| pairs_of(team) } + end + + # Keyed by both ids rather than by a counter: a related list is read over + # two requests, and a key that changed between them would move the rows + # under the operator. + def serialize(pair) + { 'id' => "#{pair["team_id"]}:#{pair["admin_id"]}", + 'team_id' => pair['team_id'], + 'admin_id' => pair['admin_id'] } + end + + private + + def pairs_of(team) + return [] unless team.is_a?(Hash) + + team_id = stringify_id(team['id']) + return [] if team_id.nil? + + Array(team['admin_ids']).filter_map do |admin_id| + id = stringify_id(admin_id) + { 'team_id' => team_id, 'admin_id' => id } unless id.nil? + end + end + + def define_schema + add_column('id', 'String', is_primary_key: true) + add_column('team_id', 'String') + add_column('admin_id', 'String') + add_many_to_one('team', foreign_collection: 'IntercomTeam', foreign_key: 'team_id') + add_many_to_one('admin', foreign_collection: 'IntercomAdmin', foreign_key: 'admin_id') + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb index 2569e4552..f071cd37c 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket.rb @@ -85,17 +85,23 @@ def define_schema define_contact_columns define_derived_columns add_column('part_count', 'Number') + # Before the attribute columns rather than after: a workspace attribute + # whose name lands on a relation is then skipped with a warning, the way + # one landing on a column already is. Declared after, it would collide + # and take the boot with it. + define_relations register_attribute_columns end - # The state arrives embedded as a whole object, so its labels cost nothing. - # `IntercomTicketState` remains a collection of its own -- it is the list of - # what a state can be -- but a row does not depend on it to be readable. + # The state arrives embedded as a whole object, so its label costs nothing: + # a queue reads without a join. One label and not three -- the category and + # the customer-facing label are read through the `state` relation, which is + # where every field of a state lives. Neither of the two was ever + # filterable, so nothing that could be saved in a segment or a scope + # depended on them. def define_state_columns add_column('state_id', 'String') - add_column('state_category', 'String') add_column('state_label', 'String') - add_column('state_external_label', 'String') add_column('previous_state_id', 'String') end @@ -104,6 +110,23 @@ def define_type_columns add_column('ticket_type_name', 'String') end + # The four reference collections a ticket points at. Every target is read + # whole in one request, so a relation resolves for a page at the price of a + # single read. + # + # Only two of them can be filtered *through*: `/tickets/search` takes a + # filter on `admin_assignee_id`, `team_assignee_id` and `ticket_type_id`, + # and none on a state id -- which the refusal names when a filter reaches + # for it, rather than letting the interface offer what the endpoint drops. + def define_relations + add_many_to_one('admin_assignee', foreign_collection: 'IntercomAdmin', foreign_key: 'admin_assignee_id') + add_many_to_one('team_assignee', foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + add_many_to_one('state', foreign_collection: 'IntercomTicketState', foreign_key: 'state_id') + add_many_to_one('previous_state', foreign_collection: 'IntercomTicketState', + foreign_key: 'previous_state_id') + add_many_to_one('ticket_type', foreign_collection: 'IntercomTicketType', foreign_key: 'ticket_type_id') + end + # The attribute columns of every ticket type, in union. Read at boot by # `TicketAttributesIntrospector`, which is also where a workspace's own # name is turned into one a Forest query string can carry. An attribute @@ -118,8 +141,8 @@ def collides?(attribute) ForestAdminDatasourceIntercom.logger.warn( "[forest_admin_datasource_intercom] #{name} skips the ticket attribute #{attribute.name.inspect}: a " \ - "native column already carries the name #{attribute.column_name.inspect}, and overwriting it would show " \ - 'the attribute where the operator expects the ticket field.' + "native column or relation already carries the name #{attribute.column_name.inspect}, and overwriting " \ + 'it would show the attribute where the operator expects the ticket field.' ) true end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb index 3fb91030a..d547fefb0 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/ticket/serializer.rb @@ -37,10 +37,10 @@ def native(attrs) def state_of(attrs) state = attrs['ticket_state'].is_a?(Hash) ? attrs['ticket_state'] : {} + # `internal_label` is what the support team reads. The category and the + # customer-facing label are a hop away, on the `state` relation. { 'state_id' => stringify_id(state['id']), - 'state_category' => state['category'], 'state_label' => state['internal_label'], - 'state_external_label' => state['external_label'], 'previous_state_id' => stringify_id(attrs['previous_ticket_state_id']) } end diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb index f9ccf242a..c2a4c2bf0 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/datasource.rb @@ -30,6 +30,10 @@ def inspect def register_collections add_collection(Collections::Admin.new(self)) add_collection(Collections::Team.new(self)) + # The join Intercom does not expose: without it the membership of a team is + # an array of ids on either side, since a many-to-many needs a collection + # to travel through. + add_collection(Collections::TeamMembership.new(self)) add_collection(Collections::TicketType.new(self)) add_collection(Collections::TicketState.new(self)) add_collection(Collections::Conversation.new(self)) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb index a98cfd112..28071c2d0 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb @@ -9,10 +9,16 @@ def filter ForestAdminDatasourceToolkit::Components::Query::Filter.new end + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + def stub_admins(*admins) - stub_request(:get, "#{base}/admins") - .to_return(status: 200, body: { 'type' => 'admin.list', 'admins' => admins }.to_json, - headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) + end + + def stub_teams(*teams) + stub_request(:get, "#{base}/teams").to_return(json('type' => 'team.list', 'teams' => teams)) end it 'is named IntercomAdmin' do @@ -21,7 +27,7 @@ def stub_admins(*admins) it 'exposes the columns an ops lead reads before assigning anything' do expect(collection.fields.keys) - .to eq(%w[id name email job_title away_mode_enabled away_mode_reassign has_inbox_seat team_ids]) + .to eq(%w[id name email job_title away_mode_enabled away_mode_reassign has_inbox_seat team_names teams]) end it 'declares id as the primary key' do @@ -38,24 +44,65 @@ def stub_admins(*admins) expect(collection.list(nil, filter, nil)) .to eq([{ 'id' => '1', 'name' => 'Alice', 'email' => 'alice@acme.test', 'job_title' => 'Support', 'away_mode_enabled' => true, 'away_mode_reassign' => false, 'has_inbox_seat' => true, - 'team_ids' => %w[814865] }]) + 'team_names' => nil }]) end # Intercom types a team id as a number here and as a string on the team - # itself; a filter value from Forest always arrives as a string. + # itself; a filter value from Forest always arrives as a string. Both sides + # of the membership therefore carry the same id, without which the relation + # would resolve to nothing rather than to an error. it 'stringifies the ids so both sides of the membership match' do - stub_admins('id' => 493_881, 'team_ids' => [814_865, 814_866]) + stub_admins('id' => 493_881, 'team_ids' => [814_865]) + stub_teams('id' => '814865', 'name' => 'Support') - row = collection.list(nil, filter, nil).first - - expect(row['id']).to eq('493881') - expect(row['team_ids']).to eq(%w[814865 814866]) + expect(collection.list(nil, filter, %w[id team_names]).first) + .to eq({ 'id' => '493881', 'team_names' => ['Support'] }) end - it 'reads a teammate with no team as one with no team, not as one with a null' do - stub_admins('id' => '1', 'team_ids' => nil) + describe 'the teams of a teammate' do + it 'names them for the whole page in one read, rather than one read per row' do + stub_admins({ 'id' => '1', 'team_ids' => [814_865] }, { 'id' => '2', 'team_ids' => [814_865, 814_866] }) + stub_teams({ 'id' => '814865', 'name' => 'Support' }, { 'id' => '814866', 'name' => 'Billing' }) + + expect(collection.list(nil, filter, %w[id team_names])) + .to eq([{ 'id' => '1', 'team_names' => ['Support'] }, + { 'id' => '2', 'team_names' => %w[Support Billing] }]) + expect(WebMock).to have_requested(:get, "#{base}/teams").once + end + + # A column nobody asked for costs no request. + it 'reads nothing when no projection asks for the names' do + stub_admins('id' => '1', 'team_ids' => [814_865]) + + collection.list(nil, filter, %w[id name]) + + expect(WebMock).not_to have_requested(:get, "#{base}/teams") + end + + # A missing permission costs the column, never the page -- and never the + # relation, which reads the teams from the other side. + it 'leaves the names empty when the teams cannot be read' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_admins('id' => '1', 'team_ids' => [814_865]) + stub_request(:get, "#{base}/teams").to_return(json({ 'type' => 'error.list' }, 403)) + + expect(collection.list(nil, filter, %w[id team_names]).first) + .to eq({ 'id' => '1', 'team_names' => [] }) + end + + it 'reads a teammate with no team as one with no team, not as one with a null' do + stub_admins('id' => '1', 'team_ids' => nil) + stub_teams('id' => '814865', 'name' => 'Support') + + expect(collection.list(nil, filter, %w[team_names]).first['team_names']).to eq([]) + end - expect(collection.list(nil, filter, nil).first['team_ids']).to eq([]) + it 'is a many-to-many through the membership collection, read-only' do + expect(collection.fields['teams']) + .to have_attributes(type: 'ManyToMany', foreign_collection: 'IntercomTeam', + through_collection: 'IntercomTeamMembership', + origin_key: 'admin_id', foreign_key: 'team_id', is_read_only: true) + end end end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb index 3b562221b..a6cfe8db5 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/conversation_spec.rb @@ -25,6 +25,12 @@ def json(payload, status = 200) { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } end + # The columns alone: a relation carries neither operators nor an order, and + # what it may be filtered through is asserted on its own below. + def columns + collection.fields.select { |_, field| field.type == 'Column' } + end + # Hand-written from the OpenAPI 2.16 spec, never captured from a workspace: # a conversation body is personal data. def conversation(id, overrides = {}) @@ -98,7 +104,7 @@ def ids(rows) # Neither search endpoint takes a sort, and Intercom ignores the one it is # sent without a word, so no column of this tier may advertise one. it 'declares every column unsortable' do - expect(collection.fields.values.map(&:is_sortable).uniq).to eq([false]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end # Derived from the measured table, never written by hand: a column @@ -135,7 +141,7 @@ def ids(rows) # No aggregate endpoint, so no group-by may be offered. it 'declares no column groupable' do - expect(collection.fields.values.map(&:is_groupable).uniq).to eq([false]) + expect(columns.values.map(&:is_groupable).uniq).to eq([false]) end # This lot writes nothing: an editable column would offer a Save that @@ -145,6 +151,54 @@ def ids(rows) end end + # All three targets are read whole in one request, and + # `/conversations/search` takes a filter on each of the three keys -- so + # these relations can be read, navigated and filtered through alike, unlike + # the state of a ticket. + describe 'the relations to the reference collections' do + it 'points every id at the collection that reads it' do + expect(collection.fields['admin_assignee']) + .to have_attributes(type: 'ManyToOne', foreign_collection: 'IntercomAdmin', + foreign_key: 'admin_assignee_id', foreign_key_target: 'id', is_read_only: true) + expect(collection.fields['team_assignee']) + .to have_attributes(foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + expect(collection.fields['closed_by']) + .to have_attributes(foreign_collection: 'IntercomAdmin', foreign_key: 'closed_by_id') + end + + # The account is on the payload as a whole object, so the name is already a + # column; the Companies collection arrives with lot 4, and a relation whose + # target is missing is a schema the agent refuses to boot on. + it 'declares no relation towards the account' do + expect(collection.fields.keys).not_to include('company') + end + + it 'nests the teammate who closed it, reading the teammates once for the page' do + stub_list(conversation('1'), conversation('2')) + stub_request(:get, "#{base}/admins") + .to_return(json('type' => 'admin.list', 'admins' => [{ 'id' => '493881', 'name' => 'Alice' }])) + + rows = collection.list(nil, filter, ['id', 'closed_by:name']) + + expect(rows).to eq([{ 'id' => '1', 'closed_by' => { 'name' => 'Alice', 'id' => '493881' } }, + { 'id' => '2', 'closed_by' => { 'name' => 'Alice', 'id' => '493881' } }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + it 'filters on the foreign key the target resolved to' do + stub_request(:get, "#{base}/admins") + .to_return(json('type' => 'admin.list', 'admins' => [{ 'id' => '493881', 'name' => 'Alice' }])) + stub_search(conversation('1')) + + collection.list(nil, filter(condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/conversations/search") + .with(query: hash_including({}), + body: hash_including('query' => { 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493881' })) + end + end + describe '#list' do it 'reads the listing endpoint as plain text and pages by cursor' do stub_list(conversation('1')) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb index 09b5cefc0..e456f5147 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb @@ -74,14 +74,14 @@ def evaluable?(operator, column_type) # A list has no in-memory counterpart for any of the three. it 'declares a Json column neither filterable nor sortable' do - expect(collection.fields['team_ids']) + expect(collection.fields['team_names']) .to have_attributes(column_type: 'Json', is_sortable: false, is_groupable: false, filter_operators: []) end # A filter the UI offers and the collection then answers by emptying the # page is the failure this whole datasource is built to avoid. it 'advertises only operators it can actually evaluate' do - advertised = collection.fields.flat_map do |_name, column| + advertised = collection.fields.select { |_, field| field.type == 'Column' }.flat_map do |_name, column| column.filter_operators.map { |operator| [operator, column.column_type] } end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb new file mode 100644 index 000000000..656301c2a --- /dev/null +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_membership_spec.rb @@ -0,0 +1,206 @@ +module ForestAdminDatasourceIntercom + RSpec.describe Collections::TeamMembership do + subject(:collection) { datasource.get_collection('IntercomTeamMembership') } + + let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } + let(:base) { datasource.configuration.url } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + def filter(condition_tree: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree) + 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 json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + def stub_teams(*teams) + stub_request(:get, "#{base}/teams").to_return(json('type' => 'team.list', 'teams' => teams)) + end + + def stub_admins(*admins) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) + end + + it 'is named IntercomTeamMembership' do + expect(collection.name).to eq('IntercomTeamMembership') + end + + it 'exposes the pair and a relation to either side of it' do + expect(collection.fields.keys).to eq(%w[id team_id admin_id team admin]) + end + + # One record per pair, keyed by both ids: a related list is read over two + # requests, and a key that changed in between would move the rows under the + # operator. + it 'reads one record per pair from the team side alone' do + stub_teams({ 'id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881, 493_882] }, + { 'id' => '814866', 'name' => 'Billing', 'admin_ids' => [493_882] }) + + expect(collection.list(nil, filter, nil)) + .to eq([{ 'id' => '814865:493881', 'team_id' => '814865', 'admin_id' => '493881' }, + { 'id' => '814865:493882', 'team_id' => '814865', 'admin_id' => '493882' }, + { 'id' => '814866:493882', 'team_id' => '814866', 'admin_id' => '493882' }]) + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + + it 'holds no record for a team nobody belongs to' do + stub_teams({ 'id' => '814865', 'admin_ids' => [] }, { 'id' => '814866', 'admin_ids' => nil }) + + expect(collection.list(nil, filter, nil)).to be_empty + end + + # Intercom types the id as a number inside `admin_ids` and as a string on the + # teammate itself: the pair carries the form the other side answers by, or + # the relation resolves to nothing. + it 'stringifies both ids' do + stub_teams('id' => 814_865, 'admin_ids' => [493_881]) + + expect(collection.list(nil, filter, nil).first) + .to eq({ 'id' => '814865:493881', 'team_id' => '814865', 'admin_id' => '493881' }) + end + + describe 'a projection through a relation' do + it 'nests the teammate under the relation, reading them once for the page' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + + expect(collection.list(nil, filter, ['id', 'admin:name'])) + .to eq([{ 'id' => '814865:493881', 'admin' => { 'name' => 'Alice', 'id' => '493881' } }, + { 'id' => '814865:493882', 'admin' => { 'name' => 'Bruno', 'id' => '493882' } }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + # A teammate who left the workspace is still named by the membership until + # Intercom drops the pair. The row reads as having no teammate rather than + # as a broken record. + it 'nests nothing when the id names no record' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493882', 'name' => 'Bruno') + + expect(collection.list(nil, filter, ['id', 'admin:name']).first) + .to eq({ 'id' => '814865:493881', 'admin' => nil }) + end + + it 'reads no relation the projection did not name' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + collection.list(nil, filter, %w[id team_id]) + + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + end + + # This tier filters in memory, over every record Intercom holds, so a + # condition through a relation becomes a plain membership on the foreign key + # -- none of the limits of Intercom's search DSL apply here, nothing going + # through it. + describe 'a condition through a relation' do + it 'keeps the pairs whose teammate the target matched' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + + rows = collection.list(nil, filter(condition_tree: leaf('admin:name', operators::EQUAL, 'Alice')), %w[id]) + + expect(rows).to eq([{ 'id' => '814865:493881' }]) + end + + it 'answers nothing when the target matched no record, rather than everything' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + + rows = collection.list(nil, filter(condition_tree: leaf('admin:name', operators::EQUAL, 'Zoe')), %w[id]) + + expect(rows).to be_empty + end + + it 'counts the pairs a relation condition keeps, exactly' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count') + + rows = collection.aggregate(nil, filter(condition_tree: leaf('admin:name', operators::EQUAL, 'Alice')), + aggregation) + + expect(rows).to eq([{ 'group' => {}, 'value' => 1 }]) + end + + # The shape the agent really builds: a scope, then a segment, then the + # operator's own filter, one branch at a time. + it 'keeps a relation condition standing next to a condition on its own column' do + stub_teams({ 'id' => '814865', 'admin_ids' => [493_881] }, { 'id' => '814866', 'admin_ids' => [493_881] }) + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('And', leaf('team_id', operators::EQUAL, '814866'), + leaf('admin:name', operators::EQUAL, 'Alice')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])) + .to eq([{ 'id' => '814866:493881' }]) + end + + # An `and` carrying a condition nothing can satisfy matches nothing itself. + it 'answers nothing when a relation condition inside an and matches nothing' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('And', leaf('team_id', operators::EQUAL, '814865'), + leaf('admin:name', operators::EQUAL, 'Zoe')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to be_empty + end + + # An `or` drops it and keeps its siblings: what the others name is still + # named. + it 'keeps the siblings of a relation condition inside an or' do + stub_teams({ 'id' => '814865', 'admin_ids' => [493_881] }, { 'id' => '814866', 'admin_ids' => [493_882] }) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + tree = branch('Or', leaf('admin:name', operators::EQUAL, 'Zoe'), + leaf('team_id', operators::EQUAL, '814866')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])) + .to eq([{ 'id' => '814866:493882' }]) + end + + it 'answers nothing when every branch of an or matches nothing' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('Or', leaf('admin:name', operators::EQUAL, 'Zoe'), + leaf('admin:name', operators::EQUAL, 'Yann')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to be_empty + end + + # The operators a relation condition may carry are the target's own: it is + # the one that evaluates them, and the one whose refusal is worth reading. + it 'refuses an operator the target cannot evaluate, naming it' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + + expect { collection.list(nil, filter(condition_tree: leaf('admin:team_names', operators::EQUAL, 'x')), nil) } + .to raise_error(UnsupportedOperatorError, /IntercomAdmin cannot filter 'team_names'/) + end + + it 'refuses a relation it does not declare, naming the ones it has' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + expect { collection.list(nil, filter(condition_tree: leaf('owner:name', operators::EQUAL, 'x')), nil) } + .to raise_error(UnsupportedOperatorError, /"owner" is not a relation it filters through.*team, admin/m) + end + + it 'refuses a path reaching through two relations' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + expect { collection.list(nil, filter(condition_tree: leaf('admin:teams:name', operators::EQUAL, 'x')), nil) } + .to raise_error(UnsupportedOperatorError, /reaches through two relations/) + end + end + end +end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb index 91f52dce4..af1ed6d5a 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb @@ -1,41 +1,128 @@ module ForestAdminDatasourceIntercom RSpec.describe Collections::Team do - subject(:collection) { described_class.new(datasource) } + subject(:collection) { datasource.get_collection('IntercomTeam') } let(:datasource) { Datasource.new(access_token: 's3cr3t', rate_limiter: nil) } let(:base) { datasource.configuration.url } - def filter - ForestAdminDatasourceToolkit::Components::Query::Filter.new + def filter(condition_tree: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree) + end + + def admins_named(name) + operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('admins:name', operators::EQUAL, name) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } end def stub_teams(*teams) - stub_request(:get, "#{base}/teams") - .to_return(status: 200, body: { 'type' => 'team.list', 'teams' => teams }.to_json, - headers: { 'Content-Type' => 'application/json' }) + stub_request(:get, "#{base}/teams").to_return(json('type' => 'team.list', 'teams' => teams)) end - it 'is named IntercomTeam' do - expect(collection.name).to eq('IntercomTeam') + def stub_admins(*admins) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) end - it 'exposes the team and its membership' do - expect(collection.fields.keys).to eq(%w[id name admin_ids]) + it 'is named IntercomTeam' do + expect(collection.name).to eq('IntercomTeam') end - # Intercom carries the membership on the team and on the admin both. Left as - # a plain list it stays readable on either side; declared as a relation it - # would give the schema two halves of a many-to-many with no join collection. - it 'keeps the membership a list rather than a relation' do - expect(collection.fields['admin_ids']) - .to have_attributes(type: 'Column', column_type: 'Json', filter_operators: []) + it 'exposes the team, its teammates by name, and the relation to them' do + expect(collection.fields.keys).to eq(%w[id name admin_names admins]) end it 'reads the endpoint under its own key and stringifies the ids' do stub_teams('type' => 'team', 'id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) - expect(collection.list(nil, filter, nil)) - .to eq([{ 'id' => '814865', 'name' => 'Support', 'admin_ids' => %w[493881] }]) + expect(collection.list(nil, filter, %w[id name])).to eq([{ 'id' => '814865', 'name' => 'Support' }]) + end + + describe 'the teammates of a team' do + it 'names them for the whole page in one read, rather than one read per row' do + stub_teams({ 'id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881, 493_882] }, + { 'id' => '814866', 'name' => 'Billing', 'admin_ids' => [493_882] }) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + + expect(collection.list(nil, filter, %w[id admin_names])) + .to eq([{ 'id' => '814865', 'admin_names' => %w[Alice Bruno] }, + { 'id' => '814866', 'admin_names' => %w[Bruno] }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + it 'reads nothing when no projection asks for the names' do + stub_teams('id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) + + collection.list(nil, filter, %w[id name]) + + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + + # A missing permission costs the column, never the page -- and never the + # relation, which reads the teammates from the other side. + it 'leaves the names empty when the teammates cannot be read' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + stub_request(:get, "#{base}/admins").to_return(json({ 'type' => 'error.list' }, 403)) + + expect(collection.list(nil, filter, %w[id admin_names]).first) + .to eq({ 'id' => '814865', 'admin_names' => [] }) + end + end + + # Intercom carries the membership on the team and on the teammate both and + # exposes no resource for the pair, so the many-to-many travels through the + # membership collection this datasource synthesizes. + describe 'the relation to the teammates' do + it 'is a many-to-many through the membership collection' do + expect(collection.fields['admins']) + .to have_attributes(type: 'ManyToMany', foreign_collection: 'IntercomAdmin', + through_collection: 'IntercomTeamMembership', + origin_key: 'team_id', foreign_key: 'admin_id') + end + + # Intercom exposes no endpoint that writes a membership, and an editable + # relation would offer an association that could only fail. + it 'is read-only' do + expect(collection.fields['admins'].is_read_only).to be(true) + end + + # What the toolkit looks for on the collection a many-to-many travels + # through. Without the two, a related list falls back to a filter on the + # foreign collection that nothing there can answer. + it 'is reachable from both ends of the membership' do + utils = ForestAdminDatasourceToolkit::Utils::Collection + + expect(utils.get_through_target(collection, 'admins')).to eq('admin') + expect(utils.get_through_origin(collection, 'admins')).to eq('team') + end + + # A many-to-many is published unfilterable, so nothing the interface offers + # reaches here -- a scope or a segment still can, and it is refused rather + # than resolved by reading the membership once per value. + it 'refuses a condition through the membership' do + stub_teams('id' => '814865', 'admin_ids' => [493_881]) + + expect { collection.list(nil, filter(condition_tree: admins_named('Alice')), nil) } + .to raise_error(UnsupportedOperatorError, /"admins" is not a relation it filters through. It has none/) + end + + # The path a related list really takes: through the membership, whose own + # relation towards the teammate is what carries the rows back. + it 'lists the teammates of one team, through the membership' do + stub_teams({ 'id' => '814865', 'admin_ids' => [493_881] }, { 'id' => '814866', 'admin_ids' => [493_882] }) + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Bruno' }) + query = ForestAdminDatasourceToolkit::Components::Query + + rows = ForestAdminDatasourceToolkit::Utils::Collection.list_relation( + collection, %w[814865], 'admins', nil, query::Filter.new, query::Projection.new(%w[id name]) + ) + + expect(rows).to eq([{ 'id' => '493881', 'name' => 'Alice' }]) + end end end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb index bf1b8fddb..328c9d2cb 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb @@ -31,6 +31,11 @@ def leaf(field, operator, value = nil) .new(field, operator, value) end + def branch(aggregator, *conditions) + ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeBranch + .new(aggregator, conditions) + end + # Hand-written from the shape measured on a real workspace: the state comes # embedded, the company as a bare id, and the parts ride along. def ticket(id, overrides = {}) @@ -64,6 +69,10 @@ def comment(at:, by: 'Alice', type: 'admin', part_type: 'comment') 'created_at' => at, 'author' => { 'type' => type, 'id' => '1', 'name' => by } } end + def stub_admins(*admins) + stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) + end + def stub_search(*records, total: nil, body: nil) answer = { 'type' => 'ticket.list', 'tickets' => records, 'total_count' => total || records.size, 'pages' => { 'type' => 'pages', 'page' => 1 } } @@ -77,16 +86,24 @@ def rows(projection = nil, **options) collection.list(nil, filter(**options), projection) end + # The columns alone: a relation carries neither operators nor an order. + def columns + collection.fields.select { |_, field| field.type == 'Column' } + end + describe 'schema' do it 'is named IntercomTicket' do expect(collection.name).to eq('IntercomTicket') end - # The state travels embedded, so its labels cost nothing and the row does - # not depend on IntercomTicketState to be readable. - it 'flattens the embedded state into its labels' do - expect(collection.fields.keys) - .to include('state_id', 'state_category', 'state_label', 'state_external_label', 'previous_state_id') + # The state travels embedded, so its label costs nothing and the row does + # not depend on IntercomTicketState to be readable. One label and not + # three: the category and the customer-facing label are a hop away, on the + # relation, and neither was ever filterable -- so no segment or scope could + # rest on them. + it 'flattens the embedded state into one label, the rest being a hop away' do + expect(collection.fields.keys).to include('state_id', 'state_label', 'previous_state_id') + expect(collection.fields.keys).not_to include('state_category', 'state_external_label') end it 'carries the attributes of every ticket type in union' do @@ -96,7 +113,7 @@ def rows(projection = nil, **options) # `/tickets/search` ignores a sort without saying so, on every column. it 'declares every column unsortable' do - expect(collection.fields.values.map(&:is_sortable).uniq).to eq([false]) + expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end it 'advertises the filters the search endpoint answers, and only those' do @@ -180,7 +197,7 @@ def rows(projection = nil, **options) expect(rows.first) .to include('id' => '1', 'ticket_id' => '11', 'category' => 'request', 'open' => true, - 'state_id' => '19', 'state_category' => 'in_progress', 'state_label' => 'En cours Tech', + 'state_id' => '19', 'state_label' => 'En cours Tech', 'previous_state_id' => '14', 'ticket_type_name' => 'Bug', 'company_id' => '696dd52099f73812610d9c7b', 'admin_assignee_id' => '493881') end @@ -265,6 +282,176 @@ def rows(projection = nil, **options) end end + # Every target is read whole in one request, so a relation resolves for a + # whole page at the price of a single read -- which is what makes eight of + # them affordable at all. + describe 'the relations to the reference collections' do + it 'points every id at the collection that reads it' do + expect(collection.fields['admin_assignee']) + .to have_attributes(type: 'ManyToOne', foreign_collection: 'IntercomAdmin', + foreign_key: 'admin_assignee_id', foreign_key_target: 'id', is_read_only: true) + expect(collection.fields['team_assignee']) + .to have_attributes(foreign_collection: 'IntercomTeam', foreign_key: 'team_assignee_id') + expect(collection.fields['state']) + .to have_attributes(foreign_collection: 'IntercomTicketState', foreign_key: 'state_id') + expect(collection.fields['previous_state']) + .to have_attributes(foreign_collection: 'IntercomTicketState', foreign_key: 'previous_state_id') + expect(collection.fields['ticket_type']) + .to have_attributes(foreign_collection: 'IntercomTicketType', foreign_key: 'ticket_type_id') + end + + # Declared before the attribute columns, so a workspace attribute whose + # name lands on a relation is skipped with a warning the way one landing on + # a column is. Declared after, it would collide and take the boot with it. + it 'keeps the relation when a ticket attribute carries its name' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + + collection = described_class.new(datasource, attributes: [attribute('state')]) + + expect(collection.fields['state'].type).to eq('ManyToOne') + end + + it 'nests the assignee under the relation, reading the teammates once for the page' do + stub_search(ticket('1'), ticket('2')) + stub_admins('id' => '493881', 'name' => 'Alice', 'email' => 'alice@acme.test') + + rows = collection.list(nil, filter, ['id', 'admin_assignee:name']) + + expect(rows).to eq([{ 'id' => '1', 'admin_assignee' => { 'name' => 'Alice', 'id' => '493881' } }, + { 'id' => '2', 'admin_assignee' => { 'name' => 'Alice', 'id' => '493881' } }]) + expect(WebMock).to have_requested(:get, "#{base}/admins").once + end + + # A teammate who left the workspace: the row reads as unassigned rather + # than as a page that could not be served. + it 'nests nothing when the id names no teammate' do + stub_search(ticket('1')) + stub_admins('id' => '493882', 'name' => 'Bruno') + + expect(collection.list(nil, filter, ['id', 'admin_assignee:name']).first) + .to eq({ 'id' => '1', 'admin_assignee' => nil }) + end + + it 'reads no relation the projection did not name' do + stub_search(ticket('1')) + + collection.list(nil, filter, %w[id state_label]) + + expect(WebMock).not_to have_requested(:get, "#{base}/admins") + end + end + + # A relation is published filterable as soon as any column of its target is, + # so the interface offers `admin_assignee:name` the moment the relation + # exists. What Intercom is really filtered on is the foreign key: the target + # says which of its records match -- over every record it holds, not over a + # page -- and the ids it names are what the search carries. + describe 'a condition through a relation' do + it 'filters on the foreign key the target resolved to' do + stub_admins('id' => '493881', 'name' => 'Alice') + stub_search(ticket('1')) + + rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493881' })) + end + + # Intercom takes no membership operator on these fields, so several matches + # become several conditions -- which is also why the group has a ceiling. + it 'writes a group of equalities when the target matched several records' do + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Alice' }) + stub_search(ticket('1')) + + rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { + 'operator' => 'OR', + 'value' => [{ 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493881' }, + { 'field' => 'admin_assignee_id', 'operator' => '=', + 'value' => '493882' }] + })) + end + + # No row can satisfy it, and Intercom's DSL has no way of saying so: the + # search is skipped rather than sent as a filter that would come back with + # every ticket. + it 'answers nothing, and reads nothing, when the target matched no record' do + stub_admins('id' => '493881', 'name' => 'Alice') + + expect(rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Zoe'))).to be_empty + expect(WebMock).not_to have_requested(:post, "#{base}/tickets/search") + end + + it 'counts none of them either, without a request' do + stub_admins('id' => '493881', 'name' => 'Alice') + aggregation = ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count') + + counted = collection.aggregate(nil, filter(condition_tree: leaf('admin_assignee:name', + operators::EQUAL, 'Zoe')), aggregation) + + expect(counted).to eq([{ 'group' => {}, 'value' => 0 }]) + expect(WebMock).not_to have_requested(:post, "#{base}/tickets/search") + end + + # A relation group nested inside the tree the agent assembled: a scope, a + # segment and the operator's own filter, and the group counts as one of + # them. + it 'nests the group inside the condition it was written next to' do + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Alice' }) + stub_search(ticket('1')) + tree = branch('And', leaf('category', operators::EQUAL, 'request'), + leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { + 'operator' => 'AND', + 'value' => [{ 'field' => 'category', 'operator' => '=', + 'value' => 'request' }, + { 'operator' => 'OR', + 'value' => [{ 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493881' }, + { 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493882' }] }] + })) + end + + it 'reads nothing when a relation condition inside an and matches nothing' do + stub_admins('id' => '493881', 'name' => 'Alice') + tree = branch('And', leaf('category', operators::EQUAL, 'request'), + leaf('admin_assignee:name', operators::EQUAL, 'Zoe')) + + expect(collection.list(nil, filter(condition_tree: tree), %w[id])).to be_empty + expect(WebMock).not_to have_requested(:post, "#{base}/tickets/search") + end + + # Fifteen conditions per group is Intercom's limit, and a relation reaches + # it without trying. Refused by name rather than sent and answered with a + # 400 naming neither the limit nor the filter that hit it. + it 'refuses a relation condition matching more records than a group holds' do + stub_admins(*(1..16).map { |index| { 'id' => index.to_s, 'name' => 'Alice' } }) + + expect { rows(%w[id], condition_tree: leaf('admin_assignee:name', operators::EQUAL, 'Alice')) } + .to raise_error(UnsupportedOperatorError, /names 16 records.*15 conditions per group/m) + end + + # `/tickets/search` filters no state id -- the table carries none -- so the + # state relation is there to be read and navigated, and the message says + # which of the two it is rather than naming a column the operator never + # wrote. Refused before the target is read, a refusal that spends a request + # costing exactly what it refuses. + it 'refuses a condition through a relation the endpoint filters nothing on' do + expect { rows(%w[id], condition_tree: leaf('state:category', operators::EQUAL, 'in_progress')) } + .to raise_error(UnsupportedOperatorError, %r{resolves to "state_id", on which tickets/search takes no}) + expect(WebMock).not_to have_requested(:get, /ticket_states/) + end + end + describe '#aggregate' do it 'counts a filtered collection through the total_count of its search' do stub_search(total: 12, body: { 'query' => { 'field' => 'open', 'operator' => '=', 'value' => true } }) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb index c5387e059..91ad2f84f 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/datasource_spec.rb @@ -8,11 +8,12 @@ module ForestAdminDatasourceIntercom # The reference collections come first: they are what turns an assignee id # into a teammate and a state id into a label. Conversations follow, Tickets - # next. + # next. The membership sits with the two collections it joins: a many-to-many + # needs a collection to travel through, and Intercom exposes none. it 'publishes the collections of the lot' do expect(datasource.collections.keys) - .to eq(%w[IntercomAdmin IntercomTeam IntercomTicketType IntercomTicketState IntercomConversation - IntercomTicket]) + .to eq(%w[IntercomAdmin IntercomTeam IntercomTeamMembership IntercomTicketType IntercomTicketState + IntercomConversation IntercomTicket]) end # The one read a boot performs: the attributes a workspace declares on its From 2db9b2b3aebddda7fa46796f1b576785b5680f79 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 4 Sep 2026 18:33:25 +0200 Subject: [PATCH 2/3] fix(datasource): intercom relation reads and group nesting Review of the relations lot, six items. A relation group is now told apart from a group the operator wrote, and inlined into a parent that aggregates the same way: the level it added was one nothing budgeted against Intercom's two, so a scope plus a "match any" filter carrying one relation condition was refused for a nesting the operator could not find in their own filter. Inlining trades depth for width and Intercom bounds both, so past fifteen conditions the nested form is kept. Where a group is still refused for depth, the message names the relation that expanded into it rather than asking to flatten a nesting nobody wrote. Relations are read one request per target collection instead of one per relation: a ticket's state and previous_state are one read of /ticket_states, over the ids both of them name, and each relation is still nested with the columns it asked for rather than the union. An enrichment guarded on a column being named left that column nil under a projection naming nothing, which project reads as every declared column: admin_names and team_names came back nil on the one listing that publishes them. get_field_schema raised a bare ForestException on the three field names a request can get wrong, which the error translator maps to a 500. They are ValidationErrors now, so a condition reaching through a relation the toolkit cannot traverse -- a scope written on a many-to-many, say -- is the 400 it always was in intent. README: the permission note separated the enrichment, which is guarded, from the relation read, which is not; the cost of a relation stated per target collection; the page cap that bounds "read whole"; and the inlining above. Co-Authored-By: Claude Opus 5 (1M context) --- .../README.md | 30 ++++- .../collections/admin.rb | 2 +- .../collections/base_collection.rb | 10 ++ .../collections/cursor_collection.rb | 13 ++- .../collections/relations.rb | 104 ++++++++++++++---- .../collections/team.rb | 2 +- .../query/condition_tree_translator.rb | 24 +++- .../collections/admin_spec.rb | 13 ++- .../collections/fetch_all_collection_spec.rb | 8 ++ .../collections/team_spec.rb | 7 ++ .../collections/ticket_spec.rb | 81 ++++++++++++++ .../query/condition_tree_translator_spec.rb | 16 ++- .../utils/collection.rb | 12 +- .../utils/collection_spec.rb | 6 +- 14 files changed, 290 insertions(+), 38 deletions(-) diff --git a/packages/forest_admin_datasource_intercom/README.md b/packages/forest_admin_datasource_intercom/README.md index ee5d176d7..40781f30b 100644 --- a/packages/forest_admin_datasource_intercom/README.md +++ b/packages/forest_admin_datasource_intercom/README.md @@ -61,8 +61,14 @@ beats not running. A read-only token is enough, and is what to recommend for this lot. A permission the token lacks costs **columns or a collection, never the boot of the agent**: the ticket-type introspection degrades to no attribute column, a collection whose endpoint answers 403 fails its own page, and a -token that cannot read `/admins` or `/teams` leaves the membership names empty without touching the -relation, which reads them from the other side. +token that cannot read `/admins` or `/teams` leaves the `admin_names` / `team_names` column empty +rather than failing the page it is on. + +A **relation is the exception**, and it is worth knowing before scoping a token: resolving one reads +the target endpoint, and that read is not guarded the way the names above are. A token denied +`/admins` fails any page projecting `admin_assignee:name`, and fails the related list behind +`IntercomTeam#admins` — the failure lands on the collection being read, not on the one that was +denied. Scope the token to the endpoints in the table below, or to none of them. ## Collections @@ -94,7 +100,14 @@ Intercom's search DSL and walked through the search endpoint. What the translati Intercom joins nothing: a ticket carries an assignee id, and the teammate behind it is a second read of a second endpoint. What makes eight relations affordable is that every collection on the far end is read whole in one request — so a relation resolves for a **whole page at the price of one read**, -never one read per row, and it resolves *exactly*. +never one read per row. The price is per target *collection*, not per relation: a ticket's `state` +and `previous_state` are one read of `/ticket_states`, over the ids both of them name. + +*Exactly*, with one bound worth naming: "read whole" is what the endpoint answers, and `fetch_all` +stops after [`MAX_COLLECTED_PAGES`](lib/forest_admin_datasource_intercom/client.rb) pages if Intercom +paginates one of these on its own — it logs when it does. A workspace whose `/admins` or `/teams` +runs past that cap resolves the relations pointing at the records it dropped as empty. The figure is +sized for reference collections, which is what every target here is. | Collection | Relation | Target | Filterable through | | --- | --- | --- | --- | @@ -277,7 +290,11 @@ That is exact, and it has three visible edges: - Intercom takes no membership operator on these fields, so several matches become **one equality per match**, inside an `OR` — which counts against the fifteen conditions a group allows. A relation condition matching more records than that is refused by name rather than sent and answered with a - 400 naming neither the limit nor the filter that hit it. + 400 naming neither the limit nor the filter that hit it. That `OR` is **inlined into a parent that + aggregates the same way**, so it costs no level of nesting where it does not have to: the two + levels Intercom allows are spent on the filter that was written, not on the expansion of a + relation. Where inlining it would take the parent past fifteen conditions it stays nested, width + being the scarcer of the two. - A condition the target matched **no record** with names no row, and the DSL cannot say so: the search is skipped entirely rather than sent as a filter that would come back with everything. - A relation whose foreign key the endpoint does not filter — the ticket `state` — is refused with a @@ -288,6 +305,11 @@ That is exact, and it has three visible edges: On the collections read whole the same condition costs nothing: they filter in memory, so the ids go in as a plain membership and none of the DSL's limits apply. +A **many-to-many is published unfilterable** — `admins` and `teams` — and a condition written on one +anyway, in a scope or a segment, is refused before it reaches this datasource: the agent's own +validator answers a 400 naming the field and its type. Filter on a column of the collection next +door instead. + ### The limits of a search, checked before the request leaves Intercom nests a search **two levels** deep and takes **fifteen conditions per group**. Past either diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb index 5814b8a4a..9cba4a2d3 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/admin.rb @@ -35,7 +35,7 @@ def serialize(admin) # asked for them: one read of `/teams` for the whole page. A token that # cannot read them costs the column and nothing else. def enrich(records, rows, projection) - return unless Array(projection).map(&:to_s).include?('team_names') + return unless column_asked?(projection, 'team_names') names = team_names records.each_with_index do |record, index| diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb index 2ed15a3c6..30d11d7e7 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/base_collection.rb @@ -51,6 +51,16 @@ def project(record, projection) asked.reject { |field| field.include?(':') }.to_h { |field| [field, record[field]] } end + # Whether a projection asks for a column. No projection at all asks for + # every declared column, which is how `project` reads it -- an enrichment + # guarded on the column being named would leave nil the very column the + # projection publishes. + def column_asked?(projection, column) + asked = Array(projection).map(&:to_s) + + asked.empty? || asked.include?(column) + end + # The window a list view asked for, cut out of records already in hand. # # A filter with no page -- or a page naming no limit -- asks for every diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb index 52a56853b..62545008f 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/cursor_collection.rb @@ -188,11 +188,22 @@ def translate(caller, filter) # relation condition matching more records than that is refused rather than # sent -- and refused here, where the message can name the relation the # operator filtered on rather than the key it resolved to. + # + # It also costs a level of nesting, which is why it is built as the group + # `Relations` can tell from one the operator wrote: inlined into a parent + # that aggregates the same way, it costs none. def relation_group(key, ids, leaf) refuse_fan_out!(leaf, key, ids) if ids.size > Query::ConditionTreeTranslator::MAX_GROUP_SIZE return Leaf.new(key, Operators::EQUAL, ids.first) if ids.size == 1 - Branch.new('Or', ids.map { |id| Leaf.new(key, Operators::EQUAL, id) }) + RelationBranch.new('Or', ids.map { |id| Leaf.new(key, Operators::EQUAL, id) }, leaf.field) + end + + # Inlining a relation group into its parent trades a level of nesting for + # width, and Intercom bounds both. Past the conditions a group may hold, + # the nested form is the one it answers. + def absorb_relation_group?(size) + size <= Query::ConditionTreeTranslator::MAX_GROUP_SIZE end # A relation this endpoint filters nothing through. It is navigable all the diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb index 51c6f76f7..bcb7dc03b 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/relations.rb @@ -25,7 +25,7 @@ module Collections # plainly: a row whose foreign key is null matches no relation filter, the # way a join drops it, negated filters included. A ticket with no assignee is # not "assigned to someone other than Marie". - module Relations + module Relations # rubocop:disable Metrics/ModuleLength ManyToOneSchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema ManyToManySchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema Filter = ForestAdminDatasourceToolkit::Components::Query::Filter @@ -40,6 +40,20 @@ module Relations # says it, while Intercom's search DSL has no way to. NOTHING = :matches_nothing + # The group a relation condition expands into, told apart from a group the + # operator wrote. Two things hang on the difference: this one may be + # inlined into a parent aggregating the same way -- the level it adds is + # one nobody budgeted for -- and a tier refusing it can name the relation + # rather than a shape the operator never wrote. + class RelationBranch < Branch + attr_reader :relation_field + + def initialize(aggregator, conditions, relation_field) + @relation_field = relation_field + super(aggregator, conditions) + end + end + protected # Every relation of this package points at a collection keyed by `id` and @@ -65,19 +79,27 @@ def add_many_to_many(name, foreign_collection:, through_collection:, origin_key: # foreign keys are read, since a projection naming `assignee:name` does not # have to name `admin_assignee_id`. # - # One request per relation per page, never one per row. + # One request per target *collection* per page -- never one per row, and + # never twice for two relations that point at the same collection: a + # ticket's `state` and `previous_state` are one read of `/ticket_states`, + # over the ids both of them name. def embed_relations(caller, records, rows, projection) - relations_asked(projection).each do |name, sub_projection| - relation = fields[name] - next unless relation.is_a?(ManyToOneSchema) + asked = many_to_one_asked(projection) + return if asked.empty? - key = relation.foreign_key - ids = records.filter_map { |record| record[key] }.uniq - targets = indexed_targets(caller, relation, ids, sub_projection) + indexed = indexed_targets(caller, records, asked) + + asked.each do |name, relation, wanted| + rows_of_target = indexed[relation.foreign_collection] # Nil rather than absent when an id names no record: a teammate who - # left the workspace reads as no teammate, not as a broken row. - records.each_with_index { |record, index| rows[index][name] = targets[record[key]] } + # left the workspace reads as no teammate, not as a broken row. Sliced + # back to what this relation asked for, the read having been widened to + # the union of what every relation on that collection did. + records.each_with_index do |record, index| + target_row = rows_of_target[record[relation.foreign_key]] + rows[index][name] = target_row&.slice(*wanted) + end end end @@ -107,18 +129,36 @@ def relations_asked(projection) Projection.new(Array(projection).map(&:to_s)).relations end - # The target's own key travels with the projection whether or not it was - # asked for: it is what the rows are indexed by here, and what makes the - # nested row a link rather than a label in the interface. - def indexed_targets(caller, relation, ids, sub_projection) + # The many-to-one relations the projection named, each with the columns it + # asked of its target. The target's own key travels with them whether or + # not it was asked for: it is what the rows are indexed by here, and what + # makes the nested row a link rather than a label in the interface. + def many_to_one_asked(projection) + relations_asked(projection).filter_map do |name, sub_projection| + relation = fields[name] + next unless relation.is_a?(ManyToOneSchema) + + [name, relation, Array(sub_projection).map(&:to_s).union([relation.foreign_key_target])] + end + end + + def indexed_targets(caller, records, asked) + asked.group_by { |_, relation, _| relation.foreign_collection } + .transform_values { |group| target_rows(caller, records, group) } + end + + # One read per target collection, over the ids every relation pointing at + # it names and the union of the columns they asked for. + def target_rows(caller, records, group) + relation = group.first[1] + ids = group.flat_map { |_, rel, _| records.filter_map { |record| record[rel.foreign_key] } }.uniq return {} if ids.empty? target = relation.foreign_key_target - wanted = Projection.new(Array(sub_projection).map(&:to_s).union([target])) + wanted = Projection.new(group.flat_map { |_, _, columns| columns }.uniq) filter = Filter.new(condition_tree: Leaf.new(target, Operators::IN, ids)) - foreign_collection(relation).list(caller, filter, wanted) - .to_h { |row| [row[target], row] } + foreign_collection(relation).list(caller, filter, wanted).to_h { |row| [row[target], row] } end def rewrite_branch(caller, branch, &builder) @@ -126,14 +166,40 @@ def rewrite_branch(caller, branch, &builder) if branch.aggregator.to_s.casecmp('or').zero? kept = rewritten.reject { |node| node == NOTHING } - kept.empty? ? NOTHING : Branch.new(branch.aggregator, kept) + kept.empty? ? NOTHING : Branch.new(branch.aggregator, absorb_groups(branch.aggregator, kept)) elsif rewritten.include?(NOTHING) NOTHING else - Branch.new(branch.aggregator, rewritten) + Branch.new(branch.aggregator, absorb_groups(branch.aggregator, rewritten)) end end + # A relation group aggregating the way its parent does is inlined into it: + # `or(x, or(a, b))` is `or(x, a, b)` and one level shallower. The level it + # saves is one nothing budgeted for -- what a tier measured its nesting + # limit against is the tree the operator wrote, not the equalities a + # relation expands into afterwards. + # + # Only a relation group is inlined. A group the operator wrote is what + # their filter means, and flattening it would spend on one group the + # conditions two groups were holding. + def absorb_groups(aggregator, nodes) + absorbed = nodes.flat_map do |node| + inlinable_group?(node, aggregator) ? Array(node.conditions) : [node] + end + + absorb_relation_group?(absorbed.size) ? absorbed : nodes + end + + def inlinable_group?(node, aggregator) + node.is_a?(RelationBranch) && node.aggregator.to_s.casecmp(aggregator.to_s).zero? + end + + # Hook for a tier that bounds how many conditions a group may hold: past + # that, the nested form is the one that fits, and the level it costs is + # spent rather than the width. + def absorb_relation_group?(_size) = true + def rewrite_relation_leaf(caller, leaf) name, path = leaf.field.to_s.split(':', 2) relation = fields[name] diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb index 59c62f85a..a0e111bcf 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/team.rb @@ -31,7 +31,7 @@ def serialize(team) # them: one read of `/admins` for the whole page, never one per team. A # token that cannot read the teammates costs the column and nothing else. def enrich(records, rows, projection) - return unless Array(projection).map(&:to_s).include?('admin_names') + return unless column_asked?(projection, 'admin_names') names = admin_names records.each_with_index do |record, index| diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb index 3418a7951..aa33b085d 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/query/condition_tree_translator.rb @@ -65,7 +65,7 @@ def translate_branch(branch, depth) # enough that a wrapper around nothing is a level worth not spending. return translate(conditions.first, depth) if conditions.size == 1 - refuse_too_deep!(depth) if depth > MAX_DEPTH + refuse_too_deep!(branch, depth) if depth > MAX_DEPTH refuse_too_wide!(branch, conditions.size) if conditions.size > MAX_GROUP_SIZE { 'operator' => operator, 'value' => conditions.map { |condition| translate(condition, depth + 1) } } @@ -75,11 +75,27 @@ def translate_branch(branch, depth) # message names the shape rather than a number, since the tree an operator # can act on is the segment and the scope they wrote, not the one the agent # assembled out of them. - def refuse_too_deep!(depth) + def refuse_too_deep!(branch, depth) raise UnsupportedOperatorError, "#{@collection} cannot answer this filter: Intercom nests a search #{MAX_DEPTH} levels deep and this " \ - "one reaches #{depth}. A group inside a group inside a group is one level too many -- flatten the " \ - 'segment, the scope or the filter carrying the innermost one.' + "one reaches #{depth}. #{deepening_cause(branch)}" + end + + # A group the operator wrote is theirs to flatten. A group a relation + # expanded into is not: it is the several records the relation matched, + # written one equality each because Intercom takes no membership operator + # on a foreign key -- and telling them to flatten a nesting they never + # wrote is a refusal they cannot act on. + def deepening_cause(branch) + field = branch.respond_to?(:relation_field) ? branch.relation_field : nil + + if field.nil? + 'A group inside a group inside a group is one level too many -- flatten the segment, the scope or the ' \ + 'filter carrying the innermost one.' + else + "The innermost group is what #{field.inspect} expanded into, one equality per record it matched: narrow " \ + 'that condition until it names a single record, or lift it out of the groups nesting it.' + end end # Fifteen is reached without trying: a scope, a segment and a filter add up, diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb index 28071c2d0..9ef97e218 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/admin_spec.rb @@ -40,11 +40,22 @@ def stub_teams(*teams) stub_admins('type' => 'admin', 'id' => '1', 'name' => 'Alice', 'email' => 'alice@acme.test', 'job_title' => 'Support', 'away_mode_enabled' => true, 'away_mode_reassign' => false, 'has_inbox_seat' => true, 'team_ids' => [814_865]) + stub_teams('id' => '814865', 'name' => 'Support') expect(collection.list(nil, filter, nil)) .to eq([{ 'id' => '1', 'name' => 'Alice', 'email' => 'alice@acme.test', 'job_title' => 'Support', 'away_mode_enabled' => true, 'away_mode_reassign' => false, 'has_inbox_seat' => true, - 'team_names' => nil }]) + 'team_names' => ['Support'] }]) + end + + # A projection naming nothing asks for every declared column, which is how + # `project` reads it -- so the enrichment has to read it that way too, or the + # one column the projection publishes comes back nil. + it 'fills the derived names when the projection names no column at all' do + stub_admins('id' => '1', 'name' => 'Alice', 'team_ids' => [814_865]) + stub_teams('id' => '814865', 'name' => 'Support') + + expect(collection.list(nil, filter, nil).first['team_names']).to eq(['Support']) end # Intercom types a team id as a number here and as a string on the team diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb index e456f5147..514476fe0 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb @@ -53,6 +53,14 @@ def evaluable?(operator, column_type) .equivalent_tree?(operator, described_class::IN_MEMORY_OPERATORS, column_type) end + # A projection naming nothing asks for every declared column, `team_names` + # among them, so a list with no projection reads the teams as well. + before do + stub_request(:get, "#{base}/teams") + .to_return(status: 200, body: { 'type' => 'team.list', 'teams' => [] }.to_json, + headers: { 'Content-Type' => 'application/json' }) + end + # The count and the group are taken over every record Intercom holds, not # over a page of them, which is what makes them exact. it 'is countable' do diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb index af1ed6d5a..6264d58e7 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb @@ -53,6 +53,13 @@ def stub_admins(*admins) expect(WebMock).to have_requested(:get, "#{base}/admins").once end + it 'names them when the projection names no column at all' do + stub_teams('id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) + stub_admins('id' => '493881', 'name' => 'Alice') + + expect(collection.list(nil, filter, nil).first['admin_names']).to eq(['Alice']) + end + it 'reads nothing when no projection asks for the names' do stub_teams('id' => '814865', 'name' => 'Support', 'admin_ids' => [493_881]) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb index 328c9d2cb..3e5530c25 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/ticket_spec.rb @@ -73,6 +73,10 @@ def stub_admins(*admins) stub_request(:get, "#{base}/admins").to_return(json('type' => 'admin.list', 'admins' => admins)) end + def stub_ticket_states(*states) + stub_request(:get, "#{base}/ticket_states").to_return(json('type' => 'list', 'data' => states)) + end + def stub_search(*records, total: nil, body: nil) answer = { 'type' => 'ticket.list', 'tickets' => records, 'total_count' => total || records.size, 'pages' => { 'type' => 'pages', 'page' => 1 } } @@ -339,6 +343,36 @@ def columns expect(WebMock).not_to have_requested(:get, "#{base}/admins") end + + # The price of a relation is one read per target *collection*, not one per + # relation: `state` and `previous_state` name the same endpoint, and it is + # read once, over the ids both of them point at. + it 'reads a collection two relations point at once, not once per relation' do + stub_search(ticket('1')) + stub_ticket_states({ 'id' => '19', 'internal_label' => 'En cours Tech' }, + { 'id' => '14', 'internal_label' => 'Recu' }) + + rows = collection.list(nil, filter, ['id', 'state:internal_label', 'previous_state:internal_label']) + + expect(rows.first).to eq({ 'id' => '1', + 'state' => { 'internal_label' => 'En cours Tech', 'id' => '19' }, + 'previous_state' => { 'internal_label' => 'Recu', 'id' => '14' } }) + expect(WebMock).to have_requested(:get, "#{base}/ticket_states").once + end + + # One read for two relations means one projection for two, and each of them + # still gets the columns it asked for rather than the union. + it 'nests under each relation only the columns that relation asked for' do + stub_search(ticket('1')) + stub_ticket_states({ 'id' => '19', 'category' => 'in_progress', 'internal_label' => 'En cours Tech' }, + { 'id' => '14', 'category' => 'submitted', 'internal_label' => 'Recu' }) + + rows = collection.list(nil, filter, ['id', 'state:category', 'previous_state:internal_label']) + + expect(rows.first).to eq({ 'id' => '1', + 'state' => { 'category' => 'in_progress', 'id' => '19' }, + 'previous_state' => { 'internal_label' => 'Recu', 'id' => '14' } }) + end end # A relation is published filterable as soon as any column of its target is, @@ -421,6 +455,53 @@ def columns })) end + # Intercom nests a search two levels deep, and the group a relation expands + # into is a level nobody wrote. Inlined into a parent aggregating the same + # way, it costs none -- without which a scope plus a "match any" filter + # carrying one relation condition would be refused for a nesting the + # operator cannot find in their own filter. + it 'inlines the group into a parent that aggregates the same way' do + stub_admins({ 'id' => '493881', 'name' => 'Alice' }, { 'id' => '493882', 'name' => 'Alice' }) + stub_search(ticket('1')) + tree = branch('And', leaf('category', operators::EQUAL, 'request'), + branch('Or', leaf('open', operators::EQUAL, true), + leaf('admin_assignee:name', operators::EQUAL, 'Alice'))) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to have_requested(:post, "#{base}/tickets/search") + .with(body: hash_including('query' => { + 'operator' => 'AND', + 'value' => [{ 'field' => 'category', 'operator' => '=', + 'value' => 'request' }, + { 'operator' => 'OR', + 'value' => [{ 'field' => 'open', 'operator' => '=', + 'value' => true }, + { 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493881' }, + { 'field' => 'admin_assignee_id', + 'operator' => '=', 'value' => '493882' }] }] + })) + end + + # Inlining trades a level of nesting for width, and Intercom bounds both. + # Past fifteen conditions the nested form is the one that fits, so the + # group stays where it was rather than emptying the budget it was spared. + it 'leaves the group nested when inlining it would pass fifteen conditions' do + stub_admins(*(1..3).map { |index| { 'id' => index.to_s, 'name' => 'Alice' } }) + stub_search(ticket('1')) + others = (1..14).map { |index| leaf('category', operators::NOT_EQUAL, "c#{index}") } + tree = branch('Or', *others, leaf('admin_assignee:name', operators::EQUAL, 'Alice')) + + collection.list(nil, filter(condition_tree: tree), %w[id]) + + expect(WebMock).to(have_requested(:post, "#{base}/tickets/search").with do |request| + query = JSON.parse(request.body)['query'] + query['value'].size == 15 && query['value'].last['operator'] == 'OR' && + query['value'].last['value'].size == 3 + end) + end + it 'reads nothing when a relation condition inside an and matches nothing' do stub_admins('id' => '493881', 'name' => 'Alice') tree = branch('And', leaf('category', operators::EQUAL, 'request'), diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb index b252c3867..797efa12a 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/query/condition_tree_translator_spec.rb @@ -134,7 +134,21 @@ def leaves(count) branch('And', *leaves(2)))) expect { translate(tree) } - .to raise_error(UnsupportedOperatorError, /nests a search 2 levels deep and this one reaches 3/) + .to raise_error(UnsupportedOperatorError, + /nests a search 2 levels deep and this one reaches 3.*flatten the segment/m) + end + + # The innermost group is sometimes not one the operator wrote: a relation + # expands into one equality per record it matched, and telling them to + # flatten a nesting they never wrote is a refusal they cannot act on. + it 'names the relation when the innermost group is what one expanded into' do + relation_group = Collections::Relations::RelationBranch.new('Or', leaves(2), 'admin_assignee:name') + tree = branch('Or', leaf('open', operators::EQUAL, true), + branch('And', leaf('read', operators::EQUAL, true), relation_group)) + + expect { translate(tree) } + .to raise_error(UnsupportedOperatorError, + /"admin_assignee:name" expanded into, one equality per record it matched/) end # A branch carrying a single condition is unwrapped, so it spends no level: diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb index 537cdf97c..8e7edd9b8 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb @@ -56,10 +56,15 @@ def self.other_inverse?(field, relation_field) field.origin_key == relation_field.foreign_key end + # ValidationError rather than a bare ForestException, for the same reason as + # below: every one of these three names a field the request asked for -- a + # filter, a sort, a scope written against another schema, a condition + # reaching through a relation that is not one this can traverse. That is a + # 400 the caller can read and fix, not a 500 saying the agent broke. def self.get_field_schema(collection, field_name) fields = collection.schema[:fields] unless field_name.include?(':') - raise ForestException, "Column not found #{collection.name}.#{field_name}" unless fields.key?(field_name) + raise ValidationError, "Column not found #{collection.name}.#{field_name}" unless fields.key?(field_name) return fields[field_name] end @@ -67,10 +72,11 @@ def self.get_field_schema(collection, field_name) association_name = field_name.split(':')[0] relation_schema = fields[association_name] - raise ForestException, "Relation not found #{collection.name}.#{association_name}" unless relation_schema + raise ValidationError, "Relation not found #{collection.name}.#{association_name}" unless relation_schema if relation_schema.type != 'ManyToOne' && relation_schema.type != 'OneToOne' - raise ForestException, "Unexpected field type #{relation_schema.type}: #{collection.name}.#{association_name}" + raise ValidationError, + "Unexpected field type #{relation_schema.type}: #{collection.name}.#{association_name}" end get_field_schema( diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb index fb7d88535..fa3deeb35 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb @@ -221,7 +221,7 @@ module Utils expect do described_class.get_field_schema(collection_person, 'foo') - end.to raise_error(ForestException, 'Column not found Person.foo') + end.to raise_error(ValidationError, 'Column not found Person.foo') end it 'get_field_schema should work with simple column' do @@ -233,14 +233,14 @@ module Utils expect do described_class.get_field_schema(collection_person, 'unknown:foo') - end.to raise_error(ForestException, 'Relation not found Person.unknown') + end.to raise_error(ValidationError, 'Relation not found Person.unknown') end it 'get_field_schema should throw with invalid relation type' do expect do described_class.get_field_schema(collection_book, 'myBookPersons:bookId') - end.to raise_error(ForestException, 'Unexpected field type OneToMany: Book.myBookPersons') + end.to raise_error(ValidationError, 'Unexpected field type OneToMany: Book.myBookPersons') end it 'get_field_schema should work with relation column' do From e1ae9b444a81b8cce5aa8db3769ce495d19fbcaf Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Mon, 7 Sep 2026 09:01:42 +0200 Subject: [PATCH 3/3] fix(datasource): dead through row, and an order not honoured Two failures a many-to-many related list could reach, neither of which any tier reported. `Utils::Collection.list_relation` unwraps a through row by the name of the relation it travels through, and a target the foreign collection no longer answers leaves a nil in its place. Deleted, outside the caller's reach, or past the page cap of a datasource reading its targets whole: the row reached the JSON:API serializer, which reads a record by key, as a 500. A join whose other side is gone yields no row, not an empty one, so the mapping compacts. The other is an order asked for and not honoured. The agent hands the through collection the sort written against the collection the relation reaches -- `Filter#nest` prefixes the condition tree and not the sort -- so a related list of teammates ordered by name reached `FetchAllCollection` with a clause it carries no column for, and dropped it without a word. Every other order this datasource cannot honour is reported; this one now is too. Co-Authored-By: Claude Opus 5 (1M context) --- .../README.md | 7 ++++ .../collections/fetch_all_collection.rb | 29 ++++++++++++--- .../collections/fetch_all_collection_spec.rb | 21 ++++++++++- .../collections/team_spec.rb | 37 +++++++++++++++++++ .../utils/collection.rb | 9 ++++- .../utils/collection_spec.rb | 19 ++++++++++ 6 files changed, 114 insertions(+), 8 deletions(-) diff --git a/packages/forest_admin_datasource_intercom/README.md b/packages/forest_admin_datasource_intercom/README.md index 40781f30b..43ce826e8 100644 --- a/packages/forest_admin_datasource_intercom/README.md +++ b/packages/forest_admin_datasource_intercom/README.md @@ -130,6 +130,13 @@ for the pair, while a many-to-many needs a collection to travel through. It is s `GET /teams`, one record per pair, keyed `teamId:adminId`. Without it, both sides read as an array of ids nobody can click. +Two consequences of travelling through it are worth knowing. A **related list of teammates is +ordered by the membership, not by the teammate**: the agent hands the through collection the columns +of the collection the relation reaches, so an order on `name` or `email` cannot be resolved there and +is logged rather than silently dropped. And a `admin_ids` entry naming a teammate `/admins` does not +answer -- one who left, one outside the token's reach -- **drops out of the related list** instead of +appearing as an empty row. + Alongside it, a team names its teammates (`admin_names`) and a teammate its teams (`team_names`) on the row itself, so a list view reads without a join. **Those replace the arrays of ids** the first lots published: one readable form plus a relation to navigate, rather than two ways to read one fact. diff --git a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb index 862c349f1..2cb9d8b30 100644 --- a/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_intercom/lib/forest_admin_datasource_intercom/collections/fetch_all_collection.rb @@ -17,7 +17,7 @@ module Collections # Each read re-reads the endpoint, so an operator sees what Intercom holds # now rather than what it held when the process booted. One request per list # against a 10 000-a-minute budget is not a figure any list view approaches. - class FetchAllCollection < BaseCollection + class FetchAllCollection < BaseCollection # rubocop:disable Metrics/ClassLength # The filters a column may advertise, per column type. Restricted to what # the toolkit can evaluate in memory, since the in-memory pass is the only # pass there is here: an operator with no equivalence makes `match` answer @@ -173,19 +173,36 @@ def sort_in_memory(records, sort) # A sort clause naming a field this collection does not carry is dropped: # ordering by a column that is not there would compare nil to nil on every - # row and leave the order to the tie-break. + # row and leave the order to the tie-break. Reported when it happens, + # rather than dropped in silence -- an order asked for and not honoured is + # reported everywhere else in this datasource, and there is one route that + # reaches this tier with a clause it cannot resolve: a related list through + # a many-to-many is served by the collection it travels through, and + # `Filter#nest` prefixes the condition tree without prefixing the sort, so + # the membership is handed the columns of the collection it reaches. def sort_clauses(sort) - Array(sort).filter_map do |clause| - field = clause[:field] || clause['field'] - next unless fields.key?(field) + known, unknown = Array(sort).partition { |clause| fields.key?(sort_field(clause)) } + warn_unsortable(unknown) unless unknown.empty? + known.map do |clause| # `key?` rather than `||`: a descending clause carries `false`, which an # `||` fallback would read as "absent" and turn back into ascending. ascending = clause.key?(:ascending) ? clause[:ascending] : clause['ascending'] - [field, ascending != false] + [sort_field(clause), ascending != false] end end + def sort_field(clause) = clause[:field] || clause['field'] + + def warn_unsortable(clauses) + ForestAdminDatasourceIntercom.logger.warn( + "[forest_admin_datasource_intercom] #{name} was asked to sort on " \ + "#{clauses.map { |clause| sort_field(clause).inspect }.join(", ")}, which it does not carry; the rows " \ + 'come back in the order Intercom returned them. A related list through a many-to-many is ordered by the ' \ + 'columns of the collection it travels through, not by those of the collection it reaches.' + ) + end + def compare_clauses(left, right, clauses) clauses.each do |field, ascending| comparison = compare_values(left[field], right[field]) diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb index 514476fe0..94d03933c 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/fetch_all_collection_spec.rb @@ -193,10 +193,29 @@ def filtered(field, operator, value = nil) expect(ids(rows)).to eq(%w[2 1 3]) end - it 'drops a clause naming a column it does not carry' do + # An order asked for and not honoured is reported everywhere else in this + # datasource, and a related list through a many-to-many reaches this tier + # with the columns of the collection it *reaches* rather than the one it + # travels through -- `Filter#nest` prefixes the condition tree and not the + # sort. Dropped in silence, that is the one order nothing would report. + it 'drops a clause naming a column it does not carry, and says so' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + rows = collection.list(nil, filter(sort: sort({ field: 'unknown', ascending: true })), nil) expect(ids(rows)).to eq(%w[2 1 3]) + expect(ForestAdminDatasourceIntercom.logger) + .to have_received(:warn).with(/sort on "unknown", which it does not carry/) + end + + it 'keeps the clauses it does carry alongside the one it drops' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + + rows = collection.list( + nil, filter(sort: sort({ field: 'unknown', ascending: true }, { field: 'name', ascending: true })), nil + ) + + expect(ids(rows)).to eq(%w[1 2 3]) end end diff --git a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb index 6264d58e7..54c01b75a 100644 --- a/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb +++ b/packages/forest_admin_datasource_intercom/spec/forest_admin_datasource_intercom/collections/team_spec.rb @@ -130,6 +130,43 @@ def stub_admins(*admins) expect(rows).to eq([{ 'id' => '493881', 'name' => 'Alice' }]) end + + # `admin_ids` names a teammate `/admins` does not answer -- one who left + # the workspace, one outside the token's reach, one past the page cap of a + # large workspace. The membership nests a nil under the relation, which is + # right on a row an operator reads; unwrapped into a related list it would + # be a row holding nothing, and the serializer reads a row by key. + it 'drops a teammate the workspace no longer answers, rather than listing an empty row' do + stub_teams('id' => '814865', 'admin_ids' => [493_881, 999_999]) + stub_admins('id' => '493881', 'name' => 'Alice') + query = ForestAdminDatasourceToolkit::Components::Query + + rows = ForestAdminDatasourceToolkit::Utils::Collection.list_relation( + collection, %w[814865], 'admins', nil, query::Filter.new, query::Projection.new(%w[id name]) + ) + + expect(rows).to eq([{ 'id' => '493881', 'name' => 'Alice' }]) + end + + # The membership is handed the columns of the collection the relation + # reaches, `Filter#nest` prefixing the condition tree and not the sort. It + # cannot order on them, and says so rather than answering an order it did + # not honour. + it 'reports the order it cannot honour on a related list' do + allow(ForestAdminDatasourceIntercom.logger).to receive(:warn) + stub_teams('id' => '814865', 'admin_ids' => [493_881, 493_882]) + stub_admins({ 'id' => '493881', 'name' => 'Zoe' }, { 'id' => '493882', 'name' => 'Alice' }) + query = ForestAdminDatasourceToolkit::Components::Query + + ForestAdminDatasourceToolkit::Utils::Collection.list_relation( + collection, %w[814865], 'admins', nil, + query::Filter.new(sort: query::Sort.new([{ field: 'name', ascending: true }])), + query::Projection.new(%w[id name]) + ) + + expect(ForestAdminDatasourceIntercom.logger) + .to have_received(:warn).with(/sort on "name", which it does not carry/) + end end end end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb index 8e7edd9b8..916d9884c 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb @@ -167,7 +167,14 @@ def self.list_relation(collection, primary_key_values, relation_name, caller, fo projection.nest(prefix: foreign_relation) ) - return records.map { |r| r[foreign_relation] } + # Compacted: a through row whose target the foreign collection no + # longer answers -- a record deleted, outside the caller's reach, or + # dropped by a datasource that reads its targets in bounded pages -- + # carries a nil where a record was expected, and every consumer of a + # related list down to the JSON:API serializer reads a row by key. + # A join whose other side is gone yields no row, it does not yield an + # empty one. + return records.filter_map { |r| r[foreign_relation] } end end diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb index fa3deeb35..ce0036c25 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb @@ -311,6 +311,25 @@ module Utils ForestAdminDatasourceToolkit::Components::Query::Projection.new)).to eq([1]) end + # A through row whose target the foreign collection no longer answers -- + # deleted, outside the caller's reach, or dropped by a datasource that + # reads its targets in bounded pages -- carries a nil where a record was + # expected, and every consumer down to the JSON:API serializer reads a + # row by key. A join whose other side is gone yields no row. + it 'list_relation should drop a through row whose target resolved to nothing' do + book_person_class = Struct.new(:bookId, :personId, :myPerson, :myBook) + stub_const('BookPerson', book_person_class) + allow(collection_book_person).to receive(:list).and_return( + [ + BookPerson.new(1, 1, 1, 1), + BookPerson.new(1, 2, nil, 1) + ] + ) + + expect(described_class.list_relation(collection_book, [1], 'myPersons', caller, ForestAdminDatasourceToolkit::Components::Query::Filter.new, + ForestAdminDatasourceToolkit::Components::Query::Projection.new)).to eq([1]) + end + it 'aggregate_relation should work with one to many relation' do allow(collection_book_person).to receive(:aggregate).and_return(1)