feat: add advanced IP/network APIs, optional DB storage, and comprehensive docs#3
Merged
feat: add advanced IP/network APIs, optional DB storage, and comprehensive docs#3
Conversation
Introduces IPType enum, TypeRegistry with IANA special-purpose CIDR data for IPv4 and IPv6, and type classification methods on IP: types(), primaryType(), is(), isGlobalRoutable(), isPrivate(), isLoopback(), isMulticast(), isLinkLocal(), isDocumentation(), isReserved().
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds extensive IPTools features: richer IP arithmetic and classifications, IPv4↔IPv6 conversions, a centralized Parser with flags, Range and RangeSet algebra, optional SQL-backed range storage with Laravel integration (provider, model, migration), comprehensive docs, CI DB integration, and broad unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Service as IPToolsServiceProvider
participant Container as Laravel Container
participant LRS as LaravelRangeStorage
participant SRS as SqlRangeStorage
participant DB as Database
App->>Service: register()/boot()
Service->>Container: bind LaravelRangeStorage singleton + alias RangeStorageInterface
App->>Container: resolve RangeStorageInterface
Container-->>App: LaravelRangeStorage
App->>LRS: store(Network|Range, metadata)
LRS->>SRS: storage() -> store(...)
SRS->>DB: INSERT(version, start_bin, end_bin, metadata)
DB-->>SRS: OK
SRS-->>LRS: success
LRS-->>App: done
App->>LRS: contains(IP)
LRS->>SRS: contains(IP)
SRS->>DB: SELECT ... WHERE version AND start_bin <= ip <= end_bin
DB-->>SRS: rows
SRS-->>LRS: bool
LRS-->>App: bool
App->>LRS: findContaining(IP)
LRS->>SRS: findContaining(IP)
SRS->>DB: SELECT ranges with metadata
DB-->>SRS: rows
SRS-->>LRS: iterable(range, metadata)
LRS-->>App: iterable
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
Introduces OverflowMode enum and arithmetic primitives on IP and Range: - IP: compareTo(), distanceTo(), addOffset(), next(), previous(), shift() - Range: addressAt(), addressAtOrFail() - next()/previous() return null at address-space boundaries (replacing wrapping behavior); OverflowMode controls addOffset()/shift() boundary handling (THROW/NULL/WRAP/CLAMP) - Updates internal iterator and subnet math in Network/Range to handle null boundary returns correctly
h3mantD
changed the title
Introduce IPv4-mapped, 6to4, and NAT64 /96 conversion APIs with coverage for round-trips and invalid inputs. Tighten long parsing bounds and exclude containment behavior to prevent silent wraparound and ambiguous edge cases.
Introduce flag-driven parsing for addresses, ranges, ports, zone IDs, and non-decimal IPv4 inputs using Parser, ParseFlags, and ParsedAddress. Keep parsing deterministic with strict mode and wildcard-to-network promotion, backed by focused tests and README examples.
Introduce SQL-backed range storage primitives and Laravel-friendly adapters while keeping table migration optional. Document dependency behavior and setup paths clearly so non-Laravel users remain unaffected and Laravel users get publishable config, migration, and model stubs.
Introduce a canonical RangeSet abstraction with union, intersection, subtraction, overlap, containment, and CIDR decomposition across IPv4 and IPv6. Document the API with examples and add focused tests for normalization, mixed-version isolation, and split/merge behavior.
Add host-boundary, containment, point-to-point, and subnet navigation helpers while preserving existing network semantics across IPv4 and IPv6. Expand tests and README examples to document edge behavior for /31,/32,/127,/128 style scenarios.
Replace README RFC-tagged headings with neutral section titles and introduce a comprehensive docs directory covering IP, parser, network, range, rangeset, storage, Laravel integration, and API reference. Add index navigation and cross-links to make the documentation easier to browse.
Add clickable navigation links from README to each detailed docs page, convert the docs index to linked entries, and add back-to-index links in all guide files for easier browsing.
Refactor the docs into a more structured, concept-first format with clearer usage flows, API semantics, behavior notes, and troubleshooting guidance across IP, parser, network, range, rangeset, storage, and Laravel integration.
h3mantD
changed the title
Introduce engine-backed integration tests for SqlRangeStorage and wire a dedicated CI job with MySQL and PostgreSQL services. This closes the portability gap between documented SQL support and runtime-validated behavior.
Update checkout and setup-php action versions and opt into Node 24 execution to avoid Node 20 deprecation impact on CI runners.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
database/migrations/create_ip_ranges_table.php.stub (1)
12-20: Consider adding timestamps for audit purposes.The table lacks
created_at/updated_atcolumns. While this may be intentional for a pure lookup table, timestamps can be valuable for debugging and auditing when ranges were added or modified.💡 Optional: Add timestamps
$table->json('metadata')->nullable(); + $table->timestamps(); $table->index(['version', 'start_bin', 'end_bin'], 'ip_ranges_lookup_idx');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/create_ip_ranges_table.php.stub` around lines 12 - 20, The migration for the ip_ranges table is missing timestamps; update the Schema::create callback that defines the ip_ranges table (the closure creating columns like id, version, start_bin, end_bin, metadata and the index 'ip_ranges_lookup_idx') to add Laravel timestamp columns (e.g., call the table's timestamps method) so created_at/updated_at are recorded for each range entry for auditing and debugging.tests/RangeSetTest.php (1)
62-75: Assert that the unaffected IPv4 tail still survives subtraction.This test still passes if
subtract('10.0.0.0/25')removes the whole IPv4 bucket instead of only the lower half. One positive assertion in10.0.0.128/25would lock the intended behavior down.Suggested test addition
$subtracted = $set->subtract('10.0.0.0/25'); $this->assertFalse($subtracted->contains(new IP('10.0.0.42'))); + $this->assertTrue($subtracted->contains(new IP('10.0.0.200'))); $this->assertTrue($subtracted->contains(new IP('2001:db8::2')));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/RangeSetTest.php` around lines 62 - 75, Update the test_mixed_versions_are_isolated test in RangeSetTest to assert that the unaffected IPv4 upper half survives subtraction: after creating RangeSet([...]) and calling $subtracted = $set->subtract('10.0.0.0/25'), add an assertion that $subtracted->contains(new IP('10.0.0.128')) (or any address inside 10.0.0.128/25) is true so the subtract method on RangeSet only removes the lower /25 and leaves 10.0.0.128/25 intact; reference RangeSet::subtract and the test method name to locate where to insert the extra positive assertion.src/ParseFlags.php (1)
9-24:STRICTreads like a flag, but0is really “no flags”.Because it disappears inside
STRICT | ALLOW_*expressions, callers can think they are requesting strict mode while actually just enabling optional parsing. A clearer alias or docblock would make this new API much harder to misuse.Possible low-risk tweak
- public const STRICT = 0; + /** Zero-value preset: use alone rather than composing with ALLOW_* flags. */ + public const STRICT = 0; + public const NONE = self::STRICT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ParseFlags.php` around lines 9 - 24, The constant STRICT is misleading because it is set to 0 (meaning "no flags"); rename or add a clearer alias such as NONE or NO_FLAGS on the ParseFlags class and update references so DEFAULT and any bitwise expressions use the new alias instead of STRICT (leave STRICT as a deprecated alias or update its docblock to indicate it's an alias for NONE), and add a short docblock to the new constant explaining that 0 means "no flags" to prevent callers from mistaking it for strict mode; ensure code using ParseFlags::STRICT still works if you keep it as an alias or update call sites to use ParseFlags::NONE.tests/ParserTest.php (1)
47-51: Split the strict-mode port and wildcard checks.This test exits on the port exception, so the wildcard behavior named in the method never runs. A second assertion around
Parser::range('192.168.*.*', ParseFlags::STRICT)would cover the remaining strict-mode branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ParserTest.php` around lines 47 - 51, The test method test_strict_mode_disables_port_and_wildcards currently only asserts that Parser::ip('1.2.3.4:80', ParseFlags::STRICT) throws, so the wildcard branch isn't exercised; update the test to split the checks by keeping the existing expectException for the port case (Parser::ip) and then add a second expectException and call asserting Parser::range('192.168.*.*', ParseFlags::STRICT) also throws, so both strict-mode port and wildcard behaviors (Parser::ip and Parser::range with ParseFlags::STRICT) are covered.src/Storage/SqlRangeStorage.php (2)
131-138: Minor:count()could return incorrect result on query failure.If
$this->pdo->query()returnsfalse(e.g., on a syntax error not caught by exception), the method silently returns0instead of signaling an error. This might mask database connectivity issues.🔧 Consider throwing on query failure for consistency with other methods
public function count(): int { $this->ensureTableExists(); $statement = $this->pdo->query(sprintf('SELECT COUNT(*) FROM %s', $this->table)); - $count = $statement === false ? false : $statement->fetchColumn(); - - return $count === false ? 0 : (int) $count; + if ($statement === false) { + throw new RuntimeException('Failed to execute count query'); + } + $count = $statement->fetchColumn(); + + return $count === false ? 0 : (int) $count; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Storage/SqlRangeStorage.php` around lines 131 - 138, The count() method currently treats a false return from $this->pdo->query(sprintf('SELECT COUNT(*) FROM %s', $this->table)) as a silent zero; instead, detect a failed query and throw an exception so failures aren't masked—modify SqlRangeStorage::count() to call $this->ensureTableExists(), run the query, and if $statement === false throw a RuntimeException (or PDOException) with a clear message including $this->table and the PDO error info, otherwise fetchColumn and return the int cast.
69-87: Potential edge case: duplicate:addr_binparameter binding.The SQL query uses
:addr_binin two places (start_bin <= :addr_bin AND end_bin >= :addr_bin), butbindValueis only called once. While PDO typically handles this correctly by reusing the bound value, some database drivers may not support this behavior consistently.♻️ Consider using distinct parameter names for clarity and portability
$statement = $this->pdo->prepare( sprintf( - 'SELECT 1 FROM %s WHERE version = :version AND start_bin <= :addr_bin AND end_bin >= :addr_bin LIMIT 1', + 'SELECT 1 FROM %s WHERE version = :version AND start_bin <= :addr_start AND end_bin >= :addr_end LIMIT 1', $this->table ) ); $statement->bindValue(':version', $version, PDO::PARAM_INT); - $statement->bindValue(':addr_bin', $encoded, PDO::PARAM_LOB); + $statement->bindValue(':addr_start', $encoded, PDO::PARAM_LOB); + $statement->bindValue(':addr_end', $encoded, PDO::PARAM_LOB); $statement->execute();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Storage/SqlRangeStorage.php` around lines 69 - 87, The contains() method uses the same SQL named parameter :addr_bin twice which may be unsupported by some PDO drivers; update the query in contains() to use two distinct parameters (e.g., :addr_bin_start and :addr_bin_end), bind both values via $statement->bindValue (or bindParam) with AddressCodec::to16($ip) and PDO::PARAM_LOB, then execute and return the same fetch logic so both comparisons use explicitly bound parameters.src/Parser.php (1)
130-139: Edge case: Non-quad IPv4 parsing allows negative integers after casting.The condition
(int) $first < 0will never be true because$firstmatched/^\d+$/, meaning it's always non-negative. However, very large numeric strings (e.g.,999999999999999999999) when cast to(int)may produce unexpected values due to integer overflow. Consider usingfilter_varor explicit numeric comparison.🛡️ Use numeric comparison instead of int casting
if (self::hasFlag($flags, ParseFlags::ALLOW_NON_QUAD_IPV4) && preg_match('/^\d+\.\d+$/', $address) === 1 ) { [$first, $last] = explode('.', $address, 2); - if ((int) $first < 0 || (int) $first > 255 || (int) $last < 0 || (int) $last > 255) { + if (!ctype_digit($first) || !ctype_digit($last) + || (int) $first > 255 || (int) $last > 255) { throw new IpException('Invalid non-quad IPv4 format'); } return new IP(sprintf('%d.0.0.%d', (int) $first, (int) $last)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Parser.php` around lines 130 - 139, The current non-quad IPv4 branch casts $first/$last to (int) and then compares, which can hide overflowed/invalid numeric strings; instead validate each part with FILTER_VALIDATE_INT and range options (min_range=0, max_range=255) before casting. Update the block guarded by ParseFlags::ALLOW_NON_QUAD_IPV4 to use filter_var($first, FILTER_VALIDATE_INT, ['options'=>['min_range'=>0,'max_range'=>255]]) and the same for $last, throw the IpException if validation fails, and only then cast to int for sprintf('%d.0.0.%d', (int)$first, (int)$last) when constructing the new IP instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@database/migrations/create_ip_ranges_table.php.stub`:
- Around line 15-16: The migration currently uses $table->binary for the
start_bin and end_bin columns which creates BLOBs; change both definitions for
columns start_bin and end_bin to use a fixed-length CHAR(16) with the binary
charset (i.e., char length 16 and ->charset('binary')) so the DB column becomes
BINARY(16) for efficient fixed-length storage and range queries; update the
create_ip_ranges_table migration where start_bin and end_bin are defined
accordingly.
In `@docs/ip.md`:
- Around line 140-147: The example showing safe iteration using IP->next() is
incorrect because next() will throw on overflow unless called with
OverflowMode::NULL; update the doc snippet to import and use OverflowMode and
call $cursor = $cursor->next(OverflowMode::NULL) in the loop so iteration
terminates with null instead of throwing (refer to the IP class and its next()
method and the OverflowMode::NULL symbol when making the change).
In `@docs/laravel.md`:
- Around line 95-99: The snippet using LaravelRangeStorage calls
DB::connection() but omits importing the DB facade; add the import statement use
Illuminate\Support\Facades\DB; to the example so DB::connection() resolves
correctly when using the LaravelRangeStorage class.
In `@docs/range.md`:
- Around line 27-33: The example uses $r1 from elsewhere, causing an undefined
variable when copied; update the snippet to first create a Range (e.g., call
Range::parse with '192.168.1.0-192.168.1.255' or similar) and then call the
contains method on that instance (contains on the Range object, using IPTools\IP
for the tested address), so the snippet is self-contained and references
Range::parse, $r1, contains and IPTools\IP.
In `@docs/rangeset.md`:
- Around line 33-55: The docs currently over-generalize the API: update the
paragraph to state that union, intersect, and subtract accept combinations of
RangeSet, iterable of items, Range, Network, IP, or parseable string and return
a new normalized RangeSet (reference union, intersect, subtract); explicitly
state that contains(IP $ip), containsRange(...), and overlaps(...) are predicate
methods that return bool; and state that toCidrs() returns Network[] and
getRanges() returns Range[] (reference to contains, containsRange, overlaps,
toCidrs, getRanges) so the contract is precise and aligned with the public API.
In `@src/IPToolsServiceProvider.php`:
- Around line 57-59: In IPToolsServiceProvider (the service provider that
currently calls $this->publishes([...], 'iptools-migrations')), replace that
publishes(...) invocation with Laravel's publishesMigrations helper so package
migration timestamps are handled correctly; locate the boot method in
IPToolsServiceProvider and change the call that references
__DIR__.'/../database/migrations/create_ip_ranges_table.php.stub' to use
$this->publishesMigrations(...) (or the publishesMigrations API that accepts the
migration stub path) instead of $this->publishes(...), keeping the same
migration stub reference and tag name semantics.
In `@src/Models/IpRange.php`:
- Around line 11-12: The IpRange model currently hardcodes the table name in the
protected $table property which can diverge from the storage adapter that reads
iptools.storage.table (or IPTOOLS_RANGES_TABLE); update the IpRange model so
$table is set from the configuration at runtime (e.g., read
config('iptools.storage.table') with an appropriate fallback to the old
'ip_ranges' name) — you can assign this in the property declaration using the
config() call or set it in the model's constructor/boot method so the model and
the adapter always use the same configurable table name.
In `@src/ParsedAddress.php`:
- Around line 9-13: The constructor on class ParsedAddress (__construct)
currently accepts any ?int for the public property port; add validation at the
start of __construct to reject out-of-range ports (e.g. allow null or 0..65535)
and throw a clear InvalidArgumentException for invalid values before assigning
to the public port property so impossible ports cannot be exposed as object
state; keep the signature but perform the check (if $port !== null && ($port < 0
|| $port > 65535)) throw new \InvalidArgumentException('port must be null or
between 0 and 65535').
In `@src/Parser.php`:
- Around line 220-232: The parsePort() function risks integer overflow when
casting very long digit strings; update it to reject strings longer than 5
digits before casting: in Parser::parsePort add a check (e.g. if (strlen($port)
> 5) throw new IpException('Port must be in the range 0-65535')) so only strings
of length <=5 are cast to int, then keep the existing numeric range check and
exceptions; reference the parsePort method and IpException class when making
this change.
In `@src/Range.php`:
- Around line 209-229: In addressAt(), remove the ineffective "@var" annotation
and validate the incoming $offset is a pure integer string (optional leading '-'
followed by digits) and throw an InvalidArgumentException on invalid input; then
always call bcadd/bccomp with scale 0 (e.g. bcadd($a,$b,0), bccomp($a,$b,0))
when computing $result and comparisons, using getFirstIP()->toLong(),
getLastIP()->toLong() and getFirstIP()->getVersion() as before, and pass the
resulting integer string to IP::parseLong($result, $version); ensure negative
offsets are handled by your validation logic and avoid any operations that could
produce decimal strings.
In `@src/Storage/LaravelRangeStorage.php`:
- Around line 32-47: The contains(), findContaining(), and count() methods
currently call storage() which builds a SqlRangeStorage from
Connection::getPdo() (write PDO) and so bypasses Laravel read/write splitting;
update these read-only methods to use the read connection by creating the
SqlRangeStorage from Connection::getReadPdo() (or add a read flag to storage()
so it returns a SqlRangeStorage backed by getReadPdo()) so that contains(),
findContaining(), and count() route queries to replicas rather than the primary.
In `@src/TypeRegistry.php`:
- Around line 80-84: The entry mapping the whole '192.0.0.0/24' to
IPType::RESERVED in TypeRegistry.php is too coarse and incorrectly marks
192.0.0.9 and 192.0.0.10 as reserved; instead, replace the single '/24' entry
with the specific sub-blocks IANA lists (retain other entries) and add explicit
entries that mark 192.0.0.9/32 and 192.0.0.10/32 as globally routable (not
IPType::RESERVED) so primaryType() and isGlobalRoutable() return correct values;
update the registry population where the array with IPType::RESERVED is built
and add a small regression test asserting that
primaryType('192.0.0.9')/isGlobalRoutable() and
primaryType('192.0.0.10')/isGlobalRoutable() reflect global reachability.
In `@stubs/IpRange.php.stub`:
- Around line 9-18: The IpRange Eloquent model class (IpRange) lacks disabling
of automatic timestamps, causing missing created_at/updated_at DB errors; update
the class to set the public/protected $timestamps property to false (i.e., add
"$timestamps = false" on the IpRange model) so Eloquent stops expecting those
columns and create/update operations succeed.
---
Nitpick comments:
In `@database/migrations/create_ip_ranges_table.php.stub`:
- Around line 12-20: The migration for the ip_ranges table is missing
timestamps; update the Schema::create callback that defines the ip_ranges table
(the closure creating columns like id, version, start_bin, end_bin, metadata and
the index 'ip_ranges_lookup_idx') to add Laravel timestamp columns (e.g., call
the table's timestamps method) so created_at/updated_at are recorded for each
range entry for auditing and debugging.
In `@src/ParseFlags.php`:
- Around line 9-24: The constant STRICT is misleading because it is set to 0
(meaning "no flags"); rename or add a clearer alias such as NONE or NO_FLAGS on
the ParseFlags class and update references so DEFAULT and any bitwise
expressions use the new alias instead of STRICT (leave STRICT as a deprecated
alias or update its docblock to indicate it's an alias for NONE), and add a
short docblock to the new constant explaining that 0 means "no flags" to prevent
callers from mistaking it for strict mode; ensure code using ParseFlags::STRICT
still works if you keep it as an alias or update call sites to use
ParseFlags::NONE.
In `@src/Parser.php`:
- Around line 130-139: The current non-quad IPv4 branch casts $first/$last to
(int) and then compares, which can hide overflowed/invalid numeric strings;
instead validate each part with FILTER_VALIDATE_INT and range options
(min_range=0, max_range=255) before casting. Update the block guarded by
ParseFlags::ALLOW_NON_QUAD_IPV4 to use filter_var($first, FILTER_VALIDATE_INT,
['options'=>['min_range'=>0,'max_range'=>255]]) and the same for $last, throw
the IpException if validation fails, and only then cast to int for
sprintf('%d.0.0.%d', (int)$first, (int)$last) when constructing the new IP
instance.
In `@src/Storage/SqlRangeStorage.php`:
- Around line 131-138: The count() method currently treats a false return from
$this->pdo->query(sprintf('SELECT COUNT(*) FROM %s', $this->table)) as a silent
zero; instead, detect a failed query and throw an exception so failures aren't
masked—modify SqlRangeStorage::count() to call $this->ensureTableExists(), run
the query, and if $statement === false throw a RuntimeException (or
PDOException) with a clear message including $this->table and the PDO error
info, otherwise fetchColumn and return the int cast.
- Around line 69-87: The contains() method uses the same SQL named parameter
:addr_bin twice which may be unsupported by some PDO drivers; update the query
in contains() to use two distinct parameters (e.g., :addr_bin_start and
:addr_bin_end), bind both values via $statement->bindValue (or bindParam) with
AddressCodec::to16($ip) and PDO::PARAM_LOB, then execute and return the same
fetch logic so both comparisons use explicitly bound parameters.
In `@tests/ParserTest.php`:
- Around line 47-51: The test method
test_strict_mode_disables_port_and_wildcards currently only asserts that
Parser::ip('1.2.3.4:80', ParseFlags::STRICT) throws, so the wildcard branch
isn't exercised; update the test to split the checks by keeping the existing
expectException for the port case (Parser::ip) and then add a second
expectException and call asserting Parser::range('192.168.*.*',
ParseFlags::STRICT) also throws, so both strict-mode port and wildcard behaviors
(Parser::ip and Parser::range with ParseFlags::STRICT) are covered.
In `@tests/RangeSetTest.php`:
- Around line 62-75: Update the test_mixed_versions_are_isolated test in
RangeSetTest to assert that the unaffected IPv4 upper half survives subtraction:
after creating RangeSet([...]) and calling $subtracted =
$set->subtract('10.0.0.0/25'), add an assertion that $subtracted->contains(new
IP('10.0.0.128')) (or any address inside 10.0.0.128/25) is true so the subtract
method on RangeSet only removes the lower /25 and leaves 10.0.0.128/25 intact;
reference RangeSet::subtract and the test method name to locate where to insert
the extra positive assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d9d9198-d698-494d-b4b6-66b839a47ac5
📒 Files selected for processing (38)
README.mdcomposer.jsonconfig/iptools.phpdatabase/migrations/create_ip_ranges_table.php.stubdocs/README.mddocs/api-reference.mddocs/ip.mddocs/laravel.mddocs/network.mddocs/parser.mddocs/range.mddocs/rangeset.mddocs/storage.mdsrc/IP.phpsrc/IPToolsServiceProvider.phpsrc/IPType.phpsrc/Models/IpRange.phpsrc/Network.phpsrc/OverflowMode.phpsrc/ParseFlags.phpsrc/ParsedAddress.phpsrc/Parser.phpsrc/Range.phpsrc/RangeSet.phpsrc/Storage/AddressCodec.phpsrc/Storage/LaravelRangeStorage.phpsrc/Storage/RangeStorageInterface.phpsrc/Storage/SqlRangeStorage.phpsrc/TypeRegistry.phpstubs/IpRange.php.stubtests/IPArithmeticTest.phptests/IPConversionTest.phptests/IPTest.phptests/IPTypeTest.phptests/NetworkTest.phptests/ParserTest.phptests/RangeSetTest.phptests/SqlRangeStorageTest.php
Comment on lines
+15
to
+16
| $table->binary('start_bin', 16); | ||
| $table->binary('end_bin', 16); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Laravel Schema binary column fixed length
💡 Result:
- In Laravel migrations,
Schema::table(..., fn (Blueprint $table) => $table->binary('col'))creates a BLOB (notBINARY(n)/ fixed-length). [1] - For a fixed-length binary value in MySQL/MariaDB (e.g., 16 bytes for hashes/UUID storage), a common approach is:
[2]
$table->char('col', 16)->charset('binary'); // results in BINARY(16) in MySQL tools
- If you’re on newer Laravel versions,
Blueprint::binary(string $column, ?int $length = null, bool $fixed = false)exists (but is marked deprecated in the API docs), so relying on it long-term may be risky. [3] - Most portable fallback (any version): run raw SQL in the migration:
[2]
DB::statement('ALTER TABLE your_table ADD col BINARY(16)');
Sources:
[1] Laravel migrations docs (binary() => BLOB) (laravel.com)
[2] Stack Overflow workaround (char(...)->charset('binary')) (stackoverflow.com)
[3] Laravel 12 Blueprint API signature / deprecation note (api.laravel.com)
Citations:
- 1: https://laravel.com/docs/10.x/migrations?utm_source=openai
- 2: https://stackoverflow.com/questions/49389233/laravel-schema-builder-creating-a-binary16-column?utm_source=openai
- 3: https://api.laravel.com/docs/12.x/Illuminate/Database/Schema/Blueprint.html
Replace binary(16) with char(16)->charset('binary') for fixed-length storage.
Laravel's binary() method creates a BLOB column, not a fixed-length BINARY(16). Since you're storing 16-byte IP addresses, use:
$table->char('start_bin', 16)->charset('binary');
$table->char('end_bin', 16)->charset('binary');This produces the correct BINARY(16) type in MySQL/MariaDB for efficient range lookups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@database/migrations/create_ip_ranges_table.php.stub` around lines 15 - 16,
The migration currently uses $table->binary for the start_bin and end_bin
columns which creates BLOBs; change both definitions for columns start_bin and
end_bin to use a fixed-length CHAR(16) with the binary charset (i.e., char
length 16 and ->charset('binary')) so the DB column becomes BINARY(16) for
efficient fixed-length storage and range queries; update the
create_ip_ranges_table migration where start_bin and end_bin are defined
accordingly.
Comment on lines
+140
to
+147
| ### Iterate safely without exceptions | ||
|
|
||
| ```php | ||
| $cursor = new IP('10.0.0.1'); | ||
| while ($cursor !== null) { | ||
| // process | ||
| $cursor = $cursor->next(); | ||
| } |
There was a problem hiding this comment.
Use OverflowMode::NULL in the “safe iteration” example.
next() is only exception-safe if overflow returns null at the upper boundary. The current loop will throw on the last address instead of terminating cleanly.
Suggested doc fix
```php
+use IPTools\OverflowMode;
+
$cursor = new IP('10.0.0.1');
while ($cursor !== null) {
// process
- $cursor = $cursor->next();
+ $cursor = $cursor->next(OverflowMode::NULL);
}</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/ip.md around lines 140 - 147, The example showing safe iteration using
IP->next() is incorrect because next() will throw on overflow unless called with
OverflowMode::NULL; update the doc snippet to import and use OverflowMode and
call $cursor = $cursor->next(OverflowMode::NULL) in the loop so iteration
terminates with null instead of throwing (refer to the IP class and its next()
method and the OverflowMode::NULL symbol when making the change).
</details>
<!-- fingerprinting:phantom:poseidon:grasshopper -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+27
to
+33
| ## Containment | ||
|
|
||
| `contains` accepts `IP|Range|Network`. | ||
|
|
||
| ```php | ||
| var_dump($r1->contains(new IPTools\IP('192.168.1.10'))); | ||
| ``` |
There was a problem hiding this comment.
Make the containment example self-contained.
$r1 is introduced in a different block, so readers copying this section in isolation get an undefined variable. Recreate the range inside the snippet.
Suggested doc fix
```php
+use IPTools\Range;
+
+$r1 = Range::parse('192.168.1.0-192.168.1.255');
var_dump($r1->contains(new IPTools\IP('192.168.1.10')));</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/range.md around lines 27 - 33, The example uses $r1 from elsewhere,
causing an undefined variable when copied; update the snippet to first create a
Range (e.g., call Range::parse with '192.168.1.0-192.168.1.255' or similar) and
then call the contains method on that instance (contains on the Range object,
using IPTools\IP for the tested address), so the snippet is self-contained and
references Range::parse, $r1, contains and IPTools\IP.
</details>
<!-- fingerprinting:phantom:poseidon:grasshopper -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+33
to
+55
| Each operation accepts combinations of: | ||
|
|
||
| - `RangeSet` | ||
| - `iterable` of items | ||
| - `Range` | ||
| - `Network` | ||
| - `IP` | ||
| - `string` parsable by `Range::parse` | ||
|
|
||
| This lets you combine strongly typed and raw user/config inputs in one operation. | ||
|
|
||
| ## Operations | ||
|
|
||
| - `union(...)`: combine sets and normalize | ||
| - `intersect(...)`: keep only overlaps | ||
| - `subtract(...)`: remove portions from the set | ||
| - `contains(IP $ip): bool` | ||
| - `containsRange(...): bool` | ||
| - `overlaps(...): bool` | ||
| - `toCidrs(): Network[]` | ||
| - `getRanges(): Range[]` | ||
|
|
||
| All operations return a new normalized `RangeSet`. |
There was a problem hiding this comment.
Tighten the API contract in this section.
This wording currently implies every operation accepts mixed inputs and returns a RangeSet, but that is not true for contains*()/overlaps() or for toCidrs()/getRanges(). Narrowing the text here will keep the guide aligned with the public API.
Suggested wording
-Each operation accepts combinations of:
+Construction and multi-range operations accept combinations of:
- `RangeSet`
- `iterable` of items
- `Range`
- `Network`
- `IP`
- `string` parsable by `Range::parse`
This lets you combine strongly typed and raw user/config inputs in one operation.
@@
-All operations return a new normalized `RangeSet`.
+`union()`, `intersect()`, and `subtract()` return a new normalized `RangeSet`.
+Query helpers like `contains*()`/`overlaps()` return `bool`, and export helpers return derived `Network[]`/`Range[]`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/rangeset.md` around lines 33 - 55, The docs currently over-generalize
the API: update the paragraph to state that union, intersect, and subtract
accept combinations of RangeSet, iterable of items, Range, Network, IP, or
parseable string and return a new normalized RangeSet (reference union,
intersect, subtract); explicitly state that contains(IP $ip),
containsRange(...), and overlaps(...) are predicate methods that return bool;
and state that toCidrs() returns Network[] and getRanges() returns Range[]
(reference to contains, containsRange, overlaps, toCidrs, getRanges) so the
contract is precise and aligned with the public API.
Comment on lines
+27
to
+29
| if ($version === 4) { | ||
| return IP::parseInAddr(substr($bin, 12, 4)); | ||
| } |
There was a problem hiding this comment.
Validate the IPv4 zero-prefix when decoding.
For version 4, this currently ignores the first 12 bytes. A malformed row can therefore decode as a valid IPv4 instead of failing fast, which makes storage corruption much harder to spot.
Suggested fix
if ($version === 4) {
+ if (substr($bin, 0, 12) !== str_repeat("\x00", 12)) {
+ throw new InvalidArgumentException('IPv4 binary value must use the canonical 12-byte zero prefix');
+ }
return IP::parseInAddr(substr($bin, 12, 4));
}📝 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.
Suggested change
| if ($version === 4) { | |
| return IP::parseInAddr(substr($bin, 12, 4)); | |
| } | |
| if ($version === 4) { | |
| if (substr($bin, 0, 12) !== str_repeat("\x00", 12)) { | |
| throw new InvalidArgumentException('IPv4 binary value must use the canonical 12-byte zero prefix'); | |
| } | |
| return IP::parseInAddr(substr($bin, 12, 4)); | |
| } |
Comment on lines
+32
to
+47
| public function contains(IP $ip): bool | ||
| { | ||
| return $this->storage()->contains($ip); | ||
| } | ||
|
|
||
| /** | ||
| * @return iterable<array{range: Network|Range, metadata: array<string, mixed>}> | ||
| */ | ||
| public function findContaining(IP $ip): iterable | ||
| { | ||
| return $this->storage()->findContaining($ip); | ||
| } | ||
|
|
||
| public function count(): int | ||
| { | ||
| return $this->storage()->count(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the current Illuminate\Database\Connection API, what is the difference between getPdo() and getReadPdo(), and do read-only queries issued through getPdo() bypass Laravel read/write splitting or replica selection?
💡 Result:
getPdo()returns the connection’s “write” PDO (the primary PDO stored in$this->pdo, lazily resolved if it’s aClosure). It also records that the latest PDO type retrieved waswrite. [1]getReadPdo()returns the connection’s “read” PDO (from$this->readPdo, lazily resolved if it’s aClosure) unless Laravel decides reads must go to the writer—specifically:- if you’re in a transaction (
$this->transactions > 0), it returnsgetPdo()[1] - if
useWriteConnectionWhenReading()was enabled ($readOnWriteConnection), it returnsgetPdo()[1] - if
stickyis enabled and the connection has recorded writes this request ($recordsModified && sticky), it returnsgetPdo()[1][2] - if no read PDO exists, it falls back to
getPdo()[1]
- if you’re in a transaction (
Do read-only queries issued through getPdo() bypass read/write splitting / replica selection?
Yes. If you run a SELECT via the PDO you got from getPdo(), you are explicitly using the write connection, so you bypass Laravel’s normal “read PDO” path (and therefore bypass read-host / replica selection that would occur when Laravel uses getReadPdo() for reads). [1][2]
Sources
[1] Laravel framework Illuminate\Database\Connection (12.x) source (getPdo, getReadPdo) (github.com)
[2] Laravel 12.x docs: “Read and Write Connections” + sticky behavior + random read host selection (laravel.com)
Citations:
- 1: https://github.com/laravel/framework/raw/refs/heads/12.x/src/Illuminate/Database/Connection.php
- 2: https://laravel.com/docs/12.x/database
Route read-only calls through Laravel's read connection.
Methods contains(), findContaining(), and count() delegate to SqlRangeStorage created from Connection::getPdo(), which returns the write PDO. This causes read-only lookups to bypass Laravel's read/write splitting and replica selection, sending all traffic to the primary connection. These methods should use getReadPdo() or separate read and write handles before release.
Also applies to: 50-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Storage/LaravelRangeStorage.php` around lines 32 - 47, The contains(),
findContaining(), and count() methods currently call storage() which builds a
SqlRangeStorage from Connection::getPdo() (write PDO) and so bypasses Laravel
read/write splitting; update these read-only methods to use the read connection
by creating the SqlRangeStorage from Connection::getReadPdo() (or add a read
flag to storage() so it returns a SqlRangeStorage backed by getReadPdo()) so
that contains(), findContaining(), and count() route queries to replicas rather
than the primary.
Normalize binary/text columns returned as PDO streams before decoding in SqlRangeStorage::findContaining. This fixes PostgreSQL integration failures where BYTEA values are delivered as resources instead of strings.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/SqlRangeStorageIntegrationTest.php (4)
91-102: Empty password is treated as unset.If
IPTOOLS_MYSQL_PASSWORDorIPTOOLS_PGSQL_PASSWORDis explicitly set to an empty string (e.g., for passwordless auth), the helper returns the default password instead. This may not be the intended behavior.♻️ Handle empty password explicitly
private function enginePass(string $engine): string { if ($engine === self::MYSQL) { $pass = getenv('IPTOOLS_MYSQL_PASSWORD'); - return is_string($pass) ? $pass : 'root'; + return is_string($pass) && $pass !== '' ? $pass : 'root'; } $pass = getenv('IPTOOLS_PGSQL_PASSWORD'); - return is_string($pass) ? $pass : 'postgres'; + return is_string($pass) && $pass !== '' ? $pass : 'postgres'; }Alternatively, if you need to support empty passwords, use a different check:
return getenv('IPTOOLS_MYSQL_PASSWORD') !== false ? (string) getenv('IPTOOLS_MYSQL_PASSWORD') : 'root';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SqlRangeStorageIntegrationTest.php` around lines 91 - 102, The enginePass helper currently treats an explicitly set empty password as unset because it uses is_string(getenv(...)); change the getenv checks in enginePass to test against false (e.g., getenv('IPTOOLS_MYSQL_PASSWORD') !== false) so an empty string is accepted as a valid value; return (string) getenv(...) when the env var is set, otherwise fall back to the defaults ('root' for MYSQL, 'postgres' for PGSQL). Ensure you update both branches that reference IPTOOLS_MYSQL_PASSWORD and IPTOOLS_PGSQL_PASSWORD and keep the function signature enginePass(string $engine) intact.
35-38: Consider asserting the returned range object.The test verifies metadata but doesn't assert that
$matches[0]['range']is correct. Adding an assertion for the range would improve test coverage and catch potential serialization issues.💡 Suggested additional assertion
$matches = iterator_to_array($storage->findContaining(new IP('10.10.1.10')), false); $this->assertCount(1, $matches); + $this->assertInstanceOf(Range::class, $matches[0]['range']); + $this->assertSame('10.10.1.0', (string) $matches[0]['range']->getFirstIP()); + $this->assertSame('10.10.1.15', (string) $matches[0]['range']->getLastIP()); $this->assertSame(self::MYSQL, $matches[0]['metadata']['engine'] ?? null); $this->assertSame(20, $matches[0]['metadata']['priority'] ?? null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SqlRangeStorageIntegrationTest.php` around lines 35 - 38, The test currently checks metadata but not the actual returned range; update the assertion after calling findContaining (and after $matches is built) to assert that $matches[0]['range'] contains the expected Range object/value. Specifically, verify the 'range' key exists on $matches[0], assert it is the correct type (e.g., Range or whatever domain class is used) and equals the expected range used in the test fixture (compare value/equality, not just string), referencing the findContaining call, the IP instantiation, and the $matches array to locate where to add the assertion.
1-11: Consider adding a namespace declaration.The test class lacks a namespace while the source classes under test (e.g.,
IPTools\Storage\SqlRangeStorage) are namespaced. Adding aTestsnamespace would align with PSR-4 autoloading conventions and prevent potential class name collisions.♻️ Suggested namespace addition
<?php declare(strict_types=1); +namespace Tests; + use IPTools\IP; use IPTools\Network; use IPTools\Range;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SqlRangeStorageIntegrationTest.php` around lines 1 - 11, The test class SqlRangeStorageIntegrationTest is declared without a namespace which can break PSR-4 autoloading and collide with other classes; add an appropriate namespace declaration (for example namespace Tests or namespace Tests\Integration) at the top of the file, update the class declaration to remain final class SqlRangeStorageIntegrationTest, and ensure any uses (IPTools\IP, IPTools\Network, IPTools\Range, IPTools\Storage\SqlRangeStorage) still resolve correctly (adjust use statements if you move the file into a sub-namespace).
104-115: PDO connection failure will throw instead of skip.If the DSN is configured but the database is unreachable or credentials are invalid, the
PDOconstructor throws aPDOException. This causes a test failure rather than a skip, which may be undesirable in CI environments where the database might be temporarily unavailable.🛡️ Wrap connection in try-catch for graceful skip
private function connect(string $engine): ?PDO { $dsn = $this->engineDsn($engine); if ($dsn === null) { return null; } - $pdo = new PDO($dsn, $this->engineUser($engine), $this->enginePass($engine)); - $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); - - return $pdo; + try { + $pdo = new PDO($dsn, $this->engineUser($engine), $this->enginePass($engine)); + $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + + return $pdo; + } catch (\PDOException) { + return null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/SqlRangeStorageIntegrationTest.php` around lines 104 - 115, The connect method currently constructs a new PDO which will throw a PDOException on network/auth failures and break tests; wrap the PDO creation and setAttribute calls in a try-catch that catches PDOException, call $this->markTestSkipped with a brief message including the exception message (so CI skips the test instead of failing), and return null to preserve the ?PDO signature; reference the connect method, the PDO constructor, and PDOException in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/SqlRangeStorageIntegrationTest.php`:
- Around line 91-102: The enginePass helper currently treats an explicitly set
empty password as unset because it uses is_string(getenv(...)); change the
getenv checks in enginePass to test against false (e.g.,
getenv('IPTOOLS_MYSQL_PASSWORD') !== false) so an empty string is accepted as a
valid value; return (string) getenv(...) when the env var is set, otherwise fall
back to the defaults ('root' for MYSQL, 'postgres' for PGSQL). Ensure you update
both branches that reference IPTOOLS_MYSQL_PASSWORD and IPTOOLS_PGSQL_PASSWORD
and keep the function signature enginePass(string $engine) intact.
- Around line 35-38: The test currently checks metadata but not the actual
returned range; update the assertion after calling findContaining (and after
$matches is built) to assert that $matches[0]['range'] contains the expected
Range object/value. Specifically, verify the 'range' key exists on $matches[0],
assert it is the correct type (e.g., Range or whatever domain class is used) and
equals the expected range used in the test fixture (compare value/equality, not
just string), referencing the findContaining call, the IP instantiation, and the
$matches array to locate where to add the assertion.
- Around line 1-11: The test class SqlRangeStorageIntegrationTest is declared
without a namespace which can break PSR-4 autoloading and collide with other
classes; add an appropriate namespace declaration (for example namespace Tests
or namespace Tests\Integration) at the top of the file, update the class
declaration to remain final class SqlRangeStorageIntegrationTest, and ensure any
uses (IPTools\IP, IPTools\Network, IPTools\Range,
IPTools\Storage\SqlRangeStorage) still resolve correctly (adjust use statements
if you move the file into a sub-namespace).
- Around line 104-115: The connect method currently constructs a new PDO which
will throw a PDOException on network/auth failures and break tests; wrap the PDO
creation and setAttribute calls in a try-catch that catches PDOException, call
$this->markTestSkipped with a brief message including the exception message (so
CI skips the test instead of failing), and return null to preserve the ?PDO
signature; reference the connect method, the PDO constructor, and PDOException
in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73184c73-a795-43bb-8be9-9a1f86efbb9b
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/SqlRangeStorageIntegrationTest.php
Address concrete review findings by hardening parser and range input validation, correcting 192.0.0.0/24 classification exceptions, and improving SQL storage portability/error handling. Keep Laravel model and integration defaults aligned with table schema while adding regression coverage for the updated behaviors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This branch delivers a major feature expansion across IPTools core APIs, optional persistence, and documentation quality.
It adds:
docs/section with cross-linked guidesWhat’s Included
Core IP APIs
IPTypeenum and classification registry (types(),primaryType(),is*()helpers)compareTo,distanceTo,addOffset,next,previous,shift)OverflowMode(THROW,NULL,WRAP,CLAMP)toIpv4Mapped/fromIpv4Mappedto6to4/from6to4toNat64/fromNat64(+ NAT64 detection)Parsing
Parser,ParseFlags, andParsedAddressRange / Network
Range::addressAt()andaddressAtOrFail()RangeSetwith normalization and algebra:union,intersect,subtract,contains,containsRange,overlaps,toCidrsnetworkAddress,broadcastAddressfirstHost,lastHost,usableHostCountisPointToPoint,containsIP,containsRangenextSubnet,previousSubnetOptional Storage + Laravel Integration
RangeStorageInterfaceSqlRangeStorageAddressCodecIPToolsServiceProviderLaravelRangeStorageIpRangemodelDocumentation
docs/:Behavioral Notes
IP::parseLong()now enforces strict numeric bounds and rejects out-of-range values instead of implicit wraparound.Network::exclude()now enforces true containment of the excluded subnet.Summary by CodeRabbit
New Features
Documentation
Tests
Chores