Support non task data environment, step 1 #1745
Conversation
WalkthroughWalkthroughThe recent changes to the Changes
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 (2)
spiffworkflow-backend/src/spiffworkflow_backend/config/default.py (1)
Line range hint
150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.Consider storing sensitive keys in environment variables or secure vaults to prevent potential security risks.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (1)
Line range hint
211-274: TheNonTaskDataBasedScriptEngineEnvironmentclass has been introduced to handle environments without task data. The implementation ofexecuteandevaluatemethods needs careful review to ensure they handle the script execution context correctly.Consider adding more robust error handling and validation in the
executeandevaluatemethods to prevent potential security issues withevalandexec.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- spiffworkflow-backend/dev.docker-compose.yml (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/config/default.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (8 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_test_runner_service.py (1 hunks)
- spiffworkflow-backend/src/spiffworkflow_backend/services/task_service.py (1 hunks)
- spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_migrator.py (2 hunks)
Files skipped from review due to trivial changes (2)
- spiffworkflow-backend/dev.docker-compose.yml
- spiffworkflow-backend/src/spiffworkflow_backend/services/task_service.py
Additional context used
Gitleaks
spiffworkflow-backend/src/spiffworkflow_backend/config/default.py
150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (7)
spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_migrator.py (2)
114-114: Added handling for serialization without script engine state.This change aligns with the PR's objective to support environments without task data, ensuring script engine state is optional during serialization.
211-211: Added handling for serialization without script engine state in another context.Consistent with changes in other parts of the codebase, this ensures uniform handling of script engine state across various serialization contexts.
spiffworkflow-backend/src/spiffworkflow_backend/config/default.py (1)
237-237: Introduced configuration for using non-task data based script engine environments.This new configuration setting allows toggling the use of non-task data based environments, which supports the PR's goal of making the script engine environment selectable at runtime.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_model_test_runner_service.py (1)
152-152: Refactored to use the newCustomScriptEngineEnvironment.create()method when initializing the script engine.This change ensures that the script engine environment is dynamically determined at runtime, aligning with the PR's objectives to support selectable environments.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (3)
302-308: TheCustomScriptEngineEnvironment.createmethod has been modified to support dynamic selection of the script engine environment based on an environment variable. This is a critical change that affects the system's configurability and flexibility.Ensure that the environment variable
SPIFFWORKFLOW_BACKEND_USE_NON_TASK_DATA_BASED_SCRIPT_ENGINE_ENVIRONMENTis documented and included in deployment configurations.
351-351: TheCustomBpmnScriptEngineconstructor now uses theCustomScriptEngineEnvironment.createmethod to instantiate the script engine environment. This change integrates the new dynamic environment selection logic into the script engine.Verify that all instances of
CustomBpmnScriptEngineacross the system are updated to accommodate this change.
1533-1544: Theserializemethod inProcessInstanceProcessornow conditionally preserves or pops the script engine state based on theserialize_script_engine_stateparameter. This modification is crucial for ensuring that the state is correctly managed during serialization.It's recommended to add integration tests to ensure that the serialization behavior works as expected with different configurations of the
serialize_script_engine_stateparameter.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py
Show resolved
Hide resolved
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py
Show resolved
Hide resolved
| @@ -111,7 +111,7 @@ def test_can_run_version_4_migration( | |||
| processor = ProcessInstanceProcessor( | |||
| process_instance, include_task_data_for_completed_tasks=True, include_completed_subprocesses=True | |||
| ) | |||
| bpmn_process_dict_version_4 = processor.serialize() | |||
| bpmn_process_dict_version_4 = processor.serialize(serialize_script_engine_state=False) | |||
There was a problem hiding this comment.
Tests are passing for me fine with default settings. Is this actually necessary? I don't think the current changes should require this.
There was a problem hiding this comment.
I edited the description, which upon a re-read wasn't very clear, but these changes are to get the migrator tests passing with the non task data based environment. This was part of the work on the non_task_data_env branch to get the unit tests passing.
Work on #1567
Starting to bring in some changes from the
non_task_data_based_envbranch that has been in progress for a while. Main changes are movingCustomScriptEngineEnvironmentfrom a hardcoded choice to a factory method so the script engine environment can be selected at runtime. This allows running tests or deploying with the non task data based environment without code changes. Also added the ability to omit the non task data state when serializing, which is required to get the migrator tests to pass edit: when using the non task data based environment.Summary by CodeRabbit
New Features
env_fileconfiguration.Enhancements
Bug Fixes
Refactor