Raw query syntax in typescript

[diegoimbert]

Support for
sql.query('SELECT $1, $2', arg1, arg2).fetchOne()

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	6854ab0
Status:	✅  Deploy successful!
Preview URL:	https://d01df241.windmill.pages.dev
Branch Preview URL:	https://di-ts-datatable-without-temp.windmill.pages.dev

View logs

[claude]

Claude finished @diegoimbert's task in 2m 25s —— View job

---

Code Review: Raw query syntax in typescript

Summary

This PR adds a new .query() method to the datatable SQL template function, enabling a raw query syntax like sql.query('SELECT $1, $2', arg1, arg2) as an alternative to the template literal syntax. This is a useful addition that provides flexibility for users who prefer positional parameters.

---

Issues Found

🔴 Critical Issues

1. Missing Type Exports and Declaration File Updates (typescript-client/sqlUtils.ts:72-74)

• The new DatatableSqlTemplateFunction interface is not exported
• The .d.ts file still declares datatable() as returning SqlTemplateFunction
• Impact: Users won't see the .query() method in autocomplete or get proper type checking
• Fix Required: Export the interface and update sqlUtils.d.ts line 52

🟡 Medium Priority Issues

2. Type Safety Concern with TemplateStringsArray Mock (typescript-client/sqlUtils.ts:204)

• Object.assign([sqlString], { raw: [sqlString] }) doesn't properly satisfy TemplateStringsArray requirements
• The raw property should be read-only per spec
• Suggestion: Use Object.freeze() on the raw property for proper immutability

3. Technical Debt Acknowledged (typescript-client/sqlUtils.ts:201)

• TODO comment indicates this is a quick implementation for a client need
• Recommendation: Create a follow-up issue to track proper refactoring
• The comment suggests breaking down SqlTemplateFunction impl for better reuse

---

Security Considerations

✅ No SQL Injection Risks: The implementation correctly:

• Passes parameters separately via the args object
• Uses the same parameterization system as the template literal syntax
• Arguments are handled by the backend executor with proper type casting

✅ No Credential Exposure: Database name is properly encapsulated in the args object

---

Performance Considerations

✅ Minimal Overhead: The .query() method is a thin wrapper that:

• Creates a mock TemplateStringsArray
• Delegates to the existing template function implementation
• No additional network calls or processing

⚠️ Minor Inefficiency: The array creation and assignment could be slightly optimized, but impact is negligible for typical usage patterns.

---

Code Quality Assessment

Strengths:

• Consistent with existing code patterns
• Minimal changes to achieve the goal
• Proper conditional implementation (datatable only)
• Maintains backward compatibility

Weaknesses:

• Missing documentation for the new method
• Type definitions incomplete (no export, no .d.ts update)
• Acknowledged technical debt (TODO comment)

---

Recommendations

Must Fix Before Merge:

1. Export the DatatableSqlTemplateFunction interface
2. Update typescript-client/sqlUtils.d.ts to reflect the new return type
3. Test the type definitions work correctly for consumers

Should Fix:

4. Add JSDoc documentation example showing the new .query() syntax
5. Consider using Object.freeze() for the raw property in the mock array
6. Create a GitHub issue to track the TODO refactoring work

Nice to Have:

7. Add unit tests for the new .query() method
8. Document when to use .query() vs template literal syntax

---

Testing Instructions

To test the new raw query syntax feature, navigate to the Windmill app and create a new TypeScript script. In the script editor, test both the existing template literal syntax and the new raw query syntax:

Existing syntax (should still work):

import * as wmill from 'windmill-client'
const sql = wmill.datatable()
const name = 'test'
await sql`SELECT * FROM users WHERE name = ${name}`.fetch()

New raw query syntax:

import * as wmill from 'windmill-client'
const sql = wmill.datatable()
await sql.query('SELECT * FROM users WHERE name = $1', 'test').fetch()

Test with multiple parameters using positional placeholders ($1, $2, etc.) and verify the results match the template literal approach. Verify that type annotations work correctly (e.g., $1::int, $2::text) and that the .fetch() and .fetchOne() methods work as expected. Check that TypeScript autocomplete shows the new .query() method in your IDE (this will only work after the type declaration fixes are applied).

---

Tasks Completed:

• Read the modified file and understand changes
• Analyze changes for code quality and best practices
• Check for potential bugs or issues
• Review performance considerations
• Review security implications
• Provide comprehensive feedback
• Create reproduction instructions for testing

[claude]

Type Safety Issue: The Object.assign approach with array literals doesn't properly satisfy the TemplateStringsArray type requirements. While this creates an object that looks like a TemplateStringsArray, it's not actually one.

Potential Issue: The raw property should be read-only according to the TemplateStringsArray interface specification. Consider using a proper type cast or creating a mock that fully satisfies the interface:

const templateStrings = Object.assign([sqlString], { 
  raw: Object.freeze([sqlString]) 
}) as unknown as TemplateStringsArray;
return sql(templateStrings, ...params);

This ensures immutability of the raw property as per spec.

[claude]

Code Quality - TODO Comment: The inline TODO suggests this implementation is temporary and needs refactoring. Before merging:

1. Consider whether the technical debt is acceptable for the "client need" mentioned
2. If this is a quick fix, create a GitHub issue to track the proper refactoring
3. Document the expected behavior and edge cases for future refactoring

Question: What's the timeline for addressing this TODO? Should we create a follow-up issue?

[claude]

Missing Type Export & Declaration File Update: The new DatatableSqlTemplateFunction interface is not:

1. Exported from this module (no export keyword)
2. Updated in the corresponding .d.ts file (typescript-client/sqlUtils.d.ts)

Impact:

• TypeScript consumers won't see the .query() method in their IDE autocomplete
• The type declaration file at line 52 still declares datatable returning SqlTemplateFunction instead of DatatableSqlTemplateFunction
• Users won't get type checking or IntelliSense for the new query() method

Suggested Fix:

export interface DatatableSqlTemplateFunction extends SqlTemplateFunction {
  query(sql: string, ...params: any[]): SqlStatement;
}

And update sqlUtils.d.ts line 52:

export declare function datatable(name: string): DatatableSqlTemplateFunction;

[claude]

Good: The return type annotation was correctly updated to reflect the new DatatableSqlTemplateFunction interface. However, the cast as DatatableSqlTemplateFunction is necessary because the implementation adds the .query() method conditionally later in sqlProviderImpl().

Consider adding a JSDoc comment explaining the new .query() method usage to the docstring above. For example:

/**
 * Create a SQL template function for PostgreSQL/datatable queries
 * @param name - Database/datatable name (default: "main")
 * @returns SQL template function for building parameterized queries
 * @example
 * // Template literal syntax
 * let sql = wmill.datatable()
 * let name = 'Robin'
 * let age = 21
 * await sql`
 *   SELECT * FROM friends
 *     WHERE name = ${name} AND age = ${age}::int
 * `.fetch()
 * 
 * @example
 * // Raw query syntax (new)
 * let sql = wmill.datatable()
 * await sql.query('SELECT * FROM friends WHERE name = $1 AND age = $2::int', 'Robin', 21).fetch()
 */