FINERACT-2493: New command processing - job scheduling#5903
Conversation
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
from
52322d5 to
9851afe
Compare
|
@nidhiii128 Fix that failing checks. |
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
2 times, most recently
from
a1278c6 to
28d0073
Compare
|
@vidakovic please review |
| @Data | ||
| @NoArgsConstructor | ||
| @AllArgsConstructor | ||
| public class UpdateJobResponse implements Serializable { |
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class UpdateJobCommandHandler implements CommandHandler<UpdateJobRequest, UpdateJobResponse> { |
There was a problem hiding this comment.
Naming pattern:
[Domain] + [Action] + "CommandHandler"
... in this case...
JobUpdateCommandHandler
28d0073 to
0e074a5
Compare
|
Hi @vidakovic, thank you, I've addressed all your comments: Renamed DTOs and handlers to follow the [Domain][Action][Suffix] pattern (JobUpdateRequest, JobUpdateResponse, JobUpdateCommandHandler) |
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
4 times, most recently
from
7ba985c to
f459d0c
Compare
nidhiii128
force-pushed
the
FINERACT-2493-update-job
branch
from
f459d0c to
61b2f94
Compare
|
@vidakovic please review again. |
|
@nidhiii128 @Aman-Mittal note for upcoming PRs: can you resolve the conversations that are fixed please? Makes it easier to do the reviews. Thanks. |
| @@ -87,6 +91,7 @@ public class SchedulerJobApiResource { | |||
| private final PlatformSecurityContext context; | |||
| private final FineractProperties fineractProperties; | |||
| private final SqlValidator sqlValidator; | |||
There was a problem hiding this comment.
Is this something that should reside in a service? Or is this validation like in Jakarta Validation of input parameters? SqlValidator is a very low level implementation detail that we shouldn't reveal in the REST layer... my 2 cents.
| @@ -37,11 +37,7 @@ public interface SchedularWritePlatformService { | |||
|
|
|||
| Long fetchMaxVersionBy(String triggerKey); | |||
There was a problem hiding this comment.
"fetch" sounds like a read service function, no? Shouldn't we move that one too to read service?
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class SchedularWritePlatformServiceJpaRepositoryImpl implements SchedularWritePlatformService { |
There was a problem hiding this comment.
Typo, how about SchedulerWritePlatformServiceJpaRepositoryImpl and SchedulerWritePlatformService?
| this.dataValidator = dataValidator; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Read function... read service? Please move and make sure that the write service only exposes functions that change data. While we are here let's clean this stuff up even if it might not be directly related to the project. Please check the rest of the service class for similar stuff. Thanks.
| } | ||
|
|
||
| @Override | ||
| public SchedulerDetail retriveSchedulerDetail() { |
There was a problem hiding this comment.
Typo, how about retrieveSchedulerDetail?
| private Boolean active; | ||
|
|
||
| @JsonIgnore | ||
| @AssertTrue(message = "{org.apache.fineract.infrastructure.jobs.update.at-least-one-field}") |
There was a problem hiding this comment.
Ok, two things:
- Translation key: how about this pattern:
org.apache.fineract.infrastructure.job.update.assertion.at-least-one ... and below...
org.apache.fineract.infrastructure.job.update.assertion.cron-expression
This keeps things short and we have the same number of segments in both keys ("assertions" tells us already that something is validated, so we don't have to use words like "invalid" and similar). Not earth shattering differences, but keeps everything compact and concise.
- Did you actually provide translations for these keys? I don't see any changes in https://github.com/apache/fineract/blob/develop/fineract-validation/src/main/resources/ValidationMessages.properties; please fix.
| private final SchedularWritePlatformService schedularWritePlatformService; | ||
| private final JobRegisterService jobRegisterService; | ||
|
|
||
| @Retry(name = "commandJobUpdate", fallbackMethod = "fallback") |
There was a problem hiding this comment.
I think the resilience configuration for this is missing? Please have a look at this here to see how this works:
. Please add missing entries for this command handler.
| private final CommandDispatcher dispatcher; | ||
|
|
||
| @GET | ||
| @Operation(summary = "Retrieve Scheduler Jobs", operationId = "retrieveAllSchedulerJobs", description = "Returns the list of jobs.\n" |
There was a problem hiding this comment.
Looks like authorization enforcement is missing. In the legacy code these checks are usually done by using PlatformSecurityContext; we really shouldn't, because it obscures the security configuration unnecessarily and makes it really hard if we need to investigate or audit things. So, good that it's removed here... but we have to provide the - correct - equivalent. Please see here
3 participants
Description
Migrates the "update job" REST endpoint (by jobId) from the legacy command-processing path to the new
fineract-commandframework.Scope (PR 1 of 2 for FINERACT-2493)
UpdateJobRequest/UpdateJobResponsePOJOs with Jakarta ValidationUpdateJobCommandandUpdateJobCommandHandlerusingCommandHandler<REQ, RES>SchedularWritePlatformService.updateJobDetailsignature changed to accept typed requestJobDetailDataValidatorno longer called, replaced by@AssertTrueannotationsNot in this PR
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.