fix: should return default value when env is empty string #758
Conversation
WalkthroughThis update refines the processing of environment variables in the env() function. The function now trims whitespace from string values and applies a falsy check rather than only checking for undefined. The error handling for boolean and number types remains intact. In addition, a comprehensive test suite has been introduced using Mocha and egg-mock to validate behavior under various conditions including default value returns, whitespace handling, type conversions, and error cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant EnvUtil as env()
participant Env as process.env
Caller ->> EnvUtil: call env(key, type, default)
EnvUtil ->> Env: Retrieve process.env[key]
EnvUtil -->> EnvUtil: Check if value is a string
EnvUtil ->> EnvUtil: Trim whitespace if applicable
EnvUtil ->> EnvUtil: Evaluate if value is falsy
alt Value is falsy
EnvUtil -->> Caller: Return defaultValue
else Valid value
EnvUtil ->> EnvUtil: Convert to required type
EnvUtil -->> Caller: Return converted value
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
不写单测不行。。。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/common/EnvUtil.test.ts (1)
56-62: Consider adding test for trimmed value.While the test suite is comprehensive, consider adding a test case to verify that leading/trailing whitespace is properly trimmed from non-empty values:
it('should work with string value', () => { mock(process.env, 'TEST_ENV_STRING', 'http://localhost:3000'); assert.equal(env('TEST_ENV_STRING', 'string', 'default'), 'http://localhost:3000'); + mock(process.env, 'TEST_ENV_STRING', ' value with spaces '); + assert.equal(env('TEST_ENV_STRING', 'string', 'default'), 'value with spaces'); + mock(process.env, 'TEST_ENV_STRING', ' '); assert.equal(env('TEST_ENV_STRING', 'string', 'default'), 'default'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/common/EnvUtil.ts(1 hunks)test/common/EnvUtil.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-mysql57-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (18, ubuntu-latest)
- GitHub Check: test-mysql57-fs-nfs (18.20.0, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (22, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (20, ubuntu-latest)
- GitHub Check: Analyze (typescript)
- GitHub Check: test-postgresql-fs-nfs (18, ubuntu-latest)
- GitHub Check: test-postgresql-fs-nfs (18.20.0, ubuntu-latest)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
app/common/EnvUtil.ts (1)
7-12: LGTM! Improved handling of empty and whitespace-only environment variables.The changes correctly handle empty strings and whitespace by:
- Trimming whitespace from string values
- Using falsy check to handle empty strings, null, and undefined consistently
This aligns with the PR objective to fix behavior when env is empty string.
test/common/EnvUtil.test.ts (1)
5-91: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Default value handling
- Empty string and whitespace handling
- Error conditions
- All supported value types
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #758 +/- ##
==========================================
+ Coverage 96.35% 96.43% +0.07%
==========================================
Files 192 192
Lines 19274 19277 +3
Branches 2513 2520 +7
==========================================
+ Hits 18572 18589 +17
+ Misses 702 688 -14 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [3.74.1](v3.74.0...v3.74.1) (2025-02-10) ### Bug Fixes * should return default value when env is empty string ([#758](#758)) ([e72e396](e72e396))
Summary by CodeRabbit
Refactor
Tests