Added support to download binary data from result grid. #4011#9645
Open
pravesh-sharma wants to merge 5 commits intopgadmin-org:masterfrom
Open
Added support to download binary data from result grid. #4011#9645pravesh-sharma wants to merge 5 commits intopgadmin-org:masterfrom
pravesh-sharma wants to merge 5 commits intopgadmin-org:masterfrom
Conversation
pravesh-sharma
force-pushed
the
GH-4011
branch
from
cbc7383 to
5326bda
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an opt-in binary download flow: new preference, backend POST endpoint to stream a result-grid cell as an octet-stream, frontend UI (download button + events) to trigger the download, and cursor-level psycopg3 typecasters to handle binary data conversion. Changes
Sequence DiagramsequenceDiagram
participant User
participant BinaryFormatter
participant EventBus
participant ResultSet
participant FrontendAPI
participant SQLEditorBackend
participant Database
User->>BinaryFormatter: Click download button
BinaryFormatter->>EventBus: Fire TRIGGER_SAVE_BINARY_DATA (rowPos, colPos)
EventBus->>ResultSet: Event received
ResultSet->>ResultSet: Create filename, show progress
ResultSet->>FrontendAPI: POST /download_binary_data (trans_id, rowPos, colPos)
FrontendAPI->>SQLEditorBackend: HTTP POST request
SQLEditorBackend->>SQLEditorBackend: Validate transaction & cursor
SQLEditorBackend->>SQLEditorBackend: register_binary_data_typecasters(cursor)
SQLEditorBackend->>Database: Fetch cell binary value
Database->>SQLEditorBackend: Return binary data
SQLEditorBackend->>FrontendAPI: Stream octet-stream attachment
FrontendAPI->>ResultSet: Receive stream
ResultSet->>User: Save file, clear progress
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2200-2209: The code dereferences conn._Connection__async_cursor
before verifying the results of check_transaction_status, which can return a
Response or a None conn; update the logic in the block that calls
check_transaction_status(trans_id) to first check if the returned value is a
Response and return it directly, then verify conn, trans_obj and session_obj are
not None before accessing conn._Connection__async_cursor; only after those
guards call register_binary_data_typecasters(cur). Ensure you stop ignoring
error_msg by either logging/including it in the returned internal_server_error
call (using TRANSACTION_STATUS_CHECK_FAILED as context) so Ruff's
unused-variable warning is resolved and real transaction errors are surfaced.
- Around line 2220-2223: The code uses cursor.scroll in relative mode and
doesn't validate inputs, so fix by parsing and validating data['rowpos'] and
data['colpos'] (ensure they are integers >= 0), call cur.scroll(int_rowpos,
mode='absolute') to move to the exact row, call cur.fetchone() and check for
None (row out of range), then verify the returned row has enough columns before
accessing binary_data[col_pos']; on any invalid input or out-of-range access
raise/return a proper validation response instead of letting
IndexError/ValueError bubble up; wrap the parsing/validation and fetch in a
try/except to return a controlled error for bad inputs.
In
`@web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx`:
- Around line 76-85: BinaryFormatter is using the hard-coded row.__temp_PK which
can be undefined when the server shifts the key (e.g. __temp_PK1); instead
locate the actual client-PK key on the row object and pass its value to
eventBus.fireEvent for QUERY_TOOL_EVENTS.TRIGGER_SAVE_BINARY_DATA. Change the
onClick handler in BinaryFormatter to compute the client PK key by scanning
Object.keys(row) for the property that startsWith('__temp_PK') (or otherwise
matches the server-generated client-PK name used elsewhere in ResultSet.jsx),
read row[thatKey], and use that value instead of row.__temp_PK when firing the
event; keep the rest of the component and props (row, column, eventBus,
downloadBinaryData) unchanged.
web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx
Outdated
Show resolved
Hide resolved
pravesh-sharma
force-pushed
the
GH-4011
branch
2 times, most recently
from
bcef5c9 to
e1ead7e
Compare
pravesh-sharma
commented
Feb 20, 2026
pravesh-sharma
commented
Feb 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2253-2254: Remove the trailing whitespace at the end of the line
containing the return call to send_file in __init__.py (the line starting with
"return send_file(") to fix the pycodestyle W291; edit the return send_file(
call so there is no space before the newline (e.g., change "return send_file(␣"
to "return send_file(") and re-run linters to confirm the warning is resolved.
- Around line 2200-2212: The current handling after
check_transaction_status(trans_id) treats any error_msg as a 500 and may
double-wrap Response objects; instead, inspect the returned error_msg from
check_transaction_status (e.g., ERROR_MSG_TRANS_ID_NOT_FOUND) and handle it
explicitly: if error_msg equals the "not found" constant return a 404 Response
(do not call internal_server_error), if error_msg is already a Response return
it directly, otherwise fall back to internal_server_error(errormsg=error_msg);
keep the subsequent guard that returns
internal_server_error(TRANSACTION_STATUS_CHECK_FAILED) only for missing/invalid
status/conn/trans_obj/session_obj as before.
In
`@web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx`:
- Around line 76-86: BinaryFormatter currently passes a page-relative
props.rowIdx into eventBus.fireEvent for
QUERY_TOOL_EVENTS.TRIGGER_SAVE_BINARY_DATA, which breaks downloads when
pagination is present; update BinaryFormatter to get pagination base from
DataGridExtrasContext (or use dataGridExtras?.startRowNum) and compute an
absolute index as props.rowIdx + (startRowNum ?? 1) - 1, then pass that absolute
index to eventBus.fireEvent instead of props.rowIdx so the saved binary data
targets the correct row.
---
Duplicate comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2230-2247: The code advances the DB cursor before validating
inputs and then scrolls again, which can skip rows; remove the premature
cur.scroll(int(data['rowpos'])) and the initial cur.fetchone() usage, validate
rowpos/colpos first (ensure they are ints and non-negative), then call
cur.scroll(row_pos, mode='absolute') to move to the absolute row and fetch with
cur.fetchone(), check that row is not None and col_pos < len(row) before
accessing row[col_pos], and keep the existing exception handling
(ValueError/IndexError/TypeError) with the internal_server_error/gettext path
unchanged.
web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2226-2234: The error message returned when request data is missing
is incorrect/copy-pasted; update the gettext string in the block that sets data
= request.values if request.values else request.get_json(silent=True) so the
make_json_response call (success=0, status=410) references the expected binary
data parameters rather than "query" — e.g. change the gettext text to mention
the required binary payload/parameter names (for example "file", "binary" or
whatever this endpoint expects) so callers know which binary parameters are
missing; keep the existing use of gettext and make_json_response.
- Around line 2258-2263: The code calls send_file(BytesIO(binary_data), ...)
where binary_data comes from row[col_pos]; if the cell is SQL NULL (binary_data
is None) BytesIO(None) raises TypeError outside the try/except, so before
constructing BytesIO check whether binary_data is None (or empty) and return a
user-friendly error response (e.g., an HTTP error/flash/message) instead of
attempting to call BytesIO; update the branch around binary_data/row[col_pos]
that calls send_file and ensure send_file(BytesIO(...)) is only executed when
binary_data is not None, referencing the existing variables binary_data,
row[col_pos], BytesIO and send_file.
- Around line 2235-2238: Remove the unvalidated duplicate cursor operations:
delete the four lines that set col_pos = data['colpos'], call
cur.scroll(int(data['rowpos'])), assign binary_data = cur.fetchone(), and then
binary_data = binary_data[col_pos]; these are run before the existing try/except
and can raise KeyError or use an uncast col_pos and corrupt cursor state—keep
only the validated logic already present in the try/except that performs
cur.scroll, cur.fetchone and integer casting of col_pos.
- Around line 2219-2224: The call to register_binary_data_typecasters(cur)
mutates adapters on the shared cursor conn._Connection__async_cursor
(registering ByteaBinaryDataLoader for OIDs 17 and 1001) and breaks subsequent
grid fetches; fix by creating and using a dedicated cursor for the
binary-download path instead of mutating the shared cursor — e.g., obtain a new
cursor (not conn._Connection__async_cursor), call
register_binary_data_typecasters(new_cur) and perform the binary fetch/streaming
on new_cur, then close it; alternatively, if creating a separate cursor is
infeasible, capture the original typecasters from the shared cursor, call
register_binary_data_typecasters(cur) for the binary read, then restore the
saved adapters immediately after the download completes.
In
`@web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx`:
- Line 87: Remove the stray debug console.log call in Formatters.jsx: delete the
line "console.log(absoluteRowPos)" (it triggers ESLint no-console and semi
errors); if runtime debugging is required instead use the project's logger API
or wrap in a debug-only conditional, but the immediate fix is to remove that
console.log reference to absoluteRowPos so the file passes linting.
- Around line 83-86: clientPK is being assigned sequentially in processRows
using clientPKLastIndex without accounting for the fetchWindow pagination
offset, so rows re-fetched on earlier pages get wrong absolute positions; update
processRows to compute clientPK from the window offset (e.g., clientPK =
(fetchWindowOffset || 0) + localIndex) or set clientPKLastIndex to the current
page start before incrementing, and when reading absoluteRowPos use
parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0, 10) to include the radix;
locate and change the code in processRows and the absoluteRowPos calculation to
apply the pagination offset (use the fetchWindow/offset value available there or
from dataGridExtras) and add the radix argument to parseInt.
web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx
Outdated
Show resolved
Hide resolved
web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2219-2257: The handler mutates the live async result cursor
conn._Connection__async_cursor by calling cur.scroll(...) and cur.fetchone(), so
preserve/restore the cursor position (or use a separate cursor) to avoid
changing shared state: either capture the current position (e.g., cur.rownumber
or equivalent on the cursor) before calling cur.scroll/cur.fetchone and then
restore it with cur.scroll(...) in the finally block, or perform the binary read
using a new cursor from conn (e.g., conn.cursor()) so
register_binary_typecasters, cur.scroll and cur.fetchone operate on an isolated
cursor and leave conn._Connection__async_cursor unchanged.
In `@web/pgadmin/utils/driver/psycopg3/typecast.py`:
- Around line 258-274: ByteaDataLoader.load currently only hex-decodes
memoryview and can return str on fallback; change load to normalize memoryview,
bytes/bytearray and str to a bytes object and always return bytes or None: for
memoryview/bytes/bytearray call b = bytes(data); if b.startswith(b'\\x') strip
the initial b'\\x', decode the remainder as ASCII hex and return
bytes.fromhex(...), catching ValueError and returning the original bytes on
error; for str convert to bytes (e.g. via encode('utf-8')) first and apply the
same logic; ensure the final return type is bytes or None and update
ByteaDataLoader.load accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84603580-642a-4e56-af20-c4806639ac2b
📒 Files selected for processing (7)
docs/en_US/preferences.rstweb/pgadmin/misc/__init__.pyweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.jsweb/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsxweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/utils/driver/psycopg3/typecast.py
🚧 Files skipped from review as they are similar to previous changes (3)
- web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
- docs/en_US/preferences.rst
- web/pgadmin/misc/init.py
Comment on lines
+258
to
+274
| class ByteaDataLoader(Loader): | ||
| # Loads the actual binary data. | ||
| def load(self, data): | ||
| if data: | ||
| if isinstance(data, memoryview): | ||
| data = bytes(data).decode() | ||
| if data.startswith('\\x'): | ||
| data = data[2:] | ||
| try: | ||
| return bytes.fromhex(data) | ||
| except ValueError: | ||
| # In case of error while converting hex to bytes, return | ||
| # original data. | ||
| return data | ||
| else: | ||
| return data | ||
| return data if data is not None else None |
There was a problem hiding this comment.
ByteaDataLoader doesn’t consistently return decoded bytes.
The hex-decoding path only runs for memoryview. If psycopg hands this loader a plain bytes buffer, the code returns the textual bytea representation unchanged, so the download endpoint can stream b'\\x...' instead of the original payload. On the fallback path it can also return a str, which is incompatible with BytesIO. Please normalize both bytes and memoryview through the same conversion path and keep the return type bytes | None.
🔧 Suggested direction
class ByteaDataLoader(Loader):
# Loads the actual binary data.
def load(self, data):
- if data:
- if isinstance(data, memoryview):
- data = bytes(data).decode()
- if data.startswith('\\x'):
- data = data[2:]
- try:
- return bytes.fromhex(data)
- except ValueError:
- # In case of error while converting hex to bytes, return
- # original data.
- return data
- else:
- return data
- return data if data is not None else None
+ if data is None:
+ return None
+
+ raw = bytes(data) if isinstance(data, memoryview) else data
+ if isinstance(raw, bytes) and raw.startswith(b'\\x'):
+ return bytes.fromhex(raw[2:].decode())
+
+ return raw🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/utils/driver/psycopg3/typecast.py` around lines 258 - 274,
ByteaDataLoader.load currently only hex-decodes memoryview and can return str on
fallback; change load to normalize memoryview, bytes/bytearray and str to a
bytes object and always return bytes or None: for memoryview/bytes/bytearray
call b = bytes(data); if b.startswith(b'\\x') strip the initial b'\\x', decode
the remainder as ASCII hex and return bytes.fromhex(...), catching ValueError
and returning the original bytes on error; for str convert to bytes (e.g. via
encode('utf-8')) first and apply the same logic; ensure the final return type is
bytes or None and update ByteaDataLoader.load accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/pgadmin/utils/driver/psycopg3/typecast.py (1)
258-266:⚠️ Potential issue | 🟠 MajorNormalize
bytesinput before decodingbytea.Line 263 only normalizes
memoryview. If psycopg passes text-formatbyteahere asbytes, this loader returns the literalb'\\x...'payload unchanged, so the download path can stream the escaped representation instead of the original bytes. It can also still fall through with a non-bytesreturn type.🔧 Suggested fix
class ByteaDataLoader(Loader): # Loads the actual binary data. def load(self, data): if data is None: return None - raw = bytes(data) if isinstance(data, memoryview) else data - if isinstance(raw, str) and raw.startswith('\\x'): - return bytes.fromhex(raw[2:]) - return raw + if isinstance(data, str): + raw = data.encode('utf-8') + elif isinstance(data, (memoryview, bytearray)): + raw = bytes(data) + else: + raw = data + + if isinstance(raw, bytes) and raw.startswith(b'\\x'): + try: + return bytes.fromhex(raw[2:].decode('ascii')) + except ValueError: + return raw + + return rawIn psycopg 3, for a custom `psycopg.adapt.Loader` registered for `bytea`, what Python types can `Loader.load(self, data)` receive for text-format and binary-format values? Can text-format `bytea` arrive as `bytes` containing the `\x...` representation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/utils/driver/psycopg3/typecast.py` around lines 258 - 266, ByteaDataLoader.load currently only normalizes memoryview to bytes, so if psycopg passes a text-format bytea as a bytes object like b'\\x...', it will be returned unchanged; update ByteaDataLoader.load to also normalize bytes input to a bytes value, detect both str and bytes prefixed with '\\x' (or b'\\x') and decode the hex portion with bytes.fromhex to ensure the method always returns raw bytes for both text- and binary-format bytea values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2219-2266: The code registers binary data typecasters on the
shared async cursor (register_binary_data_typecasters(cur)) before validating
the request payload, so an early return (e.g., when data is None) skips the
final restore and leaves the cursor mutated; move the call to
register_binary_data_typecasters(cur) so it runs only after request validation
(after computing data via request.values/request.get_json) or ensure every early
return restores the original typecasters by calling
register_binary_typecasters(cur) before returning; reference cur
(conn._Connection__async_cursor), register_binary_data_typecasters,
register_binary_typecasters, and the request.values/request.get_json check when
making the change.
---
Duplicate comments:
In `@web/pgadmin/utils/driver/psycopg3/typecast.py`:
- Around line 258-266: ByteaDataLoader.load currently only normalizes memoryview
to bytes, so if psycopg passes a text-format bytea as a bytes object like
b'\\x...', it will be returned unchanged; update ByteaDataLoader.load to also
normalize bytes input to a bytes value, detect both str and bytes prefixed with
'\\x' (or b'\\x') and decode the hex portion with bytes.fromhex to ensure the
method always returns raw bytes for both text- and binary-format bytea values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 797f3c95-9953-4680-9e98-aff12ee43034
📒 Files selected for processing (2)
web/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/utils/driver/psycopg3/typecast.py
khushboovashi
requested changes
Mar 9, 2026
| ) | ||
| ) | ||
|
|
||
| try: |
Contributor
There was a problem hiding this comment.
Please move this block to the connection file.
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 by CodeRabbit
New Features
Documentation