[Stephan-Kok]

PostgreSQL preparation

This PR prepares EngineBlock (EB) for PostgreSQL support, as the previous PR is no longer valid.

Currently, there is only one real blocker for PostgreSQL support: the certificates column in sso_provider_roles_eb5. There are also some hardcoded SQL statements that need to be addressed, as well as an issue with Consents and dates.

As part of this PR, I have removed all serialized types from the database and changed the relevant columns that store JSON values to use the JSON type.

Backwards compatibility

As agreed, EB must remain backwards compatible, meaning that EB version X and EB version X+1 must both be able to work with the same database.

To maintain this requirement, I have introduced a new table: sso_provider_roles_eb6.

During the transition, the current EB version will write to both tables. The new version will use the new table as its source of truth.

PostgreSQL support will be added in the following release, as this requires sso_provider_roles_eb5 to be phased out.

Testing

I have tested the changes locally using OpenConext-devconf:

• dev environment:

  • Manual testing with Manage
  • Manual testing with SP-debug

• ci environment:

  • Behat tests, except for the functional tests that require a Chromium browser

• All unit tests

PostgreSQL PR

Before committing/releasing this PR, please also review my follow-up PR that introduces PostgreSQL support. It would be preferable to have that PR tested before this one is released, so that any required changes can still be made.

I will add the PostgreSQL PR as a comment once it has been created.

Cleanup

Once this PR has been merged and the old PRs are no longer needed, they can be closed/removed:

• Save custom objects using custom doctrine type mappings switch to eb6 #1166
• Save custom objects using custom doctrine type mappings Remove eb5 #1167
• Save custom objects using custom doctrine type mappings #1159

[kayjoosten]

@johanib can you take a look at the pr. I approved as i see no more blockers.

[johanib]

LGTM. One comment about making the fields support nullable values.

[johanib]

It does not seem correct to make this nullable. The function never returns null. We know this because of the string return type currently in place.

We see that there is currently a mismatch: in the AbstractRole, the fields are nullable.
But in practice, the fields are always intialized with a string, or an empty string.

This change seems to open the door: Null is allowed, but does not change the initialization and usage: No longer accept '' and return null if a translation is absent.

Is this change needed for the postgres support?

[Stephan-Kok]

Well, this is exactly the proposed change. Instead of initializing the fields with an empty string to mark the absence of a translation they would contain the actual null value. This is done by changing the AbstractRole constructor, as shown in [this discussion](#2082 (comment)).

This change is not necessarily required for PostgreSQL support; it was also part of the overall cleanup.

That said, I believe you were planning to remove these columns altogether in the same release? If so, maybe it would be best to create a separate story to look into this?

Just let me know if you need any more input from me or if you want the empty strings back.

FYI:
AbstractRole != your current AbstractRole that is AbstractRoleEB5
AbstractRole = sso_provider_roles_eb6
AbstractRoleEB5 = sso_provider_roles_eb5

See:
https://github.com/OpenConext/OpenConext-engineblock/pull/2082/changes/BASE..3e60751a741297c018ba1c3b3e7ae2bd1db1f13b#diff-fbbcdf4ebef5bb4d13514bf45b782c2aa8607a034ff8634cc9d40b3252fa7903R57
and
https://github.com/OpenConext/OpenConext-engineblock/pull/2082/changes/BASE..3e60751a741297c018ba1c3b3e7ae2bd1db1f13b#diff-2f95eaeba1426bba21b2a1287922938bbe521661045aca7ffea46ad5c23b5e70R56

[johanib]

In general, I think this change is good, but two issues, I think:

• getDescription and getKeywords dont use the constructor values, but the mdui values. They were not changed, and always return a string. So these should be reverted to 'string' ?
•  IdentityProviderNameFallbackHelper::getDisplayName(): string  returns  getName('en') , which can now be  null

so, while the return type change is good, the users of those functions need to be updated to conform to those changes.

We should indeed take a good look at cleaning up this part of the code in the getName cleanup ticket, so we have a clear interface on these methods.
Question is if the current changes are good enough, or too risky..

[Stephan-Kok]

My bad, I thought you were still discussing the database values. I can see that the MDUI values are checked and never null. It wouldn't be risky tho, because it it's never null it will never throw an error, but I definitely agree that they should go back.

I also looked at the other values in the corresponding classes and corrected them to the correct values were applicable. However, when I saw the pipeline failing for an unknown reason, so I reverted them. Yet it still fails. Locally everything works at my end. Is this some issue with the ci/cd? @johanib can you look into this? I have no more time and ill be off for the coming 4 weeks.