add retry mechanism #203
add retry mechanism #203
Conversation
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe updates enhance the Sage Desktop SDK by adding a retry mechanism to handle specific exceptions during the cookie update process and introducing a method to set cookie values directly. Additionally, the changes streamline the initialization of the SDK, particularly focusing on consistent cookie setting across various components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SDK
participant Client
participant Helpers
User ->> SDK: Initialize SDK
SDK ->> Client: update_cookie(api_key, api_secret)
Client ->> Client: update_cookie
activate Client
loop Retry on failure
Client -->> Helpers: retry decorator intercepts (if exception)
end
deactivate Client
Client ->> SDK: Return cookie
SDK ->> Client: set_cookie(cookie)
SDK ->> OtherEntities: set_cookie(cookie)
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 2
Outside diff range and nitpick comments (1)
sage_desktop_sdk/core/client.py (1)
Line range hint
93-93: Use f-strings and proper exception chaining.Replace all
formatcalls with f-strings for consistency and improved performance. Also, use exception chaining to provide more context in error scenarios.- raise SageDesktopSDKError("Error while connecting with hh2 | {0}".format(e)) + raise SageDesktopSDKError(f"Error while connecting with hh2 | {e}") from eAlso applies to: 105-105, 132-132, 135-135, 138-138, 141-141, 143-143, 153-153, 177-177, 187-187, 211-211, 220-220, 244-244
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sage_desktop_sdk/core/client.py (2 hunks)
- sage_desktop_sdk/core/helpers.py (1 hunks)
Additional context used
Ruff
sage_desktop_sdk/core/client.py
46-46: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
46-46: Use f-string instead of
formatcallConvert to f-string
(UP032)
93-93: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
93-93: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
93-93: Use f-string instead of
formatcallConvert to f-string
(UP032)
105-105: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
105-105: Use f-string instead of
formatcallConvert to f-string
(UP032)
132-132: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
135-135: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
138-138: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
141-141: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
143-143: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
143-143: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
143-143: Use f-string instead of
formatcallConvert to f-string
(UP032)
153-153: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
153-153: Use f-string instead of
formatcallConvert to f-string
(UP032)
177-177: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
177-177: Use f-string instead of
formatcallConvert to f-string
(UP032)
187-187: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
187-187: Use f-string instead of
formatcallConvert to f-string
(UP032)
211-211: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
211-211: Use f-string instead of
formatcallConvert to f-string
(UP032)
220-220: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
220-220: Use f-string instead of
formatcallConvert to f-string
(UP032)
244-244: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
244-244: Use f-string instead of
formatcallConvert to f-string
(UP032)
Additional comments not posted (2)
sage_desktop_sdk/core/client.py (2)
16-16: Import statement forretrydecorator added.The import statement is correctly placed and follows Python best practices. Ensure that the
retrymodule is correctly implemented and tested since it's critical for the functionality of retrying operations.
48-48: Application of theretrydecorator onupdate_cookie.The
retrydecorator is appropriately used here to handle potential JSON decoding errors and SDK-specific errors. This is a good use of the decorator to enhance the robustness of network operations.
sage_desktop_sdk/core/helpers.py
Outdated
| def retry(exceptions, tries=3, delay=2, backoff=2): | ||
| """ | ||
| Decorator for retrying a function call with specified exceptions. | ||
|
|
||
| :param exceptions: Exception or tuple of exceptions to catch and retry on. | ||
| :param tries: Number of times to try (not retry) before giving up. | ||
| :param delay: Initial delay between retries in seconds. | ||
| :param backoff: Multiplier applied to delay between attempts. | ||
| """ | ||
| def decorator(func): | ||
| @wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| _tries, _delay = tries, delay | ||
| while _tries > 1: | ||
| try: | ||
| return func(*args, **kwargs) | ||
| except exceptions as e: | ||
| logger.exception(f"{func.__name__} failed with {e}, retrying in {_delay} seconds...") | ||
| time.sleep(_delay) | ||
| _tries -= 1 | ||
| _delay *= backoff | ||
| return func(*args, **kwargs) | ||
| return wrapper | ||
| return decorator |
There was a problem hiding this comment.
Review of the retry decorator implementation.
The retry decorator is well-implemented with clear documentation and parameters. It correctly uses wraps from functools to maintain the metadata of the decorated functions. The use of a while loop for retries and exponential backoff are appropriate for handling temporary issues effectively.
However, consider adding a maximum delay cap to avoid excessively long wait times in scenarios where many retries are necessary.
+ max_delay=60 # Adding a maximum delay cap
- _delay *= backoff
+ _delay = min(_delay * backoff, max_delay) # Ensure delay does not exceed maximum capCommittable 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.
| def retry(exceptions, tries=3, delay=2, backoff=2): | |
| """ | |
| Decorator for retrying a function call with specified exceptions. | |
| :param exceptions: Exception or tuple of exceptions to catch and retry on. | |
| :param tries: Number of times to try (not retry) before giving up. | |
| :param delay: Initial delay between retries in seconds. | |
| :param backoff: Multiplier applied to delay between attempts. | |
| """ | |
| def decorator(func): | |
| @wraps(func) | |
| def wrapper(*args, **kwargs): | |
| _tries, _delay = tries, delay | |
| while _tries > 1: | |
| try: | |
| return func(*args, **kwargs) | |
| except exceptions as e: | |
| logger.exception(f"{func.__name__} failed with {e}, retrying in {_delay} seconds...") | |
| time.sleep(_delay) | |
| _tries -= 1 | |
| _delay *= backoff | |
| return func(*args, **kwargs) | |
| return wrapper | |
| return decorator | |
| def retry(exceptions, tries=3, delay=2, backoff=2, max_delay=60): | |
| """ | |
| Decorator for retrying a function call with specified exceptions. | |
| :param exceptions: Exception or tuple of exceptions to catch and retry on. | |
| :param tries: Number of times to try (not retry) before giving up. | |
| :param delay: Initial delay between retries in seconds. | |
| :param backoff: Multiplier applied to delay between attempts. | |
| :param max_delay: Maximum delay between retries in seconds. | |
| """ | |
| def decorator(func): | |
| @wraps(func) | |
| def wrapper(*args, **kwargs): | |
| _tries, _delay = tries, delay | |
| while _tries > 1: | |
| try: | |
| return func(*args, **kwargs) | |
| except exceptions as e: | |
| logger.exception(f"{func.__name__} failed with {e}, retrying in {_delay} seconds...") | |
| time.sleep(_delay) | |
| _tries -= 1 | |
| _delay = min(_delay * backoff, max_delay) | |
| return func(*args, **kwargs) | |
| return wrapper | |
| return decorator |
sage_desktop_sdk/core/client.py
Outdated
| @@ -44,6 +45,7 @@ def set_api_url(self, indentifier: str): | |||
| """ | |||
| self.__api_url = "https://{0}".format(indentifier) | |||
|
|
|||
| @retry((json.JSONDecodeError, SageDesktopSDKError)) | |||
There was a problem hiding this comment.
Refactor set_api_url to use f-strings for better readability.
The current implementation uses format. Converting it to an f-string would enhance readability and performance.
- self.__api_url = "https://{0}".format(indentifier)
+ self.__api_url = f"https://{indentifier}"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.
| @retry((json.JSONDecodeError, SageDesktopSDKError)) | |
| @retry((json.JSONDecodeError, SageDesktopSDKError)) |
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
sage_desktop_sdk/core/client.py (1)
Line range hint
46-46: Convert to f-string for better readability and performance.The use of
.format()is outdated, and converting it to an f-string will enhance both readability and performance.- self.__api_url = "https://{0}".format(indentifier) + self.__api_url = f"https://{indentifier}"Tools
Ruff
16-16:
sage_desktop_sdk.core.helpers.retryimported but unusedRemove unused import:
sage_desktop_sdk.core.helpers.retry(F401)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sage_desktop_sdk/core/client.py (2 hunks)
Additional context used
Ruff
sage_desktop_sdk/core/client.py
16-16:
sage_desktop_sdk.core.helpers.retryimported but unusedRemove unused import:
sage_desktop_sdk.core.helpers.retry(F401)
46-46: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
46-46: Use f-string instead of
formatcallConvert to f-string
(UP032)
91-91: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
91-91: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
91-91: Use f-string instead of
formatcallConvert to f-string
(UP032)
103-103: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
103-103: Use f-string instead of
formatcallConvert to f-string
(UP032)
130-130: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
133-133: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
136-136: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
139-139: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
141-141: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
141-141: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
141-141: Use f-string instead of
formatcallConvert to f-string
(UP032)
151-151: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
151-151: Use f-string instead of
formatcallConvert to f-string
(UP032)
175-175: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
175-175: Use f-string instead of
formatcallConvert to f-string
(UP032)
185-185: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
185-185: Use f-string instead of
formatcallConvert to f-string
(UP032)
209-209: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
209-209: Use f-string instead of
formatcallConvert to f-string
(UP032)
218-218: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
218-218: Use f-string instead of
formatcallConvert to f-string
(UP032)
242-242: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
242-242: Use f-string instead of
formatcallConvert to f-string
(UP032)
| @@ -56,8 +57,7 @@ def update_cookie(self, api_key: str, api_secret: str): | |||
| """ | |||
|
|
|||
| request_header = { | |||
| 'Accept': 'application/json', | |||
| 'Content-type': 'application/json', | |||
| 'Content-type': 'application/json' | |||
There was a problem hiding this comment.
Ensure proper exception propagation in update_cookie.
When re-raising exceptions, use raise ... from e to maintain the exception chain which helps in debugging.
- except Exception as e:
- raise SageDesktopSDKError("Error while connecting with hh2 | {0}".format(e))
+ except Exception as e:
+ raise SageDesktopSDKError("Error while connecting with hh2 | {0}".format(e)) from eCommittable suggestion was skipped due to low confidence.
sage_desktop_sdk/core/client.py
Outdated
| @@ -13,6 +13,7 @@ | |||
| SageDesktopSDKError, | |||
| InternalServerError | |||
| ) | |||
| from sage_desktop_sdk.core.helpers import retry | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Remove unused import statement.
The import statement for retry in sage_desktop_sdk/core/client.py is unused and should be removed to clean up the code.
sage_desktop_sdk/core/client.py: Remove the linefrom sage_desktop_sdk.core.helpers import retry
Analysis chain
Verify the usage of the imported retry decorator.
The import statement for retry is flagged as unused. Ensure that it is correctly applied in the update_cookie method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for 'retry' usage in 'update_cookie' method.
ast-grep --lang python --pattern $'class Client {
$$$
def update_cookie($_) {
@retry($$$)
$$$
}
$$$
}'
Length of output: 466
Script:
#!/bin/bash
# Description: Search for any usage of the 'retry' decorator in the codebase.
ast-grep --lang python --pattern '@retry($$$)'
Length of output: 48
Script:
#!/bin/bash
# Description: Search for any references to 'retry' in the codebase.
rg '\bretry\b'
Length of output: 501
Tools
Ruff
16-16:
sage_desktop_sdk.core.helpers.retryimported but unusedRemove unused import:
sage_desktop_sdk.core.helpers.retry(F401)
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
sage_desktop_sdk/core/client.py (1)
Line range hint
63-81: Improve exception handling and formatting inupdate_cookie.The method correctly handles various authentication results. However, the exception handling can be improved by using
raise ... from eto maintain the exception chain. Also, consider using f-strings for better readability and performance.- raise SageDesktopSDKError("Error while connecting with hh2 | {0}".format(e)) + raise SageDesktopSDKError(f"Error while connecting with hh2 | {e}") from e- self.__api_url = "https://{0}".format(indentifier) + self.__api_url = f"https://{indentifier}"
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sage_desktop_sdk/core/client.py (3 hunks)
- sage_desktop_sdk/sage_desktop_sdk.py (1 hunks)
Additional context used
Ruff
sage_desktop_sdk/core/client.py
45-45: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
45-45: Use f-string instead of
formatcallConvert to f-string
(UP032)
96-96: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
96-96: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
96-96: Use f-string instead of
formatcallConvert to f-string
(UP032)
108-108: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
108-108: Use f-string instead of
formatcallConvert to f-string
(UP032)
135-135: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
138-138: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
141-141: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
144-144: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
146-146: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
146-146: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
146-146: Use f-string instead of
formatcallConvert to f-string
(UP032)
156-156: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
156-156: Use f-string instead of
formatcallConvert to f-string
(UP032)
180-180: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
180-180: Use f-string instead of
formatcallConvert to f-string
(UP032)
190-190: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
190-190: Use f-string instead of
formatcallConvert to f-string
(UP032)
214-214: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
214-214: Use f-string instead of
formatcallConvert to f-string
(UP032)
223-223: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
223-223: Use f-string instead of
formatcallConvert to f-string
(UP032)
247-247: Use implicit references for positional format fields
Remove explicit positional indices
(UP030)
247-247: Use f-string instead of
formatcallConvert to f-string
(UP032)
Additional comments not posted (2)
sage_desktop_sdk/sage_desktop_sdk.py (1)
72-93: Centralize cookie management and remove debug prints.The centralization of cookie management by retrieving and setting the cookie in one place is a good practice as it reduces redundancy and potential errors. However, the debug print statement should be removed or replaced with proper logging if necessary for production environments.
- print("\n\nCOOKIE: ", cookie, "\n\n") + # Consider using logging.debug("COOKIE: ", cookie) if needed for debuggingVerification successful
Centralize cookie management and remove debug prints.
The centralized cookie management is correctly implemented, and direct calls to
update_cookiein entities have been removed or commented out. However, the debug print statement should be replaced with proper logging for production environments.
- Replace the debug print statement with logging:
- print("\n\nCOOKIE: ", cookie, "\n\n") + # Consider using logging.debug("COOKIE: ", cookie) if needed for debuggingScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all entities use the centralized `set_cookie` method. # Test: Search for any direct calls to `update_cookie` method in entities. rg --type python $'\\.update_cookie\\('Length of output: 69
Script:
#!/bin/bash # Description: Verify that all entities use the centralized `set_cookie` method. # Test: Search for any direct calls to `update_cookie` method in Python files. rg --glob '*.py' '\.update_cookie\('Length of output: 1340
sage_desktop_sdk/core/client.py (1)
47-49: Good encapsulation of cookie setting.
|
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
apps/sage300/utils.py (1)
Line range hint
80-83: Simplify the assignment ofdetail['type']usingdict.get.The current
ifblock can be replaced with a more concise and Pythonic approach using thegetmethod, which also provides a default value handling.- if vendor_type_mapping: - if item.type_id in vendor_type_mapping: - detail['type'] = vendor_type_mapping[item.type_id] - else: - detail['type'] = None + detail['type'] = vendor_type_mapping.get(item.type_id, None)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/sage300/utils.py (2 hunks)
- sage_desktop_sdk/sage_desktop_sdk.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- sage_desktop_sdk/sage_desktop_sdk.py
Additional context used
Ruff
apps/sage300/utils.py
80-83: Use
detail['type'] = vendor_type_mapping.get(item.type_id, None)instead of anifblockReplace with
detail['type'] = vendor_type_mapping.get(item.type_id, None)(SIM401)
| self.connection = SageDesktopSDK( | ||
| api_key=credentials_object.api_key, | ||
| api_secret=credentials_object.api_secret, | ||
| user_name=credentials_object.username, | ||
| password=credentials_object.password, | ||
| indentifier=credentials_object.identifier | ||
| ) |
There was a problem hiding this comment.
Consider adding error handling for SDK initialization.
Removing the try-except block from the __init__ method simplifies the code but also removes error handling around the SDK initialization. This could lead to unhandled exceptions if the SDK fails to initialize properly.
- try:
- self.connection = SageDesktopSDK(credentials_object, workspace_id)
- except Exception as e:
- # Handle exception
+ self.connection = SageDesktopSdk(
+ api_key=credentials_object.api_key,
+ api_secret=credentials_object.api_secret,
+ user_name=credentials_object.username,
+ password=credentials_object.password,
+ indentifier=credentials_object.identifier
+ )Consider reintroducing exception handling or ensure upstream error handling is robust.
Committable suggestion was skipped due to low confidence.
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Summary by CodeRabbit
New Features
Improvements
Bug Fixes