Skip to content

fix(codegen): preserve regenerated response aliases#854

Merged
bokelley merged 4 commits into
mainfrom
bokelley/review-pr-824-aliases
May 25, 2026
Merged

fix(codegen): preserve regenerated response aliases#854
bokelley merged 4 commits into
mainfrom
bokelley/review-pr-824-aliases

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Supersedes #824 using an in-repository branch so required CodeQL and Argus review checks can run with normal repository permissions.

Includes the original generated-alias fix from #824 plus follow-up fixes:

  • pins semantic response aliases with a regression test
  • keeps generated Product aliases mypy-clean
  • wraps alias fallback definitions for ruff

Validation run locally:

  • uv run --extra dev ruff check src/
  • uv run --extra dev mypy src/adcp
  • uv run --extra dev pytest tests/test_code_generation.py -q --tb=short

@bokelley bokelley enabled auto-merge (squash) May 25, 2026 02:00
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

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

LGTM. Follow-ups noted below. Replaces the eager getattr(_g, "FooResponse1", _g.FooResponse) fallback with a lazy try/except helper — the right shape now that regenerated _generated.py carries concrete *Response1 arms instead of *Response1 = *Response shims.

Things I checked

  • Lazy fallback semantics (src/adcp/types/aliases.py:106-110): _generated_alias evaluates the fallback only on AttributeError. Old form forced eager evaluation of _g.FooResponse regardless. New form is import-time safe even when the parent union goes away upstream.
  • Regression pin (tests/test_code_generation.py:32-44): regex getattr\(_g,\s*"[^"]+",\s*_g\.[A-Za-z_]+\s*\) matches only the attribute-fallback form, not the new string-fallback _generated_alias calls. Sound guard.
  • Semantic alias resolution (tests/test_code_generation.py:54-104): pins ActivateSignalSuccessResponse → ActivateSignalResponse1, AcquireRightsAcquiredResponse → AcquireRightsResponse1, CreateMediaBuySubmittedResponse → CreateMediaBuyResponse3, plus six more. This is the actual fix — the original bug was these collapsing silently to the parent union.
  • Wire-shape orderings (ad-tech-protocol-expert verified against upstream schemas/source/): AcquireRights oneOf is [Acquired, PendingApproval, Rejected, Error], matching aliases.py:642-652 (1→4). CreateMediaBuy [Success, Error, Submitted] matches aliases.py:461-479. UpdateMediaBuy same ordering matches aliases.py:607-614.
  • ClaimType relocation (_generated.py:202-209): upstream verify-brand-claim-request.json defines claim_type as the singular task's discriminator, so the new import location is spec-correct. No external consumers of the old location in src/, tests/, or examples/.
  • extract_exports_from_module AnnAssign extension (scripts/consolidate_exports.py:38-46): correctly picks up Foo: TypeAlias = Bar declarations the post-generate fixer emits. Unit test at tests/test_code_generation.py:46-52 covers it.
  • _sync_protocol_envelope_import (scripts/post_generate_fixes.py:58-74): consolidates two prior call sites in restore_response_variant_aliases. Insertion is gated on actual ProtocolEnvelope references in the source, so the import doesn't get added speculatively. Regenerated acquire_rights_response.py:5-9 and create_media_buy_response.py:5-9 confirm the placement.

Follow-ups (non-blocking — file as issues)

  • UpdateMediaBuyResponse1/2/3 coercion parity. The PR adds _ergonomic.py patches for CreateMediaBuyResponse1.packages and media_buy_status. The three update_media_buy arms have the same two fields plus affected_packages: Sequence[Package] and currently get no ergonomic coercion. Per code-reviewer this is a pre-existing gap, not a regression — but the same shape, same call ergonomics, should land together.
  • Type erasure through _generated_alias (src/adcp/types/aliases.py:106). Returning Any means ActivateSignalSuccessResponse: TypeAlias = ActivateSignalResponse1 is Any to mypy on the public surface. The file's # mypy: disable-error-code="valid-type" header masks the diagnostic but doesn't restore concrete typing. Consider an if TYPE_CHECKING: branch that binds the alias to the known fallback class statically while keeping runtime lazy.
  • Duplicate ClaimType in verify_brand_claims_response.py:21. Per ad-tech-protocol-expert: pydantic generates a separately-scoped ClaimType enum inside the plural response module. Adopters doing isinstance(result.claim_type, ClaimType) against the public re-export from the singular request module will silently fail (same string values, different class objects). Codegen-level dedup, not in scope here.
  • Forward-compat on VerifyBrandClaimResponse (generated_poc/brand/verify_brand_claim_response.py:59). The Success | Error union has no Discriminator() callable and no UnknownStatus fallback arm. A new upstream oneOf arm will hit ValidationError on adopter deserialization. Same caveat applies to CreateMediaBuyResponse1.media_buy_statuscoerce_to_enum returns the raw string on miss, which strict pydantic rejects.

Minor nits (non-blocking)

  1. _generated_alias docstring (src/adcp/types/aliases.py:106-110). The helper has no docstring explaining why the helper exists or that fallback_name is the parent-union case. Two lines lifted from the PR body would prevent future churn.
  2. Branch (c) in _sync_protocol_envelope_import (scripts/post_generate_fixes.py:74). The raw-prepend path fires only when neither from __future__ import annotations nor AdcpVersionEnvelope exists in the source — which python-expert confirmed never happens on generated_poc/ files. Defensive but dead. Either drop or add a # pragma: no cover hint.

Approving on the strength of the four new regression tests plus the upstream-schema oneOf verification.

@bokelley bokelley merged commit 56d65fd into main May 25, 2026
37 of 38 checks passed
@bokelley bokelley deleted the bokelley/review-pr-824-aliases branch May 25, 2026 02:08
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.

2 participants