Not working as documented since 0.40

[bokutin]

Hello.

I'll make the question simple.

It seems that RU doesn't work unless schema is rel_name == pk naming in case of many_to_many from 0.40.

The output of the test is below.
https://github.com/bokutin/recursiveupdate-test/blob/master/out.txt
The schema is below.
https://github.com/bokutin/recursiveupdate-test/tree/master/t/lib/M2MTest

Is my usage wrong?
I think I am using it as DESIGN CHOICES - Treatment of many-to-many pseudo relations.

It would be greatly appreciated if someone could answer.

[abraxxa]

Hi Tomohiro,
I'm the only maintainer left, didn't know anybody but us uses this module any more.
Thanks for your feedback, I guess this is related to #19?
I won't be able to look into this this week. Please provide a failing test against the DBICTest schema if possible as the long-term goal is to have all tests converted to it to ease integrating the feature into core DBIC.
Thanks!

[bokutin]

Hi Alexander,

I guess this is related to #19?

Yes, but I thought it would be a long sentence to get a reply, so I made it easy.

Please provide a failing test against the DBICTest schema if possible as the long-term goal is to have all tests converted to it to ease integrating the feature into core DBIC.

I understand.

I first looked at t/lib/DBICTest/Schema and t/lib/DBSchema, but I couldn't find anything suitable to reproduce the bug, so I created it.

To reproduce the bug as a many_to_many link table

package DBSchema::Result::Dvdtag;
__PACKAGE__->belongs_to("dvd", "DBSchema::Result::Dvd", { dvd_id => "dvd" });
__PACKAGE__->belongs_to("tag", "DBSchema::Result::Tag", { id => "tag" });         # rel_name == pk

not

package DBSchema::Result::Dvdtag;
__PACKAGE__->belongs_to("dvd", "DBSchema::Result::Dvd", { dvd_id => "dvd_id" });
__PACKAGE__->belongs_to("tag", "DBSchema::Result::Tag", { id => "tag_id" });      # rel_name != pk

is needed.

Thank you for your reply despite being busy.

I will check it a bit more.

[bokutin]

I was able to reproduce the bug using DBICTest on master.
bokutin@aaeb4e7
I will go a little further.

[bokutin]

Hi Alexander,

I have further investigated.
I think you are busy, but I would appreciate it if you could read it.

---

master...bokutin:indent_warn
This is a trick that will make debugging easier.

master...bokutin:fix_side_effect
If you return in ” while ( my ( $f_key, $col ) = each %{$cond} )", it will start halfway the next time you call.
This is a basic bug so it's better to merge it.

5487ac1 [1]
This fixes a loss of compatibility with 0.34.
If belongs_to relname != colname, it does not work.

87c6d36 [2]
The above fix still fails in the case of IntrospectableM2M.
Here's the fix.

09ea2e0
Reduced code duplication.
Please forgive my selfishness.
You don't have to merge if you don't want to.

bokutin/dbix-class-resultset-recursiveupdate@reduce_dup_code...bokutin:feature_accessors
I also checked #6.
I confirmed that it passes the test with only a few modifications.

---

I'm hoping that the [1] and [2] fixes will be merged.
I want to update RU in my production environment that is using 0.34.

I also tried #1.
I wrote the code corresponding to $source->unique_constraint_columns as well as pk.
It works well in my environment.
If RU supports unique constraints, I would like to send a pull request.

Hope I can continue to use the RU without any known bugs.

Thanks,

[abraxxa]

Hi Tomohiro,
I finally found some time to look into your great additions and fixes!

master...bokutin:indent_warn
This would be great if the output of Dumper is also indented. Examples are matching row found for:, updating related row: and current data:. Maybe you have another trick for that as well.

master...bokutin:fix_side_effect
That's indeed an important one, can you please send me a pull-request for it? I'd only suggest to rename the test file to plural no_side_effects.t. Thanks!

Are 5487ac1, 87c6d36 and 09ea2e0 cumulative? I'm surprised that this doesn't work for you as in our schema we always prefix foreign key columsn with fk_ and their relationships with rel_, so they aren't identical and everything works. It could be that we don't update belong_to relationships with RU though...

09ea2e0 is awesome, I was too lazy so far although I knew that there was duplicate code, thanks!!!
I wasn't able to wrap my head around the code so far, please add comments especially for related_rows, then I'll review it again.
I'll also suggest to do that in a pull-request so we can comment on it until it's ready to merge.

bokutin/dbix-class-resultset-recursiveupdate@reduce_dup_code...bokutin:feature_accessors
This one would break my usage of RU as it implements what was requested in #6. My usage is IPv4/IPv6 networks that are transformed into two hex values, one for the address and another one for the subnet mask which are stored in RAW columns in Oracle so you can do bit logic on them.
The DBIC model as Moose attributes that take objects and fill the two underlying raw database columns with the hex values which are then used to lookup the related row as those are part of the primary key.
To sum up, the key passed corresponds to the Moose accessor method that fills the real raw database columns.
Tests for this are very welcome but the changes in line RecursiveUpdate.pm would break it.

[abraxxa]

Code based on master...bokutin:indent_warn is committed as dcc3f88.

[abraxxa]

master...bokutin:fix_side_effect is commited as 06fbd56.

[bokutin]

Hi Alexander,
Thank you for reviewing and replying even though you are busy.

Are 5487ac1, 87c6d36 and 09ea2e0 cumulative? I'm surprised that this doesn't work for you as in our schema we always prefix foreign key columsn with fk_ and their relationships with rel_, so they aren't identical and everything works. It could be that we don't update belong_to relationships with RU though...

I passed the tests on 5487ac1 and 87c6d36 first, and then reduced the code on 09ea2e0 to not break the tests.

The commit comes with a test, so try running it. It should fail.

Perhaps the fk_id from belongs_to wasn't collected before find, so if fk_id is included in the unique index then update_or_insert will fail to update instead of insert and will fail.

09ea2e0 is awesome, I was too lazy so far although I knew that there was duplicate code, thanks!!!
I wasn't able to wrap my head around the code so far, please add comments especially for related_rows, then I'll review it again.
I'll also suggest to do that in a pull-request so we can comment on it until it's ready to merge.

I'll try.

bokutin/dbix-class-resultset-recursiveupdate@reduce_dup_code...bokutin:feature_accessors
This one would break my usage of RU as it implements what was requested in #6. My usage is IPv4/IPv6 networks that are transformed into two hex values, one for the address and another one for the subnet mask which are stored in RAW columns in Oracle so you can do bit logic on them.
The DBIC model as Moose attributes that take objects and fill the two underlying raw database columns with the hex values which are then used to lookup the related row as those are part of the primary key.
To sum up, the key passed corresponds to the Moose accessor method that fills the real raw database columns.
Tests for this are very welcome but the changes in line RecursiveUpdate.pm would break it.

I understand.
For behaviors that are not in the documentation, I think it's good to keep the status quo.
I am doing the following, so there is no impact.
https://metacpan.org/pod/DBIx::Class::ResultSet::RecursiveUpdate#Additional-data-in-the-updates-hashref
It may be better to clarify get_column, accessor_name and ->$colname in the future.
My preference is to mimic DBIC's create, set_columns and set_inflated_columns etc.

---

I'd like to write a failing test case for 0.42 and make a pull request, noting the related_rows comment.

I'm not interested in my code being merged as is. I hope it works as documented.

Thanks!

[bokutin]

My preference is to mimic DBIC's create, set_columns and set_inflated_columns etc.

Oh, this is my preference.

dbix-class-resultset-recursiveupdate/lib/DBIx/Class/ResultSet/RecursiveUpdate.pm

Line 6 in ebfb027

# ABSTRACT: like update_or_create - but recursive

[bokutin]

I'm also likely to get a little busy.

First of all, I wrote a test to pass for 0.42. https://github.com/gshank/dbix-class-resultset-recursiveupdate/compare/master...bokutin:topic/0.42-more-test?expand=1

RecursiveUpdate.pm for this is the same as last time, but I'm going to make it simpler and add more comments.

[abraxxa]

Did you work on a consecutive or changed pull-request?

[abraxxa]

Hi!
Did you further work on this or should I close the issue?