feat: support default schema and catalog for PostgreSQL databases #1375
Conversation
The JDBC driver would return the empty string as the current catalog and schema for PostgreSQL databases. This is incorrect, as the default catalog and schema for PostgreSQL is the database name and the 'public' schema.
product-auto-label
bot
added
size: m
| return database == null ? "" : database; | ||
| case GOOGLE_STANDARD_SQL: | ||
| default: | ||
| return ""; |
There was a problem hiding this comment.
Since we are returning empty string at multiple places, would it be better to use a constant?
There was a problem hiding this comment.
I would say no for a couple of reasons:
- The JVM is internally creating a constant for this anyways, so there's no performance gain to be gotten.
- Using a constant means that you have to navigate to that constant to see what the value actually is (although good naming would help clarify that a lot in this case)
- Someone might be tempted to use the constant in tests as well to verify the value that is being returned. That again means that if someone changes the constant value, the test will still succeed. But real code that depends on this behavior might change.
| } | ||
|
|
||
| void checkValidSchema(String schema) throws SQLException { | ||
| String defaultSchema = getDefaultCatalog(); |
There was a problem hiding this comment.
Should this be getDefaultSchema? Also should we skip the variable assignment here?
Also since this didn't get caught, are we missing the test to check schema value for POSTGRESQL?
There was a problem hiding this comment.
Yes, good catch. The same copy-paste error is actually also in the corresponding test case, which is why it was not caught. But I agree that we also need some more testing, so I've added a couple of assertions and test cases to the integration tests as well, to ensure that this works end-to-end.
| } | ||
|
|
||
| void checkValidCatalog(String catalog) throws SQLException { | ||
| String defaultCatalog = getDefaultCatalog(); |
There was a problem hiding this comment.
Do we need the variable here?
There was a problem hiding this comment.
We use it twice on the next line... So we don't need it, but otherwise we would have to call the method twice on the next line, which also does not feel great.
🤖 I have created a release *beep* *boop* --- ## [2.14.0](https://togithub.com/googleapis/java-spanner-jdbc/compare/v2.13.4...v2.14.0) (2023-10-12) ### Features * Support default schema and catalog for PostgreSQL databases ([#1375](https://togithub.com/googleapis/java-spanner-jdbc/issues/1375)) ([2737ece](https://togithub.com/googleapis/java-spanner-jdbc/commit/2737ecec00abd51b796e13375f2ebdfbf8e1b201)) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.17.0 ([#1383](https://togithub.com/googleapis/java-spanner-jdbc/issues/1383)) ([f0209a7](https://togithub.com/googleapis/java-spanner-jdbc/commit/f0209a7be923465a30effc30ac23294299e0cd72)) * Update dependency com.google.cloud:google-cloud-spanner-bom to v6.50.0 ([#1386](https://togithub.com/googleapis/java-spanner-jdbc/issues/1386)) ([8401ef8](https://togithub.com/googleapis/java-spanner-jdbc/commit/8401ef868493a83d2c8b8d68a33a118d2b94f769)) * Update dependency com.google.cloud:google-cloud-spanner-bom to v6.50.1 ([#1388](https://togithub.com/googleapis/java-spanner-jdbc/issues/1388)) ([8ae3919](https://togithub.com/googleapis/java-spanner-jdbc/commit/8ae3919e352dbe02a03a41cfbc440619d299454c)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
The JDBC driver would return the empty string as the current catalog and schema for PostgreSQL databases. This is incorrect, as the default catalog and schema for PostgreSQL is the database name and the 'public' schema.