From 03c47f3e1605edad8411460a29d2f51e53840664 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Wed, 19 Aug 2026 18:09:28 +0200 Subject: [PATCH 1/8] feat(pylon): write operations (CRUD) Open the datasource to writes: a `writes` mixin on the client, one method per Pylon endpoint, and create/update/delete on every collection, through a mechanism shared by the three collection bases. What may be written is `is_read_only` on the column, the single source of truth the payload builder reads, the way `api_filters` already is for filtering. Pylon's own `is_read_only` is now honoured on a custom field, and a value is written back through the list of `{slug, value}` entries the API takes, `values` for a multiselect and the option slug for a select. The verbs Pylon exposes no endpoint for -- no POST or DELETE on a user, no DELETE on a team -- refuse with a message rather than the contract's NotImplementedError, which the agent answers as an unexpected 500. So do the fields it only accepts in one direction: `body_html` on a create, `state` on an update, and the like, dropped when they ask for nothing and refused when the operator really changed them. A filter-driven update or delete resolves its ids exactly or refuses: an id filter is answered without a request, anything else goes through the collection's own list so the scope applies, and a selection wider than one pass of writes is refused rather than written halfway. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/forest_admin_datasource_pylon.rb | 7 + .../forest_admin_datasource_pylon/client.rb | 2 + .../client/writes.rb | 74 +++++ .../collections/account.rb | 14 + .../collections/account/schema_definition.rb | 36 +- .../collections/base_collection.rb | 16 +- .../collections/contact.rb | 10 + .../collections/contact/schema_definition.rb | 37 ++- .../collections/cursor_collection.rb | 5 - .../collections/fetch_all_collection.rb | 15 +- .../collections/issue.rb | 22 ++ .../collections/issue/schema_definition.rb | 43 ++- .../collections/team.rb | 13 +- .../collections/user.rb | 16 +- .../collections/writes.rb | 222 +++++++++++++ .../schema/custom_fields_introspector.rb | 25 +- .../client/writes_spec.rb | 122 +++++++ .../collections/account_spec.rb | 14 +- .../collections/contact_spec.rb | 15 +- .../collections/issue_spec.rb | 15 +- .../collections/team_spec.rb | 15 +- .../collections/user_spec.rb | 15 +- .../collections/writes_spec.rb | 309 ++++++++++++++++++ .../schema/custom_fields_introspector_spec.rb | 38 ++- 24 files changed, 1007 insertions(+), 93 deletions(-) create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb create mode 100644 packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb create mode 100644 packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb 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 24aeadbba..9a291e5de 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 @@ -20,6 +20,13 @@ class ConfigurationError < Error; end # they learn which one. class UnsupportedOperatorError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end + # A write Pylon cannot perform: a verb its API has no endpoint for, a field it + # only accepts in the other direction, or a filter-driven write reaching more + # records than one page of writes may cover. Descends from ValidationError for + # the same reason as above — each names something the operator did and can + # undo, and the message is the only place they learn what. + class UnsupportedWriteError < 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 # surface Pylon's own validation message instead of a generic string. 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 101217930..c477e7646 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 @@ -2,6 +2,8 @@ module ForestAdminDatasourcePylon # 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 + include Writes + MAX_SEARCH_LIMIT = 1000 # Bounds `collect_pages`, which asks for a whole dataset rather than a diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb new file mode 100644 index 000000000..c38afa402 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb @@ -0,0 +1,74 @@ +module ForestAdminDatasourcePylon + class Client + # The write half of the client: one explicit method per Pylon write + # endpoint, each delegating to the shared helpers below. + # + # Nothing here degrades. `best_effort` exists for the calls whose result + # enriches a page — a thread that could not be read costs a column — where a + # write that silently did nothing would tell the operator their edit landed. + # + # Pylon exposes no write endpoint for every verb: there is no POST or DELETE + # on users, and no DELETE on teams. The collections answer those, not the + # client, which only spells the endpoints that exist. + module Writes + # `title` and `body_html` are the two fields POST /issues requires. + def create_issue(attributes) = post_resource('issues', attributes) + def update_issue(id, attributes) = patch_resource('issues', id, attributes) + def delete_issue(id) = delete_resource('issues', id) + + def create_account(attributes) = post_resource('accounts', attributes) + def update_account(id, attributes) = patch_resource('accounts', id, attributes) + def delete_account(id) = delete_resource('accounts', id) + + def create_contact(attributes) = post_resource('contacts', attributes) + def update_contact(id, attributes) = patch_resource('contacts', id, attributes) + def delete_contact(id) = delete_resource('contacts', id) + + def create_team(attributes) = post_resource('teams', attributes) + def update_team(id, attributes) = patch_resource('teams', id, attributes) + + def update_user(id, attributes) = patch_resource('users', id, attributes) + + private + + def post_resource(resource, attributes) + operation = "create(#{resource})" + + must_succeed(operation) { extract_written(connection.post(resource, attributes).body, operation) } + end + + # The id comes from the record the operator acted on, so it is escaped + # before being joined to the path, like every read does. + def patch_resource(resource, id, attributes) + path = "#{resource}/#{Faraday::Utils.escape(id)}" + operation = "update(#{path})" + + must_succeed(operation) { extract_written(connection.patch(path, attributes).body, operation) } + end + + # Answers true rather than the body: Pylon returns 200 or 204 with nothing + # worth reading, and a caller has no record left to serialize. + def delete_resource(resource, id) + path = "#{resource}/#{Faraday::Utils.escape(id)}" + + must_succeed("delete(#{path})") do + connection.delete(path) + true + end + end + + # Pylon answers a write with the written record under `data`. Anything else + # means the contract broke, which is worth a typed error rather than an + # envelope the collection would then serialize into a record with no id -- + # `extract_data` hands the body back untouched when `data` is absent, which + # is what a read wants and a write must not accept. + def extract_written(body, operation) + record = body['data'] if body.is_a?(Hash) + return record if record.is_a?(Hash) + + raise APIError, + "Pylon API #{operation} returned an unexpected body shape (missing 'data'): #{body.inspect}" + end + end + end +end 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 index 0f5a09f0f..42bb57aca 100644 --- 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 @@ -4,6 +4,13 @@ class Account < CursorCollection include SchemaDefinition include Serializer + # Pylon reads an account's type back as `type` and takes it as + # `account_type`. + RENAMES = { 'type' => 'account_type' }.freeze + + # An account is created enabled; only `PATCH /accounts/{id}` disables one. + UPDATE_ONLY = %w[is_disabled].freeze + def initialize(datasource, custom_fields: []) super(datasource, 'PylonAccount', custom_fields: custom_fields, searchable: true) end @@ -12,6 +19,13 @@ def initialize(datasource, custom_fields: []) def filter_table = ApiFilters + def create_record(payload) = datasource.client.create_account(payload) + def update_record(id, payload) = datasource.client.update_account(id, payload) + def delete_record(id) = datasource.client.delete_account(id) + + def update_only_fields = UPDATE_ONLY + def payload_renames = RENAMES + 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.' diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb index 00bcc089f..94004c74a 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb @@ -1,10 +1,12 @@ 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. + # A column is writable when `POST /accounts` or `PATCH /accounts/{id}` + # accepts it, in the shape it is read under — the Json columns holding + # objects rather than plain strings are left read-only, see below. No + # column is sortable — 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 @@ -41,29 +43,41 @@ def define_relations def define_identity_fields add_column('id', 'String', is_primary_key: true) - add_column('name', 'String') + add_column('name', 'String', writable: true) # 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') + # prospect but lets an organization define its own account types. It + # is written under the name `account_type`, see `Account::RENAMES`. + add_column('type', 'String', writable: true) + # Writable on an update only: an account is created enabled. + add_column('is_disabled', 'Boolean', writable: true) end # `domain` and `primary_domain` carry the same value; both are kept # because Pylon returns both, and only the `domains` list is filterable. + # Neither is writable: `domains` is the list the API takes, and writing + # one of its two projections would leave the other stale. def define_domain_fields add_column('domain', 'String') add_column('primary_domain', 'String') - add_column('domains', 'Json') - add_column('tags', 'Json') + add_column('domains', 'Json', writable: true) + add_column('tags', 'Json', writable: true) 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('owner_id', 'String', writable: true) + # Read-only although the endpoint takes it: the column shows + # `{external_id, label}` objects, and what the API writes back is not + # documented in that shape — writing one for the other would replace + # the ids of the account with something it cannot read. add_column('external_ids', 'Json') end + # Both belong to the integrations Pylon syncs them from: `crm_settings` + # is absent from every write endpoint, and `channels` — which they do + # take — is a list of objects whose write shape the reference does not + # document, the same reason `external_ids` stays read-only above. def define_integration_fields add_column('channels', 'Json') add_column('crm_settings', 'Json') 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 f6b209458..166a80ee6 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 @@ -1,6 +1,8 @@ module ForestAdminDatasourcePylon module Collections class BaseCollection < ForestAdminDatasourceToolkit::Collection + include Writes + ColumnSchema = ForestAdminDatasourceToolkit::Schema::ColumnSchema ManyToOneSchema = ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema OneToManySchema = ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema @@ -198,18 +200,19 @@ def api_filters 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 + # A native column: read-only unless the collection declares it `writable`, + # which is what the payload builder reads to know a column may be sent, 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) + def add_column(name, type, is_primary_key: false, writable: 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)) + is_read_only: !writable)) end # A record read through the endpoint of an id that is not the primary key @@ -262,6 +265,11 @@ def default_pk_sort?(sort) normalized_sort_clauses(sort) == normalized_sort_clauses(SortFactory.by_primary_keys(self)) end + # The search box sends an empty string once the operator clears it. + def no_search?(filter) + filter&.search.to_s.strip.empty? + end + def timezone_for(caller) return 'UTC' unless caller.respond_to?(:timezone) 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 index 822df0797..314fdb7b9 100644 --- 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 @@ -4,6 +4,10 @@ class Contact < CursorCollection include SchemaDefinition include Serializer + # `POST /contacts` takes the primary address alone; the other ones are set + # on an existing contact. + UPDATE_ONLY = %w[emails].freeze + def initialize(datasource, custom_fields: []) super(datasource, 'PylonContact', custom_fields: custom_fields, searchable: true) end @@ -12,6 +16,12 @@ def initialize(datasource, custom_fields: []) def filter_table = ApiFilters + def create_record(payload) = datasource.client.create_contact(payload) + def update_record(id, payload) = datasource.client.update_contact(id, payload) + def delete_record(id) = datasource.client.delete_contact(id) + + def update_only_fields = UPDATE_ONLY + 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.' diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb index 176a83a08..3d11aa061 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb @@ -1,10 +1,12 @@ 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. + # A column is writable when `POST /contacts` or `PATCH /contacts/{id}` + # accepts it, in the shape it is read under — the Json columns holding + # objects rather than plain strings are left read-only, see below. No + # column is sortable — 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 @@ -41,11 +43,13 @@ def define_relations def define_identity_fields add_column('id', 'String', is_primary_key: true) - add_column('name', 'String') + add_column('name', 'String', writable: true) # 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') + # it is the key of: the search endpoint filters it. Writable, which is + # what opens the relation editor — see the party fields of PylonIssue + # for why the key itself stays read-only in the Forest schema. + add_column('account_id', 'String', writable: true) # 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 @@ -55,20 +59,27 @@ def define_identity_fields # `email` and `primary_phone_number` carry the primary value; the lists # hold every address and number, and neither list is filterable. + # + # `emails` is writable on an update only, `POST /contacts` taking the + # primary address alone. `phone_numbers` is not writable at all: it + # holds objects, and the shape the endpoint takes them in is not the one + # the column shows. def define_contact_fields - add_column('email', 'String') - add_column('emails', 'Json') - add_column('primary_phone_number', 'String') + add_column('email', 'String', writable: true) + add_column('emails', 'Json', writable: true) + add_column('primary_phone_number', 'String', writable: true) add_column('phone_numbers', 'Json') - add_column('avatar_url', 'String') + add_column('avatar_url', 'String', writable: true) 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('portal_role', 'String', writable: true) + add_column('portal_role_id', 'String', writable: true) + # Owned by the integrations the contact was seen through; no endpoint + # takes it. add_column('integration_user_ids', 'Json') end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb index 797f8e6ed..43d68a666 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/cursor_collection.rb @@ -125,11 +125,6 @@ def records_by_id(id) [] 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 index 9cac206d3..1da8d0da8 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb @@ -76,18 +76,19 @@ def records_indexed_by_id(ids) 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) + # A column is read-only unless the collection declares it `writable`, which + # is what the payload builder reads to know it may be sent. 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, writable: 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)) + is_read_only: !writable)) end # Pylon defines custom fields on issues, accounts and contacts only, so 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 fdf48b40f..3faccb5ef 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 @@ -46,6 +46,15 @@ class Issue < BaseCollection # that would let this cap grow. MAX_MESSAGE_EMBEDS = 10 + # `body_html` is the first message of the thread, which `POST /issues` + # requires and `PATCH /issues/{id}` does not carry; `author_unverified` + # qualifies that message and travels with it. + CREATE_ONLY = %w[body_html author_unverified].freeze + + # Pylon creates every issue as `new`, of the type it decides, and takes + # both on an update only. + UPDATE_ONLY = %w[state type].freeze + def initialize(datasource, custom_fields: []) super(datasource, 'PylonIssue', custom_fields: custom_fields, searchable: true) end @@ -62,6 +71,19 @@ def list(caller, filter, projection) def filter_table = ApiFilters + def create_record(payload) = datasource.client.create_issue(payload) + def update_record(id, payload) = datasource.client.update_issue(id, payload) + def delete_record(id) = datasource.client.delete_issue(id) + + def create_only_fields = CREATE_ONLY + def update_only_fields = UPDATE_ONLY + + # Never past the primary-key fan-out: a write resolving its ids through + # `list` goes through `fetch_by_ids`, which truncates with a warning, and + # a truncated resolution would write to a subset of the selection while + # reporting the whole of it. + def max_write_targets = [Writes::MAX_WRITE_TARGETS, MAX_ID_LOOKUPS].min + def sortable_fields PYLON_SORTABLE 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 838d04045..587561698 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 @@ -1,11 +1,13 @@ module ForestAdminDatasourcePylon module Collections class Issue < BaseCollection - # Every column is read-only in this story: writes land in a later one. No - # column is sortable either — `/issues/search` exposes no sort parameter at - # all, results always come back ordered by `created_at` descending, so - # advertising a sortable column would let the UI ask for an order the API - # cannot honour. + # A column is writable when `POST /issues` or `PATCH /issues/{id}` accepts + # it, the two directions being told apart by `Issue::CREATE_ONLY` and + # `Issue::UPDATE_ONLY`; everything Pylon computes — the number, the link, + # the timestamps, the counters — stays read-only. No column is sortable, + # `/issues/search` exposing no sort parameter at all: results always come + # back ordered by `created_at` descending, 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 @@ -57,16 +59,20 @@ def define_identity_fields end def define_content_fields - add_column('title', 'String') - add_column('body_html', 'String') + add_column('title', 'String', writable: true) + # Writable on creation only: it is the first message of the thread, + # which `PATCH /issues/{id}` does not carry. + add_column('body_html', 'String', writable: true) # Left as String rather than Enum: Pylon ships five built-in states - # but organisations define their own on top of them. - add_column('state', 'String') - add_column('type', 'String') + # but organisations define their own on top of them. Writable on an + # update only — every issue is created `new`. + add_column('state', 'String', writable: true) + add_column('type', 'String', writable: true) + # Where the issue came from: Pylon sets it, no endpoint takes it. add_column('source', 'String') - add_column('tags', 'Json') - add_column('customer_portal_visible', 'Boolean') - add_column('author_unverified', 'Boolean') + add_column('tags', 'Json', writable: true) + add_column('customer_portal_visible', 'Boolean', writable: true) + add_column('author_unverified', 'Boolean', writable: true) add_column('number_of_touches', 'Number') define_thread_field end @@ -90,8 +96,17 @@ def define_thread_field # 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. + # + # Writable, although the schema the agent sends Forest marks a foreign + # key read-only whatever the datasource says — `GeneratorField` forces it + # so the detail view has one editor per key rather than two. What the + # flag opens is that editor, the `BelongsTo` reading its own read-only + # state off the key column, and the front sends the choice back as the + # very column named here. def define_party_fields - %w[account_id requester_id assignee_id team_id].each { |field| add_column(field, 'String') } + %w[account_id requester_id assignee_id team_id].each do |field| + add_column(field, 'String', writable: true) + end end def define_time_fields diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb index d4e2aca49..6f00f8277 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb @@ -7,6 +7,11 @@ def initialize(datasource) protected + # `delete_record` is left alone: Pylon exposes no DELETE on a team, and + # the default hook refuses the verb with a message rather than a 500. + def create_record(payload) = datasource.client.create_team(payload) + def update_record(id, payload) = datasource.client.update_team(id, payload) + def fetch_all datasource.client.fetch_teams end @@ -33,10 +38,12 @@ def define_relations def define_schema add_column('id', 'String', is_primary_key: true) - add_column('name', 'String') + add_column('name', 'String', writable: true) # A list, so neither filterable nor sortable, and no relation either: - # see `define_relations` above. - add_column('user_ids', 'Json') + # see `define_relations` above. Writable: `POST /teams` and + # `PATCH /teams/{id}` take the members as this very list of ids, and the + # one sent replaces the membership whole. + add_column('user_ids', 'Json', writable: true) 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 index 7b90eedfe..a04293b2a 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb @@ -9,6 +9,11 @@ def initialize(datasource) protected + # Pylon exposes no POST and no DELETE on a user — an agent is invited and + # deactivated from Pylon itself — so only the update hook is wired and the + # other two refuse the verb with a message rather than a 500. + def update_record(id, payload) = datasource.client.update_user(id, payload) + # `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 @@ -42,19 +47,22 @@ def define_relations origin_key: 'assignee_id', origin_key_target: 'id')) end + # `PATCH /users/{id}` takes the name, the avatar, the role and the status, + # and nothing else: an address is proven by the agent signing in, and the + # deactivation happens in Pylon. def define_schema add_column('id', 'String', is_primary_key: true) - add_column('name', 'String') + add_column('name', 'String', writable: true) 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') + add_column('avatar_url', 'String', writable: true) # 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('status', 'String', writable: true) + add_column('role_id', 'String', writable: true) add_column('role_name', 'String') add_column('is_deactivated', 'Boolean') end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb new file mode 100644 index 000000000..1e67e95c7 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -0,0 +1,222 @@ +module ForestAdminDatasourcePylon + module Collections + # The write half of every Pylon collection: `create`, `update` and `delete`, + # the payload they send, and the ids a filter-driven write applies to. + # + # Included by `BaseCollection`, so the mechanism is shared and each + # collection only declares what belongs to it: the client calls, through the + # `*_record` hooks, and the handful of fields Pylon accepts in one direction + # only. A hook a collection leaves alone refuses the verb, which is how the + # collections Pylon exposes no endpoint for — no POST or DELETE on users, no + # DELETE on teams — answer with a message instead of the contract's + # NotImplementedError, read by the agent as an unexpected 500. + # + # What may be written is not a list kept here: it is `is_read_only` on the + # column, the same way `api_filters` is the single source of truth for what + # may be filtered. A column the schema declares read-only is dropped from + # the payload, whether it is native, a foreign key or a custom field. + # + # Long by line count only: three verbs, the payload they share, and the + # refusals naming what Pylon cannot do. + module Writes # rubocop:disable Metrics/ModuleLength + Filter = ForestAdminDatasourceToolkit::Components::Query::Filter + Page = ForestAdminDatasourceToolkit::Components::Query::Page + Projection = ForestAdminDatasourceToolkit::Components::Query::Projection + + # How many records one filter-driven update or delete may reach. Pylon + # writes one record per request against a budget of 10 to 20 requests per + # minute, so a wider selection is refused rather than written halfway: + # a delete that stopped in the middle of the page would look done and + # would not be, which is the very thing this datasource refuses. + MAX_WRITE_TARGETS = 20 + + def create(_caller, data) + serialize(create_record(build_payload(data, :create))) + end + + def update(caller, filter, patch) + ids = ids_for(caller, filter) + return if ids.empty? + + payload = build_payload(patch, :update, caller: caller, filter: filter) + return if payload.empty? + + ids.each { |id| update_record(id, payload) } + end + + def delete(caller, filter) + ids_for(caller, filter).each { |id| delete_record(id) } + end + + protected + + # One Pylon write endpoint each, overridden by the collections having one. + def create_record(_payload) = refuse_write('created') + def update_record(_id, _payload) = refuse_write('updated') + def delete_record(_id) = refuse_write('deleted') + + # The fields Pylon accepts on one endpoint and not on the other. The Forest + # schema carries a single read-only flag per column, so both directions + # offer them; these two lists are what tells them apart at write time. + def create_only_fields = [].freeze + def update_only_fields = [].freeze + + # Columns whose Pylon write name differs from the one they are read under. + def payload_renames = {}.freeze + + def max_write_targets = MAX_WRITE_TARGETS + + # The records a filter-driven write applies to: exact, or refused. Nothing + # here may quietly answer with a subset — the caller writes one request per + # id and reports success for the whole selection. + # + # An `id equals`/`id in` filter alone is answered without a single request: + # that is what the record detail and the bulk selection of the UI send, and + # reading them back to learn ids they just named would spend the budget the + # writes themselves need. Anything else — a scope, a segment, a search, a + # condition on another column — is resolved by the collection's own `list`, + # so the scope applies and the endpoint filters what it can. + def ids_for(caller, filter) + tree = filter&.condition_tree + ids = filtered_ids(tree) + refuse_too_many_targets(ids.size) if ids && ids.size > max_write_targets + return ids if ids && id_values(tree) && no_search?(filter) + + resolve_ids_by_list(caller, filter) + end + + private + + # One record past the cap is asked for, so an overflow is seen rather than + # guessed from a full page — the same bound `foreign_keys_matching` puts on + # a resolved relation condition. + def resolve_ids_by_list(caller, filter) + window = Page.new(offset: 0, limit: max_write_targets + 1) + query = (filter || Filter.new).override(page: window) + records = list(caller, query, Projection.new(['id'])) + refuse_too_many_targets(records.size) if records.size > max_write_targets + + records.filter_map { |record| record['id'] }.uniq + end + + # The ids a filter names, whether as a leaf of its own or inside a + # top-level `and`. Unlike `extract_id_lookup`, nothing is asserted about + # what the rest of the tree can be applied in memory: the leftovers travel + # to `list`, which answers them the way a read does — server-side where the + # endpoint filters `id`, through the primary-key short-circuit where it + # does not. What this answers is only "how many records is this write + # about", which is the question the cap needs. + def filtered_ids(node) + return id_values(node) if id_values(node) + return nil unless and_branch?(node) + + condition = Array(node.conditions).find { |child| id_values(child) } + condition && id_values(condition) + end + + # Keys the schema declares writable, custom fields included, in the shape + # the endpoint takes. Everything else is dropped rather than refused: the + # front sends the fields of its form, and a read-only one reaching the + # payload is the agent's doing, not a request the operator made. + def build_payload(data, direction, caller: nil, filter: nil) + attrs = writable_attributes(data) + attrs = honour_write_direction(attrs, direction, caller, filter) + # Pylon fills in what a create leaves out; on an update a nil is the + # operator clearing a value, so it travels. + attrs = attrs.compact if direction == :create + + custom, native = split_custom_fields(attrs) + payload = native.transform_keys { |field| payload_renames.fetch(field, field) } + payload['custom_fields'] = custom unless custom.empty? + payload + end + + def writable_attributes(data) + attrs = data.is_a?(Hash) ? data.transform_keys(&:to_s) : {} + + attrs.select { |field, _value| writable_column?(field) } + end + + def writable_column?(field) + column = schema[:fields][field] + + column&.type == 'Column' && !column.is_read_only + end + + # A field of the other direction is dropped when it asks for nothing — no + # value at all, or, on an update, the value the record already holds, which + # is what a form resending an untouched field sends. It is refused when the + # operator really changed it: Pylon cannot write it, and answering the edit + # with a success it did not perform is worse than an error naming the + # field. + def honour_write_direction(attrs, direction, caller, filter) + wrong = attrs.keys & (direction == :create ? update_only_fields : create_only_fields) + return attrs if wrong.empty? + + stored = direction == :update ? stored_values(caller, filter, wrong) : [] + wrong.each do |field| + value = attrs[field] + next if value.nil? || value == '' + next if direction == :update && stored.all? { |record| record[field] == value } + + refuse_wrong_direction(field, direction) + end + + attrs.except(*wrong) + end + + # Read only when a field of the wrong direction carries a value, and only + # for that field: an update naming none costs no request at all. + def stored_values(caller, filter, fields) + list(caller, filter, Projection.new(['id'] + fields)) + end + + # Pylon reads its custom fields back as a map indexed by slug and writes + # them as a list, one entry per field, carrying `values` for a multi-value + # field and `value` for every other — a select being written by the slug of + # its option, which is what the Enum column advertises. + def split_custom_fields(attrs) + by_column = custom_fields.to_h { |custom_field| [custom_field[:column_name], custom_field] } + entries = [] + + native = attrs.each_with_object({}) do |(field, value), rest| + custom_field = by_column[field] + custom_field ? entries << custom_field_entry(custom_field, value) : rest[field] = value + end + + [entries, native] + end + + def custom_field_entry(custom_field, value) + slug = custom_field[:column_name] + return { 'slug' => slug, 'values' => Array(value) } if custom_field[:multi_value] + + { 'slug' => slug, 'value' => value } + end + + def refuse_write(verb) + raise UnsupportedWriteError, + "A #{name} record cannot be #{verb}: the Pylon API exposes no endpoint for it." + end + + def refuse_wrong_direction(field, direction) + detail = if direction == :create + 'Pylon only accepts it on an existing record: create the record, then edit it.' + else + 'Pylon only accepts it when the record is created, and exposes no endpoint to change it ' \ + 'afterwards.' + end + + raise UnsupportedWriteError, "'#{field}' cannot be set here on a #{name}: #{detail}" + end + + def refuse_too_many_targets(count) + raise UnsupportedWriteError, + "This write applies to #{count} #{name} records, more than the #{max_write_targets} one pass " \ + 'covers: Pylon writes one record per request, against a budget of ten to twenty requests per ' \ + 'minute, and a write stopping halfway would report a success it did not perform. Narrow the ' \ + 'selection to reach the records past this point.' + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb index 2286d64e4..1c0c7c0a1 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb @@ -1,8 +1,9 @@ module ForestAdminDatasourcePylon module Schema # Turns the custom fields an organization defined in Pylon into columns, as - # entries shaped `{ column_name:, schema: }` — what `add_custom_fields` - # registers on a collection. + # entries shaped `{ column_name:, schema:, multi_value: }` — what + # `add_custom_fields` registers on a collection, and what the payload + # builder writes a value back through. # # `column_name` is the Pylon slug verbatim, and there is no second key # carrying it: the slug is both what a read payload indexes the values by and @@ -38,6 +39,9 @@ class CustomFieldsIntrospector 'multiselect' => 'Json' }.freeze + # The types Pylon writes back through `values` rather than `value`. + MULTI_VALUE_TYPES = %w[multiselect].freeze + BASE_OPS = (Maps::EQUALITY.keys + Maps::PRESENCE.keys).freeze # A date drops the membership operators on the way, `Rules` granting a DATE @@ -97,19 +101,20 @@ def build_entry(raw, object_type) column_type = PYLON_TO_COLUMN_TYPE[raw['type']] return warn_unknown_type(raw, slug, object_type) if column_type.nil? - { column_name: slug, schema: build_schema(raw, column_type) } + { column_name: slug, schema: build_schema(raw, column_type), + multi_value: MULTI_VALUE_TYPES.include?(raw['type']) } end - # Every custom field is read-only in this story, like every native column: - # writes land in story 7 (EXT-11), which is also where Pylon's own - # `is_read_only` flag starts being honoured. Nothing is sortable either -- - # no Pylon endpoint takes a sort parameter, and nothing is groupable, as - # Pylon aggregates nothing: one column left groupable turns `supportGroups` - # on for the whole collection, and the group-by the UI then offers errors. + # A custom field is writable unless Pylon says otherwise: it flags the ones + # synced from an app or an integration, which its own endpoints refuse. + # Nothing is sortable -- no Pylon endpoint takes a sort parameter -- and + # nothing is groupable, as Pylon aggregates nothing: one column left + # groupable turns `supportGroups` on for the whole collection, and the + # group-by the UI then offers errors. def build_schema(raw, column_type) opts = { column_type: column_type, filter_operators: OPERATORS.fetch(column_type, []), - is_read_only: true, + is_read_only: raw['is_read_only'] == true, is_sortable: false, is_groupable: false } diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb new file mode 100644 index 000000000..d77719bbe --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb @@ -0,0 +1,122 @@ +RSpec.describe ForestAdminDatasourcePylon::Client::Writes do + let(:retry_policy) { ForestAdminDatasourcePylon::RetryPolicy.new(max_retries: 2, interval: 0) } + let(:configuration) { ForestAdminDatasourcePylon::Configuration.new(api_key: 'k', retry_policy: retry_policy) } + let(:client) { ForestAdminDatasourcePylon::Client.new(configuration) } + let(:base) { configuration.url } + + def json(payload, status = 200) + { status: status, body: payload.is_a?(String) ? payload : payload.to_json, + headers: { 'Content-Type' => 'application/json' } } + end + + # One method per endpoint, and the endpoint is the whole of what each one + # knows: the payload is the collection's to build. + describe 'the endpoint each write reaches' do + { + create_issue: [:post, 'issues'], create_account: [:post, 'accounts'], + create_contact: [:post, 'contacts'], create_team: [:post, 'teams'] + }.each do |method, (verb, path)| + it "#{method} posts to /#{path}" do + stub_request(verb, "#{base}/#{path}").to_return(json('data' => { 'id' => 'x' })) + + expect(client.public_send(method, 'name' => 'Acme')).to eq('id' => 'x') + expect(WebMock).to have_requested(verb, "#{base}/#{path}").with(body: { 'name' => 'Acme' }) + end + end + + { + update_issue: 'issues', update_account: 'accounts', update_contact: 'contacts', + update_team: 'teams', update_user: 'users' + }.each do |method, path| + it "#{method} patches /#{path}/{id}" do + stub_request(:patch, "#{base}/#{path}/x1").to_return(json('data' => { 'id' => 'x1' })) + + expect(client.public_send(method, 'x1', 'name' => 'Acme')).to eq('id' => 'x1') + expect(WebMock).to have_requested(:patch, "#{base}/#{path}/x1").with(body: { 'name' => 'Acme' }) + end + end + + { delete_issue: 'issues', delete_account: 'accounts', delete_contact: 'contacts' }.each do |method, path| + it "#{method} deletes /#{path}/{id}" do + stub_request(:delete, "#{base}/#{path}/x1").to_return(status: 204) + + expect(client.public_send(method, 'x1')).to be(true) + expect(WebMock).to have_requested(:delete, "#{base}/#{path}/x1") + end + end + end + + describe 'the record a write answers with' do + it 'unwraps the "data" envelope' do + stub_request(:post, "#{base}/issues") + .to_return(json('data' => { 'id' => 'i1', 'title' => 'Boom' }, 'request_id' => 'req_1')) + + expect(client.create_issue('title' => 'Boom')).to eq('id' => 'i1', 'title' => 'Boom') + end + + # A read hands an unwrapped body back untouched; a write must not, or the + # collection would serialize an envelope into a record carrying no id. + it 'raises when the envelope carries no record' do + stub_request(:post, "#{base}/issues").to_return(json('request_id' => 'req_1')) + + expect { client.create_issue('title' => 'Boom') } + .to raise_error(ForestAdminDatasourcePylon::APIError, /create\(issues\).*unexpected body shape/m) + end + + it 'raises when the record is not an object' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => 'ok')) + + expect { client.update_issue('i1', 'title' => 'Boom') } + .to raise_error(ForestAdminDatasourcePylon::APIError, %r{update\(issues/i1\)}) + end + end + + describe 'a failed write' do + it 'raises an APIError carrying the status and the request id' do + stub_request(:post, "#{base}/issues") + .to_return(json({ 'message' => 'title is required', 'request_id' => 'req_9' }, 422)) + + expect { client.create_issue({}) }.to raise_error(ForestAdminDatasourcePylon::APIError) { |error| + expect(error.status).to eq(422) + expect(error.message).to include('create(issues)', 'HTTP 422', 'title is required', 'req_9') + } + end + + it 'names the deleted record in the operation' do + stub_request(:delete, "#{base}/issues/i1").to_return(json({ 'message' => 'gone' }, 404)) + + expect { client.delete_issue('i1') } + .to raise_error(ForestAdminDatasourcePylon::APIError, %r{delete\(issues/i1\)}) + end + + # Ids reach the client from a filter the operator set, so they are escaped + # rather than joined to the path as they come. + it 'escapes an id that would otherwise alter the request path' do + stub_request(:patch, "#{base}/issues/a%2Fb").to_return(json('data' => { 'id' => 'a/b' })) + + client.update_issue('a/b', 'title' => 'Boom') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/a%2Fb") + end + end + + # A 429 is refused before Pylon processes the request, so replaying it creates + # nothing twice; a 502 may well have created the issue, and is not replayed. + describe 'retrying a write' do + it 'retries a rate-limited create' do + stub_request(:post, "#{base}/issues") + .to_return(json({ 'message' => 'slow down' }, 429)) + .then.to_return(json('data' => { 'id' => 'i1' })) + + expect(client.create_issue('title' => 'Boom')).to eq('id' => 'i1') + expect(WebMock).to have_requested(:post, "#{base}/issues").twice + end + + it 'does not retry a create that failed on a gateway error' do + stub_request(:post, "#{base}/issues").to_return(json({ 'message' => 'bad gateway' }, 502)) + + expect { client.create_issue('title' => 'Boom') }.to raise_error(ForestAdminDatasourcePylon::APIError) + expect(WebMock).to have_requested(:post, "#{base}/issues").once + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb index 0e9bf0b16..cc6d468bc 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/account_spec.rb @@ -102,12 +102,20 @@ def stub_search(payload = { 'data' => [account_payload('acc-1')] }) 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]) + # Neither endpoint exposes a sort parameter. + it 'declares every column non-sortable' do expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end + # The Json columns holding objects — `external_ids`, `channels` — stay + # read-only although the endpoint takes them: their write shape is not the + # one the column shows. + it 'declares writable exactly the columns an endpoint takes in the shape they are read' do + writable = columns.reject { |_name, column| column.is_read_only }.keys + + expect(writable).to contain_exactly('name', 'type', 'is_disabled', 'domains', 'tags', 'owner_id') + 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. diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb index 93ca7c47b..fd939a058 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb @@ -106,12 +106,21 @@ def stub_search(payload = { 'data' => [contact_payload('con-1')] }) 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]) + # Neither endpoint exposes a sort parameter. + it 'declares every column non-sortable' do expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end + # `phone_numbers` and `external_ids` stay read-only although the endpoint + # takes them: they hold objects, in a shape the write side does not + # document as the one the column shows. + it 'declares writable exactly the columns an endpoint takes in the shape they are read' do + writable = columns.reject { |_name, column| column.is_read_only }.keys + + expect(writable).to contain_exactly('name', 'account_id', 'email', 'emails', 'avatar_url', + 'primary_phone_number', 'portal_role', 'portal_role_id') + 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. 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 bc57d1d88..d82490d9e 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 @@ -84,12 +84,21 @@ def columns expect(collection.fields['resolution_time'].column_type).to eq('Date') end - # /issues/search exposes no 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]) + # /issues/search exposes no sort parameter. + it 'declares every column non-sortable' do expect(columns.values.map(&:is_sortable).uniq).to eq([false]) end + # Writable is what POST /issues or PATCH /issues/{id} accepts; everything + # Pylon computes itself stays read-only. + it 'declares writable exactly the columns an endpoint takes' do + writable = columns.reject { |_name, column| column.is_read_only }.keys + + expect(writable).to contain_exactly('title', 'body_html', 'state', 'type', 'tags', + 'customer_portal_visible', 'author_unverified', + 'account_id', 'requester_id', 'assignee_id', 'team_id') + 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. 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 index f9c0bf080..b8df431e8 100644 --- 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 @@ -74,14 +74,21 @@ def columns 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]) + # The order is honoured in memory over the complete dataset, so both + # scalar columns can be sorted on. + it 'declares both scalar columns sortable' do expect(columns.except('user_ids').values.map(&:is_sortable).uniq).to eq([true]) expect(collection.fields['user_ids'].is_sortable).to be(false) end + # POST /teams and PATCH /teams/{id} take the name and the members, and + # Pylon names the id itself. + it 'declares writable exactly the columns an endpoint takes' do + writable = columns.reject { |_name, column| column.is_read_only }.keys + + expect(writable).to contain_exactly('name', 'user_ids') + end + # GET /teams carries neither a search nor a filter parameter, and Pylon # exposes no count. it 'leaves search and count disabled' do 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 index c17d0dcfe..500a9cd16 100644 --- 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 @@ -83,14 +83,21 @@ def columns .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]) + # The order is honoured in memory over the complete dataset, so every + # scalar column can be sorted on. + it 'declares every scalar column sortable' do expect(columns.except('emails').values.map(&:is_sortable).uniq).to eq([true]) expect(collection.fields['emails'].is_sortable).to be(false) end + # PATCH /users/{id} takes these four and nothing else: an address is + # proven by the agent signing in, and the deactivation happens in Pylon. + it 'declares writable exactly the columns the update endpoint takes' do + writable = columns.reject { |_name, column| column.is_read_only }.keys + + expect(writable).to contain_exactly('name', 'avatar_url', 'status', 'role_id') + 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) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb new file mode 100644 index 000000000..7abcb0143 --- /dev/null +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -0,0 +1,309 @@ +module ForestAdminDatasourcePylon + RSpec.describe Collections::Writes do + def filter(condition_tree: nil, search: nil) + ForestAdminDatasourceToolkit::Components::Query::Filter.new(condition_tree: condition_tree, search: search) + end + + def leaf(field, operator, value) + 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_filter(operator, value) + filter(condition_tree: leaf('id', operator, value)) + end + + def json(payload, status = 200) + { status: status, body: payload.to_json, headers: { 'Content-Type' => 'application/json' } } + end + + def custom_field(type, slug, extra = {}) + { 'id' => "cf_#{slug}", 'slug' => slug, 'label' => slug, 'type' => type, + 'object_type' => 'issue', 'is_read_only' => false }.merge(extra) + end + + def options(*slugs) + { 'select_metadata' => { 'options' => slugs.map { |slug| { 'label' => slug.upcase, 'slug' => slug } } } } + end + + def issue_payload(id, overrides = {}) + { 'id' => id, 'number' => 12, 'title' => 'Boom', 'body_html' => '

