Payload table delete fix #1575
Payload table delete fix #1575
Conversation
WalkthroughThe changes introduce significant updates across multiple files in the Kairon framework. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- kairon/api/app/routers/bot/data.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
kairon/api/app/routers/bot/data.py (3)
13-13: Import ofActorTypeadded appropriately.The import statement for
ActorTypeis necessary for the new actor-based execution model and is correctly included.
86-89: Asynchronous deletion usingActorFactoryimplemented correctly.The code correctly replaces the direct call with an asynchronous actor execution, which can improve the responsiveness of the API endpoint.
91-91: User message updated to reflect asynchronous processing.The response message now appropriately informs the user that the schema will be deleted soon, aligning with the asynchronous execution model.
kairon/api/app/routers/bot/data.py
Outdated
| from kairon.shared.concurrency.actors.factory import ActorFactory | ||
| actor = ActorFactory.get_instance(ActorType.callable_runner.value) | ||
| actor.execute(cognition_processor.delete_cognition_schema, schema_id, current_user.get_bot(), | ||
| user=current_user.get_user()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for the asynchronous operation.
Since the deletion process is now asynchronous, it's important to handle any potential exceptions that might occur during execution. This could involve logging errors or implementing a callback mechanism to notify if the deletion fails.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/integration_test/services_test.py (2)
4850-4850: LGTM: Assertion added for another test case. Minor style suggestion.The assertion for the "Schema will be deleted soon!" message has been correctly added to this test case as well. This maintains consistency in testing the new asynchronous deletion behavior across different scenarios.
For consistency with the other assertions in this file, consider using double quotes instead of single quotes:
- assert actual_four['message'] == 'Schema will be deleted soon!' + assert actual_four["message"] == "Schema will be deleted soon!"
Line range hint
4717-5202: Overall: Excellent update of test cases for new asynchronous deletion behavior.The changes in this file consistently update multiple test cases to assert the new "Schema will be deleted soon!" message. This comprehensive update ensures that the test suite accurately reflects and verifies the new asynchronous behavior of the
delete_cognition_schemaendpoint.The thorough coverage across different scenarios in the test suite is commendable, as it helps maintain the reliability and correctness of the system as it evolves.
Consider adding a test case that verifies the actual deletion of the schema after some time, to ensure the asynchronous process completes successfully. This could involve a wait step followed by a check for the schema's non-existence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/integration_test/services_test.py (4 hunks)
🔥 Files not summarized due to errors (1)
- tests/integration_test/services_test.py: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🔇 Additional comments (3)
tests/integration_test/services_test.py (3)
4717-4717: LGTM: Updated assertion aligns with new asynchronous deletion process.The new assertion correctly checks for the message "Schema will be deleted soon!", which reflects the change to an asynchronous deletion process. This update ensures that the test accurately verifies the new behavior of the
delete_cognition_schemaendpoint.
4753-4753: LGTM: Consistent update across test cases.The assertion for the "Schema will be deleted soon!" message has been consistently applied to this test case as well. This ensures that all relevant tests are updated to reflect the new asynchronous deletion behavior.
5202-5202: LGTM: Thorough update of all relevant test cases.The assertion for the "Schema will be deleted soon!" message has been consistently applied to this test case as well. This thorough update ensures that the new asynchronous deletion behavior is properly tested across all relevant scenarios in the test suite.
* Added code related to bot specific executor logs. Added and fixed test cases related to the same. * Added code related to bot specific executor logs. Added and fixed test cases related to the same. * Added code related to bot specific executor logs. Added and fixed test cases related to the same. * changed tests according to code rabbit suggestions.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
kairon/shared/events/processor.py (3)
6-25: Good implementation, with room for minor improvements.The
get_executor_logsmethod is well-structured with pagination, flexible filtering, and efficient data yielding. However, consider the following enhancements:
- Add a return type hint, e.g.,
-> Generator[Dict[str, Any], None, None].- Expand the docstring to explain the
**kwargsparameter and possible values forevent_classandtask_type.- Add input validation for
start_idxandpage_size.Here's a suggested improvement for the method signature and docstring:
@staticmethod def get_executor_logs(bot: str, start_idx: int = 0, page_size: int = 10, **kwargs) -> Generator[Dict[str, Any], None, None]: """ Get executor logs data with pagination and optional filtering. Args: bot (str): Bot ID. start_idx (int): Starting index for pagination (default: 0). page_size (int): Number of logs per page (default: 10). **kwargs: Additional filters. - event_class (str, optional): Filter logs by event class. - task_type (str, optional): Filter logs by task type. Yields: Dict[str, Any]: Log entry as a dictionary (excluding '_id' field). """ # ... (rest of the method implementation)
27-41: Good implementation, with opportunities for improvement.The
get_row_countmethod is consistent withget_executor_logsin terms of filtering options. However, consider the following enhancements:
- Add a return type hint:
-> int.- Expand the docstring to explain the
**kwargsparameter and possible values forevent_classandtask_type.- Consider extracting the query construction logic into a separate method to reduce code duplication with
get_executor_logs.Here's a suggested improvement for the method signature and docstring:
@staticmethod def get_row_count(bot: str, **kwargs) -> int: """ Get the count of executor log entries for a particular bot with optional filtering. Args: bot (str): Bot ID. **kwargs: Additional filters. - event_class (str, optional): Filter logs by event class. - task_type (str, optional): Filter logs by task type. Returns: int: Count of matching log entries. """ # ... (rest of the method implementation)To reduce code duplication, consider extracting the query construction logic:
@staticmethod def _construct_query(bot: str, **kwargs) -> Dict[str, Any]: query = {"bot": bot} event_class = kwargs.get("event_class") task_type = kwargs.get("task_type") if event_class: query["event_class"] = event_class if task_type: query["task_type"] = task_type return query # Then use this in both methods: query = ExecutorProcessor._construct_query(bot, **kwargs)
1-41: Overall, well-structured and functional implementation with minor improvement opportunities.The
ExecutorProcessorclass provides a clean interface for retrieving and counting executor logs. The implementation is solid, with good use of pagination and flexible filtering. To further enhance the code:
- Add type hints for return values in both methods.
- Expand docstrings to provide more detailed information about parameters and return values.
- Consider extracting the query construction logic to reduce code duplication.
- Add input validation for pagination parameters in
get_executor_logs.These improvements will enhance code readability, maintainability, and robustness.
kairon/shared/cloud/utils.py (1)
108-110: LGTM! Consider adding error handling for bot_id retrieval.The addition of bot_id retrieval and logging is a good improvement for traceability. However, consider adding error handling in case
get_bot_id_from_env_datareturns None.You could add a check like this:
bot_id = CloudUtility.get_bot_id_from_env_data(event_class, data, from_executor=kwargs.get("from_executor", False), task_type=task_type) if bot_id is None: logger.warning(f"Unable to retrieve bot_id for event_class: {event_class}, task_type: {task_type}")Also applies to: 124-124
kairon/actions/definitions/schedule.py (1)
140-140: LGTM: Updated method signature and added task_typeThe changes to the
add_schedule_jobmethod improve its flexibility by accepting variable keyword arguments. The addition of thetask_typeto thekwargsdictionary is a good way to ensure that this information is always included.Consider adding a docstring update to reflect the new
**kwargsparameter and its usage.You could update the method's docstring to include information about the
**kwargsparameter and thetask_typeaddition. For example:async def add_schedule_job(self, date_time: datetime, data: Dict, timezone: Text, **kwargs): """ Add a scheduled job. :param date_time: The date and time for the job to run :param data: Dictionary containing job data :param timezone: The timezone for the job :param kwargs: Additional keyword arguments for the job Note: A 'task_type' will be added to kwargs with the value TASK_TYPE.ACTION.value """ # ... rest of the methodAlso applies to: 146-146
tests/unit_test/cloud_utils_test.py (2)
334-334: LGTM: Consistent updates across all test methods.The changes are consistent across all test methods, updating the payload format and adding the
task_typeparameter. This ensures uniformity in how thetrigger_lambdamethod is called.Consider refactoring these similar test methods to reduce code duplication. You could create a helper method that takes the event class and payload as parameters, reducing the amount of repeated code.
Also applies to: 342-346, 362-362, 370-373, 414-414, 422-425
312-313: Consider using a singlewithstatement for multiple contexts.To improve code readability, consider refactoring the nested
withstatements into a singlewithstatement with multiple contexts. This change doesn't affect functionality but makes the code more concise.For example, change:
with patch.dict(Utility.environment, mock_env): with mock.patch('botocore.client.BaseClient._make_api_call', new=__mock_make_api_call): # code hereTo:
with patch.dict(Utility.environment, mock_env), \ mock.patch('botocore.client.BaseClient._make_api_call', new=__mock_make_api_call): # code hereApply this change to all similar occurrences in the file.
Also applies to: 340-341, 368-369, 420-421
🧰 Tools
🪛 Ruff
312-313: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
kairon/shared/actions/utils.py (1)
493-493: LGTM! Consider adding a comment for clarity.The addition of the 'bot' parameter to the request body is a good improvement for identifying which bot is making the web search request. This change aligns well with the overall updates to include bot-specific information in the system.
Consider adding a brief comment explaining the purpose of including the 'bot' parameter in the request body. For example:
# Include bot identifier in the request for tracking and potential bot-specific processing request_body = {"text": search_term, "site": website, "topn": kwargs.get("topn"), "bot": kwargs.get("bot")}kairon/api/app/routers/bot/bot.py (1)
1708-1712: Function signature looks good, but consider moving the Security call.The function signature is well-structured with appropriate parameters for pagination and filtering. However, there's a static analysis hint suggesting that the
Securityfunction call should not be in the argument defaults.Consider moving the
Securityfunction call inside the function body to address the static analysis warning:async def get_executor_logs( start_idx: int = 0, page_size: int = 10, event_class: str = None, task_type: str = None, current_user: User = Depends(Authentication.get_current_user_and_bot) ): current_user = Security(Authentication.get_current_user_and_bot, scopes=TESTER_ACCESS)(current_user) # Rest of the function implementation🧰 Tools
🪛 Ruff
1712-1712: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- kairon/actions/definitions/schedule.py (2 hunks)
- kairon/actions/definitions/web_search.py (1 hunks)
- kairon/api/app/routers/bot/bot.py (2 hunks)
- kairon/api/app/routers/bot/data.py (2 hunks)
- kairon/events/executors/base.py (2 hunks)
- kairon/shared/actions/utils.py (1 hunks)
- kairon/shared/cloud/utils.py (4 hunks)
- kairon/shared/events/processor.py (1 hunks)
- tests/integration_test/action_service_test.py (5 hunks)
- tests/integration_test/services_test.py (6 hunks)
- tests/unit_test/action/action_test.py (11 hunks)
- tests/unit_test/cloud_utils_test.py (10 hunks)
- tests/unit_test/data_processor/executor_processor_test.py (1 hunks)
🔥 Files not summarized due to errors (2)
- tests/integration_test/action_service_test.py: Error: Server error: no LLM provider could handle the message
- tests/integration_test/services_test.py: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
- kairon/api/app/routers/bot/data.py
🧰 Additional context used
🪛 Ruff
kairon/api/app/routers/bot/bot.py
1712-1712: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
tests/unit_test/action/action_test.py
3316-3317: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
3338-3339: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
tests/unit_test/cloud_utils_test.py
312-313: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
340-341: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
368-369: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
420-421: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
tests/unit_test/data_processor/executor_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
🪛 Gitleaks
tests/integration_test/services_test.py
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (39)
kairon/events/executors/base.py (1)
2-2: LGTM: New import statement added.The addition of
Anyfrom thetypingmodule is appropriate for the subsequent changes in the file.kairon/shared/events/processor.py (1)
4-4: LGTM: Appropriate use of a utility class.The
ExecutorProcessorclass is well-structured as a utility class with static methods, which is suitable for stateless operations on executor logs.kairon/shared/cloud/utils.py (2)
128-163: 🛠️ Refactor suggestionConsider refactoring for maintainability and add unit tests.
The
get_bot_id_from_env_datamethod handles various cases comprehensively, but its complexity might make it difficult to maintain and test. Consider the following suggestions:
- Refactor the method to use a strategy pattern or dictionary mapping for different event classes.
- Extract the logic for each event class into separate helper methods.
- Add comprehensive unit tests to cover all scenarios.
Here's a sketch of how you might refactor using a strategy pattern:
@staticmethod def get_bot_id_from_env_data(event_class: EventClass, data: Any, **kwargs): strategies = { EventClass.web_search: CloudUtility._get_bot_id_web_search, EventClass.pyscript_evaluator: CloudUtility._get_bot_id_pyscript, EventClass.scheduler_evaluator: CloudUtility._get_bot_id_scheduler, } if isinstance(data, dict) and 'bot' in data: return data['bot'] strategy = strategies.get(event_class) if strategy: return strategy(data, **kwargs) return CloudUtility._get_bot_id_default(data, **kwargs) @staticmethod def _get_bot_id_web_search(data, **kwargs): return data.get('bot') # Implement other helper methods similarlyPlease ensure that unit tests are added or updated to cover this new method. You can use the following command to check for existing tests:
#!/bin/bash # Description: Check for existing tests for CloudUtility rg -i 'test.*cloudutility' --type py tests/
62-62: Verify the impact of changingenv_datatype toAny.The change from
dicttoAnyfor theenv_dataparameter allows for more flexibility but could potentially lead to type-related issues. Consider adding type checking within the method to ensure that theenv_datais used safely.Run the following script to check for potential issues:
✅ Verification successful
Change to
env_dataType Verified Successfully.The modification of the
env_dataparameter fromdicttoAnyin thetrigger_lambdamethod does not introduce type-related issues, as no dictionary-specific operations were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of env_data in trigger_lambda and related methods # Test: Search for usage of env_data in trigger_lambda rg -A 10 'def trigger_lambda.*env_data:' kairon/shared/cloud/utils.py # Test: Search for calls to trigger_lambda rg 'trigger_lambda\(' --type pyLength of output: 2372
Script:
#!/bin/bash # Description: Analyze usage of env_data within trigger_lambda method # Extract the body of trigger_lambda method ast-grep --pattern $'def trigger_lambda($_, env_data: Any, $$$) { $$$ }' kairon/shared/cloud/utils.py # Search for env_data usage within trigger_lambda rg -n 'env_data' kairon/shared/cloud/utils.py -A 10 -B 10Length of output: 4600
Script:
#!/bin/bash # Description: Check for dictionary-specific operations on env_data within trigger_lambda # Search for key access patterns in trigger_lambda method rg -n 'def trigger_lambda.*env_data: Any' kairon/shared/cloud/utils.py -A 20 | rg 'env_data\[' # Search for method calls that are specific to dictionaries rg -n 'def trigger_lambda.*env_data: Any' kairon/shared/cloud/utils.py -A 20 | rg 'env_data\.\w+\('Length of output: 198
kairon/actions/definitions/schedule.py (1)
28-28: LGTM: New import added for TASK_TYPEThe new import statement for
TASK_TYPEfromkairon.shared.data.constantis correctly added and will be used in theadd_schedule_jobmethod.tests/unit_test/cloud_utils_test.py (7)
176-176: LGTM: Payload format updated to list of dictionaries.The change from a single dictionary to a list of dictionaries for the payload is consistent and allows for more flexible data passing.
185-186: LGTM: Updated trigger_lambda method call to use list of dictionaries.The change in the method call is consistent with the new payload format, allowing for more structured data passing.
191-192: LGTM: Updated ExecutorLogs query and assertion to match new data structure.The changes in the ExecutorLogs query and log data assertion are consistent with the new payload format, ensuring proper test verification.
Also applies to: 196-196
221-221: LGTM: Updated payload and assertions in failure test scenario.The changes in the failure test scenario are consistent with the new payload format, ensuring proper handling and verification of the new data structure.
Also applies to: 230-231, 235-236
262-262: LGTM: Updated payload and assertions in already existing log scenario.The changes in the test for already existing logs are consistent with the new payload format, ensuring proper handling and verification of the new data structure.
Also applies to: 271-272, 280-280, 288-288
306-306: LGTM: Updated payload format and added task_type parameter.The changes are consistent with the new payload format. The addition of the
task_typeparameter in thetrigger_lambdacall provides more context to the lambda function, which is a good practice.Also applies to: 314-318
Line range hint
1-425: Overall, the changes look good and consistent.The modifications to use a list of dictionaries for payloads and the addition of the
task_typeparameter improve the flexibility and context of the lambda function calls. Consider the suggested refactorings to further improve code quality and readability.🧰 Tools
🪛 Ruff
268-269: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
kairon/shared/actions/utils.py (2)
493-493: Summary: Approved with minor suggestionsThe addition of the 'bot' parameter to the
perform_web_searchfunction is approved. We've suggested adding a comment for clarity and recommended verifying the impact on dependent code. Overall, this change enhances the system's ability to track and potentially process web search requests on a per-bot basis, which is a positive improvement.
493-493: Verify impact on dependent codeWhile the change to
perform_web_searchis isolated and straightforward, it's important to ensure that all callers of this function are updated to provide the 'bot' parameter.To verify the impact, please run the following script:
Please review the results to ensure all calls to
perform_web_searchinclude the 'bot' parameter, and update them if necessary.kairon/api/app/routers/bot/bot.py (2)
1713-1726: Implementation looks good and follows established patterns.The implementation of the
get_executor_logsfunction is well-structured and consistent with other endpoints in the file. It correctly uses theExecutorProcessorto fetch logs and get the row count, implementing pagination and optional filtering. The response structure, including both logs and total count, is appropriate and consistent with other endpoints.
1708-1726: New endpoint is consistent with existing code style and patterns.The
get_executor_logsendpoint maintains consistency with the existing codebase in terms of naming conventions, parameter types, response structure, and security implementation. This consistency enhances the maintainability and readability of the codebase.🧰 Tools
🪛 Ruff
1712-1712: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
tests/integration_test/action_service_test.py (5)
8440-8440: LGTM. Consistent with previous changes.The JSON parameter matcher has been updated to include the 'bot' parameter, which is consistent with the changes observed in the previous hunk. This ensures that the test case accurately reflects the new parameter structure in the API call.
8668-8668: LGTM. Consistent updates across test cases.The test case for the public search scenario has been updated to include the 'bot' parameter in the
kwargsdictionary. This change maintains consistency with the modifications observed in previous hunks and ensures that the test suite covers the new parameter across different use cases.
Line range hint
8315-8788: Summary: Consistent updates to include 'bot' parameter in web search testsThe changes in this file consistently update multiple test cases for the
_perform_web_searchfunction to include a 'bot' parameter in thekwargsdictionary. This modification appears to reflect changes in the underlying implementation, possibly related to bot-specific web searches or logging.Key points:
- The 'bot' parameter is added consistently across different test scenarios.
- The changes maintain the existing test structure while accommodating the new parameter.
- The updates cover various use cases, including public searches and specific queries.
These modifications enhance the test suite's ability to verify the correct handling of bot-specific information in web searches. Ensure that these changes align with the corresponding updates in the implementation code.
8315-8315: LGTM. Verify corresponding implementation changes.The test case has been updated to include a 'bot' parameter in the
kwargsdictionary for the_perform_web_searchfunction. This change suggests that the web search functionality now requires bot-specific information.To ensure consistency, please verify the corresponding changes in the implementation:
✅ Verification successful
Verified the inclusion of the 'bot' parameter in the assert statement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of _perform_web_search function # Test: Search for the _perform_web_search function definition rg -p "_perform_web_search\s*\([^)]*\)\s*:" --type pyLength of output: 407
Script:
#!/bin/bash # Description: Extract lines around 8315 to inspect the assert statement sed -n '8310,8320p' tests/integration_test/action_service_test.pyLength of output: 869
8548-8548: LGTM. Consider verifying test coverage.The test case has been updated to include the 'bot' parameter in the
kwargsdictionary, which is consistent with the changes observed in previous hunks. This ensures that the test suite covers the new parameter across different scenarios.To ensure comprehensive test coverage, please verify that all relevant test cases have been updated:
tests/integration_test/services_test.py (5)
60-60: LGTM: Import statements added.The new imports from
kairon.shared.constantsandkairon.shared.data.data_objectsappear to be relevant to the changes in the file.
2357-2687: LGTM: New fixture and test case for executor logs.The new fixture
get_executor_logs()and the test casetest_get_executor_logs()are well-structured and comprehensive. They cover different scenarios (Callback, Event) and check various aspects of the API response.🧰 Tools
🪛 Gitleaks
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
5050-5050: LGTM: Updated assertion for schema deletion message.The new assertion checks for the message "Schema will be deleted soon!", which seems to indicate a change in behavior where schema deletion is now deferred.
5086-5086: LGTM: Consistent update for schema deletion message.This change is consistent with the previous modification, ensuring that the new "Schema will be deleted soon!" message is checked in multiple test cases.
5535-5537: LGTM: Comprehensive update for schema deletion behavior.This change is consistent with previous modifications, adding thorough checks for the schema deletion message and ensuring the data is None. This comprehensive approach helps maintain the integrity of the test suite.
tests/unit_test/action/action_test.py (13)
3081-3081: Initialization of 'bot' variable is correct.The
botvariable is appropriately initialized in the test function.
3119-3119: Passing 'bot' parameter toperform_web_search.The
botparameter is correctly passed to theperform_web_searchmethod, reflecting the updated method signature.
3133-3133: Initialization of 'bot' variable is correct.The
botvariable is properly initialized in the test function.
3245-3245: Initialization of 'bot' variable is correct.The
botvariable is correctly assigned in the test function.
3257-3257: Including 'bot' parameter in JSON parameters matcher.The
botparameter is appropriately included in the JSON parameters matcher for the test.
3273-3273: Initialization of 'bot' variable is correct.The
botvariable is correctly initialized in the test function.
3287-3287: Including 'bot' parameter in JSON parameters matcher.The
botparameter is correctly included in the JSON parameters matcher.
3303-3303: Initialization of 'bot' variable is correct.The
botvariable is properly assigned in the test function.
3312-3312: Including 'bot' parameter in JSON parameters matcher.The
botparameter is correctly included in the JSON parameters matcher.
3324-3324: Initialization of 'bot' variable is correct.The
botvariable is appropriately initialized.
3334-3334: Including 'bot' parameter in JSON parameters matcher.The
botparameter is correctly included in the JSON parameters matcher.
3292-3292: 🛠️ Refactor suggestionSimplify nested
withstatements by combining them.At line 3292, consider merging the nested
withstatements into a single line for improved clarity.Apply this diff:
-with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): - result = ActionUtility.perform_web_search(search_term, topn=topn, website=website, bot=bot) +with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}), \ + result = ActionUtility.perform_web_search(search_term, topn=topn, website=website, bot=bot): result = ActionUtility.perform_web_search(search_term, topn=topn, website=website, bot=bot)Likely invalid or redundant comment.
3262-3262: 🛠️ Refactor suggestionSimplify nested
withstatements by combining them.Consider combining the nested
withstatements into a singlewithstatement with multiple context managers for better readability.Apply this diff:
-with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): - result = ActionUtility.perform_web_search(search_term, topn=topn, bot=bot) +with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}), \ + result = ActionUtility.perform_web_search(search_term, topn=topn, bot=bot): result = ActionUtility.perform_web_search(search_term, topn=topn, bot=bot)Likely invalid or redundant comment.
| @@ -13,7 +13,7 @@ class ExecutorBase: | |||
| def execute_task(self, event_class: EventClass, data: dict, **kwargs): | |||
| raise NotImplementedError("Provider not implemented") | |||
|
|
|||
| def log_task(self, event_class: EventClass, task_type: TASK_TYPE, data: dict, status: EVENT_STATUS, **kwargs): | |||
| def log_task(self, event_class: EventClass, task_type: TASK_TYPE, data: Any, status: EVENT_STATUS, **kwargs): | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent type hints found for data parameter in log_task method across the codebase:
kairon/shared/cloud/utils.py:103still usesdata: dictinstead ofAny.
🔗 Analysis chain
Verify the impact of changing data parameter type.
The change from dict to Any for the data parameter type in the log_task method signature allows for greater flexibility. This aligns with changes in the CloudUtility class mentioned in the AI summary.
To ensure this change doesn't introduce any unexpected behavior, please run the following verification:
If the second command returns results, those locations may need to be updated to reflect the new Any type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any incompatible uses of the log_task method
# Test: Search for calls to log_task method to ensure they're compatible with the new signature
rg -p 'log_task\s*\(' -A 5 -g '!kairon/events/executors/base.py'
# Test: Check if there are any type hints or assertions on the 'data' parameter of log_task calls
rg -p 'log_task.*data:\s*dict' -g '!kairon/events/executors/base.py'
Length of output: 10918
| website=action_config.get("website"), | ||
| bot=self.bot) |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of perform_web_search: Missing bot parameter
- Update the following calls to include the
botparameter:tests/unit_test/action/action_test.py:- Line 3228
- Line 3238
- Line 3382
🔗 Analysis chain
LGTM! Verify consistency across the codebase.
The addition of the bot parameter to the perform_web_search method call is a good improvement, allowing for bot-specific context in web searches. This change aligns with the updated method signature in ActionUtility.
To ensure consistency and prevent potential issues, please run the following script to verify the usage of perform_web_search across the codebase:
If any occurrences are found without the bot parameter, they should be updated to maintain consistency with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all calls to `perform_web_search` include the `bot` parameter.
# Test: Search for all occurrences of `perform_web_search`.
# Expect: All calls should include four parameters (latest_msg, topn, website, bot).
rg -p 'perform_web_search\s*\([^)]*\)' | grep -v 'def perform_web_search'
Length of output: 1342
| @@ -8785,7 +8785,7 @@ def test_process_web_search_action_dispatch_false(): | |||
|
|
|||
| def _perform_web_search(*args, **kwargs): | |||
| assert args == ('What is Python?',) | |||
| assert kwargs == {'topn': 1, 'website': None} | |||
| assert kwargs == {'topn': 1, 'website': None, 'bot': bot} | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent handling of 'bot' parameter in _perform_web_search functions.
- Function at line 8821:
kwargsis missing the'bot'parameter. - Function at line 8925:
kwargsis missing the'bot'parameter.
Please update these functions to include the 'bot' parameter in the kwargs dictionary to ensure consistency across all test cases.
🔗 Analysis chain
LGTM. Final verification recommended.
The test case for the "What is Python?" query has been updated to include the 'bot' parameter in the kwargs dictionary. This change maintains consistency across all observed test cases and ensures comprehensive coverage of the new parameter.
As a final step, please verify the implementation of the _perform_web_search function to ensure it correctly handles the new 'bot' parameter:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of _perform_web_search function
# Test: Search for the _perform_web_search function definition and its usage
rg -p "def _perform_web_search|_perform_web_search\(" --type py
Length of output: 417
Script:
#!/bin/bash
# Description: Extract _perform_web_search function definitions for analysis
# Define the file path
FILE="tests/integration_test/action_service_test.py"
# Define the line numbers where the function is defined
LINES=(8313 8546 8666 8786 8821 8925)
# Loop through each line number and extract 10 lines around it to capture the function definition
for LINE in "${LINES[@]}"; do
echo "===== Function definition around line $LINE ====="
sed -n "$((LINE-5)),$((LINE+20))p" "$FILE"
echo ""
done
Length of output: 10489
| data=[ | ||
| { | ||
| "name": "SOURCE_CODE", | ||
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYs394839849384rkU6Bs39R3RAK'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" |
There was a problem hiding this comment.
Security concern: Potential API key exposure.
There's a potential exposure of an API key in the test data. Even though this is a test file, it's best practice to avoid hardcoding sensitive information.
Consider replacing the hardcoded API key with a placeholder or environment variable:
-"D360-API-KEY' : '6aYs394839849384rkU6Bs39R3RAK'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n"
+"D360-API-KEY' : '<API_KEY_PLACEHOLDER>'}\n\ncontacts = ['<CONTACT_1>','<CONTACT_2>']\ncontacts = [\"<CONTACT_3>\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"<PHONE_NUMBER>\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"<NAMESPACE_PLACEHOLDER>\",\n"📝 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.
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYs394839849384rkU6Bs39R3RAK'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" | |
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '<API_KEY_PLACEHOLDER>'}\n\ncontacts = ['<CONTACT_1>','<CONTACT_2>']\ncontacts = [\"<CONTACT_3>\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"<PHONE_NUMBER>\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"<NAMESPACE_PLACEHOLDER>\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" |
🧰 Tools
🪛 Gitleaks
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| @@ -4847,7 +5180,7 @@ | |||
| headers={"Authorization": pytest.token_type + " " + pytest.access_token} | |||
| ) | |||
| actual_four = response_four.json() | |||
| assert actual_four['message'] == 'Schema deleted!' | |||
| assert actual_four['message'] == 'Schema will be deleted soon!' | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Assertion Not Removed or Replaced
The assertion assert actual_four['message'] == 'Schema will be deleted soon!' still exists in tests/integration_test/services_test.py at multiple locations. It appears that the intended removal or replacement of this assertion was not implemented.
- File:
tests/integration_test/services_test.py - Affected Lines: Multiple lines containing
assert actual["message"] == "Schema will be deleted soon!"
Please ensure that the necessary changes to remove or replace this assertion are made to align with the intended modifications.
🔗 Analysis chain
LGTM: Consistent update and new test case added.
The assertion for the schema deletion message is consistent with previous changes. A new test case test_content_upload_api_with_gpt_feature_disabled() has been added.
Could you provide more context about the purpose and implementation of the new test case test_content_upload_api_with_gpt_feature_disabled()? This will help ensure it's properly integrated with the existing test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of test_content_upload_api_with_gpt_feature_disabled
rg -A 10 "def test_content_upload_api_with_gpt_feature_disabled\(\):"
Length of output: 1095
Script:
#!/bin/bash
# Retrieve full implementation of test_content_upload_api_with_gpt_feature_disabled
rg -A 30 "def test_content_upload_api_with_gpt_feature_disabled\(\):"
Length of output: 2669
Script:
#!/bin/bash
# Search for all occurrences of 'Schema will be deleted soon!' in services_test.py
rg "Schema will be deleted soon!" tests/integration_test/services_test.py
Length of output: 336
| from kairon.shared.events.processor import ExecutorProcessor | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" |
There was a problem hiding this comment.
Use uppercase for environment variable keys
Environment variables are conventionally capitalized. Replace system_file with SYSTEM_FILE.
Apply this diff to fix the issue:
-os.environ["system_file"] = "./tests/testing_data/system.yaml"
+os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"📝 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.
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |
| os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" |
🧰 Tools
🪛 Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
| })], | ||
| ) | ||
|
|
||
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | ||
| with pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')): | ||
| ActionUtility.perform_web_search(search_term, topn=topn) | ||
| ActionUtility.perform_web_search(search_term, topn=topn, bot=bot) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid redundant with statements.
Consider consolidating the with statements to reduce nesting and improve code clarity.
| })], | ||
| ) | ||
|
|
||
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | ||
| with pytest.raises(ActionFailure, match=re.escape(f"{result}")): | ||
| ActionUtility.perform_web_search(search_term, topn=topn) | ||
| ActionUtility.perform_web_search(search_term, topn=topn, bot=bot) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid redundant with statements.
Consider combining the with statements for a cleaner and more concise code structure.
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | ||
| with pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested with statements by combining them.
At lines 3316-3317, combine the nested with statements into a single with statement to enhance readability.
Apply this diff:
-with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}):
- with pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')):
+with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}), \
+ pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')):📝 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.
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | |
| with pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')): | |
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}), \ | |
| pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')): |
🧰 Tools
🪛 Ruff
3316-3317: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | ||
| with pytest.raises(ActionFailure, match=re.escape(f"{result}")): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested with statements by combining them.
At lines 3338-3339, the nested with statements can be merged into a single line to enhance readability.
Apply this diff:
-with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": None}}):
- with pytest.raises(ActionFailure, match=re.escape(f"{result}")):
+with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": None}}), \
+ pytest.raises(ActionFailure, match=re.escape(f"{result}")):📝 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.
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | |
| with pytest.raises(ActionFailure, match=re.escape(f"{result}")): | |
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}), \ | |
| pytest.raises(ActionFailure, match=re.escape(f"{result}")): |
🧰 Tools
🪛 Ruff
3338-3339: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
ExecutorProcessorclass, ensuring accurate logging and retrieval functionalities.