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.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the maven-shade-plugin to version 3.6.1, enables source JAR shading, and replaces broad relocation patterns with a more specific list of subpackages. It also adds exclusions for module-info files. Feedback suggests removing the trailing dot from the com.google. relocation pattern to ensure the base package is included and to maintain consistency with other patterns. Additionally, the reviewer noted that the shift to specific patterns risks missing transitive dependencies like com.github or org.xerial, which could lead to classpath conflicts.
| <pattern>com.google.</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.com.google.</shadedPattern> |
There was a problem hiding this comment.
The relocation pattern for com.google. includes a trailing dot, which is inconsistent with the other relocation patterns defined in this file (e.g., org.apache, io.grpc). Additionally, using a trailing dot prevents the relocation of the base package string "com.google", which might be used in reflection or service loading. Removing the trailing dot is safe and will still avoid the computeMetadata string matching issue described in the PR.
| <pattern>com.google.</pattern> | |
| <shadedPattern>com.google.bqjdbc.shaded.com.google.</shadedPattern> | |
| <pattern>com.google</pattern> | |
| <shadedPattern>com.google.bqjdbc.shaded.com.google</shadedPattern> |
| <relocations> | ||
| <relocation> | ||
| <pattern>com</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.com</shadedPattern> | ||
| <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> | ||
| <relocation> | ||
| <pattern>org</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org</shadedPattern> | ||
| <excludes> | ||
| <exclude>org.conscrypt.*</exclude> | ||
| </excludes> | ||
| <pattern>com.fasterxml.jackson</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.com.fasterxml.jackson</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>org.apache</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.apache</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>org.checkerframework</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.checkerframework</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>org.codehaus</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.codehaus</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>org.jspecify</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.jspecify</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>org.threeten</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.threeten</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>org.json</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.org.json</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>io.grpc</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.io.grpc</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>io.netty</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.io.netty</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>io.opencensus</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.io.opencensus</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>io.opentelemetry</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.io.opentelemetry</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>io</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.io</shadedPattern> | ||
| <pattern>io.perfmark</pattern> | ||
| <shadedPattern>com.google.bqjdbc.shaded.io.perfmark</shadedPattern> | ||
| </relocation> | ||
| </relocations> |
There was a problem hiding this comment.
The transition from broad relocation patterns (com, org, io) to specific subpackages is intended to be more targeted, but it carries the risk of leaving some transitive dependencies unshaded. For example, common dependencies in the BigQuery/gRPC ecosystem like com.github.* (e.g., jnr, zstd), org.xerial.* (Snappy), or org.tukaani.* (XZ) are no longer covered. If these are present in the fat JAR, they could cause classpath conflicts for users. Please verify that all internal dependencies that should be isolated are explicitly included in the relocation list.
com. It also updated constant strings, such as 'computeMetadata' which is a part of certain URLs which broke GCE auth flow. b/514870009