chore(bqjdbc): log connection properties set by the user#13244
chore(bqjdbc): log connection properties set by the user#13244Neenu1995 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds logging for connection properties in BigQueryConnection, including a masking mechanism for sensitive fields, and updates DataSource to allow access to these properties. A new test case verifies this functionality. Feedback highlights a log level mismatch in the test, an incorrect assertion for PartnerToken formatting, and an opportunity to improve the readability of the masking logic by using a set of keywords.
| public void testConnectionPropertiesLoggingAndMasking() throws IOException, SQLException { | ||
| java.util.logging.Logger rootLogger = BigQueryJdbcRootLogger.getRootLogger(); | ||
| java.util.logging.Level originalLevel = rootLogger.getLevel(); | ||
| rootLogger.setLevel(java.util.logging.Level.INFO); |
There was a problem hiding this comment.
The code in BigQueryConnection logs properties at java.util.logging.Level.CONFIG. For the log message to be captured, the logger's level must be set to CONFIG or a finer level (e.g., ALL). Setting it to INFO will prevent the message from being logged, as INFO (value 800) is a coarser level than CONFIG (value 700).
| rootLogger.setLevel(java.util.logging.Level.INFO); | |
| rootLogger.setLevel(java.util.logging.Level.CONFIG); |
| assertTrue(logMessage.contains("ProjectId=MyTestProjectId")); | ||
| assertTrue(logMessage.contains("Location=US")); | ||
| assertTrue(logMessage.contains("OAuthAccessToken=*****")); | ||
| assertTrue(logMessage.contains("PartnerToken= (GPN:secretPartnerToken)")); |
There was a problem hiding this comment.
This assertion appears to be incorrect. Given the URL PartnerToken=GPN:secretPartnerToken, the DataSource constructor sets this.partnerToken to GPN:secretPartnerToken. Subsequently, createProperties() in DataSource constructs the property value as " (GPN:" + this.partnerToken + ")", which results in " (GPN:GPN:secretPartnerToken)". The log message will therefore contain PartnerToken= (GPN:GPN:secretPartnerToken). Please update the assertion to reflect the actual behavior.
| assertTrue(logMessage.contains("PartnerToken= (GPN:secretPartnerToken)")); | |
| assertTrue(logMessage.contains("PartnerToken= (GPN:GPN:secretPartnerToken)")); |
| if ((lowerName.contains("key") | ||
| || lowerName.contains("token") | ||
| || lowerName.contains("password") | ||
| || lowerName.contains("pwd") | ||
| || lowerName.contains("secret")) | ||
| && !lowerName.equals("partnertoken")) { |
There was a problem hiding this comment.
For better readability and maintainability, consider extracting the sensitive keywords into a Set and using a stream to check if the property name contains any of them. This makes the condition less verbose and easier to extend in the future.
For example, you could define a static set of keywords at the class level:
private static final java.util.Set<String> SENSITIVE_KEYWORDS = new java.util.HashSet<>(java.util.Arrays.asList("key", "token", "password", "pwd", "secret"));And then use it in the condition like this:
if (SENSITIVE_KEYWORDS.stream().anyMatch(lowerName::contains)
&& !lowerName.equals("partnertoken")) {1 participant
No description provided.