refactor: remove model indexing

jaredhendrickson13 · jaredhendrickson13 · commit f46a205d0a19 · 2025-12-15T22:46:21.000-07:00
Model indexing isn't practical here because the cache isn't retained between requests. It makes more sense to cache logical queries instead of basic indexing.
diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/ModelCache.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/ModelCache.inc
@@ -21,20 +21,11 @@ class ModelCache {
     public static ?self $instance = null;
 
     /**
-     * @var array $index An associative array that contains cache Model objects that have been loaded and
-     * indexed by specific fields for quick lookup. Structured as:
-     *
-     * [Model class name] => [
-     *     [index field name] => [
-     *         [index field value] => Model object
-     *     ]
-     * ]
-     */
-    public static array $index = [];
-
-    /**
-     * @var array $cache An associative array that contains cached Model objects that have been loaded into memory as
-     * a complete ModelSet of every object of that Model type. Structured as: [Model class name] => ModelSet object
+     * @var array $cache An associative array that contains cached ModelSet objects that have been loaded into memory.
+     * This includes root ModelSets that contain all Model objects of a given Model class as well as indexed queries
+     * that have already been run against those ModelSets. Structured as: [ModelSet signature => ModelSet object].
+     * Please note that root ModelSets are indexed by their qualified Model class name only, whereas queried ModelSets
+     * are indexed by the root signature followed by a series of query hashes separated by colons.
      */
     public static array $cache = [];
 
@@ -83,143 +74,10 @@ class ModelCache {
         return self::$cache[$model_class];
     }
 
-    /**
-     * Raises an error if the given Model object does not support being indexed by the specified field.
-     * @param Model $model The Model object to check for uniqueness.
-     * @param string $index_field The index field name to check for uniqueness.
-     * @throws ServerError If the Model is attempting to be indexed by a non-unique field.
-     * @throws ServerError If the Model is not many-enabled.
-     */
-    private static function ensure_model_supports_indexing(Model $model, string $index_field): void {
-        # Only many-enabled Models can be indexed
-        if (!$model->many) {
-            throw new ServerError(
-                message: "Cannot index Model class '" . $model->get_class_fqn() . 'because it is not many-enabled.',
-                response_id: 'MODEL_CACHE_INDEX_FIELD_ON_NON_MANY_MODEL',
-            );
-        }
-
-        # Models with parent model classes cannot be indexed
-        if ($model->parent_model_class) {
-            throw new ServerError(
-                message: "Cannot index Model class '" .
-                    $model->get_class_fqn() .
-                    "' because it has a parent model class '" .
-                    $model->parent_model_class .
-                    "'.",
-                response_id: 'MODEL_CACHE_INDEX_FIELD_ON_PARENTED_MODEL',
-            );
-        }
-
-        # If indexing by 'id', it's always unique
-        if ($index_field === 'id') {
-            return;
-        }
-
-        # Check if the index field is unique on the Model object
-        if (!$model->$index_field->unique) {
-            throw new ServerError(
-                message: "Cannot index Model class '" .
-                    $model->get_class_fqn() .
-                    "' by non-unique field " .
-                    "'$index_field'.",
-                response_id: 'MODEL_CACHE_INDEX_FIELD_NOT_UNIQUE',
-            );
-        }
-    }
-
-    /**
-     * Indexes the ModelSet cache for a given Model class by a specified field. This method will populate
-     * the $index array for the specified Model class and index field.
-     * @param string $model_class The Model class name whose ModelSet is to be indexed. If no cached ModelSet exists,
-     * one will be created by reading all Model objects from the data source.
-     * @param string $index_field The field name to index the Model objects by.
-     * @return array An associative array of indexed Model objects.
-     */
-    public static function index_modelset_by_field(string $model_class, string $index_field): array {
-        # First, check if this Model class has already been indexed by this field
-        if (isset(self::$index[$model_class][$index_field])) {
-            return self::$index[$model_class][$index_field];
-        }
-
-        # Ensure the Model can be indexed by the specified field
-        $model = new $model_class();
-        self::ensure_model_supports_indexing($model, $index_field);
-
-        # Fetch or create the ModelSet for this Model class
-        $model_set = $model->read_all();
-
-        # Create an associative array to hold the indexed Model objects
-        $indexed_models = [];
-
-        # Index each Model object in the ModelSet by the specified field
-        foreach ($model_set->model_objects as $model) {
-            $index_value = $index_field === 'id' ? $model->id : $model->$index_field->value;
-            $indexed_models[$index_value] = $model;
-        }
-
-        # Store the indexed models in the ModelCache index and return them
-        self::$index[$model_class][$index_field] = $indexed_models;
-        return $indexed_models;
-    }
-
-    /**
-     * Checks if a given Model class has a cached Model object by its index field/value.
-     * @param string $model_class The Model class name to check in the cache.
-     * @param string $index_field The index field name to look up the Model object by
-     * @param mixed $index_value The index field value to look up the Model object by
-     * @return bool True if a cached Model object exists for the specified Model class and index
-     */
-    public static function has_model(string $model_class, string $index_field = 'id', mixed $index_value = null): bool {
-        # Cache is always a miss if the Model class is not indexed
-        if (!self::$index[$model_class]) {
-            return false;
-        }
-
-        # Cache is a hit for non-many enabled models if a single Model object is indexed under the Model class
-        if (self::$index[$model_class] instanceof Model) {
-            return true;
-        }
-
-        # Otherwise, cache is only a hit for many enabled models if the index field/value exists in the index
-        return isset(self::$index[$model_class][$index_field][$index_value]);
-    }
-
-    /**
-     * Fetches a cached Model object by its Model class name and index field/value.
-     * @param string $model_class The Model class name to load from the cache.
-     * @param string $index_field The index field name to look up the Model object by. Only for many enabled Models.
-     * @param mixed $index_value The index field value to look up the Model object by. Only for many enabled Models.
-     * @return Model The cached Model object
-     * @throws NotFoundError If no cached Model object exists for the specified Model class and index.
-     */
-    public static function fetch_model(
-        string $model_class,
-        string $index_field = 'id',
-        mixed $index_value = null,
-    ): Model {
-        if (!self::has_model($model_class, $index_field, $index_value)) {
-            throw new NotFoundError(
-                message: "No cached Model found for Model class '$model_class' with " .
-                    "index field '$index_field' and index value '$index_value'.",
-                response_id: 'MODEL_CACHE_MODEL_NOT_FOUND',
-            );
-        }
-
-        # For many enabled models, return the Model object indexed under the specified index field and value
-        if (isset(self::$index[$model_class][$index_field][$index_value])) {
-            return self::$index[$model_class][$index_field][$index_value];
-        }
-
-        # Otherwise, return the Model object indexed under the Model class only
-        return self::$index[$model_class];
-    }
-
     /**
      * Clears the ModelCache of all cached ModelSets and indexed Model objects.
      */
     public static function clear(): void {
-        self::$index = [];
         self::$cache = [];
     }
 }
diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/ModelSet.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/ModelSet.inc
@@ -13,6 +13,14 @@ require_once 'RESTAPI/autoloader.inc';
 class ModelSet {
     use BaseTraits;
 
+    /**
+     * @var string $signature A unique signature for this ModelSet used to index this ModelSet in the query cache
+     * while retaining all query chains. Signatures are compromised of a root signature (the Model class name) and
+     * a series of query signatures that represent each query that has been applied to this ModelSet to this point
+     * (separated by a colon). Example 'RESTAPI\Models\User:queryhash1:queryhash2'
+     */
+    public string $signature = '';
+
     /**
      * @var array $model_objects An array of Model objects contained by this ModelSet.
      */
diff --git a/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APICoreModelCacheTestCase.inc b/pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APICoreModelCacheTestCase.inc
@@ -83,117 +83,4 @@ class APICoreModelCacheTestCase extends TestCase {
             },
         );
     }
-
-    /**
-     * Ensures Models must be many-enabled to be indexed in the ModelCache.
-     */
-    public function test_model_index_must_be_many_enabled(): void {
-        # Ensure attempting to index a non-many enabled Model throws an error
-        $this->assert_throws_response(
-            response_id: 'MODEL_CACHE_INDEX_FIELD_ON_NON_MANY_MODEL',
-            code: 500,
-            callable: function () {
-                ModelCache::index_modelset_by_field(model_class: RESTAPISettings::get_class_fqn(), index_field: 'id');
-            },
-        );
-    }
-
-    /**
-     * Ensures we cannot index a cached ModelSet by a non-unique field.
-     */
-    public function test_model_index_field_must_be_unique(): void {
-        # Ensure indexing by id is always allowed
-        $this->assert_does_not_throw(function () {
-            ModelCache::index_modelset_by_field(model_class: FirewallAlias::get_class_fqn(), index_field: 'id');
-        });
-
-        # Ensure attempting to index the Model by the non-unique field throws an error
-        $this->assert_throws_response(
-            response_id: 'MODEL_CACHE_INDEX_FIELD_NOT_UNIQUE',
-            code: 500,
-            callable: function () {
-                ModelCache::index_modelset_by_field(model_class: FirewallAlias::get_class_fqn(), index_field: 'descr');
-            },
-        );
-    }
-
-    /**
-     * Ensures we cannot index a cached ModelSet for a Model that has a parent model class.
-     */
-    public function test_model_index_field_cannot_have_parent_model_class(): void {
-        # Ensure attempting to index the Model by the non-unique field throws an error
-        $this->assert_throws_response(
-            response_id: 'MODEL_CACHE_INDEX_FIELD_ON_PARENTED_MODEL',
-            code: 500,
-            callable: function () {
-                ModelCache::index_modelset_by_field(
-                    model_class: DNSResolverHostOverrideAlias::get_class_fqn(),
-                    index_field: 'id',
-                );
-            },
-        );
-    }
-
-    /**
-     * Ensures the index_modelset_by_field() method correctly indexes a cached ModelSet by the specified field. This
-     * test also ensures the has_model() and fetch_model() methods work as expected for indexed Model objects.
-     */
-    public function test_model_index_modelset_by_field(): void {
-        # Create FirewallAlias model objects to test with
-        $alias1 = new FirewallAlias(data: ['name' => 'host_alias', 'descr' => 'First alias', 'type' => 'host']);
-        $alias2 = new FirewallAlias(data: ['name' => 'network_alias', 'descr' => 'Second alias', 'type' => 'network']);
-        $alias3 = new FirewallAlias(data: ['name' => 'port_alias', 'descr' => 'Third alias', 'type' => 'port']);
-        $alias1->create();
-        $alias2->create();
-        $alias3->create();
-
-        # Index the FirewallAlias ModelSet by the 'name' field
-        ModelCache::index_modelset_by_field(model_class: FirewallAlias::get_class_fqn(), index_field: 'name');
-
-        # Ensure the ModelCache has the indexed Models
-        $this->assert_is_true(
-            ModelCache::has_model(
-                model_class: FirewallAlias::get_class_fqn(),
-                index_field: 'name',
-                index_value: 'host_alias',
-            ),
-        );
-        $this->assert_is_true(
-            ModelCache::has_model(
-                model_class: FirewallAlias::get_class_fqn(),
-                index_field: 'name',
-                index_value: 'network_alias',
-            ),
-        );
-        $this->assert_is_true(
-            ModelCache::has_model(
-                model_class: FirewallAlias::get_class_fqn(),
-                index_field: 'name',
-                index_value: 'port_alias',
-            ),
-        );
-
-        # Fetch the indexed Models and ensure they are correct
-        $fetched_alias1 = ModelCache::fetch_model(
-            model_class: FirewallAlias::get_class_fqn(),
-            index_field: 'name',
-            index_value: 'host_alias',
-        );
-        $this->assert_equals($fetched_alias1->name->value, 'host_alias');
-        $fetched_alias2 = ModelCache::fetch_model(
-            model_class: FirewallAlias::get_class_fqn(),
-            index_field: 'name',
-            index_value: 'network_alias',
-        );
-        $this->assert_equals($fetched_alias2->name->value, 'network_alias');
-        $fetched_alias3 = ModelCache::fetch_model(
-            model_class: FirewallAlias::get_class_fqn(),
-            index_field: 'name',
-            index_value: 'port_alias',
-        );
-        $this->assert_equals($fetched_alias3->name->value, 'port_alias');
-
-        # Clean up the created aliases
-        FirewallAlias::delete_all();
-    }
 }
