Skip to content

Fix bug in JobParameters.toProperties on null value #3864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix bug in JobParameters.toProperties on null value #3864

wants to merge 1 commit into from

Conversation

acktsap
Copy link
Contributor

@acktsap acktsap commented Mar 11, 2021

fixes #834

Put "" to Properties when null parameter value. Also add some test for it.

@fmbenhassine fmbenhassine added the status: waiting-for-triage Issues that we did not analyse yet label Mar 11, 2021
PascalSchumacher
PascalSchumacher reviewed Mar 11, 2021
View reviewed changes
spring-batch-core/src/main/java/org/springframework/batch/core/JobParameters.java Outdated
if(param.getValue() != null) {
props.put(param.getKey(), param.getValue().toString());
if (param.getValue() != null) {
props.put(param.getKey(), Optional.of(param.getValue()).map(Object::toString).orElse(""));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified to Objects.toString(param.getValue(), "").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment. I've changed to Objects.toString(param.getValue().toString(), "")).

@acktsap acktsap force-pushed the topic/fix-parameters-bug branch from b7dc70a to b4b5b3e Compare March 12, 2021 07:24
@fmbenhassine fmbenhassine added in: core pr-for: bug and removed status: waiting-for-triage Issues that we did not analyse yet labels Mar 12, 2021
@fmbenhassine
Copy link
Contributor

fmbenhassine commented May 18, 2021 •
edited
Loading

Hi @acktsap ,

Thank you for this PR. However, this actually fixes the symptom of the problem and not its root cause. The real issue is that the value of the parameter must not be null in the first place (as well as the result of its toString implementation), as explained in #3913 . Since the correct fix is a breaking change and has been scheduled for v5, I will merge this as a temporary fix for the v4.3 version since it is still maintained (and will be maintained for some time).

Note that I would have fixed the issue by making JobParameter#toString return an empty String instead of null here (a toString method is not expected to return null, otherwise it's not a good idea), but this breaks other tests (namely this one) where it is expected that toString returns null.

Rebased and merged as 4af4b05. Thank you for your contribution.

@fmbenhassine fmbenhassine mentioned this pull request May 18, 2021
Closed
@acktsap acktsap deleted the topic/fix-parameters-bug branch August 10, 2021 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PascalSchumacher PascalSchumacher PascalSchumacher left review comments

Assignees
No one assigned
Labels
in: core pr-for: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

org.springframework.batch.core.JobParameter.toString may return null
3 participants
@acktsap @fmbenhassine @PascalSchumacher