-
Notifications
You must be signed in to change notification settings - Fork 12
feat: enhance start_tasks with conditional input logic and add array handling migration #216
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.
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
base: 09-16-make-initial-tasks-nullable
Are you sure you want to change the base?
feat: enhance start_tasks with conditional input logic and add array handling migration #216
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
View your CI Pipeline Execution ↗ for commit 781e413
☁️ Nx Cloud last updated this comment at |
0a5205c
to
63a2762
Compare
pkgs/core/supabase/migrations/20250916201303_pgflow_temp_handle_arrays_in_start_tasks.sql
Outdated
Show resolved
Hide resolved
ce60c8e
to
a462a29
Compare
(select flow_start_time_ms <= task_creation_time_ms * 1.5 | ||
from map_performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable comparison appears to be reversed. The test is checking if array processing scales linearly, but the variables in the comparison don't match their content:
select ok(
(select flow_start_time_ms <= task_creation_time_ms * 1.5
from map_performance
where array_size = -1),
'Array processing MUST scale linearly (time_ratio <= size_ratio * 1.5)'
);
In the sentinel row (where array_size = -1
), flow_start_time_ms
contains the time ratio and task_creation_time_ms
contains the size ratio. The comparison should be:
select ok(
(select time_ratio <= size_ratio * 1.5
from map_performance
where array_size = -1),
'Array processing MUST scale linearly (time_ratio <= size_ratio * 1.5)'
);
Or using the existing column names:
select ok(
(select flow_start_time_ms <= task_creation_time_ms * 1.5
from map_performance
where array_size = -1),
'Array processing MUST scale linearly (time_ratio <= size_ratio * 1.5)'
);
(select flow_start_time_ms <= task_creation_time_ms * 1.5 | |
from map_performance | |
(select time_ratio <= size_ratio * 1.5 | |
from map_performance |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
a462a29
to
419de1b
Compare
(SELECT jsonb_array_element(value, st.task_index) | ||
FROM jsonb_each(dep_out.deps_output) | ||
LIMIT 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical bug: The subquery uses LIMIT 1 without ORDER BY, making the result non-deterministic when a map step has multiple dependencies. While the comment states 'Map steps have exactly 1 dependency (enforced by add_step)', this creates a race condition if that constraint is ever violated or if there are concurrent modifications. The query should either add ORDER BY for deterministic results or add a runtime check to ensure exactly one dependency exists. This could cause map tasks to receive elements from random dependencies in multi-dependency scenarios.
(SELECT jsonb_array_element(value, st.task_index) | |
FROM jsonb_each(dep_out.deps_output) | |
LIMIT 1) | |
(SELECT jsonb_array_element(value, st.task_index) | |
FROM jsonb_each(dep_out.deps_output) | |
WHERE (SELECT COUNT(*) FROM jsonb_each(dep_out.deps_output)) = 1 | |
LIMIT 1) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
…handling migration - Updated start_tasks function to build step input conditionally based on step type - Implemented logic for root and dependent map steps to extract array elements - Added a new migration script to handle array elements in start_tasks - Included comprehensive tests for dependent map element extraction, large array processing, mixed JSON types, and nested arrays - Improved handling of array-based tasks and input construction for various step configurations
419de1b
to
781e413
Compare
-- Extract the element at task_index from the run's input array. | ||
-- Note: If run input is not an array, this will return NULL | ||
-- and the flow will fail (validated in start_flow). | ||
jsonb_array_element(r.input, st.task_index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array bounds vulnerability - jsonb_array_element(r.input, st.task_index)
will return NULL if task_index is out of bounds, but this NULL is passed directly as task input without validation. If there's a race condition or bug in task creation that results in task_index >= array_length, tasks will receive NULL input and likely fail at runtime. Add bounds checking or handle NULL case explicitly.
jsonb_array_element(r.input, st.task_index) | |
CASE | |
WHEN jsonb_array_length(r.input) > st.task_index THEN jsonb_array_element(r.input, st.task_index) | |
ELSE jsonb_build_object('error', 'Task index out of bounds') | |
END |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
🔍 Preview Deployment: Website✅ Deployment successful! 🔗 Preview URL: https://pr-216.pgflow.pages.dev 📝 Details:
_Last updated: _ |
🔍 Preview Deployment: Playground✅ Deployment successful! 🔗 Preview URL: https://pr-216--pgflow-demo.netlify.app 📝 Details:
_Last updated: _ |
Enhanced Map Step Input Processing