Change StepBuilerHelper#enhance parameter type to AbstractStep

[acktsap]

This commit moves StepBuilerHelper#enhance method to AbstractStep. It removes unnecessary casting to AbstractStep.

Currently, changing argument type to AbstractStep is enough. But until 4.3.x, there were another casting code (It’s been removed by 7c8fb17). But there may be similar cases in future.

By moving enhance method to AbstractStep, we can use like followings for those cases.

// in TaskletStep
@Override
public void enhance(StepProperties properties) {
    super.enhance(properties);
    this.transactionManager = properties.getTransactionManager();
}

This also can be applied to JobBuilderHelper.

I also extracted StepProperties to prevent dependency cycle (org.springframework.batch.core.step.builder <-> org.springframework.batch.core.step)

[fmbenhassine]

Thank you for the PR. Could you please rebase it in on the latest main? Please note that we added an ObservationRegistry and MeterRegistry properties in CommonStepProperties, please make sure to include them in your rebase.

Any reason you renamed CommonStepProperties to StepProperties? I think we should keep the name CommonStepProperties to be consistent with the naming of CommonJobProperties for jobs.

This also can be applied to JobBuilderHelper.

Please open a separate PR for this enhancement. Thank you upfront.

[acktsap]

Rebased and add ObservationRegistry, MeterRegistry. Renamed to CommonStepProperties back.
I'll make a pr for JobBuilderHelper soon.

[fmbenhassine]

Thank you for the update. I did not review this PR in details until you rebased it.

Looking at the change, you moved the enhance method from the builder to the step:

--builder.enhance(step);
++step.enhance(commonStepProperties);

That method is actually configuring/enhancing the step with some properties, and I believe this is the role of the builder, not the step itself. The step can provide setters for its properties, but not a public method enhance (currently, that method is protected in the builder). The addition of a new public method AbstractStep#enhance(CommonStepProperties) means that, as a user, I would need to instantiate CommonStepProperties and call myStep.enhance(properties) for things to work. This is not the case with the current way of configuring steps through the builder.

In my opinion, the way to remove the cast to AbstractStep in StepBuilderHelper#enhance is by changing the parameter type to AbstractStep, because all builders inheriting from StepBuilderHelper currently create AbstractStep types anyway, and that method does nothing for other step types anyway:

protected void enhance(Step target) {
   if (target instanceOf AbstractStep) {
      // configure AbstractStep
   } 

   // Nothing is done for other step types, so this method has no added value for other step types anyway

}

So I guess the way to remove the cast can be achieved like:

--protected void enhance(Step target) {
++protected void enhance(AbstractStep target) {

---   if (target instanceOf AbstractStep) {

---   }

}

I tried this change on the main branch and things seem to work as expected. What do you think?

[acktsap]

That method is actually configuring/enhancing the step with some properties, and I believe this is the role of the builder, not the step itself. The step can provide setters for its properties, but not a public method enhance (currently, that method is protected in the builder)

Make sense. I thought changing parameter type to AbstractStep at first. Then can i make the change (just change the parameter type) in this pr and #4231?

[fmbenhassine]

Then can i make the change (just change the parameter type) in this pr and #4231?

Yes please. It should be good to merge if all tests pass. Thank you upfront!

[acktsap]

Done

[fmbenhassine]

Rebased and merged as 3385a19. Thank you for your contribution!