Skip to content

fix(bqjdbc): update shading to be more targeted#13232

Open
logachev wants to merge 2 commits into
mainfrom
kirl/less_aggressive_shading
Open

fix(bqjdbc): update shading to be more targeted#13232
logachev wants to merge 2 commits into
mainfrom
kirl/less_aggressive_shading

Conversation

@logachev
Copy link
Copy Markdown
Contributor

  • Fixed shading warnings
  • Adding shaded sources JAR (gemini recommendation for debugging via IDE)
  • More explicit shading for libraries: there was a bug due to aggressive shading of com. It also updated constant strings, such as 'computeMetadata' which is a part of certain URLs which broke GCE auth flow. b/514870009

@logachev logachev requested review from a team as code owners May 20, 2026 05:40
Copy link
Copy Markdown
Contributor

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

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 updates the maven-shade-plugin to version 3.6.1 and replaces broad package relocation rules with a highly granular list of sub-packages. It also enables source shading and excludes module-info files from the shaded JAR. Feedback indicates that the granular relocation of com.google.api and com.google.cloud is fragile and should be replaced with parent-package relocation and specific exclusions to avoid missing base classes. Furthermore, it is recommended to avoid shading org.slf4j to ensure the driver's logs remain observable by the host application.

Comment on lines +120 to +138
<pattern>com.google.api.client</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.client</shadedPattern>
</relocation>
<relocation>
<pattern>org</pattern>
<shadedPattern>com.google.bqjdbc.shaded.org</shadedPattern>
<excludes>
<exclude>org.conscrypt.*</exclude>
</excludes>
<pattern>com.google.api.core</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.core</shadedPattern>
</relocation>
<relocation>
<pattern>io</pattern>
<shadedPattern>com.google.bqjdbc.shaded.io</shadedPattern>
<pattern>com.google.api.gax</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.gax</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.api.pathtemplate</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.pathtemplate</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.api.resourcenames</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.resourcenames</shadedPattern>
</relocation>
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.

high

Relocating individual sub-packages of com.google.api is fragile and misses classes in the base package (e.g., com.google.api.Annotations). It is more robust to relocate the entire com.google.api package while excluding the BigQuery-specific services that should remain unshaded. This ensures all transitive dependencies within the API surface are correctly relocated and avoids potential binary incompatibilities.

                    <relocation>
                      <pattern>com.google.api</pattern>
                      <shadedPattern>com.google.bqjdbc.shaded.com.google.api</shadedPattern>
                      <excludes>
                        <exclude>com.google.api.services.bigquery.**</exclude>
                      </excludes>
                    </relocation>

Comment on lines +152 to +171
<pattern>com.google.cloud.audit</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.audit</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.http</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.http</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.location</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.location</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.spi</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.spi</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.testing</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.testing</shadedPattern>
</relocation>
<relocation>
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.

high

Similar to the com.google.api relocation, targeting only specific sub-packages of com.google.cloud misses important base classes like com.google.cloud.ServiceOptions. Relocating the parent package with an explicit exclusion for BigQuery is a safer and more maintainable approach that prevents potential classpath conflicts with the host application's version of google-cloud-core.

                    <relocation>
                      <pattern>com.google.cloud</pattern>
                      <shadedPattern>com.google.bqjdbc.shaded.com.google.cloud</shadedPattern>
                      <excludes>
                        <exclude>com.google.cloud.bigquery.**</exclude>
                      </excludes>
                    </relocation>

Comment on lines +256 to +258
<pattern>org.slf4j</pattern>
<shadedPattern>com.google.bqjdbc.shaded.org.slf4j</shadedPattern>
</relocation>
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

Shading org.slf4j is generally discouraged for libraries and JDBC drivers as it breaks the logging integration with the host application. By relocating the SLF4J facade, the driver's internal logs will not be captured by the application's logging configuration (e.g., Logback or Log4j2). Unless there is a specific version conflict that cannot be resolved otherwise, it is better to leave org.slf4j unshaded to allow for proper observability.

Copy link
Copy Markdown
Contributor

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

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 updates the maven-shade-plugin to version 3.6.1 and refines the shading configuration for the BigQuery JDBC driver. Key changes include enabling the creation of shaded source jars, replacing broad package relocations with a comprehensive list of granular relocations for specific Google, Apache, and third-party libraries, and adding exclusions for module-info files to prevent conflicts in the shaded artifact. I have no feedback to provide as there were no review comments to evaluate.

