fix: ensure internal_callable pagination takes reverse into consideration

Author: jaredhendrickson13

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Core/Model.inc

@@ -918,20 +918,19 @@ class Model {
      * the output of the assigned callable.
      * @param bool $from_all_parents When set to `true`, this method will obtain internal objects from all parent
      * objects in config. This is only applicable to Models with a `parent_model_class` assigned.
+     * @param int $limit Pagination limit for internal callables (0 = no limit).
+     * @param int $offset Pagination offset for internal callables (0 = no offset).
+     * @param bool $reverse Whether to read in reverse order (oldest first) for internal callables.
      * @throws ServerError When neither a `config_path` nor a `internal_callable` are assigned to this model, OR both a
      * `config_path` and a `internal_callable` are assigned to this model
      * @return array The array of internal objects without any additional processing performed.
      */
-    /**
-     * @param bool $from_all_parents Whether to obtain objects from all parent Models.
-     * @param int $limit Pagination hint for internal callables (0 = no limit). When non-zero, callables that accept
-     *                    a $limit parameter (e.g. log-backed Models) can use it to avoid loading entire datasets
-     *                    into memory. Callables that do not accept a $limit parameter are called without it.
-     * @param int $offset Pagination hint for internal callables (0 = no offset). When non-zero, callables that accept
-     *                    an $offset parameter can use it to skip entries. Callables that do not accept an $offset
-     *                    parameter are called without it.
-     */
-    public function get_internal_objects(bool $from_all_parents = false, int $limit = 0, int $offset = 0): array {
+    public function get_internal_objects(
+        bool $from_all_parents = false,
+        int $limit = 0,
+        int $offset = 0,
+        bool $reverse = false,
+    ): array {
         global $mock_internal_objects;
 
         # Throw an error if both `config_path` and `internal_callable` are set.
@@ -957,17 +956,16 @@ class Model {
         elseif ($this->internal_callable) {
             $callable = $this->internal_callable;
 
-            # Forward the pagination limit and offset to callables that accept them. This allows log-backed
-            # Models to stop reading early instead of loading entire log files into memory.
-            # Callables that don't accept these parameters continue to work unchanged.
-            # Reflection results are cached per class+callable to avoid repeated introspection.
-            $accepts_limit = ($limit > 0 || $offset > 0) && $this->callable_accepts_param($callable, 'limit');
-            $accepts_offset = ($limit > 0 || $offset > 0) && $this->callable_accepts_param($callable, 'offset');
-
-            if ($accepts_limit && $accepts_offset) {
-                $internal_objects = $this->$callable(limit: $limit, offset: $offset);
-            } elseif ($accepts_limit) {
-                $internal_objects = $this->$callable(limit: $limit);
+            # Forward pagination parameters to callables that accept ALL THREE parameters (limit, offset, reverse).
+            # This allows pagination to be handled by callables that may attempt to load huge amounts of data into
+            # memory. Callables can use these parameters to selectively load data instead of loading everything.
+            $accepts_limit = $this->callable_accepts_param($callable, 'limit');
+            $accepts_offset = $this->callable_accepts_param($callable, 'offset');
+            $accepts_reverse = $this->callable_accepts_param($callable, 'reverse');
+            $needs_pagination = ($limit > 0 or $offset > 0 or $reverse);
+
+            if ($accepts_limit and $accepts_offset and $accepts_reverse and $needs_pagination) {
+                $internal_objects = $this->$callable(limit: $limit, offset: $offset, reverse: $reverse);
             } else {
                 $internal_objects = $this->$callable();
             }
@@ -2003,11 +2001,11 @@ class Model {
         $model_name = self::get_class_fqn();
         $model = new $model_name();
         $model_objects = [];
-        $requests_pagination = ($limit or $offset);
-        $cache_exempt = ($requests_pagination or $reverse or $model->model_cache_exempt);
+        $requests_pagination = ($limit or $offset or $reverse);
+        $cache_exempt = ($requests_pagination or $model->model_cache_exempt);
 
         # Throw an error if pagination was requested on a Model without $many enabled
-        if (!$model->many and $requests_pagination) {
+        if (!$model->many and ($limit or $offset)) {
             throw new ValidationError(
                 message: "Model `$model->verbose_name` does not support pagination. Please remove the `limit` and/or. " .
                     '`offset`parameters and try again.',
@@ -2020,70 +2018,39 @@ class Model {
             return Model::get_model_cache()::fetch_modelset($model_name);
         }
 
-        # Obtain all of this Model's internally stored objects, including those from parent Models if applicable.
-        # Pass the pagination parameters so that callables (e.g. log readers) can stop early.
-        # When $reverse is true, the caller wants the oldest entries, so we cannot pre-limit to the newest -
-        # the full dataset must be loaded to reverse correctly.
-        #
-        # For callables that support both limit and offset natively, pass both to allow efficient pagination
-        # directly at the data source level. For callables that only support limit, we pass limit+offset as
-        # the limit and then apply the offset via array_slice afterward.
-        $pagination_limit = $limit > 0 && !$reverse ? $limit : 0;
-        $pagination_offset = $offset > 0 && !$reverse ? $offset : 0;
+        # Check if the callable supports native pagination (all three parameters: limit, offset, reverse)
         $callable_handled_pagination = false;
-
-        # Check if the callable supports native offset handling
-        if ($model->internal_callable && $pagination_offset > 0) {
+        if ($model->internal_callable && $requests_pagination) {
             $callable = $model->internal_callable;
-            $callable_handles_offset = $model->callable_accepts_param($callable, 'offset');
-            $callable_handles_limit = $model->callable_accepts_param($callable, 'limit');
-
-            if ($callable_handles_offset && $callable_handles_limit) {
-                # Callable handles both - pagination will be done at source level
-                $internal_objects = $model->get_internal_objects(
-                    from_all_parents: true,
-                    limit: $pagination_limit,
-                    offset: $pagination_offset,
-                );
+            $callable_handles_all = $model->callable_accepts_param($callable, 'limit') &&
+                $model->callable_accepts_param($callable, 'offset') &&
+                $model->callable_accepts_param($callable, 'reverse');
+
+            if ($callable_handles_all) {
                 $callable_handled_pagination = true;
-            } elseif ($callable_handles_limit) {
-                # Callable only handles limit - pass limit+offset and paginate afterward
-                $internal_objects = $model->get_internal_objects(
-                    from_all_parents: true,
-                    limit: $pagination_limit + $pagination_offset,
-                    offset: 0,
-                );
-            } else {
-                # Callable handles neither - read all and paginate afterward
-                $internal_objects = $model->get_internal_objects(from_all_parents: true, limit: 0, offset: 0);
-            }
-        } else {
-            # No offset or not using internal_callable - use the standard path
-            $internal_objects = $model->get_internal_objects(
-                from_all_parents: true,
-                limit: $pagination_limit,
-                offset: $pagination_offset,
-            );
-            # If there's no internal_callable or offset is 0, check if we can skip pagination
-            if ($model->internal_callable && $pagination_limit > 0) {
-                $callable = $model->internal_callable;
-                if (
-                    $model->callable_accepts_param($callable, 'limit') &&
-                    $model->callable_accepts_param($callable, 'offset')
-                ) {
-                    $callable_handled_pagination = true;
-                }
             }
         }
 
+        # Obtain all internal objects, passing pagination params for callables that support them
+        $internal_objects = $model->get_internal_objects(
+            from_all_parents: true,
+            limit: $limit,
+            offset: $offset,
+            reverse: $reverse,
+        );
+
         # For non `many` Models, wrap the internal object in an array so we can loop
         $internal_objects = $model->many ? $internal_objects : [$internal_objects];
 
-        # Reverse the order we read the objects and/or paginate if requested
-        # Skip pagination if the callable already handled it natively
-        $internal_objects = $reverse ? array_reverse($internal_objects, preserve_keys: true) : $internal_objects;
-        if (!$callable_handled_pagination) {
-            $internal_objects = self::paginate($internal_objects, $limit, $offset, preserve_keys: true);
+        # Apply pagination and/or reverse if the callable did not handle it natively
+        if (!$callable_handled_pagination && $requests_pagination) {
+            if ($reverse) {
+                # Paginate from the beginning (oldest entries), then reverse for display
+                $internal_objects = array_reverse($internal_objects, preserve_keys: true);
+                $internal_objects = self::paginate($internal_objects, $limit, $offset, preserve_keys: true);
+            } else {
+                $internal_objects = self::paginate($internal_objects, $limit, $offset, preserve_keys: true);
+            }
         }
 
         # Loop through each internal object and create a Model object for it
@@ -2095,8 +2062,12 @@ class Model {
             $parent_id = $parent_model ? $parent_model->id : null;
             $internal_object = $parent_model ? $internal_object['_internal_object'] : $internal_object;
 
-            # Normalize IDs
+            # Normalize IDs - when the callable handled pagination natively, the array indices start
+            # at 0 but the actual IDs should account for the offset
             $internal_id = is_numeric($internal_id) ? (int) $internal_id : $internal_id;
+            if ($callable_handled_pagination && is_int($internal_id)) {
+                $internal_id += $offset;
+            }
 
             # Create a new Model object for this internal object and assign its ID
             $model_object = new $model(id: $internal_id, parent_id: $parent_id, skip_init: true);

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/ModelTraits/LogFileModelTraits.inc

@@ -317,30 +317,30 @@ trait LogFileModelTraits {
      * Reads all available logs for a given base log file path and returns the contents as an array of lines.
      * This will include rotated logs, including compressed logs.
      *
-     * When $limit > 0, reads newest log files first and stops as soon as enough lines are collected, avoiding
-     * loading the entire log history into memory. For the common case of small limits (e.g. ?limit=50), this
-     * typically only reads the current log file and skips all compressed rotated copies entirely.
+     * When $limit > 0 and $reverse is false, reads newest log files first and stops as soon as enough lines
+     * are collected, avoiding loading the entire log history into memory.
      *
-     * When $offset > 0 and $limit > 0, skips the first $offset newest entries before collecting $limit entries.
-     * This enables efficient pagination through log files without loading the entire dataset.
+     * When $reverse is true, reads from the oldest entries first (beginning of oldest log files) and
+     * paginates from there. This is useful for reading logs in chronological order.
      *
      * When $limit is 0 (default), all log files are read for full backward compatibility.
      *
      * @note zstd compressed logs are not supported.
      * @param string $base_log The base log file path.
      * @param int $limit Maximum total lines to return. 0 means unlimited (read everything).
-     * @param int $offset Number of newest entries to skip before collecting. Only used when $limit > 0.
+     * @param int $offset Number of entries to skip before collecting.
+     * @param bool $reverse When true, read from oldest entries first; when false, read from newest first.
      * @return array An array of all log file contents for the given base log.
      */
-    public function read_log(string $base_log, int $limit = 0, int $offset = 0): array {
+    public function read_log(string $base_log, int $limit = 0, int $offset = 0, bool $reverse = false): array {
         # Ensure the base log file exists
         $this->check_file_exists($base_log);
 
-        # Gather all log file paths (ordered newest-first for bounded reads)
+        # Gather all log file paths (ordered newest-first)
         $log_filepaths = $this->gather_log_filepaths($base_log);
 
-        # Unbounded read: preserve existing behavior exactly (read oldest-first, return all)
-        if ($limit <= 0) {
+        # Unbounded read: read all entries
+        if ($limit <= 0 && $offset <= 0) {
             $log_contents = [];
 
             # Read oldest-first (reverse of our newest-first ordering)
@@ -364,19 +364,37 @@ trait LogFileModelTraits {
             $log_contents = array_filter($log_contents);
             $log_contents = array_values($log_contents);
 
+            # Reverse if requested (show newest first)
+            if ($reverse) {
+                $log_contents = array_reverse($log_contents);
+            }
+
             # Map the log contents so each entry is an object with a 'text' property
             return array_map(fn($line) => ['text' => $line], $log_contents);
         }
 
-        # Bounded read with offset: collect lines from newest files first, skip $offset entries,
-        # then collect $limit entries. This is the key optimization - for small limits with offset,
-        # we typically only read the current log file and skip all compressed rotated copies entirely.
+        # Handle reverse pagination (reading from oldest entries first)
+        if ($reverse) {
+            return $this->read_log_reverse($log_filepaths, $limit, $offset);
+        }
+
+        # Normal pagination: read from newest entries first
+        return $this->read_log_forward($log_filepaths, $limit, $offset);
+    }
+
+    /**
+     * Reads log entries starting from the newest, with pagination support.
+     * @param array $log_filepaths Log file paths ordered newest-first.
+     * @param int $limit Maximum entries to return.
+     * @param int $offset Number of newest entries to skip.
+     * @return array Log entries with 'text' property.
+     */
+    private function read_log_forward(array $log_filepaths, int $limit, int $offset): array {
         $collected = [];
-        $skipped = 0;
         $total_needed = $limit + $offset;
 
         foreach ($log_filepaths as $log_filepath) {
-            $remaining = $total_needed - count($collected) - $skipped;
+            $remaining = $total_needed - count($collected);
             if ($remaining <= 0) {
                 break;
             }
@@ -399,24 +417,16 @@ trait LogFileModelTraits {
             }
         }
 
-        # Apply offset: skip the first $offset entries from the newest (end of array)
-        # Since $collected is in chronological order (oldest first), we need to:
-        # 1. Take the newest entries (from the end)
-        # 2. Skip the first $offset of those
-        # 3. Return up to $limit entries
+        # Apply offset: skip the newest $offset entries (from the end)
+        # $collected is in chronological order (oldest first), newest at end
         if (count($collected) > $total_needed) {
-            # We have more than we need, slice to get exactly what we want
             $collected = array_slice($collected, -$total_needed);
         }
 
-        # Now apply offset by taking entries after skipping the newest $offset entries
-        # The newest entries are at the end, so we slice from position 0 to (count - offset)
-        # then take the last $limit of those
         if ($offset > 0 && count($collected) > $offset) {
             # Remove the newest $offset entries (from the end)
             $collected = array_slice($collected, 0, -$offset);
         } elseif ($offset > 0) {
-            # Offset is larger than what we have, return empty
             $collected = [];
         }
 
@@ -429,4 +439,65 @@ trait LogFileModelTraits {
         $collected = array_values(array_filter($collected));
         return array_map(fn($line) => ['text' => $line], $collected);
     }
+
+    /**
+     * Reads log entries starting from the oldest, with pagination support.
+     * Returns entries in reverse chronological order (newest first within the page).
+     * @param array $log_filepaths Log file paths ordered newest-first.
+     * @param int $limit Maximum entries to return.
+     * @param int $offset Number of oldest entries to skip.
+     * @return array Log entries with 'text' property, in reverse chronological order.
+     */
+    private function read_log_reverse(array $log_filepaths, int $limit, int $offset): array {
+        $collected = [];
+        $total_needed = $limit + $offset;
+        $skipped = 0;
+
+        # Read oldest-first (reverse of newest-first ordering)
+        foreach (array_reverse($log_filepaths) as $log_filepath) {
+            if (count($collected) >= $total_needed) {
+                break;
+            }
+
+            $type = $this->get_log_type($log_filepath);
+
+            # Read the entire file (we need to read from the beginning for oldest-first)
+            $lines = match ($type) {
+                'bz2' => $this->read_bzip2_log($log_filepath),
+                'gz' => $this->read_gzip_log($log_filepath),
+                'xz' => $this->read_xz_log($log_filepath),
+                'log' => $this->read_uncompressed_log($log_filepath),
+                default => throw new NotAcceptableError(
+                    message: "The log file at $log_filepath has an unsupported file extension.",
+                    response_id: 'LOG_FILE_TRAITS_UNSUPPORTED_LOG_FILE_EXTENSION',
+                ),
+            };
+
+            # Filter empty lines
+            $lines = array_filter($lines, fn($line) => $line !== '');
+
+            foreach ($lines as $line) {
+                # Skip the first $offset entries
+                if ($skipped < $offset) {
+                    $skipped++;
+                    continue;
+                }
+
+                # Collect until we have $limit entries
+                if (count($collected) < $limit) {
+                    $collected[] = $line;
+                }
+
+                if (count($collected) >= $limit) {
+                    break 2;
+                }
+            }
+        }
+
+        # Reverse the collected entries so newest (within the page) comes first
+        $collected = array_reverse($collected);
+
+        # Wrap in ['text' => ...] format
+        return array_map(fn($line) => ['text' => $line], $collected);
+    }
 }

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/AuthLog.inc

@@ -32,10 +32,11 @@ class AuthLog extends Model {
     /**
      * Obtains the auth log as an array. This method is the internal callable for this Model.
      * @param int $limit Maximum entries to return (0 = all). Passed through from pagination.
-     * @param int $offset Number of newest entries to skip before collecting. Passed through from pagination.
+     * @param int $offset Number of entries to skip. Passed through from pagination.
+     * @param bool $reverse Whether to read from oldest entries first.
      * @return array The auth log as an array of objects.
      */
-    protected function get_auth_log(int $limit = 0, int $offset = 0): array {
-        return $this->read_log($this->log_file, $limit, $offset);
+    protected function get_auth_log(int $limit = 0, int $offset = 0, bool $reverse = false): array {
+        return $this->read_log($this->log_file, $limit, $offset, $reverse);
     }
 }