Skip to content

Fixed View/Edit Data not handling generated columns properly. #9672#9761

Open
RohitBhati8269 wants to merge 2 commits into
pgadmin-org:masterfrom
RohitBhati8269:GH-9672
Open

Fixed View/Edit Data not handling generated columns properly. #9672#9761
RohitBhati8269 wants to merge 2 commits into
pgadmin-org:masterfrom
RohitBhati8269:GH-9672

Conversation

@RohitBhati8269

@RohitBhati8269 RohitBhati8269 commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Added PostgreSQL 12+ generated-column awareness in column metadata and the SQL editor, including display of generated/sequence info.
  • Bug Fixes

    • Excluded GENERATED columns from INSERT/UPDATE payloads to avoid errors.
    • Ensured post-save refresh recalculates generated-column values after both INSERT and UPDATE by returning and consuming updated rows.

@coderabbitai

coderabbitai Bot commented Mar 17, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Expose generated-column metadata, skip generated columns in INSERT/UPDATE payloads, optionally RETURN full updated rows for UPDATEs to refetch recalculated generated values, and refresh client grid for added or updated rows.

Changes

Data editor generated-column support

Layer / File(s) Summary
UPDATE template returning control
web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql
UPDATE template now conditionally appends RETURNING * controlled by return_all_columns; statement still terminates with ; when not returning rows.
Strip generated columns on INSERT
web/pgadmin/tools/sqleditor/utils/save_changed_data.py
INSERT path filters per-row data to remove keys for columns marked is_generated.
Prepare UPDATE jobs excluding generated columns
web/pgadmin/tools/sqleditor/utils/save_changed_data.py
Detect has_generated_cols, remove generated columns from each updated row’s data, compute escaped primary-key values, and render UPDATE with return_all_columns=has_generated_cols; queue update entries with returning_all and client_row when needed.
Execution and result handling for returning updates
web/pgadmin/tools/sqleditor/utils/save_changed_data.py
Choose execute_dict vs execute_void via a needs_result flag; when returning_all is set, populate row_added from returned rows using client_row and update related comments.
Column type parsing
web/pgadmin/tools/sqleditor/utils/get_column_types.py
Populate an is_generated boolean on returned column type entries from the column-metadata query result (defaulting to False when absent).
UI row refresh
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
Post-save refresh now triggers when either dataChangeStore.added or dataChangeStore.updated is non-empty; comment clarifies refetched qr.row_added covers both INSERT and UPDATE flows.
Column metadata SQL
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/12_plus/nodes.sql
New Jinja SQL template selects column metadata including computed is_generated (from attgenerated = 's') plus datatype/display, not-null, default/sequence info, optional description, filters, and ordering.

Sequence Diagram(s)

sequenceDiagram
  participant ComponentA
  participant ComponentB
  ComponentA->>ComponentB: observable interaction
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: fixing how the View/Edit Data feature handles generated columns, with reference to the issue number.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch GH-9672

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 359942f and 85916a6.

📒 Files selected for processing (5)
  • web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/columns/sql/12_plus/nodes.sql
  • web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx
  • web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql
  • web/pgadmin/tools/sqleditor/utils/get_column_types.py
  • web/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

Comment thread web/pgadmin/tools/sqleditor/utils/save_changed_data.py Outdated
Comment thread web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql Outdated
Comment thread web/pgadmin/tools/sqleditor/utils/save_changed_data.py Outdated
Comment thread web/pgadmin/tools/sqleditor/utils/save_changed_data.py Outdated
Comment thread web/pgadmin/tools/sqleditor/utils/save_changed_data.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
web/pgadmin/tools/sqleditor/utils/save_changed_data.py (1)

185-210: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Skip UPDATE generation when no mutable columns remain.

After Line 187 filters generated columns, data can be empty. Rendering update.sql then produces an invalid UPDATE ... SET ... statement and fails the whole save operation. Add an early guard before rendering SQL.

Proposed fix
                 # Remove generated columns (GENERATED ALWAYS AS) as they
                 # cannot be updated - PostgreSQL auto-computes their values.
                 data = {k: v for k, v in data.items()
                         if not columns_info.get(k, {}).get('is_generated',
                                                            False)}
+
+                # Nothing left to update after filtering generated columns.
+                if not data:
+                    continue
 
                 pk_escaped = {
                     pk: pk_val.replace('%', '%%') if hasattr(
                         pk_val, 'replace') else pk_val
🤖 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 `@web/pgadmin/tools/sqleditor/utils/save_changed_data.py` around lines 185 -
210, After filtering out generated columns in save_changed_data.py the local
variable data may be empty which causes render_template(... 'update.sql') to
produce an invalid UPDATE; add an early guard after the generated-column filter
that checks if not data and skips UPDATE generation—e.g., return/resolve as a
no-op success or continue without calling render_template—so functions like
render_template, update.sql and variables row_primary_keys, pk_escaped,
has_generated_cols are not used when there are no mutable columns to persist.
🤖 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 `@web/pgadmin/tools/sqleditor/utils/save_changed_data.py`:
- Around line 185-210: After filtering out generated columns in
save_changed_data.py the local variable data may be empty which causes
render_template(... 'update.sql') to produce an invalid UPDATE; add an early
guard after the generated-column filter that checks if not data and skips UPDATE
generation—e.g., return/resolve as a no-op success or continue without calling
render_template—so functions like render_template, update.sql and variables
row_primary_keys, pk_escaped, has_generated_cols are not used when there are no
mutable columns to persist.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f9652c5c-55b2-4956-bebc-c50c154ff5d1

📥 Commits

Reviewing files that changed from the base of the PR and between 85916a6 and a8ad536.

📒 Files selected for processing (2)
  • web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/update.sql
  • web/pgadmin/tools/sqleditor/utils/save_changed_data.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants