github-actions[bot] commented on code in PR #68811: URL: https://github.com/apache/doris/pull/68811#discussion_r4268968789
########## fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/DiagnosticException.java: ########## @@ -0,0 +1,30 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.spi; + +/** + * Opt-in diagnostic policy for exceptions whose original causes must be retained but must not + * be rendered directly. The connector owns sanitization; the engine uses these outputs without + * inspecting connector properties. This interface is shared parent-first across plugin loaders. + */ +public interface DiagnosticException { Review Comment: [P2] Advance the plugin API majors for the new diagnostic interfaces. The supplied base already declares connector API 13.0 and JNI API 5.0 but contains neither new DiagnosticException interface; this head retains those same versions. A JDBC plugin built here therefore passes the old host's major-version gate, although JdbcDiagnosticException now implements an interface absent from that host's parent SPI (the plugin zip excludes fe-connector-spi), causing NoClassDefFoundError when the diagnostic class is resolved. JdbcIOException has the analogous JNI dependency. Bump both family majors and their pinned surface-test versions so those incompatible plugin/host combinations are rejected before use. ########## fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/JdbcJniScanner.java: ########## @@ -387,7 +391,7 @@ private void initializeClassLoaderAndDataSource() { } hikariDataSource = ds; JdbcDataSource.getDataSource().putSource(cacheKey, hikariDataSource); - LOG.info("JdbcJniScanner: Created connection pool for " + jdbcUrl); + LOG.info("JdbcJniScanner: Created connection pool for " + JdbcExceptionUtils.redact(jdbcUrl)); Review Comment: [P2] Apply the configured URL policy to successful connection logs. A valid SQL Server URL such as `jdbc:sqlserver://host;password={two";words};databaseName=db` survives FE normalization, but this redactor leaves `;words}` in the logged URL: the URL regex stops at the quote, and the unquoted-credential branch stops at the semicolon. This exposes a password suffix because the call supplies no exact sensitive values. Pass `jdbcUrl` itself (and the configured password) to `redact`, or mask the entire URL. Apply the same fix to JdbcJniWriter's pool-creation log and JdbcConnectionTester's success log. Quote/semicolon brace escaping is supported by [Microsoft's JDBC URL documentation](https://learn.microsoft.com/en-us/sql/connect/jdbc/building-the-connection-url?view=sql-server-ver17#escaping-values-in-the-connection-url). ########## fe/be-java-extensions/plugin-toolkit/src/main/java/org/apache/doris/jni/toolkit/jdbc/JdbcExceptionUtils.java: ########## @@ -0,0 +1,222 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.jni.toolkit.jdbc; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.sql.SQLException; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.Deque; +import java.util.IdentityHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** JDBC diagnostics shared by FE metadata clients and BE readers/writers. */ +public final class JdbcExceptionUtils { + private static final Pattern REMOTE_DIAGNOSTIC = Pattern.compile( + "(\\[remote_sqlstate=(?:[A-Za-z0-9]{5}|unknown), remote_vendor_error_code=-?[0-9]+, message=)" + + "((?:\\[redacted JDBC URL\\]|\\\\.|[^\\\\\\]])*+)(\\])", Pattern.DOTALL); + private static final Pattern JDBC_URL = Pattern.compile("(?i)jdbc:[^\\s\"'<>]+"); + private static final Pattern CREDENTIAL = Pattern.compile( + "(?i)([\"']?(?:password|passwd|pwd|user|username|token|secret|access[_-]?key)[\"']?)(\\s*[=:]\\s*)" + + "(\\[redacted JDBC URL\\]|\"(?:\\\\.|[^\"\\\\])*+\"|'(?:\\\\.|[^'\\\\])*+'|[^\\s;&,]+)"); + + private JdbcExceptionUtils() { + } + + /** + * Collects both JDBC chains, with identity-based cycle detection. Identical diagnostics are + * emitted once, but errors with the same message and different remote codes stay distinct. + * Non-SQL exceptions retain their messages without inventing remote error codes. + */ + public static String format(String context, Throwable error, String... sensitiveValues) { + return format(context, error, true, sensitiveValues); + } + + /** Appends only SQL diagnostics, preserving non-SQL exception messages and formatting. */ + public static String appendSqlDiagnostics(String context, Throwable error, String... sensitiveValues) { + return format(context, error, false, sensitiveValues); + } + + private static String format(String context, Throwable error, boolean includeMessages, String[] sensitiveValues) { + // RuntimeException constructors allow an absent cause; retain that existing JDBC API behavior. + if (error == null) { + return redact(context, sensitiveValues); + } + Set<Throwable> visited = Collections.newSetFromMap(new IdentityHashMap<Throwable, Boolean>()); + Set<String> messages = new LinkedHashSet<>(); + Set<String> sqlMessages = new LinkedHashSet<>(); + Set<String> diagnostics = new LinkedHashSet<>(); + Deque<Throwable> pending = new ArrayDeque<>(); + pending.add(error); + while (!pending.isEmpty()) { + Throwable current = pending.removeFirst(); + if (!visited.add(current)) { + continue; + } + String message = redact(current.getMessage(), sensitiveValues); + if (current instanceof SQLException) { + SQLException sqlError = (SQLException) current; + String sqlState = sqlError.getSQLState(); + sqlState = sqlState == null || sqlState.trim().isEmpty() ? "unknown" : sqlState; + diagnostics.add("[remote_sqlstate=" + sqlState + + ", remote_vendor_error_code=" + sqlError.getErrorCode() + + ", message=" + escapeDiagnosticMessage(message) + "]"); + sqlMessages.add(message); + if (sqlError.getNextException() != null) { + pending.addLast(sqlError.getNextException()); + } + } else if (includeMessages && !message.isEmpty()) { + messages.add(message); + } + if (current.getCause() != null) { + pending.addLast(current.getCause()); + } + } + messages.removeAll(sqlMessages); + String safeContext = redact(context, sensitiveValues); + diagnostics.removeIf(diagnostic -> safeContext.contains(diagnostic) + || messages.stream().anyMatch(message -> message.contains(diagnostic))); Review Comment: [P2] Deduplicate diagnostics inside nested wrapper messages. For an OceanBase streaming source with rejected credentials, JdbcClient.getConnection() formats a client exception, JdbcMySQLClient's constructor formats it again, and StreamingInsertJob formats that retained chain once more. At the last step, `messages` contains both `initialize: connect: [remote_...]` and `connect: [remote_...]`. This removes the standalone diagnostic but retains both messages, so the final error repeats the same SQLSTATE/vendor-code frame. The validator adds the same extra wrapping when a connection borrow fails after client initialization. Deduplicate frames carried by nested formatted messages as well, and cover at least three formatting layers; the current one-wrapper test and direct-SQLException streaming test miss this path. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
