Skip to content

Support parameters #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 17, 2024
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
.php_cs
.php_cs.cache
.phpunit.result.cache
.phpunit.cache
build
composer.lock
coverage
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ This package works as follows:
- `team_id`: The team id associated with the request (if available)
- `method`: The HTTP method (`GET/POST/...`)
- `route`: The route name (if available) or the route URI (eg `/posts/{post}`)
- `parameters`: The route parameters passed (if enabled else `null`)
- `status`: The HTTP status (eg `202`)
- `ip`: The request ip
- `date`: The date of the request as datetime (can be aggregated)
Expand Down
10 changes: 10 additions & 0 deletions config/route-statistics.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@
*/
'enabled' => env('ROUTE_STATISTICS_ENABLED', true),

/*
|--------------------------------------------------------------------------
| Store parameters
|--------------------------------------------------------------------------
|
| If this setting is set to true the route parameters will also be logged.
|
*/
'store_parameters' => env('ROUTE_STORE_PARAMETERS', false),

/*
|--------------------------------------------------------------------------
| Aggregation
Expand Down
1 change: 1 addition & 0 deletions database/factories/RouteStatisticFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public function definition()
'method' => $this->faker->randomElement(['GET', 'POST', 'PUT', 'PATCH', 'DELETE']),
'route' => $this->faker->domainWord().'.'.$this->faker->randomElement(['index', 'create', 'store', 'show', 'edit', 'update', 'destroy']),
'status' => $this->faker->randomElement([200, 201, 202, 204, 300, 301, 302, 303, 304, 400, 401, 402, 403, 404, 405, 406, 422, 429, 500, 501, 502, 503, 504]),
'parameters' => $this->faker->json(),
'ip' => $this->faker->ipv4(),
'date' => $this->faker->dateTime(),
'counter' => $this->faker->randomNumber(4),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
public function up()
{
Schema::table('route_statistics', function (Blueprint $table) {
$table->json('parameters')->after('route')->nullable();
});
}

public function down()
{
Schema::table('route_statistics', function (Blueprint $table) {
$table->dropColumn('parameters');
});
}
};
10 changes: 7 additions & 3 deletions src/Commands/LaravelRouteStatisticsCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ class LaravelRouteStatisticsCommand extends Command
public function handle()
{
$query = $this->getQuery();
$fields = $this->getFields();

if ($this->option('group')) {
$query->select($this->option('group'))
->addSelect(DB::raw('MAX(date) as last_used'))
->addSelect(DB::raw('SUM(counter) as counter'));
} else {
$query->select($fields);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that this is in theory a small performance improvement, but it does break if you start adding fields that are not nessesarely DB fields but accessors.

Suggested change
} else {
$query->select($fields);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't include it for the performance it was rather that I used a selector that was not in the table. I have a local change that splits up successors (e.g. for grouping) and real column names. In this change I also did some checks if other options where valid. Since it was out of scope of this PR I didn't add it. I will revert this to the original code.

Copy link
Owner

@bilfeldt bilfeldt Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is where you ment to post this @svenbw :)

As it turns out console table doesn't like an array, so I need to map each table element. And I now remember WHY the select is in place

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->map(function(RouteStatistic $item) {
        $data = $item->toArray();
        if (config('route-statistics.store_parameters')) {
            $data['parameters'] = json_encode($data['parameters']);
        } else {
            unset($data['parameters']); // Hide parameters if not supported in config
        }
        return $data;
    });

Instead of only selecting the fields in the query, then I suggest that we just pluck those fields AFTER query execution (which will then include all fields). That way it would work with accessors as well + solve your issue with arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgetting the parameters key by looping over the collection works if parameters is not enabled. However if they are still there it still needs to be casted to a string for table.

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->map(function (RouteStatistic $item) {
        $data = $item->toArray();
        if (config('route-statistics.store_parameters')) {
            $data['parameters'] = json_encode($data['parameters']);
        } else {
            unset($data['parameters']);
        }
        return $data;
    });

or

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->toArray();

if (config('route-statistics.store_parameters')) {
    Arr::map($results, function (array $item) {
        $item['parameters'] = json_encode($item['parameters']);
    });
} else {
    $results = Arr::map($results, function (array $item) {
        unset($item['parameters']);
        return $item;
    });
}

This is less efficient because php has to loop though each entry, else the data is collected by the database.

Can you explain why you would include other columns that are not in the fields? The table will be faulty because the headers of the table are generated by the getFields() function. If there is an additional column in the database the table will included empty headers and depending on the order even a field name above another column.

And @bilfeldt thanks for keeping the discussion going, I think we are getting there.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like:

$results = $query
    ->limit($this->option('limit'))
    ->get()
    ->pluck($fields)
    ->map(function (array $data): array {
        return array_map(function ($item): string {
            if (is_array($item)) {
                return json_encode($item);
            }

            return (string) $data;
        }, $data);
    }) 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pluck doesn't work, since it will only pluck items with specific keys from an array, and at that point there are still rows.
I filtered each row with only which will eliminate the parameters column.

I think the idea of pluck is the same as using select, where select has the benefit of offloading the complexity to the database, and thus slightly more performant.

}

$this->applyFilters($query);
Expand All @@ -43,7 +46,7 @@ public function handle()
$results = $query->limit($this->option('limit'))->get();

$this->table(
$this->getFields(),
$fields,
$results->toArray()
);

Expand Down Expand Up @@ -111,16 +114,17 @@ protected function getFields(): array
return array_merge($this->option('group'), ['last_used', 'counter']);
}

return [
return array_filter([
'id',
'user_id',
'team_id',
'method',
'route',
'status',
config('route-statistics.store_parameters') === true ? 'parameters' : null,
'ip',
'date',
'counter',
];
]);
}
}
26 changes: 26 additions & 0 deletions src/Commands/LaravelRouteTruncateCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace Bilfeldt\LaravelRouteStatistics\Commands;

use Illuminate\Console\Command;
use TypeError;

class LaravelRouteTruncateCommand extends Command
{
public $signature = 'route:truncate-stats';

public $description = 'Prune route usage statistics';

public function handle(): int
{
try {
call_user_func_array([config('route-statistics.model'), 'truncate'], []);
} catch (TypeError $ex) {
$this->components->error('Failed to truncate route usage statistics: '.$ex->getMessage());
}

$this->components->info('Route usage statistics truncated');

return Command::SUCCESS;
}
}
1 change: 0 additions & 1 deletion src/Http/Middleware/RouteStatisticsMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ class RouteStatisticsMiddleware
* Handle an incoming request.
*
* @param \Illuminate\Http\Request $request
* @param \Closure $next
* @return mixed
*/
public function handle($request, Closure $next)
Expand Down
3 changes: 3 additions & 0 deletions src/LaravelRouteStatisticsServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Bilfeldt\LaravelRouteStatistics;

use Bilfeldt\LaravelRouteStatistics\Commands\LaravelRouteStatisticsCommand;
use Bilfeldt\LaravelRouteStatistics\Commands\LaravelRouteTruncateCommand;
use Bilfeldt\LaravelRouteStatistics\Commands\LaravelRouteUnusedCommand;
use Bilfeldt\LaravelRouteStatistics\Http\Middleware\RouteStatisticsMiddleware;
use Bilfeldt\RequestLogger\RequestLoggerFacade;
Expand Down Expand Up @@ -60,6 +61,7 @@ private function publishMigrations()
{
$this->publishes([
__DIR__.'/../database/migrations/create_route_statistics_table.php.stub' => database_path('migrations/'.date('Y_m_d_His', time()).'_create_route_statistics_table.php'),
__DIR__.'/../database/migrations/add_parameters_to_route_statistics_table.php.stub' => database_path('migrations/'.date('Y_m_d_His', time()).'_add_parameters_to_route_statistics_table.php'),
// you can add any number of migrations here
], 'migrations');
}
Expand All @@ -69,6 +71,7 @@ private function bootCommands()
if ($this->app->runningInConsole()) {
$this->commands([
LaravelRouteStatisticsCommand::class,
LaravelRouteTruncateCommand::class,
LaravelRouteUnusedCommand::class,
]);
}
Expand Down
1 change: 1 addition & 0 deletions src/Models/RouteStatistic.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public function getLogAttributes(Request $request, $response, ?int $time = null,
'team_id' => $this->getRequestTeam($request)?->getKey(),
'method' => $request->getMethod(),
'route' => $request->route()?->getName() ?? $request->route()?->uri(),
'parameters' => config('route-statistics.store_parameters') ? json_encode($request->route()->originalParameters()) : null,
'status' => $response->getStatusCode(),
'ip' => $request->ip(),
'date' => $this->getDate(),
Expand Down
14 changes: 12 additions & 2 deletions tests/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,17 @@ public function getEnvironmentSetUp($app)
'prefix' => '',
]);

$migration = include __DIR__.'/../database/migrations/create_route_statistics_table.php.stub';
$migration->up();
$this->runMigrations([
'create_route_statistics_table',
'add_parameters_to_route_statistics_table',
]);
}

private function runMigrations(array $fileNames): void
{
foreach ($fileNames as $fileName) {
$class = require __DIR__.'/../database/migrations/'.$fileName.'.php.stub';
$class->up();
}
}
}
48 changes: 48 additions & 0 deletions tests/Unit/RouteStatisticModelTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,52 @@ public function test_get_log_attributes(): void
{
$this->markTestIncomplete('Mock the request and response to ensture the correct attributes are returned');
}

public function test_logs_parameters_if_config_enabled(): void
{
Config::set('route-statistics.store_parameters', true);

$route = 'home';
$params = [
'param1' => 'one',
'param2' => 'two',
];
$request = \Illuminate\Http\Request::create($route.'/'.implode('/', $this->get_route_parameters($params)), 'GET');
$this->app['router']->get($route.'/'.implode('/', $this->get_route_keys($params)), fn () => 'Test route response');
$response = $this->app['router']->dispatch($request);

(new RouteStatistic)->log($request, $response, 1, 2);

$log = RouteStatistic::first();
$this->assertEquals(json_encode($params), $log->parameters);
}

public function test_logs_parameters_if_config_disabled(): void
{
Config::set('route-statistics.store_parameters', false);

$route = 'home';
$params = [
'param1' => 'one',
'param2' => 'two',
];
$request = \Illuminate\Http\Request::create($route.'/'.implode('/', $this->get_route_parameters($params)), 'GET');
$this->app['router']->get($route.'/'.implode('/', $this->get_route_keys($params)), fn () => 'Test route response');
$response = $this->app['router']->dispatch($request);

(new RouteStatistic)->log($request, $response, 1, 2);

$log = RouteStatistic::first();
$this->assertNull($log->parameters);
}

private function get_route_parameters(array $parameters): array
{
return array_values($parameters);
}

private function get_route_keys(array $parameters): array
{
return array_map(fn ($parameter) => '{'.$parameter.'}', array_keys($parameters));
}
}
Loading