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

Add new attribute RaiseWarn #71

Merged
merged 1 commit into from Apr 30, 2019
Merged

Add new attribute RaiseWarn #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 27 additions & 7 deletions DBI.pm
View file
Open in desktop
Expand Up @@ -1546,7 +1546,7 @@ sub _new_sth { # called by DBD::<drivername>::db::prepare)
# attribute cache, i.e., boolean's and some others
$attr->{$_} = $old_dbh->FETCH($_) for (qw(
AutoCommit ChopBlanks InactiveDestroy AutoInactiveDestroy
LongTruncOk PrintError PrintWarn Profile RaiseError
LongTruncOk PrintError PrintWarn Profile RaiseError RaiseWarn
ShowErrorStatement TaintIn TaintOut
));
}
Expand Down Expand Up @@ -2768,6 +2768,7 @@ The C<AutoCommit> and C<PrintError> attributes for each connection
default to "on". (See L</AutoCommit> and L</PrintError> for more information.)
However, it is strongly recommended that you explicitly define C<AutoCommit>
rather than rely on the default. The C<PrintWarn> attribute defaults to true.
The C<RaiseWarn> attribute defaults to false.

The C<\%attr> parameter can be used to alter the default settings of
C<PrintError>, C<RaiseError>, C<AutoCommit>, and other attributes. For example:
Expand Down Expand Up @@ -3346,7 +3347,7 @@ also sets C<errstr> to undef, and C<state> to C<"">, irrespective
of the values of the $errstr and $state parameters.

The $method parameter provides an alternate method name for the
C<RaiseError>/C<PrintError>/C<PrintWarn> error string instead of
C<RaiseError>/C<PrintError>/C<RaiseWarn>/C<PrintWarn> error string instead of
the fairly unhelpful 'C<set_err>'.

The C<set_err> method normally returns undef. The $rv parameter
Expand Down Expand Up @@ -3789,6 +3790,24 @@ By default, C<DBI-E<gt>connect> sets C<PrintError> "on".
If desired, the warnings can be caught and processed using a C<$SIG{__WARN__}>
handler or modules like CGI::Carp and CGI::ErrorWrap.

=head3 C<RaiseWarn>

Type: boolean, inherited

The C<RaiseWarn> attribute can be used to force warnings to raise exceptions rather
then simply printing them. It is "off" by default.
When set "on", any method which sets warning condition will cause
the DBI to effectively do a C<die("$class $method warning: $DBI::errstr")>,
where C<$class> is the driver class and C<$method> is the name of the method
that sets warning condition. E.g.,

DBD::Oracle::db execute warning: ... warning text here ...

If you turn C<RaiseWarn> on then you'd normally turn C<PrintWarn> off.
If C<PrintWarn> is also on, then the C<PrintWarn> is done first (naturally).

This attribute was added in DBI 1.643.

=head3 C<RaiseError>

Type: boolean, inherited
Expand Down Expand Up @@ -3847,14 +3866,15 @@ Type: code ref, inherited
The C<HandleError> attribute can be used to provide your own alternative behaviour
in case of errors. If set to a reference to a subroutine then that
subroutine is called when an error is detected (at the same point that
C<RaiseError> and C<PrintError> are handled).
C<RaiseError> and C<PrintError> are handled). It is called also when
C<RaiseWarn> is enabled and a warning is detected.

The subroutine is called with three parameters: the error message
string that C<RaiseError> and C<PrintError> would use,
string that C<RaiseError>, C<RaiseWarn> or C<PrintError> would use,
the DBI handle being used, and the first value being returned by
the method that failed (typically undef).

If the subroutine returns a false value then the C<RaiseError>
If the subroutine returns a false value then the C<RaiseError>, C<RaiseWarn>
and/or C<PrintError> attributes are checked and acted upon as normal.

For example, to C<die> with a full stack trace for any error:
Expand Down Expand Up @@ -3883,7 +3903,7 @@ value is important. See L<perlsub> and L<perlref> for more information
about I<closures>.

It is possible for C<HandleError> to alter the error message that
will be used by C<RaiseError> and C<PrintError> if it returns false.
will be used by C<RaiseError>, C<RaiseWarn> and C<PrintError> if it returns false.
It can do that by altering the value of $_[0]. This example appends
a stack trace to all errors and, unlike the previous example using
Carp::confess, this will work C<PrintError> as well as C<RaiseError>:
Expand Down Expand Up @@ -3964,7 +3984,7 @@ Type: boolean, inherited

