Preserve database errors from index metadata queries - #10476
IANTHEREAL wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pgadmin-org/pgadmin4/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughIndex metadata query failures in the index utilities now raise ChangesIndex query error propagation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Index metadata query failures now keep the original database error message instead of being replaced by a misleading internal error. The change is small and covered by handler-level tests. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Independent review: PASS at 2ca7594. The reviewer traced Properties, SQL, modified SQL, update, schema comparison, and table/partition reverse SQL callers. No caller depends on the previous erroneous HTTP-response-as-data return shape. Existing ExecuteError handling preserves the driver diagnostic through Flask or the handler catch boundary. Independent candidate run: 11/11; in-memory baseline falsification: 8 failures, confirming coverage catches the masking defects. Native qualification also completed using pgAdmin 4 9.18 with this exact utility source change (base utility bytes match upstream). The original client displayed its own TypeError. The candidate HTTP response and visible dialog retain the server's named index diagnostic and detail. After explicitly replacing the owned legacy test index, Refresh restores complete index Properties with HTTP 200. No server diagnostics were suppressed or rewritten. Ready for upstream maintainer review; no claim that a released pgAdmin build already includes this fix. |
When an index metadata query failed, the Properties and SQL handlers could replace the original database error with 'string indices must be integers', or with an error about using a Response as index metadata. get_column_details() read rset['rows'] before checking the query status, and get_column_details(), get_include_details() and get_sql() returned HTTP responses where their callers expect data. Raise ExecuteError instead, as get_reverse_engineered_sql() already does, so the original database error reaches the user. Adds regression tests covering Properties, SQL, modified SQL and update.
|
Thanks, Ian. Squashed and merged to master as 9d63ea9, with one line in the new test wrapped to satisfy pep8 (E501). |
3 participants
When an index metadata query fails, Properties and SQL handlers can replace the original database error with
string indices must be integersor an error about using a Response as index metadata.get_column_details()accessesrset['rows']before checking the driver status, and both column helpers return HTTP responses where callers expect dictionaries. The update SQL helper has the same mismatch where its callers expect a tuple.Raise the existing
ExecuteErrorbefore reading failed query results, consistently withget_reverse_engineered_sql(). Flask or the existing handler catches then preserve the original diagnostic. Healthy response shapes stay unchanged.Added 11 HTTP-boundary regression scenarios covering Properties, SQL, modified SQL and update queries, failures at each metadata stage, and healthy duplicate-column/INCLUDE handling. The original source fails 8 scenarios; the candidate passes all 11. These tests use the real handlers and utilities with driver failure injection and Flask response handling, without requiring a database server.
Discovered while qualifying pgAdmin 4 9.18 against a PostgreSQL-compatible server which returns a named index diagnostic for a damaged legacy expression definition (db9-ai/db9-server#4921). The utility source is identical in 9.18 and the upstream base used here. Native qualification with pgAdmin 4 9.18 plus this committed utility fix confirmed that both HTTP 500 and the visible dialog preserve the original named server diagnostic (SQLSTATE 55000), then an explicit replacement of the owned test index followed by Refresh restored HTTP 200 and complete Properties. The source fix also passed an independent review and independent 11/11 regression run.