Added support to download binary data from result grid. #4011#9645
Added support to download binary data from result grid. #4011#9645pravesh-sharma wants to merge 3 commits intopgadmin-org:masterfrom
Conversation
pravesh-sharma
force-pushed
the
GH-4011
branch
from
cbc7383 to
5326bda
Compare
WalkthroughAdds optional binary-data download support to the Query Tool: new user preference, cursor-level bytea typecasters, a backend POST endpoint to stream cell binary data, and frontend UI/events to trigger and save binary cells from result grids. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Browser / Query Tool
participant EventBus as QueryTool EventBus
participant Backend as Flask Endpoint
participant Driver as psycopg3 Driver
participant DB as PostgreSQL
User->>Frontend: Click download button on binary cell
Frontend->>EventBus: Emit TRIGGER_SAVE_BINARY_DATA(rowPos, colPos)
EventBus->>Frontend: ResultSet handler starts download (show loader)
Frontend->>Backend: POST /download_binary_data/<trans_id> (rowpos, colpos)
Backend->>Backend: Validate transaction & cursor
Backend->>Driver: register_binary_data_typecasters(cursor)
Backend->>Driver: Fetch specified row/column
Driver->>DB: Retrieve bytea column
DB-->>Driver: bytea payload
Driver->>Backend: Bytea loader returns bytes
Backend-->>Frontend: response (application/octet-stream attachment)
Frontend->>Frontend: Save file, clear loader
Frontend->>User: Download complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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
from
5326bda to
bcef5c9
Compare
pravesh-sharma
force-pushed
the
GH-4011
branch
from
bcef5c9 to
e1ead7e
Compare
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
Outdated
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.
| cur = conn._Connection__async_cursor | ||
| if cur is None: | ||
| return internal_server_error( | ||
| errormsg=gettext('No active result cursor.') | ||
| ) | ||
| register_binary_data_typecasters(cur) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine register_binary_data_typecasters implementation
fd "typecast" web/pgadmin/ --type f | head -20Repository: pgadmin-org/pgadmin4
Length of output: 109
🏁 Script executed:
# Search for the function definition and usage
rg -n "def register_binary_data_typecasters" web/pgadmin/ -A 15Repository: pgadmin-org/pgadmin4
Length of output: 1378
🏁 Script executed:
# Check the typecast module in psycopg3 driver
find web/pgadmin -name "*.py" -path "*/psycopg3/*" | xargs grep -l "ByteaDataLoader\|register.*typecaster" | head -5Repository: pgadmin-org/pgadmin4
Length of output: 157
🏁 Script executed:
# Check how async_cursor is used in fetch operations
rg -n "__async_cursor" web/pgadmin/tools/sqleditor/__init__.py -B 3 -A 3 | head -80Repository: pgadmin-org/pgadmin4
Length of output: 739
🏁 Script executed:
# Check if there's cleanup or restoration of adapters after binary download
rg -n "register_binary_data_typecasters" web/pgadmin/tools/sqleditor/__init__.py -B 10 -A 10Repository: pgadmin-org/pgadmin4
Length of output: 1740
🏁 Script executed:
# Get the full function context around line 2224
sed -n '2200,2280p' web/pgadmin/tools/sqleditor/__init__.pyRepository: pgadmin-org/pgadmin4
Length of output: 2469
🏁 Script executed:
# Check what function contains line 2224
rg -n "^def " web/pgadmin/tools/sqleditor/__init__.py | awk -F: '$1 <= 2224' | tail -1Repository: pgadmin-org/pgadmin4
Length of output: 104
🏁 Script executed:
# Search for any adapter reset or restoration in the file
rg -n "adapters\|ByteaDataLoader\|ByteaBinaryDataLoader" web/pgadmin/tools/sqleditor/__init__.pyRepository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check the typecast module to understand the loaders
rg -n "class ByteaDataLoader\|class ByteaBinaryDataLoader" web/pgadmin/utils/driver/psycopg3/typecast.py -A 10Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Get the full typecast.py file to understand ByteaDataLoader and ByteaBinaryDataLoader
wc -l web/pgadmin/utils/driver/psycopg3/typecast.pyRepository: pgadmin-org/pgadmin4
Length of output: 113
🏁 Script executed:
# Check the loaders
sed -n '1,230p' web/pgadmin/utils/driver/psycopg3/typecast.pyRepository: pgadmin-org/pgadmin4
Length of output: 7820
🏁 Script executed:
# Search for any documentation or comments about ByteaBinaryDataLoader
rg -n "ByteaBinaryDataLoader" web/pgadmin/utils/driver/psycopg3/typecast.py -B 5 -A 10Repository: pgadmin-org/pgadmin4
Length of output: 1118
🏁 Script executed:
# Check if there are any other calls to register_binary_data_typecasters or adapter cleanup
rg -n "register_binary_data_typecasters\|adapters\.register\|adapters\.deregister" web/pgadmin/ -iRepository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check the full ByteaDataLoader to understand what it returns
sed -n '258,276p' web/pgadmin/utils/driver/psycopg3/typecast.pyRepository: pgadmin-org/pgadmin4
Length of output: 688
🏁 Script executed:
# Check if there's a separate cursor creation for binary download or cleanup after
rg -n "download_binary_data" web/pgadmin/tools/sqleditor/__init__.py -A 100 | grep -E "adapters|cursor|finally|cleanup" -iRepository: pgadmin-org/pgadmin4
Length of output: 348
🏁 Script executed:
# Check if fetch_window or other grid operations might reuse the same cursor
rg -n "def fetch_window\|def async_fetchmany_2darray" web/pgadmin/tools/sqleditor/__init__.py -A 20 | head -80Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Get the complete download_binary_data function with context
sed -n '2195,2270p' web/pgadmin/tools/sqleditor/__init__.pyRepository: pgadmin-org/pgadmin4
Length of output: 2273
🏁 Script executed:
# Check if there's any adapter cleanup or cursor reset logic elsewhere
rg -n "def query_tool\|def fetch_window\|adapters\." web/pgadmin/tools/sqleditor/__init__.py | head -40Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check what the grid display expects for bytea columns
rg -n "bytea\|binary" web/pgadmin/tools/sqleditor/ -i --type js | head -30Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check if the async cursor is created fresh for each operation or persisted
rg -n "__async_cursor\s*=" web/pgadmin/utils/driver/psycopg3/ -B 3 -A 3Repository: pgadmin-org/pgadmin4
Length of output: 1859
register_binary_data_typecasters permanently alters the shared cursor's type adapters, potentially breaking subsequent grid operations.
register_binary_data_typecasters(cur) modifies the adapter registry on conn._Connection__async_cursor by registering ByteaBinaryDataLoader for OIDs 17 and 1001 (bytea types). This cursor is reused for all subsequent fetch operations within the same query execution. After this call, if the user fetches additional rows or pages, bytea columns will return raw binary data instead of the hex-encoded strings the grid expects, breaking the display.
Consider either: (a) saving and restoring the original adapters after the binary fetch, or (b) using a separate cursor for the binary download.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 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.
| data = request.values if request.values else request.get_json(silent=True) | ||
| if data is None: | ||
| return make_json_response( | ||
| status=410, | ||
| success=0, | ||
| errormsg=gettext( | ||
| "Could not find the required parameter (query)." | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Misleading error message — copy-pasted from another endpoint.
The error text "Could not find the required parameter (query)." was copied from the query_tool_download handler. For this endpoint, it should reference the expected binary data parameters.
✏️ Suggested fix
errormsg=gettext(
- "Could not find the required parameter (query)."
+ "Could not find the required parameters (rowpos, colpos)."
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 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.
| col_pos = data['colpos'] | ||
| cur.scroll(int(data['rowpos'])) | ||
| binary_data = cur.fetchone() | ||
| binary_data = binary_data[col_pos] |
There was a problem hiding this comment.
Critical: Unvalidated duplicate code executes before the try block, corrupting cursor state.
Lines 2235–2238 scroll the cursor and fetch a row without any validation or error handling, then lines 2240–2256 repeat the same logic properly inside a try/except. This appears to be leftover code from before the validation fix was applied. Problems:
data['colpos']raises an unhandledKeyErrorif the key is missing.col_posremains a string (not cast toint), sobinary_data[col_pos]will fail or behave incorrectly.- The cursor is scrolled and a row fetched for no reason — the validated block below repeats this.
Remove these four lines entirely.
🐛 Proposed fix
register_binary_data_typecasters(cur)
data = request.values if request.values else request.get_json(silent=True)
if data is None:
return make_json_response(
status=410,
success=0,
errormsg=gettext(
"Could not find the required parameter (query)."
)
)
- col_pos = data['colpos']
- cur.scroll(int(data['rowpos']))
- binary_data = cur.fetchone()
- binary_data = binary_data[col_pos]
-
try:
row_pos = int(data['rowpos'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 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.
| return send_file( | ||
| BytesIO(binary_data), | ||
| as_attachment=True, | ||
| download_name='binary_data', | ||
| mimetype='application/octet-stream' | ||
| ) |
There was a problem hiding this comment.
Handle NULL cell values — BytesIO(None) raises TypeError.
If the selected cell contains a SQL NULL, row[col_pos] is None, and BytesIO(None) will throw an unhandled TypeError (this line is outside the try/except block). Return a user-friendly error instead.
🛡️ Proposed fix
+ if binary_data is None:
+ return bad_request(
+ errormsg=gettext('The selected cell contains NULL.')
+ )
+
return send_file(
BytesIO(binary_data),
as_attachment=True,
download_name='binary_data',
mimetype='application/octet-stream'
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return send_file( | |
| BytesIO(binary_data), | |
| as_attachment=True, | |
| download_name='binary_data', | |
| mimetype='application/octet-stream' | |
| ) | |
| if binary_data is None: | |
| return bad_request( | |
| errormsg=gettext('The selected cell contains NULL.') | |
| ) | |
| return send_file( | |
| BytesIO(binary_data), | |
| as_attachment=True, | |
| download_name='binary_data', | |
| mimetype='application/octet-stream' | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 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.
There was a problem hiding this comment.
For null value download is not allowed
There was a problem hiding this comment.
@pravesh-sharma, thank you for clarifying that NULL value downloads are not allowed by design. However, the current code doesn't explicitly enforce this requirement with a user-friendly error message.
If binary_data is None (SQL NULL), the send_file(BytesIO(binary_data), ...) call will raise an unhandled TypeError because it's outside the try/except block. This would result in a generic 500 error rather than a clear message to the user.
To properly implement the "no NULL downloads" requirement, I recommend adding an explicit check before calling send_file, as CodeRabbit suggested:
if binary_data is None:
return bad_request(
errormsg=gettext('The selected cell contains NULL.')
)
return send_file(
BytesIO(binary_data),
as_attachment=True,
download_name='binary_data',
mimetype='application/octet-stream'
)This way, users will receive a clear, informative error message when they attempt to download a NULL value, which better reflects your stated intent.
| // Use clientPK as the absolute row position | ||
| // rowKeyGetter returns the clientPK value which is a sequential counter (0, 1, 2, ...) | ||
| // that persists across pagination and represents the 0-based absolute position in the result set | ||
| const absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "client_primary_key|clientPK|__temp_PK" web/pgadmin/tools/sqleditor/static/js/components -C 3 | head -120Repository: pgadmin-org/pgadmin4
Length of output: 12195
🏁 Script executed:
# Find the rowKeyGetter function definition
rg -n "rowKeyGetter" web/pgadmin/tools/sqleditor/static/js -C 5 | head -100Repository: pgadmin-org/pgadmin4
Length of output: 10843
🏁 Script executed:
# Check the ResultSet component to see how rows are initialized
fd -t f "ResultSet" web/pgadmin/tools/sqleditor/static/js/components -name "*.jsx" -o -name "*.js"Repository: pgadmin-org/pgadmin4
Length of output: 294
🏁 Script executed:
rg -n "resetClientPKIndex" web/pgadmin/tools/sqleditor/static/js -C 4Repository: pgadmin-org/pgadmin4
Length of output: 3108
🏁 Script executed:
# Check pagination or data fetch logic
rg -n "pagination|offset|limit" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -30Repository: pgadmin-org/pgadmin4
Length of output: 689
🏁 Script executed:
rg -n "offset|rows_from|rows_to|pagination" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 2 -A 2 | grep -E "(getData|fetch|concat|append|setRows)" -A 3 -B 3Repository: pgadmin-org/pgadmin4
Length of output: 392
🏁 Script executed:
# Look for how new pagination results are appended to existing data
rg -n "setRows|setDataOutputQuery" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -A 5 | head -50Repository: pgadmin-org/pgadmin4
Length of output: 2139
🏁 Script executed:
sed -n '1130,1160p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxRepository: pgadmin-org/pgadmin4
Length of output: 1235
🏁 Script executed:
# Also check how getData is called and what newRows contains
rg -n "getData|newRows" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 3 -A 3 | grep -E "getData|newRows" -A 5 -B 5 | head -60Repository: pgadmin-org/pgadmin4
Length of output: 1605
🏁 Script executed:
# Check processRows implementation and its parameters
sed -n '705,725p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxRepository: pgadmin-org/pgadmin4
Length of output: 931
🏁 Script executed:
# Check how getWindowRows API is called and what parameters it receives
rg -n "getWindowRows" web/pgadmin/tools/sqleditor/static/js -B 3 -A 8Repository: pgadmin-org/pgadmin4
Length of output: 2642
🏁 Script executed:
# Look for the complete flow - check if clientPKLastIndex is ever reset during pagination
rg -n "clientPKLastIndex|resetClientPKIndex|fetchWindow" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -20Repository: pgadmin-org/pgadmin4
Length of output: 496
🏁 Script executed:
# Check the Formatters.jsx file to see the full context of how absoluteRowPos is used
cat -n web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx | sed -n '80,100p'Repository: pgadmin-org/pgadmin4
Length of output: 1203
🏁 Script executed:
# Search for any reset or offset handling in processRows or pagination logic
rg -n "clientPKLastIndex|fromRownum|offset|pagination" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | grep -E "fromRownum|offset|startRowNum" -A 2 -B 2Repository: pgadmin-org/pgadmin4
Length of output: 662
🏁 Script executed:
# Check if there's any mechanism to account for row offset when assigning clientPK
sed -n '700,730p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxRepository: pgadmin-org/pgadmin4
Length of output: 1425
🏁 Script executed:
# Search for TRIGGER_SAVE_BINARY_DATA event handler and backend cursor scroll usage
rg -n "TRIGGER_SAVE_BINARY_DATA" web/pgadmin/tools/sqleditor -C 5Repository: pgadmin-org/pgadmin4
Length of output: 4033
🏁 Script executed:
# Check the Python backend for cur.scroll usage
find web/pgadmin/tools/sqleditor -name "*.py" -type f | head -10Repository: pgadmin-org/pgadmin4
Length of output: 613
🏁 Script executed:
# Find the saveBinaryResultsToFile implementation in the backend
rg -n "saveBinaryResultsToFile|save_binary_results" web/pgadmin/tools/sqleditor -C 5Repository: pgadmin-org/pgadmin4
Length of output: 2551
🏁 Script executed:
# Search for the backend API endpoint that handles binary data download
find web/pgadmin/tools/sqleditor -name "*.py" -exec rg -l "binary.*download|scroll" {} \;Repository: pgadmin-org/pgadmin4
Length of output: 103
🏁 Script executed:
# Check the backend endpoint implementation
sed -n '1,100p' web/pgadmin/tools/sqleditor/__init__.py | grep -A 20 "download_binary_data"Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Search for the download_binary_data endpoint definition
rg -n "download_binary_data" web/pgadmin/tools/sqleditor/__init__.py -A 15Repository: pgadmin-org/pgadmin4
Length of output: 1327
🏁 Script executed:
# Get more of the download_binary_data implementation
sed -n '2195,2250p' web/pgadmin/tools/sqleditor/__init__.pyRepository: pgadmin-org/pgadmin4
Length of output: 1775
🏁 Script executed:
# Check how fromRownum/toRownum are passed and if there's offset calculation
sed -n '496,510p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxRepository: pgadmin-org/pgadmin4
Length of output: 742
🏁 Script executed:
# Check if there's any caching or prevention of re-fetching pages
rg -n "fetchWindow|getWindowRows" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 2 -A 8 | head -50Repository: pgadmin-org/pgadmin4
Length of output: 1757
🏁 Script executed:
# Verify the issue - check if processRows receives fromRownum parameter
rg -n "processRows" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 1 -A 1 | grep -E "processRows|fromRownum"Repository: pgadmin-org/pgadmin4
Length of output: 358
🏁 Script executed:
# Check the processRows method signature
sed -n '680,720p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxRepository: pgadmin-org/pgadmin4
Length of output: 1599
Fix clientPK calculation to account for pagination offset, and add radix to parseInt.
The clientPKLastIndex is incremented sequentially without considering the row offset from pagination. When fetchWindow requests rows from the backend (e.g., rows 100–199), processRows doesn't receive this offset, causing it to continue incrementing from its previous value. If a user navigates back to an earlier page, the rows are re-fetched and assigned clientPK values that don't match their actual absolute positions in the result set. This breaks the binary data download feature, which relies on absoluteRowPos to call cur.scroll(row_pos) on the backend with the correct absolute position.
For example: after fetching page 2 (rows 100–199 with clientPK 100–199), re-fetching page 1 (rows 0–99) would assign clientPK 200–299, causing downloads from page 1 to fetch from the wrong rows.
Additionally, parseInt should include the radix parameter:
- const absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0);
+ const absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0, 10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx`
around lines 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.
There was a problem hiding this comment.
clientPK does not holds the actual value but the key which points to actual value
There was a problem hiding this comment.
🧩 Analysis chain
@pravesh-sharma, thank you for the clarification! Let me investigate how clientPK is assigned and used to better understand the row positioning logic.
🏁 Script executed:
#!/bin/bash
# Check how clientPK is assigned in processRows
rg -n "clientPK.*=" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 3 -A 3
Length of output: 2665
🏁 Script executed:
#!/bin/bash
# Check the row structure and what fields are available
rg -n "client_primary_key|__temp_PK" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 2 -A 2
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check what the backend cur.scroll expects
sed -n '2190,2250p' web/pgadmin/tools/sqleditor/__init__.py
Length of output: 1849
🏁 Script executed:
#!/bin/bash
# Check if there's an absolute row position field stored in the row data
rg -n "row_pos|rowPos|absolute.*position" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -20
Length of output: 355
🏁 Script executed:
#!/bin/bash
# Check how processRows is called and what parameters it receives
rg -n "processRows\(" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 2 -A 5
Length of output: 1337
🏁 Script executed:
#!/bin/bash
# Check if there's logic to handle fromRownum or offset when processing rows
sed -n '705,725p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Check the getData method to see if it passes any offset information
sed -n '1130,1165p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 1265
🏁 Script executed:
#!/bin/bash
# Check if clientPKLastIndex is ever reset or adjusted based on pagination
rg -n "resetClientPKIndex|clientPKLastIndex\s*=" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Check when resetClientPKIndex is called
rg -n "resetClientPKIndex" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 5 -A 2
Length of output: 931
🏁 Script executed:
#!/bin/bash
# Check if fetchWindow is called after resetClientPKIndex
sed -n '235,245p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Check what happens on initial query execution
sed -n '910,925p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 616
🏁 Script executed:
#!/bin/bash
# Look at the getWindowRows API call to understand what fromRownum/toRownum means
rg -n "getWindowRows" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx -B 2 -A 8
Length of output: 817
🏁 Script executed:
#!/bin/bash
# Check if the backend response includes row position information
rg -n "fetch_window|from_rownum|to_rownum" web/pgadmin/tools/sqleditor/__init__.py -B 3 -A 10 | head -50
Length of output: 1944
🏁 Script executed:
#!/bin/bash
# Check what data structure is returned from getWindowRows
rg -n "rows_fetched_from|rows_fetched_to|pagination" web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx | head -20
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Check if there's any absolute row position tracking in the response
sed -n '1145,1160p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 700
🏁 Script executed:
#!/bin/bash
# Check what pagination.rows_from contains and if it's available
rg -n "rows_from|pagination" web/pgadmin/tools/sqleditor/__init__.py -B 2 -A 5 | grep -A 5 -B 2 "rows_from\|'pagination'"
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Check if fromRownum is available in the fetchWindow context
sed -n '1140,1155p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Check how the DataGrid receives startRowNum and if it's propagated to rows
rg -n "startRowNum" web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid -B 2 -A 2
Length of output: 2973
🏁 Script executed:
#!/bin/bash
# Check if pagination.rows_from is used to calculate the absolute position
sed -n '1595,1610p' web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Length of output: 831
🏁 Script executed:
#!/bin/bash
# Verify the backend cur.scroll() usage - does it expect absolute position from start?
sed -n '2230,2250p' web/pgadmin/tools/sqleditor/__init__.py
Length of output: 660
🏁 Script executed:
#!/bin/bash
# Check the backend response structure to see what rows_from represents
sed -n '1325,1360p' web/pgadmin/tools/sqleditor/__init__.py
Length of output: 1185
@pravesh-sharma, thank you for the clarification! I understand you're saying clientPK is a reference key rather than the actual data value. However, I believe CodeRabbit has identified a valid concern about how this key is being used for row positioning in the binary download feature.
Let me explain the issue:
Current Behavior:
clientPKis assigned sequentially usingclientPKLastIndex(line 717 in ResultSet.jsx)clientPKLastIndexincrements continuously and is only reset on new query execution (line 917)- When fetching different pagination windows:
- Fetch page 1 (rows 0-99):
clientPK= 0-99, counter at 100 - Fetch page 2 (rows 100-199):
clientPK= 100-199, counter at 200 - Fetch page 1 again:
clientPK= 200-299 ❌ (should be 0-99)
- Fetch page 1 (rows 0-99):
The Problem:
absoluteRowPosin Formatters.jsx usesrowKeyGetter(row)which returns theclientPKvalue- This value is sent to the backend as
rowposfor binary downloads - The backend's
cur.scroll(row_pos)expects the actual absolute position from the start of the result set (0-based) - Re-fetching earlier pages assigns wrong
clientPKvalues, causing downloads from wrong rows
The Solution:
The pagination offset (pagination.rows_from) is already available (line 1603 in ResultSet.jsx) but not used when assigning clientPK. You should either:
- Pass offset to
processRows():
// In fetchWindow around line 1147
const newRows = rsu.current.processRows(res.data.data.result, columns, {
startIndex: res.data.data.pagination.rows_from
});
// In processRows around line 717
rowObj[this.clientPK] = (options.startIndex || 0) + recIdx;- Or use
rowIdx + startRowNumin BinaryFormatter:
const absoluteRowPos = rowIdx + (dataGridExtras?.startRowNum ?? 0);Also, please add the radix parameter to parseInt:
-const absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0);
+const absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0, 10);Does this clarify the concern? Would you like me to create a plan to fix this issue?
🧬 Code Graph Analysis Results
[file: web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx]
Summary: BinaryFormatter uses clientPK-derived absolute row position via dataGridExtras.rowKeyGetter.
-
Description:
- Function: BinaryFormatter({ row, column })
- Key behavior:
- Computes absoluteRowPos using parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0)
- The code comment explains: "Use clientPK as the absolute row position" and "rowKeyGetter returns the clientPK value which is a sequential counter (0, 1, 2, ...) that persists across pagination and represents the 0-based absolute position in the result set"
- Rationale relevant to user comment: clarifies that the identifier used to reference a row is a key (clientPK) that maps to the actual row position/value, not the value itself. This aligns with understanding that clientPK is an index/key, not the data payload.
-
Additional context present in this snippet:
- The absoluteRowPos is used to trigger actions (e.g., TRIGGER_SAVE_BINARY_DATA) for the given row
- There is a DataGridExtrasContext that supplies rowKeyGetter
[file: web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/index.jsx]
Summary: DataGridExtrasContext provides a rowKeyGetter function and is used to map a row to its absolute position (clientPK).
-
Description:
- Context: DataGridExtrasContext
- Usage patterns observed:
- const dataGridExtras = useContext(DataGridExtrasContext);
- absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0)
- Note: The rowKeyGetter is described as returning the clientPK value for a given row, which is the 0-based absolute position in the full result set (across pagination). This is the key mechanism that the BinaryFormatter uses to identify a row, rather than using the actual data value directly.
-
Relevance to user comment:
- Confirms that clientPK is a reference key pointing to the actual row/value, not the actual value itself. The code relies on rowKeyGetter(row) to obtain the positional key that identifies the row in the dataset.
| // rowKeyGetter returns the clientPK value which is a sequential counter (0, 1, 2, ...) | ||
| // that persists across pagination and represents the 0-based absolute position in the result set | ||
| const absoluteRowPos = parseInt(dataGridExtras?.rowKeyGetter?.(row) ?? 0); | ||
| console.log(absoluteRowPos) |
There was a problem hiding this comment.
Remove debug console.log — breaks CI pipeline.
This leftover debug statement causes ESLint failures (no-console and semi) across all CI checks. Remove it.
🐛 Proposed fix
- console.log(absoluteRowPos)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(absoluteRowPos) |
🧰 Tools
🪛 GitHub Actions: Check Javascript style
[error] 87-87: no-console: Unexpected console statement. Only these console methods are allowed: warn, error, trace
[error] 87-87: semi: Missing semicolon
🪛 GitHub Actions: Run feature tests on PostgreSQL
[error] 87-87: ESLint: Unexpected console statement. Only these console methods are allowed: warn, error, trace (no-console)
[error] 87-87: ESLint: Missing semicolon (semi)
🪛 GitHub Actions: Run Javascript tests
[error] 87-87: no-console: Unexpected console statement. Only these methods are allowed: warn, error, trace.
🪛 GitHub Check: run-javascript-tests (macos-latest)
[failure] 87-87:
Missing semicolon
[failure] 87-87:
Unexpected console statement. Only these console methods are allowed: warn, error, trace
🪛 GitHub Check: run-javascript-tests (ubuntu-22.04)
[failure] 87-87:
Missing semicolon
[failure] 87-87:
Unexpected console statement. Only these console methods are allowed: warn, error, trace
🪛 GitHub Check: run-javascript-tests (windows-latest)
[failure] 87-87:
Missing semicolon
[failure] 87-87:
Unexpected console statement. Only these console methods are allowed: warn, error, trace
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/pgadmin/tools/sqleditor/static/js/components/QueryToolDataGrid/Formatters.jsx`
at 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.
1 participant
Summary by CodeRabbit
New Features
New Preferences
Server
Documentation