feat(@deepnote/blocks): export Python codegen helpers and SQL block types#379
Merged
Conversation
…ypes 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) <[email protected]>
Contributor
|
Caution Review failedPull request was closed or merged during review 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)
📝 WalkthroughWalkthroughAdds SQL cache-mode and SQL cell variable-type constants and types, ties schema validation to the shared SQL cell type list, implements createPythonCodeForSqlBlockWithConnectionJson (escaping SQL, connectionJson, auditComment; validating sql_cache_mode and deepnote_return_variable_type; emitting _dntk.execute_sql_with_connection_json and optional sanitized assignment/return), updates barrel exports to expose new utilities and types, and adds Vitest tests for escaping, sanitization, defaults, cache mode, return type, and validation errors. Sequence Diagram(s)sequenceDiagram
participant Caller
participant createPythonCodeForSqlBlockWithConnectionJson
participant PythonCell
participant _dntk_execute
Caller->>createPythonCodeForSqlBlockWithConnectionJson: generate(block, options)
createPythonCodeForSqlBlockWithConnectionJson->>PythonCell: emit dataframe prelude + escaped literals
PythonCell->>_dntk_execute: _dntk.execute_sql_with_connection_json(connectionJson, sql, sql_cache_mode, return_variable_type, audit_sql_comment)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 @@
## main #379 +/- ##
==========================================
+ Coverage 83.87% 83.93% +0.06%
==========================================
Files 145 145
Lines 7993 8018 +25
Branches 2221 2227 +6
==========================================
+ Hits 6704 6730 +26
+ Misses 1288 1287 -1
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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) <[email protected]>
… 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) <[email protected]>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/blocks/src/blocks/sql-blocks.ts (1)
89-102: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract the shared wrapping logic.
Lines 89-102 are identical to
createPythonCodeForSqlBlock(lines 42-55). Pull it into a helper so the two generators can't drift apart.♻️ Proposed helper
function wrapSqlExecuteCall( dataFrameConfig: string, executeSqlFunctionCall: string, sanitizedPythonVariableName: string | undefined ): string { if (sanitizedPythonVariableName === undefined) { return dedent` ${dataFrameConfig} ${executeSqlFunctionCall} ` } return dedent` ${dataFrameConfig} ${sanitizedPythonVariableName} = ${executeSqlFunctionCall} ${sanitizedPythonVariableName} ` }Then both functions end with
return wrapSqlExecuteCall(dataFrameConfig, executeSqlFunctionCall, sanitizedPythonVariableName).🤖 Prompt for 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. In `@packages/blocks/src/blocks/sql-blocks.ts` around lines 89 - 102, Extract the duplicated template logic into a helper named wrapSqlExecuteCall(dataFrameConfig, executeSqlFunctionCall, sanitizedPythonVariableName) that returns the dedented string shown in the review; replace the duplicate tail in both createPythonCodeForSqlBlock and the other generator (the function with identical lines 42-55) with a call to wrapSqlExecuteCall(dataFrameConfig, executeSqlFunctionCall, sanitizedPythonVariableName). Ensure the helper signature accepts sanitizedPythonVariableName: string | undefined and preserves the two branches (undefined vs defined) and returned template exactly as in the diff.
🤖 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.
Outside diff comments:
In `@packages/blocks/src/blocks/sql-blocks.ts`:
- Around line 89-102: Extract the duplicated template logic into a helper named
wrapSqlExecuteCall(dataFrameConfig, executeSqlFunctionCall,
sanitizedPythonVariableName) that returns the dedented string shown in the
review; replace the duplicate tail in both createPythonCodeForSqlBlock and the
other generator (the function with identical lines 42-55) with a call to
wrapSqlExecuteCall(dataFrameConfig, executeSqlFunctionCall,
sanitizedPythonVariableName). Ensure the helper signature accepts
sanitizedPythonVariableName: string | undefined and preserves the two branches
(undefined vs defined) and returned template exactly as in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: afb1c0bf-a0cd-41e2-8835-aca4fa3973d0
📒 Files selected for processing (5)
packages/blocks/src/blocks/python-utils.test.tspackages/blocks/src/blocks/python-utils.tspackages/blocks/src/blocks/sql-blocks.test.tspackages/blocks/src/blocks/sql-blocks.tspackages/blocks/src/deepnote-file/deepnote-file-schema.ts
…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) <[email protected]>
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 24-25: Replace the token-shaped OAuth value used in the test
fixtures with an inert placeholder (e.g., "test-token" or "redacted") wherever
the connectionJson string includes params.access_token and in any
expected-output strings that mirror it; specifically update the connectionJson
literal that contains "access_token":"ya29.bigquery-oauth-token" and the other
occurrences noted (around the other examples at the same file: the
connectionJson/expected strings referenced at the comment for lines 36, 57, 69)
so both the fixture and its corresponding expected assertions use the harmless
placeholder instead of a ya29.-prefixed token.
- Around line 301-302: Test currently aliases and mutates `block` via `const
invalidBlock: any = block`, bypassing typing; instead create a properly typed,
non-mutating copy and set `metadata.deepnote_return_variable_type` on the copy.
Replace the alias with a deep/structured clone or a shallow copy that also
clones `metadata` (e.g., create `invalidBlock = { ...block, metadata: {
...block.metadata, deepnote_return_variable_type: 'unexpected_value' } }`) and
use the correct Block type rather than `any`, ensuring `block` remains unchanged
and TypeScript types are preserved for `invalidBlock`.
🪄 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: 25af47c2-e284-41ba-b692-9f11f50049d8
📒 Files selected for processing (1)
packages/blocks/src/blocks/sql-blocks.test.ts
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) <[email protected]>
…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) <[email protected]>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/blocks/src/blocks/sql-blocks.test.ts (1)
301-307: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDrop
anyfrom the invalid-block fixture.Line 301 still turns off strict typing for the whole test object. Keep the invalid value, but cast at the boundary instead of making the fixture itself
any.Suggested change
- // biome-ignore lint/suspicious/noExplicitAny: testing invalid block - const invalidBlock: any = { + const invalidBlock = { ...block, metadata: { ...block.metadata, deepnote_return_variable_type: 'unexpected_value', }, - } as const + } as unknown as SqlBlockAs per coding guidelines, "Use strict type checking in TypeScript" and "Prefer type safety over convenience - avoid
anytypes and use proper type definitions".🤖 Prompt for 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. In `@packages/blocks/src/blocks/sql-blocks.test.ts` around lines 301 - 307, The test fixture uses an explicit any for invalidBlock which disables type checking; remove the any and keep the invalid value by creating the modified object normally (using the existing block and metadata modification) and apply a narrow cast only at the boundary where the invalid type is intentionally injected—i.e., remove "const invalidBlock: any" and instead declare const invalidBlock = { ...block, metadata: { ...block.metadata, deepnote_return_variable_type: 'unexpected_value' } } and cast that expression to the test-required type (e.g., using as unknown as <expected type> or the specific type union) where the test needs a mismatched type; refer to the invalidBlock variable, block variable, and metadata.deepnote_return_variable_type to locate and change the code.
🤖 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.
Duplicate comments:
In `@packages/blocks/src/blocks/sql-blocks.test.ts`:
- Around line 301-307: The test fixture uses an explicit any for invalidBlock
which disables type checking; remove the any and keep the invalid value by
creating the modified object normally (using the existing block and metadata
modification) and apply a narrow cast only at the boundary where the invalid
type is intentionally injected—i.e., remove "const invalidBlock: any" and
instead declare const invalidBlock = { ...block, metadata: { ...block.metadata,
deepnote_return_variable_type: 'unexpected_value' } } and cast that expression
to the test-required type (e.g., using as unknown as <expected type> or the
specific type union) where the test needs a mismatched type; refer to the
invalidBlock variable, block variable, and
metadata.deepnote_return_variable_type to locate and change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79ad7b6e-525a-4edc-80e1-74ae86664bb0
📒 Files selected for processing (1)
packages/blocks/src/blocks/sql-blocks.test.ts
jamesbhobbs
reviewed
Jun 4, 2026
jamesbhobbs
left a comment
jamesbhobbs
left a comment
Contributor
There was a problem hiding this comment.
Would approve with this cleanup #391
…pe (#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) <[email protected]> * 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) <[email protected]> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
jamesbhobbs
approved these changes
Jun 4, 2026
Contributor
|
Coderabbit already approved the chained PR |
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
Promote four package-internal helpers to the public API of
@deepnote/blocksand add one new helper, so downstream code generating Python from.deepnoteblocks no longer has to re-implement or re-vendor them.Re-exported (no source changes):
escapePythonString,sanitizePythonVariableName(fromblocks/python-utils)createDataFrameConfig(fromblocks/data-frame)CodeBlock,SqlBlocktypes (fromdeepnote-file/deepnote-file-schema)New:
createPythonCodeForSqlBlockWithConnectionJson(block, options)— sibling to the existingcreatePythonCodeForSqlBlock, but emits a call to_dntk.execute_sql_with_connection_json(...)with an inline connection-JSON literal instead of an env-var name.SqlCacheMode('cache_disabled' | 'always_write' | 'read_or_write')SqlCellVariableType('dataframe' | 'query_preview')The new helper mirrors the existing one's behaviour (variable-name sanitisation with
input_1fallback, dataframe-config prelude, assignment/echo wrapping) and routesconnectionJson/auditCommentthroughescapePythonStringso quoted JSON cannot break out of the Python string literal.Purely additive — no existing exports renamed, removed, or reshaped. Version bump is intentionally not included in this PR; release will be handled separately.
Test plan
pnpm test— full suite passes (2214 tests, including 14 new cases inpackages/blocks/src/blocks/sql-blocks.test.tscovering variable-name handling, allSqlCacheMode/SqlCellVariableTypevalues, defaults, and escape behaviour forconnectionJson/auditComment)pnpm typecheck— cleanpnpm build(inpackages/blocks) —dist/index.d.tslists all 8 new symbolspnpm biome:check— clean🤖 Generated with Claude Code
Summary by CodeRabbit