Postgres config 3605 #3862
Postgres config 3605 #3862
Conversation
masonwheeler
requested review from
michel-tricot,
marcosmarxm and
cgardens
and removed request for
jrhizor,
cgardens and
michel-tricot
There was a problem hiding this comment.
Approving this incremental change. There are a handful of questions, but I think they are relatively straightforward.
I don't think this closes the issue yet, as we still need to figure out how to hook this up. That said I don't think there is anything breaking in this PR, so we can go ahead and merge it and then do the hook up part separately.
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.Mockito.*; |
There was a problem hiding this comment.
by convention we try to avoid the XYZ.* imports (when you use the create test macro in intellij it does this automatically unfortunately. not sure how to get around it.)
|
|
||
| // Create a table named CONFIG with three columns: CONFIG_ID (PK UUID/string), CONFIG_TYPE (string), | ||
| // CONFIG_DATA (JSON/string) | ||
| public abstract void Setup() throws SQLException; |
There was a problem hiding this comment.
It's to be called by whatever code initializes the storage system for the first time, and also by the tests of course. That part isn't hooked up to any existing code yet other than the test.
| } | ||
|
|
||
| public PostgresConfigPersistence(JsonNode config, JsonSchemaValidator validator) { | ||
| this(config.get("username").asText(), config.get("password").asText(), Databases.getPostgresJdbcUrl(config), validator); |
There was a problem hiding this comment.
i appreciate the attempt to factor out this configuration component here, but I think this probably a place where we want to keep the code paths separate.
here's how i'm thinking about it... the config (a json blob) that we use in the postgres source, postgres destination, and the database that is used in airbyte-core are all different. at least in the case of source and destination, their configuration objects are declared separately.
this is to bringing to light one thing, which is putting these naked JsonNode in interfaces is kinda dangerous... they're almost no better than just doing type Object. this is a bit more tolerable in the connector world because (theoretically) the objects are validated (although as we discussed in your last PR, the tests don't check for it explicitly).
In general where possible we should reduce the use of just JsonNode, and I think in this case the schema of the object is knowable enough that we can do that.
| @@ -116,4 +124,30 @@ default void executeWithinTransaction(List<String> queries) throws SQLException | |||
| CheckedFunction<ResultSet, T, SQLException> recordTransform) | |||
| throws SQLException; | |||
|
|
|||
| default <T> Optional<T> querySingle(String sql, CheckedFunction<ResultSet, T, SQLException> recordTransform, String... params) | |||
There was a problem hiding this comment.
would it make sense to call this executeParameterized and have it return a Stream<T>? after that you can still do a queryFirst or just in the consuming code fo the findFirst
There was a problem hiding this comment.
if you do that you might be able to use it in listConfigs.
| jsonSchemaValidator = validator; | ||
| } | ||
|
|
||
| // Create a table named CONFIG with three columns: CONFIG_ID (PK UUID/string), CONFIG_TYPE (string), |
There was a problem hiding this comment.
i think it is idiomatic in PG to use lower cased (snake cased) table names and column names.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public abstract class DatabaseConfigPersistence implements ConfigPersistence { |
There was a problem hiding this comment.
is this abstraction saving us all that much? i'm not super convinced it's getting us much over just having PostgresConfigPersistence. I guess we are mainly avoiding duplicating the get and list queries. it's probably little enough code that i would be just okay with duplicating.
it's just a guess on my part though, so please feel free to stick with this if you think it's worth it, but i wanted to share my perception.
| @@ -109,4 +116,23 @@ private static BasicDataSource createBasicDataSource(final String username, | |||
| return connectionPool; | |||
| } | |||
|
|
|||
| public static String getPostgresJdbcUrl(JsonNode config) { | |||
There was a problem hiding this comment.
see my comment below, but i don't think using JsonNode config here is very safe. It's hard to know if this is the config for pg source, pg destination, or something else.
Fixes #3605.
UUIDtype, rather thanString, for IDs that are supposed to always be in the form of a UUID