Allow users to customize ofType columns during table creation #223 #9580
Conversation
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
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
mzabuawala
force-pushed
the
fixes_ofType_223
branch
2 times, most recently
from
c1e7467 to
d63a40f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js`:
- Around line 260-266: The map callback that builds inc_ids leaves undefined
entries when the if condition is false; update the logic around keys.map(...)
(the block assigning inc_ids) to only produce numeric ids for columns present in
currPk.include — for example replace the single map with a filter+map or a
reduce so you first filter keys where
currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1 and then
parseInt(k), or keep the map but return null/undefined and immediately follow
with .filter(Boolean) to remove empties; ensure the final inc_ids contains only
parsed integers.
- Around line 244-247: The map callback for keys.map only returns when the if
condition is true, producing undefined entries; change the logic to produce a
compact array of ints by using filter+map or flatMap: e.g., filter keys where
columns.indexOf(actionObj.oldState.columns[k].name) > -1 then map to
parseInt(k), or use keys.flatMap(k =>
columns.indexOf(actionObj.oldState.columns[k].name) > -1 ? [parseInt(k)] : []).
Update the expression that assigns ids (the keys.map usage and variable ids)
accordingly so every execution path returns a defined value.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/16_plus/create.sql`:
- Line 57: The template can produce invalid SQL if a column is marked WITH
OPTIONS while also being an identity column, so update the backend logic that
computes has_with_options to explicitly skip setting it for identity
constraints: when determining has_with_options for OF TYPE columns (the code
path that currently sets has_with_options based on DEFAULT, NOT NULL, or PRIMARY
KEY changes), add a guard that if colconstype == 'i' (identity) do not set
has_with_options; ensure the same check is applied wherever has_with_options is
derived or mutated so templates using has_with_options can never see true for
identity columns.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql`:
- Line 49: The template has a malformed conditional around rendering the column
type: wrap the direct type rendering ({{c.displaytypname}}) with an {% if is_sql
%} ... {% else %} ... {% endif %} so the existing {% else %} and {% endif %}
pair up correctly; specifically, after rendering the column name
({{conn|qtIdent(c.name)}}) insert {% if is_sql %}{{c.displaytypname}}{% else %}
and keep the existing GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen,
c.attprecision, c.hasSqrBracket) branch as the else, ensuring the final {% endif
%} closes this new conditional.
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py (1)
367-375: Unusedpk_columnsset is built but never referenced.The
pk_columnsset is populated fromdata['primary_key'], but it's never used in the subsequent logic. The only reference is in a commented-out block (lines 389-391). Consider either:
- Removing this code if it's not needed
- Uncommenting the usage if primary key detection should trigger
has_with_options♻️ Option 1: Remove unused code
- # Get list of columns in primary key constraint - pk_columns = set() - if 'primary_key' in data and len(data['primary_key']) > 0: - for pk in data['primary_key']: - if 'columns' in pk: - for col in pk['columns']: - if 'column' in col: - pk_columns.add(col['column']) - for c in columns:
Comment on lines
244
to
247
| ids = keys.map((k) => { | ||
| if (columns.indexOf(actionObj.oldState.columns[k].name) > -1) | ||
| return parseInt(k); | ||
| }); |
There was a problem hiding this comment.
Map callback should return a value on all execution paths.
This map() callback only returns a value when the condition is true, resulting in undefined values in the array when the condition is false. This is flagged by static analysis as it's semantically incorrect for map().
Consider using filter() with map() or flatMap() for clearer intent:
🔧 Proposed fix
- ids = keys.map((k) => {
- if (columns.indexOf(actionObj.oldState.columns[k].name) > -1)
- return parseInt(k);
- });
+ ids = keys
+ .filter((k) => columns.indexOf(actionObj.oldState.columns[k].name) > -1)
+ .map((k) => parseInt(k));🧰 Tools
🪛 Biome (2.3.13)
[error] 244-244: This callback passed to map() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js`
around lines 244 - 247, The map callback for keys.map only returns when the if
condition is true, producing undefined entries; change the logic to produce a
compact array of ints by using filter+map or flatMap: e.g., filter keys where
columns.indexOf(actionObj.oldState.columns[k].name) > -1 then map to
parseInt(k), or use keys.flatMap(k =>
columns.indexOf(actionObj.oldState.columns[k].name) > -1 ? [parseInt(k)] : []).
Update the expression that assigns ids (the keys.map usage and variable ids)
accordingly so every execution path returns a defined value.
Comment on lines
260
to
266
| let inc_ids = keys.map((k) => { | ||
| if ( | ||
| currPk.include.indexOf(actionObj.oldState.columns[k].name) > | ||
| -1 | ||
| ) | ||
| return parseInt(k); | ||
| }); |
There was a problem hiding this comment.
Map callback should return a value on all execution paths.
Same issue as above - this map() callback doesn't return a value when the condition is false, producing undefined entries in the resulting array.
🔧 Proposed fix
- let inc_ids = keys.map((k) => {
- if (
- currPk.include.indexOf(actionObj.oldState.columns[k].name) >
- -1
- )
- return parseInt(k);
- });
+ let inc_ids = keys
+ .filter((k) => currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1)
+ .map((k) => parseInt(k));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let inc_ids = keys.map((k) => { | |
| if ( | |
| currPk.include.indexOf(actionObj.oldState.columns[k].name) > | |
| -1 | |
| ) | |
| return parseInt(k); | |
| }); | |
| let inc_ids = keys | |
| .filter((k) => currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1) | |
| .map((k) => parseInt(k)); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 260-260: This callback passed to map() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js`
around lines 260 - 266, The map callback that builds inc_ids leaves undefined
entries when the if condition is false; update the logic around keys.map(...)
(the block assigning inc_ids) to only produce numeric ids for columns present in
currPk.include — for example replace the single map with a filter+map or a
reduce so you first filter keys where
currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1 and then
parseInt(k), or keep the map but return null/undefined and immediately follow
with .filter(Boolean) to remove empties; ensure the final inc_ids contains only
parsed integers.
...owser/server_groups/servers/databases/schemas/tables/templates/tables/sql/16_plus/create.sql
Show resolved
Hide resolved
| {% for c in data.columns %} | ||
| {% if c.name and c.cltype %} | ||
| {% if c.inheritedfromtable %}-- Inherited from table {{c.inheritedfromtable}}: {% elif c.inheritedfromtype %}-- Inherited from type {{c.inheritedfromtype}}: {% endif %}{{conn|qtIdent(c.name)}} {% if is_sql %}{{c.displaytypname}}{% else %}{{ GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen, c.attprecision, c.hasSqrBracket) }}{% endif %}{% if c.geometry and not is_sql %}({{c.geometry}}{% if c.srid %},{{c.srid}}{% endif %}){% endif %}{% if c.collspcname %} COLLATE {{c.collspcname}}{% endif %}{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %} | ||
| {% if c.inheritedfromtype and c.has_with_options %}{# Use WITH OPTIONS syntax for modified OF TYPE columns #}{{conn|qtIdent(c.name)}} WITH OPTIONS{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %}{% elif c.inheritedfromtable %}{# Inherited from parent table - keep as comment #}-- Inherited from table {{c.inheritedfromtable}}: {{conn|qtIdent(c.name)}}{% else %}{# Regular column or inherited without modifications #}{% if c.inheritedfromtype %}-- Inherited from type {{c.inheritedfromtype}}: {% endif %}{{conn|qtIdent(c.name)}}{{c.displaytypname}}{% else %}{{ GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen, c.attprecision, c.hasSqrBracket) }}{% endif %}{% if c.geometry and not is_sql %}({{c.geometry}}{% if c.srid %},{{c.srid}}{% endif %}){% endif %}{% if c.collspcname %} COLLATE {{c.collspcname}}{% endif %}{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %}{% endif %} |
There was a problem hiding this comment.
Template syntax error: Missing {% if is_sql %} conditional around type rendering.
The template has a malformed conditional structure. Comparing to the other templates (11_plus, 12_plus, 14_plus), there's a missing {% if is_sql %} check before {{c.displaytypname}}.
Current (broken):
{{conn|qtIdent(c.name)}}{{c.displaytypname}}{% else %}{{ GET_TYPE.CREATE_TYPE_SQL(...) }}{% endif %}The {% else %} has no corresponding {% if %} in scope, which will cause a Jinja2 syntax error.
Expected (from other templates):
{{conn|qtIdent(c.name)}} {% if is_sql %}{{c.displaytypname}}{% else %}{{ GET_TYPE.CREATE_TYPE_SQL(...) }}{% endif %}🐛 Proposed fix
- {% if c.inheritedfromtype and c.has_with_options %}{# Use WITH OPTIONS syntax for modified OF TYPE columns #}{{conn|qtIdent(c.name)}} WITH OPTIONS{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %}{% elif c.inheritedfromtable %}{# Inherited from parent table - keep as comment #}-- Inherited from table {{c.inheritedfromtable}}: {{conn|qtIdent(c.name)}}{% else %}{# Regular column or inherited without modifications #}{% if c.inheritedfromtype %}-- Inherited from type {{c.inheritedfromtype}}: {% endif %}{{conn|qtIdent(c.name)}}{{c.displaytypname}}{% else %}{{ GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen, c.attprecision, c.hasSqrBracket) }}{% endif %}{% if c.geometry and not is_sql %}({{c.geometry}}{% if c.srid %},{{c.srid}}{% endif %}){% endif %}{% if c.collspcname %} COLLATE {{c.collspcname}}{% endif %}{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %}{% endif %}
+ {% if c.inheritedfromtype and c.has_with_options %}{# Use WITH OPTIONS syntax for modified OF TYPE columns #}{{conn|qtIdent(c.name)}} WITH OPTIONS{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %}{% elif c.inheritedfromtable %}{# Inherited from parent table - keep as comment #}-- Inherited from table {{c.inheritedfromtable}}: {{conn|qtIdent(c.name)}}{% else %}{# Regular column or inherited without modifications #}{% if c.inheritedfromtype %}-- Inherited from type {{c.inheritedfromtype}}: {% endif %}{{conn|qtIdent(c.name)}} {% if is_sql %}{{c.displaytypname}}{% else %}{{ GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen, c.attprecision, c.hasSqrBracket) }}{% endif %}{% if c.geometry and not is_sql %}({{c.geometry}}{% if c.srid %},{{c.srid}}{% endif %}){% endif %}{% if c.collspcname %} COLLATE {{c.collspcname}}{% endif %}{% if c.attnotnull %} NOT NULL{% endif %}{% if c.defval is defined and c.defval is not none and c.defval != '' %} DEFAULT {{c.defval}}{% endif %}{% endif %}🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql`
at line 49, The template has a malformed conditional around rendering the column
type: wrap the direct type rendering ({{c.displaytypname}}) with an {% if is_sql
%} ... {% else %} ... {% endif %} so the existing {% else %} and {% endif %}
pair up correctly; specifically, after rendering the column name
({{conn|qtIdent(c.name)}}) insert {% if is_sql %}{{c.displaytypname}}{% else %}
and keep the existing GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen,
c.attprecision, c.hasSqrBracket) branch as the else, ensuring the final {% endif
%} closes this new conditional.
✅ Addressed in commit be63f61
mzabuawala
force-pushed
the
fixes_ofType_223
branch
3 times, most recently
from
a3104fd to
1aba96d
Compare
mzabuawala
force-pushed
the
fixes_ofType_223
branch
from
1aba96d to
4f03620
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js`:
- Around line 163-165: The edit/readonly logic currently treats any
inheritedfrom as table inheritance; update those gates to distinguish type
inheritance by using isInheritedFromType(state) so only true table inheritance
makes fields read-only. Specifically, replace direct checks of
state.inheritedfrom in the edit/readonly guards (and the
length/precision/default validation paths) with a check that inheritedfrom is
present AND isInheritedFromType(state) is false (e.g., only treat as
table-inherited when !isInheritedFromType(state) &&
!isEmptyString(state.inheritedfrom)). Also ensure get_oftype still preserves
compatibility but that any logic deciding editability consults
isInheritedFromType(state) to allow OF TYPE columns to be editable.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`:
- Around line 386-423: Remove the redundant is_primary_key-driven modification
flag: delete the check that sets has_modifications when c.get('is_primary_key')
is true so primary keys are not flagged as column-level changes; rely only on
actual DEFAULT/NOT NULL differences (original_defval/defval and
original_attnotnull/attnotnull) to set has_modifications, and keep the existing
identity check and has_with_options assignment for final_columns as-is (refer to
variables/keys c, is_primary_key, has_modifications, original_defval, defval,
original_attnotnull, attnotnull, has_with_options, final_columns).
Comment on lines
+163
to
+165
| isInheritedFromType(state) { | ||
| return !isEmptyString(state.inheritedfromtype); | ||
| } |
There was a problem hiding this comment.
Wire isInheritedFromType into inherited-column edit checks.
Right now the edit/readonly gates still key off inheritedfrom, and get_oftype keeps that field for backward compatibility. That means OF TYPE columns can remain read‑only, undermining the goal of making them editable. Please distinguish table inheritance from type inheritance in the checks (and in the direct inheritedfrom checks for length/precision/default).
🔧 Suggested fix
isInheritedFromType(state) {
return !isEmptyString(state.inheritedfromtype);
}
+
+ isInheritedFromTable(state) {
+ return !isEmptyString(state.inheritedfrom) && !this.isInheritedFromType(state);
+ }
...
- if (!isEmptyString(state.inheritedfrom)){
+ if (this.isInheritedFromTable(state)){
return true;
}
...
- if (!isEmptyString(state.inheritedfrom)) {
+ if (obj.isInheritedFromTable(state)) {
return false;
}
...
- if (!isEmptyString(state.inheritedfrom)) {
+ if (obj.isInheritedFromTable(state)) {
return false;
}
...
- return !(!isEmptyString(state.inheritedfrom) || !this.editableCheckForTable(state));
+ return !(obj.isInheritedFromTable(state) || !this.editableCheckForTable(state));🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js`
around lines 163 - 165, The edit/readonly logic currently treats any
inheritedfrom as table inheritance; update those gates to distinguish type
inheritance by using isInheritedFromType(state) so only true table inheritance
makes fields read-only. Specifically, replace direct checks of
state.inheritedfrom in the edit/readonly guards (and the
length/precision/default validation paths) with a check that inheritedfrom is
present AND isInheritedFromType(state) is false (e.g., only treat as
table-inherited when !isInheritedFromType(state) &&
!isEmptyString(state.inheritedfrom)). Also ensure get_oftype still preserves
compatibility but that any logic deciding editability consults
isInheritedFromType(state) to allow OF TYPE columns to be editable.
Comment on lines
+386
to
+423
| # Check if column is in PRIMARY KEY constraint | ||
| # Note: We don't include the column for PRIMARY KEY because | ||
| # it's added as a separate table-level constraint | ||
| # Uncomment this if you want column-level PRIMARY KEY: | ||
| # if c.get('name') in pk_columns: | ||
| # has_modifications = True | ||
|
|
||
| # Check for PRIMARY KEY (marked via is_primary_key checkbox) | ||
| if c.get('is_primary_key'): | ||
| has_modifications = True | ||
|
|
||
| # Check if DEFAULT value was actually modified | ||
| # (different from type) | ||
| original_defval = c.get('original_defval') | ||
| current_defval = c.get('defval') | ||
| # Compare as strings, treating None and empty string | ||
| # as equivalent | ||
| orig_val = str(original_defval) \ | ||
| if original_defval is not None else '' | ||
| curr_val = str(current_defval) \ | ||
| if current_defval is not None else '' | ||
| if orig_val != curr_val: | ||
| has_modifications = True | ||
|
|
||
| # Check if NOT NULL was actually modified | ||
| # (different from type) | ||
| original_attnotnull = c.get('original_attnotnull', False) | ||
| current_attnotnull = c.get('attnotnull', False) | ||
| if original_attnotnull != current_attnotnull: | ||
| has_modifications = True | ||
|
|
||
| if has_modifications: | ||
| # Mark this column to use WITH OPTIONS syntax in template | ||
| # Skip identity columns as WITH OPTIONS | ||
| # cannot be combined with GENERATED ALWAYS AS IDENTITY | ||
| if c.get('colconstype') != 'i': | ||
| c['has_with_options'] = True | ||
| final_columns.append(c) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate the template that uses has_with_options
find web -name "*.sql" -o -name "*.j2" -o -name "*.jinja*" | head -20Repository: pgadmin-org/pgadmin4
Length of output: 2188
🏁 Script executed:
# Search for has_with_options usage in the codebase
rg "has_with_options" --type py --type sql -B 2 -A 2Repository: pgadmin-org/pgadmin4
Length of output: 11388
🏁 Script executed:
# Get more context around the code in question - show the full function
cd web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns && \
wc -l utils.py && \
head -n 430 utils.py | tail -n 100Repository: pgadmin-org/pgadmin4
Length of output: 4144
🌐 Web query:
PostgreSQL ALTER TABLE column WITH OPTIONS syntax requirements - is at least one option required
💡 Result:
Yes. In PostgreSQL the ALTER TABLE ... ALTER COLUMN ... SET ( ... ) form requires at least one option inside the parentheses—the grammar is a comma-separated list, not an optional/empty group:
ALTER [ COLUMN ] column_name SET ( attribute_option = value [, ... ] )ALTER [ COLUMN ] column_name RESET ( attribute_option [, ... ] )(postgresql.org)
So SET () (or RESET ()) is invalid syntax. If you have nothing to set, omit that subcommand; if you want to remove options, use RESET and name one or more options (e.g. RESET (n_distinct)). (postgresql.org)
Citations:
- 1: https://www.postgresql.org/docs/current/sql-altertable.html
- 2: https://www.postgresql.org/docs/current/sql-altertable.html
Remove the is_primary_key check to avoid generating invalid WITH OPTIONS syntax.
The code marks columns for WITH OPTIONS when only is_primary_key is true, but the template outputs WITH OPTIONS with no options if DEFAULT and NOT NULL haven't changed. PostgreSQL requires at least one option in WITH OPTIONS (e.g. NOT NULL or DEFAULT), so this generates invalid SQL. Additionally, the comment already notes that PRIMARY KEY is handled as a table-level constraint, making this check redundant.
Suggested change
- # Check for PRIMARY KEY (marked via is_primary_key checkbox)
- if c.get('is_primary_key'):
- has_modifications = True🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`
around lines 386 - 423, Remove the redundant is_primary_key-driven modification
flag: delete the check that sets has_modifications when c.get('is_primary_key')
is true so primary keys are not flagged as column-level changes; rely only on
actual DEFAULT/NOT NULL differences (original_defval/defval and
original_attnotnull/attnotnull) to set has_modifications, and keep the existing
identity check and has_with_options assignment for final_columns as-is (refer to
variables/keys c, is_primary_key, has_modifications, original_defval, defval,
original_attnotnull, attnotnull, has_with_options, final_columns).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`:
- Around line 367-423: Remove the check that sets has_modifications when
c.get('is_primary_key') is true inside the columns loop so that PRIMARY KEY does
not cause has_with_options to be set; instead only detect modifications via
differences in defval (original_defval vs defval) and attnotnull
(original_attnotnull vs attnotnull) as currently implemented, and continue to
skip identity columns (colconstype == 'i') before marking c['has_with_options']
= True; locate this logic around the loop over columns and the variables
has_modifications, c.get('is_primary_key'), original_defval/current_defval, and
original_attnotnull/current_attnotnull.
Comment on lines
+367
to
+423
| # Get list of columns in primary key constraint | ||
| pk_columns = set() | ||
| if 'primary_key' in data and len(data['primary_key']) > 0: | ||
| for pk in data['primary_key']: | ||
| if 'columns' in pk: | ||
| for col in pk['columns']: | ||
| if 'column' in col: | ||
| pk_columns.add(col['column']) | ||
|
|
||
| for c in columns: | ||
| # Include non-inherited columns | ||
| if c.get('inheritedfrom', None) is None: | ||
| final_columns.append(c) | ||
| # Also include columns inherited from composite types (OF TYPE) | ||
| # that have modifications (WITH OPTIONS clause) | ||
| elif c.get('inheritedfromtype', None) is not None: | ||
| # Check if column has been modified or is in a constraint | ||
| has_modifications = False | ||
|
|
||
| # Check if column is in PRIMARY KEY constraint | ||
| # Note: We don't include the column for PRIMARY KEY because | ||
| # it's added as a separate table-level constraint | ||
| # Uncomment this if you want column-level PRIMARY KEY: | ||
| # if c.get('name') in pk_columns: | ||
| # has_modifications = True | ||
|
|
||
| # Check for PRIMARY KEY (marked via is_primary_key checkbox) | ||
| if c.get('is_primary_key'): | ||
| has_modifications = True | ||
|
|
||
| # Check if DEFAULT value was actually modified | ||
| # (different from type) | ||
| original_defval = c.get('original_defval') | ||
| current_defval = c.get('defval') | ||
| # Compare as strings, treating None and empty string | ||
| # as equivalent | ||
| orig_val = str(original_defval) \ | ||
| if original_defval is not None else '' | ||
| curr_val = str(current_defval) \ | ||
| if current_defval is not None else '' | ||
| if orig_val != curr_val: | ||
| has_modifications = True | ||
|
|
||
| # Check if NOT NULL was actually modified | ||
| # (different from type) | ||
| original_attnotnull = c.get('original_attnotnull', False) | ||
| current_attnotnull = c.get('attnotnull', False) | ||
| if original_attnotnull != current_attnotnull: | ||
| has_modifications = True | ||
|
|
||
| if has_modifications: | ||
| # Mark this column to use WITH OPTIONS syntax in template | ||
| # Skip identity columns as WITH OPTIONS | ||
| # cannot be combined with GENERATED ALWAYS AS IDENTITY | ||
| if c.get('colconstype') != 'i': | ||
| c['has_with_options'] = True | ||
| final_columns.append(c) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the complete function to understand the full context
wc -l web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.pyRepository: pgadmin-org/pgadmin4
Length of output: 151
🏁 Script executed:
# Read the function around lines 367-423 with more context
sed -n '340,450p' web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.pyRepository: pgadmin-org/pgadmin4
Length of output: 4313
🏁 Script executed:
# Search for how has_with_options is used in the codebase
rg 'has_with_options' --type py -B 2 -A 2Repository: pgadmin-org/pgadmin4
Length of output: 708
🏁 Script executed:
# Search for WITH OPTIONS in template files to understand SQL generation
rg 'WITH OPTIONS' --type-listRepository: pgadmin-org/pgadmin4
Length of output: 453
🏁 Script executed:
# Also search for any SQL templates that use this flag
find . -name '*.sql' -o -name '*.jinja2' | head -20Repository: pgadmin-org/pgadmin4
Length of output: 2228
🏁 Script executed:
# Search for table creation templates in the tables directory
find web/pgadmin/browser/server_groups/servers/databases/schemas/tables -name '*.sql' -o -name '*.jinja2' | head -30Repository: pgadmin-org/pgadmin4
Length of output: 3907
🏁 Script executed:
# Search for how has_with_options is referenced in templates
rg 'has_with_options' --type py --type sql --type jinja2 -B 3 -A 3Repository: pgadmin-org/pgadmin4
Length of output: 94
🏁 Script executed:
# Look for CREATE TABLE template files
find web/pgadmin/browser/server_groups/servers/databases/schemas/tables -type f -name '*create*' | head -20Repository: pgadmin-org/pgadmin4
Length of output: 2667
🏁 Script executed:
# Find template files for tables (look for .sql files in templates directories)
find web/pgadmin/browser/server_groups/servers/databases/schemas/tables -path '*/templates/*' -type f | head -30Repository: pgadmin-org/pgadmin4
Length of output: 3542
🏁 Script executed:
# Search for WITH OPTIONS in template files
rg 'WITH OPTIONS' -B 2 -A 2Repository: pgadmin-org/pgadmin4
Length of output: 12287
🏁 Script executed:
# Search for the create.sql template for tables
find web/pgadmin/browser/server_groups/servers/databases/schemas/tables -name 'create.sql'Repository: pgadmin-org/pgadmin4
Length of output: 3765
🏁 Script executed:
# Let me read the actual template file to see the exact syntax
cat web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql | grep -A 5 "has_with_options"Repository: pgadmin-org/pgadmin4
Length of output: 1592
🏁 Script executed:
# Also check if there are any test cases that test WITH OPTIONS without actual options
find web/pgadmin/browser/server_groups/servers/databases/schemas/tables -path '*/tests/*' -name '*.sql' | xargs grep -l "WITH OPTIONS" 2>/dev/null | head -5Repository: pgadmin-org/pgadmin4
Length of output: 46
Remove is_primary_key from triggering WITH OPTIONS syntax
Lines 394–423: Setting has_modifications = True when is_primary_key is set independently causes has_with_options to be marked even when DEFAULT and NOT NULL remain unchanged. This results in SQL like column_name WITH OPTIONS with no options, which is invalid PostgreSQL syntax. The code comments correctly note that PRIMARY KEY is handled as a table-level constraint, so it should not trigger column-level WITH OPTIONS syntax. Remove the is_primary_key check (lines 405–407) to gate has_with_options only to actual option changes (DEFAULT or NOT NULL).
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`
around lines 367 - 423, Remove the check that sets has_modifications when
c.get('is_primary_key') is true inside the columns loop so that PRIMARY KEY does
not cause has_with_options to be set; instead only detect modifications via
differences in defval (original_defval vs defval) and attnotnull
(original_attnotnull vs attnotnull) as currently implemented, and continue to
skip identity columns (colconstype == 'i') before marking c['has_with_options']
= True; locate this logic around the loop over columns and the variables
has_modifications, c.get('is_primary_key'), original_defval/current_defval, and
original_attnotnull/current_attnotnull.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This pull request improves how tables that use the
OF TYPEclause (i.e., tables based on composite types) are handled in pgAdmin. The changes ensure that columns inherited from composite types are tracked for modifications, allowing the UI and generated SQL to correctly use theWITH OPTIONSsyntax when defaults or constraints are changed. This results in more accurate CREATE TABLE statements and better support for editing inherited columns.Enhancements for OF TYPE columns:
__init__.py, columns inherited from composite types now have aninheritedfromtypefield and store their original default and NOT NULL values, enabling the backend and UI to detect and compare modifications.columns/utils.py, the column parsing logic was updated to include inherited columns with modifications (such as changed defaults or NOT NULL constraints) in the final column list, marking them forWITH OPTIONSsyntax in the generated SQL.Improvements to CREATE TABLE SQL generation:
create.sqlfor versions 11+, 12+, 14+, 16+, and default), columns inherited from composite types that are modified now use theWITH OPTIONSsyntax, ensuring the generated SQL accurately reflects changes to defaults or constraints. [1] [2] [3] [4] [5]Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.