Skip to content
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.

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

Improve developer experience for conditional flows #4460

Closed
hpoettker opened this issue Oct 4, 2023 · 3 comments
Closed

Improve developer experience for conditional flows #4460

hpoettker opened this issue Oct 4, 2023 · 3 comments

Comments

@hpoettker
Copy link
Contributor

Problem

The sample code in the reference documentation for conditional flows makes the critical assumption that stepA() is a proxied bean method. If the method were not proxied, its two invocations would return different instances where the DSL expects to be passed the same instance twice. But the assumption is not explicitly stated.

This leads to a lot of confusion and produces a number of user issues, for example #4429, #4328, #4348, and #4153.

Possible solutions

One possibility would be to adjust the documentation. Either by adding a warning to the section (which might confuse readers to whom the concept of proxied methods is unknown) and/or by changing the code snippet to inject the steps as parameters to the method.

Another possibility would be to change the implementation to be more lenient. There are already places in the code where unique names for a Step are assumed. Thus, AbstractStep could implement equals and hashCode base on the the step name, which should also fix the issue. If this path is chosen, it would make sense to address #3757 at the same time.

@hpoettker hpoettker added status: waiting-for-triage Issues that we did not analyse yet type: feature labels Oct 4, 2023
@fmbenhassine
Copy link
Contributor

fmbenhassine commented Oct 31, 2023

Thank you for raising this issue. We definitely need to update the docs to inject the steps as parameters of the job bean definition method. This injection style makes the dependencies of a bean clear from the signature of its definition method. This is the style that is used in recent additions/updates to the samples and getting started guide. We will also add a warning note about proxied vs non proxied methods for those who prefer using other dependency injection styles. I will plan this for 5.1.

Regarding step names, they should be unique within a flow definition. I don't see how we can detect that two steps are registered with the same name to correctly resolve #3757. I need to dig deeper to see what would be the best way to implement that (ideas are welcome). Adding name based equals and hashCode to AbstractStep makes sense as well, but probably won't change things much after this commit: edc197a. Probably that fix should be reviewed/reverted and fix the original issue in a another way (like detecting if the step is a proxy and get its name from the target object).

@hpoettker
Copy link
Contributor Author

Thanks for the pointer to #857. It seems like the implementation could only depend on step names retrieved from a Step if more of the flow logic were deferred to the actual job execution. That makes things a bit more difficult.

@fmbenhassine
Copy link
Contributor

The samples were updated and a warning was added at the top of the section: https://docs.spring.io/spring-batch/reference/step/controlling-flow.html. Hopefully this is better now.

The programmatic improvement will be addressed in #3757, see #3757 (comment).

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

No branches or pull requests

2 participants