CharSequence support for Sql statements

[spannm]

This pull request enhances jdbi to allow CharSequence rather than String when passing sql statements to the library e.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.

[stevenschlansker]

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 Sql convenience class for multi-line inline SQL convenience. Have you considered using the new Java text blocks feature, which does much the same thing, but in a more general way?

var mySql = """
    SELECT *
    FROM mytable
    ...
""";

[spannm]

Hi @stevenschlansker, thanks for your feedback!

[...] I'm a little worried that changing the method signature might break binary compatibility.

As String implements CharSequence, it is a (I guess we could call it) 'widening' and fully compatible change to the api. The same was done in Apache commons-lang some time back without further ado.

I see you add a Sql convenience class for multi-line inline SQL convenience. Have you considered using the new Java text blocks feature, which does much the same thing, but in a more general way?

absolutely right, class Sql is purely for convenience and to demonstrate the usefulness of CharSequence. Java multi-line strings have not been introduced until Java 15+ and won't yet be available to a majority of projects that make use of jdbi and other great libraries. We could move the class into a util package, but I was hesitant to create new packages in jdbi. What do you think? cheers

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

[spannm]

Good morning @stevenschlansker, just rebased this PR to master. Saw you released yesterday, nice ;)

[stevenschlansker]

@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 render(String) to render(CharSequence) would be treated as deleting the old one and adding the new one, breaking any users of Jdbi until they recompile.

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 String method signatures and have them delegate to the CharSequence methods as appropriate using explicit casts. Like:

public Query select(String sql, ...) {
    return select((CharSequence) sql, ...);
}

[spannm]

I have rebased this pr to master.

[spannm]

@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 render(String) to render(CharSequence) would be treated as deleting the old one and adding the new one, breaking any users of Jdbi until they recompile.

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.

Hi @stevenschlansker while the changes are compile-time compatible, you are correct that the changes indeed break binary compatibility. I just confirmed with a small sample project. Users of Jdbi will get java.lang.NoSuchMethodError until they rebuild.

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). Found the ticket, too: LANG-510 🙂

[spannm]

I am not in favor of adding delegate methods, as it would be so many of them and clutter the code. Is the only way forward to apply this PR in a 'bigger' release? This being said, will there be a major any time soon? There is only one major of jdbi (3.0.0). The version increments have been all minor (2nd digit) with an occasional patch (3rd digit).

[stevenschlansker]

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.

[spannm]

Hi there Steven (@stevenschlansker)

Unfortunately, I don't think we can break binary compatibility here. [...]

I agree :(

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!

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).

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.

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.

[spannm]

✅ rebased to master

[stevenschlansker]

Seems our Postgres 9.6 tests are flaky :( Retrying to get a clean run.

[stevenschlansker]

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 :)

[spannm]

✅ rebased to master

[spannm]

Hi @stevenschlansker

introduction of delegate methods won't work either due to @FunctionalInterface such as TemplateEngine :(

I will reduce the PR to use CharSequence in those places only where we can reap the most merit.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

[spannm]

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. Have a good week-end.

[spannm]

Hello @stevenschlansker please take another look when you have a moment. It would be nice to complete this PR.