pytest conditional deps #146
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.
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.
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.
Implement conditional pytest dependencies to maintain backward compatibility with Python 3.9 while allowing Python 3.10+ to use pytest v9. Changes: - Add environment markers in pyproject.toml [project.optional-dependencies.dev] - Python 3.9: pytest>=7.4.0,<9.0.0 - Python 3.10+: pytest>=9.0.0 - Update requirements.txt with conditional pytest dependencies using markers - Update pytest-asyncio to ^0.26.0 and black to ^24.3.0 (from PR kinde-oss#134) - Keep Poetry dev-dependencies with Python 3.9 compatible version as fallback This resolves the Python 3.9 compatibility issue in PR kinde-oss#134 where pytest v9 requires Python 3.10+. The conditional dependencies ensure: - Python 3.9 CI/CD continues to work with pytest <9.0.0 - Python 3.10+ developers can use pytest v9 features - No breaking changes for existing Python 3.9 users Fixes: kinde-oss#134
- Fix singularization for words ending in 'ies' (e.g., 'properties' -> 'property') This fixes the critical issue where 'properties' was incorrectly singularized to 'propertie' instead of 'property', causing incorrect method names. - Fix HTTP status codes to follow REST conventions: - Users create: 200 -> 201 (POST should return 201 Created) - Roles update: 201 -> 200 (PATCH should return 200 OK) These changes address the CodeRabbit review comments on PR kinde-oss#144.
|
Warning Rate limit exceeded@BrandtKruger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 2 seconds before requesting another review. ⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe management client API endpoints are updated from singular to plural resource names for consistency. New identity-related resources ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/management_client.py (1)
480-511: Redundant query string construction.
final_pathis constructed with query parameters (lines 480-485) but thenformatted_path(without query params) is used inparam_serialize(line 489-494), andquery_paramsis passed separately. Thefinal_pathvariable appears unused after construction.The
final_pathconstruction (lines 480-485) seems to be dead code. Ifparam_serializehandles query parameters, remove the redundant logic:- # 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
🧹 Nitpick comments (5)
requirements.txt (1)
26-27: Consider adding upper bounds to dev dependencies for reproducibility.While the conditional constraints for pytest are well-scoped, the lack of upper bounds on
pytest>=9.0.0(line 27),pytest-asyncio>=0.26.0(line 29), andblack>=24.3.0(line 32) could allow breaking changes in future versions to silently introduce instability.This is especially relevant given the PR description mentions Poetry uses stricter caret-based versions (e.g.,
pytest-asyncio ^0.26.0). Consider aligning requirements.txt with those stricter constraints to ensure reproducible dev environments.If you want reproducible builds, consider tightening the upper bounds. For example:
-pytest>=9.0.0; python_version >= "3.10" -pytest-asyncio>=0.26.0 +pytest>=9.0.0,<10.0.0; python_version >= "3.10" +pytest-asyncio>=0.26.0,<0.27.0(and similar for
blackif desired)Also applies to: 29-29, 32-32
kinde_sdk/management/management_client.py (3)
389-413: Singularization logic has incomplete coverage for compound resources.The
_singularize_resourcemethod handlesuser_identitiesandidentitiesexplicitly, but doesn't handle other compound resources ending in_identitiesor similar patterns. Also, resources likeindustrieswould incorrectly becomeindustrie(removing trailingsafter theies→ycheck fails).Wait, let me re-check:
industriesends withies, so it would becomeindustryvia line 407-408. That's correct.However, the method doesn't handle
timezones→timezonecorrectly since it just removes trailings. Actually, that would work:timezones[:-1] =timezone. ✓One edge case:
businessis handled, but what about potential future resources likeaddresses?addressesends withesnoties, so it would becomeaddresse(incorrect - should beaddress).Consider handling words ending in
es(likeaddresses→address) if such resources might be added:elif resource.endswith('ies'): return resource[:-3] + 'y' + # Handle words ending in 'es' (e.g., 'addresses' -> 'address') + elif resource.endswith('ses') or resource.endswith('xes') or resource.endswith('zes'): + return resource[:-2] # Default: remove trailing 's' if present elif resource.endswith('s'): return resource[:-1]Alternatively, consider using an
inflectlibrary for robust pluralization handling if this becomes more complex.
482-485: Query string construction doesn't URL-encode values.For GET/DELETE requests, query parameters are concatenated without URL encoding. Special characters in values could break the URL.
Consider 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}" + separator = '&' if '?' in final_path else '?' + final_path = f"{final_path}{separator}{urlencode(query_params)}"Note: The
param_serializecall at line 491 may already handle encoding, in which case this manual query string construction (lines 481-485) might be redundant. Verify iffinal_pathis actually used downstream or ifurlfromparam_serializeis the one used in the request.
600-623: Backward compatibility loop has no effect.The loop at lines 600-622 iterates over method name pairs but only executes
pass, making it a no-op. The comment suggests these methods "will be created dynamically," but the dynamic generation already happens in_generate_methods().Either remove the dead code or implement actual deprecation warnings if backward compatibility aliases are needed:
-# Add backwards compatibility methods for common operations -# These will be deprecated in future versions -for method_name, new_name in [ - ('get_users', 'get_users'), - # ... all pairs ... -]: - pass # These methods will be created dynamically +# Note: Backwards compatibility methods are created dynamically via _generate_methods()testv2/testv2_management/test_management_client.py (1)
104-108: Consider adding test coverage for new identity methods.The dynamic method generation test verifies feature flag methods but doesn't include assertions for the new
user_identitiesandidentitiesendpoints.Add assertions for the new identity-related methods:
# Test user identity methods assert hasattr(client, 'get_user_identities') assert hasattr(client, 'create_user_identity') # Test identity methods assert hasattr(client, 'get_identity') assert hasattr(client, 'update_identity') assert hasattr(client, 'delete_identity')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (3)
kinde_sdk/management/management_client.py(12 hunks)requirements.txt(1 hunks)testv2/testv2_management/test_management_client.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-08T14:07:43.901Z
Learnt from: brettchaldecott
Repo: kinde-oss/kinde-python-sdk PR: 87
File: test_organization_users.py:70-70
Timestamp: 2025-07-08T14:07:43.901Z
Learning: The method names in test_organization_users.py have been tested and confirmed to be correct by the developer, including get_user_feature_flag and refresh_user_refresh_claim mappings.
Applied to files:
testv2/testv2_management/test_management_client.py
🔇 Additional comments (7)
requirements.txt (1)
23-27: Conditional pytest dependencies correctly implemented.The PEP 508 environment markers are properly formatted and the version constraints correctly balance backward compatibility (Python 3.9 with pytest <9.0.0) and forward compatibility (Python 3.10+ with pytest 9+).
kinde_sdk/management/management_client.py (5)
208-216: New identity endpoints look correctly structured.The
user_identitiesandidentitiesresource definitions follow the established pattern with appropriate HTTP methods and path parameters.
350-358: Response types for identities endpoints are properly defined.The response type mappings for
user_identitiesandidentitiesfollow the established pattern and include appropriate error responses.
88-89: Verify PUT vs PATCH semantics for feature_flags update.The
feature_flagsupdate endpoint usesPUT(line 89), while most other update endpoints usePATCH.PUTtypically implies full resource replacement, whereasPATCHis for partial updates.Confirm this is intentional and aligns with the Kinde API specification. If partial updates are expected, consider:
- 'update': ('PUT', '/api/v1/feature_flags/{feature_flag_key}'), + 'update': ('PATCH', '/api/v1/feature_flags/{feature_flag_key}'),
131-131: Verify PUT vs PATCH for properties update.Similar to feature_flags, the
propertiesupdate endpoint usesPUT. Ensure this matches the API specification.
468-476: Potential issue:kwargs.pop()mutates the dictionary during iteration.The code iterates over
list(kwargs)and useskwargs.pop(k)which is safe due to the list copy. However, line 476 then iterates overkwargs.items()after modification. This works but could be clearer.The logic correctly separates query parameters from body parameters for POST/PUT/PATCH requests.
testv2/testv2_management/test_management_client.py (1)
489-531: Test correctly updated for feature flag creation flow.The test properly exercises
create_feature_flagwith the expected payload structure (key,type,value) and verifies the mocked response handling.
…atibility Address CodeRabbit review comment: pytest-asyncio 0.26.0 does not support pytest 9.0+. According to pytest-asyncio changelog, explicit support for pytest 9 was added in version 1.3.0+. Changes: - Add conditional pytest-asyncio dependencies matching pytest versions: - Python <3.10: pytest-asyncio>=0.26.0 (compatible with pytest 7.4.x) - Python >=3.10: pytest-asyncio>=1.3.0 (required for pytest 9.0+) - Update both requirements.txt and pyproject.toml - Add comments explaining the version requirements This ensures Python 3.10+ environments with pytest 9.0+ use a compatible pytest-asyncio version, preventing runtime failures. Fixes: CodeRabbit review comment on PR kinde-oss#144
2 participants
Explain your changes
Implement conditional pytest dependencies to maintain backward compatibility
with Python 3.9 while allowing Python 3.10+ to use pytest v9.
Changes:
Add environment markers in pyproject.toml [project.optional-dependencies.dev]
Python 3.9: pytest>=7.4.0,<9.0.0
Python 3.10+: pytest>=9.0.0
Update requirements.txt with conditional pytest dependencies using markers
Update pytest-asyncio to ^0.26.0 and black to ^24.3.0 (from PR #134)
Keep Poetry dev-dependencies with Python 3.9 compatible version as fallback
This resolves the Python 3.9 compatibility issue in PR #134 where pytest v9
requires Python 3.10+. The conditional dependencies ensure:
Python 3.9 CI/CD continues to work with pytest <9.0.0
Python 3.10+ developers can use pytest v9 features
No breaking changes for existing Python 3.9 users
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.