Feature/postgres support - #2083
Stephan-Kok wants to merge 8 commits into
Conversation
c0e70a3 to
ab0cc49
Compare
21e994f to
8617431
Compare
|
I have adjusted the CI scripts to contains tests using the native behat tests, but with a postgresql database. I have used the environment 'postgres' for this use case. |
|
|
||
| public function up(Schema $schema): void | ||
| { | ||
| $this->addSql('DROP TABLE sso_provider_roles_eb5'); |
There was a problem hiding this comment.
This drops sso_provider_roles_eb5 outright. I don't see a migration anywhere in either PR that copies existing data from sso_provider_roles_eb5 into sso_provider_roles_eb6 first. Combined with the point raised on #2082 about sso_provider_roles_eb6 starting out empty, can you confirm how existing environments retain their metadata through this transition?
There was a problem hiding this comment.
This release can only be done when previous release has been executed on all nodes. That means that all nodes will use EB6 and not EB5. So it can be dropped.
Data will not be migrated from EB5 to EB6 with the sole reason that the database is populated from Manage. Migrating the data is very error prone and will take quite some effort.
I believe we agreed upon this solution in our call right? Please contact me if you want to discuss some more.
| { | ||
| $conn = $this->connection; | ||
|
|
||
| $exists = $conn->fetchOne( |
There was a problem hiding this comment.
fetchOne() returns a scalar value here (the 1 from SELECT 1), not a row object. See the property access below on $exists (->hashedUserId, ->deleted_at), that won't work as written.
There was a problem hiding this comment.
Good find. I have updated the PR with a upsert fix. Please do note that when you fix the primary key on deleted_at that this will no longer work.
| 'attribute' => $parameters->attributeHash, | ||
| 'attributeStable' => $parameters->attributeStableHash, | ||
| 'consentType' => $parameters->consentType, | ||
| 'hashedUserId' => $exists->hashedUserId, |
There was a problem hiding this comment.
$exists is a scalar (see fetchOne() above), so $exists->hashedUserId/$exists->serviceId/$exists->deleted_at here would not work as written. Can you double check this path?
| "psr-4": { | ||
| "OpenConext\\": "src/OpenConext", | ||
| "OpenConext\\EngineBlock\\Doctrine\\Migrations\\": "migrations/DoctrineMigrations" | ||
| "OpenConext\\EngineBlock\\Doctrine\\Migrations\\": "migrations/MariaMigrations" |
There was a problem hiding this comment.
This namespace (OpenConext\EngineBlock\Doctrine\Migrations) is also mapped to migrations/PostgresMigrations in config/packages/postgres/doctrine_migrations.yaml. Just flagging this so we're aware two migrations (one Maria, one Postgres) could end up with the same version number across the two directories.
3 participants
PostgreSQL support
This PR is a follow-up to [PR #2082](#2082).
In this PR, we remove the
sso_provider_roles_eb5table and introduce PostgreSQL support.PostgreSQL documentation
I have added a document with additional information about how to use the PostgreSQL database and how the integration works:
https://github.com/OpenConext/OpenConext-engineblock/blob/feature/postgres_support/postgres_support.md
The document also describes some additional changes that are required in the EngineBlock container and OpenConext-devconf to support PostgreSQL natively.
Additionally, I think we should discuss whether we can add a CI/CD test using a PostgreSQL database to ensure that PostgreSQL support continues to work going forward.
Consent
One aspect I would like you to look into more deeply is the Consent functionality.
We currently do not use the Consent feature, but PostgreSQL does not support the SQL value
0000-00-00 00:00:00, which is used in a hardcoded SQL query.I have attempted to address this by replacing it with the epoch date
1970-01-01 00:00:00. Please seeDbalConsentRepository.phpin this PR.During my initial investigation, it appeared that
deleted_atmay have been incorrectly configured as a primary key, which is the reason this workaround seems to be necessary.If this is not intentional, it would be preferable to remove this requirement and solve the underlying issue rather than keeping my current workaround. Please take a closer look at this part of the PR and let me know if the
deleted_atprimary key is intentional.