Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace kinné SDK with a generated Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MG as ManagementClient
participant TM as ManagementTokenManager
participant AC as ApiClient
participant API as GeneratedAPI
participant S as ExternalService
Client->>MG: instantiate(domain, client_id, client_secret)
MG->>TM: initialize / request token
TM->>S: token request
S-->>TM: return bearer token
MG->>AC: create ApiClient(Configuration(base_url))
MG->>MG: _initialize_api_classes() -> instantiate API classes -> attach e.g., 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 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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.
|
@dtoxvanilla1991 , @daniel-kinde are we ready to approve and merge? Tests cleanup will be in a follow-up PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@generate_management_sdk.py`:
- Around line 53-62: The custom_imports block is inserting a plain assignment to
__all__ which will overwrite the generator-populated exports on regeneration;
update the custom_imports entry so it extends the existing __all__ instead of
assigning (use __all__.extend([...]) or append the two symbols) and include the
two exported names 'ManagementClient' and 'ManagementTokenManager' (reference:
__all__, custom_imports, ManagementClient, ManagementTokenManager) so the
generator's ~260 entries remain intact.
In `@kinde_sdk/management/__init__.py`:
- Around line 552-557: The generator's SDK_CONFIG custom_imports currently
assigns "__all__ = ['ManagementClient', 'ManagementTokenManager']" which will
overwrite the large generated __all__ list on regeneration; change that
assignment in generate_management_sdk.py (SDK_CONFIG -> custom_imports) to
extend or merge instead (e.g., use __all__.extend([...]) or programmatically
append those names) so the file's existing __all__ is preserved and the runtime
file can safely use
__all__.extend(['ManagementClient','ManagementTokenManager']) as shown in
management/__init__.py.
🧹 Nitpick comments (3)
generate_management_sdk.py (3)
574-578: Fragile presence-check for custom imports.
config["custom_imports"][1]is"# Custom imports for Kinde Management Client", and.lstrip('# ')strips all leading#and space characters (not the substring"# "), producing"Custom imports for Kinde Management Client". This works by coincidence but is brittle — if the comment wording changes slightly, the check silently fails and appends duplicates.Consider a more robust marker, e.g. checking for the actual import line:
♻️ Suggested improvement
- # Check if custom imports are already present (check first import line) - first_custom_import = config["custom_imports"][1] # Skip empty line - if first_custom_import.lstrip('# ') in content: + # Check if custom imports are already present + marker = "from .management_client import ManagementClient" + if marker in content: print("✓ Custom imports already present") return
427-427: Use explicitOptionaltype for theconfigparameter.
Dict[str, Any] = Noneis a PEP 484 violation (implicitOptional). This was also flagged by Ruff (RUF013).♻️ Proposed fix
-def cleanup_generated_templates(config: Dict[str, Any] = None): +def cleanup_generated_templates(config: Dict[str, Any] | None = None):Or if targeting Python < 3.10:
+from typing import Optional ... -def cleanup_generated_templates(config: Dict[str, Any] = None): +def cleanup_generated_templates(config: Optional[Dict[str, Any]] = None):
386-403:additionalPropertiesare specified both inopenapitools.jsonconfig and as CLI flags — potential for drift.
ensure_openapitools_config()writesadditionalPropertiesinto the JSON config (lines 316-322), whilegenerate_sdk()also passes them via--additional-propertieson the CLI (lines 388-393). CLI flags override the config file, so the JSON values are effectively dead. Consider using one source of truth — either the config file with--generator-keyor the CLI flags — to avoid confusion when they diverge.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Approved 🟢 |
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.