Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ These are mistakes a competent Python developer would make if they hadn't read t
- **Don't catch `httpx` errors directly.** The transport already translates them into `TFEError` subclasses. Catching `httpx.HTTPError` in a resource means the typed error never propagates.
- **Always send the bearer token, even to absolute URLs returned by the API.** Endpoints like `hosted_state_download_url`, `hosted_state_upload_url`, plan `json-output`, and apply `errored-state` redirect to `archivist.terraform.io` — which is HashiCorp infrastructure that *requires* the bearer. go-tfe does the same (see `state_version.go::Download` + `tfe.go::NewRequest`). Stripping the bearer breaks downstream consumers (notably the Ansible collection's statefile + dynamic-inventory flows). `HTTPTransport.request` accepts `include_auth=False` only as an opt-out for the hypothetical case of calling a genuinely non-HashiCorp host; do not use it for Archivist URLs.
- **Don't write a custom page loop.** `self._list(path, params=...)` handles pagination + non-paginated endpoints transparently. Rolling your own loop will diverge from the rest of the codebase.
- **Disable pagination for endpoints that ignore page params.** A few endpoints return the *whole* collection on every request and ignore `page[number]`/`page[size]` (workspace `/vars` and `/all-vars`). Call `self._list(path, params=..., paginated=False)` for those — otherwise they infinite-loop once the collection reaches the page size (the [#181](https://github.com/hashicorp/python-tfe/issues/181) bug). See [ITERATORS.md](docs/ITERATORS.md).
- **Don't reuse generators.** Iterators returned by `list_*` are single-use. If you need to traverse twice, `materialized = list(client.foo.list_bars(...))` first.
- **Don't add features beyond what was asked.** This codebase is approaching v1.0.0. Adding "while I'm here" refactors or speculative abstractions slows reviews and risks breaking the Ansible collection.
- **Don't assume every successful response is `{"data": ...}`.** Check the docs/go-tfe/spec for each endpoint: some return a JSON:API envelope, some return a bare resource object, `204 No Content`, `null`, raw bytes, or a redirect to a blob URL. Add tests for non-standard shapes.
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Unreleased

## Bug Fixes

### Pagination
* Fixed `list_*` infinite-looping for API call which are non paginated, so they are now fetched with a single request. The generic list helper also treats any response without `meta.pagination` as a single complete page, preventing the same loop on other non-paginated endpoints. [#181](https://github.com/hashicorp/python-tfe/issues/181)


# Released
# v1.0.0
Expand Down
21 changes: 19 additions & 2 deletions docs/ITERATORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,23 @@ def list(
yield self._workspace_from(item)
```

That's it. `self._list()` lives in `_base.py` and handles `page[number]` / `page[size]` and follow-through automatically. It is also robust to endpoints that **don't paginate** — if the response has no pagination metadata and the returned data is smaller than the requested page size, the helper just breaks after one round-trip. So you do not need a different code path for relationship reads like `GET /workspaces/{id}/tag-bindings` (single response) versus list endpoints like `GET /organizations/{org}/workspaces` (paginated). The same `for item in self._list(path): yield ...` works for both.
That's it. `self._list()` lives in `_base.py` and handles `page[number]` / `page[size]` and follow-through automatically. When a response carries no `meta.pagination` block, `_list` treats it as a single complete page and stops after one round-trip — so the same `for item in self._list(path): yield ...` works for ordinary paginated endpoints (`GET /organizations/{org}/workspaces`) and for single-response relationship reads (`GET /workspaces/{id}/tag-bindings`) alike.

### Endpoints that ignore pagination entirely — pass `paginated=False`

A few HCP Terraform endpoints return the **whole** collection on every request and ignore `page[number]` / `page[size]`. The workspace **`/vars`** and **`/all-vars`** endpoints are the known ones. For these you must opt out of the page loop explicitly:

```python
# variable.py — /vars is not paginated
for item in self._list(path, params=params, paginated=False):
yield self._variable_from(item)
```

With `paginated=False`, `_list` issues exactly one request and yields every row.

Why this matters: skipping the flag on such an endpoint with **≥ `page_size` rows (default 100)** used to spin forever — the helper saw a "full" page, asked for page 2, got the *same* full set back (the endpoint ignored the page param), and re-yielded it, indefinitely. That was the root cause of [#181](https://github.com/hashicorp/python-tfe/issues/181). The generic "no `meta.pagination` ⇒ single page" rule now catches this as a safety net, but **still set `paginated=False`** on a known non-paginated endpoint: it documents intent and avoids sending meaningless page params.

> Rule of thumb: if the endpoint returns no `meta.pagination` and ignores `page[size]` (check go-tfe or the API docs — it returns the full collection in one shot), pass `paginated=False`.


### Note on lazy validation
Expand Down Expand Up @@ -131,7 +147,7 @@ def list_versions(

`registry_module.list_versions` is the only method in the codebase that does this. Add a docstring note explaining the reason if you find yourself reaching for this pattern, so future readers don't mistake it for something to copy.

Do **not** reach for `iter(list)` just because the endpoint is non-paginated. Use `self._list()` for those — that's the convention.
Do **not** reach for `iter(list)` just because the endpoint is non-paginated. Use `self._list()` for those — that's the convention (with `paginated=False` if the endpoint ignores `page[size]` and returns the full set, as described above).

### Shape that does **not** match the convention (don't do this)

Expand Down Expand Up @@ -186,6 +202,7 @@ Don't assert `isinstance(result, list)` against the raw return — that asserts
- [ ] Every `list*` method returns `Iterator[X]`, not `list[X]` or `Iterable[X]`
- [ ] `Iterator` is imported from `collections.abc`, not `typing`
- [ ] The body uses the canonical `for item in self._list(path, params=params): yield ...` pattern — including for non-paginated single-shot endpoints
- [ ] Endpoints that ignore `page[size]` and return the whole collection (e.g. workspace `/vars`, `/all-vars`) pass `paginated=False` to `self._list(...)` — otherwise they infinite-loop at ≥ 100 rows (see [#181](https://github.com/hashicorp/python-tfe/issues/181))
- [ ] Hand-rolled `iter(materialized_list)` only appears if the method has a try/except fallback to a different endpoint (extremely rare — has a docstring note explaining why)
- [ ] If a class defines `def list(...)`, later annotations in that class avoid bare `list[...]` so mypy does not resolve `list` to the method
- [ ] Examples that call the method use `list(client.foo.list_bars(...))` (or stream with a `for` loop) — never assume list semantics on the bare return
Expand Down
2 changes: 1 addition & 1 deletion docs/MODELS.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class Run(BaseModel):
Rules:

- **Every multi-word JSON:API attribute** gets an alias. Don't try to invent a snake_case-to-hyphen mapper — be explicit per field.
- **Page params** use the JSON:API square-bracket form: `Field(None, alias="page[number]")`, `Field(None, alias="page[size]")`.
- **Page params** use the JSON:API square-bracket form: `Field(None, alias="page[number]")`, `Field(None, alias="page[size]")`. Note that a few endpoints (workspace `/vars`, `/all-vars`) are not paginated and ignore these — their resource methods call `self._list(..., paginated=False)`, so a `page_size` field on those options models would be a no-op. See [ITERATORS.md](ITERATORS.md).
- **Filter params** use the same convention: `Field(None, alias="filter[workspace][name]")`.
- **`include`** is a comma-separated string on the wire but exposed as `list[SomeEnum] | None` in Python; the resource layer dumps options with `mode="json"` and joins the resulting values (`",".join(params["include"])`). See the `policy_set.read_with_options` pattern.

Expand Down
8 changes: 6 additions & 2 deletions docs/pagination.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,12 @@ for run in client.runs.list("ws-abc123", options):
- `list` / `list_*` methods are lazy. If an invalid-id check is inside a
generator method, the exception is raised when you iterate, not when you
create the iterator.
- Some relationship endpoints are not paginated by the server, but pyTFE still
exposes them as iterators for a consistent public API.
- Some endpoints are not paginated by the server, but pyTFE still exposes them
as iterators for a consistent public API. A few — notably
`variables.list()` / `variables.list_all()` (the workspace `/vars` and
`/all-vars` endpoints) — return the entire collection in a single request and
ignore `page_size`. You still iterate them the same way; the SDK just fetches
everything in one round-trip.
- A small number of older methods intentionally return concrete lists for
backward compatibility. Prefer the iterator rule for new code, and check the
method's return type if you are unsure.
Expand Down
38 changes: 32 additions & 6 deletions src/pytfe/resources/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from typing import Any

from .._http import HTTPTransport
from .._logging import logger


def _to_int(value: Any) -> int | None:
Expand All @@ -26,9 +27,25 @@ def __init__(self, t: HTTPTransport) -> None:
self.t = t

def _list(
self, path: str, *, params: dict | None = None
self, path: str, *, params: dict | None = None, paginated: bool = True
) -> Iterator[dict[str, Any]]:
base_params = dict(params or {})

# Some TFE endpoints are not paginated: they return the full collection
# in a single response, ignore page[number]/page[size], and emit no
# meta.pagination block (e.g. workspace /vars and /all-vars). Callers
# opt those out so we issue exactly one request and never loop trying to
# fetch a "next" page that re-returns the same set
if not paginated:
r = self.t.request("GET", path, params=base_params)
json_response = r.json()
if not isinstance(json_response, dict):
json_response = {}
data = json_response.get("data", [])
if isinstance(data, list):
yield from data
return

page = int(base_params.get("page[number]", 1))
while True:
p = dict(base_params)
Expand Down Expand Up @@ -81,8 +98,17 @@ def _list(
# Metadata present and indicates no next page.
break

# Fallback for endpoints that do not return pagination metadata.
page_size = int(p["page[size]"])
if len(data) < page_size:
break
page += 1
# No pagination metadata. Genuine TFE list endpoints always include
# meta.pagination, so a response without it is a non-paginated
# collection returned in full. Treat it as a single complete page
# and stop. Re-requesting would loop forever when the endpoint
# ignores page[number]/page[size] and re-returns the same full set
if len(data) >= int(p["page[size]"]):
logger.debug(
"List endpoint %s returned a full page (%d rows) without "
"pagination metadata; treating it as a single, "
"non-paginated response.",
path,
len(data),
)
break
3 changes: 2 additions & 1 deletion src/pytfe/resources/no_code_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,12 @@ def read_variables(
if not valid_string(version):
raise InvalidVersionError()

# module-variables is not paginated; fetch the full set in one request.
path = (
f"/api/v2/no-code-modules/{no_code_module_id}"
f"/versions/{version}/module-variables"
)
for item in self._list(path):
for item in self._list(path, paginated=False):
attrs = item.get("attributes") or {}
yield RegistryModuleVariable.model_validate(
{
Expand Down
3 changes: 2 additions & 1 deletion src/pytfe/resources/projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,9 @@ def list_effective_tag_bindings(
if not valid_string_id(project_id):
raise ValueError("Project ID is required and must be valid")

# effective-tag-bindings is not paginated.
path = f"/api/v2/projects/{project_id}/effective-tag-bindings"
for item in self._list(path):
for item in self._list(path, paginated=False):
attr = item.get("attributes", {}) or {}
links = item.get("links", {}) or {}
yield EffectiveTagBinding(
Expand Down
3 changes: 2 additions & 1 deletion src/pytfe/resources/run_event.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ def list(
params: dict[str, Any] = {}
if options and options.include:
params["include"] = ",".join(options.include)
# The run-events endpoint is not paginated; fetch the full set in one request.
path = f"/api/v2/runs/{run_id}/run-events"
for item in self._list(path, params=params):
for item in self._list(path, params=params, paginated=False):
attrs = item.get("attributes", {})
attrs["id"] = item.get("id")
yield RunEvent.model_validate(attrs)
Expand Down
4 changes: 2 additions & 2 deletions src/pytfe/resources/task_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ def list(
raise InvalidRunIDError()

path = f"/api/v2/runs/{run_id}/task-stages"
kwargs = {"params": options.model_dump(by_alias=True)} if options else {}
params = options.model_dump(by_alias=True) if options else None

for item in self._list(path, **kwargs):
for item in self._list(path, params=params):
yield self._parse_task_stage(item)

# Override
Expand Down
8 changes: 6 additions & 2 deletions src/pytfe/resources/variable.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@ def list(
if not valid_string_id(workspace_id):
raise ValueError(ERR_INVALID_WORKSPACE_ID)

# The /vars endpoint is not paginated: it returns every variable in a
# single response and ignores page[number]/page[size]. Opt out of the
# pagination loop so we issue exactly one request (python-tfe#181).
path = f"/api/v2/workspaces/{workspace_id}/vars"
params: dict[str, Any] = {}
if options:
# Add any options if needed in the future
pass

for item in self._list(path, params=params):
for item in self._list(path, params=params, paginated=False):
attr = item.get("attributes", {}) or {}
var_id = item.get("id", "")
variable_data = dict(attr)
Expand All @@ -50,13 +53,14 @@ def list_all(
if not valid_string_id(workspace_id):
raise ValueError(ERR_INVALID_WORKSPACE_ID)

# Like /vars, the /all-vars endpoint is not paginated; request once.
path = f"/api/v2/workspaces/{workspace_id}/all-vars"
params: dict[str, Any] = {}
if options:
# Add any options if needed in the future
pass

for item in self._list(path, params=params):
for item in self._list(path, params=params, paginated=False):
attr = item.get("attributes", {}) or {}
var_id = item.get("id", "")
variable_data = dict(attr)
Expand Down
6 changes: 4 additions & 2 deletions src/pytfe/resources/workspaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -744,8 +744,9 @@ def list_tag_bindings(self, workspace_id: str) -> Iterator[TagBinding]:
if not valid_string_id(workspace_id):
raise InvalidWorkspaceIDError()

# tag-bindings is not paginated.
path = f"/api/v2/workspaces/{workspace_id}/tag-bindings"
for item in self._list(path):
for item in self._list(path, paginated=False):
attr = item.get("attributes", {}) or {}
yield TagBinding(
id=item.get("id"),
Expand All @@ -759,8 +760,9 @@ def list_effective_tag_bindings(
if not valid_string_id(workspace_id):
raise InvalidWorkspaceIDError()

# effective-tag-bindings is not paginated.
path = f"/api/v2/workspaces/{workspace_id}/effective-tag-bindings"
for item in self._list(path):
for item in self._list(path, paginated=False):
attr = item.get("attributes", {}) or {}
yield EffectiveTagBinding(
id=item.get("id", ""),
Expand Down
3 changes: 3 additions & 0 deletions tests/units/test_run_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def test_list_run_events_success(self, run_events_service):
mock_list.assert_called_once_with(
"/api/v2/runs/run-123/run-events",
params={"include": "actor"},
paginated=False,
)

# Verify results
Expand Down Expand Up @@ -113,6 +114,7 @@ def test_list_run_events_with_multiple_includes(self, run_events_service):
mock_list.assert_called_once_with(
"/api/v2/runs/run-456/run-events",
params={"include": "actor,comment"},
paginated=False,
)

assert len(results) == 1
Expand Down Expand Up @@ -140,6 +142,7 @@ def test_list_run_events_no_options(self, run_events_service):
mock_list.assert_called_once_with(
"/api/v2/runs/run-789/run-events",
params={},
paginated=False,
)

assert len(results) == 1
Expand Down
4 changes: 3 additions & 1 deletion tests/units/test_task_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,9 @@ def test_list_calls_internal_list(mocker):
assert len(result) == 1
assert isinstance(result[0], TaskStage)

service._list.assert_called_once_with("/api/v2/runs/run-123/task-stages")
service._list.assert_called_once_with(
"/api/v2/runs/run-123/task-stages", params=None
)


# Override method tests
Expand Down
Loading
Loading