Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JDBC backend: infer catalog and schema if not specified #8131

Merged
merged 2 commits into from
Feb 28, 2024
Merged

JDBC backend: infer catalog and schema if not specified #8131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ea8d59
JDBC backend: infer catalog and schema if not specified
adutra Feb 28, 2024
f5310b7
Don't default to empty string
adutra Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 3 additions & 8 deletions servers/quarkus-common/src/main/java/org/projectnessie/quarkus/config/QuarkusJdbcConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,19 @@

import io.quarkus.runtime.annotations.StaticInitSafe;
import io.smallrye.config.ConfigMapping;
import io.smallrye.config.WithConverter;
import io.smallrye.config.WithDefault;
import io.smallrye.config.WithName;
import java.util.Optional;
import org.projectnessie.versioned.storage.jdbc.JdbcBackendBaseConfig;

@StaticInitSafe
@ConfigMapping(prefix = "nessie.version.store.persist.jdbc")
public interface QuarkusJdbcConfig extends JdbcBackendBaseConfig {

@WithName("catalog")
@WithDefault("")
@WithConverter(RepoIdConverter.class)
@Override
String catalog();
Optional<String> catalog();
Copy link
Contributor Author

@adutra adutra Feb 28, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe defaulting to the empty string is wrong. According to the javadocs of java.sql.DatabaseMetaData#getTables:

catalog – a catalog name; must match the catalog name as it is stored in the database; "" retrieves those without a catalog; null means that the catalog name should not be used to narrow the search

So, using the empty string means that we are going to look for tables that do not belong to any catalog, which seems wrong. Same for schema.

Still according to the javadocs, it's imo better to pass null if the catalog is not known, the risk being that a table in a different catalog could exist with the same name as a Nessie table. But in this case, the schema check would fail anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: the PG driver does not seem to comply with the javadocs: looking at the implementation of getTables, catalog is not used at all, and schema is ignored when it's null or empty.

So the previous code was probably correct for PG, but maybe not correct for other drivers.


@WithName("schema")
@WithDefault("")
@WithConverter(RepoIdConverter.class)
@Override
String schema();
Optional<String> schema();
}
20 changes: 12 additions & 8 deletions ...oned/storage/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/JdbcBackend.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,18 @@ public final class JdbcBackend implements Backend {
private final DatabaseSpecific databaseSpecific;
private final DataSource dataSource;
private final boolean closeDataSource;
private final JdbcBackendConfig config;
private final String createTableRefsSql;
private final String createTableObjsSql;
private String catalog;
private String schema;

public JdbcBackend(
@Nonnull JdbcBackendConfig config,
@Nonnull DatabaseSpecific databaseSpecific,
boolean closeDataSource) {
this.config = config;
this.dataSource = config.dataSource();
this.catalog = config.catalog().orElse(null);
this.schema = config.schema().orElse(null);
this.databaseSpecific = databaseSpecific;
this.closeDataSource = closeDataSource;
createTableRefsSql = buildCreateTableRefsSql(databaseSpecific);
Expand Down Expand Up @@ -176,6 +178,12 @@ Connection borrowConnection() throws SQLException {
@Override
public void setupSchema() {
try (Connection conn = borrowConnection()) {
if (catalog == null || catalog.isEmpty()) {
catalog = conn.getCatalog();
}
if (schema == null || schema.isEmpty()) {
schema = conn.getSchema();
}
Integer nameTypeId = databaseSpecific.columnTypeIds().get(NAME);
Integer objIdTypeId = databaseSpecific.columnTypeIds().get(OBJ_ID);
createTableIfNotExists(
Expand Down Expand Up @@ -212,10 +220,6 @@ private void createTableIfNotExists(
Map<String, Integer> expectedPrimaryKey)
throws SQLException {

// TODO implement catalog + schema stuff...
String catalog = config.catalog();
String schema = config.schema();

try (Statement st = conn.createStatement()) {
if (conn.getMetaData().storesLowerCaseIdentifiers()) {
tableName = tableName.toLowerCase(Locale.ROOT);
Expand Down Expand Up @@ -279,11 +283,11 @@ private static String sortedColumnNames(Collection<?> input) {
@Override
public String configInfo() {
StringBuilder info = new StringBuilder();
String s = config.catalog();
String s = catalog;
if (s != null && !s.isEmpty()) {
info.append("catalog: ").append(s);
}
s = config.schema();
s = schema;
if (s != null && !s.isEmpty()) {
if (info.length() > 0) {
info.append(", ");
Expand Down
10 changes: 5 additions & 5 deletions ...ge/jdbc/src/main/java/org/projectnessie/versioned/storage/jdbc/JdbcBackendBaseConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
*/
package org.projectnessie.versioned.storage.jdbc;

import jakarta.annotation.Nullable;
import java.util.Optional;

public interface JdbcBackendBaseConfig {

@Nullable
String catalog();
/** The JDBC catalog name. If not provided, will be inferred from the datasource. */
Optional<String> catalog();

@Nullable
String schema();
/** The JDBC schema name. If not provided, will be inferred from the datasource. */
Optional<String> schema();
}