-
Notifications
You must be signed in to change notification settings - Fork 847
Fixed View/Edit Data not handling generated columns properly. #9672 #9761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| SELECT DISTINCT att.attname as name, att.attnum as OID, pg_catalog.format_type(ty.oid,NULL) AS datatype, | ||
| pg_catalog.format_type(ty.oid,att.atttypmod) AS displaytypname, | ||
| att.attnotnull as not_null, | ||
| CASE WHEN att.atthasdef OR att.attidentity != '' OR ty.typdefault IS NOT NULL THEN True | ||
| ELSE False END as has_default_val, des.description, seq.seqtypid, | ||
| {# Detect generated columns to exclude from INSERT/UPDATE in View/Edit Data #} | ||
| CASE WHEN att.attgenerated = 's' THEN true ELSE false END as is_generated | ||
| FROM pg_catalog.pg_attribute att | ||
| JOIN pg_catalog.pg_type ty ON ty.oid=atttypid | ||
| JOIN pg_catalog.pg_namespace tn ON tn.oid=ty.typnamespace | ||
| JOIN pg_catalog.pg_class cl ON cl.oid=att.attrelid | ||
| JOIN pg_catalog.pg_namespace na ON na.oid=cl.relnamespace | ||
| LEFT OUTER JOIN pg_catalog.pg_type et ON et.oid=ty.typelem | ||
| LEFT OUTER JOIN pg_catalog.pg_attrdef def ON adrelid=att.attrelid AND adnum=att.attnum | ||
| LEFT OUTER JOIN (pg_catalog.pg_depend JOIN pg_catalog.pg_class cs ON classid='pg_class'::regclass AND objid=cs.oid AND cs.relkind='S') ON refobjid=att.attrelid AND refobjsubid=att.attnum | ||
| LEFT OUTER JOIN pg_catalog.pg_namespace ns ON ns.oid=cs.relnamespace | ||
| LEFT OUTER JOIN pg_catalog.pg_index pi ON pi.indrelid=att.attrelid AND indisprimary | ||
| LEFT OUTER JOIN pg_catalog.pg_description des ON (des.objoid=att.attrelid AND des.objsubid=att.attnum AND des.classoid='pg_class'::regclass) | ||
| LEFT OUTER JOIN pg_catalog.pg_sequence seq ON cs.oid=seq.seqrelid | ||
| WHERE | ||
|
|
||
| {% if tid %} | ||
| att.attrelid = {{ tid|qtLiteral(conn) }}::oid | ||
| {% endif %} | ||
| {% if table_name and table_nspname %} | ||
| cl.relname= {{table_name |qtLiteral(conn)}} and na.nspname={{table_nspname|qtLiteral(conn)}} | ||
| {% endif %} | ||
| {% if clid %} | ||
| AND att.attnum = {{ clid|qtLiteral(conn) }} | ||
| {% endif %} | ||
| {### To show system objects ###} | ||
| {% if not show_sys_objects and not has_oids %} | ||
| AND att.attnum > 0 | ||
| {% endif %} | ||
| {### To show oids in view data ###} | ||
| {% if has_oids %} | ||
| AND (att.attnum > 0 OR (att.attname = 'oid' AND att.attnum < 0)) | ||
| {% endif %} | ||
| AND att.attisdropped IS FALSE | ||
| ORDER BY att.attnum |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,6 +118,12 @@ def save_changed_data(changed_data, columns_info, conn, command_obj, | |
| if command_obj.has_oids(): | ||
| data.pop('oid', None) | ||
|
|
||
| # Remove generated columns (GENERATED ALWAYS AS) as they | ||
| # cannot be inserted - PostgreSQL auto-computes their values. | ||
| for col_name, col_info in columns_info.items(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this will run for each row? why not simply skip from update sql generation - it is already looping through the columns. Can avoid this loop inside loop. |
||
| if col_info.get('is_generated', False): | ||
| data.pop(col_name, None) | ||
|
|
||
| # Update columns value with columns having | ||
| # not_null=False and has no default value | ||
| column_data.update(data) | ||
|
|
@@ -163,14 +169,38 @@ def save_changed_data(changed_data, columns_info, conn, command_obj, | |
| # For updated rows | ||
| elif of_type == 'updated': | ||
| list_of_sql[of_type] = [] | ||
|
|
||
| # 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() | ||
|
|
||
|
Comment on lines
+173
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 Line 347 then unconditionally accesses 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 |
||
| for each_row in changed_data[of_type]: | ||
| data = changed_data[of_type][each_row]['data'] | ||
| row_primary_keys = changed_data[of_type][each_row][ | ||
| 'primary_keys'] | ||
|
|
||
| # Remove generated columns (GENERATED ALWAYS AS) as they | ||
| # cannot be updated - PostgreSQL auto-computes their values. | ||
| for col_name, col_info in columns_info.items(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above. |
||
| if col_info.get('is_generated', False): | ||
| data.pop(col_name, None) | ||
|
|
||
| pk_escaped = { | ||
| pk: pk_val.replace('%', '%%') if hasattr( | ||
| pk_val, 'replace') else pk_val | ||
| for pk, pk_val in | ||
| changed_data[of_type][each_row]['primary_keys'].items() | ||
| for pk, pk_val in row_primary_keys.items() | ||
| } | ||
|
|
||
| # Pass pk_names and has_oids for RETURNING clause in | ||
| # UPDATE statement. | ||
| # This will help to fetch the updated row's. | ||
| sql = render_template( | ||
| "/".join([command_obj.sql_path, 'update.sql']), | ||
| data_to_be_saved=data, | ||
|
|
@@ -180,12 +210,35 @@ def save_changed_data(changed_data, columns_info, conn, command_obj, | |
| nsp_name=command_obj.nsp_name, | ||
| data_type=column_type, | ||
| type_cast_required=type_cast_required, | ||
| pk_names=pk_names if has_generated_cols else None, | ||
| has_oids=command_obj.has_oids(), | ||
| conn=conn | ||
| ) | ||
| list_of_sql[of_type].append({'sql': sql, | ||
| 'data': data, | ||
| 'row_id': | ||
| data.get(client_primary_key)}) | ||
|
|
||
| # For tables with generated columns, add select_sql to | ||
| # refetch updated row. | ||
| if has_generated_cols: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. like I mentioned above - check if we can use the returning clause to return all instead of selecting the row again. |
||
| select_sql = render_template( | ||
| "/".join([command_obj.sql_path, 'select.sql']), | ||
| object_name=command_obj.object_name, | ||
| nsp_name=command_obj.nsp_name, | ||
| pgadmin_alias=pgadmin_alias, | ||
| primary_keys=primary_keys, | ||
| has_oids=command_obj.has_oids() | ||
| ) | ||
| list_of_sql[of_type].append({ | ||
| 'sql': sql, | ||
| 'data': data, | ||
| 'client_row': each_row, | ||
| 'select_sql': select_sql, | ||
| 'row_id': data.get(client_primary_key) | ||
| }) | ||
| else: | ||
| list_of_sql[of_type].append({ | ||
| 'sql': sql, | ||
| 'data': data, | ||
| 'row_id': data.get(client_primary_key) | ||
| }) | ||
|
|
||
| # For deleted rows | ||
| elif of_type == 'deleted': | ||
|
|
@@ -287,7 +340,7 @@ def failure_handle(res, row_id): | |
| if not status: | ||
| return failure_handle(res, item.get('row_id', 0)) | ||
|
|
||
| # Select added row from the table | ||
| # Select added/updated row from the table | ||
| if 'select_sql' in item: | ||
| params = { | ||
| pgadmin_alias[k] if k in pgadmin_alias else k: v | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not return the complete row instead of just oid? That way - no need to reselect.