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

Schema Update not working from json_array to json with DBAL 3 #9078

Open
alexander-schranz opened this issue Oct 4, 2021 · 17 comments
Open
Labels

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Oct 4, 2021

Bug Report

Q A
BC Break yes
Version 2.10.0

Summary

The deprecated json_array type is not longer supported but its actually not possible to upgrade in 2.10.0 the database from json_array to json.

Current behavior

After upgrade to doctrine/orm I did replace all json_array with json but when now running:

bin/console doctrine:schema:update --dump-sql

The the FieldRegistry fails as the database still has the comment (DC2Type:json_array) in it and is so mapped to the json_array type.

How to reproduce

Create a Entity with json_array update doctrine/orm to 2.10.0 change type from json_array to json and run:

bin/console doctrine:schema:update --dump-sql
In Exception.php line 125:

  [Doctrine\DBAL\Exception]
  Unknown column type "json_array" requested. Any Doctrine type that you use has to be registered with \Doctrine\DBAL\Types\Type::addType(). You can get a list of all the known types with \Doct
  rine\DBAL\Types\Type::getTypesMap(). If this error occurs during database introspection then you might have forgotten to register all database types for a Doctrine Type. Use AbstractPlatform#
  registerDoctrineTypeMapping() or have your custom types implement Type#getMappedDatabaseTypes(). If the type name is empty you might have a problem with the cache or forgot some mapping infor
  mation.


Exception trace:
  at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Exception.php:125
 Doctrine\DBAL\Exception::unknownColumnType() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Types/TypeRegistry.php:37
 Doctrine\DBAL\Types\TypeRegistry->get() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Types/Type.php:143
 Doctrine\DBAL\Types\Type::getType() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/MySQLSchemaManager.php:228
 Doctrine\DBAL\Schema\MySQLSchemaManager->_getPortableTableColumnDefinition() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:884
 Doctrine\DBAL\Schema\AbstractSchemaManager->_getPortableTableColumnList() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:195
 Doctrine\DBAL\Schema\AbstractSchemaManager->listTableColumns() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:308
 Doctrine\DBAL\Schema\AbstractSchemaManager->listTableDetails() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/MySQLSchemaManager.php:334
 Doctrine\DBAL\Schema\MySQLSchemaManager->listTableDetails() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:293
 Doctrine\DBAL\Schema\AbstractSchemaManager->listTables() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:1132
 Doctrine\DBAL\Schema\AbstractSchemaManager->createSchema() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/SchemaTool.php:955
 Doctrine\ORM\Tools\SchemaTool->createSchemaForComparison() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/SchemaTool.php:931
 Doctrine\ORM\Tools\SchemaTool->getUpdateSchemaSql() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/UpdateCommand.php:79
 Doctrine\ORM\Tools\Console\Command\SchemaTool\UpdateCommand->executeSchemaCommand() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/AbstractCommand.php:47
 Doctrine\ORM\Tools\Console\Command\SchemaTool\AbstractCommand->execute() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/doctrine/doctrine-bundle/Command/Proxy/UpdateSchemaDoctrineCommand.php:40
 Doctrine\Bundle\DoctrineBundle\Command\Proxy\UpdateSchemaDoctrineCommand->execute() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/symfony/console/Command/Command.php:299
 Symfony\Component\Console\Command\Command->run() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/symfony/console/Application.php:996
 Symfony\Component\Console\Application->doRunCommand() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/symfony/framework-bundle/Console/Application.php:96
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/symfony/console/Application.php:295
 Symfony\Component\Console\Application->doRun() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/symfony/framework-bundle/Console/Application.php:82
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/vendor/symfony/console/Application.php:167
 Symfony\Component\Console\Application->run() at /Users/alexanderschranz/Documents/Sulu/sulu-develop.localhost/vendor/sulu/sulu/src/Sulu/Bundle/TestBundle/Resources/app/console:27

Expected behavior

An upgrade form json_array to json should work and the doctype comment be removed by using:

