fix: exclude non-editable/alias columns from Query Tool row UPDATE

[kundansable]

Summary

Fixes #10103 — editing a row in the Query Tool result grid fails to save with column "..." does not exist when the query includes an aliased/computed column (e.g. first_name || ' ' || last_name AS the_name), even though that column is already shown as read-only (lock icon) in the grid.

Root Cause

save_changed_data() has two code paths — added (INSERT) and updated (UPDATE). The added path already filters the outgoing payload down to real, editable table columns (fixed for #9939 in #10015), using the same columns_info[...]['is_editable'] metadata that drives the grid's lock icon. The updated path never got the equivalent filter, so an edited row that includes an alias/expression column sends that alias straight into the generated UPDATE ... SET clause — and Postgres rejects it because there's no such real column.

Fix

Apply the same editable-columns guard to the updated branch: drop any key from the changed-row payload that isn't a real, editable table column, and skip the row entirely if nothing editable remains after filtering (avoiding an empty/invalid SET clause).

Test Steps

1. In the Query Tool, run:

   DROP TABLE IF EXISTS some_table;
   CREATE TABLE some_table (id INT PRIMARY KEY, first_name TEXT, last_name TEXT);
   INSERT INTO some_table VALUES (1, 'John', 'Doe');
   
   SELECT id, first_name, last_name,
          first_name || ' ' || last_name AS the_name
   FROM some_table;

2. In the result grid, edit first_name from John to Jane on the existing row.
3. Press F5 / Save.
4. Expected: save succeeds; only first_name is updated. No column "the_name" ... does not exist error.
5. Regression: repeat on a query with no alias/expression columns and confirm normal edits still save correctly.

Summary by CodeRabbit

• Bug Fixes
  • Improved saving of edited table data when query results include calculated, aliased, or otherwise non-editable columns.
  • Prevented invalid update operations by ignoring non-editable fields and skipping rows that have no editable values.
  • Updates now reliably reference the correct row identifier when applying changes, avoiding misaligned or malformed UPDATE statements.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5d7d11d8-9dc4-43f0-aefb-220ee5fcd157

📥 Commits

Reviewing files that changed from the base of the PR and between 8e95bb4 and e15b0a8.

📒 Files selected for processing (1)

• web/pgadmin/tools/sqleditor/utils/save_changed_data.py

🚧 Files skipped from review as they are similar to previous changes (1)

• web/pgadmin/tools/sqleditor/utils/save_changed_data.py

---

Walkthrough

The updated save path captures each row identifier before removing tracking fields, filters row data to editable table columns, skips rows without editable fields, and records updates using the preserved identifier.

Changes

Update filtering

Layer / File(s)	Summary
Filter editable update fields web/pgadmin/tools/sqleditor/utils/save_changed_data.py	The updated operation preserves the row identifier, removes expression and alias columns, skips rows with no editable fields, and uses the captured identifier when creating UPDATE items.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: hiteshjambhale

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: filtering out non-editable or alias columns from Query Tool row updates.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

web/pgadmin/tools/sqleditor/utils/save_changed_data.py (1)

180-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression coverage for updated rows with read-only aliases.

Please add or verify tests covering:

• an updated row containing both an editable column and an alias/computed column;
• a row containing only a non-editable alias.

Assert that the editable value is persisted and the alias-only row produces no UPDATE.

🤖 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 180 -
194, Extend the regression tests covering the save-changed-data flow around the
data filtering logic to include a row with both an editable column and a
non-editable alias, asserting the editable value is persisted, and a row
containing only the alias, asserting no UPDATE is generated or executed. Reuse
the existing test fixtures and assertions for updated rows and SQL persistence.

🤖 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.

Nitpick comments:
In `@web/pgadmin/tools/sqleditor/utils/save_changed_data.py`:
- Around line 180-194: Extend the regression tests covering the
save-changed-data flow around the data filtering logic to include a row with
both an editable column and a non-editable alias, asserting the editable value
is persisted, and a row containing only the alias, asserting no UPDATE is
generated or executed. Reuse the existing test fixtures and assertions for
updated rows and SQL persistence.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d701b2fa-7918-4e34-9c15-30690aac3d3f

📥 Commits

Reviewing files that changed from the base of the PR and between b15c745 and 8e95bb4.

📒 Files selected for processing (1)

• web/pgadmin/tools/sqleditor/utils/save_changed_data.py

[asheshv]

Consistency review: added-row vs updated-row column filtering

Nice fix for #10103 — filtering non-editable columns out of data before rendering the SQL template stops the UPDATE from referencing expression/alias columns.

One thing worth double-checking: the added-row path explicitly strips the tracking keys before filtering:

# lines 113-115
data.pop(client_primary_key, None)
data.pop('is_row_copied', None)
...
data = {
    k: v for k, v in data.items()
    if k in columns_info and columns_info[k].get('is_editable', True)
}

The updated-row path (this PR, lines 179-183) relies solely on the k in columns_info filter to drop them:

data = changed_data[of_type][each_row]['data']
data = {
    k: v for k, v in data.items()
    if k in columns_info and columns_info[k].get('is_editable', True)
}

I traced client_primary_key/is_row_copied on the frontend — they're only injected into the added payload (ResultSet.jsx:759 calls setClientPK for new rows), so today the updated-row data dict never actually contains them, and the columns_info filter alone is sufficient. So I don't think this is a live bug right now.

Two things I'd still flag:

1. Defense-in-depth / consistency — if a future change ever starts tagging updated rows the same way (e.g. multi-row copy-paste into existing rows), this path would silently rely on columns_info to save it, instead of being explicit like the added-row path. Mirroring the explicit .pop() calls here would make both code paths symmetric and remove the implicit assumption.
2. row_id after filtering (line 197-200):

   list_of_sql[of_type].append({'sql': sql,
                                'data': data,
                                'row_id':
                                    data.get(client_primary_key)})

   Since data here is now the filtered dict, data.get(client_primary_key) will always be None for updated rows (as it presumably already was, since client_primary_key was never in the updated payload to begin with — so no regression, just flagging that this line is dead weight / possibly should read from the unfiltered changed_data[...]['data'] if row_id was ever meant to be populated here).

Not blocking, just wanted these on record in case someone hits it while extending this code path later.