Skip to content

chore(bigquery-jdbc): refactor tests to remove hardcoded base uri#13420

Draft
logachev wants to merge 4 commits into
mainfrom
kirl/base_url
Draft

chore(bigquery-jdbc): refactor tests to remove hardcoded base uri#13420
logachev wants to merge 4 commits into
mainfrom
kirl/base_url

Conversation

@logachev

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a helper method getBaseConnectionUrl() in TestUtilities to centralize and parameterize the base JDBC connection URL, and updates various integration tests to use this method instead of hardcoded strings. The review identified several critical compilation errors related to string concatenation and syntax in the updated test files, as well as minor cleanup opportunities for redundant string concatenations.

@logachev

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the integration tests to dynamically construct the BigQuery JDBC connection URL using environment variables or system properties, replacing hardcoded URL strings across multiple test classes. Feedback suggests improving the robustness of the URL flag parsing in TestUtilities, declaring the shared connectionUrl in ITBase as final to ensure immutability, and avoiding redundant caching of the connection URL in local class fields across several test files by referencing ITBase.connectionUrl directly.

Comment on lines +161 to +168
public static String getBaseConnectionUrl() {
String baseUrl = getBaseUrl();
String flags = getUrlFlags();
if (!flags.isEmpty() && !flags.endsWith(";")) {
flags += ";";
}
return "jdbc:bigquery://" + baseUrl + ";" + flags;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of getBaseConnectionUrl is fragile if getUrlFlags() returns a string with leading/trailing whitespaces or a leading semicolon (e.g., from environment variables or system properties). Trimming the flags and stripping any leading semicolon makes the URL construction much more robust.

  public static String getBaseConnectionUrl() {
    String baseUrl = getBaseUrl();
    String flags = getUrlFlags().trim();
    if (flags.startsWith(";")) {
      flags = flags.substring(1);
    }
    if (!flags.isEmpty() && !flags.endsWith(";")) {
      flags += ";";
    }
    return "jdbc:bigquery://" + baseUrl + ";" + flags;
  }

Comment on lines 186 to +187
public static String connectionUrl =
"jdbc:bigquery://https://www.googleapis.com/bigquery/v2:443;ProjectId="
+ DEFAULT_CATALOG
+ ";OAuthType=3;Timeout=3600;";
getBaseConnectionUrl() + "ProjectId=" + DEFAULT_CATALOG + ";OAuthType=3;Timeout=3600;";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The connectionUrl field is a constant used across multiple integration test classes. Declaring it as final prevents accidental re-assignment and ensures immutability.

Suggested change
public static String connectionUrl =
"jdbc:bigquery://https://www.googleapis.com/bigquery/v2:443;ProjectId="
+ DEFAULT_CATALOG
+ ";OAuthType=3;Timeout=3600;";
getBaseConnectionUrl() + "ProjectId=" + DEFAULT_CATALOG + ";OAuthType=3;Timeout=3600;";
public static final String connectionUrl =
getBaseConnectionUrl() + "ProjectId=" + DEFAULT_CATALOG + ";OAuthType=3;Timeout=3600;";


private static String connectionUrl =
"jdbc:bigquery://https://www.googleapis.com/bigquery/v2:443;ProjectId=%s;OAuthType=3;Timeout=3600;";
private static String connectionUrl = ITBase.connectionUrl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid caching the connection URL (which is derived from system properties or environment variables) in a local class field. Since it is only evaluated once, caching it is unnecessary. Reference ITBase.connectionUrl directly instead.

References
  1. Avoid caching system properties or environment variables in class fields if they are only evaluated once during object initialization, as caching is unnecessary in such cases.

private static String DATASET;
private static String connectionUrl =
"jdbc:bigquery://https://www.googleapis.com/bigquery/v2:443;ProjectId=%s;OAuthType=3;Timeout=3600;";
private static String connectionUrl = ITBase.connectionUrl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid caching the connection URL (which is derived from system properties or environment variables) in a local class field. Since it is only evaluated once, caching it is unnecessary. Reference ITBase.connectionUrl directly instead.

References
  1. Avoid caching system properties or environment variables in class fields if they are only evaluated once during object initialization, as caching is unnecessary in such cases.

private static final String TABLE_NAME = "JDBC_DBMETADATA_TEST_TABLE" + randomNumber;
private static String connectionUrl =
"jdbc:bigquery://https://www.googleapis.com/bigquery/v2:443;ProjectId=%s;OAuthType=3;Timeout=3600;";
private static String connectionUrl = ITBase.connectionUrl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid caching the connection URL (which is derived from system properties or environment variables) in a local class field. Since it is only evaluated once, caching it is unnecessary. Reference ITBase.connectionUrl directly instead.

References
  1. Avoid caching system properties or environment variables in class fields if they are only evaluated once during object initialization, as caching is unnecessary in such cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant