Fix failing Dependabot update tests #571
Conversation
Bumps [io.netty:netty-codec-http](https://github.com/netty/netty) from 4.2.6.Final to 4.2.7.Final. - [Commits](netty/netty@netty-4.2.6.Final...netty-4.2.7.Final) --- updated-dependencies: - dependency-name: io.netty:netty-codec-http dependency-version: 4.2.7.Final dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
Summary of ChangesHello @BrandonArp, 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 addresses and resolves failing Dependabot update tests by upgrading a critical Netty HTTP codec dependency. The update necessitated minor code modifications in HTTP request building logic to ensure proper header handling with the new library version, thereby restoring test stability and system compatibility. Highlights
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 updates the netty-codec-http dependency and adjusts the code to handle what appears to be an API incompatibility by converting Netty's AsciiString header constants to String. The changes are correct in fixing the issue. I've provided suggestions for a cleaner approach using constants from async-http-client's HttpConstants class. This improves readability and is more idiomatic for the library being used.
| .setHeader(HttpHeaderNames.CONTENT_TYPE.toString(), MediaTypes.APPLICATION_JSON.toString()) | ||
| .setBody(bodyData) | ||
| .setMethod(HttpConstants.Methods.POST); | ||
|
|
||
| if (_enableCompression) { | ||
| requestBuilder.setHeader(HttpHeaderNames.CONTENT_ENCODING, HttpHeaderValues.GZIP); | ||
| requestBuilder.setHeader(HttpHeaderNames.CONTENT_ENCODING.toString(), HttpHeaderValues.GZIP.toString()); | ||
| } |
There was a problem hiding this comment.
While adding .toString() fixes the issue, a cleaner approach would be to use the string constants from org.asynchttpclient.util.HttpConstants, which is already imported. This avoids potential issues with Netty's AsciiString type and is more idiomatic when using async-http-client.
| .setHeader(HttpHeaderNames.CONTENT_TYPE.toString(), MediaTypes.APPLICATION_JSON.toString()) | |
| .setBody(bodyData) | |
| .setMethod(HttpConstants.Methods.POST); | |
| if (_enableCompression) { | |
| requestBuilder.setHeader(HttpHeaderNames.CONTENT_ENCODING, HttpHeaderValues.GZIP); | |
| requestBuilder.setHeader(HttpHeaderNames.CONTENT_ENCODING.toString(), HttpHeaderValues.GZIP.toString()); | |
| } | |
| .setHeader(HttpConstants.HttpHeaderNames.CONTENT_TYPE, MediaTypes.APPLICATION_JSON.toString()) | |
| .setBody(bodyData) | |
| .setMethod(HttpConstants.Methods.POST); | |
| if (_enableCompression) { | |
| requestBuilder.setHeader(HttpConstants.HttpHeaderNames.CONTENT_ENCODING, HttpConstants.HttpHeaderValues.GZIP); | |
| } |
| final RequestBuilder requestBuilder = new RequestBuilder() | ||
| .setUri(getAysncHttpClientUri()) | ||
| .setHeader(HttpHeaderNames.CONTENT_TYPE, "application/gzip") | ||
| .setHeader(HttpHeaderNames.CONTENT_TYPE.toString(), "application/gzip") |
There was a problem hiding this comment.
While adding .toString() fixes the issue, a cleaner approach would be to use the string constant from org.asynchttpclient.util.HttpConstants, which is already imported. This avoids potential issues with Netty's AsciiString type and is more idiomatic when using async-http-client.
| .setHeader(HttpHeaderNames.CONTENT_TYPE.toString(), "application/gzip") | |
| .setHeader(HttpConstants.HttpHeaderNames.CONTENT_TYPE, "application/gzip") |
Replace Netty HttpHeaderNames and HttpHeaderValues constants with plain string literals to avoid compatibility issues with Netty 4.2.7.Final. Changes: - Replace HttpHeaderNames.CONTENT_TYPE with "Content-Type" - Replace HttpHeaderNames.CONTENT_ENCODING with "Content-Encoding" - Replace HttpHeaderValues.GZIP with "gzip" - Remove unused Netty header imports This avoids any potential incompatibilities between AsyncHttpClient 3.0.3 and Netty 4.2.7's header constant implementations.
BrandonArp
force-pushed
the
claude/fix-dependabot-test-failure-011CUsgPDqDgaNPq7sv4BZKY
branch
from
730f4f3 to
cc989a8
Compare
AsyncHttpClient 3.0.3 brings in various Netty 4.2 modules transitively, but the project also has old Netty 4.0.x versions defined (netty.version and netty.all.version). This version mismatch was causing runtime failures where no HTTP requests were being sent. By adding the Netty BOM (Bill of Materials) to dependencyManagement, all Netty modules across the project will be forced to use version 4.2.7.Final consistently, preventing classpath conflicts. This should resolve the test failures in KairosDbSinkTest where requests were not being sent to the mock server.
Since we replaced all Netty header constants with string literals, the explicit netty-codec-http dependency is no longer directly used. AsyncHttpClient brings in the required Netty dependencies transitively, and the Netty BOM in dependencyManagement ensures they're all at version 4.2.7.Final. This resolves the Maven dependency analyzer warning about unused declared dependencies.
Replaced the netty.codec.http.version property with the standard netty.version property, updating it from 4.0.56.Final to 4.2.7.Final. This makes the versioning clearer - netty.version is now the single source of truth for the Netty version used throughout the project, applied via the Netty BOM in dependencyManagement.
BrandonArp
deleted the
claude/fix-dependabot-test-failure-011CUsgPDqDgaNPq7sv4BZKY
branch
No description provided.