Fix domain list generation within public suffix #1823
Conversation
📝 WalkthroughWalkthroughThe pull request refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as splitDomain
participant S as Suffix Checker
C->>F: Call splitDomain(domainString)
F->>F: Trim trailing period from domainString
F->>S: Retrieve public suffix & TLD indicator
S-->>F: Return public suffix and flag
F->>F: Compare domain to suffix and split into labels
F->>F: Build domain list (append suffix if non-TLD)
F-->>C: Return processed domain list
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 1
🧹 Nitpick comments (1)
service/intel/entity.go (1)
408-410: Use strings.Split for better readability.While the current implementation with
FieldsFuncworks correctly, usingstrings.Splitwould make the code more straightforward and readable.- labels := strings.FieldsFunc(d[:len(d)-len(suffix)], func(r rune) bool { - return r == '.' - }) + parts := strings.Split(d[:len(d)-len(suffix)-1], ".") + labels := parts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
service/intel/entity.go(1 hunks)service/intel/entity_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
service/intel/entity_test.go
26-26: Function TestSplitDomain missing the call to method parallel
(paralleltest)
🪛 GitHub Check: Linter
service/intel/entity_test.go
[failure] 26-26:
Function TestSplitDomain missing the call to method parallel
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test & Build
🔇 Additional comments (4)
service/intel/entity_test.go (1)
9-24: Test cases cover various domain patterns well.The test cases effectively cover regular domains, subdomains, TLDs, and public suffix domains. Good job including trailing periods in the test cases to properly validate the function's handling of them.
service/intel/entity.go (3)
399-427: Improved domain splitting logic looks good.The refactored
splitDomainfunction now correctly:
- Handles trailing periods by trimming them first
- Properly identifies public suffixes and real TLDs
- Builds domain components more accurately
- Correctly handles non-TLD public suffixes by adding them to the list
The implementation fixes the domain list generation within public suffixes, which was the goal of this PR.
413-419: Efficient domain list generation.The domain list generation logic is well-implemented with appropriate capacity pre-allocation and efficient string joining.
421-424: Good handling of non-TLD public suffixes.Adding the public suffix to the list only when it's not a real TLD is a good approach to handle custom public suffixes like
googleapis.com.
service/intel/entity_test.go
Outdated
| func TestSplitDomain(t *testing.T) { | ||
| for _, testCase := range splitDomainTestCases { | ||
| splitted := splitDomain(testCase[0]) | ||
| assert.Equal(t, testCase, splitted, "result must match") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add t.Parallel() for better test performance.
Calling t.Parallel() at the beginning of your test function allows tests to run concurrently, which can significantly improve test execution time.
func TestSplitDomain(t *testing.T) {
+ t.Parallel()
for _, testCase := range splitDomainTestCases {
splitted := splitDomain(testCase[0])
assert.Equal(t, testCase, splitted, "result must match")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSplitDomain(t *testing.T) { | |
| for _, testCase := range splitDomainTestCases { | |
| splitted := splitDomain(testCase[0]) | |
| assert.Equal(t, testCase, splitted, "result must match") | |
| } | |
| } | |
| func TestSplitDomain(t *testing.T) { | |
| t.Parallel() | |
| for _, testCase := range splitDomainTestCases { | |
| splitted := splitDomain(testCase[0]) | |
| assert.Equal(t, testCase, splitted, "result must match") | |
| } | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
26-26: Function TestSplitDomain missing the call to method parallel
(paralleltest)
🪛 GitHub Check: Linter
[failure] 26-26:
Function TestSplitDomain missing the call to method parallel
|
CI linter failed internally. |
Summary by CodeRabbit
New Features
Tests