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

[Bug]: Unknown database type anyelement requested exception caused by a renaming migration #35

Open
Vision42 opened this issue Mar 8, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@Vision42
Copy link

Vision42 commented Mar 8, 2024

What happened?

We have a Laravel project that has the following migration steps:

  • Create column Y_temp
  • Migrate data from column X to Y_temp
  • Drop column X
  • Rename column Y_temp to X

These migrations worked without any problem on CockroachDB version 22.2.8. After updating to version 23.1.16 the renaming step from a new column to one that has been deleted previously throws the following exception:

In AbstractPlatform.php line 452:
                                                                                                                 
  Unknown database type anyelement requested, Doctrine\DBAL\Platforms\PostgreSQL100Platform may not support it.  
                                                                                                              

How to reproduce the bug

Set up a new Laravel project with CockroachDB version 23.X.X. Create migrations with the following steps:

  • Create a new table
  • Create column X
  • Create column Y
  • Drop column X
  • Rename column Y to column X

The last step should fail with the exception from above.

Package Version

1.3.0

PHP Version

8.2.0

Laravel Version

10.47.0

CockroachDB Version

23.1.6

Which operating systems does with happen with?

No response

Notes

I have already done some digging around. At first, I stumbled across this issue: doctrine/dbal#6248 with an associated PR that never got merged.
The changes in the PR worked on my machine, so it seemed to be a general bug in the Postgres implementation of doctrine. But after it tried to run the tests in the PR / write my own tests for it, I found out that it is actually not a problem with a plain PostgreSQL Database. It throws this exception only on a CRDB version above 23 and not on a plain PostgreSQL.

The SQL-Statement in the PostgreSQLSchemaManager from the selecteTableColumns-Function actually returns a different Result between CRDB Version 22, 23 and PostgreSQL 16.
In CRDB v23, it also returns the dropped fields named like this: ........pg.dropped.15...... with the type anyelement.
Doctrine now tries to cast the type anyelement which does not exist in the PostgreSQL implementation, causing the exception.

The changes in the PostgreSQLSchemaManager from the PR above does fix this issue. But i think it should not be implemented in the Doctrine DBAL 'cause it is not a PostgreSQL bug. I think we should find a way to somehow implement the fix in this package.

@Vision42 Vision42 added the bug Something isn't working label Mar 8, 2024
@peterfox
Copy link
Collaborator

@Vision42 thanks for the bug report. This might be a difficult one as DBAL is it's own thing to begin with. I don't know if there is any project covering Crdb.

Also, Laravel 11 is moving away from using DBAL but I realistically haven't had time to make the upgrades to see how much time would be involved.

Is there any chance you can make a start on a test showing the problem at all?

@Vision42
Copy link
Author

Thanks for your reply :)
I've now created additional tests that show the basic problem and created a PR for it.

Vision42 added a commit to Vision42/cockroachdb-laravel that referenced this issue Mar 12, 2024
Vision42 added a commit to Vision42/cockroachdb-laravel that referenced this issue Mar 12, 2024
Add a.attisdropped = false flag to compile columns sql to fix the output of Schema::getColumns().
Add custom cockroach db driver for loading the custom cockroach schema manager to fix renaming issue ylsideas#35
@Vision42
Copy link
Author

@peterfox I think I've found a solution for this issue. I've implemented a fix and updated the PR. The tests I built in the morning are now green with the new changes.

@peterfox
Copy link
Collaborator

@Vision42 it looks good for the latest versions of Laravel 10 but using the lowest compatible packages and older versions of Laravel the tests fail. Hard to get a good idea of what is the problem.

I wouldn't mind if the code fails for Laravel 9/8. 8 will be dropped soon as you mentioned this seems to only be a problem with Crdb v23. It would be good to investigate how all versions of Laravel 10 can work with your changes.

@Vision42
Copy link
Author

@peterfox Laravel 10 seems to require doctrine/dbal ^3.5.1 which implements the introspectTable function. With the lowest compatible packages, doctrien/dbal ^3.2.0 gets installed, which does not implement the introspectTable function called by the renameColumn function. As far as I can see, the renameColumn function does currently not work with the lowest compatible packages for Laravel 10.

Can we just bump up the composer dependency to doctrine/dbal ^3.5.1 or will that break compatibility with older versions of Laravel?

@peterfox
Copy link
Collaborator

@Vision42 It would likely break older versions although it's always hard to tell. It might be a case that your fix lives in the package but has to be switched on.

e.g.

protected function getDoctrineDriver()
{
    if (\Composer\InstalledVersions::satisfies(new VersionParser, 'doctrine/dbal', '^3.5.1')) {
        return new CockroachDBDriver();
    } else {
        return new PostgresDriver();
    }
}

That might save all but the Laravel 10 Lowest where Crdb is v23

@Vision42
Copy link
Author

@peterfox I have just tried this. But my renaming tests fail even when I use the default PostgresDriver. It's still the introspectTable exception. It actually seems to be a problem with the rename function and the lowest stable dependency definition. It is independent of my changes as far as I can see.

To get the rename function to work in Laravel 10 with the lowest stable dependencies, we need to bump up the doctrine/dbal version.
See their comment in Laravel's composer.json (I've just found it now): https://github.com/laravel/framework/blob/ce7f5b15305734ddda21b1b2be5b89204d3827e1/composer.json#L171

The other option is to skip the renaming tests altogether if the doctrine/dbal version is below 3.5.1. I think this would be the most appropriate solution, as we are basically following Laravel's approach here.
I would update the test cases if you are fine with that approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants