mirror of
https://github.com/pgadmin-org/pgadmin4.git
synced 2026-08-17 16:34:44 -05:00
fix: use pg_depend ownership instead of name guessing for SERIAL detection (#10167)
Column-owns-sequence was detected by guessing the sequence name as <table>_<col>_seq and string-matching it in nextval(...). Renaming a table/column broke the guess (false negative, #10100); an unrelated sequence with the same guessed name broke it the other way (false positive, #10101). Check col.get('seqrelid') (real pg_depend ownership) instead, and exclude identity columns. SERIAL is now emitted iff a genuine ownership dependency exists, independent of naming. Fixes #10100, #10101
This commit is contained in:
Kundan
+109
@@ -0,0 +1,109 @@
|
||||
##########################################################################
|
||||
#
|
||||
# pgAdmin 4 - PostgreSQL Tools
|
||||
#
|
||||
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
|
||||
# This software is released under the PostgreSQL Licence
|
||||
#
|
||||
##########################################################################
|
||||
|
||||
"""Unit tests for get_formatted_columns()'s SERIAL-column detection using
|
||||
mocks.
|
||||
|
||||
These verify the ownership-vs-default-sequence comparison (#10100,
|
||||
#10101) without requiring a running PostgreSQL server.
|
||||
"""
|
||||
|
||||
from unittest.mock import MagicMock, patch
|
||||
from pgadmin.utils.route import BaseTestGenerator
|
||||
|
||||
UTILS_MODULE = ('pgadmin.browser.server_groups.servers.databases.schemas.'
|
||||
'tables.columns.utils')
|
||||
|
||||
|
||||
def _make_column(**overrides):
|
||||
"""A properties.sql result row, as returned for a single column."""
|
||||
defaults = dict(
|
||||
name='id', atttypid=23, attlen=4, attnum=1, attndims=0,
|
||||
atttypmod=-1, attacl=None, attnotnull=False, attoptions=None,
|
||||
attfdwoptions=None, attstattarget=-1, attstorage='p',
|
||||
attidentity='', defval=None, typname='integer',
|
||||
displaytypname='integer', cltype='integer',
|
||||
inheritedfrom=None, inheritedid=None, elemoid=23,
|
||||
typnspname='pg_catalog',
|
||||
defaultstorage='p', description=None, indkey=None, isdup=False,
|
||||
collspcname='', is_fk=False, seclabels=None, is_sys_column=False,
|
||||
colconstype='n', genexpr=None, relname='t', is_view_only=False,
|
||||
attcompression=None, seqrelid=None, defseqrelid=None,
|
||||
)
|
||||
defaults.update(overrides)
|
||||
return defaults
|
||||
|
||||
|
||||
class TestSerialColumnDetection(BaseTestGenerator):
|
||||
"""Unit tests for ServerModule.get_formatted_columns() SERIAL
|
||||
detection using mock rows."""
|
||||
|
||||
scenarios = [
|
||||
('Split ownership/default keeps explicit nextval default',
|
||||
dict(test_method='test_split_ownership_default_not_serial')),
|
||||
('Genuine SERIAL column is reprojected',
|
||||
dict(test_method='test_genuine_serial_is_detected')),
|
||||
('Identity column is never reprojected',
|
||||
dict(test_method='test_identity_column_not_serial')),
|
||||
]
|
||||
|
||||
@patch(UTILS_MODULE + '.render_template', return_value='SELECT 1;')
|
||||
def runTest(self, mock_render):
|
||||
getattr(self, self.test_method)()
|
||||
|
||||
def _run(self, column_row):
|
||||
from pgadmin.browser.server_groups.servers.databases.schemas.\
|
||||
tables.columns.utils import get_formatted_columns
|
||||
|
||||
conn = MagicMock()
|
||||
conn.execute_dict.return_value = (True, {'rows': [column_row]})
|
||||
conn.execute_2darray.return_value = (True, {'rows': []})
|
||||
|
||||
with patch(UTILS_MODULE + '.column_formatter'):
|
||||
data = get_formatted_columns(
|
||||
conn, tid=1, data={}, other_columns=[],
|
||||
table_or_type='table', template_path='columns/sql/default')
|
||||
|
||||
return data['columns'][0]
|
||||
|
||||
def test_split_ownership_default_not_serial(self):
|
||||
# Column owns sequence 100 (pg_depend deptype='a'), but its
|
||||
# DEFAULT's nextval() call references a different sequence, 200
|
||||
# (pg_depend deptype='n' from the pg_attrdef entry). SERIAL must
|
||||
# not be applied - ownership and default disagree.
|
||||
col = _make_column(
|
||||
defval="nextval('seq_used'::regclass)",
|
||||
seqrelid=100, defseqrelid=200,
|
||||
)
|
||||
result = self._run(col)
|
||||
self.assertEqual(result['typname'], 'integer')
|
||||
self.assertEqual(result['cltype'], 'integer')
|
||||
self.assertEqual(result['defval'], "nextval('seq_used'::regclass)")
|
||||
|
||||
def test_genuine_serial_is_detected(self):
|
||||
# Ownership and default both point at the same sequence (100) -
|
||||
# this is what a real SERIAL column looks like.
|
||||
col = _make_column(
|
||||
defval="nextval('t_id_seq'::regclass)",
|
||||
seqrelid=100, defseqrelid=100,
|
||||
)
|
||||
result = self._run(col)
|
||||
self.assertEqual(result['typname'], 'serial')
|
||||
self.assertEqual(result['cltype'], 'serial')
|
||||
self.assertEqual(result['defval'], '')
|
||||
|
||||
def test_identity_column_not_serial(self):
|
||||
# Identity columns can carry an internal sequence dependency on
|
||||
# both sides, but must never be reprojected as SERIAL.
|
||||
col = _make_column(
|
||||
defval=None, seqrelid=100, defseqrelid=100, attidentity='a',
|
||||
)
|
||||
result = self._run(col)
|
||||
self.assertEqual(result['typname'], 'integer')
|
||||
self.assertEqual(result['defval'], None)
|
||||
+37
-17
@@ -17,8 +17,6 @@ from pgadmin.browser.server_groups.servers.databases.schemas.utils \
|
||||
import DataTypeReader
|
||||
from pgadmin.browser.server_groups.servers.utils import parse_priv_from_db, \
|
||||
parse_priv_to_db
|
||||
from pgadmin.browser.server_groups.servers.databases.utils \
|
||||
import make_object_name
|
||||
from functools import wraps
|
||||
import re
|
||||
|
||||
@@ -234,14 +232,18 @@ def get_formatted_columns(conn, tid, data, other_columns,
|
||||
This function will iterate and return formatted data for all
|
||||
the columns.
|
||||
|
||||
Serial-column detection is on by default: a column whose default
|
||||
is ``nextval('<table>_<col>_seq'...)`` with an integer/smallint/
|
||||
bigint type is the reverse-engineered form of a SERIAL declaration,
|
||||
and we reproject it back to ``serial`` / ``smallserial`` /
|
||||
``bigserial`` so callers can emit valid round-trippable DDL. Pass
|
||||
``with_serial=False`` only if you genuinely need the raw libpq
|
||||
representation (e.g. a low-level introspection caller that handles
|
||||
the sequence itself). Issue #9896.
|
||||
Serial-column detection is on by default: a column that owns the
|
||||
sequence referenced by its ``nextval(...)`` default (with an
|
||||
integer/smallint/bigint type) is the reverse-engineered form of a
|
||||
SERIAL declaration, and we reproject it back to ``serial`` /
|
||||
``smallserial`` / ``bigserial`` so callers can emit valid
|
||||
round-trippable DDL. Ownership is determined from ``pg_depend`` (the
|
||||
``seqrelid`` exposed by properties.sql), not by guessing the
|
||||
sequence name, so it stays correct across renames and never rewrites
|
||||
an unrelated column. Pass ``with_serial=False`` only if you genuinely
|
||||
need the raw libpq representation (e.g. a low-level introspection
|
||||
caller that handles the sequence itself). Issues #9896, #10100,
|
||||
#10101.
|
||||
|
||||
:param conn: Connection Object
|
||||
:param tid: Table ID
|
||||
@@ -269,14 +271,32 @@ def get_formatted_columns(conn, tid, data, other_columns,
|
||||
other_col['inheritedfrom']
|
||||
|
||||
if with_serial:
|
||||
# Here we assume if a column is serial
|
||||
serial_seq_name = make_object_name(
|
||||
data['name'], col['name'], 'seq')
|
||||
# replace the escaped quotes for comparison
|
||||
defval = (col.get('defval', '') or '').replace("''", "'").\
|
||||
replace('""', '"')
|
||||
# A column is SERIAL only when it genuinely owns the sequence
|
||||
# referenced by its DEFAULT. properties.sql LEFT JOINs
|
||||
# pg_depend (a sequence's pg_class depending on this column's
|
||||
# pg_attribute) and surfaces the owned sequence oid as
|
||||
# ``seqrelid`` - that dependency is the authoritative
|
||||
# ownership signal. Relying on it (instead of guessing the
|
||||
# ``<table>_<col>_seq`` name) keeps detection correct after a
|
||||
# table/column/sequence rename and never rewrites an
|
||||
# unrelated column whose default happens to match the guessed
|
||||
# name (#10100, #10101). Identity columns carry an internal
|
||||
# dependency too, but never a nextval() default, and are
|
||||
# additionally excluded via attidentity.
|
||||
#
|
||||
# Ownership and default are independent dependencies in
|
||||
# PostgreSQL: a column can own one sequence (pg_depend
|
||||
# deptype='a', -> ``seqrelid``) while its DEFAULT's nextval()
|
||||
# call names a completely different one (pg_depend deptype='n'
|
||||
# from the pg_attrdef entry, -> ``defseqrelid``). SERIAL only
|
||||
# applies when both dependencies point at the same sequence, so
|
||||
# this split-ownership/default case keeps its original,
|
||||
# explicit nextval() default instead of being rewritten.
|
||||
defval = col.get('defval', '') or ''
|
||||
|
||||
if serial_seq_name in defval and defval.startswith("nextval('")\
|
||||
if col.get('seqrelid') and defval.startswith("nextval('") \
|
||||
and not col.get('attidentity') \
|
||||
and col.get('seqrelid') == col.get('defseqrelid') \
|
||||
and col['typname'] in ('integer', 'smallint', 'bigint'):
|
||||
|
||||
serial_type = {
|
||||
|
||||
+8
@@ -37,6 +37,14 @@ SELECT DISTINCT ON (att.attnum) att.attname as name, att.atttypid, att.attlen, a
|
||||
(CASE WHEN (att.attgenerated in ('s')) THEN pg_catalog.pg_get_expr(def.adbin, def.adrelid) END) AS genexpr, tab.relname as relname,
|
||||
(CASE WHEN tab.relkind = 'v' THEN true ELSE false END) AS is_view_only,
|
||||
(CASE WHEN att.attcompression = 'p' THEN 'pglz' WHEN att.attcompression = 'l' THEN 'lz4' END) AS attcompression,
|
||||
(SELECT dsd.refobjid
|
||||
FROM pg_catalog.pg_depend dsd
|
||||
JOIN pg_catalog.pg_class dsc ON dsd.refclassid='pg_catalog.pg_class'::regclass
|
||||
AND dsd.refobjid=dsc.oid AND dsc.relkind='S'
|
||||
WHERE dsd.classid='pg_catalog.pg_attrdef'::regclass
|
||||
AND dsd.objid=def.oid AND dsd.deptype='n'
|
||||
ORDER BY dsd.refobjid
|
||||
LIMIT 1) AS defseqrelid,
|
||||
seq.*
|
||||
FROM pg_catalog.pg_attribute att
|
||||
JOIN pg_catalog.pg_type ty ON ty.oid=atttypid
|
||||
|
||||
Reference in New Issue
Block a user