feat(jdbc): add RequestReason connection property#4094
feat(jdbc): add RequestReason connection property#4094keshavdandeva wants to merge 5 commits intomainfrom
RequestReason connection property#4094Conversation
product-auto-label
bot
added
size: s
Summary of ChangesHello @keshavdandeva, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the BigQuery JDBC driver by adding support for a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new JDBC connection property RequestReason to allow specifying a reason for BigQuery API requests, which is then passed as an x-goog-request-reason header. The implementation is straightforward and correct, with corresponding changes to connection property parsing and header provider logic. A new test case has been added to ensure the property is parsed correctly. I have one minor suggestion to improve the robustness of the createHeaderProvider method by returning an immutable map, which aligns with the previous behavior and is a general best practice.
google-cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryConnection.java
Outdated
Show resolved
Hide resolved
…uery/jdbc/BigQueryConnection.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
product-auto-label
bot
added
size: m
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful RequestReason connection property to tag BigQuery API requests. The implementation is clean and follows existing patterns. I've added a couple of suggestions to enhance robustness by handling blank reasons, improve maintainability by using a constant for the header name, and increase test coverage for edge cases.
| if (this.requestReason != null) { | ||
| headers.put("x-goog-request-reason", this.requestReason); | ||
| } |
There was a problem hiding this comment.
I have a couple of suggestions for this block to improve robustness and maintainability:
- Avoid blank headers: The current code sends the
x-goog-request-reasonheader even ifrequestReasonis an empty string or just whitespace. It's better to avoid sending headers with blank values by adding a check like!this.requestReason.trim().isEmpty(). - Use a constant for the header name: The string
"x-goog-request-reason"is also used in the test file. Defining it as a public constant inBigQueryJdbcUrlUtilitywould avoid magic strings and improve maintainability.
Here's a suggested implementation that applies the first point:
| if (this.requestReason != null) { | |
| headers.put("x-goog-request-reason", this.requestReason); | |
| } | |
| if (this.requestReason != null && !this.requestReason.trim().isEmpty()) { | |
| headers.put("x-goog-request-reason", this.requestReason); | |
| } |
| @Test | ||
| public void testHeaderProviderWithRequestReason() throws IOException, SQLException { | ||
| String requestReason = "Ticket123"; | ||
| String url = | ||
| "jdbc:bigquery://https://www.googleapis.com/bigquery/v2:443;" | ||
| + "OAuthType=2;ProjectId=MyBigQueryProject;" | ||
| + "OAuthAccessToken=redactedToken;OAuthClientId=redactedToken;" | ||
| + "OAuthClientSecret=redactedToken;RequestReason=" | ||
| + requestReason; | ||
| try (BigQueryConnection connection = new BigQueryConnection(url)) { | ||
| HeaderProvider headerProvider = connection.createHeaderProvider(); | ||
| java.util.Map<String, String> headers = headerProvider.getHeaders(); | ||
| assertTrue(headers.containsKey("x-goog-request-reason")); | ||
| assertEquals(requestReason, headers.get("x-goog-request-reason")); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test covers the happy path well. To improve test coverage, please consider adding another test case for when RequestReason is provided but is empty or blank in the connection URL (e.g., RequestReason= or RequestReason= ). This would ensure the driver handles this edge case correctly and doesn't send the header, especially if you implement my other suggestion to avoid sending blank headers.
2 participants
b/478305154
This PR adds support for a new JDBC connection property
RequestReason. When this property is present in the connection string (e.g.,jdbc:bigquery://...;RequestReason=Ticket123), the driver will include thex-goog-request-reasonheader in all underlying BigQuery API requests.