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