njnu-seafish commented on PR #18664: URL: https://github.com/apache/dolphinscheduler/pull/18664#issuecomment-5886415612
> Looked at the `insertSchedule` change against `updateSchedule` / `doOnlineScheduler`. > > This is not a privilege bypass: project write permission is still checked, the created schedule stays `OFFLINE`, and `insertSchedule` does not register Quartz. Online still requires the workflow to be `ONLINE`. Replacing `orElse(null)` + `checkWorkflowDefinitionValid` with `orElseThrow` plus the `projectCode` check is also the right existence/ownership guard. > > Optional test tightening in `testInsertScheduleOfflineWorkflow`: assert the captured schedule is `ReleaseState.OFFLINE`, and `verifyNoInteractions(schedulerApi)` so the “saved but not scheduled” contract stays locked. A case that `onlineScheduler` rejects an OFFLINE workflow would help the same way. Thanks for the review. Both suggested test tightenings are now in place in commit 00c1da0740: testInsertScheduleOfflineWorkflow now asserts the captured schedule is persisted as ReleaseState.OFFLINE, and verifies no interaction with schedulerApi, so the "saved but not scheduled" contract stays locked. Added testOnlineSchedulerRejectsOfflineWorkflow to cover that onlineScheduler still rejects an OFFLINE workflow, leaving the schedule unmodified and Quartz unregistered. -- 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]
