Allow users to customize ofType columns during table creation #229 #9580
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBackend records original DEFAULT/NOT NULL for columns inherited from composite types; frontend tracks inherited-type metadata and permits editing OF TYPE columns; CREATE templates render modified OF TYPE columns using WITH OPTIONS while parent-table inheritance remains comment-only. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as Frontend (Column UI)
participant API as Backend API
participant Gen as SQL Template Generator
User->>UI: Inspect or edit column metadata
UI->>UI: isInheritedFromType(), read `original_*` hidden fields
UI->>API: Fetch/submit column metadata
API->>API: get_oftype() adds `original_defval`/`original_attnotnull`, marks `inheritedfromtype`
API->>Gen: Provide column metadata (includes `has_with_options`, `inheritedfromtable`)
Gen->>Gen: Choose rendering branch (WITH OPTIONS / inherited-from-table comment / regular)
Gen-->>API: Return generated CREATE SQL
API-->>UI: Send updated metadata and SQL
UI-->>User: Display SQL and editing result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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:
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js
Outdated
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js
Outdated
Show resolved
Hide resolved
...owser/server_groups/servers/databases/schemas/tables/templates/tables/sql/16_plus/create.sql
Show resolved
Hide resolved
mzabuawala
force-pushed
the
fixes_ofType_223
branch
4 times, most recently
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).
...gadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py
Show resolved
Hide resolved
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.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py
Show resolved
Hide resolved
|
Fixed all CodeRabbit review comments if possible. |
mzabuawala
force-pushed
the
fixes_ofType_223
branch
2 times, most recently
from
7e5ed2d to
56b65fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
372-378:⚠️ Potential issue | 🟠 MajorLength/precision editable checks still block OF TYPE columns.
The
attlenandattprecisioneditablefunctions use rawstate.inheritedfromchecks, which block editing for OF TYPE columns even though they should be editable. Thedefvalfield was already updated to useisInheritedFromType()to allow editing for type-inherited columns while blocking table-inherited ones.Update these checks to match the
defvalpattern:Suggested fix for attlen (lines 372-378)
editable: function(state) { - // inheritedfrom has value then we should disable it - if (!isEmptyString(state.inheritedfrom)) { + // Block editing for table inheritance, but allow for type inheritance + if (!isEmptyString(state.inheritedfrom) && !obj.isInheritedFromType(state)) { return false; } return Boolean(obj.attlenRange(state)); },Suggested fix for attprecision (lines 404-410)
editable: function(state) { - // inheritedfrom has value then we should disable it - if (!isEmptyString(state.inheritedfrom)) { + // Block editing for table inheritance, but allow for type inheritance + if (!isEmptyString(state.inheritedfrom) && !this.isInheritedFromType(state)) { return false; } return Boolean(this.attprecisionRange(state)); },
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql`:
- Line 49: The template has a mismatched Jinja2 conditional around rendering the
column type: after {{c.displaytypname}} there's an {% else %} with no matching
{% if %}, causing a syntax error; fix by replacing that fragment so the type
rendering is a proper if/else (i.e. test whether c.displaytypname is present and
render it, otherwise call GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen,
c.attprecision, c.hasSqrBracket)), keeping the surrounding
inherited-from-type/parent-table branches intact and preserving references to
conn|qtIdent(c.name), c.inheritedfromtype, c.inheritedfromtable,
c.displaytypname and GET_TYPE.CREATE_TYPE_SQL.
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
493-518: Simplify the disabled logic for clarity.Lines 495-497 have unusual control flow - the
ifblock returnsisDisabled = falsebut immediately exits withreturn isDisabled = false, which is redundant and confusing.♻️ Suggested simplification
disabled: function(state) { let isDisabled = ['serial', 'bigserial', 'smallserial'].indexOf(state.cltype) > -1; - if(!isDisabled && this.isInheritedFromType(state)) { - return isDisabled = false; - } + // Allow editing for OF TYPE columns + if(this.isInheritedFromType(state)) { + return false; + } isDisabled = isDisabled || state.colconstype != 'n'; return isDisabled; },web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py (1)
367-375: Remove dead code or add TODO comment.The
pk_columnsset is built but never used (the usage is commented out on lines 390-391). This is dead code that adds unnecessary complexity.Either remove this block entirely, or if it's intentionally preserved for future use, add a clear TODO comment explaining the intent.
♻️ Option 1: Remove dead 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:
...owser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql
Outdated
Show resolved
Hide resolved
A composite type has its structure (data types, length, precision, scale, etc.) fully fixed at type definition time, so kept them as is. |
mzabuawala
force-pushed
the
fixes_ofType_223
branch
from
56b65fa to
6063ab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
493-518:⚠️ Potential issue | 🟡 MinorKeep the
colconstypeguard even for OF TYPE columns.The early return when
isInheritedFromType()is true skips thecolconstype != 'n'check, so defaults can be edited for identity/generated columns, which can yield invalid SQL. Removing that early return preserves the intended OF TYPE enablement while keeping the identity/generated guard.🛠️ Suggested fix
disabled: function(state) { let isDisabled = ['serial', 'bigserial', 'smallserial'].indexOf(state.cltype) > -1; - if(!isDisabled && this.isInheritedFromType(state)) { - return isDisabled = false; - } isDisabled = isDisabled || state.colconstype != 'n'; return isDisabled; },
mzabuawala
force-pushed
the
fixes_ofType_223
branch
from
6063ab9 to
65c96db
Compare
mzabuawala
changed the title
mzabuawala
force-pushed
the
fixes_ofType_223
branch
from
65c96db to
d12ec5f
Compare
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/static/js/column.ui.js`:
- Around line 493-499: The guard in the disabled function is a no-op because it
checks !isDisabled then assigns and returns false, which never alters a true
disabled state; update the logic in disabled (in the same function that calls
isInheritedFromType) to: if the intention is to force-enable OF TYPE columns,
change the guard to check for isDisabled && this.isInheritedFromType(state) and
return false to override the disable; otherwise remove the entire if-block that
references isInheritedFromType(state) to eliminate dead code so the function
simply returns the computed isDisabled value.
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py (1)
367-374:pk_columnsis computed but never used.The set is populated but the only code that references it (lines 389–391) is commented out. This is dead code that adds unnecessary computation and may confuse future maintainers.
♻️ Suggested cleanup
- # 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:Also remove the commented-out block at lines 386–391 that references
pk_columns.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
506-508:thisvsobjinconsistency indepChangearrow function.Line 507 mixes
obj.isNew(state)(closure) withthis.isInheritedFromType(state)(lexicalthisfrom the arrow function). Both resolve to the sameColumnSchemainstance here, but the inconsistency within a single expression is a readability concern. Consider usingobjconsistently (as done elsewhere in arrow-function callbacks in this file) orthisconsistently.
...gadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js
Show resolved
Hide resolved
3 participants
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
Bug Fixes
Improvements