Bug #130686 for Data-Page: Reorder of operations breaks existing subclass assumptions

Thu Oct 10 10:59:35 2019 ribasushi [...] leporine.io - Ticket created

Subject:

Check for "accessor takes only integers" breaks subclassing

The check introduced here: https://github.com/karenetheridge/Data-Page/commit/70286c7fb2f75dd7239c6adb6d1f602964d26299#diff-2b75d6496b291502e446db7dd34f8426R54 Breaks the following subclassing: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/lib/DBIx/Class/ResultSet/Pager.pm If the checks are there to stay, please consider making them standalone methods that can be noop-ed.

Fri Oct 11 23:15:06 2019 ether [...] cpan.org - Correspondence added

On 2019-10-10 07:59:35, RIBASUSHI wrote: Show quoted text

> The check introduced here: https://github.com/karenetheridge/Data- > Page/commit/70286c7fb2f75dd7239c6adb6d1f602964d26299#diff- > 2b75d6496b291502e446db7dd34f8426R54 > > Breaks the following subclassing: > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > 0.082841/lib/DBIx/Class/ResultSet/Pager.pm > > If the checks are there to stay, please consider making them > standalone methods that can be noop-ed.

I don't see the problem. Is something calling total_entries with @_ = (undef) ?

Fri Oct 11 23:15:06 2019 The RT System itself - Status changed from 'new' to 'open'

Fri Oct 11 23:20:55 2019 ether [...] cpan.org - Correspondence added

On 2019-10-11 20:15:06, ETHER wrote: Show quoted text

> I don't see the problem. Is something calling total_entries with @_ = > (undef) ?

Ok I see the situation now. I had to unwind a few levels and find how DBIx::Class::ResultSet was constructing the pager object. I should be able to work something out.

Sat Oct 12 10:28:31 2019 ribasushi [...] leporine.io - Subject changed from 'Check for "accessor takes only integers" breaks subclassing' to 'Reorder of operations breaks existing subclass assumptions'

Sat Oct 12 10:31:00 2019 ribasushi [...] leporine.io - Correspondence added

On Sat Oct 12 05:20:55 2019, ETHER wrote: Show quoted text

> On 2019-10-11 20:15:06, ETHER wrote: >

> > I don't see the problem. Is something calling total_entries with @_ = > > (undef) ?

> > Ok I see the situation now. I had to unwind a few levels and find how > DBIx::Class::ResultSet was constructing the pager object. > > I should be able to work something out.

_05 does fix the priginally reported problem, but this change https://github.com/karenetheridge/Data-Page/commit/49529b87a1 breaks the entire concept of lazily-evaluated-total-count. If this behavior will stay as-is, I need about 2 weeks to fully prepare/test/release a workaround for DBIC.

Sat Oct 12 12:15:06 2019 ether [...] cpan.org - Correspondence added

On 2019-10-12 07:31:00, RIBASUSHI wrote: Show quoted text

> _05 does fix the priginally reported problem, but this change > https://github.com/karenetheridge/Data-Page/commit/49529b87a1 breaks > the entire concept of lazily-evaluated-total-count. > > If this behavior will stay as-is, I need about 2 weeks to fully > prepare/test/release a workaround for DBIC.

What would you suggest? I would switch to Moo + some XS type checks from Type::Tiny, except that might break something else upstream that depended on the internal accessor methods.

Sun Oct 13 12:11:52 2019 ribasushi [...] leporine.io - Correspondence added

Current subject: "Reorder of operations breaks existing subclass assumptions" Show quoted text

> On 2019-10-12 07:31:00, RIBASUSHI wrote: > > ... this change https://github.com/karenetheridge/Data-Page/commit/49529b87a1 breaks the entire concept of lazily-evaluated-total-count

Show quoted text

> On Sat Oct 12 18:15:06 2019, ETHER wrote: > > ... I would switch to Moo + some XS type checks from Type::Tiny...

I do not know how to respond to your suggestion as it focuses on operation implementation. You changed the *ORDER* of operations. My question is twofold: - Are you planning to revert this? - If not - do I have until ~Oct 27th to make a DBIC-side fix for this, or do I need to scramble something ASAP?

Sun Oct 13 12:39:31 2019 ether [...] cpan.org - Correspondence added

On 2019-10-13 09:11:52, RIBASUSHI wrote: Show quoted text

> Current subject: "Reorder of operations breaks existing subclass > assumptions" >

> > On 2019-10-12 07:31:00, RIBASUSHI wrote: > > > > ... this change https://github.com/karenetheridge/Data- > > Page/commit/49529b87a1 breaks the entire concept of lazily-evaluated- > > total-count

>

> > On Sat Oct 12 18:15:06 2019, ETHER wrote: > > > > ... I would switch to Moo + some XS type checks from Type::Tiny...

> > > I do not know how to respond to your suggestion as it focuses on > operation implementation.

I am asking what *you* would prefer to do. Were any of the changes in 2.04 significant (in a positive way) to you? Show quoted text

> - If not - do I have until ~Oct 27th to make a DBIC-side fix for this, > or do I need to scramble something ASAP?

What is the significance of that date? I had no specific timeline for moving the trial changes to stable.

Sun Oct 13 13:15:26 2019 ribasushi [...] leporine.io - Correspondence added

On Sun Oct 13 18:39:31 2019, ETHER wrote: Show quoted text

> > I am asking what *you* would prefer to do. >

As a consumer I prefer platform-type stuff to not change from under me. On the other hand the use-case of DBIC is *very* unusual, so I do not have a good position to advocate what happens to the future of Data::Page. Show quoted text

> Were any of the changes in 2.04 significant (in a positive way) to > you?

This is a loaded question, I rather not answer it. Show quoted text

> What is the significance of that date? I had no specific timeline for > moving the trial changes to stable.

Tentatively I will have time before that date to look into a workaround on my side, so that future changes in Data::Page won't be a concern. I am simply reporting a bug and inquiring what are my options. I rather not go down the path of near-synchronous dialogue - just let me know what are you planning to do.

Sun Oct 13 14:40:40 2019 ether [...] cpan.org - Correspondence added

On 2019-10-13 10:15:26, RIBASUSHI wrote: Show quoted text

> > Were any of the changes in 2.04 significant (in a positive way) to > > you?

> > This is a loaded question, I rather not answer it.

It was not loaded. I was asking if any of those changes are important to you -- that is, if I rolled back everything, would there be more problems. Show quoted text

> I am simply reporting a bug and inquiring what are my options. I > rather not go down the path of near-synchronous dialogue - just let me > know what are you planning to do.

I will not release changes to the PAUSE index that are known to be breaking an important downstream module. After that, I am trying to find the right course forward, but your terse responses are not helping. At this point I do not understand what the problem is with 49529b87a; I have not been able to reproduce any issues with DBIx::Class::ResultSet::Pager.

Sun Oct 13 17:59:42 2019 ether [...] cpan.org - Status changed from 'open' to 'stalled'

Tue Oct 15 08:54:51 2019 ribasushi [...] leporine.io - Correspondence added

On Sun Oct 13 20:40:40 2019, ETHER wrote: Show quoted text

> On 2019-10-13 10:15:26, RIBASUSHI wrote: >

> > > Were any of the changes in 2.04 significant (in a positive way) to > > > you?

> > > > This is a loaded question, I rather not answer it.

> > It was not loaded. I was asking if any of those changes are important > to you -- that is, if I rolled back everything, would there be more > problems. >

There would be no problems rom my perspective. Though again - the DBIC use case is very obscure, I rather not have the Data::Page changes be held back by it. I just need a few weeks. Show quoted text

> > I am simply reporting a bug and inquiring what are my options. I > > rather not go down the path of near-synchronous dialogue - just let > > me > > know what are you planning to do.

> > I will not release changes to the PAUSE index that are known to be > breaking an important downstream module.

Excellent. Show quoted text

> > After that, I am trying to find the right course forward, but your > terse responses are not helping. At this point I do not understand > what the problem is with 49529b87a; I have not been able to reproduce > any issues with DBIx::Class::ResultSet::Pager.

I made several bad assumptions based on reading https://github.com/karenetheridge/Data-Page/commit/02456a21505a8e7cbc67d34b7911356c2d5a79a6#diff-7d8a9cef418ee48430e98051fff20c8a . Specifically: - The fact that you fixed the bug implied that you understood what the coderef piece is for in the first place - Adding DBIC as an optional test-prereq implied that you ran DBIC's entire test suite against the new version of Data::Page. Now that I know this hasn't happened - here is the gist: - Previously all current_page() checks happened at get-time, which allowed for parts of the stack to be "lazified" - This was (ab)used to skip an expensive `SELECT COUNT(*)...` when the user cares about the first page ( 99% of the cases ) - The change you made forces the sanity check to happen at *object construction time*, which fires the coderef nullifying all lazyfication by virtue of current_page($foo) => last_page() => total_entries() To be even more specific, this block is now impossible to satisfy: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082841/t/67pager.t#L19-28 With all that said, I again must stress that this is probably the only case of lazification in the wild. Thus having DBIC part ways with Data::Page is likely the correct answer. It isup to you how this ticket gets resolved ( revert or won't-fix ), I should have my side squared away in couple weeks.

Tue Oct 15 08:54:51 2019 The RT System itself - Status changed from 'stalled' to 'open'

Tue Oct 15 14:58:42 2019 ether [...] cpan.org - Correspondence added

On 2019-10-15 05:54:51, RIBASUSHI wrote: Show quoted text

> I made several bad assumptions based on reading > https://github.com/karenetheridge/Data- > Page/commit/02456a21505a8e7cbc67d34b7911356c2d5a79a6#diff- > 7d8a9cef418ee48430e98051fff20c8a . Specifically: > - The fact that you fixed the bug implied that you understood what the > coderef piece is for in the first place > - Adding DBIC as an optional test-prereq implied that you ran DBIC's > entire test suite against the new version of Data::Page. > > Now that I know this hasn't happened - here is the gist: > - Previously all current_page() checks happened at get-time, which > allowed for parts of the stack to be "lazified" > - This was (ab)used to skip an expensive `SELECT COUNT(*)...` when the > user cares about the first page ( 99% of the cases ) > - The change you made forces the sanity check to happen at *object > construction time*, which fires the coderef nullifying all > lazyfication by virtue of current_page($foo) => last_page() => > total_entries() > > To be even more specific, this block is now impossible to satisfy: > https://metacpan.org/source/RIBASUSHI/DBIx-Class- > 0.082841/t/67pager.t#L19-28

Sorry about that. The test that I added was clearly incomplete and I didn't re-run DBIx::Class's tests after I made the change. I see the issue now. Show quoted text

> With all that said, I again must stress that this is probably the only > case of lazification in the wild. Thus having DBIC part ways with > Data::Page is likely the correct answer.

Perhaps inlining the calculations into DBIx::Class::ResultSet::Pager might be best (or at least allow for more fine-tuning of hot code paths). That's fine with me as DP's underlying use of accessors is not that great, as you have seen as I have tried to wrestle with improving it :) Show quoted text

> It isup to you how this ticket gets resolved ( revert or won't-fix ), > I should have my side squared away in couple weeks.

Unless there is an emergency I may not be able to release a fix for this in the next week however. It sounds like you aren't in a position to get a fix out ASAP either, so that's good for both of us. I'm happy to coordinate schedules on irc. Let me know what you need.

Tue Nov 05 23:15:20 2019 ribasushi [...] leporine.io - Correspondence added

On Tue Oct 15 20:58:42 2019, ETHER wrote: Show quoted text

> > I'm happy to coordinate schedules on irc. Let me know what you need.

Another trial has shipped no longer depending on Data::Page. Seems good so far. Stable should be another ~2 weeks away. Please keep the "eager" version of Data::Page unreleased until I add a "go-ahead" here.

Tue Jun 16 16:30:49 2020 ribasushi [...] leporine.io - Correspondence added

On Wed Nov 06 05:15:20 2019, RIBASUSHI wrote: Show quoted text

> On Tue Oct 15 20:58:42 2019, ETHER wrote:

> > > > I'm happy to coordinate schedules on irc. Let me know what you need.

> > Another trial has shipped no longer depending on Data::Page. Seems > good so far. Stable should be another ~2 weeks away. > > Please keep the "eager" version of Data::Page unreleased until I add a > "go-ahead" here.

Latest DBIC no longer depends on Data::Page. Do with this dist as you see fit (users who happen to upgrade Data::Page without upgrading their DBIC will see a number of hard-to-explain problems, but such is life on modern CPAN...) Leaving the ticket open as I don't have insights into next steps.

Tue Jun 16 17:25:39 2020 karen [...] froods.org - Correspondence added

Subject:	Re: [rt.cpan.org #130686] Reorder of operations breaks existing subclass assumptions
Date:	Tue, 16 Jun 2020 14:25:09 -0700
To:	bug-Data-Page [...] rt.cpan.org
From:	Karen Etheridge <karen [...] froods.org>

On Tue, Jun 16, 2020 at 1:30 PM Peter Rabbitson via RT < bug-Data-Page@rt.cpan.org> wrote: Show quoted text

> Latest DBIC no longer depends on Data::Page. Do with this dist as you see > fit (users who happen to upgrade Data::Page without upgrading their DBIC > will see a number of hard-to-explain problems, but such is life on modern > CPAN...) > > Leaving the ticket open as I don't have insights into next steps. >

I deleted the trial Data-Page releases from cpan months ago and have no intention of making another release, stable or trial, without thoroughly testing against DBIx::Class (older versions only now, apparently).

Wed Jun 17 11:26:50 2020 ribasushi [...] leporine.io - Requestor RIBASUSHI deleted