Skip to content

fix(openapi): add type discriminator to DestinationUpdate union#920

Merged
leggetter merged 1 commit into
fix/spec-sdk-fixesfrom
fix/destination-update-discriminator
May 28, 2026
Merged

fix(openapi): add type discriminator to DestinationUpdate union#920
leggetter merged 1 commit into
fix/spec-sdk-fixesfrom
fix/destination-update-discriminator

Conversation

@leggetter

@leggetter leggetter commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two distinct-but-intertwined bugs in destination PATCH, both rooted in the spec. This PR fixes both at the source.

Bug 1 — Wire-serialization (runtime, silent failure)

PATCH requests that try to update part of a non-Webhook destination's config silently fail. The API returns 200 OK, but the destination's config is unchanged.

Concretely — calling the TS SDK to change an AWS SQS destination's queue URL:

await outpost.destinations.update("test-tenant", "des_123", {
  config: { queueUrl: "https://sqs.us-west-2.amazonaws.com/.../updated-queue" },
});

The TS-facing field is queueUrl (camelCase), which the SQS outbound schema is supposed to remap to queue_url (snake_case) on the wire. But the request actually goes out as { "config": { "queueUrl": "..." } } — camelCase reaches the server. Merge-patch doesn't recognize the key, leaves queue_url untouched, and the response shows the old queue.

Why: the DestinationUpdate oneOf has no discriminator (removed in 8be278c5, May 2025, when type wasn't yet accepted on PATCH). Without one, the SDK picks a union variant by structural shape-matching. DestinationUpdateHookdeck's config: {} (intent: "no updatable config"; OpenAPI: "any value allowed") wins for every partial-config payload because it accepts anything. Hookdeck's variant has no field-name remap — that's the actual mis-serialization. Affects AWS SQS, S3, Kinesis, RabbitMQ, GCP Pub/Sub, Azure Service Bus, and Kafka partial-config updates.

Bug 2 — Typing (compile-time, blocks correct usage)

Even fully correct payloads couldn't be expressed as partial updates. The spec referenced the full *Config / *Credentials schemas — which require their fields — from the DestinationUpdate* variants. So:

// Before this PR — TS error: `queueUrl` is required on AWSSQSConfig
update("t", "id", { config: { endpoint: "https://..." } });

The OpenAPI source even acknowledges the gap with inline comments like # queue_url is required here, but PATCH means it's optional. The fix has been a TODO note in production.

Fix

  • Add type (required, enum-locked) to every DestinationUpdate* variant and discriminator: propertyName: type on the union (mirrors DestinationCreate) → resolves Bug 1.
  • Introduce 17 new *ConfigUpdate / *CredentialsUpdate companion schemas (all-optional fields, RFC 7396 merge-patch documented) and reference these from DestinationUpdate* variants → resolves Bug 2.
  • Remove config: {} from DestinationUpdateHookdeck. Hookdeck has no updatable config; HookdeckCredentialsUpdate handles partial credential updates.

Server-side merge-patch landed in bd7701b5 (April 2026), so the runtime side is already correct — only the spec needed to catch up.

TS SDK typing after regen

DestinationUpdate becomes a true discriminated union — type: "aws_sqs" literal types narrow config to the matching *ConfigUpdate. Examples:

update("t", "id", { config: { queueUrl: "x" } });
// ❌ Missing required `type`

update("t", "id", { type: "aws_sqs", config: { streamName: "x" } });
// ❌ `streamName` doesn't exist on AWSSQSConfigUpdate (it's `queueUrl` / `endpoint`)

update("t", "id", { type: "aws_kinesis", config: { streamName: "x" } });
// ✅ Narrowed to DestinationUpdateAWSKinesis; only Kinesis fields allowed; partial config ok

IDE autocomplete on config: now shows only fields valid for the chosen type.

(Zod note: the regen emits z.union([…]), not z.discriminatedUnion(…). Functionally equivalent here — every variant requires type: z.literal("…"), so only the matching variant parses. Speakeasy's choice; not worth chasing in this PR.)

Spec changes

  • type added to every DestinationUpdate* variant: required, enum-locked, documented as immutable on PATCH.
  • discriminator: propertyName: type added to DestinationUpdate.
  • 17 new *ConfigUpdate / *CredentialsUpdate companion schemas — all fields optional, RFC 7396 merge-patch documented. Variants now reference these instead of the full *Config / *Credentials.
  • config: {} removed from DestinationUpdateHookdeck.

Consumer impact

Consumer Impact
Typed SDK (TS / Python / Go) Breaking after regen. update() payloads must include type: '<destination>'. Compile-error otherwise.
Raw HTTP Server still accepts PATCH without type at runtime — calls keep working but are spec-noncompliant. Server-side enforcement is a separate decision.
API ref docs / code samples / MCP server Regenerated as part of the SDK regen PR.

What's NOT in this PR (deliberately)

  • SDK regen — bot-driven via .github/workflows/sdk_generation_outpost_*.yaml. Tests here were validated locally against a TS regen (1.4.1); CI compile-check on this PR will fail until the regen PR lands.
  • Server-side enforcement of required type — separate decision.
  • CI for spec-sdk-tests — confirmed none exists today (verified across .github/workflows and .speakeasy). Follow-up PR.

spec-sdk-tests changes (knock-on)

  • type: '<destination>' inserted into 46 updateDestination payloads across 9 files.
  • Paginator response shape fixed (response.result.models) in events.test.ts / tenants.test.tssupersedes fix/spec-sdk-tests-paginator-shape.
  • SDK retries enabled (backoff on 429/5xx) in utils/sdk-client.ts — managed-Outpost rate limits no longer flake tests.
  • Cleanup parallelized in webhook-merge-patch after hook (15 destinations × serial DELETE was exceeding mocha's 10s hook timeout).
  • console.warn('Failed to cleanup destination:', error) → safe String(error) form in all 17 cleanup hooks (node util.inspect was crashing on SDK errors with getter-only descriptors).

Test status

Against managed Outpost (api.outpost.hookdeck.com), locally with regen'd TS SDK 1.4.1:

Passing Failing Pending
Before 133 14 15
After 152 0 15

The 15 pending are the same skipped tests as before (Hookdeck token verification, GCP Pub/Sub validation gaps — pre-existing backend limitations).

Also of note: spec-sdk-tests/TEST_STATUS.md had attributed the AWS Kinesis partial-config update failure to a "backend bug." It's the same SDK-side dispatch issue this PR fixes — the Kinesis test should pass after regen and can be un-skipped in a follow-up.

Test plan

  • Spec validates (swagger-cli validate) — re-run on final branch state.
  • SDK regen PR merges → CI compile-check goes green on this PR.
  • Full spec-sdk-tests run against managed Outpost — re-run on final branch state: 152 passing / 0 failing / 15 pending.
  • Add spec-sdk-tests CI workflow — tracked in Add CI workflow for spec-sdk-tests (SDK acceptance testing) #921.
  • Decide whether to enforce required type server-side (follow-up).

🤖 Generated with Claude Code

Trigger: the DestinationUpdate union lost its discriminator in 8be278c
(May 2025), when `type` wasn't on PATCH. Server has since added `type`
acceptance and RFC 7396 merge-patch (bd7701b), making the missing
discriminator an active bug — Hookdeck's permissive `config: {}` variant
greedily matched any partial-config payload, the SDK sent it through
with no field remap (camelCase reached the server), and merge-patch
silently no-op'd. All non-Webhook partial config updates affected.

Spec: `type` becomes a required, enum-locked discriminator on every
DestinationUpdate* variant; 17 new `*ConfigUpdate`/`*CredentialsUpdate`
companions model the now-explicit partial PATCH shape; Hookdeck's
`config: {}` removed.

Impact:
  * Breaking for typed SDK consumers: `update()` must include `type`
    after regen. Server is more lenient (still accepts PATCH without
    `type`), so raw HTTP callers keep working but are spec-noncompliant.
  * SDK regen left to the bot — separate PR. CI compile-check on this
    PR will fail until that lands; tests were validated locally
    against a 1.4.1 regen.

spec-sdk-tests (knock-on): `type` added to 46 update sites; paginator
shape fixed (supersedes fix/spec-sdk-tests-paginator-shape); SDK retries
enabled; cleanup parallelized; cleanup-error logging hardened.

Test status (managed Outpost, local regen'd SDK): 133→152 passing,
14→0 failing. spec-sdk-tests have no CI today; follow-up PR will add one.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@leggetter leggetter requested a review from alexluong May 28, 2026 15:13
@leggetter leggetter changed the base branch from main to feat/spec-sdk-fixes May 28, 2026 16:33
@leggetter leggetter marked this pull request as ready for review May 28, 2026 16:43
@leggetter leggetter merged commit bc07450 into fix/spec-sdk-fixes May 28, 2026
2 of 3 checks passed
@leggetter leggetter deleted the fix/destination-update-discriminator branch May 28, 2026 17:04
leggetter added a commit that referenced this pull request May 28, 2026
…verlay drop

Trigger: #919 (merged into umbrella) drops the x-speakeasy-pagination
overlay, so the TS SDK no longer wraps list responses in PageIterator.
events.list()/tenants.list() return EventPaginatedResult/TenantPaginatedResult
directly — accessor is `response.models`, not `response.result.models`.

Tests in #920 had been adapted to the wrapper shape; this realigns them
back to the flat shape post-regen. Local run after fresh TS SDK regen:
153 passing / 0 failing / 15 pending.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alexluong added a commit that referenced this pull request May 29, 2026
…event.data oneOf (#924)

* fix(openapi): add type discriminator to DestinationUpdate union (#920)

Trigger: the DestinationUpdate union lost its discriminator in 8be278c
(May 2025), when `type` wasn't on PATCH. Server has since added `type`
acceptance and RFC 7396 merge-patch (bd7701b), making the missing
discriminator an active bug — Hookdeck's permissive `config: {}` variant
greedily matched any partial-config payload, the SDK sent it through
with no field remap (camelCase reached the server), and merge-patch
silently no-op'd. All non-Webhook partial config updates affected.

Spec: `type` becomes a required, enum-locked discriminator on every
DestinationUpdate* variant; 17 new `*ConfigUpdate`/`*CredentialsUpdate`
companions model the now-explicit partial PATCH shape; Hookdeck's
`config: {}` removed.

Impact:
  * Breaking for typed SDK consumers: `update()` must include `type`
    after regen. Server is more lenient (still accepts PATCH without
    `type`), so raw HTTP callers keep working but are spec-noncompliant.
  * SDK regen left to the bot — separate PR. CI compile-check on this
    PR will fail until that lands; tests were validated locally
    against a 1.4.1 regen.

spec-sdk-tests (knock-on): `type` added to 46 update sites; paginator
shape fixed (supersedes fix/spec-sdk-tests-paginator-shape); SDK retries
enabled; cleanup parallelized; cleanup-error logging hardened.

Test status (managed Outpost, local regen'd SDK): 133→152 passing,
14→0 failing. spec-sdk-tests have no CI today; follow-up PR will add one.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(spec): expose event.data on Attempt when include=event.data (#917)

* test(spec-sdk): fix paginator response shape (response.result.models)

Speakeasy CLI 1.741.7 → 1.753.0 (commit 3f311e0, 2026-03-13) changed
the generated TS SDK return type for list operations from the flat
paginated body to a PageIterator wrapper: ListEventsResponse now wraps
the body in `result`, so callers access `response.result.models` instead
of `response.models`. The shape change was a side-effect of the
Speakeasy version bump and wasn't surfaced in the SDK regen PR changelog
(which only flagged the unrelated `request` query-param restyle).

Tests in spec-sdk-tests/tests/{events,tenants}.test.ts still used the
pre-bump accessor and failed to compile against the current SDK.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(spec-sdk): assert event.data is exposed when include=event.data

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(spec): order EventFull before EventSummary in Attempt.event oneOf

Speakeasy's TS generator emits zod unions in declaration order. The
non-strict EventSummary matched first and silently stripped event.data
from the parsed attempt — losing the payload added by include=event.data.
Declaring EventFull first lets it match when data is present, with
EventSummary as the fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(sdks): drop x-speakeasy-pagination overlay from all SDK sources (#919)

The shared pagination-config-overlay added x-speakeasy-pagination to list
endpoints, which Speakeasy then converted into PageIterator wrappers (TS),
res.Next() helpers (Go), and res.next() chaining (Python).

In TS the wrapper changed the response shape to response.result.models,
which is a noisy regression for the common single-page case. In Go and
Python the helper saves one line at the cost of hiding which request
params get carried across the cursor call.

Drop the overlay from all three sources for symmetric, explicit DX:
callers paginate manually using res.pagination.next on the flat response.

SDK regeneration is intentionally left out of this commit so the workflow
change can be reviewed in isolation.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(spec-sdk): revert paginator wrapper accessors after pagination overlay drop

Trigger: #919 (merged into umbrella) drops the x-speakeasy-pagination
overlay, so the TS SDK no longer wraps list responses in PageIterator.
events.list()/tenants.list() return EventPaginatedResult/TenantPaginatedResult
directly — accessor is `response.models`, not `response.result.models`.

Tests in #920 had been adapted to the wrapper shape; this realigns them
back to the flat shape post-regen. Local run after fresh TS SDK regen:
153 passing / 0 failing / 15 pending.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Alex Luong <alex.luong@hookdeck.com>
leggetter added a commit that referenced this pull request May 29, 2026
… a local Outpost (#925)

* fix(openapi): add type discriminator to DestinationUpdate union (#920)

Trigger: the DestinationUpdate union lost its discriminator in 8be278c
(May 2025), when `type` wasn't on PATCH. Server has since added `type`
acceptance and RFC 7396 merge-patch (bd7701b), making the missing
discriminator an active bug — Hookdeck's permissive `config: {}` variant
greedily matched any partial-config payload, the SDK sent it through
with no field remap (camelCase reached the server), and merge-patch
silently no-op'd. All non-Webhook partial config updates affected.

Spec: `type` becomes a required, enum-locked discriminator on every
DestinationUpdate* variant; 17 new `*ConfigUpdate`/`*CredentialsUpdate`
companions model the now-explicit partial PATCH shape; Hookdeck's
`config: {}` removed.

Impact:
  * Breaking for typed SDK consumers: `update()` must include `type`
    after regen. Server is more lenient (still accepts PATCH without
    `type`), so raw HTTP callers keep working but are spec-noncompliant.
  * SDK regen left to the bot — separate PR. CI compile-check on this
    PR will fail until that lands; tests were validated locally
    against a 1.4.1 regen.

spec-sdk-tests (knock-on): `type` added to 46 update sites; paginator
shape fixed (supersedes fix/spec-sdk-tests-paginator-shape); SDK retries
enabled; cleanup parallelized; cleanup-error logging hardened.

Test status (managed Outpost, local regen'd SDK): 133→152 passing,
14→0 failing. spec-sdk-tests have no CI today; follow-up PR will add one.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(spec): expose event.data on Attempt when include=event.data (#917)

* test(spec-sdk): fix paginator response shape (response.result.models)

Speakeasy CLI 1.741.7 → 1.753.0 (commit 3f311e0, 2026-03-13) changed
the generated TS SDK return type for list operations from the flat
paginated body to a PageIterator wrapper: ListEventsResponse now wraps
the body in `result`, so callers access `response.result.models` instead
of `response.models`. The shape change was a side-effect of the
Speakeasy version bump and wasn't surfaced in the SDK regen PR changelog
(which only flagged the unrelated `request` query-param restyle).

Tests in spec-sdk-tests/tests/{events,tenants}.test.ts still used the
pre-bump accessor and failed to compile against the current SDK.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(spec-sdk): assert event.data is exposed when include=event.data

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(spec): order EventFull before EventSummary in Attempt.event oneOf

Speakeasy's TS generator emits zod unions in declaration order. The
non-strict EventSummary matched first and silently stripped event.data
from the parsed attempt — losing the payload added by include=event.data.
Declaring EventFull first lets it match when data is present, with
EventSummary as the fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(sdks): drop x-speakeasy-pagination overlay from all SDK sources (#919)

The shared pagination-config-overlay added x-speakeasy-pagination to list
endpoints, which Speakeasy then converted into PageIterator wrappers (TS),
res.Next() helpers (Go), and res.next() chaining (Python).

In TS the wrapper changed the response shape to response.result.models,
which is a noisy regression for the common single-page case. In Go and
Python the helper saves one line at the cost of hiding which request
params get carried across the cursor call.

Drop the overlay from all three sources for symmetric, explicit DX:
callers paginate manually using res.pagination.next on the flat response.

SDK regeneration is intentionally left out of this commit so the workflow
change can be reviewed in isolation.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(spec-sdk): revert paginator wrapper accessors after pagination overlay drop

Trigger: #919 (merged into umbrella) drops the x-speakeasy-pagination
overlay, so the TS SDK no longer wraps list responses in PageIterator.
events.list()/tenants.list() return EventPaginatedResult/TenantPaginatedResult
directly — accessor is `response.models`, not `response.result.models`.

Tests in #920 had been adapted to the wrapper shape; this realigns them
back to the flat shape post-regen. Local run after fresh TS SDK regen:
153 passing / 0 failing / 15 pending.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(spec-sdk-tests): add workflow that runs the contract suite against a local Outpost

Closes #921.

Trigger: PRs touching the spec, TS SDK, Speakeasy config, handlers,
destination providers, models, or server entry. Plus workflow_dispatch
and a weekly cron as drift safety net.

Job: spin up Postgres/Redis/RabbitMQ as service containers, build the
outpost binary from source, run migrations, start api + delivery in
background, wait for /healthz, build the TS SDK from this PR's spec
state, then run `npm test` in spec-sdk-tests pointed at localhost.

Notes:
* Uses RabbitMQ for the message queue (default in .outpost.yaml.dev).
* spec-sdk-tests/.env is written inline so we never need to commit a
  CI-specific .env to the repo.
* Server logs are dumped on failure for debuggability.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(spec-sdk-tests): fix YAML — use block scalar for migrations run step

The quoted-string form 'run: "$OUTPOST_BIN" migrate apply --yes' confuses
the YAML parser (treats the leading quote as opening a scalar). Block-scalar
form matches the other run: steps in the file.

* ci(spec-sdk-tests): use outpost-server in singular mode, not the CLI wrapper

The 'outpost' binary is just a CLI wrapper that delegates 'serve' to
a separate 'outpost-server' binary. Running it without a subcommand
prints help and exits — which is why /healthz never came up.

Switch to building both binaries: 'outpost' for migrations, then
'outpost-server' for the actual server. Drop SERVICE=api / SERVICE=delivery
in favour of singular mode (empty SERVICE → api + log + delivery in one
process), which is enough for the contract tests and halves the moving
parts.

* ci(spec-sdk-tests): use npm install for spec-sdk-tests deps

spec-sdk-tests/.gitignore excludes package-lock.json (intentional —
the test suite isn't published, and treating SDK contract tests as
production-locked dependencies adds noise to PRs without value).
npm ci needs a lock file; switch to npm install.

* ci(spec-sdk-tests): regen TS SDK from the PR's spec before running tests

The whole point of this workflow is to validate that spec ↔ SDK ↔
server agree. If we test against the checked-in SDK src/ (which can
lag the spec arbitrarily, since regen is bot-driven), we're testing
the wrong thing — drift hides instead of surfacing.

Add a Speakeasy regen step before the existing build. Now any PR that
changes docs/apis/openapi.yaml gets its tests run against an SDK
regen'd from that PR's spec, exercising the actual contract this CI
exists to validate.

Uses the SPEAKEASY_API_KEY secret already configured for the publish
workflows.

* ci(spec-sdk-tests): use redis-stack-server for RediSearch

tenants.list requires RediSearch; vanilla redis:7-alpine returns
501 'list tenant feature is not enabled'. Swap to the official
redis/redis-stack-server image which bundles RediSearch. Same port
(6379), same redis-cli ping for health check.

* ci(spec-sdk-tests): expand header with explicit scope and rationale

Makes it clear at the top of the file what this workflow validates
(PR's server vs PR's spec), what it does not (managed drift, SDK
version compat, post-deploy smoke), and why we picked this shape
over alternatives. Spares future maintainers from re-litigating
the design decision.

* ci(spec-sdk-tests): drop the weekly cron

The cron was copied from the docs-eval pattern out of habit. In this
workflow it adds essentially no signal — between PRs, main's state
hasn't changed, so a scheduled run re-tests what we last tested. The
"deploy drift" question a cron would help with is out of scope for
this workflow anyway (it's about PR's spec vs PR's server, not
managed vs main).

* ci(spec-sdk-tests): add cmd/outpost-server/** to trigger paths

Workflow builds and runs ./cmd/outpost-server but the path filter only
watched cmd/outpost/** (the CLI wrapper). Changes to the actual server
entrypoint could land without triggering the contract suite.

Spotted by Copilot review on #925.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Alex Luong <alex.luong@hookdeck.com>
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