-
Notifications
You must be signed in to change notification settings - Fork 25
Allow for same param name but different contextual usage #520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The param provided by users is `user_id`/ `group_id` / `directory_id`; this is expected as `user` / `group` / `directory` by the API. BUT, paginating through these endpoints requires the `*_id` version of these params, so we need to add some kind of translation to change the value, depending on context
Greptile SummaryThis PR fixes a parameter naming mismatch between the SDK and API by introducing a translation layer at the HTTP request boundary. The SDK uses Pythonic parameter names (
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DirectorySync
participant PrepareParams as _prepare_request_params()
participant HTTPClient
participant WorkOSAPI as WorkOS API
participant ListResource as WorkOSListResource
User->>DirectorySync: list_users(directory_id="dir_123")
DirectorySync->>DirectorySync: Create list_params with directory_id
DirectorySync->>PrepareParams: _prepare_request_params(list_params)
PrepareParams->>PrepareParams: Translate directory_id → directory
PrepareParams-->>DirectorySync: request_params with "directory"
DirectorySync->>HTTPClient: request(params=request_params)
HTTPClient->>WorkOSAPI: GET /directory_users?directory=dir_123
WorkOSAPI-->>HTTPClient: Response with users
HTTPClient-->>DirectorySync: response
DirectorySync->>ListResource: Create WorkOSListResource(list_args=list_params)
Note over ListResource: list_args contains directory_id for pagination
User->>ListResource: Iterate (pagination)
ListResource->>DirectorySync: list_users(directory_id="dir_123", after="cursor")
DirectorySync->>PrepareParams: _prepare_request_params(list_params)
PrepareParams-->>DirectorySync: request_params with "directory"
DirectorySync->>HTTPClient: request(params=request_params)
HTTPClient->>WorkOSAPI: GET /directory_users?directory=dir_123&after=cursor
WorkOSAPI-->>HTTPClient: Next page
HTTPClient-->>ListResource: response
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
Description
As #519 notes, #512 corrected one problem and introduced another.
The param provided by users through the SDK is
user_id/group_id/directory_id; the API accepts parameters asuser/group/directoryby the API, without the suffix. What I misunderstood was that pagination actually requires the*_idversion of these parameters, so while #512 fixed pagination (by using*_id), it went ahead and broke regular API calls (by using the incorrect, suffixless parameters).Makes sense? Yeah, I had to re-read everything a few times myself to understand the difference in usage. The key is in these lines (repeated throughout several API resources, I'm just picking one):
The request uses
list_params, and then the paginationWorkOSListResourceuseslist_paramsagain.To solve this, I added a function to translate the parameter name, depending on the context it's used. Essentially, this branch keeps the
*_idnames internally for pagination, then renames to the suffixless version them right before the HTTP call.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.