ALTER TABLE se_role_settings CHANGE value value JSON NOT NULL; -- no comment
@derrabus
Copy link
Member

derrabus commented Oct 4, 2021

Did you only upgrade ORM from 2.9 to 2.10 or did you also upgrade DBAL from 2.13 to 3.1?

@derrabus derrabus added the Bug label Oct 4, 2021
@alexander-schranz
Copy link
Contributor Author

@derrabus with dbal 3.1.

@derrabus
Copy link
Member

derrabus commented Oct 4, 2021

This is actually a DBAL issue, I'm afraid: DBAL 3 does not ship with the json_array type anymore and the ORM can only register types that DBAL actually provides.

You need to upgrade your schema before upgrading to DBAL 3.

@alexander-schranz
Copy link
Contributor Author

@derrabus but should a not longer exist type just be ignored and the schema still be updated to the new schema?

@derrabus
Copy link
Member

derrabus commented Oct 4, 2021

Yes maybe. But at the moment, the DBAL can only create schema diffs if it knows all types that are used in the current database.

@alexander-schranz alexander-schranz changed the title Schema Update not working from json_array to json on 2.10.0 Schema Update not working from json_array to json with DBAL 3 Oct 6, 2021
@beberlei
Copy link
Member

Can you register the json_array type manually to use the JsonType class for it? Since you only need it for the migration, registering the type manually could do the trick.

@alexander-schranz
Copy link
Contributor Author

Sure there are workaround like just remove manual the json_array from the table, or first downgrade to doctrine/dbal 2 before going to dbal 3. But all things require here manual updates of a project and in case of a library which migrated from json_array to json for its entities semi nice as you don't want in that case register a type which is already not longer supported. Sadly I'm not into it how the diff works but from my point of view it would be a great feature if the diff don't care about the doctrine type just about what the diff in the schema is and what it need to be changed.

@chasen
Copy link
Contributor

chasen commented Oct 26, 2021

I am having the same issue. This is a breaking change introduced in doctrine/orm 2.10. When its dependencies were updated to doctrine/dbal: ^2.13.1 || ^3.1.1. Previously this packaged installed the dbal 2 version. But after upgrading it now installs the dbal 3 version by default.

Sure this can be prevented by also adding in a top level requirement to our project for "doctrine/dbal": "^2.13.1" but that shouldn't be needed with a minor version change.

@YetiCGN
Copy link

YetiCGN commented Oct 26, 2021

Expected behavior
An upgrade from json_array to json should work and the doctype comment be removed by using:

ALTER TABLE se_role_settings CHANGE value value JSON NOT NULL; -- no comment

we_analytics.content from the WebsiteBundle is also affected as well as ac_activities (two JSON columns) from the ActivityBundle.

@derrabus
Copy link
Member

Let's not collect more me-toos in here. We know about it, we know the cause, we know the workarounds. If you want to contribute to the solution, please feel free comment or open a PR.

To recap:

  • The breaking change was introduced with DBAL 3.0. Whenever you pull a major version upgrade of a package, you need to expect breaking changes and test thoroughly.
  • The ORM right now relies on DBAL for the type system. If DBAL removed a type, it's unavailable to the ORM as well.
  • The ORM relies on DBAL to produce a schema diff, with the known limitation that all the types the current database uses have to be available.
  • ORM does not force a DBAL upgrade: DBAL 2 is still supported and probably will be for some time.

tl;dr: If you use deprecated DBAL types, don't upgrade DBAL before you have migrated away from them.

This is a community driven library. If you have an idea how to create a smoother upgrade path, please open a PR to either DBAL or ORM and we discuss it. If you want to to express that this issue affects you, feel free to 👍🏻 the issue description.

@chasen
Copy link
Contributor

chasen commented Oct 26, 2021

Would it make sense to release these changes as a new major version as well? It would seem that when dependent packages are upgraded to next major versions and when those versions are known to have breaking changes a new major version of the parent package would be warranted.

