[Bug]: ConnectionFactory::getConnection parameters messed up by switch to PrimaryReadReplicaConnection

kainhofer commented 5 months ago

⚠️ This issue respects the following points: ⚠️

[X] This is a bug, not a question or a configuration/webserver/proxy issue.
[X] This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
[X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
[X] I agree to follow Nextcloud's Code of Conduct.

Bug description

In the commit https://github.com/nextcloud/server/commit/79c4986354da2c7dd102de21174114e7687daf98, the Nextcloud server code switched from using \Doctrine\DBAL\Connection for the DB connections to Doctrine\DBAL\Connections\PrimaryReadReplicaConnection.

Apparently PrimaryReadReplicaConnection uses a different array structure for its arguments (connection params are stored in the ['primary'] sub-array instead of first-level array elements). Unfortunately, the code in ConnectionFactory::getConnection (https://github.com/nextcloud/server/blob/147426c3ca7183ad065293bd9b600e10adde4abf/lib/private/DB/ConnectionFactory.php#L129) merges in the connection parameters as first-level arguments (as was required for the old Connection class) rather than into the ['primary'] sub-array, which is now needed for the PrimaryReadReplicaConnection.

So, while the old code needed

Array(
    'host' => "localhost",
    'password' => "myPassword",
    'user' => "DBUsername",
    'dbname' => "DBUserPassword"
)

to connect to a database other than the nextcloud database, the new Primary ReadReplicaConnection now needs them in the primary sub-array:

Array(
    'primary' => Array(
        'host' => "localhost",
        'password' => "myPassword",
        'user' => "DBUsername",
        'dbname' => "DBUserPassword"
    )
)

For a difference of the params, I found this nice page: https://blog.adamcameron.me/2023/01/php-primaryreadreplicaconnection.html

I suppose the correct fix would be to properly fix the ConnectionFactory to override the primary connection data instead of merging the params at top-level, where they are never used. There actually appear some more places in the code, where connection settings appear to be merged, which probably also need fixing, so I think I'm unable to provide a proper patch to fix this issue for real.

See also my remarks and my debugging efforts in the user_sql repo: https://github.com/nextcloud/user_sql/issues/193

Steps to reproduce

Use the user_sql app with a database different from the nextcloud database. That plugin uses ConnectionFactory::getConnection to connect to an external user database.
Try to log in.
There is an error that the table cannot be found in the nextcloud database. Apparently, the array of database access parameters passed to getConnection is no longer applied.

Expected behavior

The connection should be done to the database specified in the $additionalConnectionParams argument to getConnection.

Installation method

Community Web installer on a VPS or web space

Nextcloud Server version

29

Operating system

Debian/Ubuntu

PHP engine version

None

Web server

Apache (supported)

Database engine version

MySQL

Is this bug present after an update or on a fresh install?

Upgraded to a MAJOR version (ex. 22 to 23)

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

[X] Default user-backend (database)
[ ] LDAP/ Active Directory
[ ] SSO - SAML
[X] Other

Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

kesselb commented 5 months ago

Thanks for your report 👍

cc @juliushaertl @ChristophWurst

joshtrichards commented 4 months ago

Related: #45257 (impacting db conversions).

d-jsb commented 3 months ago

What is needed to fix this? My cloud is basically broken as user_sql does not work anymore with NC29.

nextcloud / server