fix(bq jdbc): allow & ignore unknown parameters#12352
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 modifies BigQueryJdbcUrlUtility to log a message instead of throwing an exception when unknown properties are encountered in the JDBC URL, enhancing compatibility with various tools. It also introduces BigQueryJdbcLoggingBaseTest to enable log verification in tests. Review feedback highlights that the logger implementation in BigQueryJdbcUrlUtility incorrectly uses printf-style formatting and suggests using a less aggressive log level. Furthermore, a suggestion was provided to safely access captured logs in tests to avoid potential IndexOutOfBoundsException.
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
...d-bigquery-jdbc/src/test/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtilityTest.java
Show resolved
Hide resolved
logachev
force-pushed
the
kirl/parser_no_error
branch
3 times, most recently
from
cf5bb99 to
b2fbafb
Compare
logachev
force-pushed
the
kirl/parser_no_error
branch
from
b2fbafb to
b93e50b
Compare
| throw new BigQueryJdbcRuntimeException( | ||
| String.format("Wrong value or unknown setting: %s", safeRef)); | ||
| } else { | ||
| LOG.severe("Wrong value or unknown setting: %s", safeRef); |
There was a problem hiding this comment.
I kinda agree with gemini about this log should be warning instead of severe considering it's something the user should ideally fix but can proceed
| String safeRef = ref.length() > 32 ? ref.substring(0, 32) + "..." : ref; | ||
| throw new BigQueryJdbcRuntimeException( | ||
| String.format("Wrong value or unknown setting: %s", safeRef)); | ||
| // Some tools can pass unknown keys. In order not to break compatibility, throw |
There was a problem hiding this comment.
Doubt: So, now we allow unknown connection properties BUT with correct format of key=value. Should we consider allowing unknown connection properties that do not follow correct format (e.g. just a key like ;CustomToolFlag;)
There was a problem hiding this comment.
Is there a use case in the JDBC spec(or any known tools) that use just the key without value in the connection property?
There was a problem hiding this comment.
I guess not really. I was just thinking if the tool passes connection property like SomeProp= and then we end up with same problem. But yeah, maybe it is an edge case and we would still want to throw error for it for now
There was a problem hiding this comment.
Long-term I want to have this enforcement enabled for unknown parameters.. Maybe if we collect information about parameters use, we can get a full list of parameters that should be allowlisted (even if ignored).
| String safeRef = ref.length() > 32 ? ref.substring(0, 32) + "..." : ref; | ||
| throw new BigQueryJdbcRuntimeException( | ||
| String.format("Wrong value or unknown setting: %s", safeRef)); | ||
| // Some tools can pass unknown keys. In order not to break compatibility, throw |
There was a problem hiding this comment.
Is there a use case in the JDBC spec(or any known tools) that use just the key without value in the connection property?
3 participants
Looks like some existing apps provide keys that are not editable by user. At some point we will have an ability to collect such properties & build more permissive map, but for now we will ignore it & log it.
This PR also introduces helper methods for unittests to validate certain log messages were actually logged.