Producer args not used when generating SQL diffs

[fgabolde]

I'm adding a boolean column to an existing table and creating a different table with a boolean column:

    "flagged_for_deletion",
    {   data_type => "boolean",
        is_nullable => 0,
        default_value => 0,
    },

and generating the DDL via DBIx::Class:

$schema->create_ddl_dir( ['MySQL'], undef, './sql/ddl', $opt->pre_version,
                         { producer_args => { mysql_version => 5 } });

The full schema is correctly generated, both tables have "flagged_for_deletion boolean NOT NULL DEFAULT '0'" which is exactly what I expected. The diff however:

CREATE TABLE `created` (
  -- snip
  `flagged_for_deletion` enum('0','1') NOT NULL DEFAULT '0',
  -- snip
) ENGINE=InnoDB DEFAULT CHARACTER SET utf8;

ALTER TABLE altered -- snip
                    ADD COLUMN flagged_for_deletion boolean NOT NULL DEFAULT '0',
                    -- snip

The CREATE TABLE statement has the backtick quoting (that the full schema doesn't have) and uses MySQL 3-style boolean emulation with enums. From all the way over here it looks like it's been generated by a different producer with different options, so it might be a DBIC issue instead?

[ribasushi]

From all the way over here it looks like it's been generated by a different producer

It is the same producer, but it is geared to do things differently based on an option.

Please investigate how/why this is not getting populated.

Alternatively please write a test (even if as a CLI oneliner) that demonstrates the problem.

Thanks!

[fgabolde]

@ribasushi I narrowed it down to SQL::Translator::Diff, when building an SQL::Translator object in produce_diff_sql.

It should be:

      my $translator = SQL::Translator->new(
        producer_type => $self->output_db,
        add_drop_table => 0,
        no_comments => 1,
        producer_args => $self->producer_args
      );

otherwise the SQL::Translator constructor gets mysql_version directly as an argument. Obviously it wouldn't know what to do with a MySQL-specific argument, so it just gets thrown away.

The MySQL producer's produce method is then called a few lines later with that translator instance.

With this fix I can get my boolean column. I still have backticks everywhere, which is aesthetically annoying but I can live with that. :)

[ilmari]

The problem with changing it to producer_args => $self->producer_args is that that would break existing uses of that to pass arguments to the translator, like this test. The attribute really should have been called translator_args.

@fgabolde: Can you try changing the create_ddl_dir argument to { producer_args => { producer_args => { mysql_version => 5 } } }?

[fgabolde]

@ilmari I can, but then the regular producer (generating the full schema) loses mysql_version.

[ilmari]

@fgabolde D'oh, of course. I think the most backwards-compatible fix would be to change create_ddl_dir to call schema_diff with { producer_args => $sqltargs }. Can you test that locally?

[fgabolde]

@ilmari Yeah, that works. It doesn't break any tests in DBIC, which is unsettling, but I get both the full schema and the diff with the right mysql_version setting.

It changes some other things as well because those SQLT args seem tailored for the full DDL and not so much for the diff, so I get no quotes (which I like) and DROP TABLE IF EXISTS statements (which I don't really care about, I dunno if someone might).

[racke]

Honestly, I can't think of a single reason why mysql_version => 5 shouldn't be the default. I can't even remember when I last used a MySQL database 4. Now we produce crappy enum columns without any warning to the hapless user.

[melmothx]

Actually, with version 4 it gives boolean as well. So by default SQLT is producing boolean datatype for mysql 3...

[rabbiveesh]

hey all, it's a bit late (only 6 years), but I just pushed a patch changing the name of the producer_args attribute on SQLT::Diff to sqlt_args, which is better-ly self documenting. Essentially, this is the same confusion as #138 . You need to pass producer_args as an arg to producer_args. With the new sqlt_args name, it makes a lot more sense.