Uh oh!
There was an error while loading. Please reload this page.
fix(backend): Align EnterpriseConnection resource with the Backend API response - #9156
Conversation
…onParams The Backend API validates provider as required on enterprise connection creation, so calls without it type-checked but failed at runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntract Reuses the OrganizationEnterpriseConnectionProvider union from @clerk/shared/types (matching the sibling SamlConnectionApi pattern) and adds type-level tests so provider can't silently become optional or widen back to string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ef1c9ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
…ctions Stacked on #9153 (required provider field). Aligns the remaining CreateEnterpriseConnectionParams gaps with the Backend API contract, which validates name and domains (min 1) as required. Deprecates syncUserAttributes on create and provider on update, since the Backend API ignores both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the optional create/update params the Backend API supports but the SDK types omitted: allowOrganizationAccountLinking, customAttributes, authenticatable, disableJitProvisioning, disableAdditionalIdentifications (update only), and saml.loginHint. Verified against BAPI's CreateParams/UpdateParams/SAMLParams in clerk_go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ace6b49 to
7b6b5c0Compare1529981 to
d4534fcCompareCodex review: BAPI requires login_hint.source exactly when mode is custom_attribute and rejects it otherwise; model as a discriminated union. Also lock name/domains as required with type-level tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sponse The resource classes and JSON types drifted from BAPI's serializer (clerk_go api/serialize/enterprise_connection.go): several properties read fields BAPI never sends, and several returned fields were missing. New constructor params are appended to preserve positional compatibility. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d4534fc to
9d3a99eCompare…d bump to minor Codex review: BAPI's serializer always sends login_hint (no omitempty, never nil), so the JSON type and resource property are non-nullable. Changeset bumped to minor to match repo precedent for new resource properties (e.g. hasImage). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-google-signin@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
API Changes Report
Summary
🔴 Breaking changes index (18)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/backendCurrent version: 3.13.1 🔴 Breaking Changes (18)Changed: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/api/__tests__/EnterpriseConnectionApi.test.ts`:
- Around line 350-386: Add a separate test case in the EnterpriseConnectionApi
response tests using a fixture that omits the optional SAML/OAuth fields and
custom_attributes. Assert the normalized resource exposes undefined for each
omitted optional property, including the relevant fields on samlConnection and
oauthConfig, to preserve serializer-omission behavior.
In `@packages/backend/src/api/resources/EnterpriseConnection.ts`:
- Around line 253-276: Update the public EnterpriseConnection constructors at
packages/backend/src/api/resources/EnterpriseConnection.ts lines 94-99, 159-178,
and 253-276 so the newly added parameters are backward-compatible, or classify
the API change as major by changing .changeset/major-poets-refuse.md line 2 to
major; ensure all affected constructor signatures follow the same choice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 104cd8a9-6a61-4602-875c-723186eda94d
📒 Files selected for processing (5)
.changeset/major-poets-refuse.mdpackages/backend/src/api/__tests__/EnterpriseConnectionApi.test.tspackages/backend/src/api/resources/EnterpriseConnection.tspackages/backend/src/api/resources/JSON.tspackages/backend/src/index.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)
| expect(response.provider).toBe('saml_custom'); | ||
| expect(response.logoPublicUrl).toBe('https://img.example.com/connection-logo.png'); | ||
| expect(response.domains).toEqual(['clerk.dev']); | ||
| expect(response.active).toBe(true); | ||
| // organization_id is omitted by the Backend API when unset and normalized to null | ||
| expect(response.organizationId).toBeNull(); | ||
| expect(response.allowOrganizationAccountLinking).toBe(true); | ||
| expect(response.authenticatable).toBe(true); | ||
| expect(response.disableJitProvisioning).toBe(false); | ||
| expect(response.customAttributes).toEqual([ | ||
| { | ||
| name: 'Employee Number', | ||
| key: 'employee_number', | ||
| ssoPath: 'user.employeeNumber', | ||
| scimPath: '', | ||
| multiValued: false, | ||
| }, | ||
| ]); | ||
| expect(response.samlConnection).not.toBeNull(); | ||
| expect(response.samlConnection?.id).toBe('samlc_1'); | ||
| expect(response.samlConnection?.idpEntityId).toBe('https://idp.example.com'); | ||
| expect(response.samlConnection?.idpCertificateIssuedAt).toBe(1672531200000); | ||
| expect(response.samlConnection?.idpCertificateExpiresAt).toBe(1704067200000); | ||
| expect(response.samlConnection?.active).toBe(true); | ||
| expect(response.samlConnection?.forceAuthn).toBe(false); | ||
| expect(response.samlConnection?.loginHint).toEqual({ | ||
| mode: 'custom_attribute', | ||
| source: 'employee_number', | ||
| }); | ||
| expect(response.oauthConfig).not.toBeNull(); | ||
| expect(response.oauthConfig?.providerKey).toBe('custom_oidc'); | ||
| expect(response.oauthConfig?.clientId).toBe('client_abc'); | ||
| expect(response.oauthConfig?.discoveryUrl).toBe('https://oauth.example.com/.well-known/openid-configuration'); | ||
| expect(response.oauthConfig?.authUrl).toBe('https://oauth.example.com/authorize'); | ||
| expect(response.oauthConfig?.tokenUrl).toBe('https://oauth.example.com/token'); | ||
| expect(response.oauthConfig?.userInfoUrl).toBe('https://oauth.example.com/userinfo'); | ||
| expect(response.oauthConfig?.requiresPkce).toBe(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover omitted optional response fields.
This fixture populates every newly optional SAML/OAuth field and custom_attributes. Add a case omitting them and assert the resource exposes undefined; this locks in the intended serializer-omission behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/api/__tests__/EnterpriseConnectionApi.test.ts` around
lines 350 - 386, Add a separate test case in the EnterpriseConnectionApi
response tests using a fixture that omits the optional SAML/OAuth fields and
custom_attributes. Assert the normalized resource exposes undefined for each
omitted optional property, including the relevant fields on samlConnection and
oauthConfig, to preserve serializer-omission behavior.
Source: Coding guidelines
| /** | ||
| * The identity provider (IdP) of the connection. For example, `'saml_custom'` or `'oidc_custom'`. | ||
| */ | ||
| readonly provider: string, | ||
| /** | ||
| * The public URL of the provider logo, if available. | ||
| */ | ||
| readonly logoPublicUrl: string | undefined, | ||
| /** | ||
| * Whether existing users who are members of the organization can link their account to their enterprise identity. | ||
| */ | ||
| readonly allowOrganizationAccountLinking: boolean, | ||
| /** | ||
| * Whether the connection can be used for sign-in and sign-up. | ||
| */ | ||
| readonly authenticatable: boolean, | ||
| /** | ||
| * Whether Just-in-Time (JIT) provisioning of users is disabled for the connection. | ||
| */ | ||
| readonly disableJitProvisioning: boolean, | ||
| /** | ||
| * The custom attribute mappings of the connection. Only returned when the custom attributes feature is enabled for the instance. | ||
| */ | ||
| readonly customAttributes: EnterpriseConnectionCustomAttribute[] | undefined, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n -C 4 '"version"|"exports"' packages/backend/package.json
ast-grep outline packages/backend/src/api/resources/EnterpriseConnection.ts --items all --type class
rg -n -C 3 '\bEnterpriseConnection(SamlConnection|OauthConfig)?\b' packages/backend/src/index.tsRepository: clerk/javascript
Length of output: 1546
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n## Constructor definitions\n'
sed -n '1,340p' packages/backend/src/api/resources/EnterpriseConnection.ts | nl -ba | sed -n '1,340p'printf'\n## Direct usages in repo\n'
rg -n 'new\s+EnterpriseConnection(SamlConnectionLoginHint|CustomAttribute|SamlConnection|OauthConfig|)\b' packages/backend/src ||trueprintf'\n## Package entry exports around backend index\n'
sed -n '120,190p' packages/backend/src/index.ts | nl -ba | sed -n '1,120p'printf'\n## Changeset\n'
sed -n '1,80p' .changeset/major-poets-refuse.md | nl -ba | sed -n '1,80p'Repository: clerk/javascript
Length of output: 222
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n## EnterpriseConnection.ts constructors\n'
grep -nE '^export class (EnterpriseConnectionSamlConnectionLoginHint|EnterpriseConnectionCustomAttribute|EnterpriseConnectionSamlConnection|EnterpriseConnectionOauthConfig|EnterpriseConnection)\b|^ constructor\(' packages/backend/src/api/resources/EnterpriseConnection.ts
printf'\n## Relevant source around constructors\n'
sed -n '1,340p' packages/backend/src/api/resources/EnterpriseConnection.ts | cat -n | sed -n '1,340p'printf'\n## Public exports from backend entry\n'
grep -nE 'EnterpriseConnection(SamlConnectionLoginHint|CustomAttribute|SamlConnection|OauthConfig)?' packages/backend/src/index.ts
printf'\n## Direct constructor usages in repo\n'
rg -n 'new\s+EnterpriseConnection(SamlConnectionLoginHint|CustomAttribute|SamlConnection|OauthConfig)?\b' packages/backend/src ||trueprintf'\n## Changeset\n'
cat -n .changeset/major-poets-refuse.mdRepository: clerk/javascript
Length of output: 15917
Bump @clerk/backend to a major release
These exported constructors are part of the public API, so adding required trailing parameters is a breaking change. Update .changeset/major-poets-refuse.md to major, or make the new arguments backward-compatible.
packages/backend/src/api/resources/EnterpriseConnection.ts.changeset/major-poets-refuse.md
📍 Affects 2 files
packages/backend/src/api/resources/EnterpriseConnection.ts#L253-L276(this comment)packages/backend/src/api/resources/EnterpriseConnection.ts#L94-L99packages/backend/src/api/resources/EnterpriseConnection.ts#L159-L178.changeset/major-poets-refuse.md#L2-L2
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/api/resources/EnterpriseConnection.ts` around lines 253
- 276, Update the public EnterpriseConnection constructors at
packages/backend/src/api/resources/EnterpriseConnection.ts lines 94-99, 159-178,
and 253-276 so the newly added parameters are backward-compatible, or classify
the API change as major by changing .changeset/major-poets-refuse.md line 2 to
major; ensure all affected constructor signatures follow the same choice.
Source: Coding guidelines
wobsoriano
left a comment
There was a problem hiding this comment.
Looks good!
Re | undefined question: The old types were wrong. Any downstream code that breaks on upgrade was already unsafe at runtime.
If there's a compiler error then it's just surfacing existing bugs rather than creating new ones. Minor bump is perfect!
Description
Stacked on #9155. The
EnterpriseConnectionresource classes and JSON types drifted from what BAPI's serializer returns (clerk_goapi/serialize/enterprise_connection.go). Some properties read fields BAPI never sends — typedstring/booleanbut alwaysundefinedat runtime — and several returned fields were missing from the SDK.What changed:
EnterpriseConnectionnow exposesprovider,logoPublicUrl,allowOrganizationAccountLinking,authenticatable,disableJitProvisioning, andcustomAttributes.EnterpriseConnectionSamlConnectionnow exposesactive,forceAuthn, andloginHint(required and non-null — BAPI's serializer always sends it).EnterpriseConnectionOauthConfignow exposesproviderKey,authUrl,tokenUrl,userInfoUrl, andrequiresPkce.allowSubdomains(only exists on the nested SAML connection) and nestedidpMetadata/syncUserAttributes. Removing them would break positional constructor callers, so they keep their slots and gain@deprecatednotices pointing at the real field.organizationIdis normalized tonullwhen BAPI omits the key, making the declaredstring | nulltrue at runtime.omitemptyin Go (oauthConfig.clientId, the SAMLidp_*fields, certificate timestamps) are retyped| undefinedto match runtime behavior.New constructor params are appended after existing ones. Adding required constructor params to resource classes in a non-major release follows repo precedent (#1544 added
hasImagemid-constructor as a minor); this ships as a minor for the same reason.One review call worth a second opinion: the
| undefinedretyping is a compile-time change for consumers with strict null checks. It converts silent runtimeundefineds into visible type errors, but it will surface in downstream builds on upgrade.Note
Review surfaced an upstream gap: BAPI's
EnterpriseConnectionResponseis missing theobjectdiscriminator its sibling serializers all send, so the SDK's deserializer (which switches onobject) currently returns raw snake_case JSON for enterprise connection responses in production —fromJSONonly runs in tests, where the mock suppliesobject. The mock keepsobjectdeliberately: it exercises the SDK path that the existingObjectType.EnterpriseConnectiondeserializer case defines, and matches the contract once clerk_go adds the field. The upstream fix is filed separately — ORGS-1765.To test:
pnpm --filter @clerk/backend testChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
🤖 Generated with Claude Code