fix(java): support Cloud SQL socket DATABASE_URL on Cloud Run#393
fix(java): support Cloud SQL socket DATABASE_URL on Cloud Run#393
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for Cloud SQL socket-style DATABASE_URL format when running on Google Cloud Run. The changes enable the application to automatically detect and configure Cloud SQL connections using Unix domain sockets, which is the preferred connection method on Cloud Run.
Changes:
- Adds comprehensive URL parsing and normalization logic to convert PostgreSQL URLs (including socket-style URLs) into JDBC-compatible format
- Automatically configures Cloud SQL socket factory and connection properties when socket mode is detected
- Sets lazy refresh strategy for Cloud SQL connections when running on Cloud Run (detected via K_SERVICE/K_REVISION environment variables)
- Adds test coverage for socket-style URL normalization and Cloud Run detection
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/java/src/main/java/org/openapitools/config/DataSourceConfig.java | Implements URL normalization, credential extraction, and Cloud SQL socket configuration with support for postgres:// and postgresql:// URL schemes |
| src/java/src/test/java/org/openapitools/config/DataSourceConfigTest.java | Adds tests for socket-style URL normalization and Cloud Run environment detection; updates existing tests to set Cloud Run environment variables |
| url.startsWith("postgres://") | ||
| ? "postgresql://" + url.substring("postgres://".length()) | ||
| : url; | ||
| URI uri = URI.create(normalized); |
There was a problem hiding this comment.
The URI.create() method on line 104 can throw IllegalArgumentException if the URL string violates RFC 2396. There is no error handling for malformed URLs, which could cause the application to fail during startup if an invalid DATABASE_URL is provided. Consider wrapping this in a try-catch block and either providing a clear error message or falling back to a safe default behavior.
| URI uri = URI.create(normalized); | |
| URI uri; | |
| try { | |
| uri = URI.create(normalized); | |
| } catch (IllegalArgumentException ex) { | |
| throw new IllegalArgumentException("Invalid PostgreSQL database URL: " + url, ex); | |
| } |
| String[] userInfoParts = rawUserInfo.split(":", 2); | ||
| String user = userInfoParts.length > 0 ? userInfoParts[0] : ""; | ||
| String password = userInfoParts.length > 1 ? userInfoParts[1] : ""; | ||
|
|
||
| StringBuilder query = new StringBuilder(rawQuery == null ? "" : rawQuery); | ||
| if (!existingKeys.contains("user") && isNotBlank(user)) { | ||
| appendQueryParam(query, "user", user); | ||
| } | ||
| if (!existingKeys.contains("password") && isNotBlank(password)) { | ||
| appendQueryParam(query, "password", password); |
There was a problem hiding this comment.
Credentials extracted from the URL are appended to the query string without proper URL encoding. The code uses getRawUserInfo() which returns URL-encoded values, but then splits and appends them directly. If the credentials contain special characters that need encoding (e.g., '@', '&', '=', '%', spaces), and they are not already properly encoded in the source URL, the resulting JDBC URL will be malformed. The safest approach is to: 1) URL-decode the extracted credentials using URLDecoder, 2) URL-encode them again before appending using URLEncoder.encode(value, StandardCharsets.UTF_8). This ensures correct handling regardless of whether the source URL was properly encoded.
| if (isNotBlank(host) && decodeQueryValue(host).startsWith("/cloudsql/")) { | ||
| return decodeQueryValue(host); |
There was a problem hiding this comment.
The extractUnixSocketPath method decodes the host query parameter value twice on line 222 and line 223. The decodeQueryValue(host) is called twice for the same value - once in the condition check and once in the return statement. This is inefficient and could lead to double-decoding issues if the value contains URL-encoded characters. Consider decoding once and storing the result in a variable.
| if (isNotBlank(host) && decodeQueryValue(host).startsWith("/cloudsql/")) { | |
| return decodeQueryValue(host); | |
| if (isNotBlank(host)) { | |
| String decodedHost = decodeQueryValue(host); | |
| if (decodedHost.startsWith("/cloudsql/")) { | |
| return decodedHost; | |
| } |
| setField(config, "cloudRunService", "lamp-control-service"); | ||
| setField(config, "cloudRunRevision", ""); |
There was a problem hiding this comment.
The test coverage for Cloud Run detection only tests the K_SERVICE environment variable (line 115), but the isCloudRun() method also checks K_REVISION. Consider adding a test case where only K_REVISION is set to ensure both environment variables are properly handled.
| @Test | ||
| void hikariConfig_ShouldNormalizeCloudSqlSocketStyleDatabaseUrl() throws Exception { | ||
| DataSourceConfig config = new DataSourceConfig(); | ||
|
|
||
| setField(config, "springDatasourceUrl", ""); | ||
| setField( | ||
| config, | ||
| "databaseUrl", | ||
| "postgresql://postgres:redacted-password@/lamp-control?" | ||
| + "host=/cloudsql/project-id:region:instance-id&connect_timeout=5"); | ||
| setField(config, "fallbackJdbcUrl", "jdbc:postgresql://localhost:5432/fallback"); | ||
| setField(config, "username", "ignored"); | ||
| setField(config, "password", "ignored"); | ||
| setField(config, "driverClassName", "org.postgresql.Driver"); | ||
| setField(config, "cloudRunService", ""); | ||
| setField(config, "cloudRunRevision", ""); | ||
|
|
||
| HikariConfig result = config.hikariConfig(); | ||
|
|
||
| assertThat(result.getJdbcUrl()) | ||
| .isEqualTo( | ||
| "jdbc:postgresql://localhost/lamp-control" | ||
| + "?host=/cloudsql/project-id:region:instance-id" | ||
| + "&connect_timeout=5" | ||
| + "&user=postgres" | ||
| + "&password=redacted-password"); | ||
| assertThat(result.getDataSourceProperties()) | ||
| .containsEntry("socketFactory", "com.google.cloud.sql.postgres.SocketFactory") | ||
| .containsEntry("unixSocketPath", "/cloudsql/project-id:region:instance-id") | ||
| .doesNotContainKey("cloudSqlRefreshStrategy"); | ||
| } |
There was a problem hiding this comment.
Test coverage is missing for edge cases such as URLs with special characters in credentials (e.g., passwords containing '@', '&', '=', '%'), malformed URLs, and URLs without credentials. These edge cases are important to test given the complex URL parsing and credential extraction logic. Consider adding test cases for: passwords with special characters that need URL encoding, URLs with only username (no password), and URLs with unusual but valid formats.
Summary
Testing