From: Curtis "Ovid" Poe Date: Wed, 16 Dec 2009 16:40:50 +0000 (+0000) Subject: Have has_one/might_have warn if set on nullable columns. X-Git-Tag: v0.08116~96 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=dc571b762ce7865d904f506bf890600543c9de81;p=dbsrgits%2FDBIx-Class.git Have has_one/might_have warn if set on nullable columns. --- diff --git a/Changes b/Changes index 7db2a7f..2e25b36 100644 --- a/Changes +++ b/Changes @@ -1,5 +1,8 @@ Revision history for DBIx::Class + - might_have/has_one now warn if applied calling class's column + has is_nullable set to true. + 0.08115 2009-12-10 09:02:00 (CST) - Real limit/offset support for MSSQL server (via Row_Number) - Fix distinct => 1 with non-selecting order_by (the columns diff --git a/lib/DBIx/Class.pm b/lib/DBIx/Class.pm index f6b0705..cbe5988 100644 --- a/lib/DBIx/Class.pm +++ b/lib/DBIx/Class.pm @@ -294,6 +294,8 @@ norbi: Norbert Buchmuller Numa: Dan Sully +ovid: Curtis "Ovid" Poe + oyse: Øystein Torget paulm: Paul Makepeace diff --git a/lib/DBIx/Class/Relationship.pm b/lib/DBIx/Class/Relationship.pm index 3351391..e1bed80 100644 --- a/lib/DBIx/Class/Relationship.pm +++ b/lib/DBIx/Class/Relationship.pm @@ -441,6 +441,17 @@ methods and valid relationship attributes. Also see L for a L which can be assigned to relationships as well. +Note that if you supply a condition on which to join, if the column in the +current table allows nulls (i.e., has the C attribute set to a +true value), than C will warn about this because it's naughty and +you shouldn't do that. + + "might_have/has_one" must not be on columns with is_nullable set to true (MySchema::SomeClass/key) + +If you must be naughty, you can suppress the warning by setting +C environment variable to a true value. Otherwise, +you probably just want to use C. + =head2 has_one =over 4 @@ -528,6 +539,11 @@ methods and valid relationship attributes. Also see L for a L which can be assigned to relationships as well. +Note that if you supply a condition on which to join, if the column in the +current table allows nulls (i.e., has the C attribute set to a +true value), than warnings might apply just as with +L. + =head2 many_to_many =over 4 diff --git a/lib/DBIx/Class/Relationship/HasOne.pm b/lib/DBIx/Class/Relationship/HasOne.pm index 4c910b8..73cb929 100644 --- a/lib/DBIx/Class/Relationship/HasOne.pm +++ b/lib/DBIx/Class/Relationship/HasOne.pm @@ -3,6 +3,7 @@ package # hide from PAUSE use strict; use warnings; +use Carp::Clan qw/^DBIx::Class/; our %_pod_inherit_config = ( @@ -21,20 +22,16 @@ sub _has_one { my ($class, $join_type, $rel, $f_class, $cond, $attrs) = @_; unless (ref $cond) { $class->ensure_class_loaded($f_class); - my ($pri, $too_many) = $class->primary_columns; - - $class->throw_exception( - "might_have/has_one can only infer join for a single primary key; ". - "${class} has more" - ) if $too_many; + my $pri = $class->_get_primary_key; + $class->throw_exception( "might_have/has_one needs a primary key to infer a join; ". "${class} has none" ) if !defined $pri && (!defined $cond || !length $cond); my $f_class_loaded = eval { $f_class->columns }; - my ($f_key,$guess); + my ($f_key,$too_many,$guess); if (defined $cond && length $cond) { $f_key = $cond; $guess = "caller specified foreign key '$f_key'"; @@ -42,11 +39,7 @@ sub _has_one { $f_key = $rel; $guess = "using given relationship '$rel' for foreign key"; } else { - ($f_key, $too_many) = $f_class->primary_columns; - $class->throw_exception( - "might_have/has_one can only infer join for a single primary key; ". - "${f_class} has more" - ) if $too_many; + $f_key = $class->_get_primary_key($f_class); $guess = "using primary key of foreign class for foreign key"; } $class->throw_exception( @@ -54,6 +47,7 @@ sub _has_one { ) if $f_class_loaded && !$f_class->has_column($f_key); $cond = { "foreign.${f_key}" => "self.${pri}" }; } + $class->_validate_cond($cond); $class->add_relationship($rel, $f_class, $cond, { accessor => 'single', @@ -63,4 +57,34 @@ sub _has_one { 1; } +sub _get_primary_key { + my ( $class, $target_class ) = @_; + $target_class ||= $class; + my ($pri, $too_many) = $target_class->primary_columns; + $class->throw_exception( + "might_have/has_one can only infer join for a single primary key; ". + "${class} has more" + ) if $too_many; + return $pri; +} + +sub _validate_cond { + my ($class, $cond ) = @_; + + return if $ENV{DBIC_DONT_VALIDATE_RELS}; + return unless 'HASH' eq ref $cond; + foreach my $foreign_id ( keys %$cond ) { + my $self_id = $cond->{$foreign_id}; + + # we can ignore a bad $self_id because add_relationship handles this + # warning + return unless $self_id =~ /^self\.(.*)$/; + my $key = $1; + my $column_info = $class->column_info($key); + if ( $column_info->{is_nullable} ) { + carp(qq'"might_have/has_one" must not be on columns with is_nullable set to true ($class/$key) '); + } + } +} + 1; diff --git a/t/86might_have.t b/t/86might_have.t index 8ebf7b8..a375404 100644 --- a/t/86might_have.t +++ b/t/86might_have.t @@ -2,6 +2,7 @@ use strict; use warnings; use Test::More; +use Test::Warn; use lib qw(t/lib); use DBICTest; @@ -11,8 +12,6 @@ my $queries; $schema->storage->debugcb( sub{ $queries++ } ); my $sdebug = $schema->storage->debug; -plan tests => 2; - my $cd = $schema->resultset("CD")->find(1); $cd->title('test'); @@ -40,4 +39,26 @@ $cd2->update; is($queries, 1, 'liner_notes (might_have) prefetched - do not load liner_notes on update'); +warning_like { + DBICTest::Schema::Bookmark->might_have( + linky => 'DBICTest::Schema::Link', + { "foreign.id" => "self.link" }, + ); +} + qr{"might_have/has_one" must not be on columns with is_nullable set to true}, + 'might_have should warn if the self.id column is nullable'; + +{ + local $ENV{DBIC_DONT_VALIDATE_RELS} = 1; + warning_is { + DBICTest::Schema::Bookmark->might_have( + slinky => 'DBICTest::Schema::Link', + { "foreign.id" => "self.link" }, + ); + } + undef, + 'Setting DBIC_DONT_VALIDATE_RELS suppresses nullable relation warnings'; +} + $schema->storage->debug($sdebug); +done_testing();