feat(datasource-pylon): PylonIssue read-only collection with cursor pagination - #347
Conversation
5 new issues
|
|
|
||
| # POST /issues/search accepts an empty body and then returns the most recent | ||
| # issues, ordered by `created_at` descending. | ||
| def search_issues(limit:, cursor: nil, filter: nil, search_text: nil) |
| # `define_relations` as hooks; ordering between them, custom-field | ||
| # registration, and the search/count flags is owned here so collisions | ||
| # are always evaluated against the final native schema. | ||
| def initialize(datasource, name, custom_fields: [], searchable: false, countable: false, native_driver: nil) |
| raise unless e.status == 404 | ||
|
|
||
| nil | ||
| end |
| cursor = page.next_cursor | ||
| end | ||
|
|
||
| records[offset.to_i, limit.to_i] || [] |
| remaining.clamp(1, Client::MAX_SEARCH_LIMIT) | ||
| end | ||
|
|
||
| def log_truncation(offset:, limit:, pages:, collected:) |
| record = NATIVE_FIELDS.to_h { |field| [field, attrs[field]] } | ||
| PARTY_FIELDS.each { |column, source| record[column] = nested_id(attrs[source]) } | ||
| record | ||
| end |
There was a problem hiding this comment.
🟠 High issue/serializer.rb:18
serialize drops every custom_fields entry: it only copies NATIVE_FIELDS and flattened party IDs into the returned record, so any field registered via Issue.new(..., custom_fields: [...]) comes back as nil in list and detail responses even though the collection schema advertises those columns. The method never reads attrs['custom_fields'], so the custom-field support added by BaseCollection is nonfunctional. Consider merging attrs.fetch('custom_fields', {}) into record before returning.
record = NATIVE_FIELDS.to_h { |field| [field, attrs[field]] }
PARTY_FIELDS.each { |column, source| record[column] = nested_id(attrs[source]) }
+ record.merge!(attrs.fetch('custom_fields', {}))
record🚀 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/issue/serializer.rb around lines 18-21:
`serialize` drops every `custom_fields` entry: it only copies `NATIVE_FIELDS` and flattened party IDs into the returned record, so any field registered via `Issue.new(..., custom_fields: [...])` comes back as `nil` in list and detail responses even though the collection schema advertises those columns. The method never reads `attrs['custom_fields']`, so the custom-field support added by `BaseCollection` is nonfunctional. Consider merging `attrs.fetch('custom_fields', {})` into `record` before returning.
322b9b3 to
a44064a
Compare
|
|
||
| def fetch_records(filter) | ||
| ids = extract_id_lookup(filter&.condition_tree) | ||
| return fetch_by_ids(ids) if ids |
There was a problem hiding this comment.
🟠 High collections/issue.rb:19
fetch_records returns all issues matching the id IN lookup without applying filter.page, so an id IN query with a limit or offset returns every matching record instead of the requested page. For example, ten IDs with limit: 2 returns all ten issues, and repeated calls produce overlapping windows. fetch_by_ids(ids) is returned directly; consider applying the page offset and limit to its result before returning.
🚀 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/issue.rb around line 19:
`fetch_records` returns all issues matching the `id IN` lookup without applying `filter.page`, so an `id IN` query with a limit or offset returns every matching record instead of the requested page. For example, ten IDs with `limit: 2` returns all ten issues, and repeated calls produce overlapping windows. `fetch_by_ids(ids)` is returned directly; consider applying the page offset and limit to its result before returning.
Registers the first Pylon collection: issues in list + record-detail mode,
backed by POST /issues/search and GET /issues/{id}.
Forest asks for an offset/limit window while Pylon only hands out the next
page of a cursor, so CursorWalker walks pages until the window is covered
then slices. The walk is capped (20 pages / 5000 records, with a truncation
warning) because /issues/search allows 20 requests per minute and an
uncapped deep-offset walk would spend an agent's whole budget on one list
view. It also stops defensively on an empty page or a cursor that does not
advance.
The schema follows the live API rather than the ticket: Pylon has no
priority field, first_response_time/resolution_time are RFC3339 timestamps
and not durations, and /issues/search exposes no sort parameter, so no
column is sortable and translate_sort is not ported from Zendesk. Nested
account/requester/assignee/team objects are flattened into id columns until
the related collections exist.
Search and Count default to disabled in BaseCollection, the inverse of the
Zendesk template, since both land with the condition-tree translator. Until
then a condition the collection cannot honour is dropped with a warning
naming what was discarded, so an unfiltered result set does not read as a
filtered one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| page = yield(batch_size(needed - records.size), cursor) | ||
| records.concat(page.records) | ||
| pages += 1 | ||
|
|
||
| break if stop?(page, cursor) || records.size >= needed | ||
|
|
||
| if capped?(pages, records.size) | ||
| log_truncation(offset: offset, limit: limit, pages: pages, collected: records.size) | ||
| break | ||
| end |
There was a problem hiding this comment.
🟡 Medium pagination/cursor_walker.rb:28
CursorWalker#walk can accumulate more than @max_records records, so the advertised cap is not enforced. For example, if 4,500 records have been collected and the next page returns 1,000, line 29 concatenates all of them (5,500 total). When that page also satisfies the requested window or is the last page, line 32 breaks before capped? runs, so the overrun is never logged and records[offset, limit] can return rows past the configured cap. The check at line 34 only runs after the unbounded concatenation, so the budget is enforced too late. Consider clamping each batch by the remaining record budget before fetching, and/or truncating records immediately after concatenation.
page = yield(batch_size(needed - records.size), cursor)
records.concat(page.records)
pages += 1
+
+ if records.size > @max_records
+ records.replace(records.take(@max_records))
+ log_truncation(offset: offset, limit: limit, pages: pages, collected: records.size)
+ break
+ end
+
break if stop?(page, cursor) || records.size >= needed
-
if capped?(pages, records.size)
log_truncation(offset: offset, limit: limit, pages: pages, collected: records.size)
break
end🚀 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/pagination/cursor_walker.rb around lines 28-37:
`CursorWalker#walk` can accumulate more than `@max_records` records, so the advertised cap is not enforced. For example, if 4,500 records have been collected and the next page returns 1,000, line 29 concatenates all of them (5,500 total). When that page also satisfies the requested window or is the last page, line 32 breaks before `capped?` runs, so the overrun is never logged and `records[offset, limit]` can return rows past the configured cap. The check at line 34 only runs after the unbounded concatenation, so the budget is enforced too late. Consider clamping each batch by the remaining record budget before fetching, and/or truncating `records` immediately after concatenation.
|
|
||
| spec.add_dependency 'faraday', '~> 2.0' | ||
| spec.add_dependency 'faraday-retry', '~> 2.0' | ||
| spec.add_dependency 'zeitwerk', '~> 2.3' |
There was a problem hiding this comment.
🟠 High forest_admin_datasource_pylon/forest_admin_datasource_pylon.gemspec:34
The gemspec omits forest_admin_datasource_toolkit from its runtime dependencies, so installing the published gem in an application that does not already have the toolkit fails with LoadError when require 'forest_admin_datasource_pylon' runs, because the library unconditionally executes require 'forest_admin_datasource_toolkit'. The toolkit currently appears only in development/test Gemfiles, so Bundler does not install it for end users. Add spec.add_dependency 'forest_admin_datasource_toolkit' (with the appropriate version constraint) to the gemspec.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_pylon/forest_admin_datasource_pylon.gemspec around line 34:
The gemspec omits `forest_admin_datasource_toolkit` from its runtime dependencies, so installing the published gem in an application that does not already have the toolkit fails with `LoadError` when `require 'forest_admin_datasource_pylon'` runs, because the library unconditionally executes `require 'forest_admin_datasource_toolkit'`. The toolkit currently appears only in development/test Gemfiles, so Bundler does not install it for end users. Add `spec.add_dependency 'forest_admin_datasource_toolkit'` (with the appropriate version constraint) to the gemspec.
a44064a to
9c1bb8b
Compare
Story 2 of the Pylon datasource — EXT-6. Stacked on #341 (Story 1), so it targets
feature/ext-5-story-1-foundation-gem-config-resilient-clientand notmain— review only the last commit, or wait for #341 to merge and this base will retarget tofeat/datasource-pylon(EXT-4).What this adds
The first Pylon collection:
PylonIssuein list + record-detail mode, backed byPOST /issues/searchandGET /issues/{id}.BaseCollection— the Zendesk-style template: sharedSTRING_OPS/NUMBER_OPS/DATE_OPSoperator sets,define_schema/define_relationshooks, custom-field registration with collision reporting, and the primary-key short-circuit.Collections::Issue— schema, serializer andlist. Split intoSchemaDefinitionandSerializermixins so the field list stays readable next to the flattening logic.Pagination::CursorWalker— bridges Forest's offset/limit window onto Pylon's cursor.Datasource— registersPylonIssue.Client#search_issues/#fetch_issue— aSearchPagestruct that normalises the three ways Pylon spells "no next page" (absentpaginationblock,has_next_page: false, empty cursor) down to anilcursor.Cursor pagination, and why the walk is capped
Forest asks for an offset/limit window; Pylon only hands out the next page of a cursor.
CursorWalkerwalks pages until the window is covered, then slices — so a deep offset costs one request per page./issues/searchallows 20 requests per minute, so an uncapped deep-offset walk would spend an agent's entire budget on a single list view. The walk therefore stops at 20 pages / 5000 records and logs a warning naming the offset, limit and how much was collected, rather than truncating silently.It also stops defensively on an empty page or a cursor that does not advance. Pylon does neither today, but a loop driven by a remote value should terminate on its own terms, not only on the caps.
Schema follows the live API, not the ticket
Three deliberate divergences from EXT-6's field list, all verified against the live API:
priority— Pylon has no such field on issues.first_response_time/resolution_timeareDate, not durations — they are RFC3339 timestamps, so the ticket'sfirst_response_seconds/resolution_secondsdo not exist. The two genuine duration fields (time_in_status_seconds,business_hours_time_in_status_seconds) are per-status maps, mapped asJson./issues/searchexposes no sort parameter at all; results always come backcreated_atdescending. Advertising a sortable column would let the UI ask for an order the API cannot honour, sotranslate_sortis not ported from Zendesk.Nested
account/requester/assignee/teamobjects are flattened to*_idstring columns until the related collections exist. Every column is read-only in this story.Unimplemented filtering is loud, not silent
enable_searchandenable_countdefault to off inBaseCollection— the inverse of the Zendesk template — because both land with the condition-tree translator in a later story. Until then, a condition tree or search term the collection cannot honour is dropped with a warning naming exactly what was discarded, so an unfiltered result set never reads as a filtered one.The one filter that is honoured is the primary-key lookup:
id equal/id inshort-circuits toGET /issues/{id}, since/issues/searchhas noidoperator. A 404 there yields no record rather than an error — a deleted issue, or one outside the token's scope, should read as "no record", not as a failed page.Monorepo wiring
One
.rubocop.ymlexclude (Metrics/AbcSizeonbase_collection.rb)..releaserc.jsstays untouched for the same reason as in #341 — the release wiring is owned by EXT-13 (Story 9), which carries the exact 3-spot patch as a comment.Test plan
BUNDLE_GEMFILE=Gemfile-test bundle exec rspec→ 119 examples, 0 failures, coverage 100% (303/303 LOC, threshold 90)bundle exec rubocopover the whole repo → 798 files, no offenses🤖 Generated with Claude Code
Note
Add
forest_admin_datasource_pylongem with read-only PylonIssue collection and cursor paginationClientwraps Faraday with Bearer auth, retry logic (including 429 with Retry-After), and maps HTTP errors to typedAPIError.CursorWalkerbridges Forest's offset/limit pagination to Pylon's cursor-based API, with protective caps and truncation warnings.PylonIssueschema is fixed and read-only; filter operators are limited to equality/in onid; condition tree filters and free-text search are accepted but ignored with a warning.Macroscope summarized 9c1bb8b.