feat: Added Extensive Loggers #95
Conversation
WalkthroughThe changes in the pull request focus on enhancements to the Changes
Poem
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 (
|
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
![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)
sageintacctsdk/apis/api_base.py (3)
Line range hint
47-50: Initializeself.__show_privatein the constructorThe attribute
self.__show_privateis used informat_and_send_request, but it is not initialized in the constructor. Ifset_show_privateis not called beforeformat_and_send_request, this could lead to anAttributeError. Consider initializingself.__show_privatein the__init__method with a default value, such asFalse.
Line range hint
266-270: Avoid silently passing exceptionsIn the
if self.__show_private:block, catching aKeyErrorand passing silently can mask underlying issues. Instead of using a try-except block, consider checking if the necessary keys exist before assigningoptions. For example:if self.__show_private: function_dict = dict_body['request']['operation']['content']['function'] if 'query' in function_dict: options = {'showprivate': True} function_dict['query']['options'] = optionsThis approach ensures that you only attempt to set
optionswhen the'query'key exists, making the code more robust and maintainable.
Line range hint
47-52: Enhance the docstring forset_show_privateTo improve clarity, consider enhancing the docstring for
set_show_privateto better explain its purpose and usage. For example:def set_show_private(self, show_private: bool): """ Enable or disable the inclusion of private data in API responses. :param show_private: If True, include private data in the API request options. :return: None """This provides a clearer understanding of the method's functionality for future reference.
sageintacctsdk/apis/api_base.py
Outdated
| logger = logging.getLogger(__name__) | ||
| logger.level = logging.WARNING |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use setLevel method to set the logger level
Instead of setting the logger level directly with logger.level = logging.WARNING, consider using the setLevel method:
logger.setLevel(logging.WARNING)This is the standard way to configure the logging level and ensures better compatibility with logging configurations.
sageintacctsdk/apis/api_base.py
Outdated
| @@ -195,6 +200,8 @@ def __post_request(self, dict_body: dict, api_url: str): | |||
| parsed_xml = xmltodict.parse(raw_response, force_list={self.__dimension})['root'] | |||
| parsed_response = json.loads(json.dumps(parsed_xml)) | |||
|
|
|||
|
|
|||
| logger.debug('Response for post request: %s', raw_response.text) | |||
There was a problem hiding this comment.
Avoid logging sensitive information in response
Similarly, logging the entire response with logger.debug('Response for post request: %s', raw_response.text) might expose sensitive information returned from the API. It's advisable to sanitize the response or limit the logging to essential non-sensitive information to enhance security.
|
PR description must contain a link to a ClickUp (case-insensitive) |
1 similar comment
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
sageintacctsdk/apis/api_base.py (4)
5-5: LGTM! Consider usingsetLevelmethod for setting logger level.The addition of logging functionality is a good practice. However, as suggested in a previous review, consider using the
setLevelmethod instead of directly setting the logger level:logger = logging.getLogger(__name__) logger.setLevel(logging.WARNING)This is the standard way to configure the logging level and ensures better compatibility with logging configurations.
Also applies to: 21-22
Line range hint
53-59: LGTM! Consider adding a docstring for clarity.The
set_show_privatemethod is a good addition for controlling the visibility of private data. To improve clarity, consider adding a docstring explaining the purpose of the__show_privateattribute and its impact on the API behavior.Example:
def set_show_private(self, show_private: bool): """ Set whether to show private data in API requests. :param show_private: If True, include private data in requests. """ self.__show_private = show_private
Line range hint
196-200: LGTM! Consider enhancing error handling and logging.The addition of XML parsing error handling is a good improvement for dealing with potentially malformed responses from Sage Intacct. To further enhance this:
- Consider logging the parsing error for debugging purposes.
- You might want to raise a custom exception or add a warning log to track the frequency of these malformed responses.
Example:
try: parsed_xml = xmltodict.parse(raw_response.text, force_list={self.__dimension}) except Exception as e: logger.warning(f"Received malformed XML from Sage Intacct: {str(e)}") raw_response = '<root>' + raw_response.text + '</root>' parsed_xml = xmltodict.parse(raw_response, force_list={self.__dimension})['root']
Line range hint
324-328: LGTM! Consider improving clarity of the conditional logic.The addition of the
showprivateoption based on the__show_privateattribute is a good feature. To improve clarity and robustness, consider the following suggestions:
Instead of using a try-except block, you could use a more explicit conditional check:
if self.__show_private and 'query' in dict_body['request']['operation']['content']['function']: dict_body['request']['operation']['content']['function']['query']['options'] = {'showprivate': True}Add a comment explaining the purpose of this feature and when it should be used.
These changes will make the code more readable and self-explanatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sageintacctsdk/apis/api_base.py (5 hunks)
🧰 Additional context used
🔇 Additional comments (1)
sageintacctsdk/apis/api_base.py (1)
Line range hint
1-600: Overall improvements to ApiBase classThe changes made to the
ApiBaseclass in this pull request significantly enhance its functionality and robustness:
- Addition of logging improves debuggability.
- Enhanced XML parsing error handling increases resilience against malformed responses.
- The new
set_show_privatemethod and related logic provide flexibility in handling private data.These improvements will make the SDK more reliable and easier to work with. The suggested minor adjustments in the previous comments will further refine these enhancements.
Great work on these updates!
| @@ -186,6 +190,7 @@ def __post_request(self, dict_body: dict, api_url: str): | |||
| A response from the request (dict). | |||
| """ | |||
|
|
|||
| logger.debug('Payload for post request: %s', dict_body) | |||
There was a problem hiding this comment.
Improve logging security and efficiency.
The addition of logging for request payloads and responses is beneficial for debugging. However, consider the following improvements:
-
Sensitive data protection: As mentioned in a previous review, logging entire payloads and responses may expose sensitive information. Implement data sanitization before logging.
-
Avoid redundant logging: The debug log on line 204 and info log on line 245 both log the response. Consider removing one of them to avoid redundancy.
-
Use appropriate log levels: Ensure that debug logs are only enabled in non-production environments to prevent performance issues and potential data leaks.
Example of sanitized logging:
logger.debug('Payload for post request: %s', self._sanitize_data(dict_body))
logger.info('Response status: %s', raw_response.status_code)Implement a _sanitize_data method to remove or mask sensitive information before logging.
Also applies to: 204-204, 245-245
sageintacctsdk/apis/api_base.py
Outdated
| @@ -196,6 +201,7 @@ def __post_request(self, dict_body: dict, api_url: str): | |||
| parsed_response = json.loads(json.dumps(parsed_xml)) | |||
|
|
|||
| if raw_response.status_code == 200: | |||
| logger.debug('Response for post request: %s', raw_response.text) | |||
There was a problem hiding this comment.
if (parsed_response['response']['control']['status'] == 'failure' or api_response['authentication']['status'] == 'failure' or api_response['result']['status'] == 'failure'):
logger.info
else:
logger.debug
sageintacctsdk/apis/api_base.py
Outdated
| @@ -196,6 +201,12 @@ def __post_request(self, dict_body: dict, api_url: str): | |||
| parsed_response = json.loads(json.dumps(parsed_xml)) | |||
|
|
|||
| if raw_response.status_code == 200: | |||
| if (parsed_response['response']['control']['status'] == 'failure' or parsed_response['response']['operation']['authentication']['status'] == 'failure' or \ | |||
There was a problem hiding this comment.
handle all keys safely, they may not exist in all case. do it like parsed_response.get('response', {}).get('control', {}).get('status', {})
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
sageintacctsdk/apis/api_base.py (1)
204-213: Approve changes with a minor suggestionThe improvements in error handling and conditional logging are good. The use of
getmethod with default values for safely accessing nested dictionary keys is a best practice.Consider applying this approach consistently to all dictionary accesses in this block:
result_status = response.get('operation', {}).get('result', {}).get('status', '')This ensures uniform error handling throughout the method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sageintacctsdk/apis/api_base.py (5 hunks)
🧰 Additional context used
🔇 Additional comments (4)
sageintacctsdk/apis/api_base.py (4)
Line range hint
193-214: Improve logging security and efficiencyWhile the addition of logging is beneficial for debugging, consider the following improvements:
Sensitive data protection: Implement data sanitization before logging payloads and responses to prevent exposure of sensitive information.
Use appropriate log levels: Ensure that debug logs are only enabled in non-production environments to prevent performance issues and potential data leaks.
Example of sanitized logging:
logger.debug('Payload for post request: %s', self._sanitize_data(dict_body)) logger.info('Response status: %s', raw_response.status_code)Implement a
_sanitize_datamethod to remove or mask sensitive information before logging.
Line range hint
52-58: Approve addition ofset_show_privatemethodThe new
set_show_privatemethod is a good addition to theApiBaseclass. It provides more flexibility in controlling the visibility of private data in API requests. The implementation is correct and consistent with the class's coding style.Make sure to document this new functionality in the class or module docstring to inform users of this new capability.
Line range hint
1-600: Overall improvements enhance code robustnessThe changes made to this file, including the addition of logging, improved error handling, and the new
set_show_privatemethod, significantly enhance the robustness and flexibility of theApiBaseclass. These improvements will aid in debugging and provide more control over API interactions.Consider the following to further improve the code:
- Add comprehensive docstrings for new methods and update existing ones to reflect changes.
- Implement a data sanitization method for logging to ensure sensitive information is not exposed.
- Review the entire class to ensure consistent use of safe dictionary access methods (
getwith default values) throughout.These changes represent a positive step in the evolution of this SDK.
5-5: 🛠️ Refactor suggestionUse
setLevelmethod for setting logger levelThe addition of logging is a good practice. However, consider using the
setLevelmethod instead of directly setting the level:-logger.level = logging.WARNING +logger.setLevel(logging.WARNING)This is the standard way to configure the logging level and ensures better compatibility with logging configurations.
Also applies to: 21-22
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
sageintacctsdk/apis/api_base.py (4)
Line range hint
193-214: Enhance logging security and error handlingThe improvements in logging and error handling are commendable. However, consider the following enhancements:
- Implement data sanitization before logging to prevent exposure of sensitive information:
def sanitize_data(data): # Implement logic to remove or mask sensitive fields return sanitized_data logger.debug('Payload for post request: %s', sanitize_data(dict_body))
- Consider using a try-except block for XML parsing instead of a bare except:
try: parsed_xml = xmltodict.parse(raw_response.text, force_list={self.__dimension}) except xml.parsers.expat.ExpatError: # Handle specific XML parsing errors raw_response = '<root>' + raw_response.text + '</root>' parsed_xml = xmltodict.parse(raw_response, force_list={self.__dimension})['root']
- Use
logging.exception()to log the full stack trace in case of exceptions.
204-213: Approve changes with suggestions for further improvementThe improvements in response parsing and conditional logging are good. However, consider the following enhancements:
- Further sanitize the logged response to exclude sensitive information:
def sanitize_response(response): # Implement logic to remove or mask sensitive fields return sanitized_response if control_status == 'failure' or auth_status == 'failure' or result_status == 'failure': logger.info('Response status: %s', sanitize_response(raw_response.text)) else: logger.debug('Response status: %s', sanitize_response(raw_response.text))
- Consider using an enumeration for status values to avoid magic strings:
from enum import Enum class Status(Enum): SUCCESS = 'success' FAILURE = 'failure' if control_status == Status.FAILURE.value or auth_status == Status.FAILURE.value or result_status == Status.FAILURE.value: # ... (logging logic)
Line range hint
51-57: Approve new method with suggestion for input validationThe addition of the
set_show_privatemethod is a good way to control the visibility of private data. Consider adding input validation:def set_show_private(self, show_private: bool): """ Set the show private for APIs :param show_private: boolean :return: None """ if not isinstance(show_private, bool): raise ValueError("show_private must be a boolean") self.__show_private = show_privateThis ensures that only boolean values are accepted for the
show_privateparameter.
Line range hint
336-341: Approve changes with suggestion for improved exception handlingThe addition of the
showprivateoption is a good way to control the visibility of private data in API requests. However, the exception handling could be improved:
- Use a more specific exception type instead of a bare except:
if self.__show_private: try: dict_body['request']['operation']['content']['function']['query']['options'] = {'showprivate': True} except KeyError: # The 'query' key is not present in the function dictionary pass
- Consider adding the 'options' key if it doesn't exist, instead of catching an exception:
if self.__show_private: function = dict_body['request']['operation']['content']['function'] if 'query' in function: function['query'].setdefault('options', {})['showprivate'] = TrueThis approach is more explicit and avoids the use of exceptions for control flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sageintacctsdk/apis/api_base.py (5 hunks)
- setup.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- setup.py
🧰 Additional context used
🔇 Additional comments (1)
sageintacctsdk/apis/api_base.py (1)
Line range hint
1-577: Overall improvements are commendableThe changes made to this file significantly enhance the
ApiBaseclass:
- Addition of logging improves debuggability.
- Improved error handling, especially for XML parsing, increases robustness.
- New functionality for managing private data visibility adds flexibility to API interactions.
Consider the following final suggestions:
- Add unit tests for the new and modified methods to ensure their correctness.
- Update the class and method docstrings to reflect the new functionality and parameters.
- Consider adding a changelog to track these significant improvements.
Great work on these enhancements!
| @@ -2,6 +2,7 @@ | |||
| API Base class with util functions | |||
| """ | |||
| import json | |||
| import logging | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logger initialization
Good addition of logging functionality. However, consider the following improvements:
- Use
setLevelmethod instead of directly setting thelevelattribute:
logger = logging.getLogger(__name__)
logger.setLevel(logging.WARNING)- Consider allowing the log level to be configurable, perhaps through an environment variable or a configuration file.
Also applies to: 21-22
|
|
||
| logger.info('Response for post request: %s', raw_response.text) |
There was a problem hiding this comment.
Remove redundant logging
This logging statement is redundant with the logging added earlier in the method. Additionally, logging the entire response text might expose sensitive information. Consider removing this line or modifying it to log only non-sensitive information:
logger.info('Response status code: %s', raw_response.status_code)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Chores
https://app.clickup.com/1864988/v/l/6-901603904304-1