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
CharSequence support for Sql statements #2047
Conversation
Hi @sman-81 , thanks for suggesting this change. I'm a little worried that changing the method signature might break binary compatibility. I see you add a var mySql = """
SELECT *
FROM mytable
...
"""; |
Hi @stevenschlansker,
As
absolutely right, class |
91c5a52
to
9690896
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Good morning @stevenschlansker, just rebased this PR to |
@sman-81 , I still think changing the method parameter types is likely to be a problem. I am surprised that it had no compatibility concerns with Apache Commons 3. I'm no expert on the subject, but per https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.4.14 I would think changing Do you think I missed something, or maybe Apache Commons did something special? I did a brief search but didn't find anything about their transition. I think the most surefire way to preserve compatibility, as ugly as it is, is to retain the existing public Query select(String sql, ...) {
return select((CharSequence) sql, ...);
} |
9690896
to
91b8685
Compare
I have rebased this pr to master. |
Hi @stevenschlansker I dug a little into the history of Apache commons-lang3. The switch to CharSequence was done very early on and was already part of the initial 3.0 released in 2011 (after 'forking' from the original commons-lang:commons-lang). |
I am not in favor of adding delegate methods, as it would be so many of them and clutter the code. |
Unfortunately, I don't think we can break binary compatibility here. Too many people link against Jdbi and String is a common data type. Users will be very confused and frustrated when they get NoSuchMethodErrors that disappear only when you recompile the same source code. I am not aware of a better solution than adding delegate methods, but I am totally happy to hear about any ideas you might have! Jdbi is pretty stable and there is no 4.x release planned at this time. When that does happen, we'd have the opportunity to remove these admittedly ugly delegate methods. |
Hi there Steven (@stevenschlansker)
I agree :(
The delegates are certainly a solution. We could introduce them and mark them deprecated for removal, then remove them at a later time (a few versions down the road).
Well, the major version x (as in x.y.z) is effectively used up by the name of the project and its artifact name. One could argue that the minor version y then becomes the actual major version, and that breaking changes are permitted in minor versions. |
91b8685
to
c2d3cc4
Compare
✅ rebased to master |
Seems our Postgres 9.6 tests are flaky :( Retrying to get a clean run. |
I appreciate your point about the artifact name. Regardless of semver rules, I don't think we should break this - so in order for this change to go through, we will need to add the ugly delegate methods, or find another solution to not break compatibility. I am sorry if this isn't your preferred choice but we simply can't break the binary compatibility in this way at this time :) |
c2d3cc4
to
843d256
Compare
✅ rebased to master |
introduction of delegate methods won't work either due to I will reduce the PR to use |
- Allow CharSequence rather than String to pass sql statements to the library. - Add class org.jdbi.v3.core.Sql to write easy-to-read inline sql statements.
843d256
to
e9c17bf
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Hello @stevenschlansker, I have just finished condensing this PR to its essential core (few required delegates introduced). Please take a look when you have a spare moment. |
Hello @stevenschlansker please take another look when you have a moment. It would be nice to complete this PR. |
This pull request enhances jdbi to allow
CharSequence
rather thanString
when passing sql statements to the librarye.g.
public Query(Handle handle, String sql)
becomes
public Query(Handle handle, CharSequence sql)
It also adds class
org.jdbi.v3.core.Sql
to write easy-to-read inline sql statements while Java multi-line strings (from Java 15) are not yet available.