-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: enable custom lookup for httpclient and fetch #5749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Dipper30, 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 introduces a significant enhancement by allowing developers to define a custom DNS lookup function for all outgoing HTTP requests made through the application's 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
|
There was a problem hiding this 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 introduces a feature to enable custom lookup functions for httpclient and fetch, which is a valuable addition. The implementation correctly propagates the lookup configuration in most places. However, I've identified a few critical issues where per-request or per-call options are unconditionally overridden by the global configuration, which would prevent users from specifying their own lookup functions for specific calls. Additionally, there are some logic errors and areas for improvement in the new test files. My review includes suggestions to fix these issues to ensure the feature works as expected and the new tests are robust.
6a1f8f7 to
cd0caf5
Compare
fbd6085 to
65242ae
Compare
fix: httpclient_next lookup fix: lookup param chore: delete log fix: lookup param fix: lookup param
4a69984 to
3691a34
Compare
killagu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
chore: add HttpClient lookup type style: lint fix
3691a34 to
3113a31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables custom DNS lookup functionality for Eggjs's HTTP client and fetch APIs. It allows developers to configure a custom DNS resolution function through the httpclient.lookup config option, which applies to curl, safeCurl, fetch, and safeFetch methods. The implementation includes comprehensive test coverage with a new test fixture application.
Key Changes:
- Added support for custom
lookupfunction inhttpclientconfiguration that applies across all HTTP client methods - Modified fetch factory to initialize with custom lookup options per application instance
- Enhanced TypeScript definitions to include the
LookupFunctiontype
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
lib/core/fetch_factory.js |
Added fetch function export with custom lookup initialization logic; modified safeFetch to support custom lookup |
lib/egg.js |
Updated fetch binding to use new fetch function; added lookup option passing to createHttpClient |
lib/core/httpclient.js |
Added lookup option support from app config |
lib/core/httpclient_next.js |
Added lookup option support and propagation to SSRF client |
index.d.ts |
Added LookupFunction type import and lookup property to HttpClientConfig interface |
test/lib/core/fetch_factory.test.js |
Added custom lookup test configuration |
test/lib/core/dns_resolver.test.js |
New comprehensive test suite for custom DNS lookup functionality |
test/fixtures/apps/dns_resolver/* |
New test fixture app with custom lookup configuration |
Comments suppressed due to low confidence (1)
lib/core/fetch_factory.js:46
- The ssrfFetchFactory is a module-level singleton that persists across all application instances. In scenarios where different app instances need different lookup or checkAddress configurations, they will incorrectly share the same factory instance initialized by the first app. This could lead to security issues or incorrect DNS resolution. Consider making this factory instance-specific or properly scoping it to each application instance.
safeFetch = function safeFetch(url, init) {
if (!ssrfFetchFactory) {
const ssrfConfig = this.config.security?.ssrf;
const clientOptions = {};
if (ssrfConfig?.checkAddress) {
clientOptions.checkAddress = ssrfConfig.checkAddress;
} else {
this.logger.warn('[egg-security] please configure `config.security.ssrf` first');
}
if (this.config.httpclient?.lookup) {
clientOptions.lookup = this.config.httpclient.lookup;
}
ssrfFetchFactory = new FetchFactory();
ssrfFetchFactory.setClientOptions(clientOptions);
}
return ssrfFetchFactory.fetch(url, init);
};
| fetch = function fetch(url, init) { | ||
| if (!fetchInitialized) { | ||
| const clientOptions = {}; | ||
| if (this.config.httpclient?.lookup) { | ||
| clientOptions.lookup = this.config.httpclient.lookup; | ||
| } | ||
| FetchFactory.setClientOptions(clientOptions); | ||
| fetchInitialized = true; | ||
| } | ||
| return FetchFactory.fetch(url, init); | ||
| }; |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetchInitialized flag is a module-level singleton that persists across all application instances. In a testing environment where multiple app instances may be created, this flag will remain true after the first instance initializes fetch, preventing subsequent instances from properly configuring their lookup function. This could lead to incorrect behavior in tests or multi-instance scenarios. Consider making this flag instance-specific or resetting it appropriately.
e09d222 to
d366e6d
Compare
d366e6d to
58ed957
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #5749 +/- ##
==========================================
- Coverage 99.58% 99.42% -0.17%
==========================================
Files 36 36
Lines 3653 3678 +25
Branches 545 546 +1
==========================================
+ Hits 3638 3657 +19
- Misses 15 21 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
58ed957 to
ffa1a92
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
ffa1a92 to
d269a95
Compare
fengmk2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我先修复一下 3.x 的 auto release 脚本���好了会合并。
pick from #5749 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * HttpClient initialization now gracefully handles missing configuration, preventing potential runtime errors. * **New Features** * Added support for custom DNS lookup function configuration, allowing users to override default DNS resolution behavior. * **Tests** * Expanded test coverage for DNS caching functionality and HTTP client DNS resolution scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Uh oh!
There was an error while loading. Please reload this page.