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

Warn mismatch of bind values #17

Merged
merged 2 commits into from
Jul 28, 2020
Merged

Conversation

egawata
Copy link
Contributor

@egawata egawata commented Jul 27, 2020

DBIx::Sunny's additional methods accepts bind values, even if the number of them doesn't match with the number of placeholders.

For example, select_row accepts the following usages:

$dbh->select_row("SELECT * FROM student WHERE id = ?");          # too few bind values
$dbh->select_row("SELECT * FROM student WHERE id = ?", 1, 2);   # too many bind values

They will issue the following query. (Output by using DBIx::QueryLog)

[2020-07-27T23:51:24] [DBIx::Sunny::Util] [0.000409] SELECT /* sunny_test.pl line 20 */ * FROM student WHERE id = NULL at lib/DBIx/Sunny/Util.pm line 23
[2020-07-27T23:51:24] [DBIx::Sunny::Util] [0.000364] SELECT /* sunny_test.pl line 21 */ * FROM student WHERE id = '1' at lib/DBIx/Sunny/Util.pm line 23

In the former case, placeholder is replaced with NULL. And in the latter, extra value 2 is ignored.

I think those mismatch would not be what the programmer would intend. It would be safe to warn them.

This PR checks the number of bind values and warn if it doesn't match with the number of placeholders.

@kazeburo
Copy link
Owner

I think, it's better die or croak when mismatch. @Songmu How do you think?

@Songmu
Copy link
Collaborator

Songmu commented Jul 27, 2020

I agree with @kazeburo.

Indeed, an exception is raised if there is a mismatch in the argument using the named placeholder currently, so I also think that it is better to match the behavior of the array placeholder as well with throwing an exception when the argument mismatched.

However, we are concerned that throwing an exception may cause an exception in the existing environment.

Nevertheless, as you get it, warnings are often ignored, and it's disastrous when they cause unintended updates to be made. So I also think it is better to throw an exception in such a case.

@egawata
Copy link
Contributor Author

egawata commented Jul 28, 2020

@kazeburo @Songmu
Thank you for your opinions. As you said, it seems better to throw an exception rather than to warn.
I fixed this at 5f2789a

@kazeburo
Copy link
Owner

LGTM!

@kazeburo kazeburo merged commit 990468f into kazeburo:master Jul 28, 2020
kazeburo added a commit that referenced this pull request Jul 29, 2020
Changelog diff is:

diff --git a/Changes b/Changes
index f08be85..b745fa7 100644
--- a/Changes
+++ b/Changes
@@ -2,6 +2,10 @@ Revision history for Perl extension DBIx::Sunny
 
 {{$NEXT}}
 
+0.9992 2020-07-29T23:32:14Z
+
+        - Croak when mismatch of bind values #17
+
 0.9991 2018-07-19T01:18:51Z
 
         - maintenance release
@kazeburo
Copy link
Owner

I released DBIx-Sunny-0.9992.
Thank you!

kazeburo added a commit that referenced this pull request Oct 14, 2023
Changelog diff is:

diff --git a/Changes b/Changes
index b745fa7..e19c886 100644
--- a/Changes
+++ b/Changes
@@ -2,6 +2,11 @@ Revision history for Perl extension DBIx::Sunny
 
 {{$NEXT}}
 
+0.9993 2023-10-14T08:38:01Z
+
+        - Add select_row_as and select_all_as features #19
+
+
 0.9992 2020-07-29T23:32:14Z
 
         - Croak when mismatch of bind values #17
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

3 participants