njnu-seafish commented on PR #18293:
URL:
https://github.com/apache/dolphinscheduler/pull/18293#issuecomment-5019533170
> The backend still accepts an invalid `default` worker group.
>
> `ProjectWorkerGroupRelationServiceImpl.isWorkerGroupAssignedToProject()`
returns `true` whenever `WorkerGroupUtils.isWorkerGroupEmpty(workerGroup)` is
true. However, that helper treats both an empty value and the literal
`"default"` as empty. In addition, `WorkerGroupValidator` skips validation
entirely for blank values.
>
> As a result, an API client can still submit either an empty worker group
or `"default"` even when the project has not been assigned that group and no
default workers exist. This is exactly the invalid configuration described in
#18286, so removing the frontend default does not fully fix the problem.
>
> There is also inconsistent behavior between the two validation paths:
>
> * Single-value validation always accepts `"default"`.
> * Batch validation treats `"default"` as an ordinary group and may reject
it.
>
> Please resolve the effective worker group first and validate it against
the project's assigned/existing worker groups, or explicitly reject
empty/`"default"` values when no valid default group is available. Both
validation paths should follow the same semantics.
>
> Please also add a service-level test using the real
`ProjectWorkerGroupRelationServiceImpl` behavior for a project that is assigned
only `g_suyc`: `g_suyc` should pass, while empty and `"default"` should fail.
Mocking `isWorkerGroupAssignedToProject()` directly will not catch this
regression.
ok, add some logics.
1, Modified
`ProjectWorkerGroupRelationServiceImpl.isWorkerGroupAssignedToProject()` to:
- Return false for empty/null worker group values
- Treat "default" as an ordinary group name that must be explicitly
assigned
2, Modified `WorkerGroupValidator` (both single and batch validation) to:
- Reject empty/null/blank values with ServiceException
- Treat "default" as an ordinary group name
3, Added service-level tests using real
`ProjectWorkerGroupRelationServiceImpl` behavior
to verify g_suyc passes while empty and "default" fail when project only
has g_suyc assigned
@Test
public void testIsWorkerGroupAssignedToProject() {
Mockito.when(projectDao.queryByCode(projectCode)).thenReturn(getProject());
Mockito.when(projectWorkerGroupDao.queryAssignedWorkerGroupNamesByProjectCode(projectCode))
.thenReturn(Sets.newHashSet("g_suyc")); // 项目只分配了 g_suyc
Mockito.when(taskDefinitionDao.queryAllTaskDefinitionWorkerGroups(projectCode))
.thenReturn(new ArrayList<>());
Mockito.when(scheduleDao.querySchedulerListByProjectName(Mockito.any()))
.thenReturn(Lists.newArrayList());
Assertions.assertTrue(projectWorkerGroupRelationService.isWorkerGroupAssignedToProject(projectCode,
"g_suyc")); // ✅ g_suyc 通过
Assertions.assertFalse(projectWorkerGroupRelationService.isWorkerGroupAssignedToProject(projectCode,
"")); // ❌ 空值拒绝
Assertions.assertFalse(projectWorkerGroupRelationService.isWorkerGroupAssignedToProject(projectCode,
"default")); // ❌ "default" 拒绝
Assertions.assertFalse(projectWorkerGroupRelationService.isWorkerGroupAssignedToProject(projectCode,
null)); // ❌ null 拒绝
Assertions.assertFalse(projectWorkerGroupRelationService.isWorkerGroupAssignedToProject(projectCode,
"unassigned_group")); // ❌ 未分配组拒绝
}
4, Updated `WorkerGroupValidatorTest` to match new behavior with additional
edge case tests
--
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]
