Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated migrations confused by columnDefinition that contains GENERATED ALWAYS AS #6641

Open
gravitiq-cm opened this issue Dec 1, 2024 · 2 comments

Comments

@gravitiq-cm
Copy link

Bug Report

Q A
Version dbal: 3.9.3, orm: 2.20.0

Summary

When using make:migration to generate migrations, fields which use columnDefinitions end up generating the same up() statement every time (even after the migration has been run).

Current behavior

Field attribute is something like this:

#[ORM\Column(type: Types::STRING, length: 10, insertable: false, updatable: false, 
columnDefinition: 'VARCHAR(10) GENERATED ALWAYS AS (bar) STORED')]

Generated migration looks something like this:

    public function up(Schema $schema): void
    {
        $this->addSql('ALTER TABLE my_table CHANGE foo foo VARCHAR(10) GENERATED ALWAYS AS (bar) STORED');
    }

    public function down(Schema $schema): void
    {
        $this->addSql('ALTER TABLE my_table CHANGE foo foo VARCHAR(10) DEFAULT NULL');
    }

When you run the migration everything works. But when you generate the next migration it tries to make the same changes again.

Expected behavior

When you generate the next migration, Doctrine should recognise that it is trying to make a change that has already been done and not add the GENERATED ALWAYS... code to the next migration

How to reproduce

Add a columnDefinition to a field, generate migration, run it then generate the next migration.

I tried looking in the Comparator but struggled to find the detail. In diffColumn() I can see that $column2['columnDefinition'] is blank compared to $column1['columnDefinition'] which contains the GENERATED ALWAYS code, but I don't fully understand where $column2 comes from or why Connection::getSchema() would return a blank columnDefinition...

@gravitiq-cm
Copy link
Author

gravitiq-cm commented Dec 1, 2024

digging into this a bit more, I can now see that what's stored in information_schema.GENERATION_EXPRESSION is not always identical to what's run in the migration, so even if we pulled that field in MySQLSchemaManager it would be difficult to know if it has changed or not...
However,
(a) it is sometimes identical (but we do not currently detect those cases) and
(b) it seems that a little work could detect more cases... e.g. if your columnDefinition is 'VARCHAR(10) GENERATED ALWAYS AS (bar) STORED' and the informationSchema shows:

Field Value
EXTRA STORED GENERATED
COLUMN_TYPE varchar(10)
GENERATION_EXPRESSION (`bar`)

Then we can work out that it's identical and there is no remaining "unknown" part to the columnDefinition, so there's no diff.

The other possibility might be to have a (new) command to compare the current Doctrine schema with the current Database (possibly only for fields that have columnDefinitions) and make suggestions of how the columnDefinitions in PHP-space could be changed to match what's currently in the database.

The idea would be that this command could be run after the migration - the coder could then modify the detail of their columnDefinition to make it easier for Doctrine to diff it. e.g. here we could change the columnDefinition to 'VARCHAR(10) GENERATED ALWAYS AS (`bar`) STORED' (with backticks around bar so it matches what MySQL stores).

At the moment there's no way even to mark a columnDefinition as "do not check" so we get (a lot of) false positives... we have many dozens of generated columns in the database. At the moment they have to be deleted manually every time we generate a migration...

I'm not sure how best to help here... I'd like to help tidy this up, but this seems like a very technical part of the code, and I'm not entirely sure how it all fits together, or how it plays nicely with other platforms' code. Is it the right approach to add more detail in MySQLSchemaManager::selectTableColumns() and then modify _getPortableTableColumnDefinition() to process the generated parts and stash more info in the column options?

@gravitiq-cm
Copy link
Author

ugh... I think I found the immediate issue... it's that (none of) our generated fields have nullable: true set in the doctrine attributes... with this change they no longer show up in the migrations (which is a better position than we had (we want to prevent the false positives appearing).

Now we can actually use this "feature" to tag fields that have new GENERATED code (by removing nullable: true from the attribute) and then the field will be picked up by the migrations... up() will be correct but we'll need to manage down() manually. We can then put nullable: true back and the migration will ignore the GENERATED part again...

However, if you think the approach outlined above is sensible then I could do some work to try and detect actual changes to generated code added through columnDefinitions. (Of course, it would be even better to have a separate attribute parameter for generated fields... but I appreciate that it might not be cross-platform!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant