Fixed View/Edit Data not handling generated columns properly. #9672#9761
Fixed View/Edit Data not handling generated columns properly. #9672#9761RohitBhati8269 wants to merge 1 commit intopgadmin-org:masterfrom
Conversation
WalkthroughAdds PostgreSQL 12+ generated-column support to the data editor: new SQL template exposes an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as ResultSet.jsx
participant Server as save_changed_data.py
participant DB as PostgreSQL
User->>Client: Edit/insert row (may include generated cols)
Client->>Server: Submit changes (added/updated rows)
Server->>Server: Query column metadata (includes is_generated)
Server->>Server: Strip is_generated cols from payload
alt INSERT
Server->>DB: INSERT (without generated cols)
DB->>DB: Compute generated column values
DB-->>Server: Insert result (may include RETURNING)
else UPDATE
Server->>DB: UPDATE (without generated cols, with RETURNING PK/OID)
DB->>DB: Compute generated column values
Server->>DB: SELECT refreshed row by PK/OID
DB-->>Server: Refetched row with recalculated values
end
Server-->>Client: Return result + refetched rows
Client->>Client: Update grid for added/updated rows
Client->>User: Display updated data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 1
🤖 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/utils/save_changed_data.py`:
- Around line 173-183: The refetch logic must be guarded so we only enqueue a
refetch when the UPDATE can actually return identifiers: compute a boolean like
can_refetch_updated_row using command_obj.has_oids() or bool(pk_names) (pk_names
/ primary_keys are from command_obj.get_primary_keys()), and change the
condition that currently uses has_generated_cols alone to require both
has_generated_cols and can_refetch_updated_row before enqueuing the SELECT (the
place that later reads res['rows'][0]); this prevents attempting to read
returned rows when the UPDATE had no RETURNING clause.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdddcb83-6151-4d68-8a0e-6a6344e21f89
📒 Files selected for processing (5)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/12_plus/nodes.sqlweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sqlweb/pgadmin/tools/sqleditor/utils/get_column_types.pyweb/pgadmin/tools/sqleditor/utils/save_changed_data.py
✅ Files skipped from review due to trivial changes (1)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/12_plus/nodes.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
- web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql
| # Check if table has generated columns. If yes, we need to | ||
| # refetch row after UPDATE to get recalculated values for UI. | ||
| has_generated_cols = any( | ||
| col_info.get('is_generated', False) | ||
| for col_info in columns_info.values() | ||
| ) | ||
|
|
||
| # Get primary keys info (same as INSERT) - needed for RETURNING | ||
| # clause and SELECT query to refetch updated row. | ||
| pk_names, primary_keys = command_obj.get_primary_keys() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect get_primary_keys implementations/returns used by sqleditor commands
rg -n -C3 '\bdef get_primary_keys\s*\(' web/pgadmin/tools/sqleditor web/pgadmin/browser
rg -n -C2 '\breturn\b.*(pk_names|primary_keys|keys)' web/pgadmin/tools/sqleditor web/pgadmin/browser
# 2) Trace update.sql/select.sql call sites and assumptions on returned rows
rg -n -C3 'render_template\(.+update\.sql|render_template\(.+select\.sql|res\[[\"\x27]rows[\"\x27]\]\[0\]' web/pgadmin/tools/sqleditor/utils
# 3) Check tests around generated columns + update/refetch + pk/oids edge cases
rg -n -C2 'generated|is_generated|save_changed_data|has_oids|get_primary_keys|update' web/pgadmin/tools/sqleditor/testsRepository: pgadmin-org/pgadmin4
Length of output: 6571
🏁 Script executed:
cat -n web/pgadmin/tools/sqleditor/utils/save_changed_data.py | sed -n '160,250p'Repository: pgadmin-org/pgadmin4
Length of output: 4654
🏁 Script executed:
rg -n 'def has_oids' web/pgadmin/tools/sqleditorRepository: pgadmin-org/pgadmin4
Length of output: 216
🏁 Script executed:
rg -n 'res\[.rows.\]\[0\]' web/pgadmin/tools/sqleditor/utils -A 2 -B 2Repository: pgadmin-org/pgadmin4
Length of output: 1129
🏁 Script executed:
find web/pgadmin/tools/sqleditor -name 'update.sql' -o -name '*.sql' | head -20Repository: pgadmin-org/pgadmin4
Length of output: 1396
🏁 Script executed:
rg -n 'RETURNING' web/pgadmin/tools/sqleditor -B 3 -A 1 | head -80Repository: pgadmin-org/pgadmin4
Length of output: 1801
🏁 Script executed:
cat -n web/pgadmin/tools/sqleditor/command.py | sed -n '553,570p'Repository: pgadmin-org/pgadmin4
Length of output: 781
🏁 Script executed:
cat -n web/pgadmin/tools/sqleditor/command.py | sed -n '838,850p'Repository: pgadmin-org/pgadmin4
Length of output: 541
🏁 Script executed:
cat -n web/pgadmin/tools/sqleditor/utils/save_changed_data.py | sed -n '330,365p'Repository: pgadmin-org/pgadmin4
Length of output: 1924
🏁 Script executed:
cat -n web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sqlRepository: pgadmin-org/pgadmin4
Length of output: 886
🏁 Script executed:
rg -n 'get_primary_keys' web/pgadmin/tools/sqleditor/command.py -A 10 | head -60Repository: pgadmin-org/pgadmin4
Length of output: 1985
🏁 Script executed:
cat -n web/pgadmin/tools/sqleditor/command.py | sed -n '507,546p'Repository: pgadmin-org/pgadmin4
Length of output: 1656
🏁 Script executed:
rg -n 'test.*generated|test.*refetch|test.*update' web/pgadmin/tools/sqleditor/tests/ -lRepository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check if there are tests for the UPDATE + generated columns scenario
rg -n 'has_generated' web/pgadmin/tools/sqleditor/tests/ -B 2 -A 2Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check for any error handling or guards around res['rows'][0]
rg -n "res\['rows'\]" web/pgadmin/tools/sqleditor/utils/save_changed_data.py -B 3 -A 3Repository: pgadmin-org/pgadmin4
Length of output: 882
🏁 Script executed:
# Check if pk_names could be empty string (falsy) or explicitly None
cat -n web/pgadmin/tools/sqleditor/command.py | sed -n '518,542p'Repository: pgadmin-org/pgadmin4
Length of output: 1049
🏁 Script executed:
# Look for any validation or tests of the refetch flow
fd test.*save_changed web/pgadmin/tools/sqleditor/tests/Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Verify the logic: if pk_names is empty string, is it falsy in Jinja?
# Also check what execute_dict returns when there's no RETURNING clause
rg -n 'execute_dict|execute_void' web/pgadmin/tools/sqleditor/utils/save_changed_data.py -B 2 -A 2 | head -40Repository: pgadmin-org/pgadmin4
Length of output: 1519
🏁 Script executed:
# Check if there's error handling for empty res['rows']
rg -n "rows.*\[\s*0\s*\]" web/pgadmin/tools/sqleditor/utils/save_changed_data.py -B 5 -A 2Repository: pgadmin-org/pgadmin4
Length of output: 817
🏁 Script executed:
# Verify UserDefinedQueryCommand initialization with table_has_oids
rg -n 'table_has_oids|self.pk_names' web/pgadmin/tools/sqleditor/command.py | head -30Repository: pgadmin-org/pgadmin4
Length of output: 493
🏁 Script executed:
# Verify what execute_dict returns when there's no RETURNING clause
rg -n 'def execute_dict' web/pgadmin/ -A 15 | head -50Repository: pgadmin-org/pgadmin4
Length of output: 3713
🏁 Script executed:
# Check if there are integration tests for generated columns + update
find web/pgadmin/tools/sqleditor/tests -name '*.py' -exec grep -l 'generated\|refetch' {} \;Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Trace the issue: confirm empty string pk_names is falsy in Jinja template
cat web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sqlRepository: pgadmin-org/pgadmin4
Length of output: 816
🏁 Script executed:
# Verify what happens when execute_dict returns no rows (no RETURNING clause)
cat -n web/pgadmin/utils/driver/psycopg3/connection.py | sed -n '1286,1350p'Repository: pgadmin-org/pgadmin4
Length of output: 2984
🏁 Script executed:
# Final verification: trace complete flow when UPDATE has no RETURNING clause
rg -n "res\['rows'\]" web/pgadmin/utils/driver/psycopg3/connection.py -A 3 -B 3 | head -60Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
cat -n web/pgadmin/utils/driver/psycopg3/connection.py | sed -n '1350,1365p'Repository: pgadmin-org/pgadmin4
Length of output: 756
Guard refetch enqueueing when UPDATE cannot return identifiers.
When a table has generated columns, refetch is enabled (line 220). However, the UPDATE statement only includes a RETURNING clause if pk_names is non-empty AND has_oids is false, OR if has_oids is true (update.sql lines 9–10). If both conditions fail—e.g., a table with no primary keys on PostgreSQL 12+ (where has_oids() returns False)—the UPDATE has no RETURNING clause and returns an empty row set.
Line 347 then unconditionally accesses res['rows'][0] to extract parameters for the refetch SELECT, causing an IndexError when the list is empty. This crash can occur on valid tables with generated columns but no primary keys.
Add a guard to enqueue refetch only when identifiers are returnable:
can_refetch_updated_row = command_obj.has_oids() or bool(pk_names)
if has_generated_cols and can_refetch_updated_row:
# enqueue select_sql🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/sqleditor/utils/save_changed_data.py` around lines 173 -
183, The refetch logic must be guarded so we only enqueue a refetch when the
UPDATE can actually return identifiers: compute a boolean like
can_refetch_updated_row using command_obj.has_oids() or bool(pk_names) (pk_names
/ primary_keys are from command_obj.get_primary_keys()), and change the
condition that currently uses has_generated_cols alone to require both
has_generated_cols and can_refetch_updated_row before enqueuing the SELECT (the
place that later reads res['rows'][0]); this prevents attempting to read
returned rows when the UPDATE had no RETURNING clause.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes