Skip to content

chore: add Doctrine migrations#447

Merged
tomudding merged 3 commits intoGEWIS:mainfrom
tomudding:chore/doctrine-migrations
Nov 13, 2024
Merged

chore: add Doctrine migrations#447
tomudding merged 3 commits intoGEWIS:mainfrom
tomudding:chore/doctrine-migrations

Conversation

@tomudding
Copy link
Copy Markdown
Member

@tomudding tomudding commented Oct 20, 2024

Description

Adds support for migrations while using Doctrine ORM. The initial migration is based on the current main.

Normally, a migrations:rollup would be used to ensure that this migration is successful. However, in our case it is necessary to export ALL data (but not structure) from the database, drop everything in the database, and only then perform the migration. Data can be easily exported from PostgreSQL using:

pg_dump -U gewisdb --column-inserts --data-only gewisdb
pg_dump -U gewisdb --column-inserts --data-only gewisdb_report

This ensures that all FKs are the same for newer migrations preventing headaches in the future.


Convenience methods have been added for make to make using the migrations easier. However, in production everything must go directly through ./orm.


Also added the proper tooling to ensure that each database connection is made using the SET ROLE query.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation improvement (no changes to code)
  • Other (please specify)

@tomudding tomudding requested a review from rinkp October 20, 2024 22:57
@tomudding
Copy link
Copy Markdown
Member Author

Part of a series of enhancements to hopefully make maintainability easier.

@rinkp
Copy link
Copy Markdown
Member

rinkp commented Oct 21, 2024

How will this work with SET ROLE to prevent ownership issues down the road.

I don't think we can add that as a connection option. Do we wish to set the role on every connection or are we going to add this to each migration?

I think the former is preferred, but because it has to happen after creating the connection, will thus always require an extra command.

@tomudding
Copy link
Copy Markdown
Member Author

How will this work with SET ROLE to prevent ownership issues down the road.

Totally forgot about that. Need to think about the best solution. The easiest is adding it to each migration imho as you have to edit them anyway (and I already wanted to add a README for them, so then it can be documented too).

@tomudding tomudding force-pushed the chore/doctrine-migrations branch from 5fa9d53 to fe02177 Compare October 21, 2024 10:52
@tomudding
Copy link
Copy Markdown
Member Author

PHPStan fails because it is using the new config on the old main.

@tomudding
Copy link
Copy Markdown
Member Author

TODO:

  • update internal wiki page on migrations (or remove it)

tomudding and others added 2 commits November 2, 2024 13:38
Adds support for migrations while using Doctrine ORM. The initial migration is
based on the current `main`.

Normally, a `migrations:rollup` would be used to ensure that this migration is
successful. However, in our case it is necessary to export ALL data (but not
structure) from the database, drop everything in the database, and only then
perform the migration. Data can be easily exported from PostgreSQL using:

```sh
pg_dump -U gewisdb --column-inserts --data-only gewisdb
pg_dump -U gewisdb --column-inserts --data-only gewisdb_report
```

This ensures that _all_ FKs are the same for newer migrations preventing
headaches in the future.

---

Convenience methods have been added for `make` to make using the migrations
easier. However, in production everything must go directly through `./orm`.
Initially, we attempted to use the `wrapperClass` option to handle setting the
database role. However, we encountered issues because DBAL checks for the exact
`Connection` class type and not the interface, making it difficult to extend the
`Connection` class as needed.

We also considered overwriting the PgSQL-specific connection class (PDO variant)
to set the role upon connection. Unfortunately, this was not an option because
the class is declared as `final`, preventing us from extending it.

The next potential solution was to use an `EventSubscriber` to set the role
after the connection was established (using `postConnect`). However, this
approach is already deprecated in our version of DBAL and completely removed in
the next major release, rendering it unsuitable for us (maintainability).

Ultimately, we implemented the `SET ROLE` functionality using DBAL's middleware.
By wrapping the driver, and manually creating the `Connection` we can perform
the `SET ROLE` query before the connection is used by the application.

Runtime checks exist to ensure that the role for each database is defined.
However, validation of the actual value is done by PostgreSQL itself (it will
complain if the role does not exist).

Co-Authored-By: Rink <rinkp@users.noreply.github.com>
@tomudding tomudding force-pushed the chore/doctrine-migrations branch from fe02177 to f80419f Compare November 2, 2024 12:38
@tomudding tomudding requested a review from rinkp November 2, 2024 12:45
Copy link
Copy Markdown
Member

@rinkp rinkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me; perhaps we should update the instructions to get the database working initially, but apart from that, migrations seem to be working properly!

@tomudding tomudding merged commit 3e2b516 into GEWIS:main Nov 13, 2024
github-actions bot added a commit that referenced this pull request Nov 13, 2024
Tom Udding: Merge pull request #447 from tomudding/chore/doctrine-migrations

chore: add Doctrine migrations

Co-authored-by: tomudding <tomudding@users.noreply.github.com>
@tomudding
Copy link
Copy Markdown
Member Author

Seeding of test data is likely significantly delayed due to ORM issues, see GEWIS/gewisweb#1913 and GEWIS/gewisweb#1918.

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

Successfully merging this pull request may close these issues.

2 participants