"regression" introduced in DBIC r5298.. Or we're Doing It Wrong..

[Toby Corkindale-2]

Hi guys,
I've encountered a regression in DBIC, introduced in revision 5298.
It's more than likely that the way things were being done in the code
here were not correct and *shouldn't* have worked previously, but I
can't seem to work it out myself.

There's a single changed line which triggers the problem:
--------------------------------------------------------
diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
@@ -1228,7 +1229,7 @@ sub register_extra_source {
  sub _register_source {
    my ($self, $moniker, $source, $params) = @_;

-  %$source = %{ $source->new( { %$source, source_name => $moniker }) };
+  $source = $source->new({ %$source, source_name => $moniker });

--------------------------------------------------------

I've created a small unit test that demonstrates the issue, here:
http://dryft.net/dbic/tobyc-dbic-problem.tar.gz

The failure manifests itself as missing relationships on the result
sources. They're clearly setup, and it all works fine until the above
line is changed in DBIC/Schema.pm.

Any help would be most appreciated.

Cheers,
Toby

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Toby Corkindale-2]

Toby Corkindale wrote:

> Hi guys,
> I've encountered a regression in DBIC, introduced in revision 5298.
> It's more than likely that the way things were being done in the code
> here were not correct and *shouldn't* have worked previously, but I
> can't seem to work it out myself.
>
> There's a single changed line which triggers the problem:
> --------------------------------------------------------
> diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
> @@ -1228,7 +1229,7 @@ sub register_extra_source {
>  sub _register_source {
>    my ($self, $moniker, $source, $params) = @_;
>
> -  %$source = %{ $source->new( { %$source, source_name => $moniker }) };
> +  $source = $source->new({ %$source, source_name => $moniker });
>
> --------------------------------------------------------
>
> I've created a small unit test that demonstrates the issue, here:
> http://dryft.net/dbic/tobyc-dbic-problem.tar.gz
>
> The failure manifests itself as missing relationships on the result
> sources. They're clearly setup, and it all works fine until the above
> line is changed in DBIC/Schema.pm.

Following up on this..
Experimentation shows that if I setup the relationships *before* calling
register_class() then the relationships stick around.
Setting them up after register_class() means they disappear.

Is that meant to be the way things work?

Currently the relationships are added by calling:
$schema->class('Foo')->has_many(...);

Perhaps this isn't the Right Way to add them?
Is there a better way?

Note that neither of these work:
$schema->source('Foo')->has_many(..);
$schema->resultset('Foo')->result_source->has_many(..);
-- Both fail with "Can't locate object method "has_many" in.."

After some discussion on IRC with Caelum and Ribasushi (thanks!) I ended
up with the following in the test case I posted:

     for my $source ($schema_namespace->sources) {
         my $class = $schema_namespace->class($source);
         $class->_setup_relationships($schema_namespace);
         # *** This next call added to fix bug ***
         $schema_namespace->register_extra_source(
             $source => $class->new->result_source_instance
         );
     }


This seems to fix the bug in the test case!
Do you think I'm doing this the right way now?

Cheers,
Toby

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Matt S Trout]

On Wed, Jun 03, 2009 at 06:14:13PM +1000, Toby Corkindale wrote:

> Hi guys,
> I've encountered a regression in DBIC, introduced in revision 5298.
> It's more than likely that the way things were being done in the code
> here were not correct and *shouldn't* have worked previously, but I
> can't seem to work it out myself.
>
> There's a single changed line which triggers the problem:
> --------------------------------------------------------
> diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
> @@ -1228,7 +1229,7 @@ sub register_extra_source {
>  sub _register_source {
>    my ($self, $moniker, $source, $params) = @_;
>
> -  %$source = %{ $source->new( { %$source, source_name => $moniker }) };
> +  $source = $source->new({ %$source, source_name => $moniker });

Yeah, that's a bugfix.

Basically, you can't add relationships after the fact - if you can't set up
your relationships before the source is registered with the schema then
you'll need to re-register it afterwards.

Slightly annoying side effect of the prototype inheritance style DBIC uses
for schema and sources - I'm aware it's a tad annoying but things break
subtly without it.

--
        Matt S Trout         Catalyst and DBIx::Class consultancy with a clue
     Technical Director      and a commit bit: http://shadowcat.co.uk/catalyst/
 Shadowcat Systems Limited
  mst (@) shadowcat.co.uk        http://shadowcat.co.uk/blog/matt-s-trout/

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Toby Corkindale-2]

Matt S Trout wrote:

> On Wed, Jun 03, 2009 at 06:14:13PM +1000, Toby Corkindale wrote:
>> Hi guys,
>> I've encountered a regression in DBIC, introduced in revision 5298.
>> It's more than likely that the way things were being done in the code
>> here were not correct and *shouldn't* have worked previously, but I
>> can't seem to work it out myself.
>>
>> There's a single changed line which triggers the problem:
>> --------------------------------------------------------
>> diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
>> @@ -1228,7 +1229,7 @@ sub register_extra_source {
>>  sub _register_source {
>>    my ($self, $moniker, $source, $params) = @_;
>>
>> -  %$source = %{ $source->new( { %$source, source_name => $moniker }) };
>> +  $source = $source->new({ %$source, source_name => $moniker });
>
> Yeah, that's a bugfix.
>
> Basically, you can't add relationships after the fact - if you can't set up
> your relationships before the source is registered with the schema then
> you'll need to re-register it afterwards.
>
> Slightly annoying side effect of the prototype inheritance style DBIC uses
> for schema and sources - I'm aware it's a tad annoying but things break
> subtly without it.

Is the correct way to re-register it this?

My::Schema->register_extra_source(
     Users => My::Schema->class('Users')->new->result_source_instance
);

(That seems to work at least.. trying to do register_class/source()
again causes an error about it already existing)

thanks,
Toby

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Matt S Trout]

On Tue, Jun 09, 2009 at 11:29:53AM +1000, Toby Corkindale wrote:

> Matt S Trout wrote:
> >On Wed, Jun 03, 2009 at 06:14:13PM +1000, Toby Corkindale wrote:
> >>Hi guys,
> >>I've encountered a regression in DBIC, introduced in revision 5298.
> >>It's more than likely that the way things were being done in the code
> >>here were not correct and *shouldn't* have worked previously, but I
> >>can't seem to work it out myself.
> >>
> >>There's a single changed line which triggers the problem:
> >>--------------------------------------------------------
> >>diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
> >>@@ -1228,7 +1229,7 @@ sub register_extra_source {
> >> sub _register_source {
> >>   my ($self, $moniker, $source, $params) = @_;
> >>
> >>-  %$source = %{ $source->new( { %$source, source_name => $moniker }) };
> >>+  $source = $source->new({ %$source, source_name => $moniker });
> >
> >Yeah, that's a bugfix.
> >
> >Basically, you can't add relationships after the fact - if you can't set up
> >your relationships before the source is registered with the schema then
> >you'll need to re-register it afterwards.
> >
> >Slightly annoying side effect of the prototype inheritance style DBIC uses
> >for schema and sources - I'm aware it's a tad annoying but things break
> >subtly without it.
>
> Is the correct way to re-register it this?
>
> My::Schema->register_extra_source(
>     Users => My::Schema->class('Users')->new->result_source_instance
> );
>
> (That seems to work at least.. trying to do register_class/source()
> again causes an error about it already existing)

ITYM warning, and since you're doing something weird you probably -do-
want to overwrite.

Possibly what we need is a patch to allow a "yes, I meant to do this"
option to squelch the warning ...

--
        Matt S Trout         Catalyst and DBIx::Class consultancy with a clue
     Technical Director      and a commit bit: http://shadowcat.co.uk/catalyst/
 Shadowcat Systems Limited
  mst (@) shadowcat.co.uk        http://shadowcat.co.uk/blog/matt-s-trout/

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...

[Toby Corkindale-2]

Matt S Trout wrote:

> On Tue, Jun 09, 2009 at 11:29:53AM +1000, Toby Corkindale wrote:
>> Matt S Trout wrote:
>>> Basically, you can't add relationships after the fact - if you can't set up
>>> your relationships before the source is registered with the schema then
>>> you'll need to re-register it afterwards.
>>>
>>> Slightly annoying side effect of the prototype inheritance style DBIC uses
>>> for schema and sources - I'm aware it's a tad annoying but things break
>>> subtly without it.
>> Is the correct way to re-register it this?
>>
>> My::Schema->register_extra_source(
>>     Users => My::Schema->class('Users')->new->result_source_instance
>> );
>>
>> (That seems to work at least.. trying to do register_class/source()
>> again causes an error about it already existing)
>
> ITYM warning, and since you're doing something weird you probably -do-
> want to overwrite.
>
> Possibly what we need is a patch to allow a "yes, I meant to do this"
> option to squelch the warning ...

I wondered about a patch that added a function to DBIx/Class/Schema.pm
to alter the source, like so?
Although Peter suggested this sort of thing probably wasn't to be
encouraged; I'll put it out there as an option though..

---
  lib/DBIx/Class/Schema.pm |   22 ++++++++++++++++++++++
  1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
index 4b945cc..17b2a8e 100644
--- a/lib/DBIx/Class/Schema.pm
+++ b/lib/DBIx/Class/Schema.pm
@@ -1295,6 +1295,28 @@ sub _unregister_source {
      }
  }

+=head2 alter_source
+
+=over 4
+
+=item Arguments: $moniker
+
+=back
+
+As L</register_source> but should be used if the result class already
+has a source and you want to replace it with an altered one. (Eg. If
you have
+adjusted its metadata (incl. relationships) after registering it the
first time.)
+
+=cut
+
+sub alter_source {
+    my ($self, $moniker) = @_;
+    my $class = $self->class($moniker);
+    $schema->_unregister_source($moniker);
+    $schema->register_source(
+        $moniker => $class->new->result_source_instance
+    );
+}

  =head2 compose_connection (DEPRECATED)

--
1.6.3.2

_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive: http://www.grokbase.com/group/dbix-class@...