Bug #98679 for CHI: overrideability of store_multi somewhat less than ideal

Sun Sep 07 03:02:16 2014 KENTNL [...] cpan.org - Ticket created

Subject:

overrideability of store_multi somewhat less than ideal

As per: https://metacpan.org/pod/CHI::Driver::Development#store_multi-key_data-options , it seems this method is easy and safe to simply override.

However, unlike all the other methods that are listed to be overridden, this method gets passed the keys *untransformed*, which will mean keys set with it will not be read by subsequent fetchers, unless the implementor manually calls ->transform_key for each of them.

Additionally, implementors will discover they also have to check the validity of the data they get passed themselves, as the test suite expects a croak:

ok 724 - get_multi_hashref throws error when no key passed
not ok 725 - set_multi throws error when no key passed

#   Failed test 'set_multi throws error when no key passed'
#   at /home/kent/perl5/perlbrew/perls/blead/lib/site_perl/5.21.3/CHI/t/Driver.pm line 1738.
#   (in CHI::Driver::LMDB::t::CHIDriverTests->test_missing_params)
# expecting: Regexp ((?^:must specify key))
# found: normal exit
ok 726 - remove_multi throws error when no key passed

Additionally, because that method relies on the method "set" , not store, there's a very substantial amount of effort required to implement this method

https://metacpan.org/source/HAARG/CHI-0.58/lib/CHI/Driver.pm#L381

For example, I'm implementing such a thing where the write happens in a full transaction, I *cant* call set() after spawning a transaction, because that will simply call ->store(), which already creates a transaction, and the result is 2 transactions, and the point of overriding this method is completely negated.

In essence, as it stands, attempting to implement this method to be in any way useful requries implementing code replicating a sizable amount of CHI ( unless I'm missing something ).

Thu Sep 11 05:24:28 2014 swartz [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #98679] overrideability of store_multi somewhat less than ideal
Date:	Thu, 11 Sep 2014 02:24:17 -0700
To:	bug-CHI [...] rt.cpan.org
From:	Jonathan Swartz <swartz [...] pobox.com>

Yes, you’re probably right. Not sure what I was thinking with this. Should we just remove from the documentation as overridable, or do you have a fix you want to propose? Jon On Sep 7, 2014, at 12:02 AM, Kent Fredric via RT <bug-CHI@rt.cpan.org> wrote: Show quoted text

> Sun Sep 07 03:02:16 2014: Request 98679 was acted upon. > Transaction: Ticket created by KENTNL > Queue: CHI > Subject: overrideability of store_multi somewhat less than ideal > Broken in: 0.58 > Severity: (no value) > Owner: Nobody > Requestors: KENTNL@cpan.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=98679 > > > > As per: > https://metacpan.org/pod/CHI::Driver::Development#store_multi-key_data-options > , it seems this method is easy and safe to simply override. > > However, unlike all the other methods that are listed to be overridden, this > method gets passed the keys *untransformed*, which will mean keys set with it > will not be read by subsequent fetchers, unless the implementor manually calls > ->transform_key for each of them. > > Additionally, implementors will discover they also have to check the validity > of the data they get passed themselves, as the test suite expects a croak: > > ok 724 - get_multi_hashref throws error when no key passed > not ok 725 - set_multi throws error when no key passed > > # Failed test 'set_multi throws error when no key passed' > # at /home/kent/perl5/perlbrew/perls/blead/lib/site_perl/5.21.3/CHI/t/Driver.pm > line 1738. > # (in CHI::Driver::LMDB::t::CHIDriverTests->test_missing_params) > # expecting: Regexp ((?^:must specify key)) > # found: normal exit > ok 726 - remove_multi throws error when no key passed > > Additionally, because that method relies on the method "set" , not store, > there's a very substantial amount of effort required to implement this method > > https://metacpan.org/source/HAARG/CHI-0.58/lib/CHI/Driver.pm#L381 > > For example, I'm implementing such a thing where the write happens in a full > transaction, I *cant* call set() after spawning a transaction, because that > will simply call ->store(), which already creates a transaction, and the result > is 2 transactions, and the point of overriding this method is completely > negated. > > In essence, as it stands, attempting to implement this method to be in any way > useful requries implementing code replicating a sizable amount of CHI ( unless > I'm missing something ). >

Thu Sep 11 05:24:28 2014 The RT System itself - Status changed from 'new' to 'open'

Thu Sep 11 08:34:48 2014 KENTNL [...] cpan.org - Correspondence added

If there could be an interface that worked symmetrically with the other one with all the right magic already done, that would be nice.

ie: instead of passing a hash that has to do a lot of magic, perform all the relevant magic "somehow" to emit a hash of values ready for writing to the backing store that'd be nice.

I mean, the goal of documenting this interface was "maybe you have a faster way to write out loads of keys at once", so such a bit of plumbing should do just that.

For instance, if storing a single key requires a full: << opendb, begin transaction, prepare query, execute query, commit transaction, closedb >> step, doing that for dozens of keys would be inefficient, so if the backend knows it has many keys comming, it could be much more efficient.

=~ The idea is still good. Just the implementation is presently a bit "ummm".

Though you probably can't change the existing interface in case anybody is actually using it, because if they've hacked around the present defects somehow, changing it will indeed be very broken.

So ... my suggestion would be

1. Undocument

2. Devise a better way

3. When that better way seems stable and useful, document it.

4. Find everyone who used the old way and maybe nudge them on to the new way.