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 12, 2026

ENT-4372 SDK Updates

@swaroopAkkineniWorkos swaroopAkkineniWorkos changed the title moar refactored to breakout resourceId and resourceExternalId into separate itnerfaces that can be exported Feb 12, 2026
@swaroopAkkineniWorkos swaroopAkkineniWorkos marked this pull request as ready for review February 12, 2026 15:35
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested a review from a team as a code owner February 12, 2026 15:35
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested review from faroceann and removed request for a team February 12, 2026 15:35
@swaroopAkkineniWorkos swaroopAkkineniWorkos merged commit 1ffafc4 into ENT-4372-base-authorization-branch Feb 12, 2026
8 checks passed
@swaroopAkkineniWorkos swaroopAkkineniWorkos deleted the ENT-4372-option-ResourceId-Input branch February 12, 2026 15:35
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 12, 2026

Greptile Overview

Greptile Summary

This PR refactors the authorization check interfaces to use discriminated union types, breaking out resourceId and resourceExternalId into separate interfaces (AuthorizationResourceIdentifierById and AuthorizationResourceIdentifierByExternalId). This improves type safety by making it clear that these are mutually exclusive ways to identify a resource.

Key changes:

  • Created new authorization-resource-identifier.interface.ts with two distinct interfaces
  • Refactored AuthorizationCheckOptions to be a union type of two specific option types
  • Updated serializer to use in operator checks instead of truthy value checks
  • Added comprehensive test coverage for both resource identification methods
  • Fixed test descriptions for clarity

Note: The PR title contains a typo: "itnerfaces" should be "interfaces"

Confidence Score: 3/5

  • Safe to merge with one critical bug fix needed in the serializer
  • The refactoring improves type safety and test coverage is good, but the serializer has a logical flaw where runtime objects with both resourceId and resourceExternalId properties would serialize both fields instead of being mutually exclusive
  • Pay close attention to src/authorization/serializers/authorization-check-options.serializer.ts - the serialization logic needs fixing to ensure mutual exclusivity

Important Files Changed

Filename Overview
src/authorization/interfaces/authorization-resource-identifier.interface.ts New file that extracts resource identifier types into separate interfaces for better type safety
src/authorization/interfaces/authorization-resource-check.interface.ts Refactored to use discriminated union types for mutually exclusive resource identification methods
src/authorization/serializers/authorization-check-options.serializer.ts Updated serialization logic with in operator checks, but has a potential bug when both fields are present

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.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +11 to 14
...('resourceExternalId' in options && {
resource_external_id: options.resourceExternalId,
}),
...(options.resourceTypeSlug && {
resource_type_slug: options.resourceTypeSlug,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

The serialization logic will include both resource_id and resource_external_id/resource_type_slug if an object with all properties is passed at runtime. TypeScript types don't prevent { resourceId: 'x', resourceExternalId: 'y', resourceTypeSlug: 'z' } from being constructed and passed.

Consider using an else-if pattern to ensure mutual exclusivity:

Suggested change
...('resourceExternalId' in options && {
resource_external_id: options.resourceExternalId,
}),
...(options.resourceTypeSlug && {
resource_type_slug: options.resourceTypeSlug,
}),
...('resourceId' in options
? { resource_id: options.resourceId }
: {
resource_external_id: options.resourceExternalId,
resource_type_slug: options.resourceTypeSlug,
}),

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