-
Notifications
You must be signed in to change notification settings - Fork 35
Add request model support #187
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces model-based parameter handling to the Utopia framework. The PHP requirement is bumped from >=8.1 to >=8.3, and dev dependencies are relaxed to wildcard ranges. A new Model interface is added with a fromArray static method contract. The Hook class extends its param method with an optional model parameter. The App class implements model parameter validation, JSON parsing, and instantiation logic. Comprehensive test coverage is added including model implementations and validators. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
# Conflicts: # composer.lock
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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: 0
🧹 Nitpick comments (3)
composer.json (1)
31-34: Consider using caret constraints for dev dependencies.Wildcard ranges like
9.*allow any minor/patch version within the major version, which is less restrictive than the previous caret constraints (e.g.,^9.5.25). While acceptable for dev dependencies, caret constraints are generally preferred for better reproducibility.- "phpunit/phpunit": "9.*", - "laravel/pint": "1.*", - "phpstan/phpstan": "1.*", - "phpbench/phpbench": "1.*" + "phpunit/phpunit": "^9.5", + "laravel/pint": "^1.2", + "phpstan/phpstan": "^1.10", + "phpbench/phpbench": "^1.2"src/Model.php (1)
5-8: LGTM! Clean interface design.The interface is minimal and well-designed. Consider adding a docblock to guide implementors on expected behavior and error handling conventions.
interface Model { + /** + * Create a model instance from an associative array. + * + * @param array<string, mixed> $value The input data array + * @return static A new instance of the implementing class + * @throws \InvalidArgumentException When required fields are missing or invalid + */ public static function fromArray(array $value): static; }tests/ModelTest.php (1)
37-56: Consider whether nullable with default is intentional for$country.The constructor signature
public ?string $country = 'USA'allowsnullto be explicitly passed, which would override the default. If the intent is to always default to 'USA' when not provided, consider:- public ?string $country = 'USA' + public string $country = 'USA'And in
fromArray:- $value['country'] ?? 'USA' + $value['country'] ?? 'USA' // This already handles missing keys correctlyThis is a minor consideration for test code, but worth noting for real model implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
composer.json(1 hunks)src/App.php(1 hunks)src/Hook.php(2 hunks)src/Model.php(1 hunks)tests/ModelTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/App.php (5)
src/Hook.php (3)
param(205-222)setParamValue(259-268)getInjections(163-166)src/Exception.php (1)
Exception(5-7)src/Model.php (1)
fromArray(7-7)tests/ModelTest.php (2)
fromArray(23-33)fromArray(47-55)src/Route.php (1)
hook(113-118)
src/Model.php (1)
tests/ModelTest.php (2)
fromArray(23-33)fromArray(47-55)
tests/ModelTest.php (4)
src/App.php (5)
App(10-958)__construct(137-141)get(199-202)run(753-779)error(328-336)src/Request.php (1)
Request(5-745)src/Hook.php (4)
__construct(55-59)param(205-222)action(141-146)inject(176-188)src/Model.php (1)
fromArray(7-7)
🪛 PHPMD (2.15.0)
tests/ModelTest.php
201-201: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
228-228: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
255-255: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
364-364: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
391-391: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (12)
composer.json (1)
25-25: PHP 8.3 requirement is a breaking change.Bumping from
>=8.1to>=8.3will break compatibility for users on PHP 8.1 and 8.2. This is likely required for thestaticreturn type in the newModelinterface. Ensure this is documented in the changelog/upgrade guide.src/App.php (3)
700-729: Model parameter handling logic is well-structured.The implementation correctly handles:
- JSON string parsing with proper error handling
- Direct array input
- Type validation for invalid inputs
- Optional parameters with empty string coercion
One edge case to consider: when
$valueisnulland$paramExistsis true (e.g., explicitly passednull), the code reaches line 716 where$value !== nullis false, so it passes. Then line 719is_array($value)is false, so model instantiation is skipped. Finally, line 726$value === ''is false, so$valueremainsnull. This seems intentional for explicitly nullable params.
731-737: Validation skip logic is correct but condition could be simplified.The condition correctly skips validation when:
skipValidationis true, OR- Parameter is optional AND value is null, OR
- Parameter doesn't exist
However, with the early throw at lines 696-698 for non-optional missing params, the
$paramExistscheck on line 734 is now somewhat redundant for non-optional params. This is fine as a safety net but could be documented.
743-745: LGTM! Simplified iteration.Removing the unused
$keyvariable from the foreach loop is a minor cleanup.src/Hook.php (1)
193-222: LGTM! Backward-compatible extension of param method.The addition of the optional
$modelparameter is clean and follows the existing patterns. The docblock is properly updated, and the internal storage structure includes the new field.tests/ModelTest.php (7)
14-34: Good model implementation with proper validation.UserModel correctly validates required fields and throws
InvalidArgumentExceptionwhen they're missing. Thefinalconstructor prevents issues with late static binding in subclasses.
59-111: LGTM! Validators properly check model instances.Both validators correctly validate that the value is an instance of the expected model class.
119-144: Good test isolation with request state management.The
saveRequestandrestoreRequesthelpers properly isolate test state by saving and restoring$_SERVER,$_GET, and$_POSTglobals. This prevents test pollution.
146-168: LGTM! Comprehensive JSON string input test.The test verifies that JSON strings are correctly parsed and converted to model instances, including optional fields.
193-218: Static analysis false positive - unused parameter is intentional.The unused
$userparameter on line 201 is intentional because this tests the error path where the action should never be reached. The test correctly verifies that invalid JSON triggers an error.
322-354: Excellent coverage for multiple model parameters.This test validates that the framework correctly handles routes with multiple model parameters, each deserializing independently.
356-408: Good error path coverage for invalid model configurations.Tests for non-existent class and non-Model class correctly verify that appropriate errors are thrown with descriptive messages.
With this you can define a model type per parameter, and the framework will take care of transforming the JSON into the model, and then you can validate against the actual model type as well.
With this as the base, from Appwrite, we can define a base request model class, pretty much exactly the same as we have for response models. Request classes can extend the base model, implement the utopia interface, then at spec generation time we have all we need to create proper schema models as we do for responses. Then SDKs can generate fully typed request models wherever needed
Example in the API:
And from SDKs:
Summary by CodeRabbit
Breaking Changes
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.