Skip to content

feat(adcp)!: type media buy status filter unions#179

Merged
bokelley merged 1 commit into
mainfrom
keep-going-unions-v1
May 26, 2026
Merged

feat(adcp)!: type media buy status filter unions#179
bokelley merged 1 commit into
mainfrom
keep-going-unions-v1

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

  • generate a narrow scalar-or-array union helper for media buy status filters
  • type GetMediaBuysRequest.StatusFilter and GetMediaBuyDeliveryRequest.StatusFilter as *MediaBuyStatusFilter
  • fail closed when configured union helper schemas drift, and lint union helper pointers/equivalence
  • reject empty/null direct status filters and document the typed migration pattern
  • lower generated-any coverage budget from 47 to 45

Validation

  • python3 -m py_compile adcp/schemas/generate.py adcp/schemas/lint.py
  • cd adcp/schemas && python3 lint.py --strict
  • cd adcp/schemas && python3 generate.py --coverage-max-unreviewed-any 45
  • cd adcp/schemas && python3 generate.py --coverage-max-unreviewed-any 44 (expected failure)
  • generator idempotence check against adcp/types_gen.go
  • deliberate broken UNION_SCHEMA_TYPES pointer raises ValueError during coverage
  • cd adcp && go test ./...
  • cd adcp && golangci-lint run ./...
  • go test ./...
  • cd reference/seller-agent && go test ./...
  • cd reference/context-agent && go test ./...
  • cd e2e && go test ./...
  • cd bench && go test ./...
  • cd adcp && go test -race -count=1 ./...
  • git diff --check

Expert review

  • Protocol/DX/code reviewers flagged empty filter validation and union pointer false-green risk; both are fixed here.
  • Remaining DX follow-up: consider generated convenience constructors for pointer-heavy union helpers.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 26, 2026
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. Right shape — narrow scalar-or-array helper that fails closed on schema drift and on the wire, with the breaking marker (feat!:) carrying the type change correctly.

Things I checked

  • Conventional-commit story — feat(adcp)!: type media buy status filter unions carries the !, matches any → *MediaBuyStatusFilter field-type change documented in MIGRATING.md:162-163.
  • Wire shape — MarshalJSON emits bare scalar at len==1, array otherwise; UnmarshalJSON tries scalar first then array, rejects null and len==0. Faithful to both media-buy/get-media-buys-request.json#/properties/status_filter and media-buy/get-media-buy-delivery-request.json#/properties/status_filter per ad-tech-protocol-expert.
  • Empty/null rejection is faithful — schema_accepts_empty_array only emits reject-empty when the schema's array branch has minItems >= 1. The generated types_gen.go:1741-1772 contains those reject blocks, so this isn't over-reach.
  • Fail-closed posture on the generator side — supported_union_schemas re-raises bad pointers as ValueError (generate.py:1351), divergent element types at line 1354, divergent minItems at 1356. Linter validate_union_schema_specs (lint.py:312-355) mirrors all three and folds into has_problems at lint.py:542.
  • Tests cover scalar round-trip, array round-trip, empty-marshal rejection, null-unmarshal rejection, and empty-array-unmarshal rejection (generated_core_refs_test.go:458-490).
  • Coverage budget drop 47 → 45 in ci.yml:48 matches two newly-typed fields and PR-body validation that --coverage-max-unreviewed-any 44 fails as expected.
  • types_gen.go is regenerated, not hand-edited; the new helper is emitted under _will_generate_set via supported_union_schemas (generate.py:1144).

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

  • generate.py:1144_will_generate_set() calls supported_union_schemas() without skip_names=KNOWN_TYPES, while generate() (line 1604) and main() (line 1780) pass it. If a union name ever lands in KNOWN_TYPES, _will_generate_set would still mark it auto-generated. Mirror the INLINE_SCHEMA_TYPES pattern at line 1136 and pass skip_names=KNOWN_TYPES here too. Cheap.
  • generate.py:1000-1006schema_accepts_empty_array has no isinstance(schema, dict) guard the way scalar_union_go_type does. A future non-dict items (tuple-form schemas) would raise AttributeError, which is not in SCHEMA_RESOLUTION_ERRORS and would crash both generator and linter outside the fail-closed path. Add the guard.
  • generate.py:1356-1361 — the "primary_schema" selection picks the shortest description (len(description) < len(primary_schema.get('description', ''))). Either the variable name is misleading or the comparator is inverted. Comment intent or pick deterministically by spec order.
  • No automated test exercising the generator's fail-closed path — broken UNION_SCHEMA_TYPES pointer and non-equivalent schemas are only validated by hand in the PR body. A small unit test would lock in the contract.
  • No unmarshal test for garbage scalar shapes (number, object, bool). The current implementation returns the array decoder's opaque error; a test would freeze the error-surface contract for future refactors. generated_core_refs_test.go:458-490.
  • supported_union_schemas(skip_names=KNOWN_TYPES) is invoked three times per main() run (via _will_generate_set, then generate(), then main()). Cheap today at one helper; memoize once a few more land.

Minor nits (non-blocking)

  1. Pointer-to-slice for StatusFilter. *MediaBuyStatusFilter is a pointer to a slice. With empty rejected at marshal, a bare MediaBuyStatusFilter with omitempty would behave identically and avoid the double indirection. The Ptr(...) convention in MIGRATING.md:225-233 is consistent across the codebase, so leave it — worth a passing thought next time a union helper lands.
  2. Scalar Unmarshal laxness. MediaBuyStatus is a string alias, so json.Unmarshal(data, &single) accepts any JSON string including "" and unknown values. Consistent with the rest of the generated enums, but the helper inherits that laxness — note in a follow-up if enum-value validation ever becomes a project goal.

Safe to merge.

@bokelley bokelley force-pushed the keep-going-unions-v1 branch from 86456a5 to a93a149 Compare May 26, 2026 17:32
@bokelley bokelley merged commit c5e2b2f into main May 26, 2026
15 of 17 checks passed
@bokelley bokelley deleted the keep-going-unions-v1 branch May 26, 2026 17:38
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.

Approving. Right shape: narrow scalar-or-array helper, shared UNION_SCHEMA_TYPES config between generator and lint, breaking marker on the commit, regenerated adcp/types_gen.go matches the diff.

Things I checked

  • feat(adcp)!: marker on the single commit. The on-the-wire shape (scalar-or-array) is unchanged; the break is Go-side (StatusFilter any*MediaBuyStatusFilter). Marker correctly applied per the release-please contract.
  • adcp/types_gen.go:1740-1772 was regenerated in this PR; no hand-edits. The helper is generator-owned, so KNOWN_TYPES/EXEMPT semantics still hold.
  • Generator/lint share UNION_SCHEMA_TYPES in adcp/schemas/generate.py:619-627. validate_union_schema_specs (adcp/schemas/lint.py:312-355) cross-checks that both pointers resolve to equivalent scalar-or-array unions with matching minItems; CI fails closed on drift between the two surfaces.
  • INLINE_TYPE_HINTS (adcp/schemas/generate.py:780-781) wires both fields to *MediaBuyStatusFilter; struct declarations in types_gen.go:3610 and :3633 match.
  • Coverage budget 47→45 (.github/workflows/ci.yml:48) matches the two newly-typed fields. The --coverage-max-unreviewed-any 44 negative case in the PR validation list confirms it's tight.
  • Empty/null marshal+unmarshal tests in adcp/generated_core_refs_test.go:485-489 cover the value-type path. Scalar-vs-array round-trip at :478-481 confirms the wire shape.
  • TMP signing, router, identity-agent, skills/adcp-* paths untouched.

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

  • Field-level null doesn't trigger the custom UnmarshalJSON. With StatusFilter *MediaBuyStatusFilter, Go's encoding/json zeros the pointer on JSON null without calling the method. The "reject null" wording in MIGRATING.md and the PR body is accurate only for the value-typed path the test exercises — through the parent struct, {\"status_filter\": null} quietly becomes a nil pointer. Either document the field-level behavior or drop the null-rejection claim from the migration note. Interesting choice keeping the guard in the generated code for the explicit value-typed path; harmless there but not load-bearing via the parent struct.
  • "Shortest description wins" heuristic in supported_union_schemas (adcp/schemas/generate.py:1359-1364) picks the right doc here by accident — both upstream descriptions begin "Filter by status. Can be a single status or array of statuses." and only the request variant trails with "Defaults to ...". A future helper whose generic description is the longer one would get the wrong doc. Swap to an explicit primary_pointer selector in UNION_SCHEMA_TYPES, or at least comment that tie-breaking is tuple order.
  • Empty-rejection is a silent trip-wire on upstream minItems. schema_accepts_empty_array (adcp/schemas/generate.py:1000-1008) returns True when minItems is absent or 0, in which case no rejection is emitted. The generated types_gen.go does emit rejection, which proves both upstream schemas declare minItems: 1 — but if upstream relaxes that constraint in a future bundle, empty filters start round-tripping without a lint error. A one-line comment on schema_accepts_empty_array documenting this dependency would be load-bearing.
  • Pointer-to-slice ergonomics. &MediaBuyStatusFilter{} (non-nil pointer to empty slice) hits the Marshal error path rather than being elided. A bare slice with omitempty would have made "no filter" cheap and impossible to misencode, but the pointer pattern is consistent with the rest of adcp/types_gen.go. Flag for the next union helper authors; not worth re-litigating here.

Minor nits (non-blocking)

  1. Blank-tabbed line in generated code when empty-reject branch is omitted. adcp/schemas/generate.py:1334 emits \\t{reject_empty_unmarshal.strip()}\\n, which produces a \\t\\n line when the branch is empty. Inert today since the only configured union takes the populated branch, but gofumpt will flag it once it does fire. Gate the whole \\t... line rather than relying on .strip().
  2. Redundant guard in lint. adcp/schemas/lint.py:338 checks len(reports) != error_count or len(loaded) != len(schema_specs) — the second clause is implied by the first given the error paths. Trim.

Safe to merge.

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