Conversation
Contributor
Reviewer's GuideAdds support for treating direct nested array entries as input objects (bracket notation) in resolveObjectType and includes a corresponding test to verify this behavior. Class diagram for updated resolveObjectType logic in InputQueryclassDiagram
class InputQuery {
+resolveObjectType(param: ReflectionParameter, query: array, ...): mixed
+newInstance(className: string, data: array): object
+extractNestedQuery(paramName: string, query: array): array
}
InputQuery : +resolveObjectType now supports direct nested array (bracket object) as input object
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughA new conditional block was added to the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @apple-x-co - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/InputQuery.php:268` </location>
<code_context>
+ // Check if the parameter name exists as a direct nested array in query
+ if (array_key_exists($paramName, $query) && is_array($query[$paramName])) {
+ assert(class_exists($className));
+ /** @var class-string<T> $className */
+ return $this->newInstance($className, $query[$paramName]);
</code_context>
<issue_to_address>
Using assert(class_exists(...)) may be insufficient for production environments.
Since assert() can be disabled, replace it with an explicit exception or error handling to ensure $className exists in all environments.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (array_key_exists($paramName, $query) && is_array($query[$paramName])) {
assert(class_exists($className));
/** @var class-string<T> $className */
return $this->newInstance($className, $query[$paramName]);
=======
if (array_key_exists($paramName, $query) && is_array($query[$paramName])) {
if (!class_exists($className)) {
throw new \InvalidArgumentException("Class '$className' does not exist.");
}
/** @var class-string<T> $className */
return $this->newInstance($className, $query[$paramName]);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@koriym レビューお願いします! ※ sourcery-ai に指摘された箇所は、 throw をするのであれば今回追加した if の中だけの話ではないと思ったので |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Added descriptive message to assert(class_exists($className)) to explain: - This is an internal logic error, not invalid user input - $className comes from ReflectionParameter type info, so it should always exist - assert() is the appropriate tool for checking "impossible" conditions - Can be disabled in production for performance, unlike explicit exceptions This addresses code review concerns about assert() usage while maintaining the correct semantic distinction between internal logic errors (assert) and user input validation (exceptions). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
|
@apple-x-co sourcery-aiの言及しているケースはそもそもLogicExceptionでテストの書き用のないバグがあった時のケースですね。 |
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
2 participants
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.
authorのようにネストしたデータ形式の対応Summary by Sourcery
Add support for bracket object notation in InputQuery to create nested input objects from direct nested arrays and include a corresponding unit test.
New Features:
Tests:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests