refactor(@deepnote/blocks): dedupe SQL codegen and validate return type - #391
Merged
tkislan merged 2 commits intoJun 4, 2026
Merged
Conversation
Follow-up to the SQL codegen helpers added in #379. - Extract the shared dataframe-formatter prelude + variable-name sanitisation (with `input_1` fallback) + assignment/echo wrapping into a single `wrapSqlExecution(block, executeSqlFunctionCall)` helper used by both `createPythonCodeForSqlBlock` and `createPythonCodeForSqlBlockWithConnectionJson`. Output is byte-identical (regression-guarded by the existing exact-match tests). - Back-port allowlist validation of `deepnote_return_variable_type` to `createPythonCodeForSqlBlock` via `assertSqlCellVariableType`, so the env-var path is as robust as the connection-JSON sibling against hand-constructed blocks that bypass the zod schema. - Document the intentional options-vs-metadata value sourcing on `createPythonCodeForSqlBlockWithConnectionJson`. Adds direct tests for `createPythonCodeForSqlBlock` (previously only covered indirectly via the `createPythonCode` dispatcher): default output, variable-name assignment/echo, and the new validation guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo SQL code generator functions were refactored to eliminate duplication: Sequence Diagram(s)sequenceDiagram
participant createPythonCodeForSqlBlock
participant createPythonCodeForSqlBlockWithConnectionJson
participant wrapSqlExecution
participant _dntk_execute_sql as _dntk.execute_sql
createPythonCodeForSqlBlock->>wrapSqlExecution: validate return type, build execute_sql call
createPythonCodeForSqlBlockWithConnectionJson->>wrapSqlExecution: validate return/cache, escape query/connectionJson/auditComment, build execute_sql call
wrapSqlExecution->>_dntk_execute_sql: emit _dntk.execute_sql(..., return_variable_type=...)
_dntk_execute_sql-->>wrapSqlExecution: result expression
wrapSqlExecution-->>createPythonCodeForSqlBlock: assembled python snippet (prelude + call + optional assignment)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tk/export-federated-auth-helpers #391 +/- ##
====================================================================
- Coverage 83.94% 83.93% -0.01%
====================================================================
Files 145 145
Lines 8021 8018 -3
Branches 2167 2227 +60
====================================================================
- Hits 6733 6730 -3
Misses 1287 1287
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/blocks/src/blocks/sql-blocks.test.ts`:
- Around line 364-366: Replace the use of the `any` type for the test fixture
`invalidBlock` with an `unknown`-typed boundary: declare the literal object
without `any` (or explicitly type it as `unknown`) and only cast to the
function's expected input type at the call site (e.g., when passing into the
validator or constructor under test). This keeps the test's invalid shape while
preserving strict typing; update references to `invalidBlock` in the test to use
the safe cast where necessary (rather than widening the variable itself to
`any`).
In `@packages/blocks/src/blocks/sql-blocks.ts`:
- Around line 50-52: Update the JSDoc to accurately describe how the values are
embedded: state that query, connectionJson, auditComment, sqlCacheMode, and
returnVariableType are all escaped/interpolated into single-quoted Python string
literals (not that sqlCacheMode and returnVariableType are interpolated outside
quotes), and add that sqlCacheMode and returnVariableType are additionally
validated against allowlists before interpolation; reference the parameter names
query, connectionJson, auditComment, sqlCacheMode, and returnVariableType so the
corrected doc matches the actual interpolation behavior shown later in the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df71ee98-2406-4e5d-a322-33d7a248c4f2
📒 Files selected for processing (2)
packages/blocks/src/blocks/sql-blocks.test.tspackages/blocks/src/blocks/sql-blocks.ts
- Correct the doc comment on createPythonCodeForSqlBlockWithConnectionJson: sqlCacheMode/returnVariableType are interpolated into single-quoted literals (not "outside quotes"); they are safe because they are allowlist-validated to known identifiers, not because of their position. - Drop the `any` cast in the invalid-input test in favour of an `as unknown as SqlBlock` cast confined to the call boundary, removing the biome-ignore. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tkislan
approved these changes
Jun 4, 2026
jamesbhobbs
added a commit
that referenced
this pull request
Jun 4, 2026
…ypes (#379) * feat(@deepnote/blocks): export Python codegen helpers and SQL block types Promote escapePythonString, sanitizePythonVariableName, createDataFrameConfig, CodeBlock, and SqlBlock to the public API, and add the new createPythonCodeForSqlBlockWithConnectionJson helper (plus its SqlCacheMode and SqlCellVariableType types) so downstream callers generating Python from .deepnote blocks no longer need to re-implement or re-vendor these helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: fix spell-check in sql-blocks tests Use US spelling ("sanitizes") to match the sanitizePythonVariableName helper, and adjust the special-characters test input so the sanitized identifier is composed of words cspell recognises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(@deepnote/blocks): validate SQL codegen inputs and harden Python string escaping Follow-up to the SQL connection-json codegen helpers: - Validate sqlCacheMode and deepnote_return_variable_type against an allowlist in createPythonCodeForSqlBlockWithConnectionJson, throwing InvalidValueError instead of interpolating out-of-allowlist values unescaped into the generated Python. - Escape CR and NUL in escapePythonString so single-quoted Python literals stay valid for queries/comments containing those bytes. - Derive SqlCacheMode / SqlCellVariableType from shared SQL_CACHE_MODES / SQL_CELL_VARIABLE_TYPES constants. - Add escapePythonString and SQL connection-json codegen tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(@deepnote/blocks): use realistic BigQuery OAuth connection JSON in SQL codegen tests - Replace the placeholder {"type":"postgres"} with the federated BigQuery OAuth connection shape (url + params.access_token/project) that execute_sql_with_connection_json actually receives. - Simplify the invalid-block construction in the return-type validation test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(@deepnote/blocks): restore spread-based invalid block construction Revert the alias-and-mutate invalidBlock to creating a new object via spread — more readable and avoids mutating the original typed block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(@deepnote/blocks): use inert placeholder for OAuth token in SQL codegen test Replace the ya29.-prefixed access_token value (which looks like a real Google OAuth token) with a harmless "redacted" placeholder in both the connectionJson fixtures and their mirrored expected-output assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(@deepnote/blocks): dedupe SQL codegen and validate return type (#391) * refactor(@deepnote/blocks): dedupe SQL codegen and validate return type Follow-up to the SQL codegen helpers added in #379. - Extract the shared dataframe-formatter prelude + variable-name sanitisation (with `input_1` fallback) + assignment/echo wrapping into a single `wrapSqlExecution(block, executeSqlFunctionCall)` helper used by both `createPythonCodeForSqlBlock` and `createPythonCodeForSqlBlockWithConnectionJson`. Output is byte-identical (regression-guarded by the existing exact-match tests). - Back-port allowlist validation of `deepnote_return_variable_type` to `createPythonCodeForSqlBlock` via `assertSqlCellVariableType`, so the env-var path is as robust as the connection-JSON sibling against hand-constructed blocks that bypass the zod schema. - Document the intentional options-vs-metadata value sourcing on `createPythonCodeForSqlBlockWithConnectionJson`. Adds direct tests for `createPythonCodeForSqlBlock` (previously only covered indirectly via the `createPythonCode` dispatcher): default output, variable-name assignment/echo, and the new validation guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(@deepnote/blocks): address CodeRabbit review on SQL codegen - Correct the doc comment on createPythonCodeForSqlBlockWithConnectionJson: sqlCacheMode/returnVariableType are interpolated into single-quoted literals (not "outside quotes"); they are safe because they are allowlist-validated to known identifiers, not because of their position. - Drop the `any` cast in the invalid-input test in favour of an `as unknown as SqlBlock` cast confined to the call boundary, removing the biome-ignore. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: James Hobbs <15235276+jamesbhobbs@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on top of #379 (base =
tk/export-federated-auth-helpers). Addresses three review follow-ups on the SQL codegen helpers added there:Dedupe. The dataframe-formatter prelude + variable-name sanitization (
input_1fallback) + assignment/echo wrapping was copy-pasted acrosscreatePythonCodeForSqlBlockandcreatePythonCodeForSqlBlockWithConnectionJson. Extracted into a single privatewrapSqlExecution(block, executeSqlFunctionCall)that both call. Each public function now only builds its ownexecuteSqlFunctionCallstring.Validate
return_variable_typeon the env-var path too.createPythonCodeForSqlBlockpreviously interpolateddeepnote_return_variable_typestraight into the generated Python with no validation (relying solely on the upstream zod schema). It now runs through the sameassertSqlCellVariableTypeallowlist guard as its connection-JSON sibling, so a hand-constructed block that bypasses the schema throwsInvalidValueErrorinstead of emitting unexpected/unsafe code.Document the option/metadata split. Added a doc comment to
createPythonCodeForSqlBlockWithConnectionJsonexplaining thatconnectionJson/auditComment/sqlCacheModecome fromoptionswhilereturnVariableTypecomes fromblock.metadata— and why the first group is escaped while the last two are allowlist-validated.Behavior
Output is byte-identical for all valid inputs — the dedupe only moves code, it doesn't change the generated Python. The only observable change is that an invalid
deepnote_return_variable_typeon the env-var path now throws (previously it silently produced bad Python).Tests
Added a direct
describe('createPythonCodeForSqlBlock', ...)block — this function previously had no direct tests (only indirect coverage via thecreatePythonCodedispatcher):InvalidValueErroron an out-of-allowlistdeepnote_return_variable_typeVerified locally:
vitest run sql-blocks.test.ts python-code.test.ts→ 57 passed (incl. the 39 dispatcher tests that exercise the env-var path)tsc --noEmit(blocks) → cleanbiome check+cspellon changed files → cleanNote
Targets #379's branch, so the diff shown here is just this change on top of #379. Merge #379 first; this will then re-target
mainautomatically.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor