Fix View/Edit Data crash on a stale/non-filter session transaction object (#9744)#10071
Open
dpage wants to merge 2 commits into
Open
Fix View/Edit Data crash on a stale/non-filter session transaction object (#9744)#10071dpage wants to merge 2 commits into
dpage wants to merge 2 commits into
Conversation
…ject initialize_viewdata restores the filter and data-sorting from any command object previously stored in session['gridData'] under the same trans_id. It assumed that object was always a filter-capable (View/Edit Data) command and accessed old_trans_obj._row_filter / ._data_sorting directly. That assumption is wrong: the same trans_id may have been used by the Query Tool (a QueryToolCommand, which does not inherit SQLFilter), or the session may contain an incompatible object persisted by an older version after an upgrade. In those cases the attribute access raised AttributeError, returning a 500 from the endpoint and - in desktop mode, where this runs during startup - preventing the application from loading at all. Guard the restore with isinstance(old_trans_obj, SQLFilter) (short-circuited before the did/obj_id checks) so a non-filter object is simply skipped. Add a regression test that seeds a pickled QueryToolCommand under the trans_id and asserts initialize/viewdata returns 200. Closes pgadmin-org#9744
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a guard in View/Edit Data initialization to only restore filter/sorting from session objects that are ChangesStale Transaction Object Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
web/pgadmin/tools/sqleditor/tests/test_view_data_restore_stale_trans.py (1)
75-76: ⚡ Quick winUse parameterized query instead of string formatting.
While
self.tableis constructed from a UUID and not user-controlled, it's still better practice to use parameterized queries to avoid SQL injection patterns being flagged by static analysis and to set a good example in test code.♻️ Proposed fix
- pg_cursor.execute("""Select oid FROM pg_catalog.pg_class WHERE - relname = '%s' AND relkind IN ('r','s','t')""" % self.table) + pg_cursor.execute("""Select oid FROM pg_catalog.pg_class WHERE + relname = %s AND relkind IN ('r','s','t')""", (self.table,))🤖 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/tests/test_view_data_restore_stale_trans.py` around lines 75 - 76, Replace the string-formatted SQL passed to pg_cursor.execute with a parameterized query: call pg_cursor.execute with a SQL string containing a placeholder for relname and pass self.table as a separate parameter tuple (so the DB driver binds it), e.g. use "SELECT oid FROM pg_catalog.pg_class WHERE relname = %s AND relkind IN ('r','s','t')" and supply (self.table,) as the params for the pg_cursor.execute call to avoid inline string formatting.Source: Linters/SAST tools
🤖 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/tests/test_view_data_restore_stale_trans.py`:
- Around line 75-76: Replace the string-formatted SQL passed to
pg_cursor.execute with a parameterized query: call pg_cursor.execute with a SQL
string containing a placeholder for relname and pass self.table as a separate
parameter tuple (so the DB driver binds it), e.g. use "SELECT oid FROM
pg_catalog.pg_class WHERE relname = %s AND relkind IN ('r','s','t')" and supply
(self.table,) as the params for the pg_cursor.execute call to avoid inline
string formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4de1cb5-8a59-4572-a7e3-89ad729c27c7
📒 Files selected for processing (3)
docs/en_US/release_notes_9_16.rstweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/tools/sqleditor/tests/test_view_data_restore_stale_trans.py
The regression test created its table with a fixed 'table_pk' primary key constraint name, which collides with the same name used by TestViewData in this package when the full suite runs against one database. That made the CREATE TABLE fail in CI (table not found -> IndexError on the OID lookup). Let PostgreSQL auto-name the primary key instead.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in View/Edit Data initialization when session['gridData'] contains a stale/non-filter command object under the same trans_id (e.g., a QueryToolCommand), preventing AttributeError and a resulting 500 (notably breaking desktop startup after upgrades).
Changes:
- Guard filter/sorting restoration in
initialize_viewdata()withisinstance(old_trans_obj, SQLFilter)to skip incompatible session objects. - Add a regression test that seeds a pickled
QueryToolCommandintosession['gridData']and asserts initialization succeeds. - Add a release note entry for issue #9744.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/pgadmin/tools/sqleditor/init.py | Prevents View/Edit Data from crashing when restoring state from a non-filter/stale session transaction object. |
| web/pgadmin/tools/sqleditor/tests/test_view_data_restore_stale_trans.py | Adds regression coverage for stale session transaction objects (Query Tool → View/Edit Data). |
| docs/en_US/release_notes_9_16.rst | Documents the fix in the 9.16 release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
331
to
333
| command_obj.set_filter(old_trans_obj._row_filter) | ||
| command_obj.set_data_sorting( | ||
| dict(data_sorting=old_trans_obj._data_sorting), True) |
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
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
initialize_viewdata()restores the filter and data-sorting from any commandobject previously stored in
session['gridData']under the sametrans_id,and assumed that object was always a filter-capable (View/Edit Data) command —
accessing
old_trans_obj._row_filter/._data_sortingdirectly.That assumption is wrong. The same
trans_idmay have been used by the QueryTool (a
QueryToolCommand, which does not inheritSQLFilter), or thesession may contain an incompatible object persisted by an older version after
an upgrade. In those cases the attribute access raised
AttributeError,returning a 500 from the endpoint and — in desktop mode, where this runs during
startup — preventing the application from loading at all (issue #9744:
'QueryToolCommand' object has no attribute '_row_filter').The fix guards the restore with
isinstance(old_trans_obj, SQLFilter),short-circuited before the
did/obj_idchecks, so a non-filter or staleobject is simply skipped (there is no meaningful filter/sorting to restore from
it anyway).
Changes
web/pgadmin/tools/sqleditor/__init__.py— guard the filter/sorting restorewith an
isinstance(..., SQLFilter)check.web/pgadmin/tools/sqleditor/tests/test_view_data_restore_stale_trans.py—new regression test: seeds a pickled
QueryToolCommandunder thetrans_idand asserts
initialize/viewdatareturns 200.docs/en_US/release_notes_9_16.rst— changelog entry.Test plan
Closes #9744
Summary by CodeRabbit