-
Notifications
You must be signed in to change notification settings - Fork 23
feat: enhance type definitions and extend user identities support #128
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
base: main
Are you sure you want to change the base?
Conversation
- Update response types for various endpoints - Add new endpoints for user identities management
WalkthroughThe pull request reorganizes API endpoints and response mappings in the ManagementClient, updating paths for existing resources (organizations, feature flags, properties, connected apps, api applications), adding new identity-related endpoints (user_identities, identities), and adjusting HTTP status code mappings across multiple resources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: Single-file changes with systematic, repetitive endpoint and response mapping updates across many resources. Review requires careful verification that all path/method/status-code changes align with API specifications and that new endpoints integrate correctly without breaking existing functionality. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kinde_sdk/management/management_client.py (1)
390-396: Fix singularization: identities/user_identities generate wrong method names (…_identitie).Naive
resource[:-1]yieldsidentitie; callers will getcreate_user_identitie,get_identitie, etc. This is a breaking DX bug.Apply this diff in both places to handle irregular plurals:
- if resource == 'business': - resource_singular = 'business' # Don't remove 's' from 'business' - else: - resource_singular = resource[:-1] if resource.endswith('s') else resource + irregular = { + 'business': 'business', + 'identities': 'identity', + 'user_identities': 'user_identity', + } + resource_singular = irregular.get(resource, resource[:-1] if resource.endswith('s') else resource)Repeat the same change in
_create_api_methodsingularization block.Also applies to: 430-435
🧹 Nitpick comments (7)
kinde_sdk/management/management_client.py (7)
437-446: Support named path params and validate missing placeholders.Current logic only consumes positional
*argsand silently leaves{}if not enough args. Prefer named kwargs first, then args; error if missing.Apply:
- formatted_path = path - if '{' in path and args: - param_values = list(args) - while '{' in formatted_path and param_values: - start_idx = formatted_path.find('{') - end_idx = formatted_path.find('}') - if start_idx >= 0 and end_idx >= 0: - formatted_path = formatted_path[:start_idx] + str(param_values.pop(0)) + formatted_path[end_idx + 1:] + formatted_path = path + # Fill placeholders from kwargs first, then from args; validate all placeholders resolved + arg_values = list(args) + while True: + start_idx = formatted_path.find('{') + end_idx = formatted_path.find('}', start_idx + 1) + if start_idx == -1 or end_idx == -1: + break + key = formatted_path[start_idx + 1:end_idx] + if key in kwargs: + value = kwargs.pop(key) + elif arg_values: + value = arg_values.pop(0) + else: + raise ValueError(f"Missing value for path parameter '{key}'") + formatted_path = f"{formatted_path[:start_idx]}{value}{formatted_path[end_idx + 1:]}"
458-466: Remove dead manual query-string assembly; rely on ApiClient.param_serialize.
final_pathand string-join block are unused;param_serializebuilds the URL withquery_paramsalready.Apply:
- # FIXED: Use param_serialize to properly construct the full URL with host - # Handle query parameters by appending them to the path - final_path = formatted_path - if query_params and http_method in ('GET', 'DELETE'): - query_string = '&'.join([f"{k}={v}" for k, v in query_params.items() if v is not None]) - if query_string: - separator = '&' if '?' in final_path else '?' - final_path = f"{final_path}{separator}{query_string}" + # Rely on ApiClient.param_serialize for full URL and query encoding
521-532: Docstring for GET should reflect query_keys when no path params (e.g., users.get expects ?id=).Currently shows
{resource}_id, which is misleading for endpoints like/api/v1/user.Apply:
- elif action == 'get': - param_name = path.split('{')[-1].split('}')[0] if '{' in path else f"{resource_singular}_id" + elif action == 'get': + if '{' in path: + param_name = path.split('{')[-1].split('}')[0] + elif query_keys: + param_name = " | ".join(query_keys) + else: + param_name = "query parameters" docstring = f""" Get a {resource_singular} by ID. Args: {param_name}: The ID of the {resource_singular} to get.Optionally, declare
query_keysfor the users GET endpoint to make this explicit (see separate suggestion).
31-33: Declare query_keys for users.get to document theidquery parameter.This aligns docs and usage.
Apply:
- 'get': ('GET', '/api/v1/user'), + 'get': ('GET', '/api/v1/user', ['id']),
253-255: roles.update response code mapped to 201; include 200 to avoid deserialization errors.Updates typically return 200/204. Including 200 is safer across environments.
Apply:
- 'update': {'201': 'SuccessResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '404': 'ErrorResponse', '429': 'ErrorResponse'}, + 'update': {'200': 'SuccessResponse', '201': 'SuccessResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '404': 'ErrorResponse', '429': 'ErrorResponse'},Please confirm the latest Kinde API status code for PATCH /api/v1/roles/{role_id}.
359-376: Add optional request timeout to avoid hanging requests.All network calls pass
_request_timeout=None. Provide a client-level default and forward it.Apply:
-def __init__(self, domain: str, client_id: str, client_secret: str): +def __init__(self, domain: str, client_id: str, client_secret: str, request_timeout: Optional[Union[float, tuple]] = None): @@ - self.token_manager = ManagementTokenManager(domain, client_id, client_secret) + self.token_manager = ManagementTokenManager(domain, client_id, client_secret) + self.request_timeout = request_timeout- _request_timeout=None + _request_timeout=self.request_timeoutOptionally, surface a per-call override via a special kwarg (e.g.,
_timeout) you pop fromkwargs.Also applies to: 495-496
580-603: Remove dead “backwards compatibility methods” block.The loop is a no-op (
pass) and doesn’t alias anything. Safe to delete or implement actual aliases with deprecation warnings.Apply:
-# Add backwards compatibility methods for common operations -# These will be deprecated in future versions -for method_name, new_name in [ - ('get_users', 'get_users'), - ('get_user', 'get_user'), - ('create_user', 'create_user'), - ('update_user', 'update_user'), - ('delete_user', 'delete_user'), - ('get_organizations', 'get_organizations'), - ('get_organization', 'get_organization'), - ('create_organization', 'create_organization'), - ('update_organization', 'update_organization'), - ('delete_organization', 'delete_organization'), - ('get_roles', 'get_roles'), - ('get_role', 'get_role'), - ('create_role', 'create_role'), - ('update_role', 'update_role'), - ('delete_role', 'delete_role'), - ('get_feature_flags', 'get_feature_flags'), - ('create_feature_flag', 'create_feature_flag'), - ('update_feature_flag', 'update_feature_flag'), - ('delete_feature_flag', 'delete_feature_flag'), -]: - pass # These methods will be created dynamically +# (removed dead back-compat placeholder)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kinde_sdk/management/management_client.py(9 hunks)
🔇 Additional comments (3)
kinde_sdk/management/management_client.py (3)
207-215: LGTM on adding identities/user_identities endpoints and response maps.Paths and type names look consistent with the new resources. Singularization fix above will ensure method names are ergonomic.
If the API returns 204 on delete, consider adding '204': 'SuccessResponse' to
identities.delete.Also applies to: 348-356
42-44: Code is correct—no changes needed.Kinde Management API uses PATCH for updating organizations at the endpoint PATCH /api/v1/organization/{org_code}, which matches the implementation in the code. The PR summary's reference to "PUT where required" does not apply to this endpoint.
223-224: The review comment recommendation is incorrect—Kinde create endpoints return 200 OK, not 201.Official Kinde documentation confirms all four create endpoints (POST /api/v1/user, POST /api/v1/organization, POST /api/v1/webhooks, and POST /api/v1/apis) return 200 OK as the success response, not 201. The suggested diff additions for 201 status codes would be unnecessary and potentially incorrect, as the API never returns this status code. No changes are required.
Likely an incorrect or invalid review comment.
Explain your changes
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.