Merge pull request #38 from Guiziweb/refactor/symmetric-registries-and-enabled-fields

refactor(schema): symmetric registry interface and tighten filter exposure to LLM

Author: camilleislasse

src/Schema/Builder/FilterSchemaBuilderRegistry.php

@@ -4,7 +4,7 @@
 
 namespace Guiziweb\SyliusGridAssistantPlugin\Schema\Builder;
 
-final class FilterSchemaBuilderRegistry
+final class FilterSchemaBuilderRegistry implements FilterSchemaBuilderRegistryInterface
 {
     /** @var array<string, FilterSchemaBuilderInterface> */
     private array $builders = [];

src/Schema/Builder/FilterSchemaBuilderRegistryInterface.php

@@ -0,0 +1,14 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Guiziweb\SyliusGridAssistantPlugin\Schema\Builder;
+
+interface FilterSchemaBuilderRegistryInterface
+{
+    public function register(FilterSchemaBuilderInterface $builder): void;
+
+    public function has(string $type): bool;
+
+    public function get(string $type): FilterSchemaBuilderInterface;
+}

src/Schema/GridSchemaBuilder.php

@@ -4,9 +4,8 @@
 
 namespace Guiziweb\SyliusGridAssistantPlugin\Schema;
 
-use Guiziweb\SyliusGridAssistantPlugin\Schema\Builder\FilterSchemaBuilderRegistry;
+use Guiziweb\SyliusGridAssistantPlugin\Schema\Builder\FilterSchemaBuilderRegistryInterface;
 use Guiziweb\SyliusGridAssistantPlugin\Schema\Builder\TranslateLabelTrait;
-use Sylius\Component\Grid\Definition\Filter;
 use Sylius\Component\Grid\Definition\Grid;
 use Sylius\Component\Grid\Provider\GridProviderInterface;
 use Symfony\Component\DependencyInjection\Attribute\Autowire;
@@ -23,7 +22,7 @@
     public function __construct(
         #[Autowire(service: 'sylius.grid.chain_provider')]
         private GridProviderInterface $gridProvider,
-        private FilterSchemaBuilderRegistry $filterSchemaBuilderRegistry,
+        private FilterSchemaBuilderRegistryInterface $filterSchemaBuilderRegistry,
         private TranslatorInterface $translator,
     ) {
     }
@@ -64,24 +63,15 @@ private function buildFiltersSchema(Grid $grid): array
                 continue;
             }
 
-            $filters[$name] = $this->buildFilterSchema($filter);
-        }
-
-        return $filters;
-    }
-
-    /**
-     * @return array<string, mixed>
-     */
-    private function buildFilterSchema(Filter $filter): array
-    {
-        $type = $filter->getType();
+            $type = $filter->getType();
+            if (!$this->filterSchemaBuilderRegistry->has($type)) {
+                continue;
+            }
 
-        if (!$this->filterSchemaBuilderRegistry->has($type)) {
-            return [];
+            $filters[$name] = $this->filterSchemaBuilderRegistry->get($type)->build($filter);
         }
 
-        return $this->filterSchemaBuilderRegistry->get($type)->build($filter);
+        return $filters;
     }
 
     /**

src/Validator/GridCriteriaValidator.php

@@ -34,12 +34,14 @@ public function validate(array $rawCriteria, Grid $grid): array
             $filter = $grid->getFilter($filterName);
             $filterType = $filter->getType();
 
-            if ($this->formatterRegistry->has($filterType)) {
-                $formatted = $this->formatterRegistry->get($filterType)->format($value, $filter)->value;
-            } else {
-                $formatted = $value;
+            if (!$this->formatterRegistry->has($filterType)) {
+                $this->aiLogger->warning('[GridAssistant] No formatter registered for filter type, skipping', ['filter' => $filterName, 'type' => $filterType]);
+
+                continue;
             }
 
+            $formatted = $this->formatterRegistry->get($filterType)->format($value, $filter)->value;
+
             if (null !== $formatted) {
                 $valid[$filterName] = $formatted;
             }

src/Validator/GridSortingValidator.php

@@ -17,7 +17,7 @@ public function __construct(
     public function validate(array $rawSorting, Grid $grid): array
     {
         $sortableFields = [];
-        foreach ($grid->getFields() as $field) {
+        foreach ($grid->getEnabledFields() as $field) {
             if ($field->isSortable()) {
                 $sortableFields[] = $field->getName();
             }

tests/Unit/Schema/GridSchemaBuilderTest.php

@@ -30,6 +30,18 @@ public function testFiltersWithAiSearchableFalseAreExcluded(): void
         self::assertArrayNotHasKey('internal_notes', $schema['filters']);
     }
 
+    public function testFiltersWithoutRegisteredSchemaBuilderAreExcluded(): void
+    {
+        $grid = $this->makeGrid();
+        $grid->addFilter($this->makeFilter('date', 'date'));
+        $grid->addFilter($this->makeFilter('legacy', 'custom_unknown'));
+
+        $schema = $this->makeBuilder($grid)->buildSchema('any');
+
+        self::assertArrayHasKey('date', $schema['filters']);
+        self::assertArrayNotHasKey('legacy', $schema['filters']);
+    }
+
     public function testSortableFieldsWithAiSearchableFalseAreExcluded(): void
     {
         $grid = $this->makeGrid();

tests/Unit/Validator/GridCriteriaValidatorTest.php

@@ -40,14 +40,19 @@ public function testSkipsUnknownFilterAndLogsWarning(): void
         self::assertSame([], $validator->validate(['unknown' => 'value'], $grid));
     }
 
-    public function testPassesValueThroughWhenNoFormatterRegistered(): void
+    public function testSkipsValueWhenNoFormatterRegisteredAndLogsWarning(): void
     {
         $grid = $this->makeGrid();
-        $grid->addFilter(Filter::fromNameAndType('state', 'select'));
+        $grid->addFilter(Filter::fromNameAndType('state', 'custom_unknown'));
 
-        $validator = new GridCriteriaValidator($this->emptyRegistry(), $this->createMock(LoggerInterface::class));
+        $logger = $this->createMock(LoggerInterface::class);
+        $logger->expects(self::once())
+            ->method('warning')
+            ->with('[GridAssistant] No formatter registered for filter type, skipping', ['filter' => 'state', 'type' => 'custom_unknown']);
+
+        $validator = new GridCriteriaValidator($this->emptyRegistry(), $logger);
 
-        self::assertSame(['state' => 'new'], $validator->validate(['state' => 'new'], $grid));
+        self::assertSame([], $validator->validate(['state' => 'attacker-controlled'], $grid));
     }
 
     public function testAppliesFormatterWhenRegistered(): void

tests/Unit/Validator/GridSortingValidatorTest.php

@@ -104,6 +104,24 @@ public function testInvalidDirectionIsSkippedAndLogged(): void
         );
     }
 
+    public function testIgnoresDisabledField(): void
+    {
+        $logger = $this->createMock(LoggerInterface::class);
+        $logger->expects(self::once())
+            ->method('warning')
+            ->with('[GridAssistant] Unknown sortable field skipped', ['field' => 'total']);
+
+        $grid = Grid::fromCodeAndDriverConfiguration('test_grid', 'doctrine/orm', []);
+        $field = Field::fromNameAndType('total', 'string');
+        $field->setSortable('total');
+        $field->setEnabled(false);
+        $grid->addField($field);
+
+        $validator = new GridSortingValidator($logger);
+
+        self::assertSame([], $validator->validate(['total' => 'asc'], $grid));
+    }
+
     private function gridWithSortableField(string $name): Grid
     {
         $grid = Grid::fromCodeAndDriverConfiguration('test_grid', 'doctrine/orm', []);