feat: validate driver with database engine #1108
Conversation
| class DriverMapping(Enum): | ||
| """Maps a given database driver to it's corresponding database engine.""" | ||
|
|
||
| ASYNCPG = "POSTGRES" | ||
| PG8000 = "POSTGRES" | ||
| PYMYSQL = "MYSQL" | ||
| PYTDS = "SQLSERVER" |
There was a problem hiding this comment.
I am going to have a follow-up refactor PR that moves the other shared enum types (IPTypes, RefreshStrategy) out of instance.py and into this file.
| @@ -321,6 +323,14 @@ async def connect_async( | |||
| # attempt to make connection to Cloud SQL instance | |||
| try: | |||
| conn_info = await cache.connect_info() | |||
| # validate driver matches intended database engine | |||
| mapping = DriverMapping[driver.upper()] | |||
There was a problem hiding this comment.
What if we encapsulated more details and did something like this?
conn_info = await cache.connect_info()
if not DriverMapping.matching_engine(driver, conn_info.database_version):
raise IncompatibleDriverErrorwhere the DriverMapping method looks like:
class DriverMapping(Enum):
def matching_engine(driver: str, engine: str) -> bool:
return engine.startswith(DriverMapping[driver.upper()])There was a problem hiding this comment.
this would mean i no longer have access to the proper compatible database engine mapping.value in the error message without doing a separate call to DriverMapping[driver.upper()].value which is more work...
There was a problem hiding this comment.
Just an idea -- feel free to handle it however you like. A good error message is most important.
| mapping = DriverMapping[driver.upper()] | ||
| if not conn_info.database_version.startswith(mapping.value): | ||
| raise IncompatibleDriverError( | ||
| f"Database driver '{driver}' is incompatible with database " |
There was a problem hiding this comment.
This is a fatal configuration error. I think the message could be more clear. Something like:
"Fatal Error: the Cloud SQL Connector is configured with an incompatible database driver. Configured driver '{driver}' cannot connect to database instance '{instance_connection_string}' running version {conn_info.database_version}."
There was a problem hiding this comment.
@hessjcg I don't normally see Python errors get prefixed with "Fatal Error:", its more the assumption that if an error is raised from the application it must be fatal.
I like the error message as is personally, concise and actionable. I don't have access to instance_connection_string from where the error is raised after the encapsulation refactor...
Removing ambiguous behavior if a user configures a database
driver to connect to an incompatible database engine.
i.e.
pymysql(MySQL driver) -> Cloud SQL for PostgreSQL databaseThis behavior would timeout trying to connect and then throw
an ambiguous timeout error.
The
Connector.connectalready know which database enginethe Cloud SQL database is prior to attempting to connect via the
driver. So we can validate if the driver is compatible and throw an
actionable error if not.
Closes #998