-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add resource parsing method to extract ID, type, and parent #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA protected Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes RationaleChanges touch three files with a consistent goal: introduce parsing logic and apply it in single- and batch-create flows. The new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/Audit/Adapter/ClickHouse.php`:
- Line 1149: The call to parseResource in ClickHouse.php uses $log['resource']
which may not be a string; cast it to string to match parseResource's expected
parameter type and mirror the same fix applied in create(): change the
invocation in the method where $resource is set (referencing parseResource and
the surrounding code that handles $log) to pass (string)($log['resource'] ?? '')
so the parameter type always is string.
- Around line 763-764: The call to parseResource in ClickHouse.php passes
$log['resource'] which is typed as mixed; cast it to string to satisfy
parseResource(string) and avoid type errors—replace the argument with an
explicit (string) cast (i.e. pass (string)$log['resource'] to parseResource) so
parseResource($resource) always receives a string; update the invocation at the
line using parseResource to use the cast.
In `@src/Audit/Adapter/SQL.php`:
- Around line 222-225: The parseResource(string $resource) method lacks a
detailed return type for static analysis; add a PHPDoc `@return` annotation above
the parseResource method specifying the exact array shape (e.g. keys for id,
type, parent with nullable types) so PHPStan knows the value types returned by
parseResource; update the docblock for the protected function parseResource to
include something like `@return` array{id: string|null, type: string, parent:
string|null} referencing the parseResource function name.
In `@tests/Audit/Adapter/ClickHouseTest.php`:
- Around line 458-465: The assertions in ClickHouseTest are checking the wrong
array keys: update the key existence checks to match parseResource()'s output by
replacing assertions for 'id', 'type', and 'parent' with 'resourceId',
'resourceType', and 'resourceParent' respectively (i.e., change the
assertArrayHasKey calls that reference 'id'/'type'/'parent' to assert
'resourceId'/'resourceType'/'resourceParent' so they align with the subsequent
assertEquals checks and the parseResource() contract).
- Around line 420-439: The test testParseResourceComplexPath omits required
ClickHouse attributes which causes the "Required attribute 'userType' is
missing" error; update the test to merge the adapter's required attributes into
$data before calling $this->audit->log (use the adapter's
getRequiredAttributes() result or explicitly add keys like userType, projectId,
teamId, and any other keys returned by getRequiredAttributes()) so the $data
passed to $this->audit->log contains all required ClickHouse attributes while
still leaving out resourceType/resourceId to allow parsing.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.