Pr 128 #143
Pr 128 #143
Conversation
- Update response types for various endpoints - Add new endpoints for user identities management
Fix the singularization bug that was generating incorrect method names: - create_user_identitie → create_user_identity - get_identitie → get_identity - update_identitie → update_identity - delete_identitie → delete_identity Changes: - Add _singularize_resource() helper method to centralize singularization logic - Handle special cases: user_identities → user_identity, identities → identity - Update both _generate_methods() and _create_api_method() to use the helper - Maintains existing behavior for other resources (users, organizations, etc.) This fixes a pre-existing bug that was exposed by PR kinde-oss#128's addition of user_identities and identities endpoints.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughPluralized many ManagementClient API endpoints, added a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ManagementClient
participant Generator as _generate_methods()
participant Singularizer as _singularize_resource()
participant APIMethod as generated API method
participant API as Kinde API
Client->>Generator: initialize -> generate dynamic methods
Generator->>Singularizer: singularize(resource_name)
Singularizer-->>Generator: singular resource name
Generator->>APIMethod: create API caller (path, verb, response mapping)
Client->>APIMethod: call create_feature_flag(key,type,value)
APIMethod->>API: POST /feature_flags with body {key,type,value}
API-->>APIMethod: 200 + payload
APIMethod-->>Client: return mapped response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Update test assertions to reflect removed endpoints: - Remove get_permission assertion (permissions no longer has get endpoint) - Remove get_feature_flags and get_feature_flag assertions (removed in PR kinde-oss#128) - Remove update_api_application assertion (removed in PR kinde-oss#128) - Update test_feature_flags_api_calls to test create_feature_flag instead These changes align tests with the updated Management API endpoints introduced in PR kinde-oss#128.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
kinde_sdk/management/management_client.py (2)
387-408: Incomplete handling of-iesplural forms.The method correctly handles
user_identitiesandidentities, but other resources ending in-ieswould produce incorrect singular forms:
industries→industrie(should beindustry)properties→propertie(should beproperty)Currently these resources only have
listactions so no singular methods are generated, but this could break if singular actions are added later.Consider a more robust approach:
def _singularize_resource(self, resource: str) -> str: # Handle special cases if resource == 'business': return 'business' # Don't remove 's' from 'business' elif resource == 'user_identities': return 'user_identity' # Replace 'identities' with 'identity' elif resource == 'identities': return 'identity' # Replace 'identities' with 'identity' + # Handle words ending in 'ies' -> 'y' (e.g., properties -> property) + elif resource.endswith('ies'): + return resource[:-3] + 'y' # Default: remove trailing 's' if present elif resource.endswith('s'): return resource[:-1] else: return resource
474-480: Dead code: manual query string construction is unused.The
final_pathvariable constructed here is never used. The actual query parameter handling is done byparam_serializeat line 489, which receivesquery_paramsdirectly.Consider removing this unused code block:
- # 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}" - # Use param_serialize to get the proper URL with host
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kinde_sdk/management/management_client.py(11 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
kinde_sdk/management/management_client.py
[error] 1-1: Dynamic method generation missing 'get_permission' method. The ManagementClient should expose a 'get_permission' method for permissions API.
[error] 1-1: ManagementClient does not implement get_feature_flags; only has create_feature_flag as per tests.
🔇 Additional comments (3)
kinde_sdk/management/management_client.py (3)
207-215: LGTM! New identity endpoints correctly added.The new
user_identitiesandidentitiesresources are properly structured with appropriate paths and HTTP methods. This addresses the PR objective of fixing the singularization bug for these endpoints.
348-356: LGTM!Response type mappings for the new identity endpoints are correctly structured with appropriate status codes (201 for create, 200 for others).
410-434: LGTM!The method generation logic correctly uses the centralized
_singularize_resourcehelper, ensuring consistent singular form derivation for all dynamically generated method names.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testv2/testv2_management/test_management_client.py (1)
491-533: Feature flag test now matches create‑via‑POST behaviorThe
test_feature_flags_api_callsupdate correctly models the newcreate_feature_flagflow: POST to/feature_flagswith a{"key", "type", "value"}body and a single-flag response object, and the test wiring (token, deserialize, assertion) is consistent with other tests in this file.If you want this test to guard more strongly against regressions, consider also asserting on
mock_api_client_instance.param_serialize.call_args(HTTP method, URL, and body) rather than justassert_called_once(), but that’s optional given the existing patterns.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testv2/testv2_management/test_management_client.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-04T07:09:21.430Z
Learnt from: brettchaldecott
Repo: kinde-oss/kinde-python-sdk PR: 83
File: kinde_sdk/auth/feature_flags.py:85-102
Timestamp: 2025-07-04T07:09:21.430Z
Learning: In the Kinde Python SDK v2, the maintainer (brettchaldecott) prefers to keep async keywords on methods in the feature flags module even when they don't currently perform await operations, as this approach has been tested and is preferred for the SDK's design.
Applied to files:
testv2/testv2_management/test_management_client.py
📚 Learning: 2025-08-21T04:46:50.368Z
Learnt from: KomanRudden
Repo: kinde-oss/kinde-python-sdk PR: 101
File: kinde_sdk/auth/permissions.py:6-7
Timestamp: 2025-08-21T04:46:50.368Z
Learning: The kinde_sdk has both frontend and management API modules. The frontend permissions_api.py exists at kinde_sdk/frontend/api/permissions_api.py and contains PermissionsApi class. Using the management API instead of the frontend API for user permissions would be incorrect.
Applied to files:
testv2/testv2_management/test_management_client.py
🔇 Additional comments (1)
testv2/testv2_management/test_management_client.py (1)
98-120: Comment updates correctly capture removed management methodsThe new notes about
get_permission,get_feature_flags/get_feature_flag, andupdate_api_applicationbeing removed align with the updated ManagementClient surface and help future readers understand why those methods aren’t asserted here. No functional changes introduced in this block; looks good as-is.
Add the missing 'list' endpoint to feature_flags resource to enable
get_feature_flags() method generation.
Changes:
- Add 'list': ('GET', '/api/v1/feature_flags') to feature_flags API_ENDPOINTS
- Add corresponding response type mapping: 'list': {'200': 'GetFeatureFlagsResponse', ...}
- Update test to assert get_feature_flags method exists
This addresses the CodeRabbit review comment on PR kinde-oss#143 and enables
the SDK to generate the get_feature_flags() method as documented in
the Kinde Management API.
…nse types - Corrected endpoint paths for organizations and API applications to use plural forms. - Updated response types for create endpoints to reflect correct HTTP status codes (201). - Adjusted test assertions to ensure alignment with the updated API structure. These changes enhance consistency with the Kinde Management API specifications.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
kinde_sdk/management/management_client.py (1)
77-83:permissionsstill lacks agetaction /get_permissionmappingThe
permissionsresource only defines list/create/update/delete and has no'get'entry in eitherAPI_ENDPOINTSorRESPONSE_TYPES. A prior review noted this was causing a missingget_permissionmethod and pipeline failures; that appears unchanged here. If your API and consumers expect aget_permissionmethod, you’ll still need to add:
"'get': ('GET', '/api/v1/permissions/{permission_id}'),"underAPI_ENDPOINTS['permissions']- A corresponding
'get'mapping underRESPONSE_TYPES['permissions']using the appropriate single‑permission response model.Also applies to: 258-263
🧹 Nitpick comments (1)
kinde_sdk/management/management_client.py (1)
174-178: Minor:update_businessdocstring suggests an ID param that the endpoint does not useFor the
businessresource,updateis defined asPATCH /api/v1/businesswith no path parameter. The generic docstring generator foraction == 'update'will still describe an ID argument (business_id), but the generated method accepts only*args, **kwargsand ignores any positional ID because the path has no{}placeholder. If you want the docs to match behavior, consider special‑casing this resource/action in the docstring generation or adjusting the wording to omit the ID parameter.Also applies to: 327-330
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kinde_sdk/management/management_client.py(12 hunks)testv2/testv2_management/test_management_client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testv2/testv2_management/test_management_client.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-21T04:46:50.368Z
Learnt from: KomanRudden
Repo: kinde-oss/kinde-python-sdk PR: 101
File: kinde_sdk/auth/permissions.py:6-7
Timestamp: 2025-08-21T04:46:50.368Z
Learning: The kinde_sdk has both frontend and management API modules. The frontend permissions_api.py exists at kinde_sdk/frontend/api/permissions_api.py and contains PermissionsApi class. Using the management API instead of the frontend API for user permissions would be incorrect.
Applied to files:
kinde_sdk/management/management_client.py
📚 Learning: 2025-07-04T07:09:21.430Z
Learnt from: brettchaldecott
Repo: kinde-oss/kinde-python-sdk PR: 83
File: kinde_sdk/auth/feature_flags.py:85-102
Timestamp: 2025-07-04T07:09:21.430Z
Learning: In the Kinde Python SDK v2, the maintainer (brettchaldecott) prefers to keep async keywords on methods in the feature flags module even when they don't currently perform await operations, as this approach has been tested and is preferred for the SDK's design.
Applied to files:
kinde_sdk/management/management_client.py
🔇 Additional comments (2)
kinde_sdk/management/management_client.py (2)
41-41: Pluralized create endpoints and new identity routes look coherent—please confirm against the Management API specThe shifts to plural resource paths (organizations, roles, permissions, feature_flags, applications, apis, subscribers, properties) and the new user_identities / identities routes are consistent with the rest of the mapping and should generate the expected method names (e.g.,
create_user_identity,get_identity,update_identity,delete_identity). Please double‑check these paths and verbs (especially the PATCH→PUT changes) against the live Management API docs so the generated client stays in sync with the server.Also applies to: 72-72, 80-80, 88-89, 97-97, 106-106, 114-114, 131-131, 177-178, 208-216
389-411: New_singularize_resourcehelper cleanly fixes theidentitiemethod-name bugUsing
_singularize_resourcewith explicit cases forbusiness,user_identities, andidentitiesensures that dynamic methods are now correctly named (create_user_identity,get_identity,update_identity,delete_identity) while leaving existing resources’ method names stable. Reusing the same helper in_generate_methodsand_create_api_methodkeeps docstrings and method names aligned. This directly addresses the prior singularization issue without introducing obvious regressions.Also applies to: 415-415, 451-451, 208-216, 354-357
Update roles.update response type mapping to use 200 instead of 201, as update operations return HTTP 200 OK, not 201 Created. This fixes silent deserialization failures where response_type would be None and callers would receive data=None instead of the actual response. Fixes issue identified in CodeRabbit review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@kinde_sdk/management/management_client.py`:
- Around line 389-410: The _singularize_resource function currently only strips
a trailing "s" and thus turns words ending in "ies" into incorrect forms (e.g.,
properties → propertie); update _singularize_resource to detect tokens that end
with "ies" (including compound names like "user_properties" and
"organization_properties") and convert that suffix to "y" (e.g., "properties" →
"property", "user_properties" → "user_property") before falling back to the
existing special cases and the single-letter "s" removal; keep the existing
special-case branches for 'business', 'user_identities', and 'identities' and
apply the "ies"→"y" rule prior to the generic .endswith('s') branch so functions
such as create_property / update_user_property are generated correctly.
- Around line 85-91: The feature_flags mapping uses the wrong HTTP verb for the
update operation: change the 'update' entry in the 'feature_flags' dict from
using 'PUT' to 'PATCH' so the ManagementClient will call PATCH for feature flag
updates (update in the 'feature_flags' mapping, the tuple's first element should
be 'PATCH' instead of 'PUT').
♻️ Duplicate comments (1)
kinde_sdk/management/management_client.py (1)
77-83: Missinggetendpoint for permissions.The
permissionsresource still lacks agetaction to retrieve a single permission by ID, which was previously flagged.
🧹 Nitpick comments (2)
kinde_sdk/management/management_client.py (2)
476-483: Manual query string construction bypasses URL encoding.The query string is built using simple string concatenation without URL encoding. Special characters in parameter values could break the URL or cause injection issues.
♻️ Suggested fix using urllib.parse.urlencode
+from urllib.parse import urlencode + # 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}" + query_string = urlencode({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}"
597-620: Dead code: backward compatibility loop does nothing.This loop iterates over method name pairs but only executes
pass. The comment indicates these methods are created dynamically, which happens in_generate_methods(). This entire block can be removed.🧹 Remove dead code
-# 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
- Change feature_flags.update HTTP method from PUT to PATCH to match API - Fix _singularize_resource to handle -ies endings correctly - properties -> property (was propertie) - user_properties -> user_property (was user_propertie) - organization_properties -> organization_property (was organization_propertie) Fixes issues identified in CodeRabbit review: - Critical: Update operations should use PATCH, not PUT - Major: Singularization fails for words ending in '-ies'
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kinde_sdk/management/management_client.py`:
- Around line 127-133: Update the Properties API mapping to use PATCH instead of
PUT for the update operation: change the 'properties' dict entry for 'update' in
management_client.py (the tuple currently ('PUT',
'/api/v1/properties/{property_id}')) to use the PATCH HTTP method so the mapping
becomes ('PATCH', '/api/v1/properties/{property_id}'), ensuring requests to the
update route use PATCH and avoid 405 errors.
♻️ Duplicate comments (2)
kinde_sdk/management/management_client.py (2)
220-257: Validate create/update status code mappings against actual API responses.
response_deserializeis status-code sensitive; a 200/201 mismatch will drop data toNone. Please confirm the create/update codes for these resources.Kinde Management API create/update endpoint response status codes (users, roles, permissions, feature flags, subscribers, apis, applications)Also applies to: 264-286
77-83: Add missingget_permissionmapping to avoid absent method generation.Without a
getaction here (and its response type), the dynamic client won’t generateget_permission, which is likely expected by callers.🛠️ Proposed fix
'permissions': { 'list': ('GET', '/api/v1/permissions'), + 'get': ('GET', '/api/v1/permissions/{permission_id}'), 'create': ('POST', '/api/v1/permissions'), 'update': ('PATCH', '/api/v1/permissions/{permission_id}'), 'delete': ('DELETE', '/api/v1/permissions/{permission_id}'), },'permissions': { 'list': {'200': 'GetPermissionsResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '429': 'ErrorResponse'}, + 'get': {'200': 'GetPermissionResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '404': 'ErrorResponse', '429': 'ErrorResponse'}, 'create': {'201': 'SuccessResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '429': 'ErrorResponse'}, 'update': {'200': 'SuccessResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '404': 'ErrorResponse', '429': 'ErrorResponse'}, 'delete': {'200': 'SuccessResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '404': 'ErrorResponse', '429': 'ErrorResponse'}, },Also applies to: 258-263
The official Kinde Management API documentation specifies PATCH for the properties update endpoint. Using PUT results in HTTP 405 Method Not Allowed errors. Fixes critical issue identified in CodeRabbit review.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
136-145: Changeuser_properties.updateandorganization_properties.updateHTTP methods from PUT to PATCH.The Kinde Management API documentation specifies
PATCH /api/v1/users/{user_id}/properties/{property_key}andPATCH /api/v1/organizations/{org_code}/properties/{property_key}for updating properties. Using PUT will cause HTTP errors. This is also inconsistent with the mainproperties.updateendpoint (line 131), which correctly uses PATCH.
🤖 Fix all issues with AI agents
In `@kinde_sdk/management/management_client.py`:
- Around line 354-358: The identities response mapping in management_client.py
is missing 404 handling for not-found cases; update the 'identities' entry for
the 'get', 'update', and 'delete' operations to include "'404': 'ErrorResponse'"
in each mapping so they match other resources (e.g., users.get, roles.get).
Locate the 'identities' dict (keys 'get', 'update', 'delete') and add the 404 ->
ErrorResponse mapping to each operation's response map.
♻️ Duplicate comments (1)
kinde_sdk/management/management_client.py (1)
77-83: Missinggetendpoint for permissions.The
permissionsresource lacks agetaction, soget_permissionmethod won't be generated. Add the missing endpoint:'permissions': { 'list': ('GET', '/api/v1/permissions'), + 'get': ('GET', '/api/v1/permissions/{permission_id}'), 'create': ('POST', '/api/v1/permissions'),Also add the corresponding entry in
RESPONSE_TYPES['permissions']:'permissions': { 'list': {'200': 'GetPermissionsResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '429': 'ErrorResponse'}, + 'get': {'200': 'GetPermissionResponse', '400': 'ErrorResponse', '403': 'ErrorResponse', '404': 'ErrorResponse', '429': 'ErrorResponse'}, 'create': {'201': 'SuccessResponse', ...
🧹 Nitpick comments (1)
kinde_sdk/management/management_client.py (1)
605-628: Dead code: backwards compatibility loop does nothing.This loop iterates over method name pairs but only executes
pass. The comment claims methods will be created dynamically, but they're already generated in_generate_methods(). This entire block is dead code.Either remove this section or implement actual backwards-compatibility aliases with deprecation warnings if needed:
🔧 Option 1: Remove dead code
-# 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'), - ... -]: - pass # These methods will be created dynamically🔧 Option 2: Implement deprecation aliases (if old method names differ)
import warnings def _create_deprecated_alias(cls, old_name: str, new_name: str): def deprecated_method(self, *args, **kwargs): warnings.warn( f"{old_name} is deprecated, use {new_name} instead", DeprecationWarning, stacklevel=2 ) return getattr(self, new_name)(*args, **kwargs) return deprecated_method
Add 404 error response mappings to identities.get, identities.update, and identities.delete to match the pattern used by other similar endpoints for proper not found scenario handling. Fixes minor issue identified in CodeRabbit review.
…s to PATCH Change user_properties.update and organization_properties.update HTTP methods from PUT to PATCH to match the Kinde Management API specification. This is also consistent with the main properties.update endpoint which uses PATCH. Using PUT causes HTTP errors. The API documentation specifies PATCH for these endpoints. Fixes issue identified in CodeRabbit review.
3 participants
Explain your changes
fix: correct singularization for user_identities and identities
Fix singularization bug that generated incorrect method names for
user_identities and identities endpoints. Add _singularize_resource()
helper to handle special cases and maintain backward compatibility.
Fixes: create_user_identitie → create_user_identity
get_identitie → get_identity
update_identitie → update_identity
delete_identitie → delete_identity
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.