Skip to content

[player-counter] allow domains and IPs and normalise them properly#138

Open
harryyoud wants to merge 1 commit into
pelican-dev:mainfrom
harryyoud:main
Open

[player-counter] allow domains and IPs and normalise them properly#138
harryyoud wants to merge 1 commit into
pelican-dev:mainfrom
harryyoud:main

Conversation

@harryyoud
Copy link
Copy Markdown

@harryyoud harryyoud commented Jun 1, 2026

Minor refactor also to minimise duplicated code and to minimise number of times the address is (de)serialized.
New methods marked as protected/private as there shouldn't be any need to mess with these downstream.

Summary by CodeRabbit

  • Bug Fixes
    • Improved host validation and normalization to correctly handle IPv4, IPv6 (including bracketed addresses) and hostnames.
    • Query execution now uses validated/normalized host values and respects configured alias preference for more reliable connectivity.
    • Unspecified or invalid network addresses are now properly rejected to reduce query failures and false positives.
    • Simplified pre-checks to ensure queries only run when a valid host is available.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 200a5cf5-6671-427e-ad6a-0b01a7d05995

📥 Commits

Reviewing files that changed from the base of the PR and between d8fe0c8 and c8bc04b.

📒 Files selected for processing (1)
  • player-counter/src/Models/GameQuery.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • player-counter/src/Models/GameQuery.php

📝 Walkthrough

Walkthrough

The GameQuery class centralizes host selection and adds validation/normalization helpers: isValidHost(), normaliseIpAddress(), and getHost(). runQuery() and canRunQuery() delegate host resolution to getHost(), and QueryTypeService::process() now receives the validated host.

Changes

Host Validation and Query Integration

Layer / File(s) Summary
Host validation and normalization helpers
player-counter/src/Models/GameQuery.php
isValidHost() validates normalized IPs or hostnames via inet_pton()/inet_ntop() and FILTER_VALIDATE_DOMAIN. normaliseIpAddress() normalizes IPs, rejects unspecified addresses (:: and 0.0.0.0), and brackets IPv6.
Host selection orchestration
player-counter/src/Models/GameQuery.php
getHost() resolves an allocation to a host: returns false if missing, prefers alias when player-counter.use_alias is enabled and valid, otherwise uses allocation IP and normalizes it before returning.
Query method and service integration
player-counter/src/Models/GameQuery.php
runQuery() calls getHost() and returns null if resolution fails. canRunQuery() returns whether getHost() resolves a valid host. QueryTypeService::process() is called with the resolved host instead of the prior raw IP.

🎯 4 (Complex) | ⏱️ ~45 minutes


A rabbit hops through host validation clear,
No more raw IPs without care,
Aliases and brackets dance in sync,
Normalized hosts now guide each ping,
Queries run tidy — hop, hop, hooray! 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding support for domain names and IP addresses with proper normalization in the player-counter module.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@player-counter/src/Models/GameQuery.php`:
- Around line 74-77: Remove the unnecessary outer parentheses around the return
expression in the method that checks addresses; change the return so it directly
returns the boolean expression using self::normaliseIpAddress($address) !==
false || filter_var($address, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME) !==
false without wrapping the whole expression in parentheses to satisfy the
no_unneeded_control_parentheses Pint rule.
- Around line 80-85: normaliseIpAddress currently calls
inet_ntop(inet_pton($address)) which throws a TypeError if inet_pton returns
false; change it to first call inet_pton($address), check if that result is
false and return false immediately, then pass the valid binary string to
inet_ntop and return its result; update the function normaliseIpAddress to
perform this two-step check to avoid passing false into inet_ntop.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5eca8be6-c717-4daf-956a-858c6c03f1f1

📥 Commits

Reviewing files that changed from the base of the PR and between 7c78640 and 827bdf7.

📒 Files selected for processing (1)
  • player-counter/src/Models/GameQuery.php
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Lint / 0_Pint.txt
player-counter/src/Models/GameQuery.php

[error] 1-1: PHP Pint (Laravel) style check failed: no_unneeded_control_parentheses. 238 files scanned, 1 style issue found.

🪛 GitHub Actions: Lint / Pint
player-counter/src/Models/GameQuery.php

[error] 1-1: PHP CS Fixer/Pint style check failed (Laravel Pint). Rule: no_unneeded_control_parentheses. 238 files checked, 1 style issue. Fix the parentheses usage in this file to satisfy the rule.

🔇 Additional comments (3)
player-counter/src/Models/GameQuery.php (3)

40-65: LGTM!


67-70: LGTM!


102-123: LGTM!

Comment thread player-counter/src/Models/GameQuery.php Outdated
Comment thread player-counter/src/Models/GameQuery.php
Minor refactor also to minimise duplicated code and to minimise
number of times the address is (de)serialized.
New methods marked as protected/private as there shouldn't be any
need to mess with these downstream.
@harryyoud
Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@harryyoud
Copy link
Copy Markdown
Author

@Boy132 This is now ready for human review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant