Commit 75fbca7a authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix(financial): stop binding NULL as a filter Postgres cannot type

The financial overview 500'd with SQLSTATE 42P08 on `($4 IS NULL OR
p.branch_id = $4)`. Postgres fixes each prepared-statement parameter's type
during parse analysis, and `:branch_id IS NULL` gives it nothing to work from
— the statement is rejected before it ever reaches the comparison that would
have typed it. `:academy_id` was the same shape and would have failed next.

The idiom came in with 35200985 and could not be caught here: phpunit runs
SQLite in memory, which types placeholders at bind time and executes the
broken form happily.

Fixed by appending the branch and academy filters only when they apply, with
their bindings, rather than passing NULL as a sentinel — which is what the
->when() filters in the same method already do, and keeps the
(academy_id, branch_id) index usable instead of hiding it behind an OR.

The SQL build is extracted to buildTopProgramsQuery() so it can be asserted on
without a database. The test pins four things: the placeholders and the
bindings agree in all four filter combinations, the clauses are omitted rather
than nulled, the built SQL executes, and no raw SQL under app/ binds a
placeholder as a NULL sentinel again. That last one is a source scan on
purpose — the suite's driver is not the production driver, so it cannot
observe this failure by running.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent 6d59b789
......@@ -477,6 +477,48 @@ private function getRevenueBreakdown($from, $to): array
->value(DB::raw('SUM(payments.amount * prod_items.product_total / invoices.subtotal_amount)')) ?? 0;
// Top programs by revenue
[$topProgramsSql, $topProgramsBindings] = $this->buildTopProgramsQuery($from, $to, $academyId);
$topPrograms = DB::select($topProgramsSql, $topProgramsBindings);
// Per-product revenue breakdown
$byProduct = DB::table('payments')
->join('invoices', 'payments.invoice_id', '=', 'invoices.id')
->join('invoice_items', fn ($j) => $j->on('invoice_items.invoice_id', '=', 'invoices.id')->where('invoice_items.itemable_type', '=', $productType))
->join('products', 'invoice_items.itemable_id', '=', 'products.id')
->where('payments.status', 'confirmed')
->where('payments.direction', 'inbound')
->where('invoices.subtotal_amount', '>', 0)
->whereBetween('payments.payment_date', [$from, $to])
->when($this->branch_id, fn ($q) => $q->where('payments.branch_id', $this->branch_id))
->when($academyId, fn ($q) => $q->where('payments.academy_id', $academyId))
->select(
'products.id as product_id',
'products.name_ar as product_name',
DB::raw('SUM(payments.amount * invoice_items.total_amount / invoices.subtotal_amount) as total'),
DB::raw('SUM(invoice_items.quantity) as units_sold')
)
->groupBy('products.id', 'products.name_ar')
->orderByDesc('total')
->limit(10)
->get()
->toArray();
return [
'subscription_revenue' => $subscriptionRevenue,
'product_revenue' => $productRevenue,
'top_programs' => $topPrograms,
'by_product' => $byProduct,
];
}
/**
* Build the top-programs query and its bindings.
*
* Split out from getRevenueBreakdown() so the SQL and its bindings can be
* asserted on without a database connection.
*/
private function buildTopProgramsQuery($from, $to, ?int $academyId): array
{
// Attributing subscription money to a programme is only exact when the
// enrolment records which invoice billed it — and in practice most do
// not (88 of 350 on the live instance). So: use the exact link where it
......@@ -491,7 +533,32 @@ private function getRevenueBreakdown($from, $to): array
//
// Written as raw SQL because the CTE + UNION shape does not express
// cleanly through the query builder; every value is bound.
$topProgramsSql = <<<'SQL'
//
// Postgres decides a prepared-statement parameter's type during parse
// analysis, and `:branch_id IS NULL` gives it nothing to work from: the
// statement is rejected with 42P08 before it ever reaches the comparison
// that would have typed it. So the optional filters are appended only
// when they apply, the way the ->when() filters in getRevenueBreakdown()
// do — never bound as a NULL the driver has to guess a type for.
$bindings = [
'billable' => Participant::class,
'from' => $from,
'to' => $to,
];
$filters = '';
if ($branchId = (int) $this->branch_id) {
$filters .= "\n AND p.branch_id = :branch_id";
$bindings['branch_id'] = $branchId;
}
if ($academyId) {
$filters .= "\n AND p.academy_id = :academy_id";
$bindings['academy_id'] = $academyId;
}
$topProgramsSql = <<<SQL
WITH sub AS (
SELECT invoice_id, SUM(total_amount) AS sub_total
FROM invoice_items
......@@ -525,51 +592,13 @@ private function getRevenueBreakdown($from, $to): array
WHERE p.status = 'confirmed'
AND p.direction = 'inbound'
AND i.subtotal_amount > 0
AND p.payment_date BETWEEN :from AND :to
AND (:branch_id IS NULL OR p.branch_id = :branch_id)
AND (:academy_id IS NULL OR p.academy_id = :academy_id)
AND p.payment_date BETWEEN :from AND :to{$filters}
GROUP BY tp.id, tp.name_ar
ORDER BY total DESC
LIMIT 10
SQL;
$topPrograms = DB::select($topProgramsSql, [
'billable' => Participant::class,
'from' => $from,
'to' => $to,
'branch_id' => $this->branch_id ?: null,
'academy_id' => $academyId,
]);
// Per-product revenue breakdown
$byProduct = DB::table('payments')
->join('invoices', 'payments.invoice_id', '=', 'invoices.id')
->join('invoice_items', fn ($j) => $j->on('invoice_items.invoice_id', '=', 'invoices.id')->where('invoice_items.itemable_type', '=', $productType))
->join('products', 'invoice_items.itemable_id', '=', 'products.id')
->where('payments.status', 'confirmed')
->where('payments.direction', 'inbound')
->where('invoices.subtotal_amount', '>', 0)
->whereBetween('payments.payment_date', [$from, $to])
->when($this->branch_id, fn ($q) => $q->where('payments.branch_id', $this->branch_id))
->when($academyId, fn ($q) => $q->where('payments.academy_id', $academyId))
->select(
'products.id as product_id',
'products.name_ar as product_name',
DB::raw('SUM(payments.amount * invoice_items.total_amount / invoices.subtotal_amount) as total'),
DB::raw('SUM(invoice_items.quantity) as units_sold')
)
->groupBy('products.id', 'products.name_ar')
->orderByDesc('total')
->limit(10)
->get()
->toArray();
return [
'subscription_revenue' => $subscriptionRevenue,
'product_revenue' => $productRevenue,
'top_programs' => $topPrograms,
'by_product' => $byProduct,
];
return [$topProgramsSql, $bindings];
}
private function getFinancialMetrics($from, $to, array $revenue, array $expenses, array $collectionRate): array
......
<?php
namespace Tests\Feature;
use App\Livewire\Financial\FinancialOverview;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use ReflectionMethod;
use Tests\TestCase;
/**
* The financial overview's top-programs query is raw SQL against Postgres.
*
* Postgres fixes each prepared-statement parameter's type during parse
* analysis, so a placeholder whose only typing context is `:x IS NULL` is
* rejected with SQLSTATE 42P08 — the statement never reaches the comparison
* that would have typed it. The optional branch/academy filters must therefore
* be appended only when they apply, never bound as a NULL sentinel.
*
* The suite runs on SQLite, which types placeholders at bind time and would
* happily execute the broken form; the guard below is a source scan for that
* reason, and the execution tests prove the SQL and its bindings agree.
*/
class FinancialTopProgramsQueryTest extends TestCase
{
private function buildQuery(string $branchId, ?int $academyId): array
{
$component = new FinancialOverview();
$component->branch_id = $branchId;
$method = new ReflectionMethod($component, 'buildTopProgramsQuery');
$method->setAccessible(true);
return $method->invoke($component, '2026-01-01', '2026-12-31', $academyId);
}
public function test_no_raw_sql_binds_a_placeholder_as_a_null_sentinel(): void
{
$offenders = [];
$files = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator(app_path()));
foreach ($files as $file) {
if ($file->getExtension() !== 'php') {
continue;
}
foreach (file($file->getPathname()) as $i => $line) {
if (preg_match('/[(\s](:\w+|\?)\s+IS\s+NULL\s+OR/i', $line)) {
$offenders[] = $file->getPathname() . ':' . ($i + 1) . ' ' . trim($line);
}
}
}
$this->assertSame([], $offenders, implode("\n", array_merge(
['Bound placeholders used as NULL sentinels — Postgres cannot type these (42P08):'],
$offenders
)));
}
public function test_every_placeholder_in_the_query_has_a_binding_and_every_binding_is_used(): void
{
$cases = [
'no filters' => ['', null],
'branch only' => ['3', null],
'academy only' => ['', 7],
'both filters' => ['3', 7],
];
foreach ($cases as $label => [$branchId, $academyId]) {
[$sql, $bindings] = $this->buildQuery($branchId, $academyId);
preg_match_all('/:(\w+)/', $sql, $matches);
$placeholders = array_values(array_unique($matches[1]));
$bound = array_keys($bindings);
sort($placeholders);
sort($bound);
$this->assertSame($placeholders, $bound, "[{$label}] placeholders and bindings disagree; PDO would reject the statement.");
}
}
public function test_the_branch_filter_is_omitted_rather_than_bound_as_null(): void
{
[$sql, $bindings] = $this->buildQuery('', null);
$this->assertStringNotContainsString('branch_id', $sql);
$this->assertStringNotContainsString('academy_id', $sql);
$this->assertArrayNotHasKey('branch_id', $bindings);
$this->assertArrayNotHasKey('academy_id', $bindings);
[$sql, $bindings] = $this->buildQuery('3', 7);
$this->assertStringContainsString('AND p.branch_id = :branch_id', $sql);
$this->assertStringContainsString('AND p.academy_id = :academy_id', $sql);
$this->assertSame(3, $bindings['branch_id']);
$this->assertSame(7, $bindings['academy_id']);
}
public function test_the_built_query_executes_with_its_bindings(): void
{
$this->createMinimalSchema();
foreach ([['', null], ['3', null], ['', 7], ['3', 7]] as [$branchId, $academyId]) {
[$sql, $bindings] = $this->buildQuery($branchId, $academyId);
$this->assertIsArray(DB::select($sql, $bindings));
}
}
private function createMinimalSchema(): void
{
Schema::create('invoice_items', function (Blueprint $table) {
$table->id();
$table->unsignedBigInteger('invoice_id');
$table->string('itemable_type')->nullable();
$table->bigInteger('total_amount');
});
Schema::create('invoices', function (Blueprint $table) {
$table->id();
$table->string('billable_type')->nullable();
$table->unsignedBigInteger('billable_id')->nullable();
$table->bigInteger('subtotal_amount')->default(0);
});
Schema::create('enrollments', function (Blueprint $table) {
$table->id();
$table->unsignedBigInteger('invoice_id')->nullable();
$table->unsignedBigInteger('participant_id')->nullable();
$table->unsignedBigInteger('training_program_id')->nullable();
});
Schema::create('payments', function (Blueprint $table) {
$table->id();
$table->unsignedBigInteger('invoice_id')->nullable();
$table->unsignedBigInteger('branch_id')->nullable();
$table->unsignedBigInteger('academy_id')->nullable();
$table->bigInteger('amount')->default(0);
$table->string('status')->default('confirmed');
$table->string('direction')->default('inbound');
$table->date('payment_date')->nullable();
});
Schema::create('training_programs', function (Blueprint $table) {
$table->id();
$table->string('name_ar')->nullable();
});
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment