-
Notifications
You must be signed in to change notification settings - Fork 3
implemented API_KEY status Code feature #1122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: testing
Are you sure you want to change the base?
Conversation
5e5f740 to
79b259d
Compare
79b259d to
db175b2
Compare
|
/windsurf-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other comments (12)
- src/services/utils/common_utils.py (169-169) The 'images' field assignment has lost its fallback logic. Previously it would use body.get('images') OR extract image URLs from user_urls if images was None. Now it only uses body.get('images') which could result in None values where previously there would be extracted URLs.
- src/services/utils/common_utils.py (860-863) The new update_cost_usage_and_apikey_status_in_background function only updates cost if completion_success is True, whereas the previous implementation would update cost regardless of success status. This could change existing behavior if cost tracking for failed completions was expected.
- src/db_services/ConfigurationServices.py (271-324) This PR adds a new API_KEY status code feature with complex MongoDB aggregation logic. Ensure there are appropriate tests to verify this functionality works correctly with different data scenarios (null values, missing fields, etc.).
- src/services/commonServices/common.py (466-466) This function uses `print()` for error logging while the rest of the file uses the logger. For consistency and better observability, consider using the logger here as well.
- src/db_services/ConfigurationServices.py (279-293) The code adds a 'status' field to the API key projection in Stage 4, but this field isn't included in the apikeys object mapping in Stage 5. For consistency, consider adding the status field to the apikeys object as well, similar to how it's handled in the apikey_status object.
-
src/db_services/api_key_status_service.py (7-19)
Consider returning a boolean or result object to indicate success/failure of the operation. This would help callers handle errors appropriately.
async def update_apikey_status(apikey_id: str, status: str) -> bool: if not apikey_id: return False try: result = await apikeyCredentialsModel.update_one( {"_id": ObjectId(apikey_id)}, {"$set": {"status": status}} ) if not result.modified_count: logger.warning(f"No apikey credential updated for id={apikey_id}") return False return True except Exception as exc: logger.error(f"Failed to update API key status for {apikey_id}: {exc}") return False - src/db_services/api_key_status_service.py (8-9) The function silently returns when `apikey_id` is empty. Consider logging this case or raising a specific exception to make it more explicit for callers.
- src/services/utils/getConfiguration.py (81-81) The `apikey_status` field is being added to the configuration response, but there's no validation or default value handling. Consider adding a default value (like `None`) to ensure consistent behavior when this field is not present in the bridge data.
- src/db_services/ConfigurationServices.py (271-323) This section contains complex nested MongoDB aggregation operations. Consider adding comments explaining the purpose of this apikey_status mapping logic to improve maintainability.
- src/db_services/api_key_status_service.py (7-7) The function should validate the `status` parameter to ensure only valid status values are stored in the database. Consider adding validation or using an enum for status values.
-
src/services/utils/api_key_status_helper.py (5-5)
There's a typo in the STATUS_BY_CODE dictionary: 'exhuasted' should be 'exhausted'.
"429": "exhausted", - src/services/utils/api_key_status_helper.py (28-28) Add a newline at the end of the file to follow standard file formatting conventions.
💡 To request another review, post a new comment with "/windsurf-review".
| # Handle exceptions during execution | ||
| execution_failed = True | ||
| original_error = str(execution_exception) | ||
| first_execution_error_code = original_error.split()[7] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error code extraction on line 247 assumes a specific error message format and could fail with an IndexError if the error message doesn't contain at least 8 space-separated elements. Consider adding a try-except block or using a more robust method like regex pattern matching.
| "429": "exhuasted", | ||
| } | ||
|
|
||
| def classify_status_from_error(code) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function has a syntax error in the type annotation. Python uses ':' not '->' for type annotations.
| def classify_status_from_error(code) -> str: | |
| def classify_status_from_error(code) -> str: |
| if code in STATUS_BY_CODE: | ||
| return STATUS_BY_CODE[code] | ||
|
|
||
| return code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function returns code directly if it's not in STATUS_BY_CODE, but this could be a non-string value which doesn't match the return type annotation. Consider adding a default status or converting to string.
No description provided.