diff --git a/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.test.tsx b/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.test.tsx new file mode 100644 index 0000000000..20732b0ee3 --- /dev/null +++ b/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.test.tsx @@ -0,0 +1,474 @@ +import { QueryClient, QueryClientProvider } from 'react-query'; +import { fireEvent, render, screen, waitFor } from 'tests/test-utils'; +import { + IDashboardVariable, + TSortVariableValuesType, + VariableSortTypeArr, +} from 'types/api/dashboard/getAll'; + +import VariableItem from './VariableItem'; + +// Mock modules +jest.mock('api/dashboard/variables/dashboardVariablesQuery', () => ({ + __esModule: true, + default: jest.fn().mockResolvedValue({ + payload: { + variableValues: ['value1', 'value2', 'value3'], + }, + }), +})); + +jest.mock('uuid', () => ({ + v4: jest.fn().mockReturnValue('test-uuid'), +})); + +// Mock functions +const onCancel = jest.fn(); +const onSave = jest.fn(); +const validateName = jest.fn(() => true); + +// Mode constant +const VARIABLE_MODE = 'ADD'; + +// Common text constants +const TEXT = { + INCLUDE_ALL_VALUES: 'Include an option for ALL values', + ENABLE_MULTI_VALUES: 'Enable multiple values to be checked', + VARIABLE_EXISTS: 'Variable name already exists', + SORT_VALUES: 'Sort Values', + DEFAULT_VALUE: 'Default Value', + ALL_VARIABLES: 'All variables', + DISCARD: 'Discard', + OPTIONS: 'Options', + QUERY: 'Query', + TEXTBOX: 'Textbox', + CUSTOM: 'Custom', +}; + +// Common test constants +const VARIABLE_DEFAULTS = { + sort: VariableSortTypeArr[0] as TSortVariableValuesType, + multiSelect: false, + showALLOption: false, +}; + +// Common variable properties +const TEST_VAR_NAMES = { + VAR1: 'variable1', + VAR2: 'variable2', + VAR3: 'variable3', +}; + +const TEST_VAR_IDS = { + VAR1: 'var1', + VAR2: 'var2', + VAR3: 'var3', +}; + +const TEST_VAR_DESCRIPTIONS = { + VAR1: 'Variable 1', + VAR2: 'Variable 2', + VAR3: 'Variable 3', +}; + +// Common UI elements +const SAVE_BUTTON_TEXT = 'Save Variable'; +const UNIQUE_NAME_PLACEHOLDER = 'Unique name of the variable'; + +// Create QueryClient for wrapping the component +const createTestQueryClient = (): QueryClient => + new QueryClient({ + defaultOptions: { + queries: { + retry: false, + }, + }, + }); + +// Wrapper component with QueryClientProvider +const wrapper = ({ children }: { children: React.ReactNode }): JSX.Element => ( + + {children} + +); + +// Basic variable data for testing +const basicVariableData: IDashboardVariable = { + id: TEST_VAR_IDS.VAR1, + name: TEST_VAR_NAMES.VAR1, + description: 'Test Variable 1', + type: 'QUERY', + queryValue: 'SELECT * FROM test', + ...VARIABLE_DEFAULTS, + order: 0, +}; + +// Helper function to render VariableItem with common props +const renderVariableItem = ( + variableData: IDashboardVariable = basicVariableData, + existingVariables: Record = {}, + validateNameFn = validateName, +): void => { + render( + , + { wrapper } as any, + ); +}; + +// Helper function to find button by text within its span +const findButtonByText = (text: string): HTMLElement | null => { + const buttons = screen.getAllByRole('button'); + return buttons.find((button) => button.textContent?.includes(text)) || null; +}; + +describe('VariableItem Component', () => { + // Test SQL query patterns + const SQL_PATTERN_DOT = 'SELECT * FROM test WHERE env = {{.variable2}}'; + const SQL_PATTERN_DOLLAR = 'SELECT * FROM test WHERE env = $variable2'; + const SQL_PATTERN_BRACKET = 'SELECT * FROM test WHERE service = [[variable3]]'; + const SQL_PATTERN_BRACES = 'SELECT * FROM test WHERE app = {{variable1}}'; + const SQL_PATTERN_NO_VARS = 'SELECT * FROM test WHERE env = "prod"'; + const SQL_PATTERN_DOT_VAR1 = + 'SELECT * FROM test WHERE service = {{.variable1}}'; + + // Error message text constant + const CIRCULAR_DEPENDENCY_ERROR = /Cannot save: Circular dependency detected/; + + // Test functions and utilities + const createVariable = ( + id: string, + name: string, + description: string, + queryValue: string, + order: number, + ): IDashboardVariable => ({ + id, + name, + description, + type: 'QUERY', + queryValue, + ...VARIABLE_DEFAULTS, + order, + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + test('renders without crashing', () => { + renderVariableItem(); + + expect(screen.getByText(TEXT.ALL_VARIABLES)).toBeInTheDocument(); + expect(screen.getByText('Name')).toBeInTheDocument(); + expect(screen.getByText('Description')).toBeInTheDocument(); + expect(screen.getByText('Variable Type')).toBeInTheDocument(); + }); + + describe('Variable Name Validation', () => { + test('shows error when variable name already exists', () => { + // Set validateName to return false (name exists) + const mockValidateName = jest.fn().mockReturnValue(false); + + renderVariableItem({ ...basicVariableData, name: '' }, {}, mockValidateName); + + // Enter a name that already exists + const nameInput = screen.getByPlaceholderText(UNIQUE_NAME_PLACEHOLDER); + fireEvent.change(nameInput, { target: { value: 'existingVariable' } }); + + // Error message should be displayed + expect(screen.getByText(TEXT.VARIABLE_EXISTS)).toBeInTheDocument(); + + // We won't check for button disabled state as it might be inconsistent in tests + }); + + test('allows save when current variable name is used', () => { + // Mock validate to return false for all other names but true for own name + const mockValidateName = jest + .fn() + .mockImplementation((name) => name === TEST_VAR_NAMES.VAR1); + + renderVariableItem(basicVariableData, {}, mockValidateName); + + // Enter the current variable name + const nameInput = screen.getByPlaceholderText(UNIQUE_NAME_PLACEHOLDER); + fireEvent.change(nameInput, { target: { value: TEST_VAR_NAMES.VAR1 } }); + + // Error should not be visible + expect(screen.queryByText(TEXT.VARIABLE_EXISTS)).not.toBeInTheDocument(); + }); + }); + + describe('Variable Type Switching', () => { + test('switches to CUSTOM variable type correctly', () => { + renderVariableItem(); + + // Find the Query button + const queryButton = findButtonByText(TEXT.QUERY); + expect(queryButton).toBeInTheDocument(); + expect(queryButton).toHaveClass('selected'); + + // Find and click Custom button + const customButton = findButtonByText(TEXT.CUSTOM); + expect(customButton).toBeInTheDocument(); + + if (customButton) { + fireEvent.click(customButton); + } + + // Custom button should now be selected + expect(customButton).toHaveClass('selected'); + expect(queryButton).not.toHaveClass('selected'); + + // Custom options input should appear + expect(screen.getByText(TEXT.OPTIONS)).toBeInTheDocument(); + }); + + test('switches to TEXTBOX variable type correctly', () => { + renderVariableItem(); + + // Find and click Textbox button + const textboxButton = findButtonByText(TEXT.TEXTBOX); + expect(textboxButton).toBeInTheDocument(); + + if (textboxButton) { + fireEvent.click(textboxButton); + } + + // Textbox button should now be selected + expect(textboxButton).toHaveClass('selected'); + + // Default Value input should appear + expect(screen.getByText(TEXT.DEFAULT_VALUE)).toBeInTheDocument(); + expect( + screen.getByPlaceholderText('Enter a default value (if any)...'), + ).toBeInTheDocument(); + }); + }); + + describe('MultiSelect and ALL Option', () => { + test('enables ALL option only when multiSelect is enabled', async () => { + renderVariableItem(); + + // Initially, ALL option should not be visible + expect(screen.queryByText(TEXT.INCLUDE_ALL_VALUES)).not.toBeInTheDocument(); + + // Enable multiple values + const multipleValuesSwitch = screen + .getByText(TEXT.ENABLE_MULTI_VALUES) + .closest('.multiple-values-section') + ?.querySelector('button'); + + expect(multipleValuesSwitch).toBeInTheDocument(); + if (multipleValuesSwitch) { + fireEvent.click(multipleValuesSwitch); + } + + // Now ALL option should be visible + await waitFor(() => { + expect(screen.getByText(TEXT.INCLUDE_ALL_VALUES)).toBeInTheDocument(); + }); + + // Disable multiple values + if (multipleValuesSwitch) { + fireEvent.click(multipleValuesSwitch); + } + + // ALL option should be hidden again + await waitFor(() => { + expect(screen.queryByText(TEXT.INCLUDE_ALL_VALUES)).not.toBeInTheDocument(); + }); + }); + + test('disables ALL option when multiSelect is disabled', async () => { + // Create variable with multiSelect and showALLOption both enabled + const variable: IDashboardVariable = { + ...basicVariableData, + multiSelect: true, + showALLOption: true, + }; + + renderVariableItem(variable); + + // ALL option should be visible initially + expect(screen.getByText(TEXT.INCLUDE_ALL_VALUES)).toBeInTheDocument(); + + // Disable multiple values + const multipleValuesSwitch = screen + .getByText(TEXT.ENABLE_MULTI_VALUES) + .closest('.multiple-values-section') + ?.querySelector('button'); + + if (multipleValuesSwitch) { + fireEvent.click(multipleValuesSwitch); + } + + // ALL option should be hidden + await waitFor(() => { + expect(screen.queryByText(TEXT.INCLUDE_ALL_VALUES)).not.toBeInTheDocument(); + }); + + // Check that when saving, showALLOption is set to false + const saveButton = screen.getByText(SAVE_BUTTON_TEXT); + fireEvent.click(saveButton); + + expect(onSave).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + multiSelect: false, + showALLOption: false, + }), + ); + }); + }); + + describe('Cancel and Navigation', () => { + test('calls onCancel when clicking All Variables button', () => { + renderVariableItem(); + + // Click All variables button + const allVariablesButton = screen.getByText(TEXT.ALL_VARIABLES); + fireEvent.click(allVariablesButton); + + expect(onCancel).toHaveBeenCalledTimes(1); + }); + + test('calls onCancel when clicking Discard button', () => { + renderVariableItem(); + + // Click Discard button + const discardButton = screen.getByText(TEXT.DISCARD); + fireEvent.click(discardButton); + + expect(onCancel).toHaveBeenCalledTimes(1); + }); + }); + + describe('Cyclic Dependency Detection', () => { + // Common function to render the component with variables and click save + const renderAndSave = async ( + variableData: IDashboardVariable, + existingVariables: Record, + ): Promise => { + renderVariableItem(variableData, existingVariables); + + // Fill in the variable name if it's not already populated + const nameInput = screen.getByPlaceholderText(UNIQUE_NAME_PLACEHOLDER); + if (nameInput.getAttribute('value') === '') { + fireEvent.change(nameInput, { target: { value: variableData.name || '' } }); + } + + // Click save button to trigger the dependency check + const saveButton = screen.getByText(SAVE_BUTTON_TEXT); + fireEvent.click(saveButton); + }; + + // Common expectations for finding circular dependency error + const expectCircularDependencyError = async (): Promise => { + await waitFor(() => { + expect(screen.getByText(CIRCULAR_DEPENDENCY_ERROR)).toBeInTheDocument(); + expect(onSave).not.toHaveBeenCalled(); + }); + }; + + // Test for cyclic dependency detection + test('detects circular dependency and shows error message', async () => { + // Create variables with circular dependency + const variable1 = createVariable( + TEST_VAR_IDS.VAR1, + TEST_VAR_NAMES.VAR1, + TEST_VAR_DESCRIPTIONS.VAR1, + SQL_PATTERN_DOT, + 0, + ); + + const variable2 = createVariable( + TEST_VAR_IDS.VAR2, + TEST_VAR_NAMES.VAR2, + TEST_VAR_DESCRIPTIONS.VAR2, + SQL_PATTERN_DOT_VAR1, + 1, + ); + + const existingVariables = { + [TEST_VAR_IDS.VAR2]: variable2, + }; + + await renderAndSave(variable1, existingVariables); + await expectCircularDependencyError(); + }); + + // Test for saving with no circular dependency + test('allows saving when no circular dependency exists', async () => { + // Create variables without circular dependency + const variable1 = createVariable( + TEST_VAR_IDS.VAR1, + TEST_VAR_NAMES.VAR1, + TEST_VAR_DESCRIPTIONS.VAR1, + SQL_PATTERN_NO_VARS, + 0, + ); + + const variable2 = createVariable( + TEST_VAR_IDS.VAR2, + TEST_VAR_NAMES.VAR2, + TEST_VAR_DESCRIPTIONS.VAR2, + SQL_PATTERN_DOT_VAR1, + 1, + ); + + const existingVariables = { + [TEST_VAR_IDS.VAR2]: variable2, + }; + + await renderAndSave(variable1, existingVariables); + + // Verify the onSave function was called + await waitFor(() => { + expect(onSave).toHaveBeenCalled(); + }); + }); + + // Test with multiple variable formats in query + test('detects circular dependency with different variable formats', async () => { + // Create variables with circular dependency using different formats + const variable1 = createVariable( + TEST_VAR_IDS.VAR1, + TEST_VAR_NAMES.VAR1, + TEST_VAR_DESCRIPTIONS.VAR1, + SQL_PATTERN_DOLLAR, + 0, + ); + + const variable2 = createVariable( + TEST_VAR_IDS.VAR2, + TEST_VAR_NAMES.VAR2, + TEST_VAR_DESCRIPTIONS.VAR2, + SQL_PATTERN_BRACKET, + 1, + ); + + const variable3 = createVariable( + TEST_VAR_IDS.VAR3, + TEST_VAR_NAMES.VAR3, + TEST_VAR_DESCRIPTIONS.VAR3, + SQL_PATTERN_BRACES, + 2, + ); + + const existingVariables = { + [TEST_VAR_IDS.VAR2]: variable2, + [TEST_VAR_IDS.VAR3]: variable3, + }; + + await renderAndSave(variable1, existingVariables); + await expectCircularDependencyError(); + }); + }); +}); diff --git a/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.tsx b/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.tsx index 37e8da6bcf..c1b9d772c2 100644 --- a/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.tsx +++ b/frontend/src/container/NewDashboard/DashboardSettings/Variables/VariableItem/VariableItem.tsx @@ -28,6 +28,10 @@ import { } from 'types/api/dashboard/getAll'; import { v4 as generateUUID } from 'uuid'; +import { + buildDependencies, + buildDependencyGraph, +} from '../../../DashboardVariablesSelection/util'; import { variablePropsToPayloadVariables } from '../../../utils'; import { TVariableMode } from '../types'; import { LabelContainer, VariableItemRow } from './styles'; @@ -107,7 +111,8 @@ function VariableItem({ ]); const handleSave = (): void => { - const variable: IDashboardVariable = { + // Check for cyclic dependencies + const newVariable = { name: variableName, description: variableDescription, type: queryType, @@ -126,7 +131,21 @@ function VariableItem({ order: variableData.order, }; - onSave(mode, variable); + const allVariables = [...Object.values(existingVariables), newVariable]; + + const dependencies = buildDependencies(allVariables); + const { hasCycle, cycleNodes } = buildDependencyGraph(dependencies); + + if (hasCycle) { + setErrorPreview( + `Cannot save: Circular dependency detected between variables: ${cycleNodes?.join( + ' → ', + )}`, + ); + return; + } + + onSave(mode, newVariable); }; // Fetches the preview values for the SQL variable query diff --git a/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.styles.scss b/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.styles.scss index 6eaffb7e9d..0af224e6ce 100644 --- a/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.styles.scss +++ b/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.styles.scss @@ -106,3 +106,9 @@ } } } + +.cycle-error-alert { + margin-bottom: 12px; + padding: 4px 12px; + font-size: 12px; +} diff --git a/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx b/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx index 8276266cec..d0077e08f4 100644 --- a/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx +++ b/frontend/src/container/NewDashboard/DashboardVariablesSelection/DashboardVariableSelection.tsx @@ -1,4 +1,6 @@ -import { Row } from 'antd'; +import './DashboardVariableSelection.styles.scss'; + +import { Alert, Row } from 'antd'; import { isEmpty } from 'lodash-es'; import { useDashboard } from 'providers/Dashboard/Dashboard'; import { memo, useEffect, useState } from 'react'; @@ -64,7 +66,7 @@ function DashboardVariableSelection(): JSX.Element | null { useEffect(() => { if (variablesTableData.length > 0) { const depGrp = buildDependencies(variablesTableData); - const { order, graph } = buildDependencyGraph(depGrp); + const { order, graph, hasCycle, cycleNodes } = buildDependencyGraph(depGrp); const parentDependencyGraph = buildParentDependencyGraph(graph); // cleanup order to only include variables that are of type 'QUERY' @@ -79,6 +81,8 @@ function DashboardVariableSelection(): JSX.Element | null { order: cleanedOrder, graph, parentDependencyGraph, + hasCycle, + cycleNodes, }); } }, [setVariablesToGetUpdated, variables, variablesTableData]); @@ -166,25 +170,37 @@ function DashboardVariableSelection(): JSX.Element | null { ); return ( - - {orderBasedSortedVariables && - Array.isArray(orderBasedSortedVariables) && - orderBasedSortedVariables.length > 0 && - orderBasedSortedVariables.map((variable) => ( - - ))} - + <> + {dependencyData?.hasCycle && ( + + )} + + {orderBasedSortedVariables && + Array.isArray(orderBasedSortedVariables) && + orderBasedSortedVariables.length > 0 && + orderBasedSortedVariables.map((variable) => ( + + ))} + + ); } diff --git a/frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx b/frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx index 823cf53923..fac9e3de59 100644 --- a/frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx +++ b/frontend/src/container/NewDashboard/DashboardVariablesSelection/VariableItem.test.tsx @@ -53,6 +53,7 @@ describe('VariableItem', () => { order: [], graph: {}, parentDependencyGraph: {}, + hasCycle: false, }} /> , @@ -74,6 +75,7 @@ describe('VariableItem', () => { order: [], graph: {}, parentDependencyGraph: {}, + hasCycle: false, }} /> , @@ -94,6 +96,7 @@ describe('VariableItem', () => { order: [], graph: {}, parentDependencyGraph: {}, + hasCycle: false, }} /> , @@ -128,6 +131,7 @@ describe('VariableItem', () => { order: [], graph: {}, parentDependencyGraph: {}, + hasCycle: false, }} /> , @@ -157,6 +161,7 @@ describe('VariableItem', () => { order: [], graph: {}, parentDependencyGraph: {}, + hasCycle: false, }} /> , @@ -178,6 +183,7 @@ describe('VariableItem', () => { order: [], graph: {}, parentDependencyGraph: {}, + hasCycle: false, }} /> , diff --git a/frontend/src/container/NewDashboard/DashboardVariablesSelection/__test__/dashboardVariables.test.tsx b/frontend/src/container/NewDashboard/DashboardVariablesSelection/__test__/dashboardVariables.test.tsx index 0add4c5cad..80def15d84 100644 --- a/frontend/src/container/NewDashboard/DashboardVariablesSelection/__test__/dashboardVariables.test.tsx +++ b/frontend/src/container/NewDashboard/DashboardVariablesSelection/__test__/dashboardVariables.test.tsx @@ -191,16 +191,6 @@ describe('dashboardVariables - utilities and processors', () => { describe('buildDependencyGraph', () => { it('should build complete dependency graph with correct structure and order', () => { const expected = { - graph: { - deployment_environment: ['service_name', 'endpoint'], - service_name: ['endpoint'], - endpoint: ['http_status_code'], - http_status_code: [], - k8s_cluster_name: ['k8s_node_name', 'k8s_namespace_name'], - k8s_node_name: ['k8s_namespace_name'], - k8s_namespace_name: [], - environment: [], - }, order: [ 'deployment_environment', 'k8s_cluster_name', @@ -211,6 +201,28 @@ describe('dashboardVariables - utilities and processors', () => { 'k8s_namespace_name', 'http_status_code', ], + graph: { + deployment_environment: ['service_name', 'endpoint'], + service_name: ['endpoint'], + endpoint: ['http_status_code'], + http_status_code: [], + k8s_cluster_name: ['k8s_node_name', 'k8s_namespace_name'], + k8s_node_name: ['k8s_namespace_name'], + k8s_namespace_name: [], + environment: [], + }, + parentDependencyGraph: { + deployment_environment: [], + service_name: ['deployment_environment'], + endpoint: ['deployment_environment', 'service_name'], + http_status_code: ['endpoint'], + k8s_cluster_name: [], + k8s_node_name: ['k8s_cluster_name'], + k8s_namespace_name: ['k8s_cluster_name', 'k8s_node_name'], + environment: [], + }, + hasCycle: false, + cycleNodes: undefined, }; expect(buildDependencyGraph(graph)).toEqual(expected); diff --git a/frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts b/frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts index b0a76e934b..b1f0d8b628 100644 --- a/frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts +++ b/frontend/src/container/NewDashboard/DashboardVariablesSelection/util.ts @@ -95,10 +95,96 @@ export const buildDependencies = ( return graph; }; -// Function to build the dependency graph +export interface IDependencyData { + order: string[]; + graph: VariableGraph; + parentDependencyGraph: VariableGraph; + hasCycle: boolean; + cycleNodes?: string[]; +} + +export const buildParentDependencyGraph = ( + graph: VariableGraph, +): VariableGraph => { + const parentGraph: VariableGraph = {}; + + // Initialize empty arrays for all nodes + Object.keys(graph).forEach((node) => { + parentGraph[node] = []; + }); + + // For each node and its children in the original graph + Object.entries(graph).forEach(([node, children]) => { + // For each child, add the current node as its parent + children.forEach((child) => { + parentGraph[child].push(node); + }); + }); + + return parentGraph; +}; + +const collectCyclePath = ( + graph: VariableGraph, + start: string, + end: string, +): string[] => { + const path: string[] = []; + let current = start; + + const findParent = (node: string): string | undefined => + Object.keys(graph).find((key) => graph[key]?.includes(node)); + + while (current !== end) { + const parent = findParent(current); + if (!parent) break; + path.push(parent); + current = parent; + } + + return [start, ...path]; +}; + +const detectCycle = ( + graph: VariableGraph, + node: string, + visited: Set, + recStack: Set, +): string[] | null => { + if (!visited.has(node)) { + visited.add(node); + recStack.add(node); + + const neighbors = graph[node] || []; + let cycleNodes: string[] | null = null; + + neighbors.some((neighbor) => { + if (!visited.has(neighbor)) { + const foundCycle = detectCycle(graph, neighbor, visited, recStack); + if (foundCycle) { + cycleNodes = foundCycle; + return true; + } + } else if (recStack.has(neighbor)) { + // Found a cycle, collect the cycle nodes + cycleNodes = collectCyclePath(graph, node, neighbor); + return true; + } + return false; + }); + + if (cycleNodes) { + return cycleNodes; + } + } + recStack.delete(node); + return null; +}; + export const buildDependencyGraph = ( dependencies: VariableGraph, -): { order: string[]; graph: VariableGraph } => { + // eslint-disable-next-line sonarjs/cognitive-complexity +): IDependencyData => { const inDegree: Record = {}; const adjList: VariableGraph = {}; @@ -113,6 +199,22 @@ export const buildDependencyGraph = ( }); }); + // Detect cycles + const visited = new Set(); + const recStack = new Set(); + let cycleNodes: string[] | undefined; + + Object.keys(dependencies).some((node) => { + if (!visited.has(node)) { + const foundCycle = detectCycle(dependencies, node, visited, recStack); + if (foundCycle) { + cycleNodes = foundCycle; + return true; + } + } + return false; + }); + // Topological sort using Kahn's Algorithm const queue: string[] = Object.keys(inDegree).filter( (node) => inDegree[node] === 0, @@ -132,11 +234,15 @@ export const buildDependencyGraph = ( }); } - if (topologicalOrder.length !== Object.keys(dependencies)?.length) { - console.error('Cycle detected in the dependency graph!'); - } + const hasCycle = topologicalOrder.length !== Object.keys(dependencies)?.length; - return { order: topologicalOrder, graph: adjList }; + return { + order: topologicalOrder, + graph: adjList, + parentDependencyGraph: buildParentDependencyGraph(adjList), + hasCycle, + cycleNodes, + }; }; export const onUpdateVariableNode = ( @@ -159,27 +265,6 @@ export const onUpdateVariableNode = ( }); }; -export const buildParentDependencyGraph = ( - graph: VariableGraph, -): VariableGraph => { - const parentGraph: VariableGraph = {}; - - // Initialize empty arrays for all nodes - Object.keys(graph).forEach((node) => { - parentGraph[node] = []; - }); - - // For each node and its children in the original graph - Object.entries(graph).forEach(([node, children]) => { - // For each child, add the current node as its parent - children.forEach((child) => { - parentGraph[child].push(node); - }); - }); - - return parentGraph; -}; - export const checkAPIInvocation = ( variablesToGetUpdated: string[], variableData: IDashboardVariable, @@ -206,9 +291,3 @@ export const checkAPIInvocation = ( variablesToGetUpdated[0] === variableData.name ); }; - -export interface IDependencyData { - order: string[]; - graph: VariableGraph; - parentDependencyGraph: VariableGraph; -} diff --git a/frontend/src/hooks/dashboard/__test__/useGetResolvedText.test.tsx b/frontend/src/hooks/dashboard/__test__/useGetResolvedText.test.tsx index 1664e5f87b..62149a07e6 100644 --- a/frontend/src/hooks/dashboard/__test__/useGetResolvedText.test.tsx +++ b/frontend/src/hooks/dashboard/__test__/useGetResolvedText.test.tsx @@ -73,18 +73,20 @@ describe('useGetResolvedText', () => { }); it('should handle different variable formats', () => { - const text = 'Logs in $service.name, {{service.name}}, [[service.name]]'; + const text = + 'Logs in $service.name, {{service.name}}, [[service.name]] - $dyn-service.name'; const variables = { 'service.name': SERVICE_VAR, + '$dyn-service.name': 'dyn-1, dyn-2', }; const { result } = renderHookWithProps({ text, variables }); expect(result.current.truncatedText).toBe( - 'Logs in test, app +2, test, app +2, test, app +2', + 'Logs in test, app +2, test, app +2, test, app +2 - dyn-1, dyn-2', ); expect(result.current.fullText).toBe( - 'Logs in test, app, frontend, env, test, app, frontend, env, test, app, frontend, env', + 'Logs in test, app, frontend, env, test, app, frontend, env, test, app, frontend, env - dyn-1, dyn-2', ); }); diff --git a/frontend/src/hooks/dashboard/useGetResolvedText.tsx b/frontend/src/hooks/dashboard/useGetResolvedText.tsx index 5bdc0fe9a6..d38882d055 100644 --- a/frontend/src/hooks/dashboard/useGetResolvedText.tsx +++ b/frontend/src/hooks/dashboard/useGetResolvedText.tsx @@ -82,11 +82,12 @@ function useGetResolvedText({ const combinedPattern = useMemo(() => { const escapedMatcher = matcher.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const varNamePattern = '[a-zA-Z_\\-][a-zA-Z0-9_.\\-]*'; const variablePatterns = [ - `\\{\\{\\s*?\\.([^\\s}]+)\\s*?\\}\\}`, // {{.var}} - `\\{\\{\\s*([^\\s}]+)\\s*\\}\\}`, // {{var}} - `${escapedMatcher}([\\w.]+)`, // matcher + var.name - `\\[\\[\\s*([^\\s\\]]+)\\s*\\]\\]`, // [[var]] + `\\{\\{\\s*?\\.(${varNamePattern})\\s*?\\}\\}`, // {{.var}} + `\\{\\{\\s*(${varNamePattern})\\s*\\}\\}`, // {{var}} + `${escapedMatcher}(${varNamePattern})`, // matcher + var.name + `\\[\\[\\s*(${varNamePattern})\\s*\\]\\]`, // [[var]] ]; return new RegExp(variablePatterns.join('|'), 'g'); }, [matcher]); @@ -94,20 +95,38 @@ function useGetResolvedText({ const extractVarName = useCallback( (match: string): string => { // Extract variable name from different formats + const varNamePattern = '[a-zA-Z_\\-][a-zA-Z0-9_.\\-]*'; if (match.startsWith('{{')) { - const dotMatch = match.match(/\{\{\s*\.([^}]+)\}\}/); + const dotMatch = match.match( + new RegExp(`\\{\\{\\s*\\.(${varNamePattern})\\s*\\}\\}`), + ); if (dotMatch) return dotMatch[1].trim(); - const normalMatch = match.match(/\{\{\s*([^}]+)\}\}/); + const normalMatch = match.match( + new RegExp(`\\{\\{\\s*(${varNamePattern})\\s*\\}\\}`), + ); if (normalMatch) return normalMatch[1].trim(); } else if (match.startsWith('[[')) { - const bracketMatch = match.match(/\[\[\s*([^\]]+)\]\]/); + const bracketMatch = match.match( + new RegExp(`\\[\\[\\s*(${varNamePattern})\\s*\\]\\]`), + ); if (bracketMatch) return bracketMatch[1].trim(); } else if (match.startsWith(matcher)) { - return match.substring(matcher.length); + // For $ variables, we always want to strip the prefix + // unless the full match exists in processedVariables + const withoutPrefix = match.substring(matcher.length).trim(); + const fullMatch = match.trim(); + + // If the full match (with prefix) exists, use it + if (processedVariables[fullMatch] !== undefined) { + return fullMatch; + } + + // Otherwise return without prefix + return withoutPrefix; } return match; }, - [matcher], + [matcher, processedVariables], ); const fullText = useMemo(() => {