boom

', 'state' => 'new', + 'type' => 'ticket', 'source' => 'manual', 'account' => { 'id' => 'acc-1' }, 'tags' => %w[urgent], + 'custom_fields' => {}, 'created_at' => '2026-08-07T13:06:22Z' }.merge(overrides) + end + + # A text field, a select read and written by the slug of its option, a + # multiselect Pylon takes through `values`, and one it syncs from an app and + # refuses to be written. + let(:issue_custom_fields) do + [custom_field('text', 'severity'), + custom_field('select', 'priority', options('p1', 'p2')), + custom_field('multiselect', 'regions', options('us', 'emea')), + custom_field('text', 'synced_id', 'is_read_only' => true)] + end + + let(:datasource) { Datasource.new(api_key: 'k') } + let(:base) { datasource.configuration.url } + let(:issues) { datasource.get_collection('PylonIssue') } + let(:accounts) { datasource.get_collection('PylonAccount') } + let(:contacts) { datasource.get_collection('PylonContact') } + let(:teams) { datasource.get_collection('PylonTeam') } + let(:users) { datasource.get_collection('PylonUser') } + let(:operators) { ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators } + + before { stub_custom_fields(issue: issue_custom_fields) } + + describe '#create' do + it 'posts the writable columns and answers with the serialized record' do + stub_request(:post, "#{base}/issues").to_return(json('data' => issue_payload('i1'))) + + record = issues.create(nil, 'title' => 'Boom', 'body_html' => '

boom

', 'tags' => %w[urgent], + 'account_id' => 'acc-1') + + expect(record).to include('id' => 'i1', 'title' => 'Boom', 'account_id' => 'acc-1') + expect(WebMock).to have_requested(:post, "#{base}/issues") + .with(body: { 'title' => 'Boom', 'body_html' => '

boom

