feat: Log post requests and sdk changes #400
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes introduce a new middleware class, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Outside diff range and nitpick comments (3)
fyle_xero_api/settings.py (2)
152-152: LGTM: Logging configuration improvedThe changes to the logging configuration are well-structured and align with the PR objectives:
- Setting
disable_existing_loggersto False is a good practice, ensuring all loggers remain active.- The new "standard" formatter provides consistent log formatting.
- The "console" handler with the "standard" formatter ensures uniform log output.
- The new logger for "xerosdk.apis.api_base" at INFO level will capture SDK changes as intended.
These improvements will enhance the application's logging capabilities and make debugging easier.
Consider adding a comment explaining the purpose of the "xerosdk.apis.api_base" logger to improve code maintainability.
Also applies to: 162-165, 178-181, 212-216
Line range hint
301-301: Consider aligning environment variable name with setting nameThe setting
XERO_APP_URLis currently set toos.environ.get("APP_URL"). To improve clarity and maintain consistency, consider either:
- Renaming the environment variable to
XERO_APP_URL, or- Renaming the setting to
APP_URLThis alignment will reduce potential confusion and make the code more maintainable.
fyle_xero_api/logging_middleware.py (1)
41-42: Uselogger.exception()to capture full stack traceWhen handling exceptions, using
logger.exception()is more appropriate as it automatically includes the stack trace in the logs. This provides more context for debugging compared tologger.info().Apply this diff to use
logger.exception():- logger.info('Something went wrong when logging post call - %s', e) + logger.exception('An exception occurred while logging POST/PUT request')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- fyle_xero_api/logging_middleware.py (2 hunks)
- fyle_xero_api/settings.py (5 hunks)
- requirements.txt (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🧰 Additional context used
🔇 Additional comments (4)
fyle_xero_api/settings.py (3)
61-61: LGTM: LogPostRequestMiddleware added correctlyThe addition of
LogPostRequestMiddlewareat the beginning of the middleware list is appropriate. This placement ensures that incoming POST and PUT requests are logged before any other middleware processes them, aligning with the PR objective.
Line range hint
302-303: LGTM: New integration URLs addedThe addition of
XERO_INTEGRATION_APP_URLandINTEGRATIONS_APP_URLsettings is noted. These seem to be related to the integration of Xero and potentially other services.Could you provide more context on how these URLs will be used in the application? This information would be helpful for future maintenance and understanding the system architecture.
Line range hint
1-338: Summary: Changes align with PR objectives and improve loggingOverall, the changes to
settings.pyare well-implemented and align with the PR objectives:
- The addition of
LogPostRequestMiddlewareenables logging of POST and PUT requests.- Updates to the logging configuration improve log consistency and capture SDK changes.
- New integration-related settings have been added.
These changes will enhance the application's logging capabilities and support integration with Xero. A few minor suggestions have been made to improve clarity and maintainability.
fyle_xero_api/logging_middleware.py (1)
30-44: Ensure middleware is properly integrated and orderedFor the middleware to function correctly, it must be added to the
MIDDLEWARElist insettings.py. Additionally, the order of middleware can affect processing; ensure thatLogPostRequestMiddlewareis placed appropriately, especially in relation to other middleware that might modify the request.Run the following script to confirm the middleware is included and check its position:
✅ Verification successful
Middleware is properly integrated in
fyle_xero_api/settings.py.The
LogPostRequestMiddlewareis listed in theMIDDLEWAREconfiguration at line 61 offyle_xero_api/settings.py. Ensure that its position relative to other middleware components aligns with the desired request processing flow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that LogPostRequestMiddleware is present in MIDDLEWARE setting. # Test: Search for LogPostRequestMiddleware in settings.py. Expect: Middleware is listed. grep -rn 'MIDDLEWARE' settings.py -A 20 | grep 'LogPostRequestMiddleware'Length of output: 116
Script:
#!/bin/bash # Description: Find all settings.py files and verify LogPostRequestMiddleware is included in MIDDLEWARE. # Find all settings.py files in the repository settings_files=$(fd 'settings\.py' .) # Check each settings.py for LogPostRequestMiddleware in MIDDLEWARE for file in $settings_files; do echo "Checking $file for LogPostRequestMiddleware..." grep -rn 'LogPostRequestMiddleware' "$file" doneLength of output: 671
| import logging | ||
| import traceback | ||
|
|
||
| from django.conf import settings | ||
| from django.http import HttpResponse | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| logger.level = logging.INFO |
There was a problem hiding this comment.
Avoid setting logger level directly in code
Setting the logger level directly within the code overrides the logging configuration specified in your Django settings, which can lead to inconsistent logging behavior across the application. It's better to configure logging levels using the LOGGING setting in settings.py.
Apply this diff to remove the direct setting of the logger's level:
-logger.level = logging.INFO📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.level = logging.INFO |
| if request.method in ['POST', 'PUT']: | ||
| try: | ||
| body_unicode = request.body.decode('utf-8') | ||
| request_body = json.loads(body_unicode) | ||
| logger.info("POST request to %s: %s", request.path, request_body) |
There was a problem hiding this comment.
Potential security risk: Logging request bodies may expose sensitive data
Logging full request bodies for POST and PUT requests can inadvertently capture sensitive information such as passwords, tokens, or personally identifiable information (PII). This can lead to security vulnerabilities and compliance issues.
Consider sanitizing the request body before logging or avoid logging the body entirely. If logging is necessary, ensure sensitive data is masked or excluded.
Apply this diff to log only the request path and method:
- logger.info("POST request to %s: %s", request.path, request_body)
+ logger.info("Received %s request to %s", request.method, request.path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if request.method in ['POST', 'PUT']: | |
| try: | |
| body_unicode = request.body.decode('utf-8') | |
| request_body = json.loads(body_unicode) | |
| logger.info("POST request to %s: %s", request.path, request_body) | |
| if request.method in ['POST', 'PUT']: | |
| try: | |
| body_unicode = request.body.decode('utf-8') | |
| request_body = json.loads(body_unicode) | |
| logger.info("Received %s request to %s", request.method, request.path) |
| if request.method in ['POST', 'PUT']: | ||
| try: | ||
| body_unicode = request.body.decode('utf-8') | ||
| request_body = json.loads(body_unicode) | ||
| logger.info("POST request to %s: %s", request.path, request_body) | ||
| except (json.JSONDecodeError, UnicodeDecodeError): | ||
| logger.warning("Failed to decode POST request body for %s", request.path) | ||
| except Exception as e: | ||
| logger.info('Something went wrong when logging post call - %s', e) | ||
| response = self.get_response(request) |
There was a problem hiding this comment.
Check 'Content-Type' header before parsing request body as JSON
Attempting to parse the request body as JSON without verifying the 'Content-Type' may result in errors if the body is not JSON-formatted (e.g., form data, XML). This can cause unnecessary exceptions and logging noise.
Modify the code to check if the 'Content-Type' is 'application/json' before attempting to parse:
if request.method in ['POST', 'PUT']:
+ if request.content_type == 'application/json':
try:
body_unicode = request.body.decode('utf-8')
request_body = json.loads(body_unicode)
- logger.info("POST request to %s: %s", request.path, request_body)
+ logger.info("Received JSON %s request to %s: %s", request.method, request.path, request_body)
except (json.JSONDecodeError, UnicodeDecodeError):
logger.warning("Failed to decode JSON request body for %s", request.path)
except Exception as e:
- logger.info('Something went wrong when logging post call - %s', e)
+ logger.exception('An exception occurred while logging JSON request body')
+ else:
+ logger.info("Received %s request to %s with non-JSON content", request.method, request.path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if request.method in ['POST', 'PUT']: | |
| try: | |
| body_unicode = request.body.decode('utf-8') | |
| request_body = json.loads(body_unicode) | |
| logger.info("POST request to %s: %s", request.path, request_body) | |
| except (json.JSONDecodeError, UnicodeDecodeError): | |
| logger.warning("Failed to decode POST request body for %s", request.path) | |
| except Exception as e: | |
| logger.info('Something went wrong when logging post call - %s', e) | |
| response = self.get_response(request) | |
| if request.method in ['POST', 'PUT']: | |
| if request.content_type == 'application/json': | |
| try: | |
| body_unicode = request.body.decode('utf-8') | |
| request_body = json.loads(body_unicode) | |
| logger.info("Received JSON %s request to %s: %s", request.method, request.path, request_body) | |
| except (json.JSONDecodeError, UnicodeDecodeError): | |
| logger.warning("Failed to decode JSON request body for %s", request.path) | |
| except Exception as e: | |
| logger.exception('An exception occurred while logging JSON request body') | |
| else: | |
| logger.info("Received %s request to %s with non-JSON content", request.method, request.path) | |
| response = self.get_response(request) |
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
* feat: Log post requests and sdk changes * pylint resolved
https://app.clickup.com/1864988/v/l/6-901603904304-1