fix: fixing visibiloity for fields

binaryk · binaryk · commit def91d838aa8 · 2025-10-23T12:47:21.000+03:00
diff --git a/src/Fields/OrganicField.php b/src/Fields/OrganicField.php
@@ -6,6 +6,8 @@
 use Binaryk\LaravelRestify\MCP\Requests\McpIndexRequest;
 use Binaryk\LaravelRestify\MCP\Requests\McpRequestable;
 use Binaryk\LaravelRestify\MCP\Requests\McpShowRequest;
+use Binaryk\LaravelRestify\MCP\Requests\McpStoreRequest;
+use Binaryk\LaravelRestify\MCP\Requests\McpUpdateRequest;
 use Binaryk\LaravelRestify\Traits\ProxiesCanSeeToGate;
 use Closure;
 use Illuminate\Http\Request;
@@ -148,24 +150,49 @@ public function isHiddenOnIndex(RestifyRequest $request, $repository): bool
 
     public function isShownOnMcp(RestifyRequest $request, $repository): bool
     {
-        if ($this->isHidden($request)) {
+        // Check if field is hidden from MCP
+        if ($this->isHiddenFromMcp($request, $repository)) {
             return false;
         }
 
-        if ($this->isHiddenFromMcp($request, $repository)) {
+        // For store and update requests, hidden fields should still be writable
+        // So we check isHidden() only for show/index requests
+        $isReadOperation = $request instanceof McpShowRequest || $request instanceof McpIndexRequest;
+
+        if ($isReadOperation && $this->isHidden($request)) {
             return false;
         }
 
         if (is_callable($this->showOnMcp)) {
             return call_user_func($this->showOnMcp, $request, $repository);
         }
 
+        // For MCP show requests, check the base showOnShow property without recursion
         if ($request instanceof McpShowRequest) {
-            return $this->isHiddenOnShow($request, $repository) === false;
+            if (is_callable($this->showOnShow)) {
+                $this->showOnShow = call_user_func($this->showOnShow, $request, $repository);
+            }
+
+            return $this->showOnShow;
         }
 
+        // For MCP index requests, check the base showOnIndex property without recursion
         if ($request instanceof McpIndexRequest) {
-            return $this->isHiddenOnIndex($request, $repository) === false;
+            if (is_callable($this->showOnIndex)) {
+                $this->showOnIndex = call_user_func($this->showOnIndex, $request, $repository);
+            }
+
+            return $this->showOnIndex;
+        }
+
+        // For MCP store requests, check if field is not readonly
+        if ($request instanceof McpStoreRequest) {
+            return ! $this->isReadonly($request);
+        }
+
+        // For MCP update requests, check if field is not readonly
+        if ($request instanceof McpUpdateRequest) {
+            return ! $this->isReadonly($request);
         }
 
         return $this->showOnMcp;
@@ -267,21 +294,41 @@ public function isReadonly(RestifyRequest $request)
 
     public function isShownOnUpdate(RestifyRequest $request, $repository): bool
     {
+        // Check MCP-specific visibility for MCP requests
+        if ($request instanceof McpRequestable) {
+            return $this->isShownOnMcp($request, $repository);
+        }
+
         return ! $this->isReadonly($request);
     }
 
     public function isShownOnUpdateBulk(RestifyRequest $request, $repository): bool
     {
+        // Check MCP-specific visibility for MCP requests
+        if ($request instanceof McpRequestable) {
+            return $this->isShownOnMcp($request, $repository);
+        }
+
         return ! $this->isReadonly($request);
     }
 
     public function isShownOnStore(RestifyRequest $request, $repository): bool
     {
+        // Check MCP-specific visibility for MCP requests
+        if ($request instanceof McpRequestable) {
+            return $this->isShownOnMcp($request, $repository);
+        }
+
         return ! $this->isReadonly($request);
     }
 
     public function isShownOnStoreBulk(RestifyRequest $request, $repository): bool
     {
+        // Check MCP-specific visibility for MCP requests
+        if ($request instanceof McpRequestable) {
+            return $this->isShownOnMcp($request, $repository);
+        }
+
         return ! $this->isReadonly($request);
     }
 
diff --git a/tests/Fields/FieldMcpVisibilityTest.php b/tests/Fields/FieldMcpVisibilityTest.php
@@ -7,6 +7,9 @@
 use Binaryk\LaravelRestify\Http\Requests\RestifyRequest;
 use Binaryk\LaravelRestify\MCP\Requests\McpIndexRequest;
 use Binaryk\LaravelRestify\MCP\Requests\McpRequest;
+use Binaryk\LaravelRestify\MCP\Requests\McpShowRequest;
+use Binaryk\LaravelRestify\MCP\Requests\McpStoreRequest;
+use Binaryk\LaravelRestify\MCP\Requests\McpUpdateRequest;
 use Binaryk\LaravelRestify\Tests\Fixtures\Post\PostRepository;
 use Binaryk\LaravelRestify\Tests\IntegrationTestCase;
 
@@ -172,4 +175,129 @@ public function test_field_collection_preserves_other_filters_with_mcp(): void
         $this->assertNotContains('mcp_hidden', $indexFieldNames);
         $this->assertNotContains('eager_field', $indexFieldNames); // EagerFields excluded from index
     }
+
+    public function test_mcp_show_request_does_not_cause_infinite_loop(): void
+    {
+        $fields = new FieldCollection([
+            Field::make('title'),
+            Field::make('secret_key')->hideFromMcp(),
+            Field::make('visible_field')->showOnShow(true),
+        ]);
+
+        // This test ensures that McpShowRequest doesn't cause infinite recursion
+        // between isShownOnMcp() and isShownOnShow() methods
+        $mcpShowRequest = new McpShowRequest;
+
+        // This should not cause infinite loop
+        $showFields = $fields->forShow($mcpShowRequest, $this->repository);
+        $fieldNames = $showFields->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('title', $fieldNames);
+        $this->assertNotContains('secret_key', $fieldNames); // Hidden from MCP
+        $this->assertContains('visible_field', $fieldNames);
+    }
+
+    public function test_mcp_index_request_does_not_cause_infinite_loop(): void
+    {
+        $fields = new FieldCollection([
+            Field::make('name'),
+            Field::make('internal_token')->hideFromMcp(),
+            Field::make('public_data')->showOnIndex(true),
+        ]);
+
+        // This test ensures that McpIndexRequest doesn't cause infinite recursion
+        $mcpIndexRequest = new McpIndexRequest;
+
+        // This should not cause infinite loop
+        $indexFields = $fields->forIndex($mcpIndexRequest, $this->repository);
+        $fieldNames = $indexFields->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('name', $fieldNames);
+        $this->assertNotContains('internal_token', $fieldNames); // Hidden from MCP
+        $this->assertContains('public_data', $fieldNames);
+    }
+
+    public function test_mcp_store_request_respects_hide_from_mcp(): void
+    {
+        $fields = new FieldCollection([
+            Field::make('title'),
+            Field::make('secret_api_key')->hideFromMcp(),
+            Field::make('content'),
+            Field::make('internal_metadata')->hideFromMcp(function ($request) {
+                return ! $request->get('is_admin', false);
+            }),
+        ]);
+
+        // MCP store request without admin
+        $mcpStoreRequest = new McpStoreRequest;
+        $storeFields = $fields->forStore($mcpStoreRequest, $this->repository);
+        $fieldNames = $storeFields->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('title', $fieldNames);
+        $this->assertNotContains('secret_api_key', $fieldNames); // Hidden from MCP
+        $this->assertContains('content', $fieldNames);
+        $this->assertNotContains('internal_metadata', $fieldNames); // Hidden by callback
+
+        // MCP store request with admin
+        $mcpStoreRequestAdmin = new McpStoreRequest(['is_admin' => true]);
+        $storeFieldsAdmin = $fields->forStore($mcpStoreRequestAdmin, $this->repository);
+        $fieldNamesAdmin = $storeFieldsAdmin->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('title', $fieldNamesAdmin);
+        $this->assertNotContains('secret_api_key', $fieldNamesAdmin); // Still hidden from MCP
+        $this->assertContains('content', $fieldNamesAdmin);
+        $this->assertContains('internal_metadata', $fieldNamesAdmin); // Visible to admin
+
+        // Regular store request should show all fields except readonly
+        $regularStoreRequest = new RestifyRequest;
+        $regularStoreFields = $fields->forStore($regularStoreRequest, $this->repository);
+        $regularFieldNames = $regularStoreFields->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('title', $regularFieldNames);
+        $this->assertContains('secret_api_key', $regularFieldNames); // Visible in regular request
+        $this->assertContains('content', $regularFieldNames);
+        $this->assertContains('internal_metadata', $regularFieldNames);
+    }
+
+    public function test_mcp_update_request_respects_hide_from_mcp(): void
+    {
+        $fields = new FieldCollection([
+            Field::make('name'),
+            Field::make('password')->hideFromMcp(),
+            Field::make('email'),
+            Field::make('admin_notes')->hideFromMcp(function ($request) {
+                return $request->get('role') !== 'admin';
+            }),
+        ]);
+
+        // MCP update request without admin role
+        $mcpUpdateRequest = new McpUpdateRequest(['role' => 'user']);
+        $updateFields = $fields->forUpdate($mcpUpdateRequest, $this->repository);
+        $fieldNames = $updateFields->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('name', $fieldNames);
+        $this->assertNotContains('password', $fieldNames); // Hidden from MCP
+        $this->assertContains('email', $fieldNames);
+        $this->assertNotContains('admin_notes', $fieldNames); // Hidden by callback
+
+        // MCP update request with admin role
+        $mcpUpdateRequestAdmin = new McpUpdateRequest(['role' => 'admin']);
+        $updateFieldsAdmin = $fields->forUpdate($mcpUpdateRequestAdmin, $this->repository);
+        $fieldNamesAdmin = $updateFieldsAdmin->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('name', $fieldNamesAdmin);
+        $this->assertNotContains('password', $fieldNamesAdmin); // Still hidden from MCP
+        $this->assertContains('email', $fieldNamesAdmin);
+        $this->assertContains('admin_notes', $fieldNamesAdmin); // Visible to admin
+
+        // Regular update request should show all fields except readonly
+        $regularUpdateRequest = new RestifyRequest;
+        $regularUpdateFields = $fields->forUpdate($regularUpdateRequest, $this->repository);
+        $regularFieldNames = $regularUpdateFields->map(fn ($field) => $field->getAttribute())->toArray();
+
+        $this->assertContains('name', $regularFieldNames);
+        $this->assertContains('password', $regularFieldNames); // Visible in regular request
+        $this->assertContains('email', $regularFieldNames);
+        $this->assertContains('admin_notes', $regularFieldNames);
+    }
 }
