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

Method chaining - AbstractDriverBasedDataSource #25824

Closed
wants to merge 1 commit into from

Conversation

danuvian
Copy link

@danuvian danuvian commented Sep 27, 2020

Make it more convenient for callers when using multiple set methods in a row by allowing method chaining.

My pull request would make the following sample code possible:
var datasource = new SimpleDriverDataSource()
.setDriver(new org.postgresql.Driver())
.setUrl("jdbc:postgresql:my_database")
.setUsername("postgres")
.setPassword("whatever");

My motivation for this pull request was that I was writing some sample code, and I was tired of constantly having to add "object.method" for each set call like this:
var datasource = new SimpleDriverDataSource();
datasource.setDriver(new org.postgresql.Driver())
datasource.setUrl("jdbc:postgresql:my_database")
datasource.setUsername("postgres")
datasource.setPassword("whatever");

Trying to reduce code writing while having the same functionality, and to make it look more elegant and readable.

If my request was approved and merged, the "object.method" way of doing it would still work (backwards-compatible) AND also allow method chaining.

@danuvian danuvian changed the title Method chaining AbstractDriverBasedDataSource Method chaining - AbstractDriverBasedDataSource Sep 27, 2020
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 27, 2020
@quaff
Copy link
Contributor

quaff commented Sep 28, 2020

Changing public method signature is danger, It will break compatibility potentially.

@danuvian
Copy link
Author

danuvian commented Sep 28, 2020

Hi @quaff - if we hypothetically changed the public signature FROM returning an AbstractDriverBaseDataSource TO returning a void, I would agree with you.

In this case, we are doing the opposite. We are returning an object instead of void. All callers to these methods in past will not do anything with a void value. If we return an object to them now, the callers will still do nothing to the object because they expected it to be void. So if we return an object and they don't do anything with it, it should not break anything. Past/present callers can still call the methods as is, without chaining methods, and it will still work. My changes are just a convenience for future callers to chain multiple set methods together.

Updated my first comment (motivation) and examples.

Thank you.

@jhoeller
Copy link
Contributor

Unfortunately, such a change from void to a return value does break binary compatibility in Java bytecode. It is source compatible for newly compiled code (which then just ignores the return value as you pointed out) but breaks when being linked to pre-compiled code (since the bytecode-declared signature is not compatible anymore according to JVM rules). From that perspective, we cannot do such a change for established Spring classes, in particular not just for convenience purposes.

As a side note, standard JavaBeans requires setter methods to have a void return declaration. We have relaxed this rule for several Spring configuration purposes but it still applies for use in other configuration arrangements.

In terms of enabling convenient inline configuration code, we tend to introduce dedicated builder APIs (sometimes in parallel to classic bean-style setters)... or we simply provide overloaded constructors which is also the case for SimpleDriverDataSource. If you prefer a builder API with named methods, you could easily build it yourself on top of SimpleDriverDataSource.

@jhoeller jhoeller closed this Sep 28, 2020
@danuvian danuvian deleted the methodChaining branch September 28, 2020 19:19
@bclozel bclozel removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants