feat: openapi dynamic class generation #159
Conversation
WalkthroughReplace the previous kinné SDK with a generated kinde_sdk.management OpenAPI client; add a config-driven SDK generator; ManagementClient now dynamically discovers APIs and injects Bearer tokens; remove large auto-generated schemas, several tests and docs; consolidate ignore files and bump generator version. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MG as ManagementClient
participant TM as ManagementTokenManager
participant AC as ApiClient
participant API as API_Class
participant S as External Service
Client->>MG: instantiate(domain, client_id, client_secret)
MG->>TM: initialize / obtain token
TM->>S: request token
S-->>TM: return bearer token
MG->>AC: create ApiClient(Configuration(base_url))
MG->>MG: _initialize_api_classes() -> instantiate APIs (e.g., UsersApi) -> attach as users_api
Client->>API: call users_api.some_method(...)
API->>AC: ApiClient.call_api(...)
AC->>AC: inject header Authorization: Bearer <token>
AC->>S: HTTP request with Authorization
S-->>AC: HTTP response
AC-->>API: parsed response
API-->>Client: return data
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
KomanRudden
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testv2/testv2_management/test_management_client.py (1)
694-735:⚠️ Potential issue | 🟠 MajorDynamic API test doesn’t exercise
ApiClient.call_api.
timezones_api.get_timezones()callsapi_client.call_api, but the mocked ApiClient doesn’t invokeparam_serialize, so the assertion onparam_serializeis likely to fail or be ineffective. Stubcall_apiand assert it was called.🛠️ Suggested fix
@@ - expected_response = {"timezones": ["UTC", "EST"]} + expected_response = {"timezones": ["UTC", "EST"]} + mock_api_client_instance.call_api.return_value = expected_response @@ - # Verify param_serialize was called - mock_api_client_instance.param_serialize.assert_called_once() + # Verify the generated API routed through ApiClient.call_api + mock_api_client_instance.call_api.assert_called_once()
🤖 Fix all issues with AI agents
In @.openapi-generator/VERSION:
- Line 1: Replace the outdated version string "7.9.0" in the .openapi-generator
VERSION file with the latest stable release "7.19.0" so the project uses the
updated OpenAPI Generator CLI; commit the change.
In `@generate_management_sdk.py`:
- Around line 417-456: The cleanup_generated_templates() function currently
unconditionally deletes root-level directories in template_dirs ("docs", "test")
which can remove non-generated content; change it to read the generated file
list from the .openapi-generator/FILES manifest (or accept a list) and only
unlink files that appear in template_files and in that manifest, and only remove
directories (template_dirs) if they are empty after removing listed generated
files or if every entry inside the directory matches entries from
.openapi-generator/FILES; use the existing variables
template_files/template_dirs and Path checks inside
cleanup_generated_templates() to implement these safer checks so pre-existing
docs/ or test/ content is preserved.
In `@kinde_sdk/management/management_client.py`:
- Around line 55-72: Update the doc examples in the ManagementClient usage to
use the actual dynamic attribute naming convention (<name>_api) rather than bare
names; replace occurrences like billing_api (and any references to client.users
or client.users.get_users()) with the correct dynamic attributes such as
billing_entitlements_api and users_api, and update example method calls to use
client.users_api.get_users(), client.users_api.create_user(...), and
client.billing_entitlements_api.get_billing_info() so the docs match the dynamic
loader behavior implemented by ManagementClient.
In `@kinde_sdk/management/schemas.py`:
- Line 21: The import line "from datetime import date, datetime, timedelta #
noqa: F401" in schemas.py contains an unused "# noqa: F401" directive; remove
that directive so the line reads "from datetime import date, datetime,
timedelta" (leave the imported symbols date, datetime, timedelta intact) to
clear the unused-noqa lint warning.
- Around line 25-28: The management module is importing incompatible exception
classes (ApiTypeError, ApiValueError) from kinde_sdk.management.exceptions which
differ from the core hierarchy; update the import in schemas.py to use the core
exception types (kinde_sdk.core.exceptions.ApiTypeError, ApiValueError) or
otherwise ensure the management exceptions inherit from the core ones so
existing exception handlers still catch them; locate the import of ApiTypeError
and ApiValueError in schemas.py and either change the import source to
kinde_sdk.core.exceptions or modify the management exception definitions
(OpenApiException subclassing) so ApiTypeError and ApiValueError are compatible
with the core exceptions.
🧹 Nitpick comments (3)
.gitignore (1)
65-65: Minor: Inconsistent comment formatting.The comment lacks a space after
#, which is inconsistent with the formatting convention used for all other comments in this file (e.g., lines 1, 6, 9, 27, 33, 37, 52, 56, 59, 62, 68).✨ Suggested fix
-#Ipython Notebook +# Ipython Notebookgenerate_management_sdk.py (1)
367-415: Remove unusedresultassignment ingenerate_sdk.Line 401 assigns
resultbut never uses it. Dropping the assignment keeps lint clean (or log output intentionally).🧹 Suggested tweak
- result = subprocess.run(cmd, check=True, capture_output=True, text=True) + subprocess.run(cmd, check=True, capture_output=True, text=True)testv2/testv2_management/test_management_client.py (1)
452-495: Clarify the feature-flag test comment.
The comment saysget_feature_flagsdoesn't exist, buttest_dynamic_method_generationasserts it does. Reword to avoid confusion.✏️ Suggested tweak
- # Test feature flag API call - use create_feature_flag since get_feature_flags doesn't exist + # Test feature flag API call - exercise create_feature_flag path
…jection - Fix header_params handling to support positional arguments in call_api wrapper - Update generator configuration and add ignore files for custom SDK files - Remove deprecated exception classes and clean up unused files
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: 2
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/api_client.py (1)
518-535:⚠️ Potential issue | 🟠 MajorKeep URL encoding for multi-valued query params.
Dropping
quote()forcollection_format == 'multi'makes values with spaces,+,&, etc. produce invalid query strings. This is a regression from the prior encoded behavior.🔧 Suggested fix
- if collection_format == 'multi': - new_params.extend((k, str(value)) for value in v) + if collection_format == 'multi': + new_params.extend((k, quote(str(value))) for value in v)
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 45: The .gitignore pattern "*,cover" is likely a typo and should be
corrected to "*.cover" so it matches files with the .cover extension; update the
pattern string from "*,cover" to "*.cover" to properly ignore coverage files.
In `@kinde_sdk/management/management_client.py`:
- Around line 317-318: The parameter annotation for get_organization currently
uses a default None with a plain str (code: str = None) which violates PEP 484;
change the signature to use Optional[str] (e.g., code: Optional[str] = None) and
add/ensure the import from typing import Optional at the top of the module;
update any type hints or references to this parameter in get_organization to
reflect Optional[str] so linters like Ruff no longer flag RUF013.
🧹 Nitpick comments (2)
.gitignore (1)
65-66: Fix comment formatting and capitalization.The comment has two issues:
- Missing space after
#(inconsistent with other comments in the file)- Should be "IPython" (capital P) as that's the official project name
📝 Proposed fix
-#Ipython Notebook +# IPython Notebook .ipynb_checkpointsgenerate_management_sdk.py (1)
367-425: Drop the unusedresultassignment to avoid dead code.🔧 Suggested minimal fix
- result = subprocess.run(cmd, check=True, capture_output=True, text=True) + subprocess.run(cmd, check=True, capture_output=True, text=True)
- Bump openapi-generator-cli from 7.9.0 to 7.19.0 - Regenerate management SDK with updated generator templates - Fix management_client.py docstring to use correct API naming convention
There was a problem hiding this comment.
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/__init__.py (1)
552-557:⚠️ Potential issue | 🟡 MinorExtend
__all__instead of overwriting it to preserve all generated exports.Line 557 replaces the entire
__all__list (containing 282 generated API classes, models, and utilities) with only two custom classes, breaking the public API surface. Use.extend()to add the custom exports while retaining all generated ones.♻️ Suggested fix
-__all__ = ['ManagementClient', 'ManagementTokenManager'] +__all__.extend(['ManagementClient', 'ManagementTokenManager'])
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@kinde_sdk/management/management_client.py`:
- Around line 500-518: The deprecated wrapper update_feature_flag currently
forwards a single update_feature_flag_request argument but must forward the
individual fields expected by feature_flags_api.update_feature_flag; modify
update_feature_flag to either accept and pass through the individual parameters
(name, description, type, allow_override_level, default_value) or unpack
update_feature_flag_request into those named args before calling
self.feature_flags_api.update_feature_flag, handling None/missing request
gracefully and preserving **kwargs and the deprecation warning.
- Around line 466-480: The deprecated wrapper get_feature_flags currently calls
environments_api.get_environement_feature_flags (misspelled "environement"); if
you can change the OpenAPI spec, rename the endpoint to
get_environment_feature_flags and regenerate the client so EnvironmentsApi
exposes get_environment_feature_flags; otherwise preserve compatibility by
keeping the existing call to environments_api.get_environement_feature_flags (no
change to get_feature_flags) and add a clear TODO comment pointing to the
OpenAPI spec fix. Update any references to the misspelled symbol
(get_environement_feature_flags) consistently to match the chosen approach.
- Around line 434-448: The deprecated wrapper method update_role currently calls
a non-existent self.roles_api.update_role() causing an AttributeError; change
the call in update_role to use the generated method name
self.roles_api.update_roles(...) passing the same parameters (role_id and
update_role_request and **kwargs) so the wrapper delegates to
RolesApi.update_roles instead of the missing update_role.
In `@testv2/testv2_management/test_management_client.py`:
- Around line 1256-1281: The test test_deprecated_update_feature_flag_wrapper
currently swallows all exceptions with a bare "except Exception" while only
checking for a DeprecationWarning; update it to either (A) fix
ManagementClient.update_feature_flag so the deprecated wrapper accepts the
correct params, or (B) if the wrapper is intentionally broken, replace the bare
except with a pytest.raises(ValidationError) (import ValidationError from
pydantic) around client.update_feature_flag("test_flag", None) and keep the
warnings.catch_warnings/assertions so the test specifically asserts the expected
ValidationError and the DeprecationWarning instead of catching all exceptions.
- Around line 1157-1181: The deprecated wrapper method update_role on
ManagementClient incorrectly calls self.roles_api.update_role (which doesn't
exist) and emits a misleading deprecation message; update the
ManagementClient.update_role implementation to either call the correct API
method (likely self.roles_api.update_roles or
self.roles_api.update_role_permissions depending on intended behavior) and
update the deprecation text to point to the correct replacement, or remove the
update_role wrapper entirely and adjust/remove the test
test_deprecated_update_role_wrapper accordingly so it no longer expects a
deprecation warning for a non-existent replacement.
🧹 Nitpick comments (1)
testv2/testv2_management/test_management_client.py (1)
126-138:test_token_handling_setuphas no assertions.This test creates a client but doesn't assert anything about the token handling setup. It's effectively just a "doesn't crash" test. The behavior is well-covered by the subsequent token injection tests (lines 143-309), so this is low priority, but the test name implies it's verifying something it isn't.
Consider either adding a meaningful assertion (e.g., that
call_apihas been wrapped) or removing it to avoid false confidence.
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 203-206: The docstring/comment in management_client.py incorrectly
references client.users.get_users(); update the stale example to use the dynamic
attribute naming convention with the *_api suffix (e.g.,
client.users_api.get_users()) and adjust any other examples in that comment to
use the <resource>_api form so they match the dynamic attribute implementation
in ManagementClient.
🧹 Nitpick comments (2)
kinde_sdk/management/management_client.py (1)
500-523: Parametertypeshadows the built-intype().Using
typeas a parameter name (line 501, 519) shadows Python's built-in. This is a common occurrence in generated code and unlikely to cause a real issue here since the function body doesn't needtype(), but for a hand-written wrapper it's a minor code smell.testv2/testv2_management/test_management_client.py (1)
362-392: Minor:test_get_user_data_with_query_parameterandtest_get_user_data_by_idoverlap significantly.Both tests call
client.users_api.get_user_data(id="kp_abc123"/"kp_user123")and assert essentially the same things (GET method,/api/v1/userin URL,id=in query). Consider consolidating into a single parameterized test or differentiating their assertions to reduce maintenance burden.Also applies to: 606-641
dtoxvanilla1991
left a comment
dtoxvanilla1991
left a comment
There was a problem hiding this comment.
Good stuff 💯 minor suggestions.
There was a problem hiding this comment.
17 test files in kinde_sdk/test/test_models/ import from kinde_sdk.management import schemas, but schemas is NOT exported here.
Affected files:
- test_connected_apps_auth_url.py
- test_api_result.py
- test_add_organization_users_response.py
- test_success_response.py
- (and 13 more)
Recommendation: Add from kinde_sdk.management import schemas as schemas to exports, OR update all 17 test files to use direct model imports.
There was a problem hiding this comment.
Good feedback, thank you. I checked it out I noticed that these tests are actually not being run as they are not declared in pytest.ini. Only testv2 is declared and run.
I think, after discussing with Daniel that PR's are too large, I would prefer to delete these tests in another PR after this has been merged.
There was a problem hiding this comment.
@DanielRivers would you mind if we delete it in this PR?
3 participants
This PR updates the Management SDK generator and regenerates the SDK using the latest Kinde Management API specification
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.