When introducing dbal v3 why not also release that as orm 3? This would provide the expected manual upgrade path and continue to follow the values of semver.

@greg0ire
Copy link
Member

greg0ire commented Oct 26, 2021

@chasen nice question, let me answer it 🙂

No, it would not make sense. When using json_array in an ORM mapping, you are referring to a DBAL type, meaning the link this points to shows a class defined in the doctrine/dbal package, not in the ORM. If you do that, then you should require doctrine/dbal in your composer.json, thus making the dependency explicit, and the upgrade would then become something conscious on your end.

I think an improvement can be made by mentioning this in the docs: https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/tutorials/getting-started.html#getting-started-with-doctrine , in case some of you would be willing to improve the developer experience.

@chasen
Copy link
Contributor

chasen commented Oct 26, 2021

Thanks @greg0ire this is what we ended up doing but it still leaves a bad taste. This feels like a poor developer experience where you had been relying on a single package previously and then a breaking change was introduced in that package without a major version change.

If the issue is indeed only with DBAL and not with ORM why not remove it as a dependency then and have developers do as you suggest and explicitly add the requirement on the dbal version of their choice?

Or Is that what you are recommending get updated in the getting started docs? That instead of only relying on doctrine/orm that developers also specifically define the doctrine/dbal package too?

@greg0ire
Copy link
Member

If the issue is indeed only with DBAL and not with ORM why not remove it as a dependency then and have developers do as you suggest and explicitly add the requirement on the dbal version of their choice?

The ORM requires the DBAL (cannot work without it). What I'm saying is that the user should also require it if they mention it in the code they write.

Or Is that what you are recommending get updated in the getting started docs? That instead of only relying on doctrine/orm that developers also specifically define the doctrine/dbal package too?

Yes, as soon as you write mapping files, you depend on the DBAL, and if you use the ORM, I think you will be using mapping files. So I would expect doctrine/dbal to appear in the composer.json mentioned in "getting started".

@chasen
Copy link
Contributor

chasen commented Oct 26, 2021

That makes sense. Thanks for all of the explanation. If I get a few minutes later today, ill see if I can take a stab at updating the getting started docs to better call this out to help others avoid similar issues.

chasen added a commit to chasen/orm that referenced this issue Oct 26, 2021
As discussed in doctrine#9078 when entities utilize data mappings which are provided by the dbal lib it is expected behavior that users will explicitly define their dependency on the package.
greg0ire added a commit that referenced this issue Oct 28, 2021
As discussed in #9078 when entities utilize data mappings which are provided by the dbal lib it is expected behavior that users will explicitly define their dependency on the package.

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
@bordeux
Copy link

bordeux commented May 31, 2022

I also today reach this issue, but i fixed it using this migration:

<?php declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;


final class Version20220529152449 extends AbstractMigration
{
    public function up(Schema $schema): void
    {
        $query = "
        SELECT c.column_name,
                   pgd.description,
                   c.table_schema,
                   c.table_name
            FROM pg_catalog.pg_statio_all_tables as st
            inner join pg_catalog.pg_description pgd on (pgd.objoid=st.relid)
            inner join information_schema.columns c on (pgd.objsubid=c.ordinal_position and c.table_schema=st.schemaname and c.table_name=st.relname)
            WHERE pgd.description = :desc
        ";

        $data = $this->connection
            ->executeQuery($query, ["desc" => '(DC2Type:json_array)'])
            ->fetchAllAssociative();

        foreach ($data as $column) {
            $this->addSql("
                COMMENT ON COLUMN {$column['table_schema']}.{$column['table_name']}.{$column['column_name']} IS NULL;
            ");
        }
    }

    public function down(Schema $schema): void
    {
        // no way back!
    }
}

@nicolassing
Copy link

nicolassing commented Jan 30, 2023

I don't know if anyone still has this problem, but this solution seems to work perfectly :
https://dunglas.dev/2022/01/json-columns-and-doctrine-dbal-3-upgrade/

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

No branches or pull requests

8 participants