Support for Using Predefined Auth Token from Environment Variables #10
Conversation
WalkthroughThe changes refactor how authentication tokens are managed and utilized throughout the codebase. A new authentication token variable ( Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
75-89: Enhanced Documentation for Auth Token UsageThe new section clearly illustrates how to set the optional
QEQ_AUTH_TOKENusing environment variables and shows an updated example of instantiating theClientwith the new parameter. To further improve clarity, consider adding a brief note that explains the behavior when the token is not provided (i.e., that the client will generate a new token by default).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)env.template(1 hunks)quienesquien/client.py(3 hunks)quienesquien/version.py(1 hunks)tests/cassettes/test_client_with_auth_token.yaml(1 hunks)tests/conftest.py(1 hunks)tests/test_client.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- quienesquien/version.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Enforce Relative Imports for Internal Modules
En...
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
tests/conftest.pytests/test_client.pyquienesquien/client.py
`**/*.py`:
Rule: Enforce Snake Case in Python Backend
- N...
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
tests/conftest.pytests/test_client.pyquienesquien/client.py
🔇 Additional comments (10)
env.template (1)
3-4: Auth token example is properly added to environment template.The addition of the
QEQ_AUTH_TOKENvariable in the environment template file is consistent with the PR objectives, allowing authentication tokens to be configured via environment variables.tests/conftest.py (1)
19-21: New fixture for auth token is properly implemented.The fixture correctly initializes a Client with the predefined auth token, allowing tests to verify that the client works properly with a provided authentication token.
quienesquien/client.py (5)
25-25: Attribute visibility changed from private to public.The attribute has been changed from
_auth_token(private by convention) toauth_token(public). This is consistent with the PR objective to expose the authentication token for external configuration.
32-32: Updated attribute reference is consistent.The method correctly updates the reference from
_auth_tokentoauth_token, maintaining consistency with the visibility change.
51-52: Consistent use of the renamed attribute.The renamed attribute is correctly referenced in the condition check and return statement.
64-64: Token assignment uses the updated attribute.The token from the response is correctly assigned to the renamed attribute.
67-67: Return statement uses updated attribute.The method consistently returns the renamed attribute.
tests/test_client.py (2)
74-74: Updated assertion for renamed attribute.The test correctly asserts against the renamed public attribute instead of the previous private one.
189-194: New test validates client behavior with predefined auth token.This test appropriately verifies that the client works correctly when initialized with a predefined authentication token, which is a key functionality added in this PR.
tests/cassettes/test_client_with_auth_token.yaml (1)
1-63: New Cassette File for Auth Token TestingThis new YAML cassette effectively simulates an API interaction when using a predefined authentication token. The inclusion of the
Authorizationheader with a dummy token (DUMMY_AUTHORIZATION) is consistent with the PR objective of supporting shared tokens for multiple clients. Please verify that the multi-line JSON string in the response body is properly parsed by your cassette runner; if necessary, consider refactoring it for improved readability.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 174 156 -18
=========================================
- Hits 174 156 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/test_client.py (3)
2-2: Add error handling for environment variable access.Direct access to environment variables can cause exceptions if they're not set.
-import os +import os +from os import getenv
18-18: Consider using getenv with a default value.Direct access to environment variables will raise KeyError if the variable is not set.
-QEQ_SECRET_ID = os.environ['QEQ_SECRET_ID'] +QEQ_SECRET_ID = os.getenv('QEQ_SECRET_ID', '') +assert QEQ_SECRET_ID, 'QEQ_SECRET_ID environment variable is required for tests'
184-193: Remove redundant Client import.The Client class is already imported at the top of the file (line 7), so importing it again inside the test is unnecessary.
async def test_invalid_client() -> None: - from quienesquien import Client - search_client = Client('username', 'client_id') with pytest.raises(AssertionError) as excinfo: await search_client.search('Andres Manuel Lopez Obrador', 80) assert ( str(excinfo.value) == 'you must create or reuse an already created auth token' )tests/cassettes/test_search_by_curp.yaml (2)
19-30: Response JSON Structure and Formatting:
The response body includes a JSON string indicating a successful lookup and providing an array of detailed person objects. Verify that the multi-line JSON string—with its escaped Unicode characters (e.g.,\u00f3for “ó”)—is correctly parsed by your test framework. If there is any risk of parsing issues, you might consider using YAML’s block scalar format to improve readability.
41-41: Timestamp in Date Header:
The Date header is recorded asTue, 25 Mar 2025 22:03:40 GMT. Ensure that this static value does not lead to flaky tests if the test harness performs strict header validations. It might be beneficial to parameterize or ignore dynamic headers like this in your tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (17)
README.md(4 hunks)quienesquien/client.py(4 hunks)tests/cassettes/test_block_account.yaml(0 hunks)tests/cassettes/test_create_token.yaml(1 hunks)tests/cassettes/test_insufficient_balance.yaml(0 hunks)tests/cassettes/test_internal_server_error.yaml(0 hunks)tests/cassettes/test_invalid_plan.yaml(0 hunks)tests/cassettes/test_invalid_token.yaml(1 hunks)tests/cassettes/test_reusable_token.yaml(0 hunks)tests/cassettes/test_search_by_curp.yaml(1 hunks)tests/cassettes/test_search_by_name.yaml(2 hunks)tests/cassettes/test_search_not_found.yaml(1 hunks)tests/cassettes/test_search_with_params.yaml(1 hunks)tests/cassettes/test_unknown_error.yaml(0 hunks)tests/conftest.py(1 hunks)tests/test_client.py(6 hunks)tests/test_person.py(1 hunks)
💤 Files with no reviewable changes (6)
- tests/cassettes/test_internal_server_error.yaml
- tests/cassettes/test_reusable_token.yaml
- tests/cassettes/test_invalid_plan.yaml
- tests/cassettes/test_insufficient_balance.yaml
- tests/cassettes/test_unknown_error.yaml
- tests/cassettes/test_block_account.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/cassettes/test_create_token.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Enforce Relative Imports for Internal Modules
Ensure that any imports referencing internal modules use relative paths. However, if modules reside in the main module dir...
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
tests/test_client.pyquienesquien/client.pytests/test_person.py
`**/*.py`:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic...
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
tests/test_client.pyquienesquien/client.pytests/test_person.py
🧬 Code Definitions (3)
tests/test_client.py (2)
tests/conftest.py (2)
client(14-15)client_without_token(19-20)quienesquien/client.py (3)
Client(20-165)create_token(44-59)search(61-165)
quienesquien/client.py (1)
quienesquien/exc.py (1)
QuienEsQuienError(1-4)
tests/test_person.py (1)
quienesquien/person.py (1)
Person(4-33)
🔇 Additional comments (22)
README.md (4)
9-12: Great addition of an important warning!This warning box clearly communicates the critical behavior around token invalidation and provides guidance on using existing tokens to prevent authentication failures.
41-54: Clear and helpful token generation documentationThe new section effectively explains the token creation process, showing the correct method usage and highlighting the reusability of tokens.
57-58: Example section updated to use the new token approachThe example has been properly updated to reflect the new authentication mechanism, showing how to pass the auth token to the client constructor.
Also applies to: 70-75
98-116: Good addition of environment variable optionThe environment variable approach provides a convenient alternative for managing the authentication token. Line 114 uses
os.environ.get()which correctly handles the case when the variable is not defined.tests/cassettes/test_search_by_name.yaml (3)
18-18: URI updated to use the find endpoint instead of token endpointThe test cassette now properly reflects the new authentication flow, where the token is provided via the Authorization header rather than being fetched during the test.
30-41: Updated response data with comprehensive search resultsThe response data has been updated to include multiple person records with detailed information, which properly tests the person search functionality.
52-52: Updated timestamp in response headersThe date has been updated to reflect the new test recording.
quienesquien/client.py (3)
24-24: Made auth_token public instead of privateThis change exposes the authentication token as a public attribute, allowing direct access and assignment, which is consistent with the new token management approach.
90-93: Added assertion to ensure auth_token is availableThis check prevents the search method from being called without a valid authentication token, which is essential since token fetching is no longer automatic.
126-126: Updated authorization header to use the public auth_tokenThis correctly uses the public auth_token attribute in the authorization header for the search request.
tests/cassettes/test_search_not_found.yaml (3)
18-18: URI updated to use find endpoint instead of token endpointThe test cassette now properly reflects the new authentication flow for the not-found scenario.
21-21: Updated response body to show not-found resultThe response now correctly shows a not-found scenario with the appropriate error message, which is useful for testing error handling.
32-32: Updated timestamp in response headersThe date has been updated to reflect the new test recording.
tests/cassettes/test_search_with_params.yaml (1)
41-41: Date header update looks good.The updated date header in the test cassette is part of the normal VCR recording process and doesn't require any changes.
tests/test_person.py (1)
1-21: Well-implemented test for Person class's extra fields handling.The test thoroughly verifies that the
collect_extra_fieldsmodel validator correctly:
- Converts keys in model_extra to lowercase
- Moves them to the metadata dictionary
- Clears model_extra after processing
- Preserves the standard attributes
This aligns perfectly with the implementation in the Person class.
tests/test_client.py (3)
62-65: Good update to test_invalid_token.The test now directly sets the auth_token to an invalid value, which aligns with the new public auth_token attribute.
149-154: Good implementation of test_create_token.This test correctly verifies the new token creation process:
- Creating a token using the create_token method
- Setting the token on the client
- Confirming the client can successfully perform a search with the token
157-170: Good update to test_token_error_response.The test now uses client_without_token and tests the error handling of create_token, which properly aligns with the updated client API.
tests/cassettes/test_invalid_token.yaml (1)
33-33: Date header update looks good.The updated date header in the test cassette is part of the normal VCR recording process and doesn't require any changes.
tests/cassettes/test_search_by_curp.yaml (3)
1-18: Endpoint and Query Parameters Verification:
The request now targets the/api/findendpoint with detailed query parameters (birthday, client_id, curp, list, percent, sex, type, username). This aligns with the updated authentication flow where a predefined token is used via environment variables. The use of a dummy token (DUMMY_AUTHORIZATION) in the header is acceptable for testing, but please ensure that in production or live tests the token is dynamically sourced from the environment.
31-40: Response Headers Consistency Check:
The response headers (e.g.,Access-Control-Allow-Origin,Cache-Control,Connection, andContent-Type) appear to be correctly configured. Please double-check that these fixed header values match the expected outputs from the live search endpoint, especially if environments vary.
42-57: Remaining Response Headers and Status Verification:
The remainder of the response headers (e.g., Server, Set-Cookie, Transfer-Encoding, etc.) and the response status code (200 OK) conform to expectations. Validate that these static values are consistent with the real API behavior across different environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_client.py (1)
184-193: Remove redundant import in test_client_without_tokenThe Client class is already imported at the top of the file, so importing it again inside the test function is unnecessary.
async def test_client_without_token() -> None: - from quienesquien import Client - search_client = Client('username', 'client_id') with pytest.raises(AssertionError) as excinfo: await search_client.search('Andres Manuel Lopez Obrador', 80) assert ( str(excinfo.value) == 'you must create or reuse an already created auth token' )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
quienesquien/version.py(1 hunks)tests/test_client.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- quienesquien/version.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Enforce Relative Imports for Internal Modules
Ensure that any imports referencing internal modules use relative paths. However, if modules reside in the main module dir...
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
tests/test_client.py
`**/*.py`:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic...
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
tests/test_client.py
🧬 Code Definitions (1)
tests/test_client.py (2)
tests/conftest.py (2)
client(14-15)client_without_token(19-20)quienesquien/client.py (3)
Client(20-165)create_token(44-59)search(61-165)
🔇 Additional comments (6)
tests/test_client.py (6)
2-2: Good addition of os module for environment variablesThe addition of the
osmodule aligns with the PR's objective of supporting authentication tokens via environment variables.
18-19: LGTM: Environment variable for secret IDAdding the
QEQ_SECRET_IDenvironment variable is appropriate for the token creation functionality being implemented.
62-64: Simplified token invalidation testThe test has been simplified to directly set an invalid token rather than mocking the response. This is a cleaner approach that focuses on testing the behavior when an invalid token is used.
149-154: Appropriate test for token creation functionalityThis new test properly verifies the token creation workflow by:
- Creating a token using the secret ID
- Setting the token on the client
- Verifying the token works by performing a search
This test is essential for validating the new functionality added in this PR.
157-159: Good adaptation of test to use client_without_tokenThe test now correctly uses the
client_without_tokenfixture which better matches the test's purpose of verifying token creation error handling.
170-170: Test updated to use new create_token methodThe test now correctly tests the error handling of the new
create_tokenmethod instead of the previous token creation mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_person.py (1)
4-19: LGTM! The test case correctly verifies extra fields normalization.This test effectively verifies that the
collect_extra_fieldsmodel validator works as expected by:
- Testing standard field mapping via aliases
- Confirming both
peso1andcoincidenciafields (which share the same alias) are populated correctly- Verifying that extra fields are normalized to lowercase and accessible as attributes
Consider adding additional test cases to cover edge cases:
- Mixed case keys in extra fields
- Empty extra fields
- Extra fields that might conflict with standard field names
def test_collect_extra_fields_edge_cases(): # Mixed case keys person_data = { 'LISTA': 'lista1', 'COINCIDENCIA': 100, 'NOMBRECOMP': 'Juan Pérez', 'MixedCase_Field': 'mixed value', } person = Person(**person_data) assert person.mixedcase_field == 'mixed value' # No extra fields person_data = { 'LISTA': 'lista1', 'COINCIDENCIA': 100, 'NOMBRECOMP': 'Juan Pérez', } person = Person(**person_data) assert person.model_extra == {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
quienesquien/person.py(2 hunks)quienesquien/version.py(1 hunks)tests/test_person.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- quienesquien/version.py
- quienesquien/person.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Enforce Relative Imports for Internal Modules
Ensure that any imports referencing internal modules use relative paths. However, if modules reside in the main module dir...
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
tests/test_person.py
`**/*.py`:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic...
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
tests/test_person.py
🧬 Code Definitions (1)
tests/test_person.py (1)
quienesquien/person.py (1)
Person(4-35)
🔇 Additional comments (1)
tests/test_person.py (1)
1-1: Import looks good.The absolute import for the Person class from the main module is appropriate according to our coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/cassettes/test_invalid_token.yaml (1)
45-48: 💡 Verification agent❓ Verification inconclusive
HTTP Status Code Inconsistency for Invalid Token Response
Although the JSON body clearly states the token is invalid, the response status code is 200 ("OK"). This is inconsistent with the blocked account scenario (test_block_account.yaml) which correctly returns a 401 status. Please verify if the invalid token error should also return a 401 status code to accurately reflect unauthorized access. Consider updating the response status code as shown below:
- code: 200 - message: OK + code: 401 + message: Unauthorized
Invalid Token Response Should Use 401 Unauthorized
In file
tests/cassettes/test_invalid_token.yaml(lines 45–48), the response status is incorrectly set to 200 ("OK") even though the JSON indicates an invalid token. This is inconsistent with the blocked account scenario intest_block_account.yaml, which correctly returns a 401 status. Please update the response as follows:- code: 200 - message: OK + code: 401 + message: Unauthorized
🧹 Nitpick comments (4)
README.md (1)
48-49: Client instantiation inconsistencyThe client instantiation here only takes the username parameter, but in lines 109-113 it shows three parameters. These examples should be consistent with each other and with the actual implementation.
-client = Client(os.environ['QEQ_USER']) +client = Client(os.environ['QEQ_USER'], auth_token=None)tests/cassettes/test_unknown_error.yaml (1)
18-18: Fix extra ampersand in the URIThere's an unnecessary ampersand at the beginning of the query string which could cause issues.
- uri: https://app.q-detect.com/api/find?&name=Pepito+Cuenca&percent=80&username=DUMMY_USERNAME + uri: https://app.q-detect.com/api/find?name=Pepito+Cuenca&percent=80&username=DUMMY_USERNAMEquienesquien/client.py (2)
34-34: Consider more specific type annotationsWhile changing to
Anyprovides flexibility, it reduces type safety. Consider using a more specific union type if possible.- params: Mapping[str, Any] | None = None, + params: Mapping[str, str | int | float | bool | None] | None = None,
43-60: Consider storing the token during creationThe
create_tokenmethod returns the token but doesn't store it anywhere. While this works with the class method approach, it requires users to manually assign the token after creation.Consider one of these options:
- Make it an instance method and store the token:
- @classmethod - async def create_token(cls, client_id: str, secret_key: str) -> str: + async def create_token(self, client_id: str, secret_key: str) -> str: """Create a new authentication token.""" - auth_url = f'{cls.base_url}/api/token' + auth_url = f'{self.base_url}/api/token' # ...rest of method... - return response.text + self.auth_token = response.text + return self.auth_token
- Or create a helper factory method:
@classmethod async def create(cls, username: str, client_id: str, secret_key: str) -> "Client": """Create a client with a new authentication token.""" token = await cls.create_token(client_id, secret_key) return cls(username, token)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (16)
README.md(4 hunks)quienesquien/client.py(4 hunks)quienesquien/version.py(1 hunks)tests/cassettes/test_block_account.yaml(1 hunks)tests/cassettes/test_create_token.yaml(1 hunks)tests/cassettes/test_insufficient_balance.yaml(1 hunks)tests/cassettes/test_internal_server_error.yaml(1 hunks)tests/cassettes/test_invalid_plan.yaml(1 hunks)tests/cassettes/test_invalid_token.yaml(2 hunks)tests/cassettes/test_search_by_curp.yaml(2 hunks)tests/cassettes/test_search_by_name.yaml(3 hunks)tests/cassettes/test_search_not_found.yaml(2 hunks)tests/cassettes/test_search_with_params.yaml(2 hunks)tests/cassettes/test_unknown_error.yaml(1 hunks)tests/conftest.py(1 hunks)tests/test_client.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- quienesquien/version.py
- tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Enforce Relative Imports for Internal Modules
Ensure that any imports referencing internal modules use relative paths. However, if modules reside in the main module dir...
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
quienesquien/client.pytests/test_client.py
`**/*.py`:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic...
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
quienesquien/client.pytests/test_client.py
🔇 Additional comments (34)
README.md (4)
9-13: Great addition of important warning about token invalidationThis warning clearly explains a critical aspect of token management that was causing issues with multiple clients. The information about token invalidation and advice to reuse tokens addresses the core problem described in the PR objectives.
41-53: Well-documented token generation processThe new section clearly demonstrates how to create and use authentication tokens, aligning perfectly with the implementation changes in the
Clientclass.
69-73: Updated client creation example looks goodThe example correctly shows how to use the auth_token parameter instead of the secret ID, which aligns with the new implementation.
96-114: Good environment variable documentationUsing
os.environ.get('QEQ_AUTH_TOKEN')is the correct approach as it handles the case when the variable is not set. This section provides users with a convenient way to share tokens between applications.tests/cassettes/test_search_by_name.yaml (1)
18-18: URI updated to reflect new token usage approachThe test cassette has been correctly updated to test a direct search request instead of a token request, which aligns with the new implementation where tokens are provided rather than automatically fetched.
tests/cassettes/test_insufficient_balance.yaml (1)
18-18: URI updated for direct search testingThe cassette correctly tests the insufficient balance error case with the updated URI pattern that uses the token directly in the Authorization header.
quienesquien/client.py (5)
3-3: Appropriate import for type annotationAdding
Anyto the imports is necessary for the updated type annotation in the_make_requestmethod.
23-23: Good change making auth_token publicMaking the token attribute public allows external code to set and access it directly, which aligns with the approach of reusing tokens and supports the PR objective of enabling multiple clients to share tokens.
91-93: Good defensive programming with assertionThis assertion ensures that users don't accidentally attempt searches without setting an auth token. The error message clearly explains what needs to be done.
101-111: Improved parameter handlingThe new approach of creating a dictionary with all parameters and then filtering out None values is cleaner and more maintainable than the previous implementation.
Also applies to: 113-114
115-115: Consistent token usage in headersUsing the auth_token directly in the headers is consistent with the new token management approach.
tests/cassettes/test_create_token.yaml (3)
1-46: Token Creation Interaction – Validation of GET /api/token Request and ResponseThe first interaction correctly simulates a token creation request. The GET request to the
/api/tokenendpoint with aclient_idquery parameter is formatted as expected, and the response simulates a successful token retrieval with a 200 status code and appropriate headers.
47-91: Search Endpoint Interaction – Successful Search ResultsThe second interaction tests the search functionality using the
/api/findendpoint. The request parameters (name, percent, username) and headers are correctly defined, and the response includes a JSON string with expected search results and a 200 status code. Ensure that the dummy data provided truly simulates an expected successful response from the real API.
92-93: Versioning TagThe version tag is included at the end, which is good for cassette version control.
tests/cassettes/test_internal_server_error.yaml (2)
1-18: Internal Server Error Simulation – Proper Request RedirectionThe GET request on line 18 now targets the search endpoint (
/api/find) with a test case that simulates an internal server error. The updated URI and modified request headers (i.e.,accept-encodingnow set togzip, deflate) are appropriate.
19-31: Error Response Verification – Internal Server ErrorThe response block correctly returns a JSON string indicating failure with status code 500 and an "Internal Server Error" message. The simplified headers (Content-Length and Content-Type) match the expected minimal error response.
tests/cassettes/test_block_account.yaml (2)
1-18: Blocked Account Scenario – Request Structure ValidationThe request is now aligned with the new authentication flow by targeting the
/api/findendpoint. The headers, includingaccept-encodingset togzip, deflate, are set appropriately.
19-31: Blocked Account Response – Unauthorized Access SimulationThe response block simulates an unauthorized scenario for a blocked account correctly by returning a 401 status code with an appropriate error message in the JSON body. The Content-Length and Content-Type headers are correctly provided.
tests/cassettes/test_search_not_found.yaml (3)
1-18: Search Not Found – Request Setup and URI FormattingThe request is set to target the
/api/findendpoint and simulate a "no matches found" scenario. Note that the query parameter fornameincludes a trailing plus (Pepito+Cuenca+); please verify if this is intentional or if it should be corrected to remove the extra+.
19-32: Search Not Found – Response Consistency and HeadersThe response block returns a JSON string indicating no matches with a 200 status code, and the headers (including the updated Date header) seem to be formatted correctly.
33-48: Additional Response MetadataAdditional headers such as
Access-Control-Allow-Origin,Cache-Control, and connection settings are properly included to mimic an actual API response scenario.tests/cassettes/test_invalid_token.yaml (2)
1-18: Invalid Token – Request ValidationThe request block is correctly configured to target the
/api/findendpoint using the same header structure as other interactions.
19-44: Invalid Token – Response Body and Headers CheckThe response returns a JSON body indicating that the provided token is invalid. The headers are comprehensive and align with the structure seen in similar tests.
tests/test_client.py (6)
2-2: Good addition of the os module.This is necessary for accessing environment variables for authentication tokens.
18-19: Properly retrieving environment variables for authentication.The code correctly retrieves the required environment variables for the quienesquien client authentication, following good security practices by using environment variables for credentials.
63-65: Good simplification of the invalid token test.The test has been properly simplified to directly set an invalid auth token instead of using a more complex approach. This aligns with the new token management approach described in the PR objectives.
150-156: Well-structured token creation test.This new test effectively verifies that a token can be created and successfully used for searches, which is a core part of the new authentication functionality introduced in this PR.
160-173: Appropriate update to test error handling during token creation.The test has been correctly updated to use client_without_token, which properly tests the error handling when token creation fails.
187-194: Good test for client behavior without a token.This test properly verifies that the client raises an appropriate error when trying to perform a search without first setting an auth token, ensuring the new authentication flow is properly enforced.
tests/cassettes/test_invalid_plan.yaml (1)
18-18: Correctly updated test fixture to use direct API access.The URI has been properly updated to target the find endpoint directly with the necessary parameters, reflecting the new authentication approach that uses a predefined token rather than generating one.
tests/cassettes/test_search_with_params.yaml (2)
18-18: Properly updated test fixture to include all search parameters.The URI has been correctly updated to include all the search parameters (birthday, curp, list, name, etc.) for the direct API call, aligning with the new authentication approach using predefined tokens.
41-41: Updated timestamp.The date has been updated to reflect the new test recording.
tests/cassettes/test_search_by_curp.yaml (2)
18-18: Correctly updated test fixture for CURP-based search.The URI has been properly updated to target the find endpoint with CURP-specific parameters, reflecting the new direct API access using predefined authentication tokens.
41-41: Updated timestamp.The date has been updated to reflect the new test recording.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
quienesquien/client.py (1)
41-57: Well-implemented token creation methodThe
create_tokenclass method is well-implemented as a class method, which is appropriate since it doesn't need instance state. This allows creating a token without instantiating a client first.However, according to a previous review, there was a suggestion to have the method store the token in the instance. Since this is now a class method, that approach isn't applicable, but you might want to consider adding a convenience method that both creates and assigns the token in one step for instance use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
README.md(4 hunks)quienesquien/client.py(4 hunks)tests/conftest.py(1 hunks)tests/test_client.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.py`: Enforce Relative Imports for Internal Modules
Ensure that any imports referencing internal modules use relative paths. However, if modules reside in the main module dir...
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
tests/test_client.pyquienesquien/client.py
`**/*.py`:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic...
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
tests/test_client.pyquienesquien/client.py
🧬 Code Definitions (2)
tests/test_client.py (2)
tests/conftest.py (1)
client(13-14)quienesquien/client.py (3)
Client(20-151)create_token(42-57)search(59-151)
quienesquien/client.py (1)
quienesquien/exc.py (2)
QuienEsQuienError(1-4)InvalidSearchCriteriaError(7-10)
🔇 Additional comments (14)
quienesquien/client.py (5)
23-23: Good design choice making auth_token publicMaking auth_token public instead of private is a good design decision that aligns with the PR objective of allowing multiple clients to share the same token. This enables reusing existing tokens across different client instances.
32-32: LGTM: Improved type annotation flexibilityChanging the params type from
Mapping[str, str | int | None]toMapping[str, Any]provides more flexibility for different parameter types that might be passed to the request.
88-90: Good validation with clear error messageThe assertion with a descriptive error message clearly communicates to users that they must provide or create an auth token before performing searches.
98-108: Improved parameter handlingThe parameters are now properly organized and structured, making the code more readable and maintainable. The conditional logic for handling different parameter types is clear and concise.
110-112: Clean parameter filtering and authorization handlingFiltering out None values from parameters and setting up the authorization header with the auth token is done cleanly and efficiently.
README.md (4)
9-13: Excellent warning about token invalidationThe important notice clearly warns users about token invalidation behavior and its potential impact on multiple applications. This is crucial information that helps users avoid unexpected issues.
41-52: Well-documented token generation processThis new section provides clear instructions on how to create and reuse authentication tokens. The example code is concise and demonstrates the proper use of the
create_tokenmethod.
68-73: Clear example of client initialization with tokenThe updated example demonstrates the proper way to initialize the client with a token, making it easy for users to understand the new authentication flow.
96-114: Good documentation of environment variable usageThis section clearly explains how to use environment variables for token management, which directly addresses the PR objective. The use of
os.environ.get()for the optional token correctly implements feedback from past review comments.tests/test_client.py (5)
18-20: Well-structured environment variable accessGood approach using environment variables directly for tests. This makes the tests more maintainable and follows the snake_case naming convention.
64-66: Simplified token validation testThe test for invalid tokens has been simplified by directly setting the public
auth_tokenproperty. This is cleaner and more direct than the previous approach.
151-155: Good test coverage for token creationThis test thoroughly validates the token creation functionality by creating a token, initializing a client with it, and verifying that it works for searching. This covers the complete flow for the new token management approach.
159-170: Proper error handling test for token creationThe test correctly validates error handling during token creation by mocking the error response and checking that the appropriate exception is raised.
184-191: Essential validation for token requirementThis test is crucial as it verifies that clients without a token cannot perform searches. It checks both the exception type and the error message, ensuring that users get clear guidance on how to fix the issue.
Description
This PR introduces support for providing an authentication token via environment variables, allowing multiple clients to share the same token without generating a new one each time.
Changes
Why?
Previously, the client generated and stored a single token for reuse. However, if another client generated a new token, the old one became invalid, causing authentication failures. This update ensures that deployments using multiple containers can share a token without conflicts.
Summary by CodeRabbit
Personclass.