', + 'tags' => %w[urgent], 'account_id' => 'acc-1' }) + end + + # The schema is the single source of truth for what may be written: a + # read-only column reaching the payload is the agent's doing, not a request + # the operator made, so it is dropped rather than refused. + it 'drops the read-only columns and the keys the schema does not know' do + stub_request(:post, "#{base}/issues").to_return(json('data' => issue_payload('i1'))) + + issues.create(nil, 'title' => 'Boom', 'id' => 'i9', 'number' => 3, 'link' => 'http://x', + 'created_at' => '2026-01-01', 'source' => 'manual', 'messages' => [], + 'number_of_touches' => 4, 'not_a_column' => 'x') + + expect(WebMock).to have_requested(:post, "#{base}/issues").with(body: { 'title' => 'Boom' }) + end + + # Pylon fills in what a create leaves out, so a form field the operator + # never touched travels as nothing at all rather than as an explicit null. + it 'drops the columns left empty' do + stub_request(:post, "#{base}/issues").to_return(json('data' => issue_payload('i1'))) + + issues.create(nil, 'title' => 'Boom', 'team_id' => nil, 'tags' => nil) + + expect(WebMock).to have_requested(:post, "#{base}/issues").with(body: { 'title' => 'Boom' }) + end + end + + describe '#create with custom fields' do + it 'writes them as a list, through `value` or `values`, and leaves the synced one out' do + stub_request(:post, "#{base}/issues").to_return(json('data' => issue_payload('i1'))) + + issues.create(nil, 'title' => 'Boom', 'severity' => 'high', 'priority' => 'p2', + 'regions' => %w[us emea], 'synced_id' => 'zzz') + + expect(WebMock).to have_requested(:post, "#{base}/issues").with( + body: { 'title' => 'Boom', + 'custom_fields' => [{ 'slug' => 'severity', 'value' => 'high' }, + { 'slug' => 'priority', 'value' => 'p2' }, + { 'slug' => 'regions', 'values' => %w[us emea] }] } + ) + end + + it 'sends no custom_fields key when none was set' do + stub_request(:post, "#{base}/issues").to_return(json('data' => issue_payload('i1'))) + + issues.create(nil, 'title' => 'Boom') + + expect(WebMock).to(have_requested(:post, "#{base}/issues").with { |req| !req.body.include?('custom_fields') }) + end + end + + # Pylon takes `state` and `type` on an update only, and Forest has one + # read-only flag per column to say so with. + describe '#create naming a field Pylon only takes on an update' do + it 'refuses the create, naming the field' do + expect { issues.create(nil, 'title' => 'Boom', 'state' => 'closed') } + .to raise_error(UnsupportedWriteError, /'state' cannot be set here on a PylonIssue/) + end + + it 'asks for nothing when the field carries no value' do + stub_request(:post, "#{base}/issues").to_return(json('data' => issue_payload('i1'))) + + issues.create(nil, 'title' => 'Boom', 'state' => nil, 'type' => '') + + expect(WebMock).to have_requested(:post, "#{base}/issues").with(body: { 'title' => 'Boom' }) + end + end + + describe '#update' do + it 'patches the record the filter names, without reading it back first' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'title' => 'Louder') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) + expect(WebMock).not_to have_requested(:get, "#{base}/issues/i1") + end + + it 'patches every record an `in` filter names' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + stub_request(:patch, "#{base}/issues/i2").to_return(json('data' => issue_payload('i2'))) + + issues.update(nil, id_filter(operators::IN, %w[i1 i2]), 'state' => 'closed') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'state' => 'closed' }) + expect(WebMock).to have_requested(:patch, "#{base}/issues/i2").with(body: { 'state' => 'closed' }) + end + + it 'sends nothing when every key of the patch is read-only' do + issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'number' => 9, 'link' => 'http://x') + + expect(WebMock).not_to have_requested(:patch, "#{base}/issues/i1") + end + + # The scope the operator's role carries rides along as an `and`, so the ids + # are resolved through the collection's own read and a record the scope + # excludes is never written to. + it 'resolves the ids through a read when the filter carries more than an id' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1', 'state' => 'closed'))) + + issues.update(nil, filter(condition_tree: branch('And', [leaf('id', operators::EQUAL, 'i1'), + leaf('state', operators::EQUAL, 'new')])), + 'title' => 'Louder') + + expect(WebMock).to have_requested(:get, "#{base}/issues/i1") + expect(WebMock).not_to have_requested(:patch, "#{base}/issues/i1") + end + end + + describe '#update naming a field Pylon only takes on a create' do + it 'refuses the update when the operator changed it' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + expect { issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'body_html' => '

louder

') } + .to raise_error(UnsupportedWriteError, /'body_html' cannot be set here on a PylonIssue/) + end + + # A form resending an untouched field asks for nothing, so the rest of the + # edit goes through rather than erroring on a value nobody changed. + it 'drops it, and writes the rest, when it holds the value already stored' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::EQUAL, 'i1'), + 'body_html' => '

boom

', 'title' => 'Louder') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) + end + end + + describe '#delete' do + it 'deletes every record the filter names' do + stub_request(:delete, "#{base}/issues/i1").to_return(status: 204) + stub_request(:delete, "#{base}/issues/i2").to_return(status: 204) + + issues.delete(nil, id_filter(operators::IN, %w[i1 i2])) + + expect(WebMock).to have_requested(:delete, "#{base}/issues/i1") + expect(WebMock).to have_requested(:delete, "#{base}/issues/i2") + end + + it 'deletes nothing when the filter matches no record' do + stub_request(:post, "#{base}/accounts/search").to_return(json('data' => [])) + + accounts.delete(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'Nope'))) + + expect(WebMock).not_to have_requested(:delete, %r{/accounts/}) + end + end + + # One request per record against a budget of ten to twenty per minute: past + # the cap the write is refused rather than applied to the first records and + # reported as done for the whole selection. + describe 'a write reaching more records than one pass covers' do + it 'refuses it before spending a single request' do + ids = Array.new(21) { |index| "i#{index}" } + + expect { issues.delete(nil, id_filter(operators::IN, ids)) } + .to raise_error(UnsupportedWriteError, /applies to 21 PylonIssue records/) + expect(WebMock).not_to have_requested(:delete, %r{/issues/}) + end + + it 'refuses it when the count only shows once the filter is resolved' do + stub_request(:post, "#{base}/accounts/search") + .to_return(json('data' => Array.new(21) { |index| { 'id' => "a#{index}", 'name' => 'Acme' } })) + + expect { accounts.delete(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'Acme'))) } + .to raise_error(UnsupportedWriteError, /applies to 21 PylonAccount records/) + expect(WebMock).not_to have_requested(:delete, %r{/accounts/}) + end + + # "Select all except these" reaches PylonIssue as `id not_in`, which its + # endpoint cannot filter: the read refuses it, and so does the delete. + it 'refuses an excluding selection on the collection that cannot filter an id' do + expect { issues.delete(nil, id_filter(operators::NOT_IN, %w[i1])) } + .to raise_error(UnsupportedOperatorError, /A filter on 'id' has to be combined with 'and'/) + end + end + + describe 'a verb Pylon exposes no endpoint for' do + it 'refuses to create a user' do + expect { users.create(nil, 'name' => 'Ada') } + .to raise_error(UnsupportedWriteError, /A PylonUser record cannot be created/) + end + + it 'refuses to delete a user' do + expect { users.delete(nil, id_filter(operators::EQUAL, 'u1')) } + .to raise_error(UnsupportedWriteError, /A PylonUser record cannot be deleted/) + end + + it 'refuses to delete a team' do + expect { teams.delete(nil, id_filter(operators::EQUAL, 't1')) } + .to raise_error(UnsupportedWriteError, /A PylonTeam record cannot be deleted/) + end + end + + describe 'the collections read through their own endpoints' do + it 'writes an account type under the name Pylon takes it as' do + stub_request(:post, "#{base}/accounts").to_return(json('data' => { 'id' => 'a1', 'name' => 'Acme' })) + + accounts.create(nil, 'name' => 'Acme', 'type' => 'customer', 'domains' => %w[acme.test]) + + expect(WebMock).to have_requested(:post, "#{base}/accounts") + .with(body: { 'name' => 'Acme', 'account_type' => 'customer', 'domains' => %w[acme.test] }) + end + + it 'refuses to disable an account that does not exist yet' do + expect { accounts.create(nil, 'name' => 'Acme', 'is_disabled' => true) } + .to raise_error(UnsupportedWriteError, /'is_disabled' cannot be set here on a PylonAccount/) + end + + it 'patches a contact' do + stub_request(:patch, "#{base}/contacts/c1").to_return(json('data' => { 'id' => 'c1', 'name' => 'Ada' })) + + contacts.update(nil, id_filter(operators::EQUAL, 'c1'), 'name' => 'Ada', 'account_id' => 'a1') + + expect(WebMock).to have_requested(:patch, "#{base}/contacts/c1") + .with(body: { 'name' => 'Ada', 'account_id' => 'a1' }) + end + + it 'replaces the members of a team' do + stub_request(:patch, "#{base}/teams/t1").to_return(json('data' => { 'id' => 't1', 'name' => 'Support' })) + + teams.update(nil, id_filter(operators::EQUAL, 't1'), 'name' => 'Support', 'user_ids' => %w[u1 u2]) + + expect(WebMock).to have_requested(:patch, "#{base}/teams/t1") + .with(body: { 'name' => 'Support', 'user_ids' => %w[u1 u2] }) + end + + it 'patches the status of a user' do + stub_request(:patch, "#{base}/users/u1").to_return(json('data' => { 'id' => 'u1', 'name' => 'Ada' })) + + users.update(nil, id_filter(operators::EQUAL, 'u1'), 'status' => 'away', 'email' => 'ada@acme.test') + + expect(WebMock).to have_requested(:patch, "#{base}/users/u1").with(body: { 'status' => 'away' }) + end + end + end +end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb index 975864321..f84a4c986 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb @@ -162,12 +162,12 @@ def operators_of(type, **extra) end end - # Writes land in story 7 (EXT-11), which is also where Pylon's own - # `is_read_only` starts being honoured; no endpoint sorts, ever. + # No endpoint sorts, ever; read-only is Pylon's own call, which it makes for + # the fields an app or an integration syncs. describe 'the schema every custom field gets' do - it 'is read-only and unsortable, whatever Pylon declares' do + it 'is unsortable, and read-only when Pylon says so' do allow(client).to receive(:fetch_custom_fields).with('issue') - .and_return([definition('text', 'is_read_only' => false)]) + .and_return([definition('text', 'is_read_only' => true)]) schema = introspector.issue_custom_fields.first[:schema] @@ -175,6 +175,22 @@ def operators_of(type, **extra) expect(schema.is_sortable).to be(false) end + it 'is writable when Pylon declares the field editable' do + allow(client).to receive(:fetch_custom_fields).with('issue') + .and_return([definition('text', 'is_read_only' => false)]) + + expect(introspector.issue_custom_fields.first[:schema].is_read_only).to be(false) + end + + # Anything other than a true flag reads as editable, which is what a + # definition predating the flag is. + it 'is writable when Pylon declares nothing' do + allow(client).to receive(:fetch_custom_fields).with('issue') + .and_return([definition('text', 'is_read_only' => nil)]) + + expect(introspector.issue_custom_fields.first[:schema].is_read_only).to be(false) + end + # `ColumnSchema` defaults this one to true, and the capabilities route turns # `supportGroups` on as soon as a single field carries it: one custom field # left groupable is the whole collection offering a chart `aggregate` raises @@ -192,8 +208,20 @@ def operators_of(type, **extra) allow(client).to receive(:fetch_custom_fields).with('issue') .and_return([definition('text', slug: 'sev_level')]) - expect(introspector.issue_custom_fields.first.keys).to eq(%i[column_name schema]) + expect(introspector.issue_custom_fields.first.keys).to eq(%i[column_name schema multi_value]) expect(introspector.issue_custom_fields.first[:column_name]).to eq('sev_level') end + + # Pylon writes a multiselect back through `values` and every other type + # through `value`, so the payload builder is told which one this is rather + # than guessing it from the Json column type. + it 'flags a multiselect as multi-valued, and nothing else' do + allow(client).to receive(:fetch_custom_fields).with('issue') + .and_return([definition('multiselect', **select_metadata('p1')), + definition('select', **select_metadata('p1')), + definition('text')]) + + expect(introspector.issue_custom_fields.map { |cf| cf[:multi_value] }).to eq([true, false, false]) + end end end From 89b75cd605724ae2c0890446356f19115a31354f Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Thu, 20 Aug 2026 14:25:40 +0200 Subject: [PATCH 2/8] fix(pylon): review findings on the write path Six findings from the review of the CRUD writes: - a filter-driven update or delete failing on the k-th record left the k-1 before it written while reporting the whole write as failed; the loop now raises PartialWriteError naming what landed and what to retry - a PATCH answered with 204, an empty body or a null "data" raised after the write had landed, aborting the rest of a bulk edit; only a "data" carrying something other than a record is a broken contract now - false and empty collections counted as a changed value, so a create naming an update-only boolean left unchecked was refused - the stored value of a wrong-direction field was read by re-running the caller's filter, duplicating the resolution and walking the whole matching dataset for want of a page; it reads the resolved ids instead - PylonContact declared both projections of a role and of an address writable, one of them going stale in the payload - the write cap was applied to the ids a filter names, which asserts nothing about its sibling conditions; a new max_resolvable_ids hook bounds what the resolution can read exactly, and the count is reported as records named rather than records written to Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/forest_admin_datasource_pylon.rb | 7 + .../client/writes.rb | 21 ++- .../collections/contact.rb | 4 +- .../collections/contact/schema_definition.rb | 20 ++- .../collections/issue.rb | 11 +- .../collections/writes.rb | 124 +++++++++++++--- .../client/writes_spec.rb | 17 +++ .../collections/contact_spec.rb | 6 +- .../collections/writes_spec.rb | 135 ++++++++++++++++++ 9 files changed, 309 insertions(+), 36 deletions(-) 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 9a291e5de..fa06a3a2c 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 @@ -27,6 +27,13 @@ class UnsupportedOperatorError < ForestAdminDatasourceToolkit::Exceptions::Valid # undo, and the message is the only place they learn what. class UnsupportedWriteError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end + # A filter-driven write Pylon performed on some of its records and then + # failed on: one record is one request, so the ones before the failure are + # written and stay written. Descends from ValidationError so the operator + # reads which records landed rather than a 500 leaving them to guess — a + # retry of the whole selection would write those a second time. + class PartialWriteError < 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 # surface Pylon's own validation message instead of a generic string. diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb index c38afa402..b2b8caa86 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb @@ -43,7 +43,7 @@ def patch_resource(resource, id, attributes) path = "#{resource}/#{Faraday::Utils.escape(id)}" operation = "update(#{path})" - must_succeed(operation) { extract_written(connection.patch(path, attributes).body, operation) } + must_succeed(operation) { extract_updated(connection.patch(path, attributes).body, operation) } end # Answers true rather than the body: Pylon returns 200 or 204 with nothing @@ -66,8 +66,25 @@ def extract_written(body, operation) record = body['data'] if body.is_a?(Hash) return record if record.is_a?(Hash) + refuse_body_shape(body, operation, "missing 'data'") + end + + # An update is answered the same way, but its record is never read back: + # the collection discards it. So a 204, an empty body or a null `data` is + # the write having landed with nothing to hand back, and raising there + # would report a failure on a record Pylon already patched — and abort the + # records a bulk edit had left to write. Only a `data` carrying something + # that is not a record means the contract broke. + def extract_updated(body, operation) + record = body['data'] if body.is_a?(Hash) + return record if record.nil? || record.is_a?(Hash) + + refuse_body_shape(body, operation, "'data' is not a record") + end + + def refuse_body_shape(body, operation, detail) raise APIError, - "Pylon API #{operation} returned an unexpected body shape (missing 'data'): #{body.inspect}" + "Pylon API #{operation} returned an unexpected body shape (#{detail}): #{body.inspect}" end end end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact.rb index 314fdb7b9..3799e615e 100644 --- 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 @@ -5,7 +5,8 @@ class Contact < CursorCollection include Serializer # `POST /contacts` takes the primary address alone; the other ones are set - # on an existing contact. + # on an existing contact, through the list. + CREATE_ONLY = %w[email].freeze UPDATE_ONLY = %w[emails].freeze def initialize(datasource, custom_fields: []) @@ -20,6 +21,7 @@ def create_record(payload) = datasource.client.create_contact(payload) def update_record(id, payload) = datasource.client.update_contact(id, payload) def delete_record(id) = datasource.client.delete_contact(id) + def create_only_fields = CREATE_ONLY def update_only_fields = UPDATE_ONLY def unsortable_warning diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb index 3d11aa061..1a71ad06b 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb @@ -60,10 +60,15 @@ def define_identity_fields # `email` and `primary_phone_number` carry the primary value; the lists # hold every address and number, and neither list is filterable. # - # `emails` is writable on an update only, `POST /contacts` taking the - # primary address alone. `phone_numbers` is not writable at all: it - # holds objects, and the shape the endpoint takes them in is not the one - # the column shows. + # `email` is written on a create and `emails` on an update, one + # direction each: `POST /contacts` takes the primary address alone, and + # the other ones are set on an existing contact. Two writable + # projections of the same addresses would otherwise travel in one patch, + # the list leaving out whatever the primary carries — the reason + # PylonAccount keeps `domain` read-only next to `domains`. + # + # `phone_numbers` is not writable at all: it holds objects, and the + # shape the endpoint takes them in is not the one the column shows. def define_contact_fields add_column('email', 'String', writable: true) add_column('emails', 'Json', writable: true) @@ -75,8 +80,11 @@ def define_contact_fields 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', writable: true) + # is what `portal_role_id` points at. The id is the one written and + # the name is read-only, like `role_id` and `role_name` on PylonUser: + # writing both would carry two projections of one role in the same + # patch, and whichever Pylon ignored would come back stale. + add_column('portal_role', 'String') add_column('portal_role_id', 'String', writable: true) # Owned by the integrations the contact was seen through; no endpoint # takes it. 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 3faccb5ef..f83f83184 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 @@ -78,12 +78,15 @@ def delete_record(id) = datasource.client.delete_issue(id) def create_only_fields = CREATE_ONLY def update_only_fields = UPDATE_ONLY - # Never past the primary-key fan-out: a write resolving its ids through - # `list` goes through `fetch_by_ids`, which truncates with a warning, and - # a truncated resolution would write to a subset of the selection while - # reporting the whole of it. def max_write_targets = [Writes::MAX_WRITE_TARGETS, MAX_ID_LOOKUPS].min + # Never past the primary-key fan-out: an issue is read by its own + # endpoint, so a write resolving named ids through `list` goes through + # `fetch_by_ids`, which truncates with a warning past this many, and a + # truncated resolution would write to a subset of the selection while + # reporting the whole of it. + def max_resolvable_ids = MAX_ID_LOOKUPS + def sortable_fields PYLON_SORTABLE end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb index 1e67e95c7..fd96ab01f 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -22,6 +22,8 @@ module Writes # rubocop:disable Metrics/ModuleLength Filter = ForestAdminDatasourceToolkit::Components::Query::Filter Page = ForestAdminDatasourceToolkit::Components::Query::Page Projection = ForestAdminDatasourceToolkit::Components::Query::Projection + Leaf = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators # How many records one filter-driven update or delete may reach. Pylon # writes one record per request against a budget of 10 to 20 requests per @@ -38,14 +40,14 @@ def update(caller, filter, patch) ids = ids_for(caller, filter) return if ids.empty? - payload = build_payload(patch, :update, caller: caller, filter: filter) + payload = build_payload(patch, :update, caller: caller, ids: ids) return if payload.empty? - ids.each { |id| update_record(id, payload) } + write_each(ids, 'updated') { |id| update_record(id, payload) } end def delete(caller, filter) - ids_for(caller, filter).each { |id| delete_record(id) } + write_each(ids_for(caller, filter), 'deleted') { |id| delete_record(id) } end protected @@ -66,6 +68,14 @@ def payload_renames = {}.freeze def max_write_targets = MAX_WRITE_TARGETS + # How many named ids the collection's own read can resolve exactly. No + # bound by default: the search endpoint filters `id` server-side, so any + # id list is answered by one request per chunk. The collection resolving a + # named id by its own endpoint overrides this with its fan-out cap — past + # it the read truncates, and a truncated resolution would write to part of + # the selection while reporting the whole of it. + def max_resolvable_ids = Float::INFINITY + # The records a filter-driven write applies to: exact, or refused. Nothing # here may quietly answer with a subset — the caller writes one request per # id and reports success for the whole selection. @@ -78,15 +88,37 @@ def max_write_targets = MAX_WRITE_TARGETS # so the scope applies and the endpoint filters what it can. def ids_for(caller, filter) tree = filter&.condition_tree - ids = filtered_ids(tree) - refuse_too_many_targets(ids.size) if ids && ids.size > max_write_targets - return ids if ids && id_values(tree) && no_search?(filter) + if (named = id_values(tree)) && no_search?(filter) + refuse_too_many_targets(named.size) if named.size > max_write_targets + return named + end + named_count = filtered_ids(tree)&.size + refuse_unresolvable_selection(named_count) if named_count && named_count > max_resolvable_ids resolve_ids_by_list(caller, filter) end private + # One request per record, so a failure on the k-th record leaves the k-1 + # before it written — the cap bounds how many records a write reaches, + # nothing bounds the endpoint answering 429 or 422 halfway through. The + # error names the records that landed: raising the API error alone reads + # as "the write failed, nothing happened", and retrying the selection on + # that reading would write them a second time. + def write_each(ids, verb) + written = [] + + ids.each do |id| + yield id + written << id + rescue APIError => e + raise if written.empty? + + refuse_partial_write(verb, written, id, ids.size, e) + end + end + # One record past the cap is asked for, so an overflow is seen rather than # guessed from a full page — the same bound `foreign_keys_matching` puts on # a resolved relation condition. @@ -104,8 +136,9 @@ def resolve_ids_by_list(caller, filter) # what the rest of the tree can be applied in memory: the leftovers travel # to `list`, which answers them the way a read does — server-side where the # endpoint filters `id`, through the primary-key short-circuit where it - # does not. What this answers is only "how many records is this write - # about", which is the question the cap needs. + # does not. Nothing is asserted about the sibling conditions either, so + # this is a count of records *named*, never of records the write applies + # to: it answers what `max_resolvable_ids` needs, not what the cap does. def filtered_ids(node) return id_values(node) if id_values(node) return nil unless and_branch?(node) @@ -118,9 +151,9 @@ def filtered_ids(node) # the endpoint takes. Everything else is dropped rather than refused: the # front sends the fields of its form, and a read-only one reaching the # payload is the agent's doing, not a request the operator made. - def build_payload(data, direction, caller: nil, filter: nil) + def build_payload(data, direction, caller: nil, ids: []) attrs = writable_attributes(data) - attrs = honour_write_direction(attrs, direction, caller, filter) + attrs = honour_write_direction(attrs, direction, caller, ids) # Pylon fills in what a create leaves out; on an update a nil is the # operator clearing a value, so it travels. attrs = attrs.compact if direction == :create @@ -149,26 +182,54 @@ def writable_column?(field) # operator really changed it: Pylon cannot write it, and answering the edit # with a success it did not perform is worse than an error naming the # field. - def honour_write_direction(attrs, direction, caller, filter) + def honour_write_direction(attrs, direction, caller, ids) wrong = attrs.keys & (direction == :create ? update_only_fields : create_only_fields) return attrs if wrong.empty? - stored = direction == :update ? stored_values(caller, filter, wrong) : [] - wrong.each do |field| - value = attrs[field] - next if value.nil? || value == '' - next if direction == :update && stored.all? { |record| record[field] == value } - - refuse_wrong_direction(field, direction) - end + asked = wrong.reject { |field| blank_write_value?(attrs[field]) } + asked -= unchanged_fields(caller, ids, asked, attrs) if direction == :update + asked.each { |field| refuse_wrong_direction(field, direction) } attrs.except(*wrong) end + # What a form sends for a field the operator never touched: no value at + # all, an unchecked box, an empty list. Pylon fills a create in with + # exactly this, and on an existing record it is the state a stored nothing + # already reads as — so asking for it is asking for nothing, where a `0` + # or a string is a value only the other endpoint could write. + def blank_write_value?(value) + return true if value.nil? || value == false + return value.empty? if value.respond_to?(:empty?) + + false + end + + # The wrong-direction fields already holding the value the patch asks for. + # An unreadable record counts as none of them: the field is refused rather + # than dropped, since nothing here may claim a value is unchanged without + # having read it. + def unchanged_fields(caller, ids, fields, attrs) + return [] if fields.empty? + + stored = stored_values(caller, ids, fields) + return [] if stored.empty? + + fields.select { |field| stored.all? { |record| record[field] == attrs[field] } } + end + # Read only when a field of the wrong direction carries a value, and only # for that field: an update naming none costs no request at all. - def stored_values(caller, filter, fields) - list(caller, filter, Projection.new(['id'] + fields)) + # + # Read by id rather than through the caller's filter: the filter was + # already resolved into these ids, so re-running it would spend those + # requests a second time and — carrying no page of its own — walk every + # record it matches rather than the handful about to be written. + def stored_values(caller, ids, fields) + query = Filter.new(condition_tree: Leaf.new('id', Operators::IN, ids), + page: Page.new(offset: 0, limit: ids.size)) + + list(caller, query, Projection.new(['id'] + fields)) end # Pylon reads its custom fields back as a map indexed by slug and writes @@ -217,6 +278,27 @@ def refuse_too_many_targets(count) 'minute, and a write stopping halfway would report a success it did not perform. Narrow the ' \ 'selection to reach the records past this point.' end + + # Named ids the collection cannot resolve exactly, the filter carrying + # more than the ids themselves. How many of them the rest of the filter + # matches is unknown here — it is what the read would answer — so the + # count is reported as what it is, records named rather than records + # written to. + def refuse_unresolvable_selection(count) + raise UnsupportedWriteError, + "This write names #{count} #{name} records and filters them further, which #{name} answers with " \ + "one request per named record, more than the #{max_resolvable_ids} one pass reads: the resolution " \ + 'would stop short and the write would then cover part of the selection while reporting all of ' \ + 'it. Select fewer records, or drop the other conditions to write the ones named.' + end + + def refuse_partial_write(verb, written, failed_id, total, error) + raise PartialWriteError, + "#{written.size} of #{total} #{name} records were #{verb} and then '#{failed_id}' failed: " \ + "#{error.message}. The records already #{verb} are #{written.join(", ")}, and they stay " \ + "#{verb} — the ones after them were left untouched. Retry the write on the untouched records " \ + "alone: retrying the whole selection would perform it twice on the ones already #{verb}." + end end end end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb index d77719bbe..c55f314b5 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb @@ -69,6 +69,23 @@ def json(payload, status = 200) expect { client.update_issue('i1', 'title' => 'Boom') } .to raise_error(ForestAdminDatasourcePylon::APIError, %r{update\(issues/i1\)}) end + + # An update's record is discarded by the collection, so an answer carrying + # none is the write having landed with nothing to hand back — raising there + # would report a failure on a record Pylon already patched. + it 'accepts an update answered with no body at all' do + stub_request(:patch, "#{base}/issues/i1").to_return(status: 204) + + expect(client.update_issue('i1', 'title' => 'Boom')).to be_nil + end + + it 'accepts an update answered without a record' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => nil, 'request_id' => 'req_1')) + stub_request(:patch, "#{base}/issues/i2").to_return(json('request_id' => 'req_2')) + + expect(client.update_issue('i1', 'title' => 'Boom')).to be_nil + expect(client.update_issue('i2', 'title' => 'Boom')).to be_nil + end end describe 'a failed write' 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 index fd939a058..cde96b091 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/contact_spec.rb @@ -113,12 +113,14 @@ def stub_search(payload = { 'data' => [contact_payload('con-1')] }) # `phone_numbers` and `external_ids` stay read-only although the endpoint # takes them: they hold objects, in a shape the write side does not - # document as the one the column shows. + # document as the one the column shows. `portal_role` stays read-only next + # to the `portal_role_id` it is the name of, so one patch never carries two + # projections of the same role. it 'declares writable exactly the columns an endpoint takes in the shape they are read' do writable = columns.reject { |_name, column| column.is_read_only }.keys expect(writable).to contain_exactly('name', 'account_id', 'email', 'emails', 'avatar_url', - 'primary_phone_number', 'portal_role', 'portal_role_id') + 'primary_phone_number', 'portal_role_id') end # No Pylon endpoint aggregates, and the pages of a cursor walk are not the diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb index 7abcb0143..17f57ebef 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -177,6 +177,53 @@ def issue_payload(id, overrides = {}) end end + # The record a write answers with is discarded here, so a patch Pylon + # answers with no body at all wrote the record just the same — and the rest + # of the selection is written rather than aborted on it. + describe '#update answered with no record' do + it 'writes every record of the selection' do + stub_request(:patch, "#{base}/issues/i1").to_return(status: 204) + stub_request(:patch, "#{base}/issues/i2").to_return(json('data' => nil)) + + issues.update(nil, id_filter(operators::IN, %w[i1 i2]), 'state' => 'closed') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1") + expect(WebMock).to have_requested(:patch, "#{base}/issues/i2") + end + end + + # One record is one request, so a failure on the k-th leaves the k-1 before + # it written and written for good: the error names them, where the API error + # alone would read as "the write failed, nothing happened" and a retry of the + # whole selection would write them twice. + describe 'a write failing partway through the selection' do + it 'names the records already written' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + stub_request(:patch, "#{base}/issues/i2").to_return(json({ 'message' => 'state is invalid' }, 422)) + + expect { issues.update(nil, id_filter(operators::IN, %w[i1 i2]), 'state' => 'closed') } + .to raise_error(PartialWriteError, /1 of 2 PylonIssue records were updated and then 'i2' failed/) + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1") + end + + it 'names the records already deleted' do + stub_request(:delete, "#{base}/issues/i1").to_return(status: 204) + stub_request(:delete, "#{base}/issues/i2").to_return(json({ 'message' => 'gone' }, 404)) + + expect { issues.delete(nil, id_filter(operators::IN, %w[i1 i2])) } + .to raise_error(PartialWriteError, /records already deleted are i1/) + end + + # Nothing was written, so the failure is the whole of what happened and + # travels as the error Pylon answered with. + it 'raises the API error itself when the first record failed' do + stub_request(:delete, "#{base}/issues/i1").to_return(json({ 'message' => 'gone' }, 404)) + + expect { issues.delete(nil, id_filter(operators::IN, %w[i1 i2])) } + .to raise_error(APIError, %r{delete\(issues/i1\)}) + end + end + describe '#update naming a field Pylon only takes on a create' do it 'refuses the update when the operator changed it' do stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) @@ -196,6 +243,34 @@ def issue_payload(id, overrides = {}) expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) end + + # An unchecked box is not an edit of the field: dropping it costs no read + # at all, where refusing it would fail every edit whose form carries one. + it 'drops a boolean left false without reading the record back' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'author_unverified' => false, 'title' => 'Louder') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) + expect(WebMock).not_to have_requested(:get, "#{base}/issues/i1") + end + + # The filter was already resolved into ids, so reading it again would spend + # the same requests twice and, carrying no page of its own, walk every + # record it matches instead of the one about to be written. + it 'reads the stored value by id rather than running the filter a second time' do + contact = { 'id' => 'c1', 'name' => 'Ada', 'email' => 'ada@acme.test' } + stub_request(:post, "#{base}/contacts/search").to_return(json('data' => [contact])) + stub_request(:get, "#{base}/contacts/c1").to_return(json('data' => contact)) + stub_request(:patch, "#{base}/contacts/c1").to_return(json('data' => contact)) + + contacts.update(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'Ada')), + 'email' => 'ada@acme.test', 'avatar_url' => 'http://x') + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").once + expect(WebMock).to have_requested(:get, "#{base}/contacts/c1") + expect(WebMock).to have_requested(:patch, "#{base}/contacts/c1").with(body: { 'avatar_url' => 'http://x' }) + end end describe '#delete' do @@ -239,6 +314,37 @@ def issue_payload(id, overrides = {}) expect(WebMock).not_to have_requested(:delete, %r{/accounts/}) end + # The ids a filter names are not the records the write applies to when the + # filter narrows them further: the collections filtering `id` server-side + # learn the real count in one request, and write it. + it 'writes the records a narrowed selection really matches' do + stub_request(:post, "#{base}/accounts/search") + .to_return(json('data' => [{ 'id' => 'a1', 'name' => 'Acme' }])) + stub_request(:patch, "#{base}/accounts/a1").to_return(json('data' => { 'id' => 'a1' })) + + ids = Array.new(25) { |index| "a#{index}" } + accounts.update(nil, filter(condition_tree: branch('And', [leaf('id', operators::IN, ids), + leaf('name', operators::EQUAL, 'Acme')])), + 'name' => 'Acme Inc') + + expect(WebMock).to have_requested(:patch, "#{base}/accounts/a1").with(body: { 'name' => 'Acme Inc' }) + end + + # An issue is read one request per named id, so past the fan-out the + # resolution would stop short and the write would cover part of the + # selection. Refused — as the ids it names, never as records it was found + # to apply to. + it 'refuses more named ids than the collection can resolve, without claiming they all match' do + ids = Array.new(25) { |index| "i#{index}" } + + expect do + issues.update(nil, filter(condition_tree: branch('And', [leaf('id', operators::IN, ids), + leaf('state', operators::EQUAL, 'new')])), + 'title' => 'Louder') + end.to raise_error(UnsupportedWriteError, /names 25 PylonIssue records and filters them further/) + expect(WebMock).not_to have_requested(:get, %r{/issues/}) + end + # "Select all except these" reaches PylonIssue as `id not_in`, which its # endpoint cannot filter: the read refuses it, and so does the delete. it 'refuses an excluding selection on the collection that cannot filter an id' do @@ -279,6 +385,35 @@ def issue_payload(id, overrides = {}) .to raise_error(UnsupportedWriteError, /'is_disabled' cannot be set here on a PylonAccount/) end + # An account is created enabled, which is what the form asks for when the + # box is left unchecked: the create it produces is the one requested. + it 'creates an account whose update-only boolean is left false' do + stub_request(:post, "#{base}/accounts").to_return(json('data' => { 'id' => 'a1', 'name' => 'Acme' })) + + accounts.create(nil, 'name' => 'Acme', 'is_disabled' => false) + + expect(WebMock).to have_requested(:post, "#{base}/accounts").with(body: { 'name' => 'Acme' }) + end + + # `POST /contacts` takes the primary address and `PATCH /contacts/{id}` the + # list, so one payload never carries both projections of the addresses. + it 'creates a contact with its primary address' do + stub_request(:post, "#{base}/contacts").to_return(json('data' => { 'id' => 'c1', 'name' => 'Ada' })) + + contacts.create(nil, 'name' => 'Ada', 'email' => 'ada@acme.test', 'emails' => []) + + expect(WebMock).to have_requested(:post, "#{base}/contacts") + .with(body: { 'name' => 'Ada', 'email' => 'ada@acme.test' }) + end + + it 'refuses to change the primary address of an existing contact' do + stub_request(:get, "#{base}/contacts/c1") + .to_return(json('data' => { 'id' => 'c1', 'name' => 'Ada', 'email' => 'ada@acme.test' })) + + expect { contacts.update(nil, id_filter(operators::EQUAL, 'c1'), 'email' => 'new@acme.test') } + .to raise_error(UnsupportedWriteError, /'email' cannot be set here on a PylonContact/) + end + it 'patches a contact' do stub_request(:patch, "#{base}/contacts/c1").to_return(json('data' => { 'id' => 'c1', 'name' => 'Ada' })) From 02a16d4b93710672d73e28593f69249fee23c302 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Thu, 20 Aug 2026 15:42:33 +0200 Subject: [PATCH 3/8] fix(pylon): second review pass on the write path - a wrong-direction field left false or empty was dropped on an update without reading the record, so unchecking a stored `true` -- or clearing a stored body -- reported an edit Pylon never performed. Blankness now settles a create only; an update compares against the stored value, two blanks counting as the same state so an unchecked box over a null is still nothing to write - the stored/asked comparison strips strings: an editor handing back the markup it was given re-indented refused an edit nobody made, naming a field the operator never touched - a patch naming nothing writable is settled before the ids are, so it no longer spends the resolution read nor gets refused for reaching more records than one pass covers - every field of the wrong direction is named in one message rather than the first one only - `id not_in`, which is what selecting every record except a few sends, was answered with advice about rewriting a filter with `and` that the operator never wrote; the message names the exclusion - max_resolvable_ids defaults to nil rather than Float::INFINITY, which the refusal message would have printed verbatim - filtered_ids stops calling id_values three times per node, and the custom-field index is built once per collection rather than per payload Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/base_collection.rb | 18 ++- .../collections/writes.rb | 129 ++++++++++++------ .../collections/base_collection_spec.rb | 2 +- .../collections/writes_spec.rb | 70 +++++++++- 4 files changed, 164 insertions(+), 55 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/base_collection.rb index 166a80ee6..a073db76e 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 @@ -354,9 +354,13 @@ def and_branch?(node) # An `id` the short-circuit could not take out of the tree has no # 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". + # would bring in records the lookup never fetched. Worth an error an + # operator can act on rather than the translator's "add it to api_filters", + # because two things they do reach it: the `id equals` filter next to the + # or/and toggle, and — through the write path — an excluding selection, + # "select every record except these", which arrives as `id not_in` and + # names the records to leave out rather than the ones to read. The message + # names both, an exclusion being no filter the operator wrote. # # A collection whose endpoint does filter id declares it in `api_filters` # and never short-circuits, so the translator handles its ids like any @@ -366,9 +370,11 @@ def ensure_no_stray_id!(node) return unless node.some_leaf { |leaf| leaf.field == 'id' } raise UnsupportedOperatorError, - "A filter on 'id' has to be combined with 'and' conditions only: Pylon cannot filter on id, so the " \ - 'agent reads the records by id and applies the rest in memory, which an id inside an `or` would ' \ - 'silently widen. Rewrite the filter with `and`, or filter on another field.' + "#{name} cannot answer this selection: Pylon cannot filter on id, so the agent reads the records " \ + 'by id and applies the rest in memory, which only an `and` of `id equals` / `id in` conditions ' \ + 'names a set of records to read. An id inside an `or` names none, and neither does an exclusion, ' \ + 'which is what selecting every record except a few sends. Select the records to act on rather ' \ + 'than the ones to leave out, rewrite the filter with `and`, or filter on another field.' end def resolve_relation_conditions(caller, node) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb index fd96ab01f..7bf8262f0 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -19,6 +19,9 @@ module Collections # Long by line count only: three verbs, the payload they share, and the # refusals naming what Pylon cannot do. module Writes # rubocop:disable Metrics/ModuleLength + # Re-declared rather than borrowed from BaseCollection: a method defined + # here resolves a constant against this module and its ancestors, never + # against the class including it. Filter = ForestAdminDatasourceToolkit::Components::Query::Filter Page = ForestAdminDatasourceToolkit::Components::Query::Page Projection = ForestAdminDatasourceToolkit::Components::Query::Projection @@ -33,14 +36,21 @@ module Writes # rubocop:disable Metrics/ModuleLength MAX_WRITE_TARGETS = 20 def create(_caller, data) - serialize(create_record(build_payload(data, :create))) + serialize(create_record(build_payload(writable_attributes(data), :create))) end + # What the patch may write is settled before the ids are: a patch naming + # nothing writable sends no request and, more to the point, is not refused + # for reaching too many records — the cap bounds a write, and there is + # none here. def update(caller, filter, patch) + attributes = writable_attributes(patch) + return if attributes.empty? + ids = ids_for(caller, filter) return if ids.empty? - payload = build_payload(patch, :update, caller: caller, ids: ids) + payload = build_payload(attributes, :update, caller: caller, ids: ids) return if payload.empty? write_each(ids, 'updated') { |id| update_record(id, payload) } @@ -68,13 +78,13 @@ def payload_renames = {}.freeze def max_write_targets = MAX_WRITE_TARGETS - # How many named ids the collection's own read can resolve exactly. No - # bound by default: the search endpoint filters `id` server-side, so any - # id list is answered by one request per chunk. The collection resolving a - # named id by its own endpoint overrides this with its fan-out cap — past - # it the read truncates, and a truncated resolution would write to part of - # the selection while reporting the whole of it. - def max_resolvable_ids = Float::INFINITY + # How many named ids the collection's own read can resolve exactly. `nil` + # is no bound at all, the default: the search endpoint filters `id` + # server-side, so any id list is answered by one request per chunk. The + # collection resolving a named id by its own endpoint overrides this with + # its fan-out cap — past it the read truncates, and a truncated resolution + # would write to part of the selection while reporting the whole of it. + def max_resolvable_ids = nil # The records a filter-driven write applies to: exact, or refused. Nothing # here may quietly answer with a subset — the caller writes one request per @@ -93,7 +103,7 @@ def ids_for(caller, filter) return named end - named_count = filtered_ids(tree)&.size + named_count = max_resolvable_ids && filtered_ids(tree)&.size refuse_unresolvable_selection(named_count) if named_count && named_count > max_resolvable_ids resolve_ids_by_list(caller, filter) end @@ -140,20 +150,16 @@ def resolve_ids_by_list(caller, filter) # this is a count of records *named*, never of records the write applies # to: it answers what `max_resolvable_ids` needs, not what the cap does. def filtered_ids(node) - return id_values(node) if id_values(node) + named = id_values(node) + return named if named return nil unless and_branch?(node) - condition = Array(node.conditions).find { |child| id_values(child) } - condition && id_values(condition) + Array(node.conditions).filter_map { |child| id_values(child) }.first end - # Keys the schema declares writable, custom fields included, in the shape - # the endpoint takes. Everything else is dropped rather than refused: the - # front sends the fields of its form, and a read-only one reaching the - # payload is the agent's doing, not a request the operator made. - def build_payload(data, direction, caller: nil, ids: []) - attrs = writable_attributes(data) - attrs = honour_write_direction(attrs, direction, caller, ids) + # The writable attributes, in the shape the endpoint takes them. + def build_payload(attributes, direction, caller: nil, ids: []) + attrs = honour_write_direction(attributes, direction, caller, ids) # Pylon fills in what a create leaves out; on an update a nil is the # operator clearing a value, so it travels. attrs = attrs.compact if direction == :create @@ -164,6 +170,10 @@ def build_payload(data, direction, caller: nil, ids: []) payload end + # Keys the schema declares writable, custom fields included. Everything + # else is dropped rather than refused: the front sends the fields of its + # form, and a read-only one reaching the payload is the agent's doing, not + # a request the operator made. def writable_attributes(data) attrs = data.is_a?(Hash) ? data.transform_keys(&:to_s) : {} @@ -176,28 +186,35 @@ def writable_column?(field) column&.type == 'Column' && !column.is_read_only end - # A field of the other direction is dropped when it asks for nothing — no - # value at all, or, on an update, the value the record already holds, which - # is what a form resending an untouched field sends. It is refused when the - # operator really changed it: Pylon cannot write it, and answering the edit - # with a success it did not perform is worse than an error naming the - # field. + # A field of the other direction is dropped when it asks for nothing, and + # refused when the operator really changed it: Pylon cannot write it, and + # answering the edit with a success it did not perform is worse than an + # error naming the field. + # + # What "asks for nothing" means differs by direction, and only a create + # can tell without reading. Pylon fills a create in with exactly what a + # blank value asks for, so a blank one is dropped there. On an update the + # record already holds a value, and the only thing that settles whether + # the patch changes it is that value: an unchecked box is nothing to write + # over a stored `false`, and a real edit over a stored `true`. Blankness + # alone would drop the second, reporting an edit Pylon never performed. def honour_write_direction(attrs, direction, caller, ids) wrong = attrs.keys & (direction == :create ? update_only_fields : create_only_fields) return attrs if wrong.empty? - asked = wrong.reject { |field| blank_write_value?(attrs[field]) } - asked -= unchanged_fields(caller, ids, asked, attrs) if direction == :update - asked.each { |field| refuse_wrong_direction(field, direction) } + asked = if direction == :create + wrong.reject { |field| blank_write_value?(attrs[field]) } + else + wrong - unchanged_fields(caller, ids, wrong, attrs) + end + refuse_wrong_direction(asked, direction) unless asked.empty? attrs.except(*wrong) end # What a form sends for a field the operator never touched: no value at - # all, an unchecked box, an empty list. Pylon fills a create in with - # exactly this, and on an existing record it is the state a stored nothing - # already reads as — so asking for it is asking for nothing, where a `0` - # or a string is a value only the other endpoint could write. + # all, an unchecked box, an empty list. A `0` or a string is a value only + # the other endpoint could write. def blank_write_value?(value) return true if value.nil? || value == false return value.empty? if value.respond_to?(:empty?) @@ -215,11 +232,31 @@ def unchanged_fields(caller, ids, fields, attrs) stored = stored_values(caller, ids, fields) return [] if stored.empty? - fields.select { |field| stored.all? { |record| record[field] == attrs[field] } } + fields.select { |field| stored.all? { |record| same_write_value?(record[field], attrs[field]) } } + end + + # Whether the patch asks for the value the record already holds. + # + # Two blanks are the same state: Pylon returns a null where the form sends + # `false` or an empty string for the same untouched field, and refusing + # that pair would fail every edit whose form carries one. + # + # Strings are compared stripped: `body_html` travels through an editor + # that may hand back the markup it was given re-indented, and refusing an + # edit nobody made — naming a field the operator never touched — is the + # one error they cannot act on. + def same_write_value?(stored, asked) + return true if blank_write_value?(stored) && blank_write_value?(asked) + return stored.to_s.strip == asked.to_s.strip if stored.is_a?(String) || asked.is_a?(String) + + stored == asked end - # Read only when a field of the wrong direction carries a value, and only - # for that field: an update naming none costs no request at all. + # Read only when the patch names a field of the wrong direction, and only + # for those fields: an update naming none costs no request at all. It is + # one request on the collections whose endpoint filters `id`, and one per + # record on the ones reading an id through its own endpoint — PylonIssue, + # whose fan-out `max_resolvable_ids` bounds. # # Read by id rather than through the caller's filter: the filter was # already resolved into these ids, so re-running it would spend those @@ -237,7 +274,7 @@ def stored_values(caller, ids, fields) # field and `value` for every other — a select being written by the slug of # its option, which is what the Enum column advertises. def split_custom_fields(attrs) - by_column = custom_fields.to_h { |custom_field| [custom_field[:column_name], custom_field] } + by_column = custom_fields_by_column entries = [] native = attrs.each_with_object({}) do |(field, value), rest| @@ -248,6 +285,10 @@ def split_custom_fields(attrs) [entries, native] end + def custom_fields_by_column + @custom_fields_by_column ||= custom_fields.to_h { |field| [field[:column_name], field] } + end + def custom_field_entry(custom_field, value) slug = custom_field[:column_name] return { 'slug' => slug, 'values' => Array(value) } if custom_field[:multi_value] @@ -260,15 +301,19 @@ def refuse_write(verb) "A #{name} record cannot be #{verb}: the Pylon API exposes no endpoint for it." end - def refuse_wrong_direction(field, direction) + # Every offending field at once: refusing them one at a time would have the + # operator undo one, retry, and learn about the next. + def refuse_wrong_direction(fields, direction) + them = fields.one? ? 'it' : 'them' detail = if direction == :create - 'Pylon only accepts it on an existing record: create the record, then edit it.' + "Pylon only accepts #{them} on an existing record: create the record, then edit it." else - 'Pylon only accepts it when the record is created, and exposes no endpoint to change it ' \ - 'afterwards.' + "Pylon only accepts #{them} when the record is created, and exposes no endpoint to change " \ + "#{them} afterwards." end - raise UnsupportedWriteError, "'#{field}' cannot be set here on a #{name}: #{detail}" + named = fields.map { |field| "'#{field}'" }.join(', ') + raise UnsupportedWriteError, "#{named} cannot be set here on a #{name}: #{detail}" end def refuse_too_many_targets(count) 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 607b2d93f..edeeed5a9 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 @@ -497,7 +497,7 @@ def returning(*ids) node = branch('Or', [leaf('id', operators::EQUAL, 'uuid-1'), leaf('state', operators::EQUAL, 'new')]) expect { collection.build_pylon_filter(nil, filter(condition_tree: node)) } - .to raise_error(UnsupportedOperatorError, /has to be combined with 'and' conditions only/) + .to raise_error(UnsupportedOperatorError, /An id inside an `or` names none/) end # A collection whose endpoint filters id server-side never short-circuits, diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb index 17f57ebef..46993acf0 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -162,6 +162,16 @@ def issue_payload(id, overrides = {}) expect(WebMock).not_to have_requested(:patch, "#{base}/issues/i1") end + # The cap bounds a write, and a patch naming nothing writable is not one: + # it is settled before the ids are, so the selection is never resolved and + # never refused for its width. + it 'sends nothing, and refuses nothing, when the patch is read-only over a wide selection' do + ids = Array.new(21) { |index| "i#{index}" } + + expect { issues.update(nil, id_filter(operators::IN, ids), 'number' => 9) }.not_to raise_error + expect(WebMock).not_to have_requested(:patch, %r{/issues/}) + end + # The scope the operator's role carries rides along as an `and`, so the ids # are resolved through the collection's own read and a record the scope # excludes is never written to. @@ -244,15 +254,60 @@ def issue_payload(id, overrides = {}) expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) end - # An unchecked box is not an edit of the field: dropping it costs no read - # at all, where refusing it would fail every edit whose form carries one. - it 'drops a boolean left false without reading the record back' do + # An unchecked box over a record holding nothing is not an edit: Pylon + # returns a null where the form sends `false`, and refusing that pair + # would fail every edit whose form carries one. + it 'drops a boolean left false over a record holding nothing' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'author_unverified' => false, 'title' => 'Louder') expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) - expect(WebMock).not_to have_requested(:get, "#{base}/issues/i1") + end + + # Over a record holding `true` the same `false` is the operator unchecking + # the box: Pylon cannot write it, and dropping it would report an edit it + # never performed. + it 'refuses a boolean the operator unchecked' do + stub_request(:get, "#{base}/issues/i1") + .to_return(json('data' => issue_payload('i1', 'author_unverified' => true))) + + expect { issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'author_unverified' => false) } + .to raise_error(UnsupportedWriteError, /'author_unverified' cannot be set here on a PylonIssue/) + end + + # Same story for a value cleared rather than unchecked: an empty body over + # a stored one is an edit, where an empty body over an empty one is not. + it 'refuses a string the operator cleared' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + expect { issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'body_html' => '') } + .to raise_error(UnsupportedWriteError, /'body_html' cannot be set here on a PylonIssue/) + end + + # The markup an editor hands back may be the markup it was given, + # re-indented. Refusing that would name a field the operator never touched. + it 'drops a string the editor only re-indented' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::EQUAL, 'i1'), + 'body_html' => "

boom

\n", 'title' => 'Louder') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) + end + + # Both offending fields at once: refusing them one at a time would have the + # operator undo one, retry, and learn about the next. + it 'names every field of the wrong direction in one message' do + stub_request(:get, "#{base}/issues/i1") + .to_return(json('data' => issue_payload('i1', 'author_unverified' => true))) + + expect do + issues.update(nil, id_filter(operators::EQUAL, 'i1'), + 'body_html' => '

louder

', 'author_unverified' => false) + end.to raise_error(UnsupportedWriteError, /'body_html', 'author_unverified' cannot be set here/) end # The filter was already resolved into ids, so reading it again would spend @@ -346,10 +401,13 @@ def issue_payload(id, overrides = {}) end # "Select all except these" reaches PylonIssue as `id not_in`, which its - # endpoint cannot filter: the read refuses it, and so does the delete. + # endpoint cannot filter: the read refuses it, and so does the delete. The + # message names that selection rather than the `and`/`or` of a filter the + # operator never wrote. it 'refuses an excluding selection on the collection that cannot filter an id' do expect { issues.delete(nil, id_filter(operators::NOT_IN, %w[i1])) } - .to raise_error(UnsupportedOperatorError, /A filter on 'id' has to be combined with 'and'/) + .to raise_error(UnsupportedOperatorError, + /Select the records to act on rather than the ones to leave out/) end end From bed97b7e94bd9f9b5d9150496a641cc6b20deac5 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Thu, 20 Aug 2026 16:46:28 +0200 Subject: [PATCH 4/8] fix(pylon): third review pass on the write path Three findings from the review of the write half. A verb Pylon has no endpoint for is refused before anything is spent. delete resolved its ids first, so a delete of 25 teams answered "more than the 20 one pass covers" and sent the operator to narrow a selection that was never the problem, a delete over a filter spent a GET /teams to get there, and one matching no record answered 204 on a collection that cannot delete at all. The *_record hook stays the single declaration of what exists: write_endpoint? reads it rather than a second list of verbs to keep in step. Pylon's own refusal reaches the operator. APIError descends from the package's Error, which the agent's ErrorTranslator does not recognise: it keeps the status and answers 'Unexpected error', so a missing required field or a value the endpoint refuses (the likeliest way a write fails) arrived as nothing at all. A 4xx is re-raised as WriteRejectedError, a ValidationError whose message the agent surfaces; a 5xx or a dropped connection is not the operator's to act on and stays the APIError it was. An id named twice is one record. id_values deduplicates, where only resolve_ids_by_list did: `id in [i1, i1]` patched the same issue twice, and a delete answered 404 on the second, reported as a partial failure of a delete that fully succeeded. The caps now count records rather than mentions, and a primary-key lookup no longer spends two requests on one id. 637 examples, 0 failures; coverage 1285/1285 lines, RuboCop clean over the 55 files of the package. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/forest_admin_datasource_pylon.rb | 10 +++ .../collections/base_collection.rb | 8 +- .../collections/writes.rb | 39 ++++++++- .../collections/writes_spec.rb | 82 ++++++++++++++++++- 4 files changed, 134 insertions(+), 5 deletions(-) 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 fa06a3a2c..deeafd9d0 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 @@ -34,6 +34,16 @@ class UnsupportedWriteError < ForestAdminDatasourceToolkit::Exceptions::Validati # retry of the whole selection would write those a second time. class PartialWriteError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end + # A write Pylon itself refused, carrying the reason it gave. `APIError` below + # descends from the package's own Error, which the agent's ErrorTranslator + # does not recognise: it keeps the status and answers 'Unexpected error', so + # the likeliest way a write fails — a required field left out, a value the + # endpoint does not accept — would reach the operator as nothing at all. + # Only Pylon's 4xx travels this way: it names something the operator can fix, + # where a 5xx or a dropped connection is not theirs to act on and stays the + # APIError it was. + class WriteRejectedError < 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 # surface Pylon's own validation message instead of a generic string. 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 a073db76e..fe214c0e9 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 @@ -340,11 +340,17 @@ def walker @walker ||= Pagination::CursorWalker.new end + # A set of ids, not a list: `id in` names the records to act on, and the + # same one named twice is one record. Deduplicating here is what keeps a + # lookup from spending two requests on one id and, on the write side, from + # writing it twice — a delete answering 404 the second time, reported as a + # partial failure of a delete that fully succeeded. It is also what the + # caps count against, both bounding records rather than mentions. def id_values(node) return nil unless node.is_a?(Leaf) && node.field == 'id' return nil unless [Operators::EQUAL, Operators::IN].include?(node.operator) - Array(node.value).map(&:to_s).reject(&:empty?) + Array(node.value).map(&:to_s).reject(&:empty?).uniq end def and_branch?(node) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb index 7bf8262f0..61eb366bd 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -35,8 +35,18 @@ module Writes # rubocop:disable Metrics/ModuleLength # would not be, which is the very thing this datasource refuses. MAX_WRITE_TARGETS = 20 + # A verb Pylon has no endpoint for is refused first, before the payload is + # built and before the ids are resolved: the refusal holds whatever the + # selection turns out to be, and everything the write would do on the way + # there answers with something else — the cap naming a count, a field of + # the wrong direction naming a field — sending the operator to narrow a + # selection that was never the problem. def create(_caller, data) + refuse_write('created') unless write_endpoint?(:create_record) + serialize(create_record(build_payload(writable_attributes(data), :create))) + rescue APIError => e + surface_write_rejection(e) end # What the patch may write is settled before the ids are: a patch naming @@ -44,6 +54,8 @@ def create(_caller, data) # for reaching too many records — the cap bounds a write, and there is # none here. def update(caller, filter, patch) + refuse_write('updated') unless write_endpoint?(:update_record) + attributes = writable_attributes(patch) return if attributes.empty? @@ -57,6 +69,8 @@ def update(caller, filter, patch) end def delete(caller, filter) + refuse_write('deleted') unless write_endpoint?(:delete_record) + write_each(ids_for(caller, filter), 'deleted') { |id| delete_record(id) } end @@ -110,6 +124,15 @@ def ids_for(caller, filter) private + # Whether the collection wired the Pylon endpoint for a verb. The + # `*_record` hook is the declaration, read here rather than repeated in a + # list of supported verbs a collection would have to keep in step with its + # own hooks — the same reason `is_read_only` on the column, and not a + # second list of writable names, is what the payload builder reads. + def write_endpoint?(hook) + method(hook).owner != Writes + end + # One request per record, so a failure on the k-th record leaves the k-1 # before it written — the cap bounds how many records a write reaches, # nothing bounds the endpoint answering 429 or 422 halfway through. The @@ -123,12 +146,26 @@ def write_each(ids, verb) yield id written << id rescue APIError => e - raise if written.empty? + # Always raises, so nothing reaches the partial report below: with no + # record written the failure is the whole of what happened. + surface_write_rejection(e) if written.empty? refuse_partial_write(verb, written, id, ids.size, e) end end + # Pylon's own refusal, in the operator's hands. A 4xx names something they + # did — a required field left out, a value the endpoint does not take, a + # record already gone — and travels as the ValidationError whose message + # the agent surfaces, where the APIError it arrived as would be answered + # with 'Unexpected error'. Anything else is Pylon or the network failing, + # which no edit of theirs would change: it stays what it was. + def surface_write_rejection(error) + raise error unless (400..499).cover?(error.status.to_i) + + raise WriteRejectedError, error.message + end + # One record past the cap is asked for, so an overflow is seen rather than # guessed from a full page — the same bound `foreign_keys_matching` puts on # a resolved relation condition. diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb index 46993acf0..1ae8183ed 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -225,12 +225,67 @@ def issue_payload(id, overrides = {}) end # Nothing was written, so the failure is the whole of what happened and - # travels as the error Pylon answered with. - it 'raises the API error itself when the first record failed' do + # travels as the reason Pylon gave — as a ValidationError, the agent + # answering an APIError with 'Unexpected error' whatever it carries. + it 'surfaces the refusal itself when the first record failed' do stub_request(:delete, "#{base}/issues/i1").to_return(json({ 'message' => 'gone' }, 404)) expect { issues.delete(nil, id_filter(operators::IN, %w[i1 i2])) } - .to raise_error(APIError, %r{delete\(issues/i1\)}) + .to raise_error(WriteRejectedError, %r{delete\(issues/i1\).*gone}m) + end + end + + # Pylon's own refusal is the likeliest way a write fails, and `APIError` + # descends from the package's Error, which the agent's translator answers + # with 'Unexpected error': a 4xx is re-raised as a ValidationError so the + # reason reaches the operator, and nothing else is. + describe 'a write Pylon refused' do + it 'surfaces the reason a rejected create was given' do + stub_request(:post, "#{base}/issues") + .to_return(json({ 'message' => 'title is required' }, 422)) + + expect { issues.create(nil, 'title' => '', 'body_html' => '

b

') } + .to raise_error(WriteRejectedError, /title is required/) + end + + it 'surfaces the reason a rejected update was given' do + stub_request(:patch, "#{base}/issues/i1").to_return(json({ 'message' => 'unknown state' }, 422)) + + expect { issues.update(nil, id_filter(operators::EQUAL, 'i1'), 'state' => 'nope') } + .to raise_error(WriteRejectedError, /unknown state/) + end + + # Not the operator's to fix, and not theirs to be told to fix: a gateway + # error stays the APIError it was, carrying its status for the agent to + # answer with. + it 'leaves a Pylon-side failure as it was' do + stub_request(:post, "#{base}/issues").to_return(json({ 'message' => 'boom' }, 500)) + + expect { issues.create(nil, 'title' => 'Boom', 'body_html' => '

b

') } + .to raise_error(APIError) { |error| expect(error.status).to eq(500) } + end + end + + # `id in` names the records to act on, and the same one named twice is one + # record: writing it twice would answer 404 on the second delete and report + # a partial failure of a delete that fully succeeded. + describe 'an id named twice in the same selection' do + it 'writes the record once' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::IN, %w[i1 i1]), 'state' => 'closed') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").once + end + + # The caps bound records, not mentions: a selection naming the same id + # over and over reaches one record and is not refused for reaching many. + it 'counts it once against the cap' do + stub_request(:patch, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::IN, ['i1'] * 25), 'state' => 'closed') + + expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").once end end @@ -426,6 +481,27 @@ def issue_payload(id, overrides = {}) expect { teams.delete(nil, id_filter(operators::EQUAL, 't1')) } .to raise_error(UnsupportedWriteError, /A PylonTeam record cannot be deleted/) end + + # The refusal holds whatever the selection reaches, so it comes before the + # ids are resolved: answering with the cap would send the operator to + # narrow a selection that was never the problem, and answering a selection + # matching nothing with a silent success would report a delete on a + # collection that cannot perform one. + it 'refuses a selection wider than the cap without naming the cap' do + expect { teams.delete(nil, id_filter(operators::IN, (1..25).map { |i| "t#{i}" })) } + .to raise_error(UnsupportedWriteError, /A PylonTeam record cannot be deleted/) + end + + it 'refuses a selection matching nothing rather than answering it' do + expect { teams.delete(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'nope'))) } + .to raise_error(UnsupportedWriteError, /A PylonTeam record cannot be deleted/) + end + + it 'refuses before spending a request to resolve the selection' do + expect { teams.delete(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'x'))) } + .to raise_error(UnsupportedWriteError) + expect(WebMock).not_to have_requested(:get, "#{base}/teams") + end end describe 'the collections read through their own endpoints' do From 01f486bfab28e19ddfa131265f7c15e4e6c538de Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 21 Aug 2026 16:33:26 +0200 Subject: [PATCH 5/8] fix(pylon): fourth review pass on the write path Three findings from the review of the write path, each one a write reporting something that did not happen. A DELETE is no longer replayed on anything but a 429. faraday-retry ships :delete among its idempotent methods and ORs them with retry_if, so a 502 or a dropped connection on the way back from a DELETE Pylon did perform was replayed into a 404 and surfaced as a deletion that failed when it landed. Reads keep the blanket retry; PUT leaves the list for having no endpoint at all. A dependent overflow no longer names a count. The resolution asks for one record past the cap, so the size of its window is not the size of the selection: an operator whose filter matched thousands of records was told it matched 21. The exact count stays on the path where the filter named the ids, the two refusals now sharing one explanation. A partial read no longer settles a wrong-direction field. The guard was "no record came back" where it had to be "a record did not come back": with two ids named and one unreadable, body_html was dropped as unchanged against the record that did answer, and both were patched. Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/writes.rb | 30 +++++++++++++------ .../retry_policy.rb | 21 +++++++++---- .../client/writes_spec.rb | 20 +++++++++++++ .../collections/writes_spec.rb | 22 ++++++++++++-- .../retry_policy_spec.rb | 11 +++++-- 5 files changed, 85 insertions(+), 19 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb index 61eb366bd..4561b4afd 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -173,7 +173,7 @@ def resolve_ids_by_list(caller, filter) window = Page.new(offset: 0, limit: max_write_targets + 1) query = (filter || Filter.new).override(page: window) records = list(caller, query, Projection.new(['id'])) - refuse_too_many_targets(records.size) if records.size > max_write_targets + refuse_unbounded_targets if records.size > max_write_targets records.filter_map { |record| record['id'] }.uniq end @@ -260,14 +260,15 @@ def blank_write_value?(value) end # The wrong-direction fields already holding the value the patch asks for. - # An unreadable record counts as none of them: the field is refused rather - # than dropped, since nothing here may claim a value is unchanged without - # having read it. + # A record the read did not hand back counts as none of them, and one + # missing record is enough: the field is refused rather than dropped, since + # nothing here may claim a value is unchanged on a record it never read -- + # which a selection where only some ids came back would otherwise do. def unchanged_fields(caller, ids, fields, attrs) return [] if fields.empty? stored = stored_values(caller, ids, fields) - return [] if stored.empty? + return [] if stored.size < ids.size fields.select { |field| stored.all? { |record| same_write_value?(record[field], attrs[field]) } } end @@ -353,12 +354,23 @@ def refuse_wrong_direction(fields, direction) raise UnsupportedWriteError, "#{named} cannot be set here on a #{name}: #{detail}" end + # The count is exact here, the filter having named the ids. def refuse_too_many_targets(count) + refuse_write_reach("applies to #{count} #{name} records, more than the #{max_write_targets} one pass covers") + end + + # The resolution asks for one record past the cap, so all it knows is that + # the selection overflows: reporting the size of its window as a count + # would name 21 records to an operator whose selection holds thousands. + def refuse_unbounded_targets + refuse_write_reach("applies to more than the #{max_write_targets} #{name} records one pass covers") + end + + def refuse_write_reach(reach) raise UnsupportedWriteError, - "This write applies to #{count} #{name} records, more than the #{max_write_targets} one pass " \ - 'covers: Pylon writes one record per request, against a budget of ten to twenty requests per ' \ - 'minute, and a write stopping halfway would report a success it did not perform. Narrow the ' \ - 'selection to reach the records past this point.' + "This write #{reach}: Pylon writes one record per request, against a budget of ten to twenty " \ + 'requests per minute, and a write stopping halfway would report a success it did not perform. ' \ + 'Narrow the selection to reach the records past this point.' end # Named ids the collection cannot resolve exactly, the filter carrying diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb index 5aaa3a1ae..163cb0aea 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb @@ -19,12 +19,21 @@ class RetryPolicy Faraday::RetriableResponse, Faraday::ConnectionFailed ].freeze - # Faraday only retries these by default; a 429 is safe to retry on any verb - # because Pylon rejected the request before processing it, whereas a 502 on a - # POST /issues may well have created the issue. This has to go through - # retry_if rather than methods: faraday-retry ORs the two, so methods can - # only widen the set, never restrict it. - IDEMPOTENT_METHODS = %i[delete get head options put].freeze + # The verbs a replay cannot repeat: a GET, a HEAD and an OPTIONS change + # nothing, so any transient failure is worth another attempt. + # + # DELETE is deliberately out, although HTTP calls it idempotent and + # faraday-retry ships it as a default: a 502 or a dropped connection on the + # way back from a DELETE Pylon did perform is replayed into a 404, which the + # write path then surfaces as a deletion that failed when it landed -- the + # very report of something that did not happen this datasource refuses. PUT + # is out for having no endpoint: Pylon writes through POST and PATCH. + # + # A 429 stays safe to retry on any verb, Pylon having rejected the request + # before processing it, and travels through retry_if rather than through this + # list: faraday-retry ORs the two, so methods can only widen the set, never + # restrict it. + IDEMPOTENT_METHODS = %i[get head options].freeze RETRY_IF = ->(env, _exception) { env[:status] == 429 } BACKOFF_FACTOR = 2 diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb index c55f314b5..4b0e8fe72 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/client/writes_spec.rb @@ -135,5 +135,25 @@ def json(payload, status = 200) expect { client.create_issue('title' => 'Boom') }.to raise_error(ForestAdminDatasourcePylon::APIError) expect(WebMock).to have_requested(:post, "#{base}/issues").once end + + it 'retries a rate-limited delete' do + stub_request(:delete, "#{base}/issues/i1") + .to_return(json({ 'message' => 'slow down' }, 429)) + .then.to_return(json({}, 204)) + + expect(client.delete_issue('i1')).to be(true) + expect(WebMock).to have_requested(:delete, "#{base}/issues/i1").twice + end + + # A 502 on the way back from a DELETE Pylon did perform would be replayed + # into a 404, which the write path surfaces as a deletion that failed when + # it landed -- a report of something that did not happen. So the gateway + # error stays what it is, on a delete as on a create. + it 'does not retry a delete that failed on a gateway error' do + stub_request(:delete, "#{base}/issues/i1").to_return(json({ 'message' => 'bad gateway' }, 502)) + + expect { client.delete_issue('i1') }.to raise_error(ForestAdminDatasourcePylon::APIError, /502/) + expect(WebMock).to have_requested(:delete, "#{base}/issues/i1").once + end end end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb index 1ae8183ed..e2fba5bbd 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -353,6 +353,21 @@ def issue_payload(id, overrides = {}) expect(WebMock).to have_requested(:patch, "#{base}/issues/i1").with(body: { 'title' => 'Louder' }) end + # Nothing here may claim a value is unchanged on a record it never read, + # and one record short of the selection is enough: dropping the field would + # write the rest of the patch to a record whose stored value is unknown and + # report the whole edit as performed. + it 'refuses it when one of the named records could not be read' do + stub_request(:get, "#{base}/issues/i1").to_return(json('data' => issue_payload('i1'))) + stub_request(:get, "#{base}/issues/i2").to_return(json({ 'message' => 'gone' }, 404)) + + expect do + issues.update(nil, id_filter(operators::IN, %w[i1 i2]), + 'body_html' => '

boom

', 'title' => 'Louder') + end.to raise_error(UnsupportedWriteError, /'body_html' cannot be set here on a PylonIssue/) + expect(WebMock).not_to have_requested(:patch, %r{/issues/}) + end + # Both offending fields at once: refusing them one at a time would have the # operator undo one, retry, and learn about the next. it 'names every field of the wrong direction in one message' do @@ -415,12 +430,15 @@ def issue_payload(id, overrides = {}) expect(WebMock).not_to have_requested(:delete, %r{/issues/}) end - it 'refuses it when the count only shows once the filter is resolved' do + # The resolution asks for one record past the cap, so the overflow is seen + # rather than counted: reporting the window as a count would name 21 to an + # operator whose selection holds thousands. + it 'refuses it when the overflow only shows once the filter is resolved, without naming a count' do stub_request(:post, "#{base}/accounts/search") .to_return(json('data' => Array.new(21) { |index| { 'id' => "a#{index}", 'name' => 'Acme' } })) expect { accounts.delete(nil, filter(condition_tree: leaf('name', operators::EQUAL, 'Acme'))) } - .to raise_error(UnsupportedWriteError, /applies to 21 PylonAccount records/) + .to raise_error(UnsupportedWriteError, /applies to more than the 20 PylonAccount records one pass covers/) expect(WebMock).not_to have_requested(:delete, %r{/accounts/}) end diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/retry_policy_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/retry_policy_spec.rb index f88450901..d8d2cfe21 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/retry_policy_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/retry_policy_spec.rb @@ -36,10 +36,17 @@ expect(options[:exceptions]).to include(Faraday::ConnectionFailed, Faraday::RetriableResponse) end - it 'limits blanket retries to idempotent verbs' do - expect(options[:methods]).to eq(%i[delete get head options put]) + it 'limits blanket retries to the verbs that read' do + expect(options[:methods]).to eq(%i[get head options]) expect(options[:methods]).not_to include(:post, :patch) end + + # A 502 on the way back from a DELETE Pylon did perform would be replayed + # into a 404, which the write path surfaces as a deletion that failed when + # it landed. Only its 429 is retried, through RETRY_IF. + it 'never replays a delete on anything but a 429' do + expect(options[:methods]).not_to include(:delete) + end end describe 'RETRY_IF' do From c8934b14cf5ffeb5540a80267d9c09c9cfab6aa4 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 21 Aug 2026 18:15:07 +0200 Subject: [PATCH 6/8] refactor(pylon): trim the comments on the write path The write path carried roughly as many lines of prose as of code, a good part of it restating the body underneath or repeating the pull request. What survives is the rationale that cannot be re-derived from the code: the Pylon API asymmetries, the GeneratorField behaviour a writable foreign key relies on, and the constant lookup the module re-declares for. Three review findings move into the comments that stayed, where they belong rather than in a thread: surface_write_rejection names the 4xx it does not cover, the one raised while resolving a selection; filtered_ids names the intersection it over-refuses; and stored_values names the requests max_write_targets does not count. IDEMPOTENT_METHODS becomes RETRYABLE_METHODS. DELETE is idempotent and was just taken out of the list, so the name needed four lines of comment to apologise for itself. No behaviour change: 641 examples, coverage 1289/1289 lines, RuboCop clean over the 55 files of the package. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/forest_admin_datasource_pylon.rb | 29 +-- .../client/writes.rb | 23 +-- .../collections/account/schema_definition.rb | 7 +- .../collections/base_collection.rb | 19 +- .../collections/contact/schema_definition.rb | 21 +- .../collections/fetch_all_collection.rb | 11 +- .../collections/issue.rb | 9 +- .../collections/issue/schema_definition.rb | 21 +- .../collections/team.rb | 9 +- .../collections/user.rb | 8 +- .../collections/writes.rb | 191 +++++++----------- .../retry_policy.rb | 18 +- .../schema/custom_fields_introspector.rb | 5 +- 13 files changed, 146 insertions(+), 225 deletions(-) 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 deeafd9d0..2bc610d5b 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 @@ -20,28 +20,21 @@ class ConfigurationError < Error; end # they learn which one. class UnsupportedOperatorError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end - # A write Pylon cannot perform: a verb its API has no endpoint for, a field it - # only accepts in the other direction, or a filter-driven write reaching more - # records than one page of writes may cover. Descends from ValidationError for - # the same reason as above — each names something the operator did and can - # undo, and the message is the only place they learn what. + # The three write errors below descend from ValidationError for that same + # reason: each names something the operator did and can undo. + + # A verb Pylon's API has no endpoint for, a field it only accepts in the other + # direction, or a write reaching more records than one pass may cover. class UnsupportedWriteError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end - # A filter-driven write Pylon performed on some of its records and then - # failed on: one record is one request, so the ones before the failure are - # written and stay written. Descends from ValidationError so the operator - # reads which records landed rather than a 500 leaving them to guess — a - # retry of the whole selection would write those a second time. + # A write Pylon performed on some of its records and then failed on: one + # record is one request, so the ones before the failure stay written, and a + # retry of the whole selection would write them a second time. class PartialWriteError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end - # A write Pylon itself refused, carrying the reason it gave. `APIError` below - # descends from the package's own Error, which the agent's ErrorTranslator - # does not recognise: it keeps the status and answers 'Unexpected error', so - # the likeliest way a write fails — a required field left out, a value the - # endpoint does not accept — would reach the operator as nothing at all. - # Only Pylon's 4xx travels this way: it names something the operator can fix, - # where a 5xx or a dropped connection is not theirs to act on and stays the - # APIError it was. + # A write Pylon itself refused, carrying the reason it gave — the likeliest + # way a write fails. Only its 4xx travels this way: a 5xx or a dropped + # connection is not the operator's to act on and stays the APIError it was. class WriteRejectedError < ForestAdminDatasourceToolkit::Exceptions::ValidationError; end # Raised when a Pylon API call fails. Carries the HTTP status and the diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb index b2b8caa86..9a50e9cb9 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/client/writes.rb @@ -7,9 +7,9 @@ class Client # enriches a page — a thread that could not be read costs a column — where a # write that silently did nothing would tell the operator their edit landed. # - # Pylon exposes no write endpoint for every verb: there is no POST or DELETE - # on users, and no DELETE on teams. The collections answer those, not the - # client, which only spells the endpoints that exist. + # Pylon exposes no POST or DELETE on users, and no DELETE on teams. The + # collections answer those, not the client, which only spells the endpoints + # that exist. module Writes # `title` and `body_html` are the two fields POST /issues requires. def create_issue(attributes) = post_resource('issues', attributes) @@ -58,10 +58,9 @@ def delete_resource(resource, id) end # Pylon answers a write with the written record under `data`. Anything else - # means the contract broke, which is worth a typed error rather than an - # envelope the collection would then serialize into a record with no id -- - # `extract_data` hands the body back untouched when `data` is absent, which - # is what a read wants and a write must not accept. + # broke the contract: `extract_data` hands the body back untouched when + # `data` is absent, which is what a read wants and a write must not accept + # — the collection would serialize the envelope into a record with no id. def extract_written(body, operation) record = body['data'] if body.is_a?(Hash) return record if record.is_a?(Hash) @@ -69,12 +68,10 @@ def extract_written(body, operation) refuse_body_shape(body, operation, "missing 'data'") end - # An update is answered the same way, but its record is never read back: - # the collection discards it. So a 204, an empty body or a null `data` is - # the write having landed with nothing to hand back, and raising there - # would report a failure on a record Pylon already patched — and abort the - # records a bulk edit had left to write. Only a `data` carrying something - # that is not a record means the contract broke. + # An update discards its record, so a 204, an empty body or a null `data` + # is the write having landed with nothing to hand back: raising there would + # report a failure on a record Pylon already patched, and abort the records + # a bulk edit had left to write. def extract_updated(body, operation) record = body['data'] if body.is_a?(Hash) return record if record.nil? || record.is_a?(Hash) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb index 94004c74a..bea8623a8 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/account/schema_definition.rb @@ -68,16 +68,15 @@ def define_ownership_fields # returns; a plain column, see `define_relations` above. add_column('owner_id', 'String', writable: true) # Read-only although the endpoint takes it: the column shows - # `{external_id, label}` objects, and what the API writes back is not - # documented in that shape — writing one for the other would replace + # `{external_id, label}` objects, and the write shape the reference + # documents is not that one — writing one for the other would replace # the ids of the account with something it cannot read. add_column('external_ids', 'Json') end # Both belong to the integrations Pylon syncs them from: `crm_settings` # is absent from every write endpoint, and `channels` — which they do - # take — is a list of objects whose write shape the reference does not - # document, the same reason `external_ids` stays read-only above. + # take — holds objects, like `external_ids` above. def define_integration_fields add_column('channels', 'Json') add_column('crm_settings', 'Json') 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 fe214c0e9..ee82a9f5f 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 @@ -201,8 +201,7 @@ def api_filters end # A native column: read-only unless the collection declares it `writable`, - # which is what the payload builder reads to know a column may be sent, and - # never groupable, as no Pylon endpoint aggregates. It is not sortable + # 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 @@ -340,12 +339,10 @@ def walker @walker ||= Pagination::CursorWalker.new end - # A set of ids, not a list: `id in` names the records to act on, and the - # same one named twice is one record. Deduplicating here is what keeps a - # lookup from spending two requests on one id and, on the write side, from - # writing it twice — a delete answering 404 the second time, reported as a - # partial failure of a delete that fully succeeded. It is also what the - # caps count against, both bounding records rather than mentions. + # A set of ids, not a list: the same one named twice is one record, so a + # lookup spends one request on it and a delete does not answer 404 the + # second time. The caps count records rather than mentions for the same + # reason. def id_values(node) return nil unless node.is_a?(Leaf) && node.field == 'id' return nil unless [Operators::EQUAL, Operators::IN].include?(node.operator) @@ -363,10 +360,8 @@ def and_branch?(node) # would bring in records the lookup never fetched. Worth an error an # operator can act on rather than the translator's "add it to api_filters", # because two things they do reach it: the `id equals` filter next to the - # or/and toggle, and — through the write path — an excluding selection, - # "select every record except these", which arrives as `id not_in` and - # names the records to leave out rather than the ones to read. The message - # names both, an exclusion being no filter the operator wrote. + # or/and toggle, and an excluding selection — "every record except these" — + # which arrives as `id not_in` and is no filter they wrote. # # A collection whose endpoint does filter id declares it in `api_filters` # and never short-circuits, so the translator handles its ids like any diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb index 1a71ad06b..aaa804abd 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/contact/schema_definition.rb @@ -47,8 +47,7 @@ def define_identity_fields # 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. Writable, which is - # what opens the relation editor — see the party fields of PylonIssue - # for why the key itself stays read-only in the Forest schema. + # what opens the relation editor — see the party fields of PylonIssue. add_column('account_id', 'String', writable: true) # Read-only Json, and deliberately unfilterable although the search # endpoint does not offer it either: the API matches bare external-id @@ -60,12 +59,11 @@ def define_identity_fields # `email` and `primary_phone_number` carry the primary value; the lists # hold every address and number, and neither list is filterable. # - # `email` is written on a create and `emails` on an update, one - # direction each: `POST /contacts` takes the primary address alone, and - # the other ones are set on an existing contact. Two writable - # projections of the same addresses would otherwise travel in one patch, - # the list leaving out whatever the primary carries — the reason - # PylonAccount keeps `domain` read-only next to `domains`. + # `email` is written on a create and `emails` on an update, one direction + # each: `POST /contacts` takes the primary address alone, and the other + # ones are set on an existing contact. Two writable projections of the + # same addresses would otherwise travel in one patch, the list leaving + # out whatever the primary carries. # # `phone_numbers` is not writable at all: it holds objects, and the # shape the endpoint takes them in is not the one the column shows. @@ -80,10 +78,9 @@ def define_contact_fields 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. The id is the one written and - # the name is read-only, like `role_id` and `role_name` on PylonUser: - # writing both would carry two projections of one role in the same - # patch, and whichever Pylon ignored would come back stale. + # is what `portal_role_id` points at. The id is the one written and the + # name is read-only, like `role_id` and `role_name` on PylonUser: + # whichever of two projections Pylon ignored would come back stale. add_column('portal_role', 'String') add_column('portal_role_id', 'String', writable: true) # Owned by the integrations the contact was seen through; no endpoint diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb index 1da8d0da8..f29e8c851 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/fetch_all_collection.rb @@ -76,12 +76,11 @@ def records_indexed_by_id(ids) protected - # A column is read-only unless the collection declares it `writable`, which - # is what the payload builder reads to know it may be sent. 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. + # A column is read-only unless the collection declares it `writable`. + # 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, writable: false) add_field(name, ColumnSchema.new(column_type: type, filter_operators: self.class.operators_for(type), 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 f83f83184..4da069547 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 @@ -80,11 +80,10 @@ def update_only_fields = UPDATE_ONLY def max_write_targets = [Writes::MAX_WRITE_TARGETS, MAX_ID_LOOKUPS].min - # Never past the primary-key fan-out: an issue is read by its own - # endpoint, so a write resolving named ids through `list` goes through - # `fetch_by_ids`, which truncates with a warning past this many, and a - # truncated resolution would write to a subset of the selection while - # reporting the whole of it. + # Never past the primary-key fan-out: a write resolving named ids through + # `list` goes through `fetch_by_ids`, which truncates with a warning past + # this many, and a truncated resolution would write to a subset of the + # selection while reporting the whole of it. def max_resolvable_ids = MAX_ID_LOOKUPS def sortable_fields 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 587561698..dae0fda73 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 @@ -3,11 +3,11 @@ module Collections class Issue < BaseCollection # A column is writable when `POST /issues` or `PATCH /issues/{id}` accepts # it, the two directions being told apart by `Issue::CREATE_ONLY` and - # `Issue::UPDATE_ONLY`; everything Pylon computes — the number, the link, - # the timestamps, the counters — stays read-only. No column is sortable, - # `/issues/search` exposing no sort parameter at all: results always come - # back ordered by `created_at` descending, so advertising a sortable column - # would let the UI ask for an order the API cannot honour. + # `Issue::UPDATE_ONLY`; everything Pylon computes stays read-only. No + # column is sortable, `/issues/search` exposing no sort parameter at all: + # results always come back ordered by `created_at` descending, 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 @@ -97,12 +97,11 @@ def define_thread_field # 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. # - # Writable, although the schema the agent sends Forest marks a foreign - # key read-only whatever the datasource says — `GeneratorField` forces it - # so the detail view has one editor per key rather than two. What the - # flag opens is that editor, the `BelongsTo` reading its own read-only - # state off the key column, and the front sends the choice back as the - # very column named here. + # Writable, although `GeneratorField` forces a foreign key read-only in + # the emitted schema whatever the datasource says, so the detail view has + # one editor per key rather than two. What the flag opens is that editor: + # the `BelongsTo` reads its own read-only state off the key column, and + # the front sends the choice back as the very column named here. def define_party_fields %w[account_id requester_id assignee_id team_id].each do |field| add_column(field, 'String', writable: true) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb index 6f00f8277..1f621df12 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/team.rb @@ -7,8 +7,7 @@ def initialize(datasource) protected - # `delete_record` is left alone: Pylon exposes no DELETE on a team, and - # the default hook refuses the verb with a message rather than a 500. + # Pylon exposes no DELETE on a team, so that hook is left to refuse. def create_record(payload) = datasource.client.create_team(payload) def update_record(id, payload) = datasource.client.update_team(id, payload) @@ -40,9 +39,9 @@ def define_schema add_column('id', 'String', is_primary_key: true) add_column('name', 'String', writable: true) # A list, so neither filterable nor sortable, and no relation either: - # see `define_relations` above. Writable: `POST /teams` and - # `PATCH /teams/{id}` take the members as this very list of ids, and the - # one sent replaces the membership whole. + # see `define_relations` above. `POST /teams` and `PATCH /teams/{id}` + # take the members as this very list, and the one sent replaces the + # membership whole. add_column('user_ids', 'Json', writable: true) 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 index a04293b2a..149cb0918 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/user.rb @@ -9,9 +9,8 @@ def initialize(datasource) protected - # Pylon exposes no POST and no DELETE on a user — an agent is invited and - # deactivated from Pylon itself — so only the update hook is wired and the - # other two refuse the verb with a message rather than a 500. + # Pylon exposes no POST and no DELETE on a user: an agent is invited and + # deactivated from Pylon itself, so the other two hooks are left to refuse. def update_record(id, payload) = datasource.client.update_user(id, payload) # `include_deactivated` is left at the client default of true on purpose: @@ -48,8 +47,7 @@ def define_relations end # `PATCH /users/{id}` takes the name, the avatar, the role and the status, - # and nothing else: an address is proven by the agent signing in, and the - # deactivation happens in Pylon. + # and nothing else. def define_schema add_column('id', 'String', is_primary_key: true) add_column('name', 'String', writable: true) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb index 4561b4afd..b01188427 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -1,23 +1,13 @@ module ForestAdminDatasourcePylon module Collections - # The write half of every Pylon collection: `create`, `update` and `delete`, - # the payload they send, and the ids a filter-driven write applies to. + # The write half of every Pylon collection. Included by `BaseCollection`, so + # a collection only declares the client calls, through the `*_record` hooks, + # and the fields Pylon accepts in one direction only. A hook left alone + # refuses the verb — no POST or DELETE on users, no DELETE on teams — + # instead of the contract's NotImplementedError, read by the agent as a 500. # - # Included by `BaseCollection`, so the mechanism is shared and each - # collection only declares what belongs to it: the client calls, through the - # `*_record` hooks, and the handful of fields Pylon accepts in one direction - # only. A hook a collection leaves alone refuses the verb, which is how the - # collections Pylon exposes no endpoint for — no POST or DELETE on users, no - # DELETE on teams — answer with a message instead of the contract's - # NotImplementedError, read by the agent as an unexpected 500. - # - # What may be written is not a list kept here: it is `is_read_only` on the - # column, the same way `api_filters` is the single source of truth for what - # may be filtered. A column the schema declares read-only is dropped from - # the payload, whether it is native, a foreign key or a custom field. - # - # Long by line count only: three verbs, the payload they share, and the - # refusals naming what Pylon cannot do. + # What may be written is `is_read_only` on the column, the way `api_filters` + # is what may be filtered: no second list to keep in step with the schema. module Writes # rubocop:disable Metrics/ModuleLength # Re-declared rather than borrowed from BaseCollection: a method defined # here resolves a constant against this module and its ancestors, never @@ -30,17 +20,12 @@ module Writes # rubocop:disable Metrics/ModuleLength # How many records one filter-driven update or delete may reach. Pylon # writes one record per request against a budget of 10 to 20 requests per - # minute, so a wider selection is refused rather than written halfway: - # a delete that stopped in the middle of the page would look done and - # would not be, which is the very thing this datasource refuses. + # minute, so a wider selection is refused rather than written halfway. MAX_WRITE_TARGETS = 20 - # A verb Pylon has no endpoint for is refused first, before the payload is - # built and before the ids are resolved: the refusal holds whatever the - # selection turns out to be, and everything the write would do on the way - # there answers with something else — the cap naming a count, a field of - # the wrong direction naming a field — sending the operator to narrow a - # selection that was never the problem. + # The refusal comes before the payload and the ids: everything on the way + # there answers with something else — a count, a field of the wrong + # direction — for a selection that was never the problem. def create(_caller, data) refuse_write('created') unless write_endpoint?(:create_record) @@ -49,10 +34,8 @@ def create(_caller, data) surface_write_rejection(e) end - # What the patch may write is settled before the ids are: a patch naming - # nothing writable sends no request and, more to the point, is not refused - # for reaching too many records — the cap bounds a write, and there is - # none here. + # What the patch may write is settled before the ids are, so a patch naming + # nothing writable is not refused for reaching too many records. def update(caller, filter, patch) refuse_write('updated') unless write_endpoint?(:update_record) @@ -81,9 +64,8 @@ def create_record(_payload) = refuse_write('created') def update_record(_id, _payload) = refuse_write('updated') def delete_record(_id) = refuse_write('deleted') - # The fields Pylon accepts on one endpoint and not on the other. The Forest - # schema carries a single read-only flag per column, so both directions - # offer them; these two lists are what tells them apart at write time. + # The Forest schema carries a single read-only flag per column, so both + # directions offer these; the two lists tell them apart at write time. def create_only_fields = [].freeze def update_only_fields = [].freeze @@ -93,23 +75,18 @@ def payload_renames = {}.freeze def max_write_targets = MAX_WRITE_TARGETS # How many named ids the collection's own read can resolve exactly. `nil` - # is no bound at all, the default: the search endpoint filters `id` - # server-side, so any id list is answered by one request per chunk. The - # collection resolving a named id by its own endpoint overrides this with - # its fan-out cap — past it the read truncates, and a truncated resolution - # would write to part of the selection while reporting the whole of it. + # is no bound: the search endpoint filters `id` server-side, so any id list + # costs one request per chunk. A collection reading an id through its own + # endpoint overrides this with its fan-out cap — past it the read truncates, + # and a truncated resolution writes to part of a selection it reports whole. def max_resolvable_ids = nil - # The records a filter-driven write applies to: exact, or refused. Nothing - # here may quietly answer with a subset — the caller writes one request per - # id and reports success for the whole selection. + # The records a filter-driven write applies to: exact, or refused — the + # caller writes one request per id and reports success for the whole + # selection, so a subset may never be answered quietly. # - # An `id equals`/`id in` filter alone is answered without a single request: - # that is what the record detail and the bulk selection of the UI send, and - # reading them back to learn ids they just named would spend the budget the - # writes themselves need. Anything else — a scope, a segment, a search, a - # condition on another column — is resolved by the collection's own `list`, - # so the scope applies and the endpoint filters what it can. + # An `id equals`/`id in` filter alone — what the record detail and the bulk + # selection send — costs no request. Anything else goes through `list`. def ids_for(caller, filter) tree = filter&.condition_tree if (named = id_values(tree)) && no_search?(filter) @@ -124,21 +101,15 @@ def ids_for(caller, filter) private - # Whether the collection wired the Pylon endpoint for a verb. The - # `*_record` hook is the declaration, read here rather than repeated in a - # list of supported verbs a collection would have to keep in step with its - # own hooks — the same reason `is_read_only` on the column, and not a - # second list of writable names, is what the payload builder reads. + # The `*_record` hook is the declaration that the collection wired the + # endpoint, read here rather than repeated in a list of supported verbs. def write_endpoint?(hook) method(hook).owner != Writes end - # One request per record, so a failure on the k-th record leaves the k-1 - # before it written — the cap bounds how many records a write reaches, - # nothing bounds the endpoint answering 429 or 422 halfway through. The - # error names the records that landed: raising the API error alone reads - # as "the write failed, nothing happened", and retrying the selection on - # that reading would write them a second time. + # One request per record, so a failure on the k-th leaves the k-1 before it + # written. The error names them: raising the API error alone reads as + # "nothing happened", and retrying on that reading would write them twice. def write_each(ids, verb) written = [] @@ -154,12 +125,14 @@ def write_each(ids, verb) end end - # Pylon's own refusal, in the operator's hands. A 4xx names something they - # did — a required field left out, a value the endpoint does not take, a - # record already gone — and travels as the ValidationError whose message - # the agent surfaces, where the APIError it arrived as would be answered - # with 'Unexpected error'. Anything else is Pylon or the network failing, - # which no edit of theirs would change: it stays what it was. + # A 4xx names something the operator did, and travels as the + # ValidationError whose message the agent surfaces where the APIError it + # arrived as would be answered with 'Unexpected error'. Anything else is + # Pylon or the network failing, which no edit of theirs would change. + # + # Only the write goes through here: a 4xx raised while resolving the + # selection still reaches them as a 500, reporting a read failure as a + # refused write being the worse of the two. def surface_write_rejection(error) raise error unless (400..499).cover?(error.status.to_i) @@ -167,8 +140,7 @@ def surface_write_rejection(error) end # One record past the cap is asked for, so an overflow is seen rather than - # guessed from a full page — the same bound `foreign_keys_matching` puts on - # a resolved relation condition. + # guessed from a full page. def resolve_ids_by_list(caller, filter) window = Page.new(offset: 0, limit: max_write_targets + 1) query = (filter || Filter.new).override(page: window) @@ -178,14 +150,11 @@ def resolve_ids_by_list(caller, filter) records.filter_map { |record| record['id'] }.uniq end - # The ids a filter names, whether as a leaf of its own or inside a - # top-level `and`. Unlike `extract_id_lookup`, nothing is asserted about - # what the rest of the tree can be applied in memory: the leftovers travel - # to `list`, which answers them the way a read does — server-side where the - # endpoint filters `id`, through the primary-key short-circuit where it - # does not. Nothing is asserted about the sibling conditions either, so - # this is a count of records *named*, never of records the write applies - # to: it answers what `max_resolvable_ids` needs, not what the cap does. + # The ids a filter names, as a leaf of its own or inside a top-level `and`. + # Unlike `extract_id_lookup`, nothing is asserted about the rest of the + # tree — the leftovers travel to `list` — nor about the sibling conditions, + # so this counts records *named*, never records the write applies to. The + # first id leaf of an `and` of two wins, over-refusing a narrower one. def filtered_ids(node) named = id_values(node) return named if named @@ -207,10 +176,9 @@ def build_payload(attributes, direction, caller: nil, ids: []) payload end - # Keys the schema declares writable, custom fields included. Everything - # else is dropped rather than refused: the front sends the fields of its - # form, and a read-only one reaching the payload is the agent's doing, not - # a request the operator made. + # Everything else is dropped rather than refused: the front sends the + # fields of its form, and a read-only one reaching the payload is the + # agent's doing, not a request the operator made. def writable_attributes(data) attrs = data.is_a?(Hash) ? data.transform_keys(&:to_s) : {} @@ -224,17 +192,13 @@ def writable_column?(field) end # A field of the other direction is dropped when it asks for nothing, and - # refused when the operator really changed it: Pylon cannot write it, and - # answering the edit with a success it did not perform is worse than an - # error naming the field. + # refused when the operator really changed it: answering an edit with a + # success Pylon did not perform is worse than an error naming the field. # - # What "asks for nothing" means differs by direction, and only a create - # can tell without reading. Pylon fills a create in with exactly what a - # blank value asks for, so a blank one is dropped there. On an update the - # record already holds a value, and the only thing that settles whether - # the patch changes it is that value: an unchecked box is nothing to write - # over a stored `false`, and a real edit over a stored `true`. Blankness - # alone would drop the second, reporting an edit Pylon never performed. + # Only a create can tell without reading, Pylon filling it in with exactly + # what a blank value asks for. On an update the stored value is what + # settles it — an unchecked box is nothing over a stored `false`, and a + # real edit over a stored `true`. def honour_write_direction(attrs, direction, caller, ids) wrong = attrs.keys & (direction == :create ? update_only_fields : create_only_fields) return attrs if wrong.empty? @@ -260,10 +224,8 @@ def blank_write_value?(value) end # The wrong-direction fields already holding the value the patch asks for. - # A record the read did not hand back counts as none of them, and one - # missing record is enough: the field is refused rather than dropped, since - # nothing here may claim a value is unchanged on a record it never read -- - # which a selection where only some ids came back would otherwise do. + # One record the read did not hand back is enough to refuse them all: + # nothing here may claim a value is unchanged on a record it never read. def unchanged_fields(caller, ids, fields, attrs) return [] if fields.empty? @@ -273,16 +235,11 @@ def unchanged_fields(caller, ids, fields, attrs) fields.select { |field| stored.all? { |record| same_write_value?(record[field], attrs[field]) } } end - # Whether the patch asks for the value the record already holds. - # # Two blanks are the same state: Pylon returns a null where the form sends - # `false` or an empty string for the same untouched field, and refusing - # that pair would fail every edit whose form carries one. - # - # Strings are compared stripped: `body_html` travels through an editor - # that may hand back the markup it was given re-indented, and refusing an - # edit nobody made — naming a field the operator never touched — is the - # one error they cannot act on. + # `false` or an empty string for the same untouched field. Strings are + # compared stripped, `body_html` travelling through an editor that may hand + # back the markup it was given re-indented — and refusing an edit nobody + # made is the one error the operator cannot act on. def same_write_value?(stored, asked) return true if blank_write_value?(stored) && blank_write_value?(asked) return stored.to_s.strip == asked.to_s.strip if stored.is_a?(String) || asked.is_a?(String) @@ -291,15 +248,14 @@ def same_write_value?(stored, asked) end # Read only when the patch names a field of the wrong direction, and only - # for those fields: an update naming none costs no request at all. It is - # one request on the collections whose endpoint filters `id`, and one per - # record on the ones reading an id through its own endpoint — PylonIssue, - # whose fan-out `max_resolvable_ids` bounds. + # for those fields. One request where the endpoint filters `id`, one per + # record where an id is read through its own endpoint — which + # `max_write_targets` does not count, so a wide update naming such a field + # spends two requests per record against the write budget. # - # Read by id rather than through the caller's filter: the filter was - # already resolved into these ids, so re-running it would spend those - # requests a second time and — carrying no page of its own — walk every - # record it matches rather than the handful about to be written. + # By id rather than through the caller's filter: that filter was already + # resolved into these ids, so re-running it would spend those requests + # twice and, carrying no page, walk every record it matches. def stored_values(caller, ids, fields) query = Filter.new(condition_tree: Leaf.new('id', Operators::IN, ids), page: Page.new(offset: 0, limit: ids.size)) @@ -308,9 +264,8 @@ def stored_values(caller, ids, fields) end # Pylon reads its custom fields back as a map indexed by slug and writes - # them as a list, one entry per field, carrying `values` for a multi-value - # field and `value` for every other — a select being written by the slug of - # its option, which is what the Enum column advertises. + # them as a list, `values` for a multi-value field and `value` for every + # other — a select by the slug of its option, what the Enum advertises. def split_custom_fields(attrs) by_column = custom_fields_by_column entries = [] @@ -359,9 +314,8 @@ def refuse_too_many_targets(count) refuse_write_reach("applies to #{count} #{name} records, more than the #{max_write_targets} one pass covers") end - # The resolution asks for one record past the cap, so all it knows is that - # the selection overflows: reporting the size of its window as a count - # would name 21 records to an operator whose selection holds thousands. + # The resolution only knows the selection overflows: reporting the size of + # its window would name 21 records to a selection holding thousands. def refuse_unbounded_targets refuse_write_reach("applies to more than the #{max_write_targets} #{name} records one pass covers") end @@ -373,11 +327,8 @@ def refuse_write_reach(reach) 'Narrow the selection to reach the records past this point.' end - # Named ids the collection cannot resolve exactly, the filter carrying - # more than the ids themselves. How many of them the rest of the filter - # matches is unknown here — it is what the read would answer — so the - # count is reported as what it is, records named rather than records - # written to. + # How many of the named ids the rest of the filter matches is unknown here, + # so the count is reported as what it is: records named. def refuse_unresolvable_selection(count) raise UnsupportedWriteError, "This write names #{count} #{name} records and filters them further, which #{name} answers with " \ diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb index 163cb0aea..5b1224a86 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/retry_policy.rb @@ -19,21 +19,17 @@ class RetryPolicy Faraday::RetriableResponse, Faraday::ConnectionFailed ].freeze - # The verbs a replay cannot repeat: a GET, a HEAD and an OPTIONS change - # nothing, so any transient failure is worth another attempt. - # - # DELETE is deliberately out, although HTTP calls it idempotent and - # faraday-retry ships it as a default: a 502 or a dropped connection on the - # way back from a DELETE Pylon did perform is replayed into a 404, which the - # write path then surfaces as a deletion that failed when it landed -- the - # very report of something that did not happen this datasource refuses. PUT - # is out for having no endpoint: Pylon writes through POST and PATCH. + # The verbs that change nothing, so any transient failure is worth another + # attempt. Narrower than faraday-retry's idempotent default: a 502 or a + # dropped connection on the way back from a DELETE Pylon did perform is + # replayed into a 404, which the write path then surfaces as a deletion that + # failed when it landed. # # A 429 stays safe to retry on any verb, Pylon having rejected the request # before processing it, and travels through retry_if rather than through this # list: faraday-retry ORs the two, so methods can only widen the set, never # restrict it. - IDEMPOTENT_METHODS = %i[get head options].freeze + RETRYABLE_METHODS = %i[get head options].freeze RETRY_IF = ->(env, _exception) { env[:status] == 429 } BACKOFF_FACTOR = 2 @@ -54,7 +50,7 @@ def to_faraday_options backoff_factor: BACKOFF_FACTOR, retry_statuses: STATUSES, exceptions: EXCEPTIONS, - methods: IDEMPOTENT_METHODS, + methods: RETRYABLE_METHODS, retry_if: RETRY_IF } end diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb index 1c0c7c0a1..9f3040654 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb @@ -108,9 +108,8 @@ def build_entry(raw, object_type) # A custom field is writable unless Pylon says otherwise: it flags the ones # synced from an app or an integration, which its own endpoints refuse. # Nothing is sortable -- no Pylon endpoint takes a sort parameter -- and - # nothing is groupable, as Pylon aggregates nothing: one column left - # groupable turns `supportGroups` on for the whole collection, and the - # group-by the UI then offers errors. + # nothing is groupable: one column left groupable turns `supportGroups` on + # for the whole collection, and the group-by the UI then offers errors. def build_schema(raw, column_type) opts = { column_type: column_type, filter_operators: OPERATORS.fetch(column_type, []), From dda783aeaedb289636e454743bb97857402df108 Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 21 Aug 2026 18:50:37 +0200 Subject: [PATCH 7/8] fix(pylon): charge a write the reads it spends resolving its selection MAX_WRITE_TARGETS counted the writes of a pass and nothing else, while PylonIssue reads a record through its own endpoint: resolving a selection cost a request per named record and reading a stored value cost another, so a filter-driven update could spend 60 requests against the budget of ten to twenty per minute the cap exists to respect. Past it the 429 outlives its retries and the write stops mid-selection, which is the half-written write the cap was there to refuse. The constant becomes MAX_WRITE_REQUESTS, the budget of a whole pass, and the reach is derived from what one record of the write costs: requests_per_record_read is zero where the search endpoint filters `id` (a selection travels in one request whatever its size) and one on PylonIssue. A named selection still reaches twenty; one resolved by reading each named id, or compared against a stored value, reaches ten, and six when it does both. Nothing changes on the four collections whose read does not fan out. The resolution is charged per record only when the filter names ids: any other selection is resolved by one page of the collection's own read, whose cost does not grow with the count, so it keeps the full reach. max_resolvable_ids follows the same arithmetic and stays clamped to the primary-key fan-out, which no longer binds at these numbers. Worst case falls from 60 requests to 21. Co-Authored-By: Claude Opus 5 (1M context) --- .../collections/issue.rb | 20 +++-- .../collections/writes.rb | 90 ++++++++++++------- .../collections/writes_spec.rb | 62 +++++++++++++ 3 files changed, 134 insertions(+), 38 deletions(-) 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 4da069547..1dac463f2 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 @@ -78,13 +78,19 @@ def delete_record(id) = datasource.client.delete_issue(id) def create_only_fields = CREATE_ONLY def update_only_fields = UPDATE_ONLY - def max_write_targets = [Writes::MAX_WRITE_TARGETS, MAX_ID_LOOKUPS].min - - # Never past the primary-key fan-out: a write resolving named ids through - # `list` goes through `fetch_by_ids`, which truncates with a warning past - # this many, and a truncated resolution would write to a subset of the - # selection while reporting the whole of it. - def max_resolvable_ids = MAX_ID_LOOKUPS + # An issue is read through `GET /issues/{id}`, one request per record: a + # selection resolved or compared that way spends the write budget twice + # over, so it divides the records one pass reaches rather than fitting + # beside them. + def requests_per_record_read = 1 + + # Never past the primary-key fan-out either: a write resolving named ids + # through `list` goes through `fetch_by_ids`, which truncates with a + # warning past this many, and a truncated resolution would write to a + # subset of the selection while reporting the whole of it. The budget is + # the tighter of the two at today's numbers; the clamp keeps that true if + # either moves. + def max_resolvable_ids(reads: 0) = [super, MAX_ID_LOOKUPS].min def sortable_fields PYLON_SORTABLE diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb index b01188427..313e46b39 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb @@ -18,10 +18,15 @@ module Writes # rubocop:disable Metrics/ModuleLength Leaf = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf Operators = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Operators - # How many records one filter-driven update or delete may reach. Pylon - # writes one record per request against a budget of 10 to 20 requests per - # minute, so a wider selection is refused rather than written halfway. - MAX_WRITE_TARGETS = 20 + # What one filter-driven update or delete may spend, in requests. Pylon + # allows 10 to 20 per minute, so a costlier selection is refused rather + # than written halfway. + # + # The budget covers the whole pass, not its writes: where a record is read + # through its own endpoint, resolving the selection costs a request per + # record and reading a stored value costs another, so a cap counting the + # writes alone would let one pass spend three times this. + MAX_WRITE_REQUESTS = 20 # The refusal comes before the payload and the ids: everything on the way # there answers with something else — a count, a field of the wrong @@ -42,7 +47,7 @@ def update(caller, filter, patch) attributes = writable_attributes(patch) return if attributes.empty? - ids = ids_for(caller, filter) + ids = ids_for(caller, filter, extra_reads: stored_read?(attributes) ? 1 : 0) return if ids.empty? payload = build_payload(attributes, :update, caller: caller, ids: ids) @@ -72,14 +77,25 @@ def update_only_fields = [].freeze # Columns whose Pylon write name differs from the one they are read under. def payload_renames = {}.freeze - def max_write_targets = MAX_WRITE_TARGETS + # What reading one record costs here. Nothing where the search endpoint + # filters `id`, a whole selection travelling in one request whatever its + # size; one request where an id is read through its own endpoint. + def requests_per_record_read = 0 + + # How many records one write may reach: the budget divided by what each of + # them costs — the write itself, plus the `reads` the path still owes it. + def max_write_targets(reads: 0) + MAX_WRITE_REQUESTS / (1 + (reads * requests_per_record_read)) + end - # How many named ids the collection's own read can resolve exactly. `nil` - # is no bound: the search endpoint filters `id` server-side, so any id list - # costs one request per chunk. A collection reading an id through its own - # endpoint overrides this with its fan-out cap — past it the read truncates, - # and a truncated resolution writes to part of a selection it reports whole. - def max_resolvable_ids = nil + # How many ids a filter may name before the resolution is refused rather + # than spent: the same reach, a named id being read before it is written + # to. `nil` is no bound, a read costing nothing per record. + def max_resolvable_ids(reads: 0) + return nil if requests_per_record_read.zero? + + max_write_targets(reads: reads) + end # The records a filter-driven write applies to: exact, or refused — the # caller writes one request per id and reports success for the whole @@ -87,16 +103,23 @@ def max_resolvable_ids = nil # # An `id equals`/`id in` filter alone — what the record detail and the bulk # selection send — costs no request. Anything else goes through `list`. - def ids_for(caller, filter) + def ids_for(caller, filter, extra_reads: 0) tree = filter&.condition_tree if (named = id_values(tree)) && no_search?(filter) - refuse_too_many_targets(named.size) if named.size > max_write_targets + cap = max_write_targets(reads: extra_reads) + refuse_too_many_targets(named.size, cap) if named.size > cap return named end - named_count = max_resolvable_ids && filtered_ids(tree)&.size - refuse_unresolvable_selection(named_count) if named_count && named_count > max_resolvable_ids - resolve_ids_by_list(caller, filter) + # A selection naming ids is resolved by reading each of them; any other + # one by a single page of the collection's own read, whose cost does not + # grow with the count. + named_ids = filtered_ids(tree) + reads = extra_reads + (named_ids ? 1 : 0) + bound = named_ids && max_resolvable_ids(reads: reads) + refuse_unresolvable_selection(named_ids.size, bound) if bound && named_ids.size > bound + + resolve_ids_by_list(caller, filter, reads: reads) end private @@ -141,11 +164,12 @@ def surface_write_rejection(error) # One record past the cap is asked for, so an overflow is seen rather than # guessed from a full page. - def resolve_ids_by_list(caller, filter) - window = Page.new(offset: 0, limit: max_write_targets + 1) + def resolve_ids_by_list(caller, filter, reads:) + cap = max_write_targets(reads: reads) + window = Page.new(offset: 0, limit: cap + 1) query = (filter || Filter.new).override(page: window) records = list(caller, query, Projection.new(['id'])) - refuse_unbounded_targets if records.size > max_write_targets + refuse_unbounded_targets(cap) if records.size > cap records.filter_map { |record| record['id'] }.uniq end @@ -191,6 +215,11 @@ def writable_column?(field) column&.type == 'Column' && !column.is_read_only end + # Whether the patch will have `stored_values` read every record it reaches + # before a field of the wrong direction is dropped or refused, which the + # cap has to charge it for: see `ids_for`. + def stored_read?(attributes) = (attributes.keys & create_only_fields).any? + # A field of the other direction is dropped when it asks for nothing, and # refused when the operator really changed it: answering an edit with a # success Pylon did not perform is worse than an error naming the field. @@ -249,9 +278,9 @@ def same_write_value?(stored, asked) # Read only when the patch names a field of the wrong direction, and only # for those fields. One request where the endpoint filters `id`, one per - # record where an id is read through its own endpoint — which - # `max_write_targets` does not count, so a wide update naming such a field - # spends two requests per record against the write budget. + # record where an id is read through its own endpoint — which the cap does + # charge the patch for, `stored_read?` declaring it before the ids are + # resolved. # # By id rather than through the caller's filter: that filter was already # resolved into these ids, so re-running it would spend those requests @@ -310,14 +339,14 @@ def refuse_wrong_direction(fields, direction) end # The count is exact here, the filter having named the ids. - def refuse_too_many_targets(count) - refuse_write_reach("applies to #{count} #{name} records, more than the #{max_write_targets} one pass covers") + def refuse_too_many_targets(count, cap) + refuse_write_reach("applies to #{count} #{name} records, more than the #{cap} one pass covers") end # The resolution only knows the selection overflows: reporting the size of # its window would name 21 records to a selection holding thousands. - def refuse_unbounded_targets - refuse_write_reach("applies to more than the #{max_write_targets} #{name} records one pass covers") + def refuse_unbounded_targets(cap) + refuse_write_reach("applies to more than the #{cap} #{name} records one pass covers") end def refuse_write_reach(reach) @@ -329,12 +358,11 @@ def refuse_write_reach(reach) # How many of the named ids the rest of the filter matches is unknown here, # so the count is reported as what it is: records named. - def refuse_unresolvable_selection(count) + def refuse_unresolvable_selection(count, bound) raise UnsupportedWriteError, "This write names #{count} #{name} records and filters them further, which #{name} answers with " \ - "one request per named record, more than the #{max_resolvable_ids} one pass reads: the resolution " \ - 'would stop short and the write would then cover part of the selection while reporting all of ' \ - 'it. Select fewer records, or drop the other conditions to write the ones named.' + "one request per named record, on top of the one each write costs: more than the #{bound} one " \ + 'pass covers. Select fewer records, or drop the other conditions to write the ones named.' end def refuse_partial_write(verb, written, failed_id, total, error) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb index e2fba5bbd..a7a668a2f 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -473,6 +473,68 @@ def issue_payload(id, overrides = {}) expect(WebMock).not_to have_requested(:get, %r{/issues/}) end + # The cap is a budget of requests, not of writes: where a record is read + # through its own endpoint, every read the path owes it comes out of the + # same twenty, so the records one pass reaches halve for each of them. + it 'halves the reach when the patch has every record read before it is written' do + ids = Array.new(11) { |index| "i#{index}" } + + expect { issues.update(nil, id_filter(operators::IN, ids), 'body_html' => '

x

', 'title' => 'Louder') } + .to raise_error(UnsupportedWriteError, /applies to 11 PylonIssue records, more than the 10 one pass/) + expect(WebMock).not_to have_requested(:get, %r{/issues/}) + expect(WebMock).not_to have_requested(:patch, %r{/issues/}) + end + + # The same eleven records, with nothing to read before writing to them. + it 'keeps the full reach when the patch owes the records no read' do + stub_request(:patch, %r{/issues/i\d+}).to_return(json('data' => issue_payload('i1'))) + + issues.update(nil, id_filter(operators::IN, Array.new(11) { |index| "i#{index}" }), 'title' => 'Louder') + + expect(WebMock).to have_requested(:patch, %r{/issues/i\d+}).times(11) + end + + # A selection naming ids is resolved by reading each of them, so it is + # bounded by the same halved reach — and refused before the first of those + # reads rather than after twenty of them. + it 'refuses a resolution costing a request per record past the halved reach' do + ids = Array.new(12) { |index| "i#{index}" } + + expect do + issues.update(nil, filter(condition_tree: branch('And', [leaf('id', operators::IN, ids), + leaf('state', operators::EQUAL, 'new')])), + 'title' => 'Louder') + end.to raise_error(UnsupportedWriteError, /names 12 PylonIssue records .* more than the 10 one pass covers/m) + expect(WebMock).not_to have_requested(:get, %r{/issues/}) + end + + # A selection naming no id is resolved by one page of the search endpoint, + # whose cost does not grow with the count: nothing to charge per record, so + # the full reach stands. + it 'keeps the full reach when the resolution costs one request whatever the count' do + stub_request(:post, "#{base}/issues/search") + .to_return(json('data' => Array.new(21) { |index| issue_payload("i#{index}") })) + + expect { issues.delete(nil, filter(condition_tree: leaf('state', operators::EQUAL, 'new'))) } + .to raise_error(UnsupportedWriteError, /more than the 20 PylonIssue records one pass covers/) + expect(WebMock).to have_requested(:post, "#{base}/issues/search").once + end + + # And nothing is charged per record where a record costs no request of its + # own: the search endpoint filters `id`, so reading the stored value of a + # whole selection is one request, whatever the reach. + it 'keeps the full reach on a collection whose read does not fan out' do + ids = Array.new(11) { |index| "c#{index}" } + stored = ids.map { |id| { 'id' => id, 'name' => 'Ada', 'email' => 'ada@acme.test' } } + stub_request(:post, "#{base}/contacts/search").to_return(json('data' => stored)) + stub_request(:patch, %r{/contacts/c\d+}).to_return(json('data' => stored.first)) + + contacts.update(nil, id_filter(operators::IN, ids), 'email' => 'ada@acme.test', 'name' => 'Ada Lovelace') + + expect(WebMock).to have_requested(:post, "#{base}/contacts/search").once + expect(WebMock).to have_requested(:patch, %r{/contacts/c\d+}).times(11) + end + # "Select all except these" reaches PylonIssue as `id not_in`, which its # endpoint cannot filter: the read refuses it, and so does the delete. The # message names that selection rather than the `and`/`or` of a filter the From 44cd3c2d02143fda25db39959d194cf3cf61154e Mon Sep 17 00:00:00 2001 From: Brun Christophe Date: Fri, 21 Aug 2026 19:00:34 +0200 Subject: [PATCH 8/8] fix(pylon): leave a custom field read-only until Pylon flags it writable `is_read_only == true` read the absence of the flag as "editable", so a definition Pylon returns without it -- a renamed key, an endpoint that does not carry it, a type predating it -- opened every custom field of the collection to writes, the ones synced from an app included, whose every save Pylon then rejects. This datasource advertises nothing an endpoint would refuse, so the absence is read the other way: only an explicit false opens a field, and an unflagged definition is left read-only and reported once, the capability being the cheaper of the two losses and the warning making a missing flag diagnosable. Also covers Team#create, the one create whose record is serialized by a collection read in whole: POST /teams answers with the members nested where the column carries their ids, so the flattening of the read side has to run on a write response too. 647 examples, 0 failures; coverage 1305/1305 lines, RuboCop clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../schema/custom_fields_introspector.rb | 26 +++++++++++++++++-- .../collections/writes_spec.rb | 16 ++++++++++++ .../schema/custom_fields_introspector_spec.rb | 13 +++++++--- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb index 9f3040654..c98e8827f 100644 --- a/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb +++ b/packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/schema/custom_fields_introspector.rb @@ -105,7 +105,7 @@ def build_entry(raw, object_type) multi_value: MULTI_VALUE_TYPES.include?(raw['type']) } end - # A custom field is writable unless Pylon says otherwise: it flags the ones + # A custom field is writable when Pylon says it is: it flags the ones # synced from an app or an integration, which its own endpoints refuse. # Nothing is sortable -- no Pylon endpoint takes a sort parameter -- and # nothing is groupable: one column left groupable turns `supportGroups` on @@ -113,7 +113,7 @@ def build_entry(raw, object_type) def build_schema(raw, column_type) opts = { column_type: column_type, filter_operators: OPERATORS.fetch(column_type, []), - is_read_only: raw['is_read_only'] == true, + is_read_only: !writable_definition?(raw), is_sortable: false, is_groupable: false } @@ -141,6 +141,28 @@ def option_slugs(raw) end end + # Only an explicit `false` opens a custom field to writes. A definition + # carrying no flag at all is left read-only and reported: this datasource + # advertises nothing an endpoint would refuse, and reading the absence as + # "writable" would turn every field synced from an app into an editor whose + # every save Pylon rejects -- where reading it as "read-only" costs the + # capability and says so once per boot. + def writable_definition?(raw) + return true if raw['is_read_only'] == false + return false if raw['is_read_only'] == true + + warn_unflagged_writability(raw) + false + end + + def warn_unflagged_writability(raw) + ForestAdminDatasourcePylon.logger.warn( + "[forest_admin_datasource_pylon] Custom field '#{raw["slug"]}' carries no 'is_read_only' flag; " \ + 'leaving it read-only. Pylon refuses a write on the fields it syncs from an app or an integration, ' \ + 'and nothing here can tell this one apart from those without the flag.' + ) + end + def warn_unknown_type(raw, slug, object_type) ForestAdminDatasourcePylon.logger.warn( "[forest_admin_datasource_pylon] Custom field '#{slug}' on #{object_type} has type " \ diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb index a7a668a2f..3d0252522 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/collections/writes_spec.rb @@ -637,6 +637,22 @@ def issue_payload(id, overrides = {}) .with(body: { 'name' => 'Ada', 'account_id' => 'a1' }) end + # The only create whose record is serialized by a collection read in whole: + # `POST /teams` answers with the members nested where the column carries + # their ids, so the flattening the read side does has to run here too. + it 'creates a team and flattens the members of the record it answers with' do + stub_request(:post, "#{base}/teams") + .to_return(json('data' => { 'id' => 't1', 'name' => 'Support', + 'users' => [{ 'id' => 'u1', 'email' => 'ada@acme.test' }, + { 'id' => 'u2' }] })) + + record = teams.create(nil, 'name' => 'Support', 'user_ids' => %w[u1 u2]) + + expect(record).to eq('id' => 't1', 'name' => 'Support', 'user_ids' => %w[u1 u2]) + expect(WebMock).to have_requested(:post, "#{base}/teams") + .with(body: { 'name' => 'Support', 'user_ids' => %w[u1 u2] }) + end + it 'replaces the members of a team' do stub_request(:patch, "#{base}/teams/t1").to_return(json('data' => { 'id' => 't1', 'name' => 'Support' })) diff --git a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb index f84a4c986..ea2c78af2 100644 --- a/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb +++ b/packages/forest_admin_datasource_pylon/spec/forest_admin_datasource_pylon/schema/custom_fields_introspector_spec.rb @@ -182,13 +182,18 @@ def operators_of(type, **extra) expect(introspector.issue_custom_fields.first[:schema].is_read_only).to be(false) end - # Anything other than a true flag reads as editable, which is what a - # definition predating the flag is. - it 'is writable when Pylon declares nothing' do + # A definition carrying no flag is left read-only: this datasource + # advertises nothing an endpoint would refuse, and the fields Pylon syncs + # from an app are exactly the ones the flag tells apart, so reading its + # absence as "editable" would offer an editor whose every save is rejected. + it 'is read-only, and says so, when Pylon declares nothing' do + allow(ForestAdminDatasourcePylon.logger).to receive(:warn) allow(client).to receive(:fetch_custom_fields).with('issue') .and_return([definition('text', 'is_read_only' => nil)]) - expect(introspector.issue_custom_fields.first[:schema].is_read_only).to be(false) + expect(introspector.issue_custom_fields.first[:schema].is_read_only).to be(true) + expect(ForestAdminDatasourcePylon.logger) + .to have_received(:warn).with(/carries no 'is_read_only' flag; leaving it read-only/) end # `ColumnSchema` defaults this one to true, and the capabilities route turns