Skip to content

Commit 01d98ec

Browse files
rwdaigleclaude
andauthored
Add sql<number> aggregate type assertion check (#40)
* Add sql<number> aggregate type assertion check to persistence check Detects sql<number> tagged templates containing aggregate functions (SUM, AVG, COUNT, MIN, MAX). Databases return these as strings, creating silent runtime type mismatches. Severity: error, category: drizzle. Includes test fixture with 5 bad patterns and 4 safe patterns, comprehensive test coverage with 7 new tests, all 213 tests passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Bump to 0.10.0, update docs for sql<number> aggregate check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e3ff7ba commit 01d98ec

9 files changed

Lines changed: 302 additions & 4 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ Static analysis and functional verifier tool for React Router applications. CLI
77
- `src/cli.ts` - CLI entry point
88
- `src/analyzer.ts` - Main analysis orchestration
99
- `src/parsers/` - AST parsers for routes, components, and actions
10-
- `src/checks/` - Individual check implementations (links, forms, loader, params, hydration, interactivity, persistence)
10+
- `src/checks/` - Individual check implementations (links, forms, loader, params, hydration, interactivity, drizzle/persistence)
1111
- `src/sql/` - SQL query extraction from ORM code (drizzle.ts, drizzle-operations.ts, types.ts)
1212
- `src/sql-extractor.ts` - SQL extraction orchestration and formatting
1313
- `src/types.ts` - Type definitions

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ Point `rr` at your project and it scans your route files for issues. With no arg
3131
- **interactivity** (default) -- catches "Save" buttons that don't save, "Delete" buttons that don't delete, and empty click handlers
3232
- **hydration** (default) -- detects SSR/client mismatches from `new Date()`, `Math.random()`, locale-dependent formatting, `window` access in render
3333
- **forms** (default) -- validates `<Form>` targets have actions and that field names match `formData.get()` calls
34-
- **drizzle** (opt-in) -- validates Drizzle ORM operations against your schema (missing columns, type mismatches, etc.)
34+
- **drizzle** (opt-in) -- validates Drizzle ORM operations against your schema (missing columns, type mismatches, `sql<number>` with aggregates, etc.)
3535

3636
**Example output:**
3737

SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ rr sql --drizzle --format json # SQL extraction with JSON output
5656

5757
**Optional checks** (opt-in via `--check`):
5858

59-
- **drizzle** - validates database operations against Drizzle ORM schema (unknown tables/columns, missing required columns, null-to-notNull, invalid enum literals, type mismatches, auto-generated column writes, DELETE/UPDATE without WHERE)
59+
- **drizzle** - validates database operations against Drizzle ORM schema (unknown tables/columns, missing required columns, null-to-notNull, invalid enum literals, type mismatches, auto-generated column writes, DELETE/UPDATE without WHERE, `sql<number>` with aggregates)
6060

6161
## Examples
6262

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "roto-rooter",
3-
"version": "0.9.0",
3+
"version": "0.10.0",
44
"description": "Static analysis and functional verifier tool for React Router applications",
55
"main": "./dist/index.js",
66
"types": "./dist/index.d.ts",

src/checks/persistence-check.ts

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import * as fs from 'fs';
2+
import ts from 'typescript';
13
import type {
24
AnalyzerIssue,
35
DrizzleSchema,
@@ -14,6 +16,7 @@ import {
1416
isNumericColumn,
1517
isEnumColumn,
1618
} from '../parsers/drizzle-schema-parser.js';
19+
import { parseFile, walkAst, getLineAndColumn } from '../utils/ast-utils.js';
1720

1821
/**
1922
* Check persistence operations against Drizzle schema
@@ -37,6 +40,9 @@ export function checkPersistence(
3740
}
3841
}
3942

43+
// Check for aggregate type assertion issues (sql<number> with COUNT/SUM/etc.)
44+
issues.push(...checkAggregateTypeAssertions(files));
45+
4046
return issues;
4147
}
4248

@@ -407,3 +413,112 @@ function checkTypeMismatch(
407413

408414
return undefined;
409415
}
416+
417+
// ============================================================================
418+
// Aggregate Type Assertion Check
419+
// ============================================================================
420+
421+
const AGGREGATE_PATTERN = /\b(SUM|AVG|COUNT|MIN|MAX)\s*\(/i;
422+
423+
/**
424+
* Check for sql<number> tagged templates containing aggregate functions.
425+
* Databases return aggregate results as strings, so sql<number>`COUNT(*)`
426+
* will silently return "123" instead of 123.
427+
*/
428+
function checkAggregateTypeAssertions(files: string[]): AnalyzerIssue[] {
429+
const issues: AnalyzerIssue[] = [];
430+
431+
for (const file of files) {
432+
try {
433+
const content = fs.readFileSync(file, 'utf-8');
434+
const sourceFile = parseFile(file, content);
435+
436+
walkAst(sourceFile, (node) => {
437+
if (!ts.isTaggedTemplateExpression(node)) return;
438+
439+
const tag = node.tag;
440+
if (!ts.isIdentifier(tag) || tag.text !== 'sql') return;
441+
442+
const typeArgs = node.typeArguments;
443+
if (!typeArgs || typeArgs.length === 0) return;
444+
445+
if (!typeContainsNumber(typeArgs[0])) return;
446+
447+
const templateText = extractRawTemplateText(node.template);
448+
if (!templateText) return;
449+
450+
const match = templateText.match(AGGREGATE_PATTERN);
451+
if (!match) return;
452+
453+
const aggFunc = match[1].toUpperCase();
454+
const loc = getLineAndColumn(sourceFile, node.getStart(sourceFile));
455+
const snippet = content
456+
.slice(node.getStart(sourceFile), node.getEnd())
457+
.replace(/\s+/g, ' ')
458+
.trim();
459+
460+
issues.push({
461+
category: 'drizzle',
462+
severity: 'error',
463+
message: `sql<number> with ${aggFunc}() -- database returns string, not number`,
464+
location: { file, line: loc.line, column: loc.column },
465+
code: snippet.length > 120 ? snippet.slice(0, 117) + '...' : snippet,
466+
suggestion: `Cast the result: Number(result) or use sql<string> and convert explicitly`,
467+
});
468+
});
469+
} catch {
470+
// Skip unparseable files
471+
}
472+
}
473+
474+
return issues;
475+
}
476+
477+
/**
478+
* Check if a TypeNode contains the `number` type
479+
*/
480+
function typeContainsNumber(typeNode: ts.TypeNode): boolean {
481+
if (typeNode.kind === ts.SyntaxKind.NumberKeyword) {
482+
return true;
483+
}
484+
485+
if (ts.isUnionTypeNode(typeNode)) {
486+
return typeNode.types.some((t) => typeContainsNumber(t));
487+
}
488+
489+
if (ts.isIntersectionTypeNode(typeNode)) {
490+
return typeNode.types.some((t) => typeContainsNumber(t));
491+
}
492+
493+
if (ts.isTypeLiteralNode(typeNode)) {
494+
for (const member of typeNode.members) {
495+
if (ts.isPropertySignature(member) && member.type) {
496+
if (typeContainsNumber(member.type)) {
497+
return true;
498+
}
499+
}
500+
}
501+
}
502+
503+
return false;
504+
}
505+
506+
/**
507+
* Extract raw text from a template literal (head + all span literals)
508+
*/
509+
function extractRawTemplateText(
510+
template: ts.TemplateLiteral
511+
): string | undefined {
512+
if (ts.isNoSubstitutionTemplateLiteral(template)) {
513+
return template.text;
514+
}
515+
if (ts.isTemplateExpression(template)) {
516+
let result = template.head.text;
517+
for (const span of template.templateSpans) {
518+
result += '___';
519+
result += span.literal.text;
520+
}
521+
return result;
522+
}
523+
return undefined;
524+
}

test/fixtures/sample-app/app/routes.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,5 +53,7 @@ export default [
5353
route('/component-links', 'routes/component-links.tsx'),
5454
// Raw SQL tagged template literal test
5555
route('/sql-raw-subquery', 'routes/sql-raw-subquery.tsx'),
56+
// Aggregate type assertion test
57+
route('/sql-aggregate-types', 'routes/sql-aggregate-types.tsx'),
5658
]),
5759
] satisfies RouteConfig;
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
// Test fixture: sql<number> with aggregate functions (type assertion bug)
2+
// Databases return aggregate results as strings, so sql<number> with
3+
// COUNT/SUM/AVG/MIN/MAX silently returns "123" instead of 123.
4+
5+
import { db } from '~/db';
6+
import { orders } from '~/db/schema';
7+
import { sql } from 'drizzle-orm';
8+
9+
export async function loader() {
10+
// BAD: sql<number> with COUNT -- database returns string
11+
const withCount = await db
12+
.select({
13+
userId: orders.userId,
14+
orderCount: sql<number>`COUNT(${orders.id})`,
15+
})
16+
.from(orders)
17+
.groupBy(orders.userId);
18+
19+
// BAD: sql<number> with SUM
20+
const withSum = await db
21+
.select({
22+
userId: orders.userId,
23+
totalSpent: sql<number>`SUM(${orders.total})`,
24+
})
25+
.from(orders)
26+
.groupBy(orders.userId);
27+
28+
// BAD: sql<number> with AVG
29+
const withAvg = await db
30+
.select({
31+
avgTotal: sql<number>`AVG(${orders.total})`,
32+
})
33+
.from(orders);
34+
35+
// BAD: sql<number | null> with MIN (union type containing number)
36+
const withMin = await db
37+
.select({
38+
minTotal: sql<number | null>`MIN(${orders.total})`,
39+
})
40+
.from(orders);
41+
42+
// BAD: sql<number> with MAX in db.execute
43+
const withMax = await db.execute(
44+
sql<number>`SELECT MAX(${orders.total}) FROM orders`
45+
);
46+
47+
// GOOD: sql<string> with COUNT -- developer knows it returns string
48+
const countAsString = await db
49+
.select({
50+
orderCount: sql<string>`COUNT(${orders.id})`,
51+
})
52+
.from(orders)
53+
.groupBy(orders.userId);
54+
55+
// GOOD: sql<number> without aggregate -- simple arithmetic, no issue
56+
const withLiteral = await db
57+
.select({
58+
doubled: sql<number>`${orders.total} * 2`,
59+
})
60+
.from(orders);
61+
62+
// GOOD: sql<string | null> with aggregate -- developer handles it
63+
const sumAsString = await db
64+
.select({
65+
total: sql<string | null>`SUM(${orders.total})`,
66+
})
67+
.from(orders)
68+
.groupBy(orders.userId);
69+
70+
// GOOD: sql without type parameter (no assertion to check)
71+
const rawCount = await db.execute(sql`SELECT COUNT(*) FROM orders`);
72+
73+
return {
74+
withCount,
75+
withSum,
76+
withAvg,
77+
withMin,
78+
withMax,
79+
countAsString,
80+
withLiteral,
81+
sumAsString,
82+
rawCount,
83+
};
84+
}
85+
86+
export default function AggregateTypes() {
87+
return (
88+
<div>
89+
<h1>Aggregate Types</h1>
90+
</div>
91+
);
92+
}

test/persistence-check.test.ts

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,4 +599,92 @@ describe('persistence-check', () => {
599599
expect(whereIssues).toHaveLength(0);
600600
});
601601
});
602+
603+
describe('aggregate type assertion', () => {
604+
const aggFixture = path.join(
605+
fixturesDir,
606+
'app/routes/sql-aggregate-types.tsx'
607+
);
608+
609+
it('should error on sql<number> with COUNT', () => {
610+
const schema = parseDrizzleSchema(schemaPath);
611+
const issues = checkPersistence([aggFixture], schema);
612+
613+
const countIssue = issues.find(
614+
(i) => i.message.includes('COUNT') && i.message.includes('sql<number>')
615+
);
616+
expect(countIssue).toBeDefined();
617+
expect(countIssue!.severity).toBe('error');
618+
expect(countIssue!.category).toBe('drizzle');
619+
expect(countIssue!.suggestion).toContain('Number(');
620+
});
621+
622+
it('should error on sql<number> with SUM', () => {
623+
const schema = parseDrizzleSchema(schemaPath);
624+
const issues = checkPersistence([aggFixture], schema);
625+
626+
const sumIssue = issues.find(
627+
(i) => i.message.includes('SUM') && i.message.includes('sql<number>')
628+
);
629+
expect(sumIssue).toBeDefined();
630+
expect(sumIssue!.severity).toBe('error');
631+
});
632+
633+
it('should error on sql<number> with AVG', () => {
634+
const schema = parseDrizzleSchema(schemaPath);
635+
const issues = checkPersistence([aggFixture], schema);
636+
637+
const avgIssue = issues.find(
638+
(i) => i.message.includes('AVG') && i.message.includes('sql<number>')
639+
);
640+
expect(avgIssue).toBeDefined();
641+
expect(avgIssue!.severity).toBe('error');
642+
});
643+
644+
it('should error on sql<number | null> with MIN (union containing number)', () => {
645+
const schema = parseDrizzleSchema(schemaPath);
646+
const issues = checkPersistence([aggFixture], schema);
647+
648+
const minIssue = issues.find(
649+
(i) => i.message.includes('MIN') && i.message.includes('sql<number>')
650+
);
651+
expect(minIssue).toBeDefined();
652+
expect(minIssue!.severity).toBe('error');
653+
});
654+
655+
it('should error on sql<number> with MAX in db.execute', () => {
656+
const schema = parseDrizzleSchema(schemaPath);
657+
const issues = checkPersistence([aggFixture], schema);
658+
659+
const maxIssue = issues.find(
660+
(i) => i.message.includes('MAX') && i.message.includes('sql<number>')
661+
);
662+
expect(maxIssue).toBeDefined();
663+
expect(maxIssue!.severity).toBe('error');
664+
});
665+
666+
it('should produce exactly 5 aggregate warnings (no false positives)', () => {
667+
const schema = parseDrizzleSchema(schemaPath);
668+
const issues = checkPersistence([aggFixture], schema);
669+
670+
const aggIssues = issues.filter((i) =>
671+
i.message.includes('database returns string')
672+
);
673+
expect(aggIssues).toHaveLength(5);
674+
});
675+
676+
it('should not flag existing sql-raw-subquery fixture (uses sql<string>)', () => {
677+
const schema = parseDrizzleSchema(schemaPath);
678+
const filePath = path.join(
679+
fixturesDir,
680+
'app/routes/sql-raw-subquery.tsx'
681+
);
682+
const issues = checkPersistence([filePath], schema);
683+
684+
const aggIssues = issues.filter((i) =>
685+
i.message.includes('database returns string')
686+
);
687+
expect(aggIssues).toHaveLength(0);
688+
});
689+
});
602690
});

test/route-parser.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ describe('route-parser', () => {
5757
'/client-loader-safe',
5858
'/component-links',
5959
'/sql-raw-subquery',
60+
'/sql-aggregate-types',
6061
]);
6162
});
6263

0 commit comments

Comments
 (0)