Switching incrementers causes jobs to have old job parameters

[lifeweaver]

Bug description
Switching the incrementer for a job causes new instances of that job to keep the old incrementer's parameters around, causing errors if you have a job parameters validator.

Actual error: "JobParametersInvalidException: The JobParameters contains keys that are not explicitly optional or required: [run.id]"

Environment
Spring boot 2.6.4
Spring batch 4.3.5
Java 1.8.0_312
H2 database saved to file

Steps to reproduce
If you start with the code from https://github.com/Apress/def-guide-spring-batch/blob/7db62ca34a1c582fce722f4f266681f29d29b12d/Chapter04/src/main/java/com/example/Chapter04/jobs/HelloWorldJob.java, you should be almost there

1. New project from spring boot initializer with spring batch, and h2
2. Create simple job with a DefaultJobParametersValidator with 'run.id', and the RunIdIncrementer
3. Run the job via the command line
4. Switch the incrementer to something else that doesn't use 'run.id' and remove 'run.id' from your validator, adding your new parameter name
5. Run your job again, and note the error about 'run.id' being "not explicitly optional or required".

Expected behavior
For the new job to work, without caring that the old job use a different incrementor

I believe the JobParametersBuilder.getNextJobParameters() is at fault, specifically:
nextParameters = incrementer.getNext(previousExecution.getJobParameters());
It appears to use the previous execution's job parameters, which would have the old, now invalid, parameter.

This is for sure an edge case, how often, would someone change the incrementer for a job? And I'm sure I can work around this issue if I changed my job name or deleted my h2 database. Hopefully there wasn't some warning about changing the incremeter that I missed.

[fmbenhassine]

If you change job parameters incrementing rules, you should change parameters validation rules as well. Those work together. You can't change the way parameters are incremented without updating the rules of how to validate them. Do you agree?

So in your case, if you switch the incrementer, then you should switch the validator as well to ignore the, now invalid, obsolete parameters used by the previous incrementer (The DefaultJobParametersValidator might not be suitable to your use case and should be changed to a custom validator that works with you new incrementer).

[lifeweaver]

I believe I did change the validator, Removing "run.id", and replacing it with another parameter name:

4. Switch the incrementer to something else that doesn't use 'run.id' and remove 'run.id' from your validator, adding your new parameter name

Unless by " switch the validator" you mean something else?

To me the fact that the code grabs the next incrementor from the previous execution, assuming it's not changed, is the issue. Even if 99.999% the incrementor hasn't changed, it is possible. However I'm not familiar enough with the code to say if it's possible to catch this kind of issue, so it would be nice if at least the documentation mentioned this potential.

[fmbenhassine]

Yes, I meant something else if necessary, that's why I said the DefaultJobParametersValidator might not be suitable to your use case . But removing the old parameter from the required keys in the DefaultJobParametersValidator should be enough.

I tried the scenario you described in this repo. Removing the old parameter from the required keys in the DefaultJobParametersValidator was enough. So even if the old parameter is still passed to the second instance (coming from the previous execution), it should not cause any issue if the validator is updated accordingly to ignore it.

[lifeweaver]

Thank you for looking into this.

if the validator is updated accordingly to ignore it.

I looked at the repo mentioned, it appears to replicate the scenario I described. However since the error "'run.id' being 'not explicitly optional or required'." did not appear. Looking closer, I was able to make the error occur if I added an optional key. If that is the only way to make it happen, it's my bad for not including it in the "Steps to reproduce".

If so then:
I still think the scenario is unexpected and confusing. Again I'm not asking for this behavior to be changed, only that I think updating the documentation to mention this issue would be enough. Of course in that case I guess this ticket would be an enhancement not a bug.

Either way I guess it could be argued that changing an incrementor falls under the "For more complex constraints, you can implement the interface yourself." line in the current documentation:

A job declared in the XML namespace or using any subclass of AbstractJob can optionally declare a validator for the job parameters at runtime. This is useful when, for instance, you need to assert that a job is started with all its mandatory parameters. There is a DefaultJobParametersValidator that you can use to constrain combinations of simple mandatory and optional parameters. For more complex constraints, you can implement the interface yourself.

[fmbenhassine]

Thank you for your feedback. This is a bug in the implementation of DefaultJobParametersValidator, not in the logic of switching the incrementer/validator in SimpleJobLauncher.

In fact, with an optional key in place, removing run.id (or changing it to something else) from the required keys fails at job parameters validation time. When optional keys are present, the DefaultJobParametersValidator expects all parameters to be either required or optional, which is correct, but the error reporting is incorrect IMO. I believe the default validator should fail for missing required keys, but only warn about missing optional keys (currently it fails for both types).

As you mentioned, this could be unexpected and confusing, so it should be fixed (and not documented as you suggest). The fix will introduce a small breaking change, so I will plan it for a minor release rather than a patch release.