fix(bqjdbc): update shading to be more targeted#13232
Conversation
There was a problem hiding this comment.
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.
| <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> |
There was a problem hiding this comment.
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>
| <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> |
There was a problem hiding this comment.
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>
| <pattern>org.slf4j</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.slf4j</shadedPattern> | ||
| </relocation> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| <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> |
There was a problem hiding this comment.
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.
| <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> |
com. It also updated constant strings, such as 'computeMetadata' which is a part of certain URLs which broke GCE auth flow. b/514870009