Refactor relationship model and fixes (#180)

Author: iam404

AGENTS.md

@@ -13,7 +13,8 @@ src/pytfe/
   client.py            # TFEClient — composition root, wires every resource
   config.py            # TFEConfig — auth, timeout, retry, proxy settings
   _http.py             # HTTPTransport — request, retry, redirects, auth
-  _jsonapi.py          # JSON:API envelope helpers
+  _jsonapi.py          # JSON:API helpers: headers, error payloads, and the
+                       #   shared relationship/included parser (parse_relationships)
   _logging.py.         # Logging primitives for the pytfe SDK
   errors.py            # Typed exception hierarchy (TFEError + ~80 subclasses)
   utils.py             # Validation + small helpers
@@ -77,6 +78,7 @@ These are mistakes a competent Python developer would make if they hadn't read t
 - **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.
+- **Don't hand-roll relationship parsing, and don't let response models drop unknown fields.** Response models (the ones you parse from API payloads) set `extra="allow"` so new server fields survive in `model_extra` instead of being silently dropped. Parse the `relationships` block with the shared `parse_relationships` helper from `pytfe._jsonapi` (a declarative `{wire_relation: Model}` map + optional `included` hydration), not a bespoke if-ladder. `resources/workspaces.py` and `resources/run.py` are the reference parsers; details in [MODELS.md](docs/MODELS.md) and [RESOURCE.md](docs/RESOURCE.md).
 - **Don't use bare `list[...]` annotations inside a resource class after defining `def list(...)`.** In class scope, mypy can resolve `list` to the method instead of the builtin. Use `builtins.list[...]`, `Sequence[...]`, or another unshadowed type.
 - **Don't `print()` or use ad-hoc `logging.getLogger(__name__)` calls in library code.** The SDK has a structured logging framework — use `pytfe._logging.transport_logger` for HTTP traffic, or `pytfe._logging.logger` (the `pytfe` root) for higher-level events. Everything from that namespace is silent by default (NullHandler) and respects the user's `setup_logging()` or stdlib configuration. See [LOGGING.md](docs/LOGGING.md) for redaction rules — bearer tokens and `token`/`secret`/`password` keys are auto-redacted by `RoundTrip`, but only inside that formatter. Never `log.info(token)` directly.
 

docs/MODELS.md

@@ -26,6 +26,7 @@ New or touched `BaseModel` classes should set `model_config = ConfigDict(...)` u
 | `populate_by_name=True` | **Always.** Lets callers pass either the field name (`created_at=...`) or the alias (`{"created-at": ...}`) when constructing. |
 | `validate_by_name=True` | Use on models that are parsed *from* API responses **or** constructed by callers via field names. Pair with `populate_by_name=True`. |
 | `extra="forbid"` | Use on `*CreateOptions` / `*UpdateOptions` / option models where you want a typo (`workspce_id=...`) to fail loudly instead of being silently dropped. Don't put it on response models — the API can add fields and we don't want that to break parsing. |
+| `extra="allow"` | Standard for **response models** parsed from API payloads. The default (`extra="ignore"`) silently drops any wire attribute without a declared field, so a new server field becomes a data-loss bug. `extra="allow"` retains undeclared fields in `model_extra` under their wire names (e.g. `model_extra["future-field"]`). Note: extra keys are *not* dot-accessible as snake_case and have no type — add an explicit aliased field for anything users should access ergonomically. `Workspace` is the reference implementation; relationship parsing for these models goes through `pytfe._jsonapi.parse_relationships` (see the Relationships section). |
 | `arbitrary_types_allowed=True` | Only when you genuinely have a non-Pydantic type in a field (rare). |
 
 The standard line you'll write 90% of the time:
@@ -177,12 +178,34 @@ class TaskStage(BaseModel):
     task_results: list[TaskResult] | None = Field(None, alias="task-results")
 ```
 
-Use `model_construct` (not `model_validate`) in the resource for these stubs — it skips validation, which is correct because you only have `{id, type}`:
+The resource layer should populate these via the shared `parse_relationships` helper (see below) rather than a hand-rolled if-ladder. For a true one-off, `Model.model_construct(id=...)` (not `model_validate`) is correct — it skips validation, which is right because you only have `{id, type}`.
+
+### Parsing relationships — `pytfe._jsonapi.parse_relationships`
+
+Don't hand-roll the `relationships.get("x", {}).get("data")` if-ladder per resource. The canonical parser lives in `src/pytfe/_jsonapi.py` and is driven by a declarative map of `{wire_relation: Model}` (or `{wire_relation: (python_attr, Model)}` when the attribute name diverges from `wire.replace("-", "_")`):
 
 ```python
-attributes["run"] = Run.model_construct(id=run_data["id"])
+from .._jsonapi import parse_relationships
+
+_WIDGET_REL_MAP = {
+    "organization": Organization,                 # attr derived: "organization"
+    "current-run": Run,                            # attr derived: "current_run"
+    "vars": ("variables", Variable),               # wire name diverges from attr
+}
+
+def _widget_from(d, included=None):
+    attrs = dict(d.get("attributes") or {})
+    attrs["id"] = d.get("id")
+    attrs.update(parse_relationships(d.get("relationships"), _WIDGET_REL_MAP, included=included))
+    return Widget.model_validate(attrs)
 ```
 
+`parse_relationships` handles single vs list `data`, skips null/absent and unmapped relations (so they fall back to model defaults / `extra="allow"`), and — when the caller passes the response's top-level `included` array — hydrates the **full** related object instead of an id-only stub (so `?include=current_run` returns a populated `Run`, not just `{id}`). Thread `included` from read paths: `payload = r.json(); _widget_from(payload["data"], payload.get("included"))`.
+
+Keep genuinely polymorphic relations (e.g. workspace `locked-by`, `data-retention-policy-choice`) and relations whose `data` carries inline attributes (e.g. workspace `outputs`) as explicit special cases — they don't fit the simple map. `Workspace` (`resources/workspaces.py`) and `Run` (`resources/run.py`) are the reference implementations.
+
+> One gotcha: a parser written as a **method** on a service class that also defines `def list(...)` cannot annotate `included: list[...] | None` — in class scope `list` resolves to the method. Make the parser a module-level function (preferred, matches `_ws_from`/`_run_from`), or use `builtins.list[...]`.
+
 ### Option 2 — Flat `*_id` field
 
 When the relationship is "owned" by this resource and just one id matters, expose it as a flat `*_id` field (with hyphen alias if needed). Less plumbing, fine when you don't need the related model object:
@@ -275,6 +298,7 @@ The CI check in `tests/units/test_model_conventions.py` enforces this for every
 - [ ] `from __future__ import annotations` at the top
 - [ ] New or touched classes use `model_config = ConfigDict(populate_by_name=True, validate_by_name=True)` unless preserving a local legacy pattern
 - [ ] Hyphenated JSON:API attribute names → `Field(alias="...")`
+- [ ] **Response models** (parsed from API payloads) add `extra="allow"` to their `ConfigDict` for forward compatibility; relationships are parsed via `parse_relationships`, not a hand-rolled if-ladder
 - [ ] Response model fields default to `T | None = Field(None, alias="...")`
 - [ ] `*CreateOptions` uses `Field(...)` for required fields, `T | None = None` for optional
 - [ ] `*UpdateOptions` is fully optional

docs/RESOURCE.md

@@ -184,26 +184,35 @@ def _widget_from(self, data: dict[str, Any]) -> Widget:
     return Widget.model_validate(attrs)
 ```
 
-If the model has relationships, pull them from `data["relationships"]` and either:
-
-1. **Embed an id-stub** using `Model.model_construct(id=...)` — use this when the model defines the relation as `OtherModel | None`. `model_construct` skips validation, which is correct for partial `{id, type}` data:
+If the model has relationships, parse them with the shared `parse_relationships` helper from `pytfe._jsonapi` — don't hand-roll a per-relation if-ladder. Pass a declarative `{wire_relation: Model}` map (or `{wire: (python_attr, Model)}` when the attribute name diverges from `wire.replace("-", "_")`), and thread the response's top-level `included` so `?include=` requests hydrate full nested objects instead of id-only stubs:
 
 ```python
-relationships = data.get("relationships", {})
-run_data = relationships.get("run", {}).get("data")
-if run_data:
-    attributes["run"] = Run.model_construct(id=run_data["id"])
+from .._jsonapi import parse_relationships
+
+_WIDGET_REL_MAP = {"organization": Organization, "current-run": Run}
+
+def _widget_from(self, data: dict[str, Any], included=None) -> Widget:
+    attrs = dict(data.get("attributes") or {})
+    attrs["id"] = data.get("id")
+    attrs.update(parse_relationships(data.get("relationships"), _WIDGET_REL_MAP, included=included))
+    return Widget.model_validate(attrs)
+
+# caller threads included from the envelope:
+payload = r.json()
+return self._widget_from(payload["data"], payload.get("included"))
 ```
 
-2. **Flatten to `*_id`** when the model exposes a flat `team_id: str | None` field:
+`parse_relationships` skips null/absent/unmapped relations, handles single vs list `data`, and builds id-only stubs via `model_construct` when `included` is absent. For a relation the model exposes as a **flat `*_id`** field instead of an embedded model, keep the small manual extraction:
 
 ```python
-team_data = (relationships.get("team") or {}).get("data") or {}
+team_data = (data.get("relationships", {}).get("team") or {}).get("data") or {}
 if team_data.get("id"):
-    attributes["team-id"] = team_data["id"]
+    attrs["team-id"] = team_data["id"]
 ```
 
-Always defensively coalesce with `or {}` — relationships may be missing from sparse responses.
+Keep polymorphic relations (e.g. `locked-by`) and relations whose `data` carries inline attributes (e.g. workspace `outputs`) as explicit special cases — they don't fit the map. See `resources/workspaces.py` and `resources/run.py` for the reference parsers. Always defensively coalesce with `or {}` — relationships may be missing from sparse responses.
+
+> If the parser is a **method** on a service class that defines `def list(...)`, you can't annotate `included: list[...] | None` (in class scope `list` is the method). Prefer a module-level `_widget_from` function (like `_ws_from`/`_run_from`), or use `builtins.list[...]`.
 
 
 ## Pagination — use `self._list`, don't roll your own

src/pytfe/_jsonapi.py

@@ -24,3 +24,106 @@ def parse_error_payload(payload: dict[str, Any]) -> list[dict | str]:
     if "message" in payload:
         return [{"detail": payload.get("message")}]
     return []
+
+
+# --------------------------------------------------------------------------- #
+# Relationship / included parsing
+#
+# HCP Terraform speaks JSON:API: a resource object carries an ``attributes`` map
+# and a ``relationships`` map, and a response may carry a top-level ``included``
+# array holding the full bodies of related resources requested via ``?include=``.
+#
+# These helpers are the single canonical implementation that resource parsers
+# converge on, replacing the per-resource hand-rolled relationship parsing
+# (``workspaces._ws_from`` if-ladder, ``run.transform_relationships``,
+# ``no_code_module`` included-index).
+# --------------------------------------------------------------------------- #
+
+IncludedIndex = dict[tuple[Any, Any], dict[str, Any]]
+
+# A relation map value is either the model class (attr name derived from the
+# wire relation name by ``-`` -> ``_``) or an explicit ``(python_attr, Model)``
+# tuple for the cases where they diverge (e.g. ``vars`` -> ``variables``).
+RelationSpec = "type | tuple[str, type]"
+RelationMap = dict[str, Any]
+
+
+def build_included_index(included: list[dict[str, Any]] | None) -> IncludedIndex:
+    """Index a JSON:API ``included`` array by ``(type, id)``.
+
+    JSON:API guarantees ``(type, id)`` uniqueness within a document, so the
+    first occurrence wins deterministically if a server ever violates that.
+    """
+    index: IncludedIndex = {}
+    for item in included or []:
+        index.setdefault((item.get("type"), item.get("id")), item)
+    return index
+
+
+def _hydrate(ref: dict[str, Any], model: Any, index: IncludedIndex) -> Any | None:
+    """Resolve a single relationship reference to a model instance.
+
+    If the full resource body is present in ``index`` (because it was requested
+    via ``?include=``), validate it into a fully-populated model. Otherwise
+    return a lightweight ``{id}`` stub via ``model_construct`` (no validation —
+    correct, because only ``id``/``type`` are known).
+    """
+    rid = ref.get("id")
+    if rid is None:
+        return None
+    full = index.get((ref.get("type"), rid))
+    if full is not None:
+        attrs = dict(full.get("attributes") or {})
+        attrs["id"] = rid
+        try:
+            return model.model_validate(attrs)
+        except Exception:
+            # A hydration failure must never break parsing of the parent.
+            return model.model_construct(id=rid)
+    return model.model_construct(id=rid)
+
+
+def parse_relationships(
+    relationships: dict[str, Any] | None,
+    rel_map: RelationMap,
+    *,
+    included: list[dict[str, Any]] | None = None,
+) -> dict[str, Any]:
+    """Parse a JSON:API ``relationships`` block into ``{python_attr: value}``.
+
+    ``rel_map`` maps each wire relationship name to either a model class (the
+    python attribute is derived as ``wire.replace("-", "_")``) or an explicit
+    ``(python_attr, Model)`` tuple. Single references become a model instance;
+    lists become a list of model instances. Null/empty relationships and
+    relations absent from ``rel_map`` are skipped, so they never clobber model
+    defaults (undeclared relations are caught by ``extra="allow"``).
+    """
+    rels = relationships or {}
+    index = build_included_index(included)
+    out: dict[str, Any] = {}
+
+    for wire, spec in rel_map.items():
+        if isinstance(spec, tuple):
+            attr, model = spec
+        else:
+            attr, model = wire.replace("-", "_"), spec
+
+        rel = rels.get(wire)
+        if not isinstance(rel, dict):
+            continue
+        data = rel.get("data")
+        if data is None:
+            continue
+        if isinstance(data, list):
+            out[attr] = [
+                m
+                for ref in data
+                if isinstance(ref, dict)
+                and (m := _hydrate(ref, model, index)) is not None
+            ]
+        elif isinstance(data, dict):
+            m = _hydrate(data, model, index)
+            if m is not None:
+                out[attr] = m
+
+    return out

src/pytfe/models/agent.py

@@ -52,7 +52,9 @@ class Agent(BaseModel):
 class AgentPool(BaseModel):
     """Agent Pool represents a Terraform Enterprise agent pool."""
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     name: str | None = Field(default=None, alias="name")

src/pytfe/models/apply.py

@@ -22,7 +22,9 @@ class ApplyStatus(str, Enum):
 
 
 class Apply(BaseModel):
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     log_read_url: str | None = Field(None, alias="log-read-url")
@@ -37,7 +39,9 @@ class Apply(BaseModel):
 
 
 class ApplyStatusTimestamps(BaseModel):
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     canceled_at: datetime | None = Field(None, alias="canceled-at")
     errored_at: datetime | None = Field(None, alias="errored-at")

src/pytfe/models/assessment_result.py

@@ -11,7 +11,9 @@
 class AssessmentResult(BaseModel):
     """Result of a workspace health assessment (drift detection)."""
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     succeeded: bool | None = Field(default=None, alias="succeeded")

src/pytfe/models/comment.py

@@ -10,7 +10,9 @@
 
 
 class Comment(BaseModel):
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     body: str = Field(default="", alias="body")

src/pytfe/models/cost_estimate.py

@@ -10,7 +10,9 @@
 
 
 class CostEstimate(BaseModel):
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     delta_monthly_cost: str = Field(default="", alias="delta-monthly-cost")
@@ -36,7 +38,9 @@ class CostEstimateStatus(str, Enum):
 
 
 class CostEstimateStatusTimestamps(BaseModel):
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     canceled_at: datetime = Field(..., alias="canceled-at")
     errored_at: datetime = Field(..., alias="errored-at")

src/pytfe/models/explorer.py

@@ -27,7 +27,9 @@ class ExplorerViewType(str, Enum):
 class ExplorerUrlFilter(BaseModel):
     """One slot in ExplorerQueryOptions.filters → filter[i][field][op][idx] query keys."""
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     index: int = Field(..., ge=0, description="Filter index in the query string")
     field: str = Field(
@@ -72,7 +74,9 @@ class ExplorerRow(BaseModel):
     snake variants.
     """
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     type: str
@@ -82,7 +86,9 @@ class ExplorerRow(BaseModel):
 class ExplorerSavedQueryFilter(BaseModel):
     """One saved-view filter row (list-valued `value` matches create/update JSON)."""
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     field: str = Field(..., min_length=1)
     operator: str = Field(..., min_length=1)
@@ -92,7 +98,9 @@ class ExplorerSavedQueryFilter(BaseModel):
 class ExplorerSavedQuery(BaseModel):
     """Nested query on a saved view: view type, filters, optional fields and sort lists."""
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     query_type: ExplorerViewType = Field(..., alias="type")
     filter: list[ExplorerSavedQueryFilter] | None = None
@@ -109,7 +117,9 @@ class ExplorerSavedView(BaseModel):
     request/response payload.
     """
 
-    model_config = ConfigDict(populate_by_name=True, validate_by_name=True)
+    model_config = ConfigDict(
+        populate_by_name=True, validate_by_name=True, extra="allow"
+    )
 
     id: str
     name: str