Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Added Extensive Loggers #95

Merged
merged 7 commits into from
Oct 14, 2024
Merged

feat: Added Extensive Loggers #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7dc9acf
feat: Added Extensive Loggers
Ashutosh619-sudo Oct 3, 2024
b67eb29
better logging
Ashutosh619-sudo Oct 8, 2024
f8343fe
comment resolved
Ashutosh619-sudo Oct 13, 2024
1ee9134
bump version
Ashutosh619-sudo Oct 13, 2024
d078f5e
comment resolved
Ashutosh619-sudo Oct 14, 2024
1409af4
Merge branch 'master' into add-extensive-loggers
Ashutosh619-sudo Oct 14, 2024
758f507
removed level
Ashutosh619-sudo Oct 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions sageintacctsdk/apis/api_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
API Base class with util functions
"""
import json
import logging
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve logger initialization

Good addition of logging functionality. However, consider the following improvements:

  1. Use setLevel method instead of directly setting the level attribute:
logger = logging.getLogger(__name__)
logger.setLevel(logging.WARNING)
  1. Consider allowing the log level to be configurable, perhaps through an environment variable or a configuration file.

Also applies to: 21-22

import datetime
import uuid
from warnings import warn
Expand All @@ -17,6 +18,8 @@
from .constants import dimensions_fields_mapping


logger = logging.getLogger(__name__)

class ApiBase:
"""The base class for all API classes."""

Expand Down Expand Up @@ -186,6 +189,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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve logging security and efficiency.

The addition of logging for request payloads and responses is beneficial for debugging. However, consider the following improvements:

  1. Sensitive data protection: As mentioned in a previous review, logging entire payloads and responses may expose sensitive information. Implement data sanitization before logging.

  2. 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.

  3. 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

raw_response = self.__post_request_for_raw_response(dict_body, api_url)
try:
parsed_xml = xmltodict.parse(raw_response.text, force_list={self.__dimension})
Expand All @@ -196,6 +200,17 @@ def __post_request(self, dict_body: dict, api_url: str):
parsed_response = json.loads(json.dumps(parsed_xml))

if raw_response.status_code == 200:
response = parsed_response.get('response', {})
control_status = response.get('control', {}).get('status', '')
auth_status = response.get('operation', {}).get('authentication', {}).get('status', '')
result_status = response.get('operation', {}).get('result', {}).get('status', '')


if control_status == 'failure' or auth_status == 'failure' or result_status == 'failure':
logger.info('Response for post request: %s', raw_response.text)
else:
logger.debug('Response for post request: %s', raw_response.text)

if parsed_response['response']['control']['status'] == 'success':
api_response = parsed_response['response']['operation']

Expand Down Expand Up @@ -235,6 +250,8 @@ def __post_request(self, dict_body: dict, api_url: str):
}
raise WrongParamsError('Something went wrong', custom_response)


logger.info('Response for post request: %s', raw_response.text)
Comment on lines +253 to +254
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)

if 'result' in parsed_response:
if 'errormessage' in parsed_response['result']:
parsed_response = parsed_response['result']['errormessage']
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

setuptools.setup(
name='sageintacctsdk',
version='1.23.2',
version='1.23.3',
author='Ashwin T',
author_email='[email protected]',
description='Python SDK for accessing Sage Intacct APIs',
Expand Down