Refactor OpenSRS suggest method to properly handle limit parameter#44
Refactor OpenSRS suggest method to properly handle limit parameter#44
Conversation
- Simplified service selection logic by always including all services - Fixed TLD filtering to apply across all services (suggestion, premium, lookup) - Implemented post-processing to enforce limit by prioritizing premium domains - Removed conditional service selection based on price filters - Improved code readability by reducing nested conditionals 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughSuggest API expanded with new filterType and premium price bounds, added getPrice(), stricter validation, always requests suggestion/premium/lookup with per-service overrides, aggregates and limits suggestion/premium results, and updates the Adapter signature and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Adapter as Registrar Adapter (OpenSRS)
participant API as OpenSRS API
Note over Client,Adapter: Input: query, tlds, limit, filterType, premiumPriceMin/Max
Client->>Adapter: suggest(params)
Adapter->>Adapter: validate filterType and premium price bounds
Adapter->>Adapter: normalize query/tlds, build message (service_override, per-service max, premium overrides)
Adapter->>API: request services: suggestion, premium, lookup
API-->>Adapter: response {suggestion[], premium[], lookup[]}
Adapter->>Adapter: parse suggestion & premium blocks
alt price needed for domain
Adapter->>Adapter: call getPrice(domain)
end
alt filterType or limits
Adapter->>Adapter: apply filtering, merging, and in-memory limiting (prioritize premium then suggestion)
end
Adapter-->>Client: consolidated items array
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Domains/Registrar/Adapter.php(1 hunks)src/Domains/Registrar/OpenSRS.php(1 hunks)tests/OpenSRSTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Domains/Registrar/OpenSRS.php (2)
src/Domains/Registrar/Adapter.php (2)
suggest(32-32)tlds(37-37)src/Domains/Registrar.php (2)
suggest(26-29)tlds(31-34)
src/Domains/Registrar/Adapter.php (2)
src/Domains/Registrar/OpenSRS.php (2)
suggest(240-435)tlds(479-483)src/Domains/Registrar.php (2)
suggest(26-29)tlds(31-34)
tests/OpenSRSTest.php (2)
src/Domains/Registrar/Adapter.php (1)
suggest(32-32)src/Domains/Registrar/OpenSRS.php (1)
suggest(240-435)
🪛 GitHub Actions: Linter
src/Domains/Registrar/OpenSRS.php
[error] 1-1: PSR-12: 1 style issue in 11 files. 'OpenSRS.php' issue: braces, function_declaration, no_trailing (linting).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Domains/Registrar/OpenSRS.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.554Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
📚 Learning: 2025-10-05T13:46:00.554Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.554Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (1)
src/Domains/Registrar/OpenSRS.php (2)
src/Domains/Registrar.php (3)
suggest(26-29)tlds(31-34)available(16-19)src/Domains/Registrar/Adapter.php (3)
suggest(32-32)tlds(37-37)available(13-13)
🔇 Additional comments (6)
src/Domains/Registrar/OpenSRS.php (6)
234-256: LGTM! Parameter validation is comprehensive.The validation logic correctly enforces:
- Price range consistency (
premiumPriceMin < premiumPriceMax)- Valid filter types and sort orders
- Incompatible parameter combinations (e.g., price filters with
filterType='suggestion')
258-285: LGTM! Request building correctly implements uniform service querying.The implementation correctly:
- Always queries all three services (
suggestion,premium,lookup) as per PR objectives- Applies TLD filtering uniformly across all services via
service_override- Sets per-service maximum limits when
$limitis provided- Applies premium price filters only to the premium service
292-351: LGTM! Suggestion processing correctly prioritizes non-premium domains.The implementation correctly:
- Processes suggestion domains first, up to
$limit- Fetches prices for available domains via
getPrice()- Includes early return optimizations when sorting is not requested
- Enforces the limit during iteration to avoid over-processing
This aligns with the learnings that suggestion domains should be prioritized first. Based on learnings.
415-436: LGTM! Sorting logic itself is correct.The price-based sorting correctly:
- Uses
uasortto maintain key associations- Treats
nullprices asPHP_FLOAT_MAX(reasonable for unavailable/unpriceable domains)- Applies
$limitafter sorting viaarray_sliceNote: The sorting implementation is sound, but effectiveness depends on having both suggestion and premium domains collected before sorting (see previous comment regarding line 356).
441-481: LGTM! Well-structured price lookup method.The
getPrice()method correctly:
- Follows the established pattern for OpenSRS API calls
- Uses XPath to extract price, premium flag, and premium group from the response
- Handles missing elements gracefully with
isset()checks- Returns a clear, documented structure
This method integrates well with the suggestion processing at line 326.
292-438: Clarify: Implementation prioritizes suggestions, but PR objectives state premium priority.The current implementation prioritizes suggestion domains over premiums when enforcing the limit (lines 308-313, 356, 372), which aligns with the retrieved learning that states: "suggestion domains should be prioritized and shown first... premium domains should only be included if there's remaining capacity after processing suggestions." Based on learnings.
However, this directly contradicts:
- The PR objectives which state: "enforcing the limit via post-processing that prioritizes premium domains over suggestions"
- The previous review comment which flagged that premium domains are excluded when suggestions fill the limit
Current behavior:
- When
$limit=5: fetch up to 5 suggestions first, then fill remaining capacity with premiums- Result: suggestions always appear first; premiums only if space remains
Please confirm:
- Is the current prioritization of suggestions over premiums the intended behavior?
- Should the PR objectives/description be updated to reflect this?
- Is the previous review comment outdated and should be disregarded?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/OpenSRSTest.php (1)
107-107: Consider using assertLessThanOrEqual for consistency.Using
assertCount(5, $result)expects exactly 5 results, which could cause test flakiness if the API returns fewer results. Other tests in this file useassertLessThanOrEqual(lines 132, 154, 172) for more resilient assertions.Apply this diff for consistency:
-$this->assertCount(5, $result); +$this->assertLessThanOrEqual(5, count($result));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Domains/Registrar/OpenSRS.php(1 hunks)tests/OpenSRSTest.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.554Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.554Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
Applied to files:
src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (2)
src/Domains/Registrar/OpenSRS.php (2)
src/Domains/Registrar/Adapter.php (3)
suggest(32-32)tlds(37-37)available(13-13)src/Domains/Registrar.php (3)
suggest(26-29)tlds(31-34)available(16-19)
tests/OpenSRSTest.php (2)
src/Domains/Registrar/OpenSRS.php (2)
suggest(239-408)getPrice(418-450)src/Domains/Registrar/Adapter.php (1)
suggest(32-32)
🔇 Additional comments (5)
tests/OpenSRSTest.php (1)
180-190: LGTM!The test properly validates the structure and types of the getPrice() response, including the new registry premium fields.
src/Domains/Registrar/OpenSRS.php (4)
241-251: LGTM! Solid parameter validation.The validation logic correctly enforces parameter constraints:
- Price range validation (min < max)
- filterType enum validation
- Prevents invalid combinations (suggestion + price filters)
265-288: LGTM! Service overrides configured correctly.The service override logic properly:
- Applies TLD filtering to all services conditionally based on filterType
- Sets per-service maximum limits for suggestion and premium services
- Applies premium price bounds only to the premium service
336-342: Clarify the prioritization behavior in the PR description.The implementation prioritizes suggestion domains over premium domains (filling the limit with suggestions first), which contradicts the PR description stating "prioritizing premium domains over suggestions." However, this matches your stated preference in the past review discussion where you mentioned wanting to show non-premium domains first.
Based on learnings: The current implementation correctly prioritizes suggestions as per your preference, but the PR description should be updated to reflect this behavior accurately.
Consider updating the PR description to state: "enforce the limit parameter via post-processing, prioritizing suggestion domains over premium domains."
410-450: LGTM! Well-structured pricing API.The
getPrice()method provides comprehensive pricing information including:
- Domain price extraction with proper float conversion
- Registry premium status (boolean)
- Premium group classification (string|null)
The XPath queries are clear and the response structure is well-documented.
2 participants
Summary
Changes
The previous implementation had complex conditional logic for selecting services and applying overrides. This refactor:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests