Repository navigation
feat(data): add link and connection pooling composites for v12 CLI - #184
Conversation
There was a problem hiding this comment.
AI found a thing in testing (not sure how I'd test what you did manually)
Redis link support appears inconsistent with Heroku’s Data Links API. This PR accepts heroku-redis as a valid remote, but I tested link creation with Mini Redis both on the target app and on a second app. In both cases, SDK resolution succeeded, then Heroku’s link-create API returned 404: “Remote service … cannot be found or is not supported.”
For comparison, createForDatabase successfully created a link to a Standard-0 Postgres on a second app; listing and deleting the link also passed. That confirms the cross-app fallback works for Postgres.
The current Heroku Data Links docs (https://devcenter.heroku.com/articles/heroku-data-links) describe Postgres-to-Postgres links. Can you confirm whether any Key-Value Store plans are supported as link remotes? If not, the heroku-redis acceptance in link.ts should be removed and the PR/GUS expectations updated.
One blocker, some other AI suggestions.
-
Blocker: src/resources/data/internal/resolve-addon-scoped.ts:42 retries globally when a 404 lacks a resource label. If that response means the requested app is missing, link creation can proceed using a database from other-app::DATABASE. Fail closed on unclassified 404s, or prove the API always labels app-not-found responses.
-
Consider: connectionPooling.attach can reject after creating the pool if onAttached throws or the signal aborts. The attachment remains; callers should avoid blindly retrying and potentially creating a duplicate.
-
Consider: This PR provides the SDK prerequisite, not the full GUS story. Keep the work item open until the downstream CLI migration, characterization tests, and V12-CHANGES.md updates are completed.
|
Thanks, all four were useful. Pushed 0290b3d. Blocker: unlabeled 404 retry. Fixed: Redis remotes. Agreed and removed. The current Data Links article describes Postgres-to-Postgres links only, and your Mini test matches.
GUS work item. Agreed; W-23387136 stays open until the CLI PR lands. The CLI branch already has the migration, characterization tests, and |
Add data.link.listByApp, createForDatabase, and deleteFromDatabase, data.connectionPooling.attach, and data.postgresDatabase.listLegacyByApp so the v12 CLI can migrate pg:links:* and pg:connection-pooling:attach without resolving databases itself. Extension names avoid the generated link and connectionPooling route names. Adds EssentialDatabaseNotSupportedError and InvalidLinkRemoteError. W-23387136
resolveAddonInternal throws AddonNotFoundError (not NotFoundError) when the app-scoped resolve only matches non-Postgres add-ons, so the global retry never ran. Catch both. W-23387136
…posites
- Retry add-on resolution globally only on an add-on miss; a 404 for the
app is rethrown. Shared by createAttachment and createForDatabase, and
app::addon identities resolve once.
- Require an exact heroku-redis or Postgres service name for link remotes
and throw InvalidLinkRemoteError otherwise.
- Rename hooks to beforeCreate/beforeDelete; onResolved takes the add-on.
- attach takes {name, credential}; the release lookup is best-effort.
- Export PgDatabaseNotFoundError, ResolvedPgDatabase, and
EssentialDatabaseNotSupportedError from the data barrel.
- Remove the duplicated createAttachment test block; tests now use derived
mock clients and check extensions don't shadow generated routes.
- resolveAddonScopedThenGlobal retries globally only on an empty match or a 404 labeled resource: 'add_on'. Unlabeled 404s now rethrow (fail closed). The live API labels both app and add-on misses, so this changes no observed behavior. - link.createForDatabase accepts only Heroku Postgres remotes. Heroku Data Links connects Postgres to Postgres; the API rejects Key-Value Store remotes with a 404, so the SDK now fails fast with InvalidLinkRemoteError instead. - Document that connectionPooling.attach can reject after the attachment exists (onAttached throw or abort), so callers shouldn't blindly retry.
…eral An add-on can have namespaced attachments on the same app alongside its default one (credential:<name>, connection-pooling:default). heroku-cli-util kept only un-namespaced matches when a resolve returned several; the SDK reported them as ambiguous, so resolving such a database by add-on name failed (e.g. pg:connection-pooling:attach <addon> after a pool exists). singularize now narrows multiple matches to the default attachments when any exist, and stays ambiguous otherwise.
ebf3326 to
8ff7eed
Compare
0.12.6 ships the link and connection pooling composites from heroku/heroku-sdk#184, replacing the temporary git branch dependency. W-23387136
…ch (#4046) * test(pg:links:create): add characterization cases * test(pg:links:destroy): add characterization cases * test(pg:links): add characterization cases * test(pg:connection-pooling:attach): add characterization cases * refactor: use @heroku/sdk for pg:links:create command * test(pg:links:create): stub @heroku/sdk directly, drop nock * refactor: remove unused src/lib/addons/resolve.ts pg:links:create was its last consumer; add-on resolution now happens in the SDK. * refactor(pg:links): share the SDK-to-CLI link error mapping * refactor: use @heroku/sdk for pg:links:destroy command * test(pg:links:destroy): stub @heroku/sdk directly, drop nock * refactor: use @heroku/sdk for pg:links command * test(pg:links): stub @heroku/sdk directly, drop nock * refactor: use @heroku/sdk for pg:connection-pooling:attach command * test(pg:connection-pooling:attach): stub @heroku/sdk directly, drop nock * docs: note pg:links and pg:connection-pooling:attach behavior changes in V12-CHANGES * fix(pg:links:create): accept only Heroku Postgres remotes Heroku Data Links connects Postgres to Postgres; the API rejects Key-Value Store remotes with a 404. The SDK now rejects them up front with InvalidLinkRemoteError, so the CLI error and command examples drop the heroku-redis remote, and V12-CHANGES records the change. * fix(pg:links:create): show the SDK's invalid-remote message Pass InvalidLinkRemoteError through instead of replacing it with a CLI-written string. The SDK message names the remote and says Data Links only connect Heroku Postgres databases. Also drop a stale comment in pg:links about the pre-SDK response shape, and give the --as create test a distinct remote so it catches a swapped spinner label. * docs: clarify pg:links V12-CHANGES entries after review * deps: point @heroku/sdk at the heroku-sdk#184 branch Temporary until the link and connection pooling composites are released. Main already published 0.12.5 without them, so they will ship in 0.12.6. W-23387136 * deps: bump @heroku/sdk to ^0.12.6 0.12.6 ships the link and connection pooling composites from heroku/heroku-sdk#184, replacing the temporary git branch dependency. W-23387136
Summary
Adds SDK composites so the v12 CLI can move
pg:links,pg:links:create,pg:links:destroy, andpg:connection-pooling:attachto@heroku/sdkwithout resolving databases itself. The CLI passes rawapp/database/remoteinput. These methods resolve internally with the privateresolvePgDatabase, the same waydata.database.describe(app, database?)does today.New methods (the names don't reuse the generated
link.{list,create,delete,info}orconnectionPooling.{info,create}route names, so those routes stay callable):data.postgresDatabase.listLegacyByApp(app)attachmentNames, in API order. Includes Essential-tier databases and databases owned by other apps. MirrorsDatabaseResolver.getAllLegacyDatabases.data.link.listByApp(app, database?)database, resolves that one database. Without it, useslistLegacyByApp. Then fetches each database's links in parallel. Returns[]when the app has no databases.data.link.createForDatabase(app, database, remote, {as}, {beforeCreate})InvalidLinkRemoteError. Essential tier is rejected on either side. Then runsbeforeCreate({addon, target}), followed bylink.create(addon.id, {as, target}).data.link.deleteFromDatabase(app, database, link, {beforeDelete})beforeDelete({addon, link}), where the CLI prompts for confirmation; a throw or abort there prevents the delete. Then callslink.delete.data.connectionPooling.attach(app, database?, {name, credential}, {onResolved, onAttached})DATABASE_URL) and rejects Essential tier. RunsonResolved(addon), thenconnectionPooling.create(addon.name, {app, name, credential})(credentialdefaults todefault), thenonAttached(attachment). Then reads the newest release using aRangeheader, so no pagination. This lookup is best-effort: if it fails after the pool is attached, the call returnsreleaseVersion: undefinedinstead of rejecting. An abort still rejects.Hook naming:
before<Verb>hooks take a preview object and act as a gate, where a throw prevents the mutation.onResolved/on<PastTense>hooks are notifications.Errors and types exported from
@heroku/sdk/extensions/data:EssentialDatabaseNotSupportedError(carries the add-on; modeled onNotAdvancedDatabaseError) andInvalidLinkRemoteError(carries the remote add-on). Both have abodygetter.PgDatabaseNotFoundErrorandResolvedPgDatabase, so callers can match not-found errors without importing internals.Neither message mentions CLI commands. The CLI maps them to its existing strings.
createAttachmentfix: when the app-scoped resolve matched only non-Postgres add-ons,resolveAddonInternalthrewAddonNotFoundErrorinstead ofNotFoundError, so the global retry never ran. The scoped-then-global logic now lives in a single private helper,resolveAddonScopedThenGlobal, whichcreateAttachmentandlink.createForDatabaseboth use. The helper retries only onAddonNotFoundErroror a 404 labeledresource: 'add_on'. Any other 404, whether for the app or without a label, is rethrown so a mistyped app doesn't slip through. It resolves anapp::addonidentity in one request.Attachment resolution fix: when an attachment lookup matches several attachments,
resolveAddonAttachmentnow keeps only the default (un-namespaced) ones, asheroku-cli-utildid. Previously, resolving a database by add-on name failed as ambiguous when the database also had a credential (credential:<name>) or connection-pooling attachment on the same app. If none of the matches is a default attachment, the error is still ambiguous. This applies to every command that resolves databases through the SDK, not only the new composites.Open question: should these composites also reject Advanced databases? Right now they rely on the API to do that.
resolvePgDatabasestays private:@heroku/sdk/resources/data/internal/*still resolves toERR_PACKAGE_PATH_NOT_EXPORTED.Type of Change
Breaking Changes (major semver update)
!after your change type to denote a change that breaks current behaviorFeature Additions (minor semver update)
Patch Updates (patch semver update)
Testing
Notes:
You need an app with at least two Postgres databases, at least one non-Essential, plus a Heroku Key-Value Store add-on for the rejection check. You also need a second app that owns a non-Essential Postgres add-on. Build the branch (
npm run build), then run the snippets below withHEROKU_API_KEYset.Steps:
await sdk.data.postgresDatabase.listLegacyByApp('<app>'). Each database should appear once, with all its attachment names. Advanced databases shouldn't appear.await sdk.data.link.listByApp('<app>')andawait sdk.data.link.listByApp('<app>', 'DATABASE_URL'). The results should matchheroku pg:links -a <app>(v11).await sdk.data.link.createForDatabase('<app>', 'DATABASE_URL', '<other-postgres-addon-name>', {as: 'sdk_test'}). Confirm withheroku pg:links -a <app>that the link exists.InvalidLinkRemoteErrorbefore any link request is sent.'<app>-typo') and another-app::DATABASEdatabase. It should reject with a 404 for the app and must not create a link on the remote.EssentialDatabaseNotSupportedErrorand no link should be created.await sdk.data.link.deleteFromDatabase('<app>', 'DATABASE_URL', 'sdk_test', {beforeDelete: () => { throw new Error('abort') }}). It should throwabortand leave the link in place. Then run it again withoutbeforeDelete; the link should be gone.await sdk.data.connectionPooling.attach('<app>', undefined, {name: 'SDK_POOL'}). The attachment should be namedSDK_POOL, andreleaseVersionshould equal the version shown byheroku releases -a <app> -n 1. Detach the pool before re-running: a rejection afteronAttachedleaves the attachment in place.SDK_POOLis still attached, runawait sdk.data.link.listByApp('<app>', '<database-addon-name>')using the pooled database's add-on name. It should resolve to the default attachment instead of throwingAddonAttachmentAmbiguousError. Then detachSDK_POOL.sdk.data.link.listandsdk.data.connectionPooling.infostill work, which confirms the generated routes weren't replaced.Screenshots (if applicable)
Related Issues
GitHub issue: N/A
GUS work item: W-23387136