Skip to content

Conversation

@swaroopAkkineniWorkos
Copy link
Contributor

Description

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@linear
Copy link

linear bot commented Feb 11, 2026

ENT-4372 SDK Updates

@swaroopAkkineniWorkos swaroopAkkineniWorkos changed the title meep union of parentResourceExternalId & parentResourceTypeSlug: string; Feb 11, 2026
@swaroopAkkineniWorkos swaroopAkkineniWorkos changed the title union of parentResourceExternalId & parentResourceTypeSlug: string; union of parentResourceExternalId & parentResourceTypeSlug Feb 11, 2026
@swaroopAkkineniWorkos swaroopAkkineniWorkos marked this pull request as ready for review February 11, 2026 21:31
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested a review from a team as a code owner February 11, 2026 21:31
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested review from blairworkos and removed request for a team February 11, 2026 21:31
@swaroopAkkineniWorkos swaroopAkkineniWorkos merged commit 53d03ba into ENT-4372-resources-external Feb 11, 2026
8 checks passed
@swaroopAkkineniWorkos swaroopAkkineniWorkos deleted the ENT-4372-create-update branch February 11, 2026 21:31
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 11, 2026

Greptile Overview

Greptile Summary

This PR refactors the CreateAuthorizationResourceOptions type from a single interface with all-optional parent fields to a discriminated union that enforces mutual exclusivity between parentResourceId and (parentResourceExternalId + parentResourceTypeSlug). The serializer now uses the in operator for conditional field inclusion based on which union member is used.

Critical Issue: The union is missing a member for creating resources without any parent. Previously, all parent fields were optional, allowing top-level resources. The new type requires EITHER parentResourceId OR both parentResourceExternalId and parentResourceTypeSlug, but doesn't support omitting all parent fields. This breaks the ability to create root-level authorization resources.

Test Coverage: All updated tests now include parent resource fields. Tests that previously verified behavior when parent fields are omitted have been removed, and some test names are now misleading (e.g., "excludes description when omitted" but always includes parentResourceId).

Confidence Score: 1/5

  • This PR breaks the ability to create top-level authorization resources
  • The type system change removes support for creating resources without a parent, which was previously possible. This is a breaking change that will cause TypeScript compilation errors for any code creating top-level resources and removes critical functionality.
  • Pay close attention to src/authorization/interfaces/authorization-resource.interface.ts - the union type needs a third member to support resources without parents

Important Files Changed

Filename Overview
src/authorization/interfaces/authorization-resource.interface.ts Introduced discriminated union for parent resource options, but missing union member for resources without any parent, breaking ability to create top-level resources
src/authorization/serializers/create-authorization-resource-options.serializer.ts Updated serializer to use in operator checks for conditional field inclusion, correctly handles union type discrimination
src/authorization/authorization.spec.ts Tests updated to always include parent resource fields, removing coverage for top-level resources without parents and making some test names misleading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +44 to +46
export type CreateAuthorizationResourceOptions =
| CreateOptionsWithParentResourceId
| CreateOptionsWithParentExternalId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing union member for resources without a parent. The type currently requires EITHER parentResourceId OR parentResourceExternalId+parentResourceTypeSlug, but doesn't allow creating top-level resources (no parent at all). Add a third union member with just BaseCreateAuthorizationResourceOptions to support this case.

Suggested change
export type CreateAuthorizationResourceOptions =
| CreateOptionsWithParentResourceId
| CreateOptionsWithParentExternalId;
export type CreateAuthorizationResourceOptions =
| BaseCreateAuthorizationResourceOptions
| CreateOptionsWithParentResourceId
| CreateOptionsWithParentExternalId;

Comment on lines +776 to 790
it('excludes description when omitted', async () => {
fetchOnce(authorizationResourceFixture, { status: 201 });

await workos.authorization.createResource({
organizationId: testOrgId,
resourceTypeSlug: 'document',
externalId: 'doc-456',
name: 'Q4 Budget Report',
parentResourceId: 'resource_01HXYZ',
});

const body = fetchBody();
expect(body).not.toHaveProperty('description');
expect(body).not.toHaveProperty('parent_resource_id');
expect(body).toHaveProperty('parent_resource_id', 'resource_01HXYZ');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Test updated to always include parentResourceId, but the test name says "excludes description when omitted". This doesn't test the scenario where parent fields are actually omitted (which would be a top-level resource). Either restore the original test case or update the name to reflect what's actually being tested.

Comment on lines 792 to 812
@@ -822,33 +804,12 @@ describe('Authorization', () => {
externalId: 'doc-456',
name: 'Q4 Budget Report',
description: null,
parentResourceId: 'resource_01HXYZ',
});

const body = fetchBody();
expect(body).toHaveProperty('description', null);
expect(body).not.toHaveProperty('parent_resource_id');
});

it('sends null when parentResourceId is explicitly set to null', async () => {
fetchOnce(
{
...authorizationResourceFixture,
parent_resource_id: null,
},
{ status: 201 },
);

await workos.authorization.createResource({
organizationId: testOrgId,
resourceTypeSlug: 'document',
externalId: 'doc-456',
name: 'Q4 Budget Report',
parentResourceId: null,
});

const body = fetchBody();
expect(body).toHaveProperty('parent_resource_id', null);
expect(body).not.toHaveProperty('description');
expect(body).toHaveProperty('parent_resource_id', 'resource_01HXYZ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Test now always sets parentResourceId, making the test name misleading. The test says it "sends null when description is explicitly set to null" but doesn't verify behavior when parentResourceId is omitted or null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant