codeant-ai-for-open-source[bot] commented on code in PR #36368: URL: https://github.com/apache/superset/pull/36368#discussion_r2738893973
########## superset-frontend/src/features/tasks/TaskStatusIcon.tsx: ########## @@ -0,0 +1,145 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React from 'react'; +import { useTheme, SupersetTheme, t } from '@apache-superset/core/ui'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { Tooltip } from '@superset-ui/core/components'; +import { TaskStatus } from './types'; +import { formatProgressTooltip } from './timeUtils'; + +function getStatusColor(status: TaskStatus, theme: SupersetTheme): string { + switch (status) { + case TaskStatus.Pending: + return theme.colorPrimaryText; + case TaskStatus.InProgress: + return theme.colorPrimaryText; + case TaskStatus.Success: + return theme.colorSuccessText; + case TaskStatus.Failure: + return theme.colorErrorText; + case TaskStatus.TimedOut: + return theme.colorErrorText; + case TaskStatus.Aborting: + return theme.colorWarningText; + case TaskStatus.Aborted: + return theme.colorWarningText; + default: + return theme.colorText; + } +} + +const statusIcons = { + [TaskStatus.Pending]: Icons.ClockCircleOutlined, + [TaskStatus.InProgress]: Icons.LoadingOutlined, + [TaskStatus.Success]: Icons.CheckCircleOutlined, + [TaskStatus.Failure]: Icons.CloseCircleOutlined, + [TaskStatus.TimedOut]: Icons.ClockCircleOutlined, // Clock to indicate timeout + [TaskStatus.Aborting]: Icons.LoadingOutlined, // Spinning to show in-progress abort + [TaskStatus.Aborted]: Icons.StopOutlined, +}; + +const statusLabels = { + [TaskStatus.Pending]: t('Pending'), + [TaskStatus.InProgress]: t('In Progress'), + [TaskStatus.Success]: t('Success'), + [TaskStatus.Failure]: t('Failed'), + [TaskStatus.TimedOut]: t('Timed Out'), + [TaskStatus.Aborting]: t('Aborting'), + [TaskStatus.Aborted]: t('Aborted'), Review Comment: **Suggestion:** Translations are executed at module initialization because `t(...)` is called when building `statusLabels`, so labels will not update when locale changes and can capture the wrong locale. Store raw label keys at module scope and call `t()` at render time (inside the component) to ensure labels respect the current locale. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ TaskStatusIcon labels won't update after locale change. - ⚠️ Tooltips remain in initial language post locale switch. ``` </details> ```suggestion [TaskStatus.Pending]: 'Pending', [TaskStatus.InProgress]: 'In Progress', [TaskStatus.Success]: 'Success', [TaskStatus.Failure]: 'Failed', [TaskStatus.TimedOut]: 'Timed Out', [TaskStatus.Aborting]: 'Aborting', [TaskStatus.Aborted]: 'Aborted', ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the app so module `superset-frontend/src/features/tasks/TaskStatusIcon.tsx` is evaluated; during module initialization lines 58-66 execute `t(...)` and store translated strings in `statusLabels`. 2. `t()` implementation (superset-frontend/packages/superset-core/src/ui/translation/TranslatorSingleton.ts:66-68) delegates to `getInstance().translate(...)`. 3. Later, when the user changes the UI locale at runtime (translator instance updates), TaskStatusIcon will re-render but will reuse the already-evaluated `statusLabels` values (module scope), so labels remain in the original language captured at import-time. 4. Observe tooltips and labels in TaskStatusIcon (superset-frontend/src/features/tasks/TaskStatusIcon.tsx) not updating to new locale because translations were executed once on module load. Note: This is a concrete problem because `t()` is visible (TranslatorSingleton.ts:66-68) and module-scope evaluation occurs in TaskStatusIcon.tsx:58-66. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/tasks/TaskStatusIcon.tsx **Line:** 59:65 **Comment:** *Logic Error: Translations are executed at module initialization because `t(...)` is called when building `statusLabels`, so labels will not update when locale changes and can capture the wrong locale. Store raw label keys at module scope and call `t()` at render time (inside the component) to ensure labels respect the current locale. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/features/tasks/TaskStatusIcon.tsx: ########## @@ -0,0 +1,145 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React from 'react'; +import { useTheme, SupersetTheme, t } from '@apache-superset/core/ui'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { Tooltip } from '@superset-ui/core/components'; +import { TaskStatus } from './types'; +import { formatProgressTooltip } from './timeUtils'; + +function getStatusColor(status: TaskStatus, theme: SupersetTheme): string { + switch (status) { + case TaskStatus.Pending: + return theme.colorPrimaryText; + case TaskStatus.InProgress: + return theme.colorPrimaryText; + case TaskStatus.Success: + return theme.colorSuccessText; + case TaskStatus.Failure: + return theme.colorErrorText; + case TaskStatus.TimedOut: + return theme.colorErrorText; + case TaskStatus.Aborting: + return theme.colorWarningText; + case TaskStatus.Aborted: + return theme.colorWarningText; + default: + return theme.colorText; + } +} + +const statusIcons = { + [TaskStatus.Pending]: Icons.ClockCircleOutlined, + [TaskStatus.InProgress]: Icons.LoadingOutlined, + [TaskStatus.Success]: Icons.CheckCircleOutlined, + [TaskStatus.Failure]: Icons.CloseCircleOutlined, + [TaskStatus.TimedOut]: Icons.ClockCircleOutlined, // Clock to indicate timeout + [TaskStatus.Aborting]: Icons.LoadingOutlined, // Spinning to show in-progress abort + [TaskStatus.Aborted]: Icons.StopOutlined, +}; + +const statusLabels = { + [TaskStatus.Pending]: t('Pending'), + [TaskStatus.InProgress]: t('In Progress'), + [TaskStatus.Success]: t('Success'), + [TaskStatus.Failure]: t('Failed'), + [TaskStatus.TimedOut]: t('Timed Out'), + [TaskStatus.Aborting]: t('Aborting'), + [TaskStatus.Aborted]: t('Aborted'), +}; + +interface TaskStatusIconProps { + status: TaskStatus; + progressPercent?: number | null; + progressCurrent?: number | null; + progressTotal?: number | null; + durationSeconds?: number | null; + errorMessage?: string | null; + exceptionType?: string | null; +} + +export default function TaskStatusIcon({ + status, + progressPercent, + progressCurrent, + progressTotal, + durationSeconds, + errorMessage, + exceptionType, +}: TaskStatusIconProps) { + const theme = useTheme(); + const IconComponent = statusIcons[status]; Review Comment: **Suggestion:** Runtime error if `status` is not present in `statusIcons`: `IconComponent` can be undefined and React will throw when trying to render `<IconComponent />`. Provide a safe fallback component to avoid rendering an undefined element. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ TaskStatusIcon rendering crashes UI components. - ⚠️ Any page showing task statuses fails to render icons. ``` </details> ```suggestion const IconComponent = statusIcons[status] || Icons.QuestionCircleOutlined; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Build frontend with PR code and open any page that renders task icons. 2. A React render calls TaskStatusIcon (export default in superset-frontend/src/features/tasks/TaskStatusIcon.tsx). At runtime the component executes code at line 88: `const IconComponent = statusIcons[status];`. 3. If the `status` prop is undefined or a value not present in `statusIcons` (for example a new enum value or a runtime bug from the caller), `statusIcons[status]` is undefined. 4. React then attempts to render `<IconComponent ... />` (line 137 in file), causing a "Element type is invalid" runtime error and breaking the UI where TaskStatusIcon is mounted. Note: TypeScript typing (TaskStatus) reduces likelihood but does not guarantee runtime safety; this is a real runtime failure path originating at TaskStatusIcon.tsx:88 and rendering at TaskStatusIcon.tsx:137. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/features/tasks/TaskStatusIcon.tsx **Line:** 88:88 **Comment:** *Possible Bug: Runtime error if `status` is not present in `statusIcons`: `IconComponent` can be undefined and React will throw when trying to render `<IconComponent />`. Provide a safe fallback component to avoid rendering an undefined element. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset/tasks/registry.py: ########## @@ -0,0 +1,110 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Task registry for the Global Task Framework (GTF)""" + +import logging +from typing import Any, Callable + +logger = logging.getLogger(__name__) + + +class TaskRegistry: + """ + Registry for async task functions. + + Stores task functions by name, allowing the Celery executor to look up + and execute registered tasks. This enables the decorator pattern where + functions are registered at module import time. + """ + + _tasks: dict[str, Callable[..., Any]] = {} + + @classmethod + def register(cls, task_name: str, func: Callable[..., Any]) -> None: + """ + Register a task function by name. + + :param task_name: Unique task identifier (e.g., "superset.generate_thumbnail") + :param func: The task function to register + :raises ValueError: If task name is already registered + """ + if task_name in cls._tasks: + existing_func = cls._tasks[task_name] + if existing_func is not func: + raise ValueError( + f"Task '{task_name}' is already registered with a different " + "function. " + f"Existing: {existing_func.__module__}.{existing_func.__name__}, " + f"New: {func.__module__}.{func.__name__}" + ) + # Same function being registered again (e.g., module reload) - allow it + logger.debug("Task '%s' re-registered with same function", task_name) + return + + cls._tasks[task_name] = func + logger.info( + "Registered async task: %s -> %s.%s", + task_name, + func.__module__, + func.__name__, Review Comment: **Suggestion:** The `logger.info` call assumes the registered `func` has `__module__` and `__name__` attributes; some callable objects (e.g., functools.partial, callable instances) may lack `__name__`, causing an AttributeError during logging. Use safe `getattr` fallbacks to derive module and name without raising. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Registration crash in superset/tasks/registry.py register() - ⚠️ Tasks unavailable to executors (get_executor) ``` </details> ```suggestion getattr(func, "__module__", getattr(getattr(func, "__class__", None), "__module__", "<unknown>")), getattr(func, "__name__", getattr(getattr(func, "__class__", None), "__name__", repr(func))), ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Prepare a Python process and import the registry module: `import superset.tasks.registry` (file: `superset/tasks/registry.py`). 2. Create a callable instance without `__name__` (concrete example): `class C: def __call__(self): pass; c = C()` in a test script. 3. Call `TaskRegistry.register("my_task", c)`. This invokes `register()` at `superset/tasks/registry.py:36-65`; after assigning into `_tasks` it executes the logging lines at `superset/tasks/registry.py:58-64`. 4. When the logger expression evaluates `func.__name__` (line ~63), Python raises AttributeError because the callable instance `c` lacks `__name__`, causing the registration call to raise and abort. Verify with `TaskRegistry.is_registered("my_task")` (method at `superset/tasks/registry.py:82-91`) to see the registration either not completed or interrupted. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/tasks/registry.py **Line:** 62:63 **Comment:** *Possible Bug: The `logger.info` call assumes the registered `func` has `__module__` and `__name__` attributes; some callable objects (e.g., functools.partial, callable instances) may lack `__name__`, causing an AttributeError during logging. Use safe `getattr` fallbacks to derive module and name without raising. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/pages/TaskList/index.tsx: ########## @@ -0,0 +1,627 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { SupersetClient } from '@superset-ui/core'; +import { t, useTheme } from '@apache-superset/core'; +import { useMemo, useCallback, useState } from 'react'; +import { Tooltip, Label, Modal, Checkbox } from '@superset-ui/core/components'; +import { + CreatedInfo, + ListView, + ListViewFilterOperator as FilterOperator, + type ListViewFilters, + FacePile, +} from 'src/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import withToasts from 'src/components/MessageToasts/withToasts'; +import SubMenu from 'src/features/home/SubMenu'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { createErrorHandler, createFetchRelated } from 'src/views/CRUD/utils'; +import TaskStatusIcon from 'src/features/tasks/TaskStatusIcon'; +import TaskPayloadPopover from 'src/features/tasks/TaskPayloadPopover'; +import TaskStackTracePopover from 'src/features/tasks/TaskStackTracePopover'; +import { formatDuration } from 'src/features/tasks/timeUtils'; +import { + Task, + TaskStatus, + TaskScope, + canAbortTask, + isTaskAborting, + TaskSubscriber, +} from 'src/features/tasks/types'; +import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; +import getBootstrapData from 'src/utils/getBootstrapData'; + +const PAGE_SIZE = 25; + +/** + * Typed cell props for react-table columns. + * Replaces `: any` for better type safety in Cell render functions. + */ +interface TaskCellProps { + row: { + original: Task; + }; +} + +interface TaskListProps { + addDangerToast: (msg: string) => void; + addSuccessToast: (msg: string) => void; + user: { + userId: string | number; + firstName: string; + lastName: string; + }; +} + +function TaskList({ addDangerToast, addSuccessToast, user }: TaskListProps) { + const theme = useTheme(); + const { + state: { loading, resourceCount: tasksCount, resourceCollection: tasks }, + fetchData, + refreshData, + } = useListViewResource<Task>('task', t('task'), addDangerToast); + + // Get full user with roles to check admin status + const bootstrapData = getBootstrapData(); + const fullUser = bootstrapData?.user; + const isAdmin = useMemo(() => isUserAdmin(fullUser), [fullUser]); + + // State for cancel confirmation modal + const [cancelModalTask, setCancelModalTask] = useState<Task | null>(null); + const [forceCancel, setForceCancel] = useState(false); + + // Determine dialog message based on task context + const getCancelDialogMessage = useCallback((task: Task) => { + const isSharedTask = task.scope === TaskScope.Shared; + const subscriberCount = task.subscriber_count || 0; + const otherSubscribers = subscriberCount - 1; + + // If it's going to abort (private, system, or last subscriber) + if (!isSharedTask || subscriberCount <= 1) { + return t('This will cancel the task.'); + } + + // Shared task with multiple subscribers + return t( + "You'll be removed from this task. It will continue running for %s other subscriber(s).", + otherSubscribers, + ); + }, []); + + // Get force abort message for admin checkbox + const getForceAbortMessage = useCallback((task: Task) => { + const subscriberCount = task.subscriber_count || 0; + return t( + 'This will abort (stop) the task for all %s subscriber(s).', + subscriberCount, + ); + }, []); + + // Check if current user is subscribed to a task + const isUserSubscribed = useCallback( + (task: Task) => + task.subscribers?.some( + (sub: TaskSubscriber) => sub.user_id === user.userId, Review Comment: **Suggestion:** Comparing `sub.user_id` to `user.userId` with strict equality can fail when one is a string and the other a number (common across APIs and frontend state). Normalizing both sides to the same type (e.g., strings) prevents false negatives and ensures the subscription check works reliably. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Admin force-abort checkbox visibility incorrect (TaskList: line 127). - ❌ Cancel/unsubscribe action hidden for subscribed users (Actions: line 422). - ⚠️ Cancel confirmation pre-check state wrong (openCancelModal: line 182). - ⚠️ UX inconsistency between Subscribers list and action buttons. ``` </details> ```suggestion (sub: TaskSubscriber) => String(sub.user_id) === String(user.userId), ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Build and run the frontend containing the TaskList component (file: superset-frontend/src/pages/TaskList/index.tsx). The TaskList component is mounted when the Tasks page is opened (see the returned JSX around lines ~577-594 where <SubMenu name={t('Tasks')} /> and <ListView> are rendered). 2. Ensure the application displays a task that includes a subscribers array where subscriber.user_id is a different primitive type than the current user prop. For example: backend returns subscriber objects with user_id as a number (e.g., 42) while the TaskList receives the current user prop with userId as a string ('42'). The current user prop shape is defined in this file (TaskListProps, lines 63-71) and the subscribers check is performed by isUserSubscribed (lines 118-124). 3. When the Tasks page renders, isUserSubscribed at superset-frontend/src/pages/TaskList/index.tsx:118 executes task.subscribers?.some((sub) => sub.user_id === user.userId). Because === performs strict comparison, a subscriber with user_id: 42 (number) will not match user.userId: '42' (string), so isUserSubscribed returns false even though the user is actually subscribed. 4. Observe incorrect UI behavior in two concrete places that call/inline the same check: - showForceCancelOption (superset-frontend/src/pages/TaskList/index.tsx:127) uses isUserSubscribed to decide whether to show the admin "Force abort" checkbox. If isUserSubscribed erroneously returns false, the checkbox visibility/initial checked state is wrong for subscribed admins. - Actions column cell (superset-frontend/src/pages/TaskList/index.tsx:422-425) performs original.subscribers?.some((sub) => sub.user_id === user.userId) to determine userIsSubscribed and whether to allow unsubscribe/cancel actions. The strict type mismatch causes Cancel/unsubscribe affordances to be missing or incorrect. Expected: subscribed users (and admins) should see the correct force-cancel checkbox state and Cancel/unsubscribe actions. Observed: UI hides or misconfigures these controls when ID types differ. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/TaskList/index.tsx **Line:** 121:121 **Comment:** *Logic Error: Comparing `sub.user_id` to `user.userId` with strict equality can fail when one is a string and the other a number (common across APIs and frontend state). Normalizing both sides to the same type (e.g., strings) prevents false negatives and ensures the subscription check works reliably. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/pages/TaskList/TaskList.test.tsx: ########## @@ -0,0 +1,399 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { MemoryRouter } from 'react-router-dom'; +import fetchMock from 'fetch-mock'; +import { + render, + screen, + waitFor, + fireEvent, +} from 'spec/helpers/testing-library'; +import { QueryParamProvider } from 'use-query-params'; +import { TaskStatus, TaskScope } from 'src/features/tasks/types'; + +// Set up window.featureFlags before importing TaskList +window.featureFlags = {}; + +// Mock getBootstrapData before importing components that use it +jest.mock('src/utils/getBootstrapData', () => ({ + __esModule: true, + default: () => ({ + user: { + userId: 1, + firstName: 'admin', + lastName: 'user', + roles: { Admin: [] }, + }, + common: { + feature_flags: {}, + conf: {}, + }, + }), +})); + +import TaskList from 'src/pages/TaskList'; + +const tasksInfoEndpoint = 'glob:*/api/v1/task/_info*'; +const tasksCreatedByEndpoint = 'glob:*/api/v1/task/related/created_by*'; +const tasksEndpoint = 'glob:*/api/v1/task/?*'; +const taskCancelEndpoint = 'glob:*/api/v1/task/*/cancel'; + +const mockTasks = [ + { + id: 1, + uuid: 'task-uuid-1', + task_key: 'test_task_1', + task_type: 'data_export', + task_name: 'Export Data Task', + status: TaskStatus.Success, + scope: TaskScope.Private, + created_on: '2024-01-15T10:00:00Z', + changed_on: '2024-01-15T10:05:00Z', + created_on_delta_humanized: '5 minutes ago', + started_at: '2024-01-15T10:00:01Z', + ended_at: '2024-01-15T10:05:00Z', + created_by: { id: 1, first_name: 'admin', last_name: 'user' }, + user_id: 1, + payload: {}, + duration_seconds: 299, + subscriber_count: 0, + subscribers: [], + properties: { + is_abortable: null, + progress_percent: 1.0, + progress_current: null, + progress_total: null, + error_message: null, + exception_type: null, + stack_trace: null, + timeout: null, + }, + }, + { + id: 2, + uuid: 'task-uuid-2', + task_key: 'test_task_2', + task_type: 'report_generation', + task_name: null, + status: TaskStatus.InProgress, + scope: TaskScope.Private, + created_on: '2024-01-15T11:00:00Z', + changed_on: '2024-01-15T11:00:00Z', + created_on_delta_humanized: '1 minute ago', + started_at: '2024-01-15T11:00:01Z', + ended_at: null, + created_by: { id: 1, first_name: 'admin', last_name: 'user' }, + user_id: 1, + payload: { report_id: 42 }, + duration_seconds: null, + subscriber_count: 0, + subscribers: [], + properties: { + is_abortable: true, + progress_percent: 0.5, + progress_current: null, + progress_total: null, + error_message: null, + exception_type: null, + stack_trace: null, + timeout: null, + }, + }, + { + id: 3, + uuid: 'task-uuid-3', + task_key: 'shared_task_1', + task_type: 'bulk_operation', + task_name: 'Shared Bulk Task', + status: TaskStatus.Pending, + scope: TaskScope.Shared, + created_on: '2024-01-15T12:00:00Z', + changed_on: '2024-01-15T12:00:00Z', + created_on_delta_humanized: 'just now', + started_at: null, + ended_at: null, + created_by: { id: 2, first_name: 'other', last_name: 'user' }, + user_id: 2, + payload: {}, + duration_seconds: null, + subscriber_count: 2, + subscribers: [ + { + user_id: 1, + first_name: 'admin', + last_name: 'user', + subscribed_at: '2024-01-15T12:00:00Z', + }, + { + user_id: 2, + first_name: 'other', + last_name: 'user', + subscribed_at: '2024-01-15T12:00:01Z', + }, + ], + properties: { + is_abortable: null, + progress_percent: null, + progress_current: null, + progress_total: null, + error_message: null, + exception_type: null, + stack_trace: null, + timeout: null, + }, + }, +]; + +const mockUser = { + userId: 1, + firstName: 'admin', + lastName: 'user', +}; + +fetchMock.get(tasksInfoEndpoint, { + permissions: ['can_read', 'can_write'], +}); +fetchMock.get(tasksCreatedByEndpoint, { + result: [], +}); +fetchMock.get(tasksEndpoint, { + result: mockTasks, + count: 3, +}); +fetchMock.post(taskCancelEndpoint, { + action: 'aborted', + message: 'Task cancelled', +}); + +const renderTaskList = (props = {}, userProp = mockUser) => + render( + <MemoryRouter> + <QueryParamProvider> + <TaskList {...props} user={userProp} /> + </QueryParamProvider> + </MemoryRouter>, + { useRedux: true }, + ); + +beforeEach(() => { + fetchMock.resetHistory(); Review Comment: **Suggestion:** Test state leak / flakiness: using only `fetchMock.resetHistory()` in `beforeEach` clears call history but does NOT remove or reset routes/overrides set by tests (for example routes registered with `overwriteRoutes: true`). If a test overrides routes and fails before restoring them, later tests may hit the wrong mock route causing intermittent failures. Use `fetchMock.reset()` (or `fetchMock.restore()` depending on desired behavior) to clear routes and history between tests. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Flaky TaskList unit tests in CI. - ⚠️ Intermittent test failures obscure real regressions. - ⚠️ Slows developer feedback loops for tasks feature. ``` </details> ```suggestion // Reset both registered routes and call history to avoid leaking overrides between tests fetchMock.reset(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the TaskList test suite. The global setup in `superset-frontend/src/pages/TaskList/TaskList.test.tsx:194-196` executes `beforeEach()` and calls only `fetchMock.resetHistory()` which clears call history but not registered routes. 2. The test "does not show cancel button for completed shared tasks" at `TaskList.test.tsx:311-378` registers an override for `tasksEndpoint` using `fetchMock.get(tasksEndpoint, { result: [completedSharedTask], count: 1 }, { overwriteRoutes: true })` (route registration shown around lines 351-355). 3. If that test fails before it reaches the code that restores the default mock (the file attempts to re-register the default at `TaskList.test.tsx:373-377`), the override remains registered in fetch-mock. 4. Subsequent tests rely on the default `tasksEndpoint` mock registered at `TaskList.test.tsx:175-178`. Because `beforeEach` did not clear registered routes, later tests will receive the wrong response (the override), causing intermittent failures. Using `fetchMock.reset()` in the `beforeEach` clears routes and prevents this leak. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/TaskList/TaskList.test.tsx **Line:** 195:195 **Comment:** *Possible Bug: Test state leak / flakiness: using only `fetchMock.resetHistory()` in `beforeEach` clears call history but does NOT remove or reset routes/overrides set by tests (for example routes registered with `overwriteRoutes: true`). If a test overrides routes and fails before restoring them, later tests may hit the wrong mock route causing intermittent failures. Use `fetchMock.reset()` (or `fetchMock.restore()` depending on desired behavior) to clear routes and history between tests. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset/commands/tasks/create.py: ########## @@ -0,0 +1,85 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import logging +from functools import partial +from typing import Any, TYPE_CHECKING + +from marshmallow import ValidationError +from superset_core.api.tasks import TaskScope + +from superset.commands.base import BaseCommand +from superset.commands.tasks.exceptions import ( + TaskCreateFailedError, + TaskInvalidError, +) +from superset.daos.exceptions import DAOCreateFailedError +from superset.utils.decorators import on_error, transaction + +if TYPE_CHECKING: + from superset.models.tasks import Task + +logger = logging.getLogger(__name__) + + +class CreateTaskCommand(BaseCommand): + """Command to create a new async task.""" + + def __init__(self, data: dict[str, Any]): + self._properties = data.copy() + + @transaction(on_error=partial(on_error, reraise=TaskCreateFailedError)) + def run(self) -> "Task": + """Execute the command.""" + from superset.daos.tasks import TaskDAO + + self.validate() + try: + return TaskDAO.create_task( + task_type=self._properties["task_type"], + task_key=self._properties.get("task_key"), + scope=self._properties.get("scope", TaskScope.PRIVATE.value), + task_name=self._properties.get("task_name"), + user_id=self._properties.get("user_id"), + payload=self._properties.get("payload", {}), + properties=self._properties.get("properties", {}), + ) + except DAOCreateFailedError as ex: + raise TaskCreateFailedError() from ex + + def validate(self) -> None: + """Validate command parameters.""" + exceptions: list[ValidationError] = [] + + # Require task_type + if not self._properties.get("task_type"): + exceptions.append( + ValidationError("task_type is required", field_name="task_type") + ) + + # Validate scope if provided + scope = self._properties.get("scope", TaskScope.PRIVATE.value) + valid_scopes = [s.value for s in TaskScope] + if scope not in valid_scopes: + exceptions.append( + ValidationError( + f"scope must be one of {valid_scopes}", + field_name="scope", + ) Review Comment: **Suggestion:** `scope` validation assumes `scope` is a string and compares against `TaskScope` values directly, which fails if callers provide a `TaskScope` enum member; normalize the incoming `scope` to its value (accepting enum or string) and validate against a list of allowed values, emitting a clear ValidationError mapping. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ CreateTaskCommand rejects valid enum scope inputs. - ❌ Task creation blocked by false ValidationError. - ⚠️ Inconsistent with superset/daos/tasks.py enum handling. ``` </details> ```suggestion scope = self._properties.get("scope", TaskScope.PRIVATE) # Normalize to the enum value if an enum member was provided scope_value = scope.value if isinstance(scope, TaskScope) else scope valid_scope_values = [s.value for s in TaskScope] if scope_value not in valid_scope_values: exceptions.append( ValidationError({ "scope": [f"scope must be one of {valid_scope_values}"] }) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Locate validation logic in `CreateTaskCommand.validate()` (file: superset/commands/tasks/create.py, scope validation at lines 74-82). 2. Create data using an enum value for scope (common pattern in codebase): e.g. `from superset_core.api.tasks import TaskScope` then `cmd = CreateTaskCommand({"task_type": "x", "scope": TaskScope.SHARED})`. 3. Call `cmd.validate()` or `cmd.run()` (run() calls validate() at line 49 in the same file). 4. Current code compares the enum member (TaskScope.SHARED) to a list of strings (`valid_scopes = [s.value for s in TaskScope]`), the enum is not equal to any string, so validation appends a ValidationError and later `TaskInvalidError` is raised, blocking task creation even though `TaskScope.SHARED` is a valid scope (DAO accepts enum). 5. Evidence of mixed usage: `superset/daos/tasks.py:create_task` accepts TaskScope or str (file: superset/daos/tasks.py, create_task signature and scope handling), showing enum usage is realistic. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/commands/tasks/create.py **Line:** 75:81 **Comment:** *Logic Error: `scope` validation assumes `scope` is a string and compares against `TaskScope` values directly, which fails if callers provide a `TaskScope` enum member; normalize the incoming `scope` to its value (accepting enum or string) and validate against a list of allowed values, emitting a clear ValidationError mapping. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## tests/integration_tests/tasks/commands/test_update.py: ########## @@ -0,0 +1,258 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file to you under +# the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import pytest +from superset_core.api.tasks import TaskScope, TaskStatus + +from superset import db +from superset.commands.tasks import UpdateTaskCommand +from superset.commands.tasks.exceptions import ( + TaskForbiddenError, + TaskNotFoundError, +) +from superset.daos.tasks import TaskDAO + + +def test_update_task_success(app_context, get_user, login_as) -> None: + """Test successful task update""" + admin = get_user("admin") + login_as("admin") + + # Create a task using DAO + task = TaskDAO.create_task( + task_type="test_type", + task_key="update_test", + scope=TaskScope.PRIVATE, + user_id=admin.id, + ) + task.created_by = admin + task.set_status(TaskStatus.IN_PROGRESS) + db.session.commit() + + try: + # Update the task status + command = UpdateTaskCommand( + task_uuid=task.uuid, + status=TaskStatus.SUCCESS.value, + ) + result = command.run() + + # Verify update + assert result.uuid == task.uuid + assert result.status == TaskStatus.SUCCESS.value + + # Verify in database + db.session.refresh(task) + assert task.status == TaskStatus.SUCCESS.value + finally: + # Cleanup + db.session.delete(task) + db.session.commit() + + +def test_update_task_not_found(app_context, login_as) -> None: + """Test update fails when task not found""" + login_as("admin") + + command = UpdateTaskCommand( + task_uuid="00000000-0000-0000-0000-000000000000", + status=TaskStatus.SUCCESS.value, + ) + + with pytest.raises(TaskNotFoundError): + command.run() + + +def test_update_task_forbidden(app_context, get_user, login_as) -> None: + """Test update fails when user doesn't own task (via base filter)""" + gamma = get_user("gamma") + login_as("gamma") + + # Create a task owned by gamma (non-admin) using DAO + task = TaskDAO.create_task( + task_type="test_type", + task_key="forbidden_test", + scope=TaskScope.PRIVATE, + user_id=gamma.id, + ) + task.created_by = gamma + task.set_status(TaskStatus.IN_PROGRESS) + db.session.commit() + + try: + # Login as alpha user (different non-admin, non-owner) + login_as("alpha") + + # Try to update gamma's task as alpha user + command = UpdateTaskCommand( + task_uuid=task.uuid, + status=TaskStatus.SUCCESS.value, + ) + + # Should raise ForbiddenError because ownership check fails + with pytest.raises(TaskForbiddenError): + command.run() + + # Verify task was NOT updated + db.session.refresh(task) + assert task.status == TaskStatus.IN_PROGRESS.value + finally: + # Cleanup + db.session.delete(task) + db.session.commit() Review Comment: **Suggestion:** Several test functions use the same unconditional `finally` cleanup pattern; the same NameError/transaction masking risk appears in other tests (e.g., the forbidden-case test). Replace the `finally` block with a guarded delete + rollback to avoid masking earlier exceptions and to ensure safe cleanup. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Test failures masked across several test functions. - ⚠️ CI runs harder to triage due to misleading errors. ``` </details> ```suggestion if "task" in locals() and task is not None: try: db.session.delete(task) db.session.commit() except Exception: db.session.rollback() ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run tests/integration_tests/tasks/commands/test_update.py::test_update_task_forbidden (function defined around line 80). 2. If TaskDAO.create_task or any earlier setup in the test raises (see superset/daos/tasks.py:create_task at lines 86-206 which can raise DAOCreateFailedError for duplicates or surface DB errors), the test body will jump to the finally block at tests/.../test_update.py lines 113-116. 3. The finally unconditionally calls db.session.delete(task) even when `task` was never assigned, producing a NameError at tests/.../test_update.py:115 that replaces the original exception and may leave a pending DB transaction unhandled. 4. Repeating this pattern across multiple tests (see similar finally blocks at lines ~61-64, ~155-157, ~215-217, ~256-258) means a single setup failure can produce confusing NameErrors in several tests. Guarding deletion and rolling back on DB errors prevents masking and stray transactions. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/tasks/commands/test_update.py **Line:** 115:116 **Comment:** *Possible Bug: Several test functions use the same unconditional `finally` cleanup pattern; the same NameError/transaction masking risk appears in other tests (e.g., the forbidden-case test). Replace the `finally` block with a guarded delete + rollback to avoid masking earlier exceptions and to ensure safe cleanup. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## tests/integration_tests/tasks/commands/test_update.py: ########## @@ -0,0 +1,258 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file to you under +# the Apache License, Version 2.0 (the "License"); you may not +# use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import pytest +from superset_core.api.tasks import TaskScope, TaskStatus + +from superset import db +from superset.commands.tasks import UpdateTaskCommand +from superset.commands.tasks.exceptions import ( + TaskForbiddenError, + TaskNotFoundError, +) +from superset.daos.tasks import TaskDAO + + +def test_update_task_success(app_context, get_user, login_as) -> None: + """Test successful task update""" + admin = get_user("admin") + login_as("admin") + + # Create a task using DAO + task = TaskDAO.create_task( + task_type="test_type", + task_key="update_test", + scope=TaskScope.PRIVATE, + user_id=admin.id, + ) + task.created_by = admin + task.set_status(TaskStatus.IN_PROGRESS) + db.session.commit() + + try: + # Update the task status + command = UpdateTaskCommand( + task_uuid=task.uuid, + status=TaskStatus.SUCCESS.value, + ) + result = command.run() + + # Verify update + assert result.uuid == task.uuid + assert result.status == TaskStatus.SUCCESS.value + + # Verify in database + db.session.refresh(task) + assert task.status == TaskStatus.SUCCESS.value + finally: + # Cleanup + db.session.delete(task) + db.session.commit() Review Comment: **Suggestion:** The cleanup finally block unconditionally references `task` and calls DB operations; if `TaskDAO.create_task` or any earlier line raised before `task` was assigned, the `finally` will raise a NameError and hide the original error (and cause a test failure). Guard deletion with an existence check and handle DB errors (rollback) to avoid masking the root failure and leaking a partial transaction. [resource leak] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Tests may show NameError instead of root cause. - ⚠️ CI debugging obscured by masked exceptions. ``` </details> ```suggestion if "task" in locals() and task is not None: try: db.session.delete(task) db.session.commit() except Exception: db.session.rollback() ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the single test function tests/integration_tests/tasks/commands/test_update.py::test_update_task_success (defined at ~line 30 in the file). 2. Cause TaskDAO.create_task to raise before it returns (e.g. a duplicate active task triggers DAOCreateFailedError in superset/daos/tasks.py:create_task — function range shown at repo path superset/daos/tasks.py lines 86-206). The raise happens inside create_task before the local name `task` is assigned in the test. 3. The exception propagates back into the test, entering the finally block at tests/.../test_update.py lines 61-64. The finally block executes db.session.delete(task) while `task` is not defined, raising NameError at tests/.../test_update.py:63 and masking the original DAOCreateFailedError. 4. Outcome: test failure shows NameError from the cleanup line instead of the original error, and because db.session.commit() is reached in a bad state the original transaction/exception context may be obscured. The proposed guard prevents the NameError and preserves the original exception for debugging. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/tasks/commands/test_update.py **Line:** 63:64 **Comment:** *Resource Leak: The cleanup finally block unconditionally references `task` and calls DB operations; if `TaskDAO.create_task` or any earlier line raised before `task` was assigned, the `finally` will raise a NameError and hide the original error (and cause a test failure). Guard deletion with an existence check and handle DB errors (rollback) to avoid masking the root failure and leaking a partial transaction. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-core/src/superset_core/api/tasks.py: ########## @@ -0,0 +1,361 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +from abc import ABC, abstractmethod +from dataclasses import dataclass +from enum import Enum +from typing import Any, Callable, Generic, Literal, ParamSpec, TypedDict, TypeVar + Review Comment: **Suggestion:** Import-time dependency/circular-import risk and unnecessary runtime import: `Task` is only used as a type annotation in this module but is imported unconditionally at module import time which can cause circular imports or heavyweight imports. Move the import behind a TYPE_CHECKING guard so it's only imported for type checking and not at runtime. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ App startup can raise ImportError on circular imports. - ⚠️ Extra runtime cost importing model at module load. ``` </details> ```suggestion from typing import Any, Callable, Generic, Literal, ParamSpec, TypedDict, TypeVar, TYPE_CHECKING if TYPE_CHECKING: ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Import the tasks module at runtime (e.g., Python `import superset_core.api.tasks`). This executes lines in `superset-core/src/superset_core/api/tasks.py` starting at line 23 and reaches the runtime import at line 25. 2. The module executes `from superset_core.api.models import Task` on line 25, performing a runtime import of `superset_core.api.models`. 3. If `superset_core.api.models` also imports `superset_core.api.tasks` (a common pattern where models and api helpers reference each other), Python will hit a circular import during package initialization: `superset_core.api.models` will attempt to import `tasks` while `tasks` is waiting for `models.Task` to finish importing. 4. The circular import manifests as partially-initialized module attributes or ImportError/AttributeError at startup when code expects `Task` to be available; moving the import under a TYPE_CHECKING guard avoids the runtime import since annotations are postponed by `from __future__ import annotations`, preventing the circular-import path. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-core/src/superset_core/api/tasks.py **Line:** 23:24 **Comment:** *Possible Bug: Import-time dependency/circular-import risk and unnecessary runtime import: `Task` is only used as a type annotation in this module but is imported unconditionally at module import time which can cause circular imports or heavyweight imports. Move the import behind a TYPE_CHECKING guard so it's only imported for type checking and not at runtime. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset/models/tasks.py: ########## @@ -0,0 +1,368 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Task model for Global Task Framework (GTF)""" + +from __future__ import annotations + +import uuid +from datetime import datetime, timezone +from typing import Any, cast + +from flask_appbuilder import Model +from sqlalchemy import ( + Column, + DateTime, + Integer, + String, + Text, +) +from sqlalchemy.orm import relationship +from superset_core.api.models import Task as CoreTask +from superset_core.api.tasks import TaskProperties, TaskStatus + +from superset.models.helpers import AuditMixinNullable +from superset.models.task_subscribers import TaskSubscriber +from superset.tasks.utils import ( + error_update, + get_finished_dedup_key, + parse_properties, + serialize_properties, +) +from superset.utils import json + + +class Task(CoreTask, AuditMixinNullable, Model): + """ + Concrete Task model for the Global Task Framework (GTF). + + This model represents async tasks in Superset, providing unified tracking + for all background operations including SQL queries, thumbnail generation, + reports, and other async operations. + + Non-filterable fields (progress, error info, execution config) are stored + in a `properties` JSON blob for schema flexibility. + """ + + __tablename__ = "tasks" + + # Primary key and identifiers + id = Column(Integer, primary_key=True) + uuid = Column( + String(36), nullable=False, unique=True, default=lambda: str(uuid.uuid4()) + ) + + # Task metadata (filterable) + task_key = Column(String(256), nullable=False, index=True) # For deduplication + task_type = Column(String(100), nullable=False, index=True) # e.g., 'sql_execution' + task_name = Column(String(256), nullable=True) # Human readable name + scope = Column( + String(20), nullable=False, index=True, default="private" + ) # private/shared/system + status = Column( + String(50), nullable=False, index=True, default=TaskStatus.PENDING.value + ) + dedup_key = Column( + String(512), nullable=False, unique=True, index=True Review Comment: **Suggestion:** The `dedup_key` column is declared non-nullable and unique but has no default or initialization in this model; inserting a new Task without explicitly setting `dedup_key` will raise a database IntegrityError. Provide a safe default (e.g., use the task UUID) to ensure new rows can be created without immediate dedup_key assignment. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ ORM inserts may raise IntegrityError on Task creation. - ⚠️ Ad-hoc Task creation in shell or tests fails. ``` </details> ```suggestion String(512), nullable=False, unique=True, index=True, default=lambda: str(uuid.uuid4()) ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a Python shell in the app context (e.g. `flask shell`) so the ORM and models are importable. 2. Import the Task model from superset/models/tasks.py and inspect the column declaration at `superset/models/tasks.py:62-81` (dedup_key defined at `superset/models/tasks.py:78-80`). 3. Attempt to create and commit a Task record without providing dedup_key: - t = Task(task_key="k", task_type="type", user_id=1) - db.session.add(t); db.session.commit() 4. Observe a database IntegrityError on commit because the dedup_key column is declared nullable=False/unique and no default is provided (the model does not set dedup_key before insert). Explanation: The model column at `superset/models/tasks.py:78-80` requires a non-null unique value. If higher-level task-creation code doesn't always set dedup_key prior to session.commit(), an IntegrityError will occur. If your application already ensures dedup_key is computed at creation time (intended design), this error won't occur. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/models/tasks.py **Line:** 79:79 **Comment:** *Possible Bug: The `dedup_key` column is declared non-nullable and unique but has no default or initialization in this model; inserting a new Task without explicitly setting `dedup_key` will raise a database IntegrityError. Provide a safe default (e.g., use the task UUID) to ensure new rows can be created without immediate dedup_key assignment. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
