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.

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

Prepared statements: Support bound parameters of type CharSequence #2057

Merged
merged 3 commits into from Jul 5, 2022
Merged

Prepared statements: Support bound parameters of type CharSequence #2057

merged 3 commits into from Jul 5, 2022

Conversation

spannm
Copy link
Contributor

@spannm spannm commented Jun 15, 2022

This small pull request adds support for bound parameters of type CharSequence in prepared statements.
Such parameters are to be treated same as String.

stevenschlansker
stevenschlansker approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@stevenschlansker stevenschlansker left a comment

Choose a reason for hiding this comment

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

The addition looks good - thanks. It would be nice to add a test case for this as well, just to make sure everything works and stays working.

@spannm
Copy link
Contributor Author

spannm commented Jun 16, 2022

good morning @stevenschlansker
I'll look into adding a test case to this PR.

@spannm spannm force-pushed the sman-81-jdbi-bound-parm-charsequence branch from 3c8afb0 to a473a83 Compare June 16, 2022 09:15
@spannm
Copy link
Contributor Author

spannm commented Jun 16, 2022

Hi @stevenschlansker,
I found a more versatile solution for CharSequence support in parameters and added tests.
EssentialsArgumentFactory only works on the explicit type. A (low-priority) factory that handles parameters that implement CharSequence and that are not handled by a more specific or user-defined factory is what we would want.
I have squashed and force pushed the update.
Please take a look - thanks.

stevenschlansker
stevenschlansker reviewed Jun 16, 2022
View reviewed changes
core/src/main/java/org/jdbi/v3/core/argument/CharSequenceArgumentFactory.java Outdated
@Override
protected Argument build(CharSequence value, ConfigRegistry config) {
return (position, statement, ctx) -> {
StatementBinder<String> binder = PreparedStatement::setString;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why the binder here is helpful. Wouldn't it be simpler to just call statement.setString(position, Objects.toString(value, null));?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Fixed.

@spannm spannm force-pushed the sman-81-jdbi-bound-parm-charsequence branch from 9fabe76 to 917d62d Compare June 17, 2022 10:48
@sonarcloud
Copy link

sonarcloud bot commented Jun 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stevenschlansker stevenschlansker merged commit add0af5 into jdbi:master Jul 5, 2022
@spannm spannm deleted the sman-81-jdbi-bound-parm-charsequence branch July 7, 2022 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenschlansker stevenschlansker stevenschlansker approved these changes

Assignees
No one assigned
Labels
feature
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@spannm @stevenschlansker @hgschmie