Copy link
Copy Markdown
Contributor

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

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 updates the maven-shade-plugin to version 3.6.1, enables source shading, and replaces broad relocation patterns with a granular list of sub-packages. It also adds exclusions for module-info files to the shade configuration. Feedback indicates that the manual listing of com.google sub-packages is fragile and potentially incomplete; a more robust approach using a single relocation pattern with specific exclusions for BigQuery packages is recommended to ensure maintainability and avoid classpath conflicts.

Comment on lines 119 to +227
<relocation>
<pattern>com</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com</shadedPattern>
<excludes>
<exclude>com.google.cloud.bigquery.**</exclude>
<exclude>com.google.api.services.bigquery.**</exclude>
</excludes>
<pattern>com.google.api.client</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.client</shadedPattern>
</relocation>
<relocation>
<pattern>org</pattern>
<shadedPattern>com.google.bqjdbc.shaded.org</shadedPattern>
<excludes>
<exclude>org.conscrypt.*</exclude>
</excludes>
<pattern>com.google.api.core</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.core</shadedPattern>
</relocation>
<relocation>
<pattern>io</pattern>
<shadedPattern>com.google.bqjdbc.shaded.io</shadedPattern>
<pattern>com.google.api.gax</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.gax</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.api.pathtemplate</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.pathtemplate</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.api.resourcenames</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.resourcenames</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.apps</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.apps</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.auth</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.auth</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.auto</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.auto</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.audit</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.audit</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.http</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.http</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.location</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.location</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.spi</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.spi</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.testing</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.testing</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.common</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.common</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.errorprone</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.errorprone</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.flatbuffers</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.flatbuffers</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.geo</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.geo</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.gson</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.gson</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.iam</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.iam</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.j2objc</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.j2objc</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.logging</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.logging</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.longrunning</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.longrunning</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.protobuf</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.protobuf</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.rpc</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.rpc</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.shopping</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.shopping</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.thirdparty</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.thirdparty</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.type</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.type</shadedPattern>
</relocation>
<relocation>
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.

high

The current approach of manually listing every com.google.* sub-package is fragile and difficult to maintain. It also misses several critical base packages such as com.google.cloud (which contains core classes like ServiceOptions) and com.google.api, which can lead to runtime classpath conflicts if users have different versions of these core libraries in their environment.

Consolidating these into a single com.google relocation pattern is more robust, ensures all current and future sub-packages are covered, and still avoids the computeMetadata string replacement bug since computeMetadata does not start with com.google.

Suggested change
<relocation>
<pattern>com</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com</shadedPattern>
<excludes>
<exclude>com.google.cloud.bigquery.**</exclude>
<exclude>com.google.api.services.bigquery.**</exclude>
</excludes>
<pattern>com.google.api.client</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.client</shadedPattern>
</relocation>
<relocation>
<pattern>org</pattern>
<shadedPattern>com.google.bqjdbc.shaded.org</shadedPattern>
<excludes>
<exclude>org.conscrypt.*</exclude>
</excludes>
<pattern>com.google.api.core</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.core</shadedPattern>
</relocation>
<relocation>
<pattern>io</pattern>
<shadedPattern>com.google.bqjdbc.shaded.io</shadedPattern>
<pattern>com.google.api.gax</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.gax</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.api.pathtemplate</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.pathtemplate</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.api.resourcenames</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.api.resourcenames</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.apps</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.apps</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.auth</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.auth</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.auto</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.auto</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.audit</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.audit</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.http</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.http</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.location</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.location</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.spi</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.spi</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.cloud.testing</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.cloud.testing</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.common</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.common</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.errorprone</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.errorprone</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.flatbuffers</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.flatbuffers</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.geo</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.geo</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.gson</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.gson</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.iam</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.iam</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.j2objc</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.j2objc</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.logging</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.logging</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.longrunning</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.longrunning</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.protobuf</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.protobuf</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.rpc</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.rpc</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.shopping</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.shopping</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.thirdparty</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.thirdparty</shadedPattern>
</relocation>
<relocation>
<pattern>com.google.type</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google.type</shadedPattern>
</relocation>
<relocation>
<relocation>
<pattern>com.google</pattern>
<shadedPattern>com.google.bqjdbc.shaded.com.google</shadedPattern>
<excludes>
<exclude>com.google.cloud.bigquery.**</exclude>
<exclude>com.google.api.services.bigquery.**</exclude>
</excludes>
</relocation>

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