Skip to content

fix(client): restore pyright field-access checks on lazy models#629

Open
RapidPoseidon wants to merge 2 commits into
mainfrom
fix(client)/restore-pyright-field-checks-on-lazy-models
Open

fix(client): restore pyright field-access checks on lazy models#629
RapidPoseidon wants to merge 2 commits into
mainfrom
fix(client)/restore-pyright-field-checks-on-lazy-models

Conversation

@RapidPoseidon

Copy link
Copy Markdown
Contributor

Why

The job-status break fixed in #628 (.status.state rename) sailed through the type-check CI and only surfaced at runtime in the nightly rapidata-testing job. This PR fixes the reason pyright couldn't catch it.

LazyValidatedModel (the base class of every generated API model) overrides __getattribute__ with a -> Any return. That override is class-visible to type checkers, so pyright treats every attribute access on any generated model as a valid Any — renamed/removed backend fields are silently accepted at type-check time and blow up only at runtime.

Stock Pydantic avoids exactly this by hiding its own __getattr__/__setattr__ behind if not TYPE_CHECKING: (see pydantic/main.py and pydantic#8643). The lazy base re-opened the hole.

Fix

Guard the __getattribute__ override behind if not TYPE_CHECKING:. One file, in the generated-client base class — so it protects all models at once.

  • Runtime unchanged: TYPE_CHECKING is False at runtime, so the lazy access-time guard still runs.
  • Type-check restored: pyright now resolves attribute access against each model's declared field annotations and flags unknown fields.

Verification

# pyright on a deliberate bad access against the real model:
GetJobByIdEndpointOutput.status  -> error: Cannot access attribute "status" (reportAttributeAccessIssue)
GetJobByIdEndpointOutput.state   -> clean

This would have failed the type-check job on the #624 regen PR instead of the downstream nightly run.

🔗 Session: https://session-0eb86da9.poseidon.rapidata.internal/

RapidPoseidon and others added 2 commits June 19, 2026 08:50
The job endpoint response model (GetJobByIdEndpointOutput) was
regenerated in 3.14.2 with its `status` field renamed to `state`
(now an AudienceJobState enum), but RapidataJob.get_status() still
read `.status`, raising AttributeError on every job status check
(e.g. display_progress_bar, get_results).

Read `.state.value` to restore the str return contract; the enum
values (Completed, Failed, ...) already match the strings the
callers compare against.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: lino <lino@rapidata.ai>
LazyValidatedModel overrode __getattribute__ with a `-> Any` return,
which is class-visible to type checkers. pyright then treats every
attribute access on any generated model as a valid `Any`, so renamed
or removed backend fields (e.g. reading `.status` after it became
`.state`) pass the type-check CI and only blow up at runtime.

Guard the override behind `if not TYPE_CHECKING:` — the same trick
Pydantic uses for its own __getattr__/__setattr__ (see pydantic/main.py
and pydantic#8643). Runtime behaviour is unchanged (TYPE_CHECKING is
False at runtime, so the lazy access-time guard still runs); pyright
now resolves attribute access against each model's declared field
annotations and flags unknown fields.

Verified: pyright flags `.status` on GetJobByIdEndpointOutput while
`.state` stays clean, and lazy construct/access still works at runtime.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: lino <lino@rapidata.ai>
@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR fixes a type-checking blind spot that allowed pyright to silently accept renamed/removed backend fields (the immediate cause: .status.state from #628 went undetected). The fix is a one-liner pattern change to LazyValidatedModel, plus the downstream field-access fix in rapidata_job.py.


lazy_model.py — Core Fix

Correctness: ✅ Sound

Wrapping __getattribute__ in if not TYPE_CHECKING: is exactly the right approach. At runtime TYPE_CHECKING is False, so not TYPE_CHECKING is True and the method is defined as before — zero runtime behaviour change. Under pyright TYPE_CHECKING is True, so the block is skipped, and pyright falls back to BaseModel.__getattribute__ which resolves fields against declared annotations. This is the same technique pydantic uses for its own __getattr__/__setattr__ overrides.

Mustache templates: ✅ No changes needed

lazy_model.py is a hand-maintained base class, not a generated file. The mustache templates only import it; they don't define or override anything inside it, so no template update is required.

Minor observations:

  • The TYPE_CHECKING import is appended inline: from typing import Any, Dict, Optional, TYPE_CHECKING. Per the CLAUDE.md convention (from __future__ import annotations + TYPE_CHECKING for type guards), having TYPE_CHECKING on its own import line would be slightly more readable, but this is a nit.
  • The class-body if not TYPE_CHECKING: pattern is intentionally unusual. The comment explaining it is good — keep it.

rapidata_job.py — Field Access Fix

Correctness: ✅ Sound, with one note

# Before
return self._openapi_service.order.job_api.job_job_id_get(self.id).status

# After
return self._openapi_service.order.job_api.job_job_id_get(self.id).state.value

AudienceJobState is a str, Enum — its .value yields the plain string ('Completed', 'Failed', etc.), which matches all the string comparisons in _wait_for_status and get_results. This is correct.

One observation: Since AudienceJobState inherits from str, .state itself would pass string comparisons directly without .value. That said, .state.value is more explicit about intent and avoids relying on the str mixin behaviour, so it's the better choice here.


Tests

No tests are added or modified. Given that this fix is about static analysis (pyright), a unit test for the runtime path wouldn't directly validate the type-checking improvement. The verification in the PR description (pyright flagging a deliberate bad access, 0 errors on the real codebase) is the meaningful signal here.

That said, a test that exercises __getattribute__ on a lazy-constructed model with a bad field — ensuring it still raises TypeError — would protect the runtime guard from future regressions. Consider adding one, even if it's not blocking for this PR.


Summary

Area Status
Core fix correctness
Runtime behaviour unchanged
Downstream field-access fix
Mustache templates ✅ No update needed
Pyright 0 errors ✅ (per PR description)
Test coverage for runtime guard ⚠️ Missing, worth adding

The fix is correct, minimal, and well-motivated. The only actionable suggestion is adding a regression test for the runtime __getattribute__ guard.

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