|
| 1 | +# Fix nested resource types in the Python SDK |
| 2 | + |
| 3 | +## Task |
| 4 | + |
| 5 | +Nested properties on generated resource dataclasses have **no types, no docs, and |
| 6 | +no autocomplete**, and the declared annotation *contradicts* the runtime object. |
| 7 | +Fix this by generating a dataclass per nested object, so nested properties get |
| 8 | +real types, real docstrings, and completion — and so the annotation matches what |
| 9 | +`from_dict` actually assigns. |
| 10 | + |
| 11 | +All generated code lives in `seam/resources/` and `seam/routes/` and is produced |
| 12 | +by `codegen/`. **Do not hand-edit generated files** — change the templates and |
| 13 | +regenerate (`npm run generate`). |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## The problem, concretely |
| 18 | + |
| 19 | +`seam/resources/device.py:118` declares: |
| 20 | + |
| 21 | +```python |
| 22 | +properties: Dict[str, Any] |
| 23 | +``` |
| 24 | + |
| 25 | +but `from_dict` (line 173) assigns a different type entirely: |
| 26 | + |
| 27 | +```python |
| 28 | +properties=DeepAttrDict(d.get("properties", None)), |
| 29 | +``` |
| 30 | + |
| 31 | +Consequences: |
| 32 | + |
| 33 | +| Expression | Runtime | Type checker | |
| 34 | +|---|---|---| |
| 35 | +| `device.properties.locked` | works | **error** — `Dict[str, Any]` has no attribute `locked` | |
| 36 | +| `device.properties["locked"]` | works | passes, returns `Any` — no completion, no docs | |
| 37 | + |
| 38 | +So the access pattern the SDK is designed for does not type-check, and the one |
| 39 | +that type-checks tells you nothing. |
| 40 | + |
| 41 | +**Docs are top-level only.** `codegen/layouts/partials/resource-dataclass.hbs` |
| 42 | +iterates `properties` exactly once and emits a flat `:ivar` list, so nested |
| 43 | +shapes are undocumented. The only mention of any nested field name in all of |
| 44 | +`device.py` is incidental prose inside a *different* field's docstring: |
| 45 | + |
| 46 | +``` |
| 47 | +:ivar display_name: Display name of the device, defaults to nickname (if it is |
| 48 | + set) or ``properties.appearance.name``, otherwise. |
| 49 | +``` |
| 50 | + |
| 51 | +**`errors`/`warnings` are worse than nested objects.** They are |
| 52 | +`List[Dict[str, Any]]` and, unlike `properties`, are *not* wrapped at all |
| 53 | +(`errors=d.get("errors", None)`, line 169). So `device.errors[0].error_code` |
| 54 | +raises `AttributeError` — subscript access is mandatory. Compare Ruby, which |
| 55 | +coerces these into real `ResourceError` objects where `.error_code` works. |
| 56 | + |
| 57 | +**Nested unknown keys are never stripped.** Because nested objects are |
| 58 | +untyped passthrough, `device.properties.anything_new` resolves. This is why |
| 59 | +Python's LTS enforcement is shallower than PHP's — PHP is the only SDK where the |
| 60 | +known-property guarantee holds at depth. Generating nested dataclasses closes |
| 61 | +that gap as a side effect, and that is a primary benefit of this change, not an |
| 62 | +incidental one. |
| 63 | + |
| 64 | +--- |
| 65 | + |
| 66 | +## Approach |
| 67 | + |
| 68 | +Mirror the PHP SDK, which generates a class per nested object (74 of them for |
| 69 | +`Device` alone) and is the only SDK where the guarantee holds deeply. |
| 70 | + |
| 71 | +Blueprint already exposes the nested shape — the JS templates in |
| 72 | +`seamapi/javascript-http` consume `properties`, `itemProperties`, and `variants` |
| 73 | +today — so **no upstream `@seamapi/blueprint` change is needed**. |
| 74 | + |
| 75 | +### 1. `codegen/lib/layouts/resources.ts` — recurse and register nested classes |
| 76 | + |
| 77 | +Currently `getResourceLayoutContexts` builds a flat property list and never |
| 78 | +recurses. Add a registry of nested classes discovered while walking each |
| 79 | +resource's properties: |
| 80 | + |
| 81 | +- `format: 'object'` → nested class from `property.properties` |
| 82 | +- `format: 'list'` with `itemFormat: 'object'` → nested class from `property.itemProperties` |
| 83 | +- `format: 'list'` with `itemFormat: 'discriminated_object'` → merge `variants` |
| 84 | + properties, reusing the existing `mergeResourceProperties` helper (line 34) |
| 85 | +- `format: 'record'` → **leave as `Dict[str, Any]`.** This is genuinely |
| 86 | + free-form JSON (`custom_metadata`) and must stay passthrough. Do not generate a |
| 87 | + class for it and do not strip its keys. |
| 88 | + |
| 89 | +Name nested classes `{ResourceClass}{PascalCasePropertyPath}`, matching PHP: |
| 90 | +`DeviceProperties`, `DeviceBattery`, `DeviceAppearance`. Dedupe by class name — |
| 91 | +PHP does `if (classes.has(name)) return`, and the same collision is expected here |
| 92 | +(e.g. shared sub-shapes between `errors` and `warnings`). |
| 93 | + |
| 94 | +Note the existing `isDictParam` hack becomes unnecessary for object properties: |
| 95 | + |
| 96 | +```ts |
| 97 | +isDictParam: type.startsWith('Dict') || property.name === 'properties', |
| 98 | +``` |
| 99 | + |
| 100 | +The `|| property.name === 'properties'` special case exists only to force |
| 101 | +attribute access on `device.properties`. Once `properties` has a real class, drop |
| 102 | +the special case and keep `isDictParam` for true `record` properties only. |
| 103 | + |
| 104 | +### 2. `codegen/lib/python-type.ts` — return nested class names |
| 105 | + |
| 106 | +`mapPropertyToPythonType` currently maps both `object` and `record` to |
| 107 | +`Dict[str, Any]`. It must return the generated class name when one exists. |
| 108 | + |
| 109 | +This means it can no longer be a pure per-property function — build the nested |
| 110 | +class registry **first**, then thread it into type mapping. Restructure rather |
| 111 | +than trying to keep the current signature. |
| 112 | + |
| 113 | +Leave the scalar mapping alone: `datetime`/`id`/`enum` → `str` stays as-is (see |
| 114 | +Out of scope). |
| 115 | + |
| 116 | +### 3. `codegen/layouts/resource.hbs` and `resource-dataclass.hbs` |
| 117 | + |
| 118 | +Emit multiple dataclasses per module. **Nested classes must be defined before the |
| 119 | +parent** — dataclass field annotations are evaluated at class-creation time, so |
| 120 | +definition order matters. (Alternatively add `from __future__ import annotations`, |
| 121 | +but ordering is simpler and clearer in generated output.) |
| 122 | + |
| 123 | +Hydration in `from_dict`: |
| 124 | + |
| 125 | +```python |
| 126 | +# object |
| 127 | +properties=DeviceProperties.from_dict(d.get("properties")) if d.get("properties") is not None else None, |
| 128 | + |
| 129 | +# list of objects |
| 130 | +errors=[DeviceErrors.from_dict(i) for i in d.get("errors") or []], |
| 131 | +``` |
| 132 | + |
| 133 | +Use `or []` rather than `d.get("errors", [])` so an explicit `null` from the API |
| 134 | +also yields `[]` rather than crashing the comprehension. |
| 135 | + |
| 136 | +**Emit `:ivar` docs for nested classes too.** This is the actual user-facing win — |
| 137 | +each nested class gets the same generated docstring treatment the top-level |
| 138 | +resource gets today. |
| 139 | + |
| 140 | +### 4. Backwards compatibility — read this carefully |
| 141 | + |
| 142 | +`device.properties` is currently a `DeepAttrDict`, which subclasses `dict`. So all |
| 143 | +of this works today and will break under a bare dataclass: |
| 144 | + |
| 145 | +```python |
| 146 | +device.properties["locked"] |
| 147 | +device.properties.get("locked") |
| 148 | +"locked" in device.properties |
| 149 | +for k in device.properties: ... |
| 150 | +json.dumps(device.properties) |
| 151 | +``` |
| 152 | + |
| 153 | +Add a small hand-written mixin under `seam/utils/` that generated nested classes |
| 154 | +inherit, providing `__getitem__`, `get`, `__contains__`, `__iter__`, and `keys`. |
| 155 | +That keeps both access styles working. Only `dict(...)` and |
| 156 | +`isinstance(x, dict)` would still break, which is an acceptable and documentable |
| 157 | +narrowing. |
| 158 | + |
| 159 | +**One intentional behavior change to call out in the changelog:** today |
| 160 | +`device.properties.typo` silently returns an empty `DeepAttrDict` *and mutates the |
| 161 | +object*, inserting the typo'd path (see `seam/utils/deep_attr_dict.py:19-25`). |
| 162 | +With dataclasses it raises `AttributeError`. That is strictly better — it matches |
| 163 | +Ruby, and it turns silent typos into errors — but it is a behavior change and |
| 164 | +someone may be relying on the falsy-empty-dict result. |
| 165 | + |
| 166 | +### 5. Add `seam/py.typed` |
| 167 | + |
| 168 | +There is **no `py.typed` marker anywhere in the repo**. Per PEP 561 that means |
| 169 | +mypy treats the entire `seam` package as untyped and ignores every annotation. |
| 170 | +Pyright/Pylance reads it anyway (`useLibraryCodeForTypes` defaults on), which is |
| 171 | +why top-level completion appears to work in VS Code while mypy users get nothing. |
| 172 | + |
| 173 | +Create an empty `seam/py.typed` and add it to the Poetry package data in |
| 174 | +`pyproject.toml` so it ships in the wheel. |
| 175 | + |
| 176 | +This is independent of the rest of the work and can land first. |
| 177 | + |
| 178 | +### 6. Add a type checker to lint |
| 179 | + |
| 180 | +`just lint` is `pylint` + `black --check` + `rstcheck`. There is no mypy or |
| 181 | +pyright in `pyproject.toml` devDeps, so **the repo currently cannot verify any |
| 182 | +typing claim it makes** — which is how the `Dict[str, Any]` vs `DeepAttrDict` |
| 183 | +mismatch survived. |
| 184 | + |
| 185 | +Add mypy or pyright to devDeps and to `just lint`. Without this there is no |
| 186 | +regression gate on the thing being fixed. |
| 187 | + |
| 188 | +--- |
| 189 | + |
| 190 | +## Out of scope |
| 191 | + |
| 192 | +**Date parsing.** `codegen/lib/python-type.ts` maps `datetime` → `str`, so |
| 193 | +`created_at` is a string. Ruby parses these to `Time` via `date_accessor`. |
| 194 | +Bringing Python to parity is a larger breaking change and belongs in a separate |
| 195 | +task. Do not change it here. |
| 196 | + |
| 197 | +--- |
| 198 | + |
| 199 | +## Tests |
| 200 | + |
| 201 | +pytest, via `just test` (`poetry run pytest --cov=./seam`). Fixtures live in |
| 202 | +`test/conftest.py` and run against `@seamapi/fake-seam-connect`. |
| 203 | + |
| 204 | +Add coverage for: |
| 205 | + |
| 206 | +- nested object hydration — `device.properties` is a `DeviceProperties`, and |
| 207 | + `device.properties.locked` returns the right value |
| 208 | +- list-of-objects hydration — `device.errors[0].error_code` works via |
| 209 | + **attribute** access (this is the regression the change is meant to fix) |
| 210 | +- unknown **nested** key is dropped |
| 211 | +- `record` properties (`custom_metadata`) still pass through untouched, including |
| 212 | + unknown keys |
| 213 | +- dict-compat shims: `["locked"]`, `.get()`, `in`, iteration |
| 214 | +- missing nested object → `None`, missing nested list → `[]` |
| 215 | +- a union resource (`action_attempt`) hydrates its nested `result`/`error` |
| 216 | + |
| 217 | +Note `seam/utils/deep_attr_dict_test.py` exists and tests `DeepAttrDict` |
| 218 | +directly. If `DeepAttrDict` remains in use for `record` properties, keep it and |
| 219 | +its test; only remove it if nothing references it after the change. |
| 220 | + |
| 221 | +--- |
| 222 | + |
| 223 | +## Verification |
| 224 | + |
| 225 | +``` |
| 226 | +cd /home/user/python |
| 227 | +npm ci |
| 228 | +npm run generate # must be idempotent; review the seam/resources/ diff |
| 229 | +just lint |
| 230 | +just test |
| 231 | +``` |
| 232 | + |
| 233 | +Then confirm by inspection: |
| 234 | + |
| 235 | +- `seam/resources/device.py` declares `properties: DeviceProperties` (not |
| 236 | + `Dict[str, Any]`), and `DeviceProperties` is defined above `Device` in the file |
| 237 | + with its own `:ivar` docstrings |
| 238 | +- `custom_metadata` is still `Dict[str, Any]` |
| 239 | +- in an editor with Pylance, `device.properties.` offers completions and |
| 240 | + `device.errors[0].error_code` no longer errors |
| 241 | + |
| 242 | +CI regenerates on every non-`main` push and auto-commits `ci: Generate code` |
| 243 | +(`.github/workflows/generate.yml`), so the generated diff is the real regression |
| 244 | +gate — make sure it is clean and reviewed, not just green. |
| 245 | + |
| 246 | +--- |
| 247 | + |
| 248 | +## Release note |
| 249 | + |
| 250 | +This changes the runtime type of every nested resource property and starts |
| 251 | +dropping unknown nested keys. Treat it as a **major** release, and document: |
| 252 | + |
| 253 | +- nested properties are now typed objects, not `dict` subclasses |
| 254 | +- `dict(...)` / `isinstance(x, dict)` on a nested property no longer works |
| 255 | +- a typo'd nested attribute now raises `AttributeError` instead of returning `{}` |
| 256 | +- previously-visible undocumented nested API fields are now stripped |
0 commit comments