For PrimaryReadReplicaConnection passthrough primary server version

[jdelaune]

Passthrough server version from primary.

Q	A
Type	bug

Summary

When running cache:clear in Symfony we started to get an issue where it was trying to connect to the database after introducing replicas (like: symfony/symfony#37473.)

In src/Connection.php getDatabasePlatformVersion() it is expecting the serverVersion parameter. However for the PrimaryReadReplicaConnection this is one level up in the primary connection.

I don't know the best place for this, so I have put it in the constructor for PrimaryReadReplicaConnection that it copies any serverVersion from the primary to the root for Connection to use.

If anyone who knows this code better thinks this should live somewhere else I'm happy to update my PR.

[derrabus]

Thank you. Please provide a test that reproduces the problem you're attempting to fix.

[jdelaune]

@derrabus okay I have added a test. I couldn't see any previous tests where we check the serverVersion parsed from the connection config. So hopefully I have done something which is acceptable.

[derrabus]

Thank you for the test. However, I don't see how this reproduces the issue you're having. As far as I can tell, the test makes some assertions on the array returned by Connection::getParams() which is an internal method. Whatever this method returns should be completely irrelevant for your application.

[jdelaune]

@derrabus okay thank you for the feedback. As I said initially I don't really know the DBAL codebase, so I'm not sure if I have fixed the issue in the correct place. Let me try and explain the issue I'm experiencing and maybe you can point me to the best place to fix it in.

Following the Symfony guidelines on setting up a database connection I have this:

DATABASE_URL="mysql://db_user:db_password@127.0.0.1:3306/db_name?serverVersion=5.7"

Although you say this is now deprecated, so maybe that is why I'm getting the issue.

Anyway after this url is parsed into all the individual parameters. For the PrimaryReadReplicaConnection the serverVersion is extracted into the config within the higher level key of 'primary'. So for the above example it would produce:

['primary' => ['driver' => 'pdo_mysql', 'host' => '127.0.0.1', 'serverVersion' => '5.7']]

Now eventually this gets used by Connection->getDatabasePlatformVersion() which is looking for the serverVersion parameter at the root level and not within the primary parameter that you get with the PrimaryReadReplicaConnection.

So this test was to validate that the serverVersion is copied to the root level as required elsewhere in the code base.

[jdelaune]

The other option is I can update Connection->getDatabasePlatformVersion() to look for the serverVersion at the root level, or within the primary key if that exists. Although that seems like a less universal solution to me.

[derrabus]

Following the Symfony guidelines on setting up a database connection I have this:

DATABASE_URL="mysql://db_user:db_password@127.0.0.1:3306/db_name?serverVersion=5.7"

Although you say this is now deprecated,

I didn't say that. DriverManager won't take URLs in DBAL 4, but DoctrineBundle will continue to support them.

so maybe that is why I'm getting the issue.

I doubt it. 🙂

So for the above example it would produce:

['primary' => ['driver' => 'pdo_mysql', 'host' => '127.0.0.1', 'serverVersion' => '5.7']]

And with that piece of configuration, do you still encounter the issue? If yes, let's leave URLs out of this test as they just add unnecessary complexity.

So this test was to validate that the serverVersion is copied to the root level as required elsewhere in the code base.

I get that, but whether or not serverVersion is copied to the root level of an internal data structure is irrelevant for the issue you're encountering. What you want to test here is if Connection::getDatabasePlatform() produces the correct platform, so why don't we test that?

[jdelaune]

@derrabus Okay well I have reworked the fix and tests. Hopefully this is to your satisfaction now. I also included a test to cover non-replica connections as there didn't seem to be coverage for this.

[jdelaune]

Anything else I need to do to this?

[derrabus]

Looks good, thank you!