Bug #65565 for DBIx-Class-ResultSet-RecursiveUpdate: allowing scalar values for has

Tue Feb 08 11:00:57 2011 LUKAST [...] cpan.org - Ticket created

Subject:

allowing scalar values for has_many updates

The following is NOT a bug! It is a feature which makes cooperation of RecursiveUpdate with HTML::FormHander (and maybe other modules) more easy to set up. At the moment, recursive_update expects all update-parameters to be hashrefs. This is almost always good, except one case: It would be possible to use scalar values as updates for has_many relations, IF the related resultsource has EXACTLY one primary key. In this case, recursive_update can know how to use the single scalar value and build the corresponding hash by itself. Why is this usefull? Because: HTML::FomHander::Model::DBIC (and maybe other modules) return a list of scalar values (selected ids) when multiple items are selected in a select-field. Because this list can not be passed to recursive_update (because it contains scalars, and no hashes), it is not possible to use HTML::FormHander to update has_many relations. I have created a patch which solves the problem by transforming the updates-variable from 'SCALAR_VALUE' to { primary_key_name => 'SCALAR_VALUE' } IF the current relation is a has_many-relation AND the $updates variable stores a scalar AND the related resultsource has exactly one primary column. I also thought about solving the problem in HTML::FormHander, but I thougt that RecursiveUpdate is the better place to fix this issue. My reasons are: 1. IF the related source has exactly one primary column, the primary columns value is enough to identify the coresponding object/row 2. It is easy to fix the problem within RecursiveUpdate and (a little) more complicated in HTML::FormHander 3. The patch does not change RecursiveUpdates behaviour, it just fixes unexpected data instead of throwing an exception. This change of input-data is always safe. 4. Fixing the issue in RecursiveUpdate solves the problem for HTML::FormHander, and any other module which uses RecursiveUpdate, now and in the future. I would be very glad to see this feature in future releases of DBIx::Class::ResultSet::RecursiveUpdate kind regards, lukast

Subject:

recursiveupdate_hasmany_scalarupdates.patch

--- DBIx/Class/ResultSet/RecursiveUpdate.pm 2011-02-08 15:39:12.000000000 +0100 +++ DBIx/Class/ResultSet/RecursiveUpdate.pm.request.patched 2011-02-08 16:14:36.000000000 +0100 @@ -353,6 +353,24 @@ #warn "\tupdating has_many rel '$name' ($rel_col_cnt columns cols)\n"; for my $sub_updates ( @{$updates} ) { + unless ( ref($sub_updates) eq 'HASH') { + # sub_updates is a scalar + if((not ref($sub_updates)) or ref($sub_updates) eq 'SCALAR'){ + my @primary_cols = $related_resultset->result_source->primary_columns; + # related result has EXACTLY one primary column + if(@primary_cols == 1){ + # "hashify" updates + $sub_updates = {$primary_cols[0] => $sub_updates}; + } + else{ + croak "$name has more than one primary column, and sub_updates is not a HASH"; + } + } + else{ + croak "updates for $name has to be a HASH or a single SCALAR"; + } + + } my $sub_object = recursive_update( resultset => $related_resultset, updates => $sub_updates,

Tue Feb 08 12:41:04 2011 abraxxa [...] cpan.org - Correspondence added

Passing a hashref with pk key-value pairs works already and also supports multi-key relationships.

Tue Feb 08 12:41:05 2011 The RT System itself - Status changed from 'new' to 'open'

Wed Feb 09 07:09:31 2011 LUKAST [...] cpan.org - Correspondence added

On Tue Feb 08 12:41:04 2011, ABRAXXA wrote: Show quoted text

> Passing a hashref with pk key-value pairs works already and also > supports multi-key relationships.

Yes, you are right. But that not what i wanted to say. Perhaps my description was unclear. What I mean is, that it could be possible to specify just the related item pks, instead of keyname-value pairs, when updating has_many relations with single keys. I have created a test which demonstrates the issue. The test will fail with RecursiveUpdate 0.21 and pass after applying the patch. I know that this is definitely NOT a bug in recursive_update! I just figuered out that some Modules that i use (for example HTML::FormHander) sometimes produce update-hashes that are not compatible with the current recursive_update. I would prefer to fix this issue in recursive_update, because: - the modification is totaly safe. It only applies to single-keyed has_many relations, where it is easy to transform the passed scalar into a correct keyname->value hash. - the modification does no harm at all. The updates-parameter will almost always be a hashref. In this case, only one additional IF-statement is executed, and the modification-block is entirely skipped. - in my opinion, it is ok to specify only the pk values in a HTML-form if the previously described conditions are met. Recursive_update does not need the keyname. It can figure it out by itself, so why should the user be forced to pass it? - there several modules which produce this conflict. Fixing the issue in recursive_update will solve the problem for all of them. As far as i can see, applying the patch has no negative consequences, and will increase compatiblity with other modules. There is at least one user (me) who loves RecursiveUpdate and would be happy to see this feature in future releases. Thanks for your time! regards, lukast

Subject:

scalar_hasmany.t

# this test will try to update the has_many-relation # 'owned_dvds' in DBSchema::Result::User by specifying the related # ids as scalar values, instead of pk key-value-pairs use lib 't/lib'; use Test::More tests => 9; use Test::Exception; use DBSchema; my $schema = DBSchema::get_test_schema(); isa_ok($schema, 'DBIx::Class::Schema'); my $user_rs = $schema->resultset('User'); isa_ok($user_rs, 'DBIx::Class::ResultSet'); my $user = $user_rs->find(1); isa_ok($user, 'DBIx::Class::Core'); # fetching owned dvds my $dvds = $user->owned_dvds; isa_ok($dvds, 'DBIx::Class::ResultSet'); #counting owned dvds my $dvdcount = $dvds->count; ok($dvdcount, 'counting currently owned dvd'); # creating array containing the owned dvd_ids my @dvd_ids = $dvds->get_column('dvd_id')->all; ok(@dvd_ids, 'extracting dvd ids'); # remove one id from dvds shift @dvd_ids; my $dvdcount_2 = @dvd_ids; is($dvdcount_2, $dvdcount - 1, "counting shortened dvd_ids"); # creating update-hash # a hash like this is produced by # HTML::FormHandler for has_many relations with # single-key relationships my $updates = { id => 1, owned_dvds => \@dvd_ids, }; # running the update lives_ok( sub{$user_rs->recursive_update($updates)}, 'running recursive_update'); my $dvdcount_3 = $user->owned_dvds->count; is($dvdcount_3, $dvdcount_2, 'counting owned dvd after recursive update');

Wed Feb 09 12:54:43 2011 abraxxa [...] cpan.org - Correspondence added

I've reviewed all this again. If you look at http://search.cpan.org/~abraxxa/DBIx-Class-0.08127/lib/DBIx/Class/ResultSet.pm#create you see the DBIC api for creating/linking to related rows aka multicreate. To make the transition from RU to DBIC smoother when RU's features become core I don't want to add non-DBIC APIs to RU. If HFH doesn't integrate with RU the way it should I'd suggest to patch HFH::Model::DBIC.

Wed Feb 09 12:54:43 2011 abraxxa [...] cpan.org - Status changed from 'open' to 'stalled'

Bug #65565 for DBIx-Class-ResultSet-RecursiveUpdate: allowing scalar values for has_many updates