nextcloud / nextcloud/server

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

Open
#45,097 4 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

1. to develop 29-feedback bug
Dominant language
PHP
Stars
36.9k
Forks
5.2k
Avg merge
2d 3h
Merged PRs (30d)
713

Description

⚠️ This issue respects the following points: ⚠️
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
  1. 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.
  2. Try to log in.
  3. 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?
  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other
Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in lib/private/DB/ConnectionFactory.php at ConnectionFactory::getConnection, then compare its parameter merging with Doctrine's PrimaryReadReplicaConnection structure. Reproduce through the user_sql app using an external database and verify that the additional connection parameters select that database rather than the Nextcloud database.

Written by the indexing model from the issue text.

Assessment

Tech stack
mysql, php
Domain
database
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.