Skip to content

feat: step6 resource by ext-id#518

Open
swaroopAkkineniWorkos wants to merge 7 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step6-resources-by-ext-id
Open

feat: step6 resource by ext-id#518
swaroopAkkineniWorkos wants to merge 7 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step6-resources-by-ext-id

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 Mar 6, 2026

@swaroopAkkineniWorkos swaroopAkkineniWorkos changed the title feat: step6 resource by id feat: step6 resource by ext-id Mar 6, 2026
@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile review

@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile re-review

Swaroop Akkineni added 3 commits March 16, 2026 11:38
@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile re-review

@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile re-review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR implements three previously-stubbed Client methods in the authorization package — GetResourceByExternalId, UpdateResourceByExternalId, and DeleteResourceByExternalId — replacing their errors.New("not implemented") stubs with real HTTP logic. It also introduces comprehensive test coverage across two new test files.

Key changes:

  • GetResourceByExternalId: Issues a GET to /authorization/organizations/{org_id}/resources/{type_slug}/{external_id}, decoding the response into an AuthorizationResource.
  • UpdateResourceByExternalId: Issues a PATCH to the same URL pattern with a JSON body derived from UpdateResourceByExternalIdOpts (using *string for optional fields, omitempty only on Name, matching the pattern established by existing opts types like UpdateAuthorizationResourceOpts).
  • DeleteResourceByExternalId: Issues a DELETE to the same URL pattern with an optional cascade_delete=true query parameter controlled by opts.CascadeDelete.
  • All three methods use url.PathEscape for path segment encoding, set Authorization/User-Agent headers consistently, propagate context, and delegate error checking to workos_errors.TryGetHTTPError.
  • Tests cover happy paths, auth-less error cases, cascade delete toggling, path construction, and URL encoding of special characters.

Confidence Score: 4/5

  • This PR is safe to merge with minor style improvements suggested.
  • The implementation is correct, consistent with existing patterns in the codebase, and well-tested. The only issues found are style-level: a misleading Content-Type header on body-less requests, a slightly weak URL-encoding assertion in tests, and unguarded global DefaultClient mutation in test setup. None of these affect production correctness.
  • No files require special attention; all issues flagged are style suggestions.

Important Files Changed

Filename Overview
pkg/authorization/client.go Implements three previously-stubbed methods (GetResourceByExternalId, UpdateResourceByExternalId, DeleteResourceByExternalId) using correct URL path construction with url.PathEscape, proper HTTP verbs, context propagation, and error handling via workos_errors. Minor style issue: Content-Type header is set on body-less GET and DELETE requests.
pkg/authorization/client_test.go New comprehensive test file covering happy paths, auth errors, cascade delete, path construction, and URL encoding for all three new client methods. URL encoding assertion for special characters is slightly under-specified.
pkg/authorization/authorization_test.go New test file covering the package-level DefaultClient delegation functions. Mutates the global DefaultClient across tests without cleanup, which could cause fragile ordering-dependent failures if future tests are added without resetting DefaultClient first.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Client
    participant WorkOSAPI as WorkOS API

    Note over Caller,WorkOSAPI: GetResourceByExternalId
    Caller->>Client: GetResourceByExternalId(ctx, {OrgId, TypeSlug, ExternalId})
    Client->>WorkOSAPI: GET /authorization/organizations/{org_id}/resources/{type_slug}/{external_id}
    WorkOSAPI-->>Client: 200 AuthorizationResource JSON
    Client-->>Caller: AuthorizationResource, nil

    Note over Caller,WorkOSAPI: UpdateResourceByExternalId
    Caller->>Client: UpdateResourceByExternalId(ctx, {OrgId, TypeSlug, ExternalId, Name, Description})
    Client->>WorkOSAPI: PATCH /authorization/organizations/{org_id}/resources/{type_slug}/{external_id} + JSON body
    WorkOSAPI-->>Client: 200 AuthorizationResource JSON
    Client-->>Caller: AuthorizationResource, nil

    Note over Caller,WorkOSAPI: DeleteResourceByExternalId
    Caller->>Client: DeleteResourceByExternalId(ctx, {OrgId, TypeSlug, ExternalId, CascadeDelete})
    Client->>WorkOSAPI: DELETE /authorization/organizations/{org_id}/resources/{type_slug}/{external_id}[?cascade_delete=true]
    WorkOSAPI-->>Client: 204 No Content
    Client-->>Caller: nil
Loading

Comments Outside Diff (3)

  1. pkg/authorization/client.go, line 660-663 (link)

    Content-Type header on body-less GET request

    Content-Type: application/json is set on the GET request in GetResourceByExternalId, which carries no request body. While harmless, it is semantically misleading and could confuse some intermediaries. The same pattern is repeated for the DELETE request in DeleteResourceByExternalId (line 744).

  2. pkg/authorization/client_test.go, line 394-403 (link)

    Weak URL-encoding assertion for special characters

    The test only checks that the raw URI contains the beginning of the path and that %2F appears somewhere in it, but doesn't verify the full encoded form of "ext/with spaces" (i.e. ext%2Fwith%20spaces). A tighter assertion would catch regressions where the space or other characters are not properly encoded.

  3. pkg/authorization/authorization_test.go, line 13-16 (link)

    Global DefaultClient mutation without cleanup

    TestSetAPIKey replaces DefaultClient with a bare &Client{} (no Endpoint, no HTTPClient) and never restores the original. Go runs test functions within a package sequentially, so each subsequent test in this file overrides DefaultClient before using it — but this tight coupling on ordering is fragile. If a future test is added that calls a DefaultClient method without first resetting it, it will hit a zero-valued client and produce a confusing failure.

    Consider deferring a restore of the original:

    func TestSetAPIKey(t *testing.T) {
        original := DefaultClient
        defer func() { DefaultClient = original }()
    
        DefaultClient = &Client{}
        SetAPIKey("test-key-123")
        require.Equal(t, "test-key-123", DefaultClient.APIKey)
    }

Last reviewed commit: 78094fd

@swaroopAkkineniWorkos swaroopAkkineniWorkos marked this pull request as ready for review March 16, 2026 21:00
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested a review from a team as a code owner March 16, 2026 21:00
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested review from gjtorikian and removed request for a team March 16, 2026 21:00
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