feat(datasource-pylon): write operations (CRUD) (EXT-11) - #362
Merged
Conversation
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) <noreply@anthropic.com>
11 new issues
|
| # `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) |
| # 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) |
| # so the scope applies and the endpoint filters what it can. | ||
| def ids_for(caller, filter) | ||
| tree = filter&.condition_tree | ||
| ids = filtered_ids(tree) |
There was a problem hiding this comment.
🟠 High collections/writes.rb:81
ids_for returns duplicate IDs from the direct id in shortcut, so update and delete invoke the write endpoint repeatedly for the same record; a second delete can fail after the first succeeds, and duplicates can also incorrectly trigger the 20-target limit. Deduplicate the extracted IDs before enforcing the cap and returning the shortcut result.
- ids = filtered_ids(tree)
+ ids = filtered_ids(tree)&.uniq🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_pylon/lib/forest_admin_datasource_pylon/collections/writes.rb around line 81:
`ids_for` returns duplicate IDs from the direct `id in` shortcut, so `update` and `delete` invoke the write endpoint repeatedly for the same record; a second delete can fail after the first succeeds, and duplicates can also incorrectly trigger the 20-target limit. Deduplicate the extracted IDs before enforcing the cap and returning the shortcut result.
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) <noreply@anthropic.com>
| raise if written.empty? | ||
|
|
||
| refuse_partial_write(verb, written, id, ids.size, e) | ||
| end |
| # 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, ids) |
| # 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) |
| '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) |
- 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) <noreply@anthropic.com>
| end | ||
|
|
||
| # The writable attributes, in the shape the endpoint takes them. | ||
| def build_payload(attributes, direction, caller: nil, ids: []) |
| end | ||
| refuse_wrong_direction(asked, direction) unless asked.empty? | ||
|
|
||
| attrs.except(*wrong) |
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
| payload = build_payload(attributes, :update, caller: caller, ids: ids) | ||
| return if payload.empty? | ||
|
|
||
| write_each(ids, 'updated') { |id| update_record(id, payload) } |
| 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) |
`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) <noreply@anthropic.com>
christophebrun-forest
merged commit Aug 21, 2026
46e1e5b
into
feat/datasource-pylon
52 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes EXT-11 — Story 7 of EXT-4.
What
The datasource was read-only: every column declared
is_read_only: true, andcreate/update/deletefell through to the contract'sNotImplementedError. This opens the write half — the endpoints, the payload, and the ids a filter-driven write applies to — through a mechanism the three collection bases share.client/writes.rbpost/patch/deletecollections/writes.rbcreate/update/delete, the payload builder,ids_forcollections/base_collection.rbadd_column(writable:);no_search?moves up;id_valuesdeduplicatescollections/fetch_all_collection.rbadd_column(writable:)for the two in-memory collectionsissue.rb/account.rb/contact.rb/team.rb/user.rbschema_definition.rbwritable: trueon the columns Pylon acceptsschema/custom_fields_introspector.rbis_read_only; carriesmulti_valueforest_admin_datasource_pylon.rbUnsupportedWriteError,PartialWriteError,WriteRejectedErrorWhat may be written is the schema, not a second list
is_read_onlyon the column is the single source of truth the payload builder reads, the wayapi_filtersalready is for filtering. A column the schema declares read-only is dropped from the payload — native, foreign key or custom field alike — and nothing here keeps a parallel list of writable names that could drift from what the UI offers.A column is
writable: truewhenPOSTorPATCHaccepts it in the shape it is read under. That last clause is what leaves severalJsoncolumns read-only although the endpoint takes something by that name:external_ids,phone_numbers,channelshold{external_id, label}-style objects, and the write shape the reference documents is not the one the column shows — writing one for the other would replace the account's ids with something Pylon cannot read back.domainandprimary_domainstay read-only for a different reason: they are two projections of thedomainslist, and writing one would leave the other stale, sodomainsis the writable one. Everything Pylon computes — the issue number, the link, the timestamps, the counters,source,role_name,is_deactivated— stays read-only.A foreign key is declared writable even though the emitted schema will not carry it.
GeneratorFieldforcesisReadOnlyon a foreign key whatever the datasource says, so the detail view has one editor per key rather than two. What the flag opens is that editor: theBelongsToreads its own read-only state off the key column, and the front sends the choice back as the very column named here.account_id,requester_id,assignee_id,team_idon an issue,account_idon a contact.A custom field is writable when Pylon flags it writable. EXT-10 declared every one read-only for lack of a write endpoint; the definition's own
is_read_onlyflag is now what decides, Pylon setting it on the fields synced from an app or an integration, which its own endpoints refuse. Only an explicitfalseopens a field: a definition carrying no flag at all — a renamed key, an endpoint that does not return it, a type predating it — is left read-only and reported once per boot, rather than read as editable. Reading the absence the other way would open every custom field of the collection, the synced ones included, and offer an editor whose every save Pylon rejects; this way costs the capability and says so.Five decisions worth reviewing
A verb Pylon has no endpoint for refuses with a message. There is no
POSTorDELETEon a user — an agent is invited and deactivated from Pylon itself — and noDELETEon a team. Left alone, the toolkit'sNotImplementedErrorreaches the operator as an unexpected 500 with nothing in it. The three*_recordhooks default torefuse_write, so a collection wires only the verbs that exist and the others answerA PylonUser record cannot be created: the Pylon API exposes no endpoint for it.The hook is also whatwrite_endpoint?reads to refuse before the payload is built and the ids are resolved, rather than a second list of supported verbs a collection would have to keep in step with its own hooks: the refusal holds whatever the selection turns out to be, and everything the write would otherwise 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, or answering a selection matching no record with a success on a collection that cannot perform one.UnsupportedWriteErrordescends fromValidationErrorfor the same reasonUnsupportedOperatorErrordoes: it names something the operator did and can undo, and the message is the only place they learn what.A field Pylon accepts in one direction only is dropped when it asks for nothing and refused when it does. The Forest schema carries a single read-only flag per column, so both directions offer
body_html,state,type,is_disabled,emails;create_only_fields/update_only_fieldsis what tells them apart at write time. A form resending an untouched field is not an edit, so a field asking for nothing is dropped silently, and a value the operator really changed is refused, naming the field and the direction — answering an edit with a success Pylon did not perform is worse than an error.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 that value is the only thing settling whether the patch changes it — an unchecked box is nothing to write over a stored
false, and a real edit over a storedtrue. Blankness alone would drop the second and report an edit Pylon never performed. Two blanks count as the same state, so an unchecked box over a null still costs nothing, and strings compare stripped, so an editor handing back the markup it was given re-indented does not refuse an edit nobody made.The read this costs is one request on the collections whose endpoint filters
id, and one per record onPylonIssue, which reads an id through its own endpoint — charged only on an update whose patch names a field of the wrong direction, and only for those fields. It is charged against the write budget as well:stored_read?declares it before the ids are resolved, so the cap knows what the patch will owe every record it reaches.Pylon's own refusal reaches the operator.
APIErrordescends from the package'sError, which the agent'sErrorTranslatordoes 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 arrive as nothing at all. A 4xx is re-raised asWriteRejectedError, aValidationErrorwhose message the agent surfaces; a 5xx or a dropped connection is not the operator's to act on and stays theAPIErrorit was, carrying its status.PartialWriteErroralready travelled this way, and it is the write that made it worth it: a read failing costs a page, where a write failing costs the operator the reason their edit did not land.A filter-driven write resolves its ids exactly or refuses. An
id equals/id infilter with no search 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 need. Anything else — a scope, a segment, a search, a condition on another column — goes through the collection's ownlist, so the scope applies and the endpoint filters what it can.filtered_idsreads the ids off a bare leaf or a top-levelandand asserts nothing about the rest of the tree: the leftovers travel tolist, which answers them the way a read does.A selection costing more than one pass is refused rather than written halfway.
MAX_WRITE_REQUESTS = 20, and it is the budget of a whole pass rather than of its writes: Pylon writes one record per request against a quota of ten to twenty per minute, so a delete that stopped in the middle of the page would look done and would not be — the very failure this datasource refuses elsewhere.What the cap bounds is therefore derived from what one record of the write costs.
requests_per_record_readis zero where the search endpoint filtersid, a whole selection travelling in one request whatever its size, and one onPylonIssue, which reads an id throughGET /issues/{id}: a named selection there still reaches twenty, one resolved by reading each named id reaches ten, and six when the patch also has a stored value read. The resolution is charged per record only when the filter names ids — any other selection is answered by a single page of the collection's own read, whose cost does not grow with the count, so it keeps the full reach.The window still asks for one record past the cap, so an overflow is seen rather than guessed from a full page, the same bound
foreign_keys_matchingputs on a resolved relation condition.max_resolvable_idsfollows the same arithmetic and stays clamped toMAX_ID_LOOKUPS: past that fan-outfetch_by_idstruncates with a warning, and a truncated resolution would write to a subset while reporting the whole. The budget is the tighter of the two at today's numbers; the clamp keeps that true if either moves.Two shapes on the wire
A custom field is written as a list of entries, not a map.
{'slug' => …, 'value' => …}per field undercustom_fields,valuesfor a multiselect, and a select by the slug of its option — which is what theEnumcolumn advertises, so the round trip is closed with the read side EXT-10 pinned. The introspector now carriesmulti_valueon each entry for that one branch.A write answers with the record under
data, or it is a broken contract.extract_datahands the body back untouched whendatais 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.extract_writtenraises a typedAPIErrornaming the operation and the body instead. Nothing in this file degrades —best_effortexists for the calls whose result enriches a page, where a missing thread costs a column, whereas a write that silently did nothing would tell the operator their edit landed.Also on the wire: a
nilis dropped on a create, Pylon filling in what is left out, and travels on an update, where it is the operator clearing a value. And the id is escaped before being joined to the path, like every read does.Four deviations from the plan
The custom-field payload is a list, not
{slug: {value}}. The scope spelled the map shape;POST /issuesand the other write endpoints takecustom_fieldsas an array of{slug, value}/{slug, values}objects. The list is what travels.MAX_WRITE_REQUESTSwas not in the scope.ids_forwas, and resolving ids without bounding them is what makes a bulk delete report a success it did not perform, given a write budget of ten to twenty requests per minute. The cap is the smaller half of that budget, and it is spent by the whole pass rather than by its writes alone: counting the writes only, it let a filter-driven update onPylonIssuespend sixty requests where it was allowed twenty — the 429 outliving its retries, the write stopping mid-selection, which is exactly what the cap is there to refuse. The message tells the operator to narrow the selection, and names the reach that applies to theirs.id_valuesdeduplicates.id innames the records to act on, and the same one named twice is one record: onlyresolve_ids_by_listdeduplicated, so a named selection carrying a duplicate patched one record twice and, on a delete, answered 404 the second time — 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.no_search?moved fromCursorCollectiontoBaseCollection.ids_forneeds it on every collection, not only on the ones read through a cursor. Same body, one level up; no behaviour change.Verification
647 examples, 0 failures— coverage 1305/1305 lines (100%, threshold 90), branch 95.0%. RuboCop clean over the 55 files of the package.Client specs cover: the endpoint each of the eleven write methods reaches, the
dataenvelope unwrapped, the two malformed shapes raising, anAPIErrorcarrying the status and the request id, an id escaped so it cannot alter the path, and a rate-limited create retried where a gateway error is not.Collection specs cover: the writable columns posted and the record serialized back; read-only columns, unknown keys and empty values dropped; custom fields written through
valueandvalueswith the synced one left out, and nocustom_fieldskey when none was set; a field of the wrong direction refused when changed and dropped when it holds the stored value or nothing; an id filter patched without a read first, aninfilter reaching every record, a patch of only read-only keys sending nothing, and a filter carrying more than an id resolved through a read; a delete over a filter and a delete matching nothing; the cap refused before spending a request, refused once the resolution reveals the count, and refused on the collection that cannot filter an id; the reach dividing onPylonIssuewhen the patch has every record read before it is written and holding when it owes them nothing, the resolution of a named selection refused before the first of its reads, and the full reach kept both where the resolution costs one request whatever the count and on the collection whose read does not fan out; the three verbs Pylon has no endpoint for, refused whatever the selection reaches and before a request is spent on resolving it; a 4xx create and a 4xx patch surfaced with the reason Pylon gave where a 5xx stays what it was; an id named twice written once and counted once against the cap; and the per-collection specifics — an account type written asaccount_type, an account refusedis_disabledon a create, a contact patched, a team created and the members of the record it answers with flattened, a team's members replaced, a user's status patched.The eight collection specs also re-assert their schemas against the new flags, including the columns that stay read-only and why. The introspector specs cover the three states of Pylon's flag: read-only when it says so, writable when it says so, and read-only with a warning when it says nothing.
🤖 Generated with Claude Code
Note
Add CRUD write operations to
datasource-pyloncollectionsCREATE_ONLYandUPDATE_ONLY.UnsupportedWriteError,PartialWriteError, andWriteRejectedErrorto report write rejections and partial failures.is_read_onlyflag and use thevalueskey for multi-value types.Macroscope summarized 44cd3c2.