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

ResetDatabase does not work with multiple schemas #495

Open
Qronicle opened this issue Aug 25, 2023 · 18 comments · May be fixed by #497
Open

ResetDatabase does not work with multiple schemas #495

Qronicle opened this issue Aug 25, 2023 · 18 comments · May be fixed by #497

Comments

@Qronicle
Copy link

Qronicle commented Aug 25, 2023

[Version: 1.35]

We're having issues with the ResetDatabase trait ever since we introduced new schemas to our PostgreSQL database. (In addition to the default "public" scheme, that is.)

After a little digging I found that the issue lies with how the database is cleared in the ORMDatabaseResetter::dropSchema method. The doctrine:schema:drop command does not seem to drop the schemas, but doctrine:schema:create tries to recreate them anyway in ORMDatabaseResetter::createSchema, which throws an exception.

I can fix it (for me) by just executing the doctrine:schema:update command instead, in ORMDatabaseResetter::createSchema. I'm pretty sure this will work for non-postgres platforms as well.

@kbond
Copy link
Member

kbond commented Aug 28, 2023

Hi @Qronicle, did you configure both object managers in the config?

zenstruck_foundry:
    database_resetter:
        orm:
            object_managers:
                - default
                - other

@Qronicle
Copy link
Author

Qronicle commented Aug 28, 2023

It's not a different object/entity manager though, we only have the default manager.

One example of how we use schemas, is to have a separate one for our audit trail, which we define like this on the entity:

#[Table(name: 'audit_user', schema: 'audit_trail')]

This schema is not configured in any way with the global doctrine settings.

@kbond
Copy link
Member

kbond commented Aug 28, 2023

Ok, I didn't know multiple schema's in the same OM was possible.

So is the issue that doctrine:schema:drop isn't fully dropping all the schemas?

@Qronicle
Copy link
Author

Yeah, I was just adding an edit that I would understand if you think this is more of a Doctrine issue (which it has been since 2016, if I take a look at their issue tracker.) But since this looks like an easy fix in this package, I thought it was worth a shot.

Otherwise I'll have a stab at overriding the method in our project, but that might prove difficult with all those final non-service-container-injected classes. (Although I might be missing something, only had a quick look just now)

@kbond
Copy link
Member

kbond commented Aug 28, 2023

So to confirm, your fix is to use doctrine:schema:update instead of doctrine:schema:create?

Oh, I just confirmed that doctrine:schema:update creates the schema if it doesn't exist (didn't know this) so shouldn't be an issue to change to this?

Question about your use case though - if doctrine:schema:drop doesn't fully remove the schema's, don't you still have data in the database?

@kbond
Copy link
Member

kbond commented Aug 28, 2023

which it has been since 2016, if I take a look at their issue tracker

Can you link this issue here for context? I can't find it.

@Qronicle
Copy link
Author

Qronicle commented Aug 28, 2023

About schema:drop: It removes all tables, just not the schemas.

I guess if you would compare mysql and postgres to a filesystem; mysql would have a file for each table in the mysql root dir. Postgres has subdirs for each schema that contain the table files (by default it puts tables in the "public" dir). Dropping the schema removes all table files, not the schema dirs.

Doctrine issue: doctrine/DoctrineBundle#548 - it's closed though (but I'm pretty sure I've read this on multiple occasions on different places, maybe in combination with migrations?)

Other suggestion: instead of replacing the existing functionality, you could also make it a new reset mode or something else configurable

@kbond
Copy link
Member

kbond commented Aug 28, 2023

It removes all tables, just not the schemas.

Got it, thanks for the explanation! @nikophil, could this in anyway be related to #458/#455?

Other suggestion: instead of replacing the existing functionality, you could also make it a new reset mode or something else configurable

If swapping doctrine:schema:create with doctrine:schema:update works with all db platforms, I'm fine doing a straight-up replace. Could you create a PR so we can see if the test suite passes? This will at least test mysql/postgres.

@Qronicle
Copy link
Author

I'll give it a shot :)

@Qronicle
Copy link
Author

Seems like it's failing on all fronts, I'll retest some things locally to see what I messed up :/

@Qronicle
Copy link
Author

Qronicle commented Aug 28, 2023

Okay, three conclusions:

  1. I'm an idiot and should not create issues on Fridays 😅 - it seems that I enabled the migrate reset mode at some point, but forgot about it, and managed to test it together with the schema:update command modification
  2. Using the migrate reset mode also fixes this issue (if you're using migrations ofc)
  3. My PR minor: use doctrine:schema:update in ResetDatabase #497 for the schema reset mode also works (as long as you add the --force flag, which I hadn't done before and made me discover point 1).

@nikophil nikophil linked a pull request Aug 28, 2023 that will close this issue
@kbond
Copy link
Member

kbond commented Aug 28, 2023

Heh, ok, glad you got it sorted.

So to clarify, doctrine:schema:update is still required to fix your schema issue?

@Qronicle
Copy link
Author

Yes, for me it now works using either the migrate reset mode, or using the schema reset mode with my fix.

Without the fix it throws an exception like

There were 8 errors:

1) App\MyTest::testStuff
RuntimeException: Error running "doctrine:schema:create": 
 ! [CAUTION] This operation should not be executed in a production environment!                                         

 Creating database schema...


In ToolsException.php line 19:
                                                                                                                                                                                                                      
  Schema-Tool failed with Error 'An exception occurred while executing a query: SQLSTATE[42P06]: Duplicate schema: 7 ERROR:  schema "non_public_schema" already exists' while executing DDL: CREATE SCHEMA non_public_schema

@nikophil
Copy link
Member

I can see the command doctrine:schema:drop have the option --full-database, shouldn't we use it if it does not drop all schemas?

@Qronicle
Copy link
Author

Qronicle commented Aug 28, 2023

I tested the --full-database option as well, but it did not remove the schemas for me either. It's intended to also remove tables that are not managed by the entity manager (like migrations or messages)

@nikophil
Copy link
Member

Got it, thanks for the explanation! @nikophil, could this in anyway be related to #458?

I think it's not related:
#458 only kills active connections to the db in order to be able to drop the schema

@nikophil
Copy link
Member

nikophil commented Aug 28, 2023

doctrine acts in a really weird way with postgre's schemas: in all my auto-generated migrations, I have to remove a line where it adds a sql statement with CREATE SCHEMA public in the down() method 🤔

AbstractSchemaManager::dropSchema() and AbstractPlatform::getDropSchemaSQL() output the right sql statement, but it seems they are never called 🤷

@Qronicle do you understand why when you have only one schema, the error does not occur? how I understand the problem, it should occur as well, since it does not drop the schema but tries to re-create it 🤔

@Qronicle
Copy link
Author

Sorry, I don't have much time for testing today, but yeah, schemas do seem to behave a bit inconsistent. For example we also run an event listener to prevent Doctrine from creating empty migrations because of schema usage.

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

Successfully merging a pull request may close this issue.

3 participants