Experimental: RBX V2 serializer - #473
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The RBXV2 implementation and tests/docs have correctness issues (notably reflection access for private/protected properties and an invalid RBXV2 test header helper) that will cause failures or misleading behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces an experimental RBX V2 object-file serializer for Rubix ML, aiming to improve integrity checking and reduce overhead versus the legacy gzip-based RBX format, while adding a class allow-list to constrain unserialization.
Changes:
- Added
RBXV2serializer + header schema validation, and made it the default serializer forPersistentModel. - Introduced
RBXV1as the legacy gzip-based serializer and deprecatedRBX/GzipNative. - Updated tests, benchmarks, and documentation to reflect the new serializer versions and defaults.
File summaries
| File | Description |
|---|---|
| tests/Serializers/RBXV2Test.php | Adds test coverage for RBX V2 serialization, header checksums, and allow-list behavior. |
| tests/Serializers/RBXV1Test.php | Renames/updates legacy RBX tests to target RBXV1. |
| tests/Base/PersistentModelTest.php | Updates PersistentModel tests to use RBXV2 by default. |
| src/Specifications/RBXV2HeaderSchemaIsValid.php | Adds schema validation for RBX V2 headers. |
| src/Serializers/RBXV2.php | Implements RBX V2 (no gzip, sha256 header checksum, payload checksum + length, allow-list-based unserialize). |
| src/Serializers/RBXV1.php | Extracts legacy gzip-based RBX behavior into an explicit RBXV1 serializer. |
| src/Serializers/RBX.php | Deprecates RBX and aliases behavior via RBXV1. |
| src/Serializers/GzipNative.php | Deprecates GzipNative in favor of RBXV1. |
| src/PersistentModel.php | Switches default serializer to RBXV2 for persistence/load paths. |
| mkdocs.yml | Updates documentation nav to list RBX V1 and RBX V2 pages. |
| docs/serializers/rbx-v2.md | Adds documentation page for RBX V2. |
| docs/serializers/rbx-v1.md | Updates legacy RBX documentation to RBX V1 branding and guidance. |
| docs/serializers/gzip-native.md | Notes deprecation of gzip-native serializer. |
| docs/persisters/filesystem.md | Updates examples to use RBXV2. |
| docs/persistent-model.md | Updates default serializer references and examples to RBXV2. |
| docs/model-persistence.md | Updates model persistence examples to RBXV2. |
| benchmarks/Serializers/RBXV2Bench.php | Adds benchmark for RBX V2 serialize/deserialize. |
| benchmarks/Serializers/RBXV1Bench.php | Updates benchmark to use RBXV1 instead of RBX. |
Review details
Suppressed comments (2)
docs/serializers/rbx-v1.md:1
- The "source" link URL points to a non-existent path (
src/Persisters/Serializers/...). It should link to the actual serializer implementation undersrc/Serializers/.
This issue also appears on line 8 of the same file.
docs/serializers/rbx-v1.md:8
- This doc link target
rbxv2.mddoes not match the actual RBX V2 doc filename (rbx-v2.md), so the link will be broken in the generated site.
- Files reviewed: 18/18 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| foreach ($class->getProperties() as $property) { | ||
| if ($property->getDeclaringClass()->getName() === $class->getName()) { | ||
| $properties[] = $property; | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe this is only for PHP < 8.1
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
I want it to be materially better than V1 ... the main changes are class whitelist, drop gzip, and sha256 header checksum instead of crc32.
Allow list to help prevent the attack where you get a malicious payload although I'm not sure if it's just weak or strong security. Theoretically an attacker could take a saved file and recraft it with a malicious payload and regenerate the checksums. Sure the checksums would give it away but who would bother to check? All we can say is that its a valid checksum not that it's the right one. We'd need a shared secret to guarantee authenticity and there's already solutions like file encryption.
Drop Gzip because it's 3X slower and you can Gzip the whole file later if you want.
Sha256 instead of crc32 for the header was just a way to bolster tamper resistance but again I'm not sure if it's really providing anything or just the illusion of security (which is worse IMO).