lihaosky commented on code in PR #28791:
URL: https://github.com/apache/flink/pull/28791#discussion_r3993932385
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -64,6 +65,8 @@ public class ShowCreateUtil {
private static final DateTimeFormatter TIMESTAMP_FORMATTER =
DateTimeFormatter.ofPattern("uuuu-MM-dd HH:mm:ss");
private static final String PRINT_INDENT = " ";
+ // Internal secret-store metadata must not be emitted in recreated
connection SQL.
+ private static final String CONNECTION_SECRET_REFERENCE_KEY =
"__flink.encrypted-secret-key__";
Review Comment:
**Hardcoded secret-reference key.**
This literal (also duplicated in `ShowCreateUtilTest` and
`CreateConnectionITCase`) is the same value as
`DefaultConnectionFactory.SECRET_REFERENCE_KEY`. If the factory's key ever
changes, this filter silently stops matching and the reserved key leaks into
output. Consider making the factory constant `public` and referencing it here
and in the tests.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/CatalogManager.java:
##########
@@ -1924,6 +1924,30 @@ public Optional<CatalogConnection>
getConnection(ObjectIdentifier objectIdentifi
}
}
+ /** Get a connection from the catalog with contextual metadata. */
+ public Optional<ContextResolvedConnection> getResolvedConnection(
Review Comment:
**`getResolvedConnection` duplicates `getConnection`.**
This re-implements `getConnection`'s temporary-then-catalog lookup
(including the `ConnectionNotExistException` / `UnsupportedOperationException`
handling) verbatim, only adding the `ContextResolvedConnection` wrapper and
temporary flag. It could delegate to avoid keeping two copies in sync:
```java
return getConnection(objectIdentifier)
.map(c -> ContextResolvedConnection.of(
objectIdentifier, c,
temporaryConnections.containsKey(objectIdentifier)));
```
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -98,6 +101,29 @@ public static String buildShowCreateModelRow(
return sb.toString();
}
+ public static String buildShowCreateConnectionRow(
+ CatalogConnection connection,
+ ObjectIdentifier connectionIdentifier,
+ boolean isTemporary,
+ List<String> additionalSensitiveKeys) {
+ StringBuilder sb =
+ new StringBuilder()
+ .append(
+ buildCreateFormattedPrefix(
+ "CONNECTION",
+ isTemporary,
+ connectionIdentifier,
+ false,
+ false));
+ extractComment(connection).ifPresent(c ->
sb.append(formatComment(c)).append("\n"));
+ extractFormattedOptions(
+
withoutConnectionInternalOptions(connection.getOptions()),
+ PRINT_INDENT,
+ additionalSensitiveKeys)
+ .ifPresent(v -> sb.append("WITH
(\n").append(v).append("\n)\n"));
Review Comment:
**Non-re-runnable DDL when the only option is the filtered secret
reference.**
When a connection's stored options consist solely of the internal secret
reference (e.g. `CREATE CONNECTION c WITH ('password'='x')` —
`DefaultConnectionFactory` extracts the password to the secret store, leaving
`{__flink.encrypted-secret-key__: id}`), `withoutConnectionInternalOptions`
strips that key and `extractFormattedOptions` returns empty, so no `WITH`
clause is appended. The output is then ``CREATE CONNECTION `cat`.`db`.`c` `` —
but the `SqlCreateConnection` grammar makes `WITH` mandatory and rejects an
empty property list ("Connection property list can not be empty."), so the
`SHOW CREATE` output can't be re-executed. Worth handling this edge case (e.g.
retain the credential keys masked, or emit a placeholder).
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlConnectionOperationConverterTest.java:
##########
@@ -121,4 +123,50 @@ void testCreateConnectionWithEmptyOptionsRejected() {
.isInstanceOf(SqlValidateException.class)
.hasMessageContaining("Connection property list can not be
empty.");
}
+
+ @Test
+ void testShowCreateConnection() {
Review Comment:
**Permanent-connection path untested.**
All `SHOW CREATE CONNECTION` happy-path tests (here and in
`CreateConnectionITCase`) use temporary connections, so the permanent branch of
`getResolvedConnection` (`catalog.getConnection(...)`, `isTemporary() ==
false`) and its `CREATE CONNECTION` (no `TEMPORARY`) output are never
exercised. This is harder to set up in the current harness (no
`WritableSecretStore`, and `GenericInMemoryCatalog` doesn't implement
`getConnection`), so a fake connection-supporting catalog fixture would be one
way to cover it. Non-blocking, but worth a follow-up.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -349,6 +375,19 @@ static Optional<String>
extractComment(ResolvedCatalogModel model) {
: Optional.of(model.getComment());
}
+ static Optional<String> extractComment(CatalogConnection connection) {
Review Comment:
**Third near-identical `extractComment` overload.**
This has the same body as the existing
`extractComment(ResolvedCatalogBaseTable)` /
`extractComment(ResolvedCatalogModel)` overloads. Consider folding them into
one helper taking the comment string/supplier to avoid a growing set of
copy-paste overloads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
