Skip to content

Overhaul MongoDB Dao to fix various bugs #4898

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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Overhaul MongoDB Dao to fix various bugs #4898

wants to merge 4 commits into from

Conversation

@quaff
Copy link
Contributor

@quaff quaff commented Jul 3, 2025 •
edited
Loading

Now it's covered by tests copied from JDBC implementation.

See GH-4896

@fmbenhassine
Copy link
Contributor

Thank you for the PR! I think that is the right fix. However, can you please add a test that covers the case? With that in place, the PR should be good merge.

@fmbenhassine fmbenhassine added pr-for: bug in: core status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Jul 20, 2025
HyunSangHan
HyunSangHan reviewed Sep 10, 2025
View reviewed changes
Copy link
Contributor

@HyunSangHan HyunSangHan left a comment

Choose a reason for hiding this comment

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

Could you check the feedbacks? :)

@quaff quaff changed the title Fix MongoStepExecutionDao::getLastStepExecution Overhaul MongoDB Dao to fix various bugs Sep 11, 2025
@quaff
Copy link
Contributor Author

quaff commented Sep 11, 2025

Thank you for the PR! I think that is the right fix. However, can you please add a test that covers the case? With that in place, the PR should be good merge.

@fmbenhassine Please review.

@fmbenhassine fmbenhassine added status: feedback-provided Issues for which the feedback requested from the reporter was provided and removed status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Sep 11, 2025
@fmbenhassine fmbenhassine added this to the 6.0.0-RC1 milestone Sep 11, 2025
fmbenhassine added a commit that referenced this pull request Oct 22, 2025
@fmbenhassine
Fix last step execution retrieval in MongoStepExecutionDao
845f2c3 
The patch is taken from #4898, credits to @quaff.

Resolves #4896
@fmbenhassine
Copy link
Contributor

Very interesting fixes in this PR! However, I was not sure about the change of JobParameters so I will review this for RC2.

I wanted to cherry-pick the change to fix #4896 but I couldn't as this PR has a single commit and I didn't want to amend yours. I took the patch in 845f2c3 and gave you credits, thank you! So please remove that part from the PR and I will merge the remaining changes in RC2.

This PR addresses different things (adds missing methods like MongoStepExecutionDao#deleteStepExecution, fixes an issue with job parameters of type Date, etc). We need to create issues that describe each problem separately (with separate commits) to be able to back port what could be back ported to 5.2.x.

@fmbenhassine fmbenhassine added status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter and removed status: feedback-provided Issues for which the feedback requested from the reporter was provided labels Oct 22, 2025
@fmbenhassine fmbenhassine modified the milestones: 6.0.0-RC1, 6.0.0-RC2 Oct 22, 2025
@quaff
Copy link
Contributor Author

quaff commented Oct 22, 2025

This PR addresses different things (adds missing methods like MongoStepExecutionDao#deleteStepExecution, fixes an issue with job parameters of type Date, etc). We need to create issues that describe each problem separately (with separate commits) to be able to back port what could be back ported to 5.2.x.

Should I close this PR and create other PRs?

@fmbenhassine
Copy link
Contributor

As you wish. You can keep a single PR with different commits so I can cherry pick what can be back-ported to 5.2.x. Otherwise you can open separate PRs if you want.

Many thanks upfront 🙏

quaff added 4 commits October 23, 2025 11:20
@quaff
Introduce AbstractMongoDBDaoIntegrationTests
47aacca 
Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@quaff
Overhaul MongoJobInstanceDao
270867b 
1. Implement method `deleteJobInstance()`
2. Fix missing range check for `List.subList()`
3. Introduce MongoJobInstanceDaoIntegrationTests

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@quaff
Overhaul MongoJobExecutionDao
7130e2f 
1. Implement method `deleteJobExecution()`
2. Fix find job executions ordering
3. Convert Date to LocalDate/LocalTime/LocalDateTime
4. Introduce MongoJobExecutionDaoIntegrationTests

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@quaff
Overhaul MongoStepExecutionDao
31469a3 
1. Implement method `deleteStepExecution()`
2. Fix NPE if stepExecutionId not exists when get step execution
3. Fix count step executions by step name
4. Introduce MongoStepExecutionDaoIntegrationTests

Signed-off-by: Yanming Zhou <zhouyanming@gmail.com>
@quaff
Copy link
Contributor Author

quaff commented Oct 23, 2025

As you wish. You can keep a single PR with different commits so I can cherry pick what can be back-ported to 5.2.x. Otherwise you can open separate PRs if you want.

Many thanks upfront 🙏

@fmbenhassine I break it down to 4 commits.

@fmbenhassine
Copy link
Contributor

Thank you for the updates! However, I meant to break the PR down to a commit per feature/fix, not per class. But that is not a problem, I am looking if I can merge this PR as is in main and how I can back port the changes to 5.2.x.

I created a couple issues to track the bugs this PR addresses, see #5060, #5061 and #5062. However, can you please create an issue to explain the problem addressed by https://github.com/spring-projects/spring-batch/pull/4898/files#diff-207077ea36f0c4881806491fff426dd74819f8e9fb1d0966b229e910d00c134eR167?

@quaff
Copy link
Contributor Author

quaff commented Oct 30, 2025

can you please create an issue to explain the problem addressed by https://github.com/spring-projects/spring-batch/pull/4898/files#diff-207077ea36f0c4881806491fff426dd74819f8e9fb1d0966b229e910d00c134eR167?

Created #5063

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@HyunSangHan HyunSangHan HyunSangHan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

in: core pr-for: bug status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter

Projects

None yet

Milestone

6.0.0-RC2

Development

Successfully merging this pull request may close these issues.

3 participants

@quaff @fmbenhassine @HyunSangHan