Skip to content

UN-3770 [MISC] Follow DRF pagination on all clone list endpoints - #24

Open
chandrasekharan-zipstack wants to merge 4 commits into
mainfrom
feat/paginate-list-helpers
Open

UN-3770 [MISC] Follow DRF pagination on all clone list endpoints#24
chandrasekharan-zipstack wants to merge 4 commits into
mainfrom
feat/paginate-list-helpers

Conversation

@chandrasekharan-zipstack

Copy link
Copy Markdown
Contributor

What & why

Part of the UN-3770 list-pagination workstream. This is step 1 of 4 and is the prerequisite for flipping the OSS list endpoints to unconditional pagination — it must be merged and released before that lands.

Every list_* helper in PlatformClient already unwrapped a paginated envelope:

return result if isinstance(result, list) else result.get("results", [])

…but none of them ever sent ?page or followed next. They read page one and stopped.

That is already a live bug, not a future one. tags/, pipeline/ and api/deployment/ use CustomPagination on the backend today, so cloning an org with more than 50 tags / pipelines / API deployments silently drops the rest — no error, no warning. The stale comment on list_adapters ("no pagination on this endpoint") is the assumption that made this easy to miss.

Once unstract#2187's follow-up makes adapters, connectors, workflows and prompt-studio unconditionally paginated, the same truncation hits those too. Silent partial data loss is a far worse failure mode than a hard break, so the client has to be fixed first.

Approach

PlatformClient._paginate(path, params):

  • Bare list → returned unchanged. No server-version negotiation, no feature flag. The same client works against an old on-prem deployment that doesn't paginate and a new one that does, so this ships and releases entirely independently of the backend work.
  • Envelope → walks next to exhaustion.
  • Refuses to return a short read. Collected rows are checked against the reported count; a mismatch raises PlatformAPIError. This is the durable protection — any future truncation becomes loud instead of silent.
  • Cyclic next raises rather than hanging a migration forever.

_request() is split into a URL-building wrapper over _send(), because DRF's next links are absolute and can't go through org-relative path composition. _request()'s signature is unchanged, so every existing caller is untouched.

All 23 list helpers now route through _paginate. list_lookup_versions deliberately keeps its bespoke unwrap — that endpoint returns {"versions": [...], "next_version_number"}, not a DRF envelope.

Testing

  • tests/clone/test_client.py: 3 new tests — follows next across pages and hits the absolute URL verbatim; raises on a short read; raises on a cyclic next. The two pre-existing tests pinning bare-list and single-page-envelope behaviour still pass unchanged, which is the backward-compatibility guarantee.
  • Full suite: 250 passed.
  • ruff check + ruff format clean.
  • ⚠️ Live validation still pending — hence draft. tags/ already paginates in staging, so list_tags exercises the real envelope → nextcount path end-to-end with no backend change required.

Residual risk

A customer pinned to an older client version still truncates silently after the backend flip; nothing here helps them retroactively. Tracked in the rollout plan, along with the option of instrumenting bare-array responses for one release to measure who is still affected before anything breaks.

🤖 Generated with Claude Code

https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ

Every list_* helper unwrapped a paginated envelope but never sent ?page
and never followed `next` — it read page one and stopped. Endpoints that
already paginate (tags/, pipeline/, api/deployment/) have therefore been
silently truncating at 50 rows, and the same would hit adapters,
connectors and prompt-studio once UN-3770 makes their pagination
unconditional. A clone that copies a subset without erroring is worse
than one that fails.

Add PlatformClient._paginate(), which short-circuits on a bare list so
the client keeps working against deployments where an endpoint is not
paginated, otherwise walks `next` to exhaustion. It refuses to return a
short read: the collected row count is checked against the reported
count, and a cyclic `next` raises instead of looping forever.

_request() is split so the absolute `next` URLs DRF emits can be issued
without going through org-relative path composition. Its signature is
unchanged, so every existing caller is untouched.

All 23 list helpers now route through it. list_lookup_versions keeps its
bespoke unwrap — that endpoint returns {"versions": [...]}, not a DRF
envelope.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
@chandrasekharan-zipstack chandrasekharan-zipstack changed the title [FIX] Follow DRF pagination on all clone list endpoints UN-3770 [MISC] Follow DRF pagination on all clone list endpoints Jul 24, 2026
Adds a paths-scoped GitHub Actions workflow that runs the clone test
suite whenever clone code, its tests, or the dependency set changes.
Gives a fast, dedicated signal for the pagination page-following logic
in client._paginate — a silent-truncation regression there is worse
than a hard failure, so it must stay guarded on every clone change.

The full suite in test.yml still runs on every PR; this narrows the
trigger and the run to tests/clone/ for quicker feedback.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
UN-3770 [CI] Add focused clone test workflow
Comment thread src/unstract/clone/client.py
Comment on lines +33 to +40
- uses: actions/checkout@v4

- uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}

- name: Install uv
uses: astral-sh/setup-uv@v6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security Mutable CI action references

The workflow executes checkout, setup-python, and setup-uv through mutable major-version tags, so an upstream tag change can execute unexpected code in the CI environment. Pin each action to a reviewed full commit SHA to make these dependencies immutable.

How this was verified: The workflow directly references all three executable actions using mutable major-version tags.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/clone-tests.yml
Line: 33-40

Comment:
**Mutable CI action references**

The workflow executes `checkout`, `setup-python`, and `setup-uv` through mutable major-version tags, so an upstream tag change can execute unexpected code in the CI environment. Pin each action to a reviewed full commit SHA to make these dependencies immutable.

**How this was verified:** The workflow directly references all three executable actions using mutable major-version tags.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Comment thread src/unstract/clone/client.py
@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds exhaustive DRF pagination to PlatformClient list helpers and focused CI coverage.

  • Splits URL construction from request sending so absolute next-page links can be followed.
  • Adds count-mismatch and pagination-cycle detection while preserving bare-list compatibility.
  • Routes all applicable list helpers through the shared paginator and adds pagination tests.
  • Adds a path-filtered Python 3.11/3.12 clone-test workflow.

Confidence Score: 4/5

The PR is safe to merge with non-blocking improvements recommended for pagination response validation, pagination-link origin checks, and immutable CI action pinning.

The core pagination path handles bare lists, normal DRF envelopes, short reads, and cycles; the accepted concerns affect malformed later-page responses, authenticated cross-origin page following, and CI dependency hardening.

Files Needing Attention: src/unstract/clone/client.py; .github/workflows/clone-tests.yml

Security Review

The new clone-test workflow uses mutable action tags that should be pinned to full commit SHAs. The paginator also follows absolute server-provided links through an authenticated session without constraining them to the configured platform origin.

Important Files Changed

Filename Overview
src/unstract/clone/client.py Adds shared exhaustive pagination and migrates list helpers, but later-page payloads bypass envelope validation and absolute next links are followed without an origin check.
tests/clone/test_client.py Adds focused coverage for absolute next links, count mismatches, and cyclic pagination.
.github/workflows/clone-tests.yml Adds a focused clone-test matrix consistent with existing CI setup, while retaining mutable action references.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[List helper] --> B[_paginate]
    B --> C[_request initial org-relative URL]
    C --> D{Bare list?}
    D -->|Yes| E[Return rows]
    D -->|No| F{DRF envelope?}
    F -->|No| G[Raise PlatformAPIError]
    F -->|Yes| H[Collect results]
    H --> I{next present?}
    I -->|Yes| J[_send absolute next URL]
    J --> K{URL already seen?}
    K -->|Yes| L[Raise pagination-loop error]
    K -->|No| H
    I -->|No| M{Collected length equals count?}
    M -->|No| N[Raise short-read error]
    M -->|Yes| E
Loading

Fix All in Claude Code

Prompt To Fix All With AI
### Issue 1
src/unstract/clone/client.py:148
**Validate every pagination response**

Only the initial response is validated as a DRF envelope. When a later request returns an empty body, bare list, or object without `results`, the next iteration raises an incidental `AttributeError` or `TypeError` instead of the actionable `PlatformAPIError` used for the same malformed first-page payload.

```suggestion
            result = self._send("GET", next_url, path)
            if not isinstance(result, dict) or "results" not in result:
                raise PlatformAPIError(
                    f"GET {path} returned an unrecognised list payload"
                )
```

### Issue 2
.github/workflows/clone-tests.yml:33-40
**Mutable CI action references**

The workflow executes `checkout`, `setup-python`, and `setup-uv` through mutable major-version tags, so an upstream tag change can execute unexpected code in the CI environment. Pin each action to a reviewed full commit SHA to make these dependencies immutable.

**How this was verified:** The workflow directly references all three executable actions using mutable major-version tags.

### Issue 3
src/unstract/clone/client.py:148
**Constrain pagination link origins**

The paginator sends the session's bearer authorization header to each absolute `next` URL without checking that its origin matches the configured platform endpoint. Validate pagination links before following them so a compromised or misconfigured API response cannot forward the platform key to another host.

**How this was verified:** The server-provided `next` value is passed directly to `_send`, which uses the shared session containing the bearer authorization header.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "Merge pull request #25 from Zipstack/ci/..." | Re-trigger Greptile

Addresses Greptile review on #24:
- Validate the DRF envelope of every page, not just the first — a later
  page that is a bare list / non-envelope now raises PlatformAPIError
  instead of an incidental AttributeError on the next loop turn.
- Reject a `next` link whose origin differs from the configured platform
  endpoint before following it, so a compromised/misconfigured response
  cannot forward the bearer key to another host.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_018HXYFVCZGi7YN9qjDGyJCQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant