Recent changes break backcompat

[mmcclimon]

See also #67. We have a fairly straightforward subclass of DeploymentHandler::Dad. This code works as expected up through 0.002223, and begins breaking in 0.002224 with SQL splitting error.

This broke further in 0.002225 because of the issue in #67, which was supposedly fixed by 0.002229. But it's also broken in 0.002229, with this error:

'DBIx::Class::DeploymentHandler::WithReasonableDefaults' requires the methods 'prepare_downgrade' and 'prepare_upgrade' to be implemented....[snip]

But my class does implement these, by virtue of doing the DeployMethod::SQL::Translator role. For reference, my subclass looks like this:

extends 'DBIx::Class::DeploymentHandler::Dad';

has initial_version => (
  is => 'ro',
  lazy => 1,
  default => sub { $_[0]->database_version },
);

with (
  'DBIx::Class::DeploymentHandler::WithApplicatorDumple' => {
    interface_role       => 'DBIx::Class::DeploymentHandler::HandlesDeploy',
    class_name           => 'DBIx::Class::DeploymentHandler::DeployMethod::SQL::Translator',
    delegate_name        => 'deploy_method',
    attributes_to_assume => [qw(schema schema_version version_source)],
    attributes_to_copy   => [qw(
      ignore_ddl databases script_directory sql_translator_args force_overwrite
    )],
  },

  'DBIx::Class::DeploymentHandler::WithApplicatorDumple' => {
    interface_role       => 'DBIx::Class::DeploymentHandler::HandlesVersionStorage',
    class_name           => 'DBIx::Class::DeploymentHandler::VersionStorage::Standard',
    delegate_name        => 'version_storage',
    attributes_to_assume => ['schema'],
    attributes_to_copy   => [qw(version_source version_class)],
  },

  # This is the role we're subbing out.
  'DBIx::Class::DeploymentHandler::WithApplicatorDumple' => {
    interface_role       => 'DBIx::Class::DeploymentHandler::HandlesVersioning',
    class_name           => 'MyApp::Schema::DeploymentHandler::Monotonic::Next',
    delegate_name        => 'version_handler',
    attributes_to_assume => [qw( initial_version schema_version to_version )],
    attributes_to_copy   => [qw( last_numeric next_version )],
  },
);

with 'DBIx::Class::DeploymentHandler::WithReasonableDefaults';
...

You'll note that this is basically exactly the same code as was in DeploymentHandler.pm prior to the conversion to Moo. I tried changing the with there to use MooX::Role::Parameterized::With (which, gross, but fine if it works), and I get a whole other pile of Moose garbage:

Found unknown argument 'index' in the has declaration for 'ignore_ddl' in class DBIx::Class::DeploymentHandler::WithApplicatorDumple at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose/Meta/Attribute.pm line 87.
        Moose::Meta::Attribute::new("Moose::Meta::Attribute", "ignore_ddl", "isa", Type::Tiny=HASH(0x7fe3f403de00), "index", 0, "reader", "ignore_ddl", ...) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose/Meta/Attribute.pm line 105
        Moose::Meta::Attribute::interpolate_class_and_new("Moose::Meta::Attribute", "ignore_ddl", "definition_context", HASH(0x7fe3f78f1a10), "reader", "ignore_ddl", "is", "ro", ...) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose/Meta/Class.pm line 724
        Moose::Meta::Class::_process_new_attribute(Moose::Meta::Class=HASH(0x7fe3f7920d50), "ignore_ddl", "definition_context", HASH(0x7fe3f78f1a10), "reader", "ignore_ddl", "is", "ro", ...) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose/Meta/Class.pm line 717
        Moose::Meta::Class::_process_attribute(Moose::Meta::Class=HASH(0x7fe3f7920d50), "ignore_ddl", "definition_context", HASH(0x7fe3f78f1a10), "reader", "ignore_ddl", "is", "ro", ...) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose/Meta/Class.pm line 581
        Moose::Meta::Class::add_attribute(Moose::Meta::Class=HASH(0x7fe3f7920d50), "ignore_ddl", "definition_context", HASH(0x7fe3f78f1a10), "reader", "ignore_ddl", "is", "ro", ...) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose.pm line 74
        Moose::has(Moose::Meta::Class=HASH(0x7fe3f7920d50), "ignore_ddl", "reader", "ignore_ddl", "is", "ro", "default", undef, ...) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/darwin-2level/Moose/Exporter.pm line 419
        Moose::has(MooX::Role::Parameterized::Proxy=HASH(0x7fe3f6eac940), "ignore_ddl", "reader", "ignore_ddl", "is", "ro", "default", undef, ...) called at /Users/michael/code/hub/DBIx-Class-DeploymentHandler/DBIx-Class-DeploymentHandler-0.002229/lib/DBIx/Class/DeploymentHandler/WithApplicatorDumple.pm line 31
        DBIx::Class::DeploymentHandler::WithApplicatorDumple::__ANON__(HASH(0x7fe3f7bc7630), MooX::Role::Parameterized::Proxy=HASH(0x7fe3f6eac940)) called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/MooX/Role/Parameterized.pm line 48
        MooX::Role::Parameterized::apply("DBIx::Class::DeploymentHandler::WithApplicatorDumple", HASH(0x7fe3f7bc7630), "target", "Topicbox::Schema::DeploymentHandler") called at /Users/michael/.plenv/versions/5.26.1/lib/perl5/site_perl/5.26.1/MooX/Role/Parameterized/With.pm line 21
        MooX::Role::Parameterized::With::import("MooX::Role::Parameterized::With", "DBIx::Class::DeploymentHandler::WithApplicatorDumple", HASH(0x7fe3f7bc7630), "DBIx::Class::DeploymentHandler::WithApplicatorDumple", HASH(0x7fe3f7bc63e0), "DBIx::Class::DeploymentHandler::WithApplicatorDumple", HASH(0x7fe3f7bc8350))

At which point I sort of gave up and said "this is stupid, I'm filing an issue." I suspect this breakage ultimately comes from the switch from MooseX::Role::Parameterized to MooX::Role::Parameterized (which is marked as experimental), but have not fully confirmed that yet.

I have comaint on this module, and I have mostly not been paying attention because I've been busy with other things, but I had occasion to happen across it tonight when a coworker said "hey why is this broken?" And I have nothing to say other than "beats me!"

It's really frustrating to have this module, which is pretty heavily depended on, break backcompat, especially with no mention of it in the changes file. This breakage seems largely due to the switch from Moose to Moo, which was done for seemingly no reason. (And not to mention, we've dropped support for perl 5.8 while doing so.) Maybe there was a good reason, and I don't happen to remember seeing it come across my email; if so, I apologize for being so harsh.

In the meantime, I think I'm going to have to pin my deps at 0.002223.

[mohawk2]

In fact, as I'm sure you saw, DeploymentHandler.pm is very nearly identical before and after, apart from two major things:

• use Moo
• the use MooX::Role::Parameterized::With etc

You'll notice I took the liberty of updating your issue (by the way, thanks for the report) so the line-endings are like the original. That then looked very clear to me your consuming class was using Moose.

I've made a branch moo-vs-moose. The added test file basically reproduces the current contents of DeploymentHandler.pm, and works. On changing the use Moo to use Moose, it blows up as you have seen. Evidently the MooX module does not work when used directly by a Moose class.

Therefore, could you please try changing your own use Moose to use Moo and seeing what happens?

[mohawk2]

I thought more about this; the underlying situation here is the DH class using DH internals, that aren't documented. The reason I switched the system to Moo is it starts up, and quite possible runs, much faster than Moose.

Pondering the specifics here, I initially thought you could just extends the DH class, and only override the bits you want to, but then I realised you want to use the Moose parameterised role thingy, which is Moose-specific. I have actually asked the MooX module-maker whether they want support in making it work on 5.8, and we'll see on that one. An option if you don't want your code to be faster using Moo would be to also locally reproduce the old Moose parameterised role.

More generally, I think the need for the parameterised role, which is a complex way to make the DH be "is a" the incorporated parts, instead of "has a", similar to just using handles without the extra stuff. Perhaps @frioux can help me understand why this wasn't simply done with handles? Is it the lazy-build bit?

Finally, if you're having specific SQL-splitting issues, then it would be actually helpful if you could provide test-cases.

[frioux]

It was implemented this way so that users could create simple subclasses to modify behavior, instead of being required to create a subclass of all of the pieces.

[mmcclimon]

If you're having specific SQL-splitting issues, then it would be actually helpful if you could provide test-cases.

I'll see what I can do; I didn't dig too far into this particular rabbit-hole, since the other breakage was more severe.

Therefore, could you please try changing your own use Moose to use Moo and seeing what happens?

I have done so, and after changing a bunch of other things I needed to change to get it to work, it works with Moo. (For example: all of my isas, the fact that I had multiple packages in one file, which Moo couldn't deal with, etc.)

But that's not really how Moo/Moose interop is supposed to work: in all of my other code, I can use Moose to extend a Moo class and it Just Works, and with MooX::Role::Parameterized, it doesn't, even if I change my standard with to MooX::Role::Parameterized::With (which means it doesn't look like every other class in my tree). Furthermore, MooseX::Role::Parameterized is extremely stable software, and maintained by the same people who maintain Moose. MooX::Role::Parameterized is explicitly marked experimental.

Perhaps @frioux can help me understand why this wasn't simply done with handles? Is it the lazy-build bit?

It was implemented this way so that users could create simple subclasses to modify behavior, instead of being required to create a subclass of all of the pieces.

And that was explicitly what I had done, which worked in an old version and is broken in newer versions. You say that the DH internals aren't documented, which is true, in a way, but the docs explicitly say (in the section "Where is all the Doc?!"):

For the full story you should realise that "DBIx::Class::DeploymentHandler" extends DBIx::Class::DeploymentHandler::Dad, so that's probably the first place to look when you are trying to figure out how everything works.

Next would be to look at all the pieces that fill in the blanks that DBIx::Class::DeploymentHandler::Dad expects to be filled. They would be DBIx::Class::DeploymentHandler::DeployMethod::SQL::Translator, DBIx::Class::DeploymentHandler::VersionHandler::Monotonic, DBIx::Class::DeploymentHandler::VersionStorage::Standard, and DBIx::Class::DeploymentHandler::WithReasonableDefaults.

So the documentation explicitly says to "Look at DBIC::DH::Dad, then fill in all the bits it does." Which is exactly what I'd done, and (at the risk of beating a dead horse) used to work, and no longer works.

The reason I switched the system to Moo is it starts up, and quite possible runs, much faster than Moose.

I understand that Moose startup times are slower than Moo. The difference in runtime performance is negligible, in my experience, but that's not really my issue here.

I knew about the startup costs of Moose vs. Moo when I decided to use DBIC::DH in the first place, several years ago. I chose it because it was easy to extend (partly because it used Moose and parameterized roles), and because it came highly recommended, in no small part because it was very stable software. So: I get that switching to Moo leads to faster startup, but if it comes at the cost of breaking backward compatibilty, then IMO that's totally not worth it.

And to be honest, it seems unlikely anyone in 2019 will start using DH just because it starts up a bit faster. Partially that's because very few people are writing new code in Perl 5. But also, anybody that needs a schema management system like DH probably already has a reasonably complex software solution, and such a system is likely to use Moose somewhere already. (And certainly, is using DBIC already, which is itself not a quick and lean piece of software.)

Looking at the changelog, it seems like everything from 0.002225 on was all about converting to Moo and dealing with subsequent breakage of formerly functioning code. If I had sole maint of this package, I'd probably revert all of that. I don't, though, and it's possible I could be convinced that there is some good reason for switching to Moo for which it is worth breaking backcompat. But "faster startup and maybe runtime" is not, to me, that argument.

-Michael