Skip to content

Conversation

@abnegate
Copy link
Member

@abnegate abnegate commented Sep 22, 2025

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:

class User extends Model implements UtopiaModel
{
    final public function __construct(
        public string $name,
        public int $age,
        public ?string $email = null
    ) {
        $this->addRule('name', [
            'type' => self::TYPE_STRING,
            'description' => 'Users name.',
            'default' => '',
            'example' => 'jake',
        ]);
        ...
    }

    public static function fromArray(array $value): static
    {
        return new static(
            $value['name'],
            $value['age'],
            $value['email'] ?? null
        );
    }
}
App::post('/users')
    ->param('user', null, new UserValidator(), ..., model: User::class)
    ->action(function (?User $user) {
      ...
    });

And from SDKs:

$users->create(new User(name: '...', ...));

Summary by CodeRabbit

  • Breaking Changes

    • PHP minimum version requirement updated to 8.3
  • New Features

    • Added support for model-based parameters with automatic JSON parsing and object instantiation
  • Chores

    • Updated dev dependencies to use flexible version constraints
  • Tests

    • Added comprehensive test coverage for model parameter binding

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 22, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This 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

  • src/App.php: Contains the core logic changes for model parameter handling—review the parameter consolidation, model class validation, JSON parsing, and instantiation flow carefully
  • src/Hook.php: Verify the model parameter is properly threaded through the param method signature and internal storage
  • tests/ModelTest.php: Assess test coverage completeness for edge cases including JSON validation, missing fields, type errors, and optional parameter handling
  • src/Model.php: Confirm the interface contract aligns with expected implementations
  • composer.json: Verify PHP 8.3 requirement bump and dependency range changes are intentional and compatible

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add request model support' accurately describes the main change: introducing model-based parameter handling for request parameters across the framework.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@abnegate abnegate requested a review from Meldiron November 5, 2025 11:03
@abnegate
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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' allows null to 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 correctly

This 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

📥 Commits

Reviewing files that changed from the base of the PR and between 838e3a2 and 32f2a93.

⛔ Files ignored due to path filters (1)
  • composer.lock is 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.1 to >=8.3 will break compatibility for users on PHP 8.1 and 8.2. This is likely required for the static return type in the new Model interface. 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 $value is null and $paramExists is true (e.g., explicitly passed null), the code reaches line 716 where $value !== null is false, so it passes. Then line 719 is_array($value) is false, so model instantiation is skipped. Finally, line 726 $value === '' is false, so $value remains null. 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:

  1. skipValidation is true, OR
  2. Parameter is optional AND value is null, OR
  3. Parameter doesn't exist

However, with the early throw at lines 696-698 for non-optional missing params, the $paramExists check 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 $key variable 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 $model parameter 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 InvalidArgumentException when they're missing. The final constructor 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 saveRequest and restoreRequest helpers properly isolate test state by saving and restoring $_SERVER, $_GET, and $_POST globals. 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 $user parameter 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.

@abnegate abnegate merged commit 76def92 into 0.33.x Dec 8, 2025
5 checks passed
@abnegate abnegate deleted the feat-request-model branch December 8, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants