Skip to content

[AJDA-2706] Tolerate webhook recipients in Responses\Subscription#26

Draft
ondrajodas wants to merge 5 commits intomainfrom
ondra/AJDA-2706
Draft

[AJDA-2706] Tolerate webhook recipients in Responses\Subscription#26
ondrajodas wants to merge 5 commits intomainfrom
ondra/AJDA-2706

Conversation

@ondrajodas
Copy link
Copy Markdown
Contributor

Fixes deserialization in Responses\Subscription, which previously hardcoded recipient.address and threw Unrecognized response: Undefined array key "address" when a project had any webhook subscription (recipient.url instead of recipient.address).

Changes

  • New polymorphic recipient VOs under Responses\Recipient\:
    • RecipientInterface with getChannel(): string
    • EmailRecipient (channel email, getAddress(): string, EmailRecipient::CHANNEL constant)
    • WebhookRecipient (channel webhook, getUrl(): string, WebhookRecipient::CHANNEL constant)
  • Subscription now branches on recipient.channel and instantiates the right concrete type. Unknown channels still throw ClientException via the existing Unrecognized response: ... wrapper.
  • New Subscription::getRecipient(): RecipientInterface is the primary API.
  • Subscription::getRecipientChannel(): string unchanged (now a thin delegate).
  • README documents the new API and the BC note.

BC change

Subscription::getRecipientAddress() return type widens from string to ?string and returns null for non-email channels (e.g. webhook). New code should prefer getRecipient() with instanceof EmailRecipient / instanceof WebhookRecipient.

Tests

Tests\Responses\SubscriptionTest covers email recipient, webhook recipient, unknown-channel error, and the existing invalid-data path. Dedicated unit tests for EmailRecipient and WebhookRecipient added.

Follow-up

Unblocks AJDA-2596 (flow-migration-tool) — once released there, the loader can branch on getRecipientChannel() and stop filtering webhooks out client-side.

@linear
Copy link
Copy Markdown

linear Bot commented Apr 30, 2026

@ondrajodas ondrajodas requested a review from Copilot April 30, 2026 11:26
@ondrajodas
Copy link
Copy Markdown
Contributor Author

# Release Notes
`Keboola\NotificationClient\Responses\Subscription` no longer throws `Unrecognized response: Undefined array key "address"` when listing subscriptions for a project that contains webhook subscriptions. The recipient is now exposed via a polymorphic VO `getRecipient(): RecipientInterface` with two concrete types: `EmailRecipient` (`getAddress()`) and `WebhookRecipient` (`getUrl()`). Channel literals are also exposed as `EmailRecipient::CHANNEL` / `WebhookRecipient::CHANNEL`.

# Plans for Customer Communication
None — this is an internal PHP client library. Consumers (notably flow-migration-tool, AJDA-2596) will pick up the fix on next composer update.

# Impact Analysis
- Affects every consumer of `Responses\Subscription`. Today: `flow-migration-tool` (currently filters webhooks client-side as a workaround), and any other tool listing notification subscriptions.
- Minor BC: `Subscription::getRecipientAddress()` return type widened from `string` to `?string`. Strictly typed callers may need to handle `null` for webhook recipients. `getRecipientChannel()` and the rest of the public API are unchanged.
- Risk is low: previous behavior threw on webhook subscriptions, so no production code path could rely on `getRecipientAddress()` for webhooks (it would have aborted earlier).

# Change Type
Bug fix + minor refactor (introduces polymorphic recipient VOs).

# Justification
The hardcoded `recipient.address` deserialization aborted listing of any project containing a webhook subscription, blocking webhook notification migration in flow-migration-tool (AJDA-2596). Linear: https://linear.app/keboola/issue/AJDA-2706/

# Deployment Plan
Standard composer release. No config changes, no migrations, no infra changes. Consumers update their composer.lock.

# Rollback Plan
Revert the merge commit and re-release the previous patch version. Consumers pin to the prior version. No data migrations to undo.

# Post-Release Support Plan
- Verify `flow-migration-tool` (AJDA-2596) can drop its client-side webhook filter after upgrading.
- Watch for any third-party consumers that strict-typed `getRecipientAddress()` as `string`; PHPStan in their projects will surface the widened return type.
- No alerts/dashboards to add — this is library code without runtime telemetry.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Responses\Subscription deserialization to support non-email recipients (webhooks) by introducing polymorphic recipient value objects, preventing failures when the API returns recipient.url instead of recipient.address.

Changes:

  • Add RecipientInterface plus EmailRecipient and WebhookRecipient response VOs.
  • Update Responses\Subscription to instantiate the correct recipient type based on recipient.channel, and add getRecipient(): RecipientInterface (with getRecipientAddress(): ?string as BC behavior).
  • Extend unit test coverage and document the new API + BC note in the README.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Responses/Subscription.php Branch recipient parsing by channel and expose a polymorphic getRecipient() API.
src/Responses/Recipient/RecipientInterface.php Defines a common interface for response recipient VOs.
src/Responses/Recipient/EmailRecipient.php Email recipient VO with getAddress() and CHANNEL constant.
src/Responses/Recipient/WebhookRecipient.php Webhook recipient VO with getUrl() and CHANNEL constant.
tests/Responses/SubscriptionTest.php Adds coverage for email/webhook recipients and unknown-channel behavior.
tests/Responses/Recipient/EmailRecipientTest.php Unit test for EmailRecipient accessors and interface conformance.
tests/Responses/Recipient/WebhookRecipientTest.php Unit test for WebhookRecipient accessors and interface conformance.
README.md Documents the new polymorphic recipient API and the BC change for getRecipientAddress().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Responses/Subscription.php Outdated
Comment on lines +41 to +51
$channel = $data['channel'];
assert(is_string($channel));
switch ($channel) {
case EmailRecipient::CHANNEL:
$address = $data['address'];
assert(is_string($address));
return new EmailRecipient($address);
case WebhookRecipient::CHANNEL:
$url = $data['url'];
assert(is_string($url));
return new WebhookRecipient($url);
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

createRecipient() relies on assert(is_string(...)) for runtime input validation. PHP assertions are often disabled in production, which can allow non-string values (or missing keys resulting in null) to flow into the switch/sprintf path and potentially emit warnings/deprecations that are not caught by the surrounding try/catch. Prefer explicit runtime checks (e.g. isset/array_key_exists + is_string) and throw a regular exception when the data is invalid, letting the existing constructor wrapper convert it to ClientException.

Suggested change
$channel = $data['channel'];
assert(is_string($channel));
switch ($channel) {
case EmailRecipient::CHANNEL:
$address = $data['address'];
assert(is_string($address));
return new EmailRecipient($address);
case WebhookRecipient::CHANNEL:
$url = $data['url'];
assert(is_string($url));
return new WebhookRecipient($url);
if (!array_key_exists('channel', $data) || !is_string($data['channel'])) {
throw new \InvalidArgumentException('Invalid recipient data: "channel" must be a string.');
}
$channel = $data['channel'];
switch ($channel) {
case EmailRecipient::CHANNEL:
if (!array_key_exists('address', $data) || !is_string($data['address'])) {
throw new \InvalidArgumentException('Invalid recipient data: "address" must be a string.');
}
return new EmailRecipient($data['address']);
case WebhookRecipient::CHANNEL:
if (!array_key_exists('url', $data) || !is_string($data['url'])) {
throw new \InvalidArgumentException('Invalid recipient data: "url" must be a string.');
}
return new WebhookRecipient($data['url']);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@ondrajodas ondrajodas Apr 30, 2026

Choose a reason for hiding this comment

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

Fixed. Replaced the assert(is_string(...)) calls with an explicit extractString() helper that throws ClientException when a required field is missing or not a string. Behaves correctly even with assertions disabled in production.

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