You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "saiaika (via GitHub)" <gi...@apache.org> on 2025/06/28 07:56:57 UTC
[PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
saiaika opened a new pull request, #33960:
URL: https://github.com/apache/superset/pull/33960
## Description
Fixes #33908 - Prevents users from creating bi-directional (cyclic) dependencies between dashboard filters, including indirect cycles that could be created after saving initial configurations.
## Problem
The existing validation only checked for cyclic dependencies during immediate UI interactions, but users could still create indirect cycles by:
1. Setting up Filter A → depends on Filter B
2. Saving the configuration
3. Later editing Filter B → to depend on Filter A
4. Saving again (this would create a cycle but wasn't blocked)
## Solution
- Added comprehensive validation in the `handleSave` method of `FiltersConfigModal.tsx`
- The validation now runs before every save operation and blocks saving if any cyclic dependency is detected
- Added proper error handling with user feedback when cycles are found
- Ensures all filter types are protected from creating bidirectional dependencies
## Changes Made
- **FiltersConfigModal.tsx**: Added `validateDependencies()` call in `handleSave` method with error checking
- **FiltersConfigModal.test.tsx**: Added comprehensive tests covering both immediate and indirect cyclic dependency scenarios
- **ISSUE_33908_FIX_SUMMARY.md**: Detailed documentation of the fix and verification
## Test Coverage
- ✅ Test immediate cyclic dependency blocking (existing functionality preserved)
- ✅ Test indirect cyclic dependency blocking (main issue fix)
- ✅ Test error message display when cycles are detected
- ✅ Test successful save when no cycles exist
- ✅ Verify focus is set to problematic filter when cycle is found
## Technical Details
The fix adds save-time validation by:
1. Calling `validateDependencies()` before saving
2. Checking form fields for dependency validation errors
3. Blocking save operation if cycles are detected
4. Providing user feedback by focusing on the problematic filter
## Verification
This fix has been verified to address the exact scenario described in issue #33908:
- ✅ Indirect cyclic dependencies are now blocked at save time
- ✅ Users receive clear feedback about the cyclic dependency
- ✅ The problematic filter is automatically focused for easy correction
- ✅ All existing functionality remains intact
- ✅ Works for all filter types (select, range, time, etc.)
## Testing
Run the following to test the changes:
```bash
cd superset-frontend
npm test -- FiltersConfigModal.test.tsx
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3064945832
@saiaika You put `i don't have` on the survey form field for slack handle. I don't have any idea who you are in the channel. Would you kindly send me a message there?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3083314235
@saiaika You have some ci errors to fix. We have pre-commit hooks that check linting and formatting, you can find how to set it up in the docs
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2175558116
##########
ISSUE_33908_FIX_SUMMARY.md:
##########
Review Comment:
Why do we have this file?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "saiaika (via GitHub)" <gi...@apache.org>.
saiaika commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3021431059
DELETED ISSUE_33908_FIX_SUMMARY.md
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2173166552
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -466,6 +466,30 @@ function FiltersConfigModal({
handleErroredFilters();
+ // Validate dependencies to prevent saving cyclic dependencies
+ validateDependencies();
Review Comment:
### Incomplete Cyclic Dependency Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validateDependencies function is not preventing all cyclic dependencies as it's only validating the existing dependencies and not potential ones that could be created during the form editing process.
###### Why this matters
If a user is actively editing dependencies in the form, the validation may not catch cycles that could be formed by these pending changes until after they're committed to the form state.
###### Suggested change ∙ *Feature Preview*
Modify the validateDependencies function to include both existing and pending dependencies:
```typescript
const validateDependencies = useCallback(() => {
const dependencyMap = buildDependencyMap();
const formValues = form.getFieldsValue().filters || {};
// Include pending changes from the form
Object.keys(formValues).forEach(key => {
const pendingDeps = formValues[key]?.dependencies || [];
dependencyMap.set(key, pendingDeps);
});
filterIds
.filter(id => !removedFilters[id])
.forEach(filterId => {
const result = hasCircularDependency(dependencyMap, filterId);
const field = {
name: ['filters', filterId, 'dependencies'] as ['filters', string, 'dependencies'],
errors: result ? [t('Cyclic dependency detected')] : [],
};
form.setFields([field]);
});
handleErroredFilters();
}, [buildDependencyMap, filterIds, form, handleErroredFilters, removedFilters]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:283548ea-86ee-489c-a337-060bd648ce90 -->
[](283548ea-86ee-489c-a337-060bd648ce90)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -466,6 +466,30 @@ function FiltersConfigModal({
handleErroredFilters();
+ // Validate dependencies to prevent saving cyclic dependencies
+ validateDependencies();
+
+ // Check if validation added any dependency errors
+ const fieldsWithErrors = form.getFieldsError();
+ const hasDependencyErrors = fieldsWithErrors.some(field =>
+ field.name?.[0] === 'filters' &&
+ field.name?.[2] === 'dependencies' &&
Review Comment:
### Magic Array Indices in Form Field Access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic array indices [0] and [2] are used to access form field names without type safety or clear meaning.
###### Why this matters
Using magic indices makes the code's intent unclear and brittle to changes in the form field structure.
###### Suggested change ∙ *Feature Preview*
```typescript
interface FormField {
name: ['filters', string, 'dependencies'];
errors?: string[];
}
const isDependencyError = (field: FormField) =>
field.name[0] === 'filters' &&
field.name[2] === 'dependencies' &&
field.errors?.length > 0;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d6edb9c7-daca-48e9-9642-8f6202eaf2ec -->
[](d6edb9c7-daca-48e9-9642-8f6202eaf2ec)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2173169120
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -466,6 +466,33 @@ function FiltersConfigModal({
handleErroredFilters();
+ // Validate dependencies to prevent saving cyclic dependencies
+ validateDependencies();
+
+ // Check if validation added any dependency errors
+ const fieldsWithErrors = form.getFieldsError();
+ const FORM_FIELD_FILTERS_INDEX = 0;
+ const FORM_FIELD_DEPENDENCIES_INDEX = 2;
+
+ const hasDependencyErrors = fieldsWithErrors.some(field =>
+ field.name?.[FORM_FIELD_FILTERS_INDEX] === 'filters' &&
+ field.name?.[FORM_FIELD_DEPENDENCIES_INDEX] === 'dependencies' &&
+ field.errors?.length > 0
+ );
+
+ if (hasDependencyErrors) {
+ // Focus on the first filter with dependency errors
+ const errorField = fieldsWithErrors.find(field =>
+ field.name?.[FORM_FIELD_FILTERS_INDEX] === 'filters' &&
+ field.name?.[FORM_FIELD_DEPENDENCIES_INDEX] === 'dependencies' &&
+ field.errors?.length > 0
+ );
Review Comment:
Consider extracting the dependency error check logic (currently using constant indexes for field names) into a separate helper function. This refactoring could improve readability and reduce duplication, making future maintenance easier if the form structure evolves.
```suggestion
const checkDependencyErrors = (fields: any[]) => {
const FORM_FIELD_FILTERS_INDEX = 0;
const FORM_FIELD_DEPENDENCIES_INDEX = 2;
return fields.some(field =>
field.name?.[FORM_FIELD_FILTERS_INDEX] === 'filters' &&
field.name?.[FORM_FIELD_DEPENDENCIES_INDEX] === 'dependencies' &&
field.errors?.length > 0,
);
};
const getFirstDependencyErrorField = (fields: any[]) => {
const FORM_FIELD_FILTERS_INDEX = 0;
const FORM_FIELD_DEPENDENCIES_INDEX = 2;
return fields.find(field =>
field.name?.[FORM_FIELD_FILTERS_INDEX] === 'filters' &&
field.name?.[FORM_FIELD_DEPENDENCIES_INDEX] === 'dependencies' &&
field.errors?.length > 0,
);
};
// Check if validation added any dependency errors
const fieldsWithErrors = form.getFieldsError();
const FORM_FIELD_FILTERS_INDEX = 0;
const FORM_FIELD_DEPENDENCIES_INDEX = 2;
const hasDependencyErrors = checkDependencyErrors(fieldsWithErrors);
if (hasDependencyErrors) {
// Focus on the first filter with dependency errors
const errorField = getFirstDependencyErrorField(fieldsWithErrors);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2175651317
##########
ISSUE_33908_FIX_SUMMARY.md:
##########
Review Comment:
Wondering the same thing, what is this file?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3043554296
@saiaika Can you join our slack channel?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2173604302
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -457,6 +457,38 @@ function FiltersConfigModal({
}
}, [form, erroredFilters]);
+ /**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no dependency errors
+ */
+ const findDependencyError = (fields: any[]) => {
Review Comment:
[nitpick] Extract `findDependencyError` into a shared utility (e.g., `utils.ts`) so that it can be unit tested in isolation and reused across components.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -457,6 +457,38 @@ function FiltersConfigModal({
}
}, [form, erroredFilters]);
+ /**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no dependency errors
+ */
+ const findDependencyError = (fields: any[]) => {
Review Comment:
Consider strongly typing the `fields` parameter (e.g., use Ant Design's FieldError type) and destructuring `field.name` into meaningful variables instead of relying on numeric indexes to improve readability and reduce fragility.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -584,3 +584,199 @@ test('modifies the name of a filter', async () => {
),
);
});
+
+test('prevents saving cyclic dependencies created indirectly', async () => {
+ // Test case: Reproduce exact scenario from GitHub issue #33908
+ // This test verifies that users cannot create A→B dependency, save it,
+ // then later add B→A dependency and save again (which would create a cycle)
+
+ const nativeFilterState = [
+ buildNativeFilter('NATIVE_FILTER-1', 'state', []),
+ buildNativeFilter('NATIVE_FILTER-2', 'country', []),
+ ];
+ const state = {
+ ...defaultState(),
+ dashboardInfo: {
+ metadata: { native_filter_configuration: nativeFilterState },
+ },
+ dashboardLayout,
+ };
+ const onSave = jest.fn();
+ defaultRender(state, {
+ ...props,
+ createNewOnOpen: false,
+ onSave,
+ });
+
+ // STEP 1: Set up Filter 1 → depends on Filter 2 (should succeed)
+ const filterTabs = screen.getAllByRole('tab');
+ userEvent.click(filterTabs[0]); // Select first filter
+
+ // Enable the dependencies option for Filter 1
+ const dependenciesCheckbox = screen.getByRole('checkbox', { name: DEPENDENCIES_REGEX });
+ userEvent.click(dependenciesCheckbox);
+
+ // Set Filter 1 to depend on Filter 2
+ const dependencySelect = screen.getByRole('combobox', { name: /values dependent on/i });
+ userEvent.click(dependencySelect);
+ const filter2Option = screen.getByText('country');
+ userEvent.click(filter2Option);
+
+ // Save this configuration (should work as there's no cycle yet)
+ userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
+ await waitFor(() => expect(onSave).toHaveBeenCalledTimes(1));
+
+ // Verify the save was successful with correct dependency structure
+ expect(onSave).toHaveBeenCalledWith(
+ expect.objectContaining({
+ filterConfig: expect.objectContaining({
+ 'NATIVE_FILTER-1': expect.objectContaining({
+ dependsOn: ['NATIVE_FILTER-2'], // Filter 1 now depends on Filter 2
+ }),
+ 'NATIVE_FILTER-2': expect.objectContaining({
+ dependsOn: [], // Filter 2 has no dependencies
+ }),
+ }),
+ }),
+ );
+
+ // Reset mock for second save attempt
+ onSave.mockClear();
+
+ // STEP 2: Simulate reopening the modal and trying to create reverse dependency
+ // This reproduces the bug scenario: user reopens modal to edit Filter 2
+ defaultRender(state, {
+ ...props,
+ createNewOnOpen: false,
+ onSave,
+ });
+
+ const filterTabs2 = screen.getAllByRole('tab');
+ userEvent.click(filterTabs2[1]); // Select second filter (Filter 2)
+
+ // Try to set Filter 2 to depend on Filter 1 (creating A→B→A cycle)
+ const dependenciesCheckbox2 = screen.getByRole('checkbox', { name: DEPENDENCIES_REGEX });
+ userEvent.click(dependenciesCheckbox2);
+
+ const dependencySelect2 = screen.getByRole('combobox', { name: /values dependent on/i });
+ userEvent.click(dependencySelect2);
+ const filter1Option = screen.getByText('state');
+ userEvent.click(filter1Option);
+
+ // STEP 3: Attempt to save the cyclic configuration (should be blocked)
+ userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
+
+ // Verify that save was prevented due to cyclic dependency detection
+ await waitFor(() => {
+ expect(onSave).not.toHaveBeenCalled();
+ }, { timeout: 3000 });
+
+ // Verify that user receives appropriate error feedback
+ await waitFor(() => {
+ expect(screen.getByText(/cyclic dependency detected/i)).toBeInTheDocument();
+ }, { timeout: 3000 });
+
+ // Verify that focus is moved to the problematic filter for user guidance
+ await waitFor(() => {
+ const activeTab = screen.getByRole('tab', { selected: true });
+ expect(activeTab).toHaveTextContent('country'); // Should focus on Filter 2
+ }, { timeout: 3000 });
Review Comment:
[nitpick] Consider reducing the `waitFor` timeout from 3000ms to a lower value to speed up CI runs, as the UI should update quickly after validation.
```suggestion
}, { timeout: 1000 });
// Verify that user receives appropriate error feedback
await waitFor(() => {
expect(screen.getByText(/cyclic dependency detected/i)).toBeInTheDocument();
}, { timeout: 1000 });
// Verify that focus is moved to the problematic filter for user guidance
await waitFor(() => {
const activeTab = screen.getByRole('tab', { selected: true });
expect(activeTab).toHaveTextContent('country'); // Should focus on Filter 2
}, { timeout: 1000 });
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "saiaika (via GitHub)" <gi...@apache.org>.
saiaika commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3064792013
> @saiaika Can you join our slack channel?
Im in your slack channel
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2176247832
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts:
##########
@@ -49,6 +50,39 @@ export const hasCircularDependency = (
return false;
};
+/**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no dependency errors
+ */
+export const findDependencyError = (fields: any[]) => {
Review Comment:
Using `any[]` for form fields reduces type safety. Consider importing and using Ant Design’s `FormFieldData[]` (or the correct type) to improve maintainability and catch issues at compile time.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts:
##########
@@ -49,6 +50,39 @@ export const hasCircularDependency = (
return false;
};
+/**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no dependency errors
+ */
+export const findDependencyError = (fields: any[]) => {
+ // Form field structure constants for better readability
+ const FORM_FIELD_FILTERS_INDEX = 0;
Review Comment:
[nitpick] These numeric indices (`0`, `1`, `2`) are somewhat opaque. Consider destructuring the `field.name` array (e.g., `[section, filterId, prop]`) for readability instead of relying on magic constants.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "saiaika (via GitHub)" <gi...@apache.org>.
saiaika commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3041861871
@msyavuz Please pay attention
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2194574042
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts:
##########
@@ -49,6 +50,39 @@ export const hasCircularDependency = (
return false;
};
+/**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no dependency errors
+ */
+export const findDependencyError = (fields: any[]) => {
+ // Form field structure constants for better readability
+ const FORM_FIELD_FILTERS_INDEX = 0;
+ const FORM_FIELD_DEPENDENCIES_INDEX = 2;
+ const FILTER_ID_INDEX = 1;
+
+ // Find the first field with dependency validation errors
+ const errorField = fields.find(
+ field =>
+ field.name?.[FORM_FIELD_FILTERS_INDEX] === 'filters' &&
+ field.name?.[FORM_FIELD_DEPENDENCIES_INDEX] === 'dependencies' &&
+ field.errors?.length > 0,
+ );
+
+ if (errorField) {
+ return {
+ hasError: true,
+ filterId: errorField.name[FILTER_ID_INDEX] as string,
+ errors: errorField.errors,
+ };
+ }
+
+ return null;
+};
Review Comment:
```suggestion
interface FieldError {
name: (string | number)[];
errors: string[];
warnings: string[];
}
/**
* Helper function to find dependency validation errors in form fields.
* This function improves code maintainability by centralizing the logic
* for detecting cyclic dependency errors and extracting the problematic filter ID.
*
* @param fields - Array of form field error objects from Ant Design Form
* @returns Object containing error status and filter ID, or null if no dependency errors
*/
export const findDependencyError = (fields: FieldError[]) => {
const FILTER_ID_INDEX = 1;
const errorField = fields.find(({ name, errors }) => {
const [path, subpath, id] = name;
return (
path === 'filters' &&
subpath === 'dependencies' &&
typeof id === 'string' &&
errors?.length > 0
);
});
if (!errorField) {
return null;
}
const filterId = errorField.name[FILTER_ID_INDEX];
return {
hasError: true,
filterId: filterId as string,
errors: errorField.errors,
};
};
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cyclic dependencies in dashboard filters (#33908) [superset]
Posted by "saiaika (via GitHub)" <gi...@apache.org>.
saiaika commented on PR #33960:
URL: https://github.com/apache/superset/pull/33960#issuecomment-3065241517
> @saiaika You put `i don't have` on the survey form field for slack handle. I don't have any idea who you are in the channel. Would you kindly send me a message there?
I filled out that form earlier and later joined Slack channel
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org