The C<ShowErrorStatement> attribute can be used to cause the relevant
Statement text to be appended to the error messages generated by
the C<RaiseError>, C<PrintError>, and C<PrintWarn> attributes.
the C<RaiseError>, C<PrintError>, C<RaiseWarn> and C<PrintWarn> attributes.
Only applies to errors on statement handles
plus the prepare(), do(), and the various C<select*()> database handle methods.
(The exact format of the appended text is subject to change.)
Expand Down
17 changes: 13 additions & 4 deletions DBI.xs
View file
Open in desktop
Expand Up @@ -1598,6 +1598,7 @@ dbih_dumpcom(pTHX_ imp_xxh_t *imp_xxh, const char *msg, int level)
if (DBIc_is(imp_xxh, DBIcf_HandleError)) sv_catpv(flags,"HandleError ");
if (DBIc_is(imp_xxh, DBIcf_RaiseError)) sv_catpv(flags,"RaiseError ");
if (DBIc_is(imp_xxh, DBIcf_PrintError)) sv_catpv(flags,"PrintError ");
if (DBIc_is(imp_xxh, DBIcf_RaiseWarn)) sv_catpv(flags,"RaiseWarn ");
if (DBIc_is(imp_xxh, DBIcf_PrintWarn)) sv_catpv(flags,"PrintWarn ");
if (DBIc_is(imp_xxh, DBIcf_ShowErrorStatement)) sv_catpv(flags,"ShowErrorStatement ");
if (DBIc_is(imp_xxh, DBIcf_AutoCommit)) sv_catpv(flags,"AutoCommit ");
Expand Down Expand Up @@ -2109,6 +2110,9 @@ dbih_set_attr_k(SV *h, SV *keysv, int dbikey, SV *valuesv)
else if (strEQ(key, "PrintError")) {
DBIc_set(imp_xxh,DBIcf_PrintError, on);
}
else if (strEQ(key, "RaiseWarn")) {
DBIc_set(imp_xxh,DBIcf_RaiseWarn, on);
}
else if (strEQ(key, "PrintWarn")) {
DBIc_set(imp_xxh,DBIcf_PrintWarn, on);
}
Expand Down Expand Up @@ -2552,6 +2556,9 @@ dbih_get_attr_k(SV *h, SV *keysv, int dbikey)
if (keylen==10 && strEQ(key, "RaiseError")) {
valuesv = boolSV(DBIc_has(imp_xxh,DBIcf_RaiseError));
}
else if (keylen==9 && strEQ(key, "RaiseWarn")) {
valuesv = boolSV(DBIc_has(imp_xxh,DBIcf_RaiseWarn));
}
else if (keylen==12 && strEQ(key, "RowCacheSize")) {
valuesv = &PL_sv_undef;
}
Expand Down Expand Up @@ -3980,8 +3987,8 @@ XS(XS_DBI_dispatch)
&& (
/* is an error and has RaiseError|PrintError|HandleError set */
(SvTRUE(err_sv) && DBIc_has(imp_xxh, DBIcf_RaiseError|DBIcf_PrintError|DBIcf_HandleError))
/* is a warn (not info) and has PrintWarn set */
|| ( SvOK(err_sv) && strlen(SvPV_nolen(err_sv)) && DBIc_has(imp_xxh, DBIcf_PrintWarn))
/* is a warn (not info) and has RaiseWarn|PrintWarn set */
|| ( SvOK(err_sv) && strlen(SvPV_nolen(err_sv)) && DBIc_has(imp_xxh, DBIcf_RaiseWarn|DBIcf_PrintWarn))
)
) {
SV *msg;
Expand Down Expand Up @@ -4042,7 +4049,7 @@ XS(XS_DBI_dispatch)
}

hook_svp = NULL;
if ( SvTRUE(err_sv)
if ( (SvTRUE(err_sv) || (is_warning && DBIc_has(imp_xxh, DBIcf_RaiseWarn)))
Copy link
Member

Choose a reason for hiding this comment

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

With this change HandleError will be called for warnings but only if RaiseWarn is true.
That seems surprising, and also not mentioned in the docs.
You could revert this hunk and remove the mention of !hook_svp in the hunk below, or update the docs and provide a good rationale for the behaviour.
I think I'd prefer the former.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently HandleError is not called for warnings even when PrintWarn is set to true.

My change adds a new RaiseWarn attribute which is by default false. So default behavior of HandleError is not changed, it is not called for warnings.

When RaiseWarn is set to true value, then all warnings are fatal similar to errors and I think it make sense to send them into HandleError callback. So users can handle in HandleError anything which is going to throw an exception from DBI. I think this is better behavior and expected behavior. If you agree I update information about it in documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated documentation for HandleError and rebased this patch on top of master branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

@timbunce Is it now better?

&& DBIc_has(imp_xxh, DBIcf_HandleError)
&& (hook_svp = hv_fetch((HV*)SvRV(h),"HandleError",11,0))
&& hook_svp && SvOK(*hook_svp)
Expand Down Expand Up @@ -4093,9 +4100,11 @@ XS(XS_DBI_dispatch)
dbi_profile(h, imp_xxh, statement_sv, imp_msv ? imp_msv : (SV*)cv,
profile_t1, dbi_time());
}
if (is_warning) {
if (!hook_svp && is_warning) {
if (DBIc_has(imp_xxh, DBIcf_PrintWarn))
warn_sv(msg);
if (DBIc_has(imp_xxh, DBIcf_RaiseWarn))
croak_sv(msg);
}
else if (!hook_svp && SvTRUE(err_sv)) {
if (DBIc_has(imp_xxh, DBIcf_PrintError))
Expand Down
1 change: 1 addition & 0 deletions DBIXS.h
View file
Open in desktop
Expand Up @@ -287,6 +287,7 @@ typedef struct { /* -- FIELD DESCRIPTOR -- */
#define DBIcf_PrintWarn 0x100000 /* warn() on warning (err="0") */
#define DBIcf_Callbacks 0x200000 /* has Callbacks attribute hash */
#define DBIcf_AIADESTROY 0x400000 /* auto DBIcf_IADESTROY if pid changes */
#define DBIcf_RaiseWarn 0x800000 /* throw exception (croak) on warn */
/* NOTE: new flags may require clone() to be updated */

#define DBIcf_INHERITMASK /* what NOT to pass on to children */ \
Expand Down
1 change: 1 addition & 0 deletions MANIFEST
View file
Open in desktop
Expand Up @@ -89,6 +89,7 @@ t/13taint.t
t/14utf8.t
t/15array.t
t/16destroy.t
t/17handle_error.t
t/19fhtrace.t
t/20meta.t
t/30subclass.t
Expand Down
2 changes: 1 addition & 1 deletion lib/DBD/Gofer.pm
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@
AutoInactiveDestroy
PrintError PrintWarn
Profile
RaiseError
RaiseError RaiseWarn
RootClass
ShowErrorStatement
Taint TaintIn TaintOut
Expand Down
12 changes: 9 additions & 3 deletions lib/DBI/PurePerl.pm
View file
Open in desktop
Expand Up @@ -149,6 +149,7 @@ my %is_flag_attribute = map {$_ =>1 } qw(
PrintError
PrintWarn
RaiseError
RaiseWarn
ShowErrorStatement
Warn
);
Expand Down Expand Up @@ -363,11 +364,11 @@ sub _install_method {
&& ($call_depth <= 1 && !$h->{dbi_pp_parent}{dbi_pp_call_depth})
) {

my($pe,$pw,$re,$he) = @{$h}{qw(PrintError PrintWarn RaiseError HandleError)};
my($pe,$pw,$re,$rw,$he) = @{$h}{qw(PrintError PrintWarn RaiseError RaiseWarn HandleError)};
my $msg;

if ($err && ($pe || $re || $he) # error
or (!$err && length($err) && $pw) # warning
or (!$err && length($err) && ($pw || $rw)) # warning
) {
my $last = ($DBI::last_method_except{$method_name})
? ($h->{'dbi_pp_last_method'}||$method_name) : $method_name;
Expand All @@ -388,6 +389,11 @@ sub _install_method {
}
if ($err eq "0") { # is 'warning' (not info)
carp $msg if $pw;
my $do_croak = $rw;
if ((my $subsub = $h->{'HandleError'}) && $do_croak) {
$do_croak = 0 if &$subsub($msg,$h,$ret[0]);
}
die $msg if $do_croak;
}
else {
my $do_croak = 1;
Expand Down Expand Up @@ -498,7 +504,7 @@ sub _setup_handle {
$h_inner->{"Kids"} = $h_inner->{"ActiveKids"} = 0; # XXX not maintained
if ($parent) {
foreach (qw(
RaiseError PrintError PrintWarn HandleError HandleSetErr
RaiseError PrintError RaiseWarn PrintWarn HandleError HandleSetErr
Warn LongTruncOk ChopBlanks AutoCommit ReadOnly
ShowErrorStatement FetchHashKeyName LongReadLen CompatMode
)) {
Expand Down
3 changes: 3 additions & 0 deletions t/06attrs.t
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@ ok(!$dbh->{AutoInactiveDestroy}, '... checking AutoInactiveDestroy attribute for
ok(!$dbh->{PrintError}, '... checking PrintError attribute for dbh');
ok( $dbh->{PrintWarn}, '... checking PrintWarn attribute for dbh'); # true because of perl -w above
ok( $dbh->{RaiseError}, '... checking RaiseError attribute for dbh');
ok(!$dbh->{RaiseWarn}, '... checking RaiseWarn attribute for dbh');
ok(!$dbh->{ShowErrorStatement}, '... checking ShowErrorStatement attribute for dbh');
ok(!$dbh->{ChopBlanks}, '... checking ChopBlanks attribute for dbh');
ok(!$dbh->{LongTruncOk}, '... checking LongTrunkOk attribute for dbh');
Expand Down Expand Up @@ -124,6 +125,7 @@ ok(!$drh->{AutoInactiveDestroy}, '... checking AutoInactiveDestroy attribute for
ok(!$drh->{PrintError}, '... checking PrintError attribute for drh');
ok( $drh->{PrintWarn}, '... checking PrintWarn attribute for drh'); # true because of perl -w above
ok(!$drh->{RaiseError}, '... checking RaiseError attribute for drh');
ok(!$dbh->{RaiseWarn}, '... checking RaiseWarn attribute for dbh');
ok(!$drh->{ShowErrorStatement}, '... checking ShowErrorStatement attribute for drh');
ok(!$drh->{ChopBlanks}, '... checking ChopBlanks attribute for drh');
ok(!$drh->{LongTruncOk}, '... checking LongTrunkOk attribute for drh');
Expand Down Expand Up @@ -193,6 +195,7 @@ ok(!$sth->{AutoInactiveDestroy}, '... checking AutoInactiveDestroy attribute for
ok(!$sth->{PrintError}, '... checking PrintError attribute for sth');
ok( $sth->{PrintWarn}, '... checking PrintWarn attribute for sth');
ok( $sth->{RaiseError}, '... checking RaiseError attribute for sth');
ok(!$dbh->{RaiseWarn}, '... checking RaiseWarn attribute for dbh');
ok(!$sth->{ShowErrorStatement}, '... checking ShowErrorStatement attribute for sth');
ok(!$sth->{ChopBlanks}, '... checking ChopBlanks attribute for sth');
ok(!$sth->{LongTruncOk}, '... checking LongTrunkOk attribute for sth');
Expand Down
4 changes: 4 additions & 0 deletions t/08keeperr.t
View file
Open in desktop
Expand Up @@ -128,6 +128,7 @@ isa_ok($dbh, "DBI::db");

$dbh->{RaiseError} = 1;
$dbh->{PrintError} = 1;
$dbh->{RaiseWarn} = 0;
$dbh->{PrintWarn} = 1;

# warning handler
Expand Down Expand Up @@ -218,6 +219,7 @@ is_deeply(\@handlewarn, [ 2, 1, 0 ], '... the @handlewarn array is (2, 1, 0)');

$dbh->{RaiseError} = 0;
$dbh->{PrintError} = 1;
$dbh->{RaiseWarn} = 1;

# ----

Expand Down Expand Up @@ -335,6 +337,8 @@ SKIP: {
$dbh->{examplep_set_err} = ""; # set information state
cmp_ok($warn{warning}, '==', 0, 'no extra warning generated for set_err("") in STORE');

$dbh->{RaiseWarn} = 0;

$dbh->{examplep_set_err} = "0"; # set warning state
cmp_ok($warn{warning}, '==', 1, 'warning generated for set_err("0") in STORE');
}
Expand Down