Copilot commented on code in PR #18003: URL: https://github.com/apache/dolphinscheduler/pull/18003#discussion_r2963817593
########## dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/executor/workflow/BackfillWorkflowExecutorDelegateTest.java: ########## @@ -0,0 +1,327 @@ +/* + * 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. + */ + +package org.apache.dolphinscheduler.api.executor.workflow; + +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.apache.dolphinscheduler.api.service.WorkflowLineageService; +import org.apache.dolphinscheduler.api.validator.workflow.BackfillWorkflowDTO; +import org.apache.dolphinscheduler.common.enums.ComplementDependentMode; +import org.apache.dolphinscheduler.common.enums.ExecutionOrder; +import org.apache.dolphinscheduler.common.enums.ReleaseState; +import org.apache.dolphinscheduler.common.enums.RunMode; +import org.apache.dolphinscheduler.dao.entity.DependentWorkflowDefinition; +import org.apache.dolphinscheduler.dao.entity.WorkflowDefinition; +import org.apache.dolphinscheduler.dao.repository.WorkflowDefinitionDao; + +import java.lang.reflect.Method; +import java.time.ZonedDateTime; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +public class BackfillWorkflowExecutorDelegateTest { + + @Spy + @InjectMocks + private BackfillWorkflowExecutorDelegate backfillWorkflowExecutorDelegate; + + @Mock + private WorkflowLineageService workflowLineageService; + + @Mock + private WorkflowDefinitionDao workflowDefinitionDao; + + @Test + public void testDoBackfillDependentWorkflow_NoDownstreamDefinitions() throws Exception { + long upstreamCode = 1L; + WorkflowDefinition upstreamWorkflow = + WorkflowDefinition.builder() + .code(upstreamCode) + .releaseState(ReleaseState.ONLINE) + .build(); + + BackfillWorkflowDTO.BackfillParamsDTO params = BackfillWorkflowDTO.BackfillParamsDTO.builder() + .runMode(RunMode.RUN_MODE_SERIAL) + .backfillDateList(Collections.<ZonedDateTime>emptyList()) + .backfillDependentMode(ComplementDependentMode.ALL_DEPENDENT) + .allLevelDependent(true) + .executionOrder(ExecutionOrder.ASC_ORDER) + .build(); + + BackfillWorkflowDTO dto = BackfillWorkflowDTO.builder() + .workflowDefinition(upstreamWorkflow) + .backfillParams(params) + .build(); + + when(workflowLineageService.queryDownstreamDependentWorkflowDefinitions(upstreamCode)) + .thenReturn(Collections.emptyList()); + + Method method = BackfillWorkflowExecutorDelegate.class.getDeclaredMethod( + "doBackfillDependentWorkflow", BackfillWorkflowDTO.class, List.class, Set.class); + method.setAccessible(true); + + List<String> backfillTimeList = Collections.singletonList("2026-02-01 00:00:00"); + + Set<Long> visitedCodes = new HashSet<>(); + visitedCodes.add(dto.getWorkflowDefinition().getCode()); + method.invoke(backfillWorkflowExecutorDelegate, dto, backfillTimeList, visitedCodes); + + verify(workflowDefinitionDao, never()).queryByCode(anyLong()); + } + + @Test + public void testDoBackfillDependentWorkflow_WithDownstream_AllLevelDependent() throws Exception { + long upstreamCode = 10L; + long downstreamCode = 20L; + + WorkflowDefinition upstreamWorkflow = Review Comment: The new tests cover only single-hop dependent triggering and filtering (self, offline, missing). They don't exercise multi-level recursion or cycle detection behavior for `allLevelDependent=true` (e.g., A -> B -> C, and A -> B -> A), which is where the new visited-set logic is most critical. Adding a test that simulates a small dependency graph and asserts triggering stops at already-visited nodes would help prevent regressions. ########## dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/executor/workflow/BackfillWorkflowExecutorDelegate.java: ########## @@ -166,13 +179,100 @@ private Integer doBackfillWorkflow(final BackfillWorkflowDTO backfillWorkflowDTO } final BackfillWorkflowDTO.BackfillParamsDTO backfillParams = backfillWorkflowDTO.getBackfillParams(); if (backfillParams.getBackfillDependentMode() == ComplementDependentMode.ALL_DEPENDENT) { - doBackfillDependentWorkflow(backfillWorkflowDTO, backfillTimeList); + final Set<Long> visitedCodes = new HashSet<>(); + visitedCodes.add(backfillWorkflowDTO.getWorkflowDefinition().getCode()); + doBackfillDependentWorkflow(backfillWorkflowDTO, backfillTimeList, visitedCodes); } Review Comment: `visitedCodes` is re-initialized inside `doBackfillWorkflow`, but dependent triggering calls `execute(dependentBackfillDTO)`, which will create a *new* `visitedCodes` set for the downstream workflow. This breaks cycle/duplicate protection across multiple levels and can lead to infinite recursion and repeated backfill triggers when there is a dependency cycle (e.g., A -> B -> A). Consider threading the same `visitedCodes` through downstream triggers (e.g., an overload that accepts the set, or moving recursive traversal into `doBackfillDependentWorkflow` without resetting state) so cycle detection works for all levels. ########## dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/executor/workflow/BackfillWorkflowExecutorDelegate.java: ########## @@ -166,13 +179,100 @@ private Integer doBackfillWorkflow(final BackfillWorkflowDTO backfillWorkflowDTO } final BackfillWorkflowDTO.BackfillParamsDTO backfillParams = backfillWorkflowDTO.getBackfillParams(); Review Comment: The PR description template currently indicates "code cleanup without any test coverage", but this PR introduces new backfill-dependent workflow behavior and adds a new unit test class. Please update the PR description's verification section to accurately reflect the functional change and how it was tested. -- 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]
