Add support of DEPENDS/NO DEPENDS ON EXTENSION for INDEX.#6388 #9357
Conversation
WalkthroughAdds a "Depends on extensions" multi-select to the Index UI, surfaces extension options via schema AJAX, propagates dependsonextensions through utils and SQL templates to emit ALTER INDEX ... DEPENDS/NO DEPENDS ON EXTENSION, updates properties queries, adds tests/fixtures and docs, and removes CLUSTER/COMMENT bodies from the default alter template. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Index Dialog UI
participant Schema as IndexSchema
participant Backend as Web Backend
participant DB as PostgreSQL
User->>UI: open index create/edit
UI->>Schema: request schema (includes extensionsList)
Schema->>Backend: getNodeAjaxOptions(extensionNode)
Backend->>DB: query pg_extension/related nodes
DB-->>Backend: extensions list
Backend-->>Schema: formatted options (label,value,data)
Schema-->>UI: render dependsonextensions multi-select
User->>UI: select extensions, save
UI->>Backend: submit index data (dependsonextensions)
Backend->>Backend: utils normalizes dependsonextensions -> []
Backend->>DB: execute generated SQL (CREATE/ALTER + DEPENDS/NO DEPENDS)
DB-->>Backend: result
Backend-->>UI: success/failure
UI-->>User: show confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![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
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py (2)
245-247: Simplify the redundant logic.The conditional checks if
dependsonextensionsisNone, so the right-hand sidedata.get('dependsonextensions') or []will always evaluate to[]since we already know it'sNone.Apply this diff to simplify the logic:
if data.get('dependsonextensions') is None: - data['dependsonextensions'] = \ - data.get('dependsonextensions') or [] + data['dependsonextensions'] = []
278-280: Simplify the redundant logic.Same issue as lines 245-247: the conditional already checks that
dependsonextensionsisNone, making theor []fallback redundant.Apply this diff to simplify the logic:
if old_data.get('dependsonextensions') is None: - old_data['dependsonextensions'] = \ - old_data.get('dependsonextensions') or [] + old_data['dependsonextensions'] = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/index_definition.pngis excluded by!**/*.png
📒 Files selected for processing (23)
docs/en_US/index_dialog.rst(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/alter.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql(1 hunks)web/regression/javascript/schema_ui_files/index.ui.spec.js(1 hunks)
💤 Files with no reviewable changes (2)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js (2)
web/pgadmin/browser/static/js/node.js (5)
pgBrowser(26-26)data(610-610)data(677-680)res(1005-1005)res(1014-1018)web/pgadmin/browser/static/js/node_ajax.js (1)
getNodeAjaxOptions(83-136)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js (1)
web/regression/javascript/fake_gettext.js (1)
gettext(40-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-python-tests-pg (windows-latest, 18)
- GitHub Check: run-python-tests-pg (macos-latest, 16)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 17)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-python-tests-pg (windows-latest, 13)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 18)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 14)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 15)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 13)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 16)
- GitHub Check: run-python-tests-pg (macos-latest, 14)
- GitHub Check: run-python-tests-pg (macos-latest, 17)
- GitHub Check: run-python-tests-pg (macos-latest, 13)
- GitHub Check: build-docs
- GitHub Check: build-container
🔇 Additional comments (20)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql (1)
1-17: LGTM!The test file correctly demonstrates the usage of
DEPENDS ON EXTENSIONfor an index. The SQL syntax is valid for PostgreSQL 13+, and the test covers a comprehensive set of index options including the new extension dependency feature.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js (1)
137-147: LGTM!The
extensionsListimplementation correctly follows the established pattern for fetching and mapping node data. ThecacheLevel: 'server'parameter is appropriate since extensions are server-level objects, and the data transformation properly creates the required structure for the UI dropdown.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql (1)
1-2: LGTM!The test correctly demonstrates the
NO DEPENDS ON EXTENSIONsyntax for removing an extension dependency from an index. This complements theDEPENDS ON EXTENSIONtest and provides comprehensive coverage.web/regression/javascript/schema_ui_files/index.ui.spec.js (1)
205-211: LGTM!The test properly validates the
dependsonextensionsfield configuration, ensuring it exists as a multi-select dropdown. This provides good coverage for the UI schema changes.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json (2)
34-34: LGTM!The test data correctly includes
dependsonextensionswith theplpgsqlextension for the create operation. This ensures the feature is tested during index creation.
533-534: LGTM!The test data properly includes
dependsonextensionsfor the update operation, ensuring the feature is tested when modifying existing indexes.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql (1)
33-38: LGTM!The subquery correctly retrieves extension dependencies for the index by:
- Joining
pg_dependwithpg_extensionto get extension names- Filtering on
objid = cls.oidto match the current index- Using
array_agg(DISTINCT e.extname)to return a properly formatted arrayThe SQL follows PostgreSQL best practices and will return
NULLwhen no dependencies exist, which is handled appropriately by the Python backend.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/alter.sql (1)
1-9: LGTM!The template correctly handles cluster settings and index comments with proper SQL quoting and conditional rendering.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql (1)
33-39: LGTM!The template correctly emits
ALTER INDEX ... DEPENDS ON EXTENSIONstatements after index creation. The use of schema-qualified names and proper quoting is appropriate.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql (1)
1-16: LGTM!The test correctly exercises the extension dependency feature with appropriate PostgreSQL 13 syntax. The complex index name provides good coverage for special character handling.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql (1)
1-17: LGTM!The test correctly exercises PostgreSQL 15+ features including
NULLS NOT DISTINCTand extension dependencies on both plpgsql and postgres_fdw.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql (1)
1-20: LGTM!The test provides appropriate coverage for PostgreSQL 13+ with
IF NOT EXISTSsyntax and extension dependencies, correctly omitting the PG15+NULLS NOT DISTINCTfeature.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql (1)
1-21: LGTM!The test correctly validates PostgreSQL 15+ functionality with both
IF NOT EXISTSandNULLS NOT DISTINCTfeatures, along with proper extension dependency handling.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql (1)
27-33: LGTM! Extension dependency logic is correct.The conditional rendering properly emits ALTER INDEX ... DEPENDS ON EXTENSION statements for each extension in the dependsonextensions list. Identifier quoting is correct using conn|qtIdent.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json (3)
3-14: Good test setup for extension dependencies.Creating the postgres_fdw extension as a prerequisite for testing index dependencies is appropriate.
36-74: Comprehensive test coverage for index creation with extension dependencies.The test data properly exercises the new dependsonextensions field with multiple extensions (plpgsql and postgres_fdw) and includes expected output files for verification.
75-87: Good test for NO DEPENDS ON EXTENSION.This test verifies that reducing the dependsonextensions list properly generates NO DEPENDS ON EXTENSION statements for removed extensions.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json (1)
3-104: LGTM! Consistent test coverage for PostgreSQL 15+.The test scenarios properly mirror the 13_plus tests, ensuring consistent behavior across PostgreSQL versions for extension dependency management.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql (1)
97-110: Excellent differential update logic for extension dependencies.The template correctly compares old and new extension lists and generates appropriate ALTER INDEX statements:
- DEPENDS ON EXTENSION for newly added extensions
- NO DEPENDS ON EXTENSION for removed extensions
The logic properly handles edge cases and uses correct identifier quoting.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js (1)
509-523: Field definition is correct and extensionsList is properly populated.Verification confirms that
extensionsListat line 138 is correctly initialized as a lazy-evaluated function that fetches extension data via AJAX and transforms it into the expected label/value format. The field configuration at lines 509-523 is properly set up with appropriate version gating for PostgreSQL 13+ and correct UX controls.
| }, | ||
| getSchema: (treeNodeInfo, itemNodeData) => { | ||
| let nodeObj = pgAdmin.Browser.Nodes['index']; | ||
| let nodeExtObj = pgBrowser.Nodes['extension']; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the extension node is always registered.
The code assumes pgBrowser.Nodes['extension'] exists. If the extension node hasn't been registered, nodeExtObj will be undefined, potentially causing issues in getNodeAjaxOptions at line 138.
Run the following script to verify that the extension node is properly registered before this code executes:
🏁 Script executed:
#!/bin/bash
# Search for extension node registration to confirm it's loaded before index.js
rg -nP --type=js -C3 "Nodes\['extension'\]\s*=" Length of output: 828
🏁 Script executed:
#!/bin/bash
# Check for imports/requires of extension.js in index.js
rg -nP --type=js "extension" web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js
# Search for how these modules are registered/loaded
fd --type f -name "*.py" | xargs grep -l "extension.js\|index.js" | head -10Length of output: 417
🏁 Script executed:
#!/bin/bash
# Check for plugin registration or module loading configuration
find web/pgadmin/browser/server_groups/servers/databases -name "*.py" -type f -exec grep -l "extensionsList\|extension.*index" {} \; | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any blueprint or module registration in the tables and extensions directories
rg -nP "Blueprint\|register" web/pgadmin/browser/server_groups/servers/databases/extensions/ -g "*.py" | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for __init__.py files that might control script loading order
cat web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/__init__.pyLength of output: 39587
🏁 Script executed:
#!/bin/bash
# Find and examine the extensions module __init__.py to check script_load order
cat web/pgadmin/browser/server_groups/servers/databases/extensions/__init__.pyLength of output: 19448
🏁 Script executed:
#!/bin/bash
# Search for any module initialization order or registry configuration
find web/pgadmin/browser/server_groups/servers/databases -name "__init__.py" -type f -exec grep -l "script_load\|register" {} \; | head -5Length of output: 2034
🏁 Script executed:
#!/bin/bash
# Check if there are any import dependencies between index.js and extension.js
rg -nP --type=js "import|require|getNodeAjaxOptions" web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js | head -20Length of output: 684
🏁 Script executed:
#!/bin/bash
# Check if index.js includes any initialization that waits for extension node
rg -nP --type=js -A5 "nodeExtObj\|extension" web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.jsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any module loading/initialization logic in the framework
find web/pgadmin/browser -name "*.py" -type f -exec grep -l "CollectionNodeModule\|script_load" {} \; | xargs grep -l "sort\|order" | head -3Length of output: 111
🏁 Script executed:
#!/bin/bash
# Get full context around nodeExtObj definition and usage in index.js
rg -nP --type=js -B10 -A10 "let nodeExtObj" web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.jsLength of output: 1308
🏁 Script executed:
#!/bin/bash
# Check the collection.py for module loading/ordering logic
cat web/pgadmin/browser/collection.py | head -100Length of output: 3116
🏁 Script executed:
#!/bin/bash
# Check browser __init__.py for script loading order
rg -nP "script_load|sort|order" web/pgadmin/browser/__init__.py -A3 -B3 | head -60Length of output: 445
Add defensive null check for extension node before using it.
The code assumes pgBrowser.Nodes['extension'] is always defined, but both the extension and index modules are triggered on the same event with no documented ordering guarantee. While alphabetical directory structure may provide implicit safety, this lacks defensive programming.
Add a null/undefined check at line 128 before passing nodeExtObj to getNodeAjaxOptions, or verify and document that the framework guarantees extension.js loads first.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js
around line 128, the code assumes pgBrowser.Nodes['extension'] exists before
passing it into getNodeAjaxOptions; add a defensive null/undefined check and
handle the missing node: if nodeExtObj is undefined/null, either skip calling
getNodeAjaxOptions (return or no-op), or pass a safe default object (e.g., {})
and/or log a warning—ensure no property access occurs on nodeExtObj when it may
be absent.
| -- Index: Idx_$%{}[]()&*^!@"'`\/# | ||
|
|
||
| -- DROP INDEX IF EXISTS public."Idx_$%{}[]()&*^!@""'`\/#"; | ||
|
|
||
| CREATE UNIQUE INDEX IF NOT EXISTS "Idx_$%{}[]()&*^!@""'`\/#" | ||
| ON public.test_table_for_indexes USING btree | ||
| (id ASC NULLS LAST, name COLLATE pg_catalog."POSIX" text_pattern_ops ASC NULLS LAST) | ||
| INCLUDE(name, id) | ||
| NULLS NOT DISTINCT | ||
| WITH (fillfactor=10, deduplicate_items=False) | ||
| TABLESPACE pg_default | ||
| WHERE id < 100; | ||
|
|
||
| ALTER INDEX public."Idx_$%{}[]()&*^!@""'`\/#" | ||
| DEPENDS ON EXTENSION plpgsql; | ||
|
|
||
| COMMENT ON INDEX public."Idx_$%{}[]()&*^!@""'`\/#" | ||
| IS 'Test Comment'; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Verify filename mismatch with test content.
The filename suggests "no_depends" but the test actually includes an ALTER INDEX ... DEPENDS ON EXTENSION plpgsql statement on Lines 14-15. This appears to be a mismatch between the filename and the test content.
Please verify:
- Is this test intended to verify removing dependencies (NO DEPENDS)?
- Should this file be renamed to reflect that it adds a dependency?
- Or should the test content be updated to match the "no_depends" scenario?
Run the following script to check if there are other test files that might clarify the intended test scenarios:
🏁 Script executed:
#!/bin/bash
# Search for test files related to extension dependencies
fd -e sql -e msql . web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/ | rg -i "depend|ext"Length of output: 810
Verify filename mismatch with test content.
NO DEPENDS ON EXTENSION syntax was added in PostgreSQL 13, and it marks the index as no longer dependent on that extension if NO is specified. The filename alter_index_no_depends.sql indicates the test should remove extension dependency, but the file actually contains ALTER INDEX ... DEPENDS ON EXTENSION (lines 14-15), which adds a dependency instead.
This represents a mismatch. Either:
- The filename should change to reflect that it adds a dependency (e.g.,
alter_index_with_depends.sqloralter_index_add_depends.sql) - Or the test content should be updated to use
NO DEPENDS ON EXTENSIONto match the "no_depends" intent
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql
around lines 1-18, the test filename implies removing an extension dependency
("no_depends") but the SQL uses "ALTER INDEX ... DEPENDS ON EXTENSION" which
adds a dependency; fix by making content and filename consistent: either change
the ALTER statement to "ALTER INDEX ... NO DEPENDS ON EXTENSION plpgsql" to
match the current filename, or rename the file to something like
alter_index_with_depends.sql (or alter_index_add_depends.sql) if you intend to
test adding a dependency; update any test metadata or references accordingly.
| ( | ||
| SELECT array_agg(DISTINCT e.extname) | ||
| FROM pg_depend d | ||
| JOIN pg_extension e ON d.refobjid = e.oid | ||
| WHERE d.objid = cls.oid | ||
| ) AS dependsonextensions, |
There was a problem hiding this comment.
Add filters for dependency type and class in the extension dependency query.
The subquery retrieves extension dependencies but doesn't filter by deptype. When using ALTER INDEX ... DEPENDS ON EXTENSION, PostgreSQL creates a dependency with deptype = 'x'. Without this filter, the query may incorrectly include other dependency types.
Additionally, filtering by classid ensures you're only considering index dependencies.
Apply this diff to add the necessary filters:
(
SELECT array_agg(DISTINCT e.extname)
FROM pg_depend d
JOIN pg_extension e ON d.refobjid = e.oid
- WHERE d.objid = cls.oid
+ WHERE d.objid = cls.oid
+ AND d.classid = 'pg_class'::regclass
+ AND d.deptype = 'x'
) AS dependsonextensions,📝 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.
| ( | |
| SELECT array_agg(DISTINCT e.extname) | |
| FROM pg_depend d | |
| JOIN pg_extension e ON d.refobjid = e.oid | |
| WHERE d.objid = cls.oid | |
| ) AS dependsonextensions, | |
| ( | |
| SELECT array_agg(DISTINCT e.extname) | |
| FROM pg_depend d | |
| JOIN pg_extension e ON d.refobjid = e.oid | |
| WHERE d.objid = cls.oid | |
| AND d.classid = 'pg_class'::regclass | |
| AND d.deptype = 'x' | |
| ) AS dependsonextensions, |
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql
around lines 34 to 39, the subquery collecting extension names must only return
extension deps created by ALTER INDEX ... DEPENDS ON EXTENSION and only for
index objects; add filters to the WHERE clause: require d.deptype = 'x' and
restrict d.classid to the pg_index class (e.g. d.classid = 'pg_index'::regclass)
so only extension dependencies of indexes are aggregated.
RohitBhati8269
force-pushed
the
GH-6388
branch
from
e344a8b to
b1076bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js (1)
128-147: Add defensive null check for extension node.
pgBrowser.Nodes['extension']could be undefined if the extension module hasn't been registered yet. While module loading order may provide implicit safety, defensive programming is preferred.Apply this diff to add a null check:
getSchema: (treeNodeInfo, itemNodeData) => { let nodeObj = pgAdmin.Browser.Nodes['index']; let nodeExtObj = pgBrowser.Nodes['extension']; return new IndexSchema( { tablespaceList: ()=>getNodeListByName('tablespace', treeNodeInfo, itemNodeData, {}, (m)=>{ return (m.label != 'pg_global'); }), amnameList : ()=>getNodeAjaxOptions('get_access_methods', nodeObj, treeNodeInfo, itemNodeData, {jumpAfterNode: 'schema'}), columnList: ()=>getNodeListByName('column', treeNodeInfo, itemNodeData, {}), collationList: ()=>getNodeAjaxOptions('get_collations', nodeObj, treeNodeInfo, itemNodeData, {jumpAfterNode: 'schema'}), opClassList: ()=>getNodeAjaxOptions('get_op_class', nodeObj, treeNodeInfo, itemNodeData, {jumpAfterNode: 'schema'}), - extensionsList:()=>getNodeAjaxOptions('nodes', nodeExtObj, treeNodeInfo, itemNodeData, { cacheLevel: 'server'}, + extensionsList:()=>nodeExtObj ? getNodeAjaxOptions('nodes', nodeExtObj, treeNodeInfo, itemNodeData, { cacheLevel: 'server'}, (data)=>{ let res = []; if (data && _.isArray(data)) { _.each(data, function(d) { res.push({label: d.label, value: d.label, data: d}); }); } return res; - }), + }) : Promise.resolve([]), },web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql (1)
34-39: Add filters for dependency type and class in the extension dependency query.The subquery lacks
deptype = 'x'andclassidfilters. Without these, it may include incorrect dependency types.Apply this diff to add the necessary filters:
( SELECT array_agg(DISTINCT e.extname) FROM pg_depend d JOIN pg_extension e ON d.refobjid = e.oid - WHERE d.objid = cls.oid + WHERE d.objid = cls.oid + AND d.classid = 'pg_class'::regclass + AND d.deptype = 'x' ) AS dependsonextensions,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/index_definition.pngis excluded by!**/*.png
📒 Files selected for processing (24)
docs/en_US/index_dialog.rst(1 hunks)docs/en_US/materialized_view_dialog.rst(3 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql(0 hunks)web/regression/javascript/schema_ui_files/index.ui.spec.js(1 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/en_US/index_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql
- web/regression/javascript/schema_ui_files/index.ui.spec.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js (1)
web/regression/javascript/fake_gettext.js (1)
gettext(40-42)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js (2)
web/pgadmin/browser/static/js/node.js (5)
pgBrowser(26-26)data(610-610)data(677-680)res(1005-1005)res(1014-1018)web/pgadmin/browser/static/js/node_ajax.js (1)
getNodeAjaxOptions(83-136)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build-container
- GitHub Check: build-python-package
- GitHub Check: run-python-tests-pg (windows-latest, 18)
- GitHub Check: run-python-tests-pg (macos-latest, 13)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 13)
- GitHub Check: run-javascript-tests (macos-latest)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 15)
- GitHub Check: run-python-tests-pg (macos-latest, 14)
- GitHub Check: run-python-tests-pg (macos-latest, 18)
- GitHub Check: run-python-tests-pg (macos-latest, 16)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-javascript-tests (ubuntu-22.04)
- GitHub Check: run-javascript-tests (windows-latest)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: build-tarball
- GitHub Check: run-feature-tests-pg (15)
🔇 Additional comments (15)
docs/en_US/materialized_view_dialog.rst (3)
1-135: Scope mismatch: This documentation file doesn't directly relate to the PR's stated objective.This file documents the Materialized View dialog, but the PR is titled "Add support of DEPENDS/NO DEPENDS ON EXTENSION for INDEX." The enriched summary mentions related changes to
index_dialog.rstintroducing "Depends on extensions" functionality, but only the materialized view documentation is provided for review. Clarify whether:
- This file's tab reorganization is part of a broader UI refactoring affecting both materialized views and indexes, or
- This file was included in the PR by mistake.
13-13: Tab sequence updates are consistently applied throughout the file.The changes correctly reflect the removal/reorganization of tabs from the dialog tab sequence. All references to tab names have been updated consistently:
- Line 13: Tab order declaration updated
- Lines 31, 37: "Definition" tab references (formerly "Storage")
- Lines 51, 62, 78: Downstream tabs correctly referenced
Also applies to: 31-31, 37-37, 51-51, 62-62, 78-78
33-35: All image file references and alt-text verified successfully.The verification confirms:
- All 6 referenced image files exist in
docs/en_US/images/✓- Alt-text is present and descriptive at the specified lines (lines 34 and 54) ✓
- Filenames in the RST file match the actual image files ✓
- RST image directives are properly formatted with
:alt:and:align:options ✓No broken references or missing files detected.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json (2)
34-34: LGTM!The test data appropriately includes extension dependencies using the built-in
plpgsqlextension, which is a safe choice for testing.
533-534: LGTM!The PUT test data correctly includes extension dependencies for update scenarios.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql (1)
1-16: LGTM!The test SQL correctly demonstrates index creation with extension dependencies. The structure follows PostgreSQL syntax properly with separate
ALTER INDEX ... DEPENDS ON EXTENSIONstatements for each extension.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql (1)
1-20: LGTM!The test SQL correctly uses
IF NOT EXISTSfor idempotent testing and properly declares extension dependencies with separateALTER INDEXstatements.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql (1)
1-21: LGTM!The 15_plus version correctly includes
NULLS NOT DISTINCT(line 9) which is a PostgreSQL 15+ feature, and properly declares extension dependencies.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json (1)
3-104: Excellent test coverage!The test scenarios comprehensively cover the extension dependency feature:
- Creating an extension for testing (postgres_fdw)
- Creating an index with multiple extension dependencies
- Altering dependencies with
NO DEPENDS ONscenario- Proper cleanup of both index and extension
This ensures both creation and modification paths are validated.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js (1)
509-523: LGTM!The
dependsonextensionsfield is properly configured with:
- Multi-select capability with select-all and clear options
- Appropriate placeholder text for user guidance
min_version: 130000correctly restricts to PostgreSQL 13+- Available in all relevant modes (create, edit, properties)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql (2)
97-109: Verify backend initialization ofdependsonextensionsfield.The template correctly handles the case where
dependsonextensionsis not present in the data dict (line 98 sets it toNone), and the condition at line 99 prevents SQL generation when it's absent. However, ensure that the backend code initializes this field as an empty list when not explicitly provided, rather than leaving it undefined—this prevents subtle issues if the field is accessed elsewhere without proper null checks.
97-109: Extension dependency delta logic is sound.The comparison at line 99 correctly detects changes, the
| uniquefilter at line 100 prevents duplicate processing, and the mutually exclusive conditionals at lines 102–107 properly partition additions and removals. The logic correctly generatesDEPENDS ON EXTENSIONfor newly added extensions andNO DEPENDS ON EXTENSIONfor removed ones.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json (3)
3-14: Extension creation precedes index creation (good dependency ordering).The test correctly creates the
postgres_fdwextension before attempting to create an index that depends on it. Thestore_object_id: trueflag ensures the extension ID is captured for cleanup.
75-87: Alter scenario correctly validates delta handling.The alter test reduces
dependsonextensionsfrom["plpgsql", "postgres_fdw"]to["plpgsql"], which should trigger the template logic at lines 105–107 of update.sql to emitNO DEPENDS ON EXTENSION postgres_fdw. This is the correct way to test the removal path.
96-104: Placeholder substitution confirmed to work correctly.Verification trace confirms the placeholder ID substitution mechanism functions as intended:
- Line 13 area: The create extension scenario sets
"store_object_id": true, which invokesstore_object_ids()to extract the "name" field ("postgres_fdw") and store the returned object ID in theall_object_idsdictionary- Line 101-103: The delete extension scenario uses
"preprocess_data": true, which triggers thepreprocess_data()function during execution- Execution order: The create scenario (line 4) executes before the delete scenario (line 97)
- Substitution: The
replace_placeholder_with_id()method strips the angle brackets from<postgres_fdw>, looks up the key inall_object_ids, and returns the actual ID before the DELETE request is sentThe test framework correctly handles placeholder resolution as designed.
...owser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json
Show resolved
Hide resolved
RohitBhati8269
force-pushed
the
GH-6388
branch
from
b1076bd to
ea81c64
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql (1)
85-87: Move{% endif %}to its own line for consistency.The closing
{% endif %}is placed on the same line as the SQL statement's semicolon, which is inconsistent with the template style used throughout the rest of the file (e.g., lines 5, 15, 26, etc.). This makes the template harder to read.Apply this diff to improve consistency:
COMMENT ON INDEX {{conn|qtIdent(data.schema, data.name)}} - IS {{data.description|qtLiteral(conn)}};{% endif %} + IS {{data.description|qtLiteral(conn)}}; +{% endif %}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/index_definition.pngis excluded by!**/*.png
📒 Files selected for processing (24)
docs/en_US/index_dialog.rst(1 hunks)docs/en_US/materialized_view_dialog.rst(3 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql(0 hunks)web/regression/javascript/schema_ui_files/index.ui.spec.js(1 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql
🚧 Files skipped from review as they are similar to previous changes (16)
- web/regression/javascript/schema_ui_files/index.ui.spec.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json
- docs/en_US/materialized_view_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py
- docs/en_US/index_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql
🔇 Additional comments (7)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql (2)
1-26: LGTM: Index creation template structure is correct.The CREATE INDEX template properly handles all standard PostgreSQL index options including uniqueness, concurrency, columns, INCLUDE clause, storage parameters, tablespace, and partial index predicates. The conditional rendering logic for create vs. non-create modes is appropriate.
27-32: LGTM: Extension dependency handling is correctly implemented.The template properly generates separate
ALTER INDEX ... DEPENDS ON EXTENSIONstatements after index creation, which aligns with PostgreSQL's requirement that dependency declarations must follow object creation. The conditional rendering and identifier quoting are correct.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql (1)
1-2: LGTM: Test SQL correctly demonstrates NO DEPENDS ON EXTENSION.The ALTER INDEX statement with
NO DEPENDS ON EXTENSION postgres_fdwis syntactically correct and aligns with the test scenario in test_indexes.json where the index dependencies are reduced from["plpgsql", "postgres_fdw"]to["plpgsql"].web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json (4)
3-14: LGTM: Extension creation scenario properly configured.The postgres_fdw extension creation is necessary for testing index extension dependencies, and
store_object_id: truecorrectly enables referencing this extension in the cleanup (drop) scenario.
75-87: LGTM: Alter scenario correctly tests dependency removal.The test properly validates the
NO DEPENDS ON EXTENSIONfunctionality by reducingdependsonextensionsfrom["plpgsql", "postgres_fdw"]to["plpgsql"], which should generate an ALTER INDEX statement removing the postgres_fdw dependency.
88-104: LGTM: Cleanup scenarios properly structured.The drop scenarios correctly clean up test objects in the proper order (index before extension) and use
preprocess_data: truewith placeholder syntax<postgres_fdw>to reference the stored extension ID.
36-74: All test output files exist and contain correct DDL with extension dependencies.Verification confirmed:
- Both
create_index_with_ext.sqlandcreate_index_with_ext.msqlexist with proper DDL statements- Extension dependencies are correctly declared:
ALTER INDEX ... DEPENDS ON EXTENSION plpgsql;andALTER INDEX ... DEPENDS ON EXTENSION postgres_fdw;- Index structure matches test data (UNIQUE, INCLUDE columns, fillfactor=10, WHERE constraint, special characters in name, comment)
...owser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json
Show resolved
Hide resolved
| {% set new_exts = data.dependsonextensions if 'dependsonextensions' in data else None %} | ||
| {% if new_exts is not none and old_exts != new_exts %} | ||
| {% for ext in (old_exts + new_exts) | unique %} | ||
|
|
||
| {% if ext in new_exts and ext not in old_exts %} | ||
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | ||
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | ||
| {% elif ext in old_exts and ext not in new_exts %} | ||
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | ||
| NO DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% endif %} No newline at end of file |
There was a problem hiding this comment.
Ensure new_exts is converted to list for consistent type handling.
Line 97 explicitly converts old_exts to a list using the | list filter, but line 98 does not convert new_exts. If data.dependsonextensions is a tuple or other iterable type, the concatenation on line 100 (old_exts + new_exts) and the comparison on line 99 (old_exts != new_exts) may behave unexpectedly or fail due to type mismatch.
Apply this diff to ensure consistent list type for both variables:
{% set old_exts = (o_data.dependsonextensions or []) | list %}
-{% set new_exts = data.dependsonextensions if 'dependsonextensions' in data else None %}
+{% set new_exts = (data.dependsonextensions | list) if 'dependsonextensions' in data else None %}📝 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.
| {% set new_exts = data.dependsonextensions if 'dependsonextensions' in data else None %} | |
| {% if new_exts is not none and old_exts != new_exts %} | |
| {% for ext in (old_exts + new_exts) | unique %} | |
| {% if ext in new_exts and ext not in old_exts %} | |
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | |
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | |
| {% elif ext in old_exts and ext not in new_exts %} | |
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | |
| NO DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | |
| {% endif %} | |
| {% endfor %} | |
| {% endif %} | |
| {% set new_exts = (data.dependsonextensions | list) if 'dependsonextensions' in data else None %} | |
| {% if new_exts is not none and old_exts != new_exts %} | |
| {% for ext in (old_exts + new_exts) | unique %} | |
| {% if ext in new_exts and ext not in old_exts %} | |
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | |
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | |
| {% elif ext in old_exts and ext not in new_exts %} | |
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | |
| NO DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | |
| {% endif %} | |
| {% endfor %} | |
| {% endif %} |
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql
around lines 98 to 110, new_exts may be a non-list iterable causing unexpected
behavior when compared or concatenated with old_exts; convert new_exts to a list
(e.g., use the Jinja2 | list filter when assigning new_exts from
data.dependsonextensions) so comparisons (old_exts != new_exts) and
concatenation (old_exts + new_exts) operate on the same type.
d3b4f85 to
25295c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql (1)
97-110: Ensurenew_extsis converted to list for consistent type handling (unresolved from previous review).Line 97 explicitly converts
old_extsto a list using the| listfilter, but line 98 does not convertnew_exts. Ifdata.dependsonextensionsis a tuple, set, or other iterable type, this can cause subtle bugs:
- Line 99 comparison (
old_exts != new_exts) may incorrectly returnTruewhen comparing list vs tuple with identical content- Line 100 concatenation (
old_exts + new_exts) works in Jinja2 but relies on duck-typing rather than explicit consistencyApply this diff to ensure both variables are lists:
{% set old_exts = (o_data.dependsonextensions or []) | list %} -{% set new_exts = data.dependsonextensions if 'dependsonextensions' in data else None %} +{% set new_exts = (data.dependsonextensions | list) if 'dependsonextensions' in data else None %}web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql (1)
14-15: Filename/content mismatch: file adds dependency instead of removing it.The filename
alter_index_no_depends.sqlindicates the test should remove an extension dependency usingNO DEPENDS ON EXTENSION, but the content usesDEPENDS ON EXTENSION plpgsql, which adds a dependency instead.Either:
- Change the SQL to
ALTER INDEX ... NO DEPENDS ON EXTENSION postgres_fdw;to match the filename intent, or- Rename the file to
alter_index_add_depends.sqlor similar to match the current contentweb/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json (1)
75-87: Test scenario intent doesn't match expected output file content.The test scenario on line 77 is named "Alter index with NO DEPENDS ON" and changes
dependsonextensionsfrom["plpgsql", "postgres_fdw"](created in line 70) to["plpgsql"](line 83). This should generate SQL that removes thepostgres_fdwdependency:ALTER INDEX ... NO DEPENDS ON EXTENSION postgres_fdw;However, the referenced expected output file
alter_index_no_depends.sqlcontainsALTER INDEX ... DEPENDS ON EXTENSION plpgsql;, which adds a dependency instead of removing one.Either:
- Update the expected output file to contain
NO DEPENDS ON EXTENSION postgres_fdw, or- Change the test scenario to match what the expected output file demonstrates
Also note that the past review identified
alter_index_no_depends.msql(line 86) as missing.
🧹 Nitpick comments (1)
web/regression/javascript/schema_ui_files/index.ui.spec.js (1)
205-210: LGTM! Test provides adequate structural verification.The test correctly verifies that the
dependsonextensionsfield exists with the expected properties (typeselectand multi-select enabled viacontrolProps.multiple). This follows the existing pattern of field verification tests in the file and will catch regressions if the field structure changes.For completeness, you could optionally enhance the test to verify:
- The field label
- Data source binding (e.g., that it's connected to
extensionsList)- Visibility or editability conditions, if applicable
However, the current test is consistent with similar tests in this file and provides the essential verification needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/index_definition.pngis excluded by!**/*.png
📒 Files selected for processing (24)
docs/en_US/index_dialog.rst(1 hunks)docs/en_US/materialized_view_dialog.rst(3 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql(0 hunks)web/regression/javascript/schema_ui_files/index.ui.spec.js(1 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql
🚧 Files skipped from review as they are similar to previous changes (14)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js
- docs/en_US/index_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql
- docs/en_US/materialized_view_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py
🔇 Additional comments (7)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql (1)
97-110: Logic for computing extension dependencies is sound.The approach of computing the union of old and new extensions, then iterating to emit
DEPENDS ONfor additions andNO DEPENDS ONfor removals, is correct. Identifier quoting viaconn|qtIdentis proper for both schema-qualified index names and extension names.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql (3)
1-8: LGTM! CREATE INDEX statement is correctly structured.The index creation statement properly uses PostgreSQL 15+ features (NULLS NOT DISTINCT), correctly quotes the special-character index name, and includes all necessary clauses in the proper order.
10-14: LGTM! Extension dependencies are correctly declared.The ALTER INDEX DEPENDS ON EXTENSION statements follow the correct PostgreSQL syntax, with separate ALTER statements for each extension dependency.
16-17: LGTM! Comment statement is correct.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql (1)
1-16: LGTM! Version-appropriate test for PostgreSQL 13+.This test correctly omits the NULLS NOT DISTINCT clause (line 5 in the 15_plus version), which is only available in PostgreSQL 15+. The extension dependency syntax (DEPENDS ON EXTENSION) is correctly used, as it was introduced in PostgreSQL 13.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json (2)
36-74: LGTM! Test scenario correctly defines extension dependencies.The test creates an index with dependencies on both
plpgsqlandpostgres_fdwextensions (line 70), and the referenced expected output files exist and contain the appropriateALTER INDEX ... DEPENDS ON EXTENSIONstatements for both extensions.
3-35: LGTM! Test setup scenarios are properly structured.The extension and table creation scenarios correctly use
store_object_id: trueto track created objects for the subsequent index tests and cleanup operations.
| ALTER INDEX public."Idx_$%{}[]()&*^!@""'`\/#" | ||
| DEPENDS ON EXTENSION plpgsql; |
There was a problem hiding this comment.
Filename/content mismatch: file adds dependency instead of removing it.
Similar to the 15_plus version, this filename suggests NO DEPENDS ON EXTENSION (removing dependencies), but the content uses DEPENDS ON EXTENSION plpgsql (adding a dependency).
Either:
- Update the SQL to use
ALTER INDEX ... NO DEPENDS ON EXTENSION postgres_fdw;to match the filename, or - Rename the file to reflect that it adds a dependency
🤖 Prompt for AI Agents
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql
lines 13-14: the SQL currently adds a dependency (DEPENDS ON EXTENSION plpgsql)
but the filename indicates it should remove one; change the statement to use NO
DEPENDS ON EXTENSION postgres_fdw to match the file name (i.e., ALTER INDEX ...
NO DEPENDS ON EXTENSION postgres_fdw), keeping the same index identifier/quoting
and escaping as-is.
25295c8 to
dd23f7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql (1)
15-16: Consider adding a trailing newline.The file ends without a trailing newline after line 16. Many style guides and linters prefer files to end with a newline for better compatibility with text processing tools and version control systems.
Apply this diff to add a trailing newline:
COMMENT ON INDEX public."Idx_$%{}[]()&*^!@""'`\/#" - IS 'Test Comment'; + IS 'Test Comment'; +web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py (1)
245-247: Simplifydependsonextensionsnormalization for readabilityThe current pattern:
if data.get('dependsonextensions') is None: data['dependsonextensions'] = data.get('dependsonextensions') or [](and the equivalent for
old_data) is functionally fine but a bit redundant. You could simplify and make intent clearer with something like:- if data.get('dependsonextensions') is None: - data['dependsonextensions'] = \ - data.get('dependsonextensions') or [] + if not data.get('dependsonextensions'): + data['dependsonextensions'] = [] ... - if old_data.get('dependsonextensions') is None: - old_data['dependsonextensions'] = \ - old_data.get('dependsonextensions') or [] + if not old_data.get('dependsonextensions'): + old_data['dependsonextensions'] = []or by using
setdefaultif you never rely on distinguishingNonefrom an empty list.Also applies to: 278-280
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/index_definition.pngis excluded by!**/*.png
📒 Files selected for processing (24)
docs/en_US/index_dialog.rst(1 hunks)docs/en_US/materialized_view_dialog.rst(3 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql(0 hunks)web/regression/javascript/schema_ui_files/index.ui.spec.js(1 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/en_US/index_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql
- docs/en_US/materialized_view_dialog.rst
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (16)
🔇 Additional comments (6)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql (2)
9-13: No action required—extensions are properly configured in test setup.The test framework explicitly creates the
postgres_fdwextension as part of the test setup via the "NODE-extension.obj" endpoint (referenced in test_indexes.json). The test lifecycle manages extension availability: the extension is created before executing the ALTER INDEX statements and cleaned up afterward. The original concern about missing extensions in the test environment is not applicable here.
1-7: Verify whether the INCLUDE redundancy pattern is intentional test behavior.The table prerequisites are properly handled—
test_table_for_indexesis created with bothidandnamecolumns via the test setup before the index is created.However, the INCLUDE clause at line 4 includes both
nameandid, which are already the indexed columns at line 3. This redundancy is not a syntax error, but it is present consistently across all 30+ test files in this directory (11_plus, 13_plus, 15_plus). The test data explicitly defines"include": ["name", "id"], suggesting this is intentional.Confirm whether this redundant INCLUDE is intended to test edge-case PostgreSQL behavior. If so, add a comment to the test data or SQL explaining the purpose. If not, the test data should be corrected to remove the overlapping columns from INCLUDE across the test suite.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json (1)
34-35: Dependsonextensions test data wiring looks correctUsing
dependsonextensions: ["plpgsql"]in both the create and PUT scenarios cleanly exercises the new field without affecting existing tests; structure and naming match the rest of the JSON.Also applies to: 533-535
web/regression/javascript/schema_ui_files/index.ui.spec.js (1)
205-210: Good targeted coverage fordependsonextensionsUI fieldThe new test cleanly verifies that
dependsonextensionsis present, typed as aselect, and configured for multi-select, which is exactly what we need for the new schema option.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json (1)
3-104: 15_plus extension/index scenarios are consistent and completeThe new 15_plus scenarios (extension create/drop plus index create/alter/drop with
dependsonextensions) mirror the 13_plus flow and match the structure expected by the updated templates andutils.get_sql(named index, include list, column options, dependency list, and expected SQL/MSQL filenames). With the previously reported missing fixtures now addressed, this block looks good.web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json (1)
3-104: All referenced expected files for the new test scenarios (lines 3–104) exist.Verification confirms that both
create_index_with_ext.sql,create_index_with_ext.msql,alter_index_no_depends.sql, andalter_index_no_depends.msqlare present. The new test scenarios for the extension/index feature are properly supported by their expected output files.
| {% if data.dependsonextensions %} | ||
| {% for ext in data.dependsonextensions %} | ||
|
|
||
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | ||
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | ||
| {% endfor %} | ||
| {% endif %} |
There was a problem hiding this comment.
Guard DEPENDS ON EXTENSION block for unnamed indexes
If an index is created without an explicit name but dependsonextensions is (incorrectly) populated, the template will emit an ALTER INDEX with a missing identifier (data.name is null/empty), resulting in invalid SQL.
You can defensively guard this block to avoid generating bad DDL for unnamed indexes:
-{% if data.dependsonextensions %}
+{% if data.dependsonextensions and data.name %}
{% for ext in data.dependsonextensions %}
ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }}
DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }};
{% endfor %}
{% endif %}This keeps normal named-index behavior intact while making the edge case safe.
📝 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.
| {% if data.dependsonextensions %} | |
| {% for ext in data.dependsonextensions %} | |
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | |
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | |
| {% endfor %} | |
| {% endif %} | |
| {% if data.dependsonextensions and data.name %} | |
| {% for ext in data.dependsonextensions %} | |
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | |
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | |
| {% endfor %} | |
| {% endif %} |
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql
around lines 33-39, the DEPENDS ON EXTENSION block emits ALTER INDEX even when
data.name is empty (unnamed index), producing invalid SQL; change the template
to only render the ALTER INDEX loop when both data.dependsonextensions is
non-empty and data.name is present/non-empty (i.e., add a guard that checks
data.name before emitting ALTER INDEX for each extension) so named-index
behavior is unchanged but unnamed indexes do not generate bad DDL.
dd23f7b to
ef97fa5
Compare
RohitBhati8269
force-pushed
the
GH-6388
branch
from
ef97fa5 to
8dcf99f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js (1)
128-148: Add defensive null check for extension node.The past review comment correctly identified that
pgBrowser.Nodes['extension']may be undefined if the extension module hasn't loaded. Without a guard, line 139'sgetNodeAjaxOptionscall withnodeExtObjcould fail.Add a null check before using
nodeExtObj:let nodeExtObj = pgBrowser.Nodes['extension']; +if (!nodeExtObj) { + console.warn('Extension node not available'); +} return new IndexSchema( { tablespaceList: ()=>getNodeListByName('tablespace', treeNodeInfo, itemNodeData, {}, (m)=>{ return (m.label != 'pg_global'); }), amnameList : ()=>getNodeAjaxOptions('get_access_methods', nodeObj, treeNodeInfo, itemNodeData, {jumpAfterNode: 'schema'}), columnList: ()=>getNodeListByName('column', treeNodeInfo, itemNodeData, {}), collationList: ()=>getNodeAjaxOptions('get_collations', nodeObj, treeNodeInfo, itemNodeData, {jumpAfterNode: 'schema'}), opClassList: ()=>getNodeAjaxOptions('get_op_class', nodeObj, treeNodeInfo, itemNodeData, {jumpAfterNode: 'schema'}), - extensionsList:()=>getNodeAjaxOptions('nodes', nodeExtObj, treeNodeInfo, itemNodeData, { cacheLevel: 'server'}, + extensionsList: nodeExtObj ? ()=>getNodeAjaxOptions('nodes', nodeExtObj, treeNodeInfo, itemNodeData, { cacheLevel: 'server'}, (data)=>{ let res = []; if (data && _.isArray(data)) { _.each(data, function(d) { res.push({label: d.label, value: d.label, data: d}); }); } return res; - }), + }) : ()=>Promise.resolve([]), },web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql (1)
33-39: Guard DEPENDS ON EXTENSION block for unnamed indexes.The past review comment correctly identified that if
data.nameis empty (unnamed index) butdependsonextensionsis populated, this will emitALTER INDEXwith a missing identifier, producing invalid SQL.Apply this diff to guard the block:
-{% if data.dependsonextensions %} +{% if data.dependsonextensions and data.name %} {% for ext in data.dependsonextensions %} ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; {% endfor %} {% endif %}web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql (1)
97-110: Ensure consistent list type for new_exts.The past review comment correctly identified that
old_extsis explicitly converted to a list (line 97), butnew_exts(line 98) is not. Ifdata.dependsonextensionsis a tuple or other iterable, the concatenation on line 100 may behave unexpectedly due to type mismatch.Apply this diff to ensure consistent list type:
{% set old_exts = (o_data.dependsonextensions or []) | list %} -{% set new_exts = data.dependsonextensions if 'dependsonextensions' in data else None %} +{% set new_exts = (data.dependsonextensions | list) if 'dependsonextensions' in data else None %} {% if new_exts is not none and old_exts != new_exts %} {% for ext in (old_exts + new_exts) | unique %}
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql (1)
1-16: Add trailing newline for consistency.The expected SQL output is correct, but the file lacks a trailing newline, which can cause issues with some diff tools and violates POSIX text file standards.
Add a newline at the end of the file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/index_definition.pngis excluded by!**/*.png
📒 Files selected for processing (24)
docs/en_US/index_dialog.rst(1 hunks)docs/en_US/materialized_view_dialog.rst(3 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql(0 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql(0 hunks)web/regression/javascript/schema_ui_files/index.ui.spec.js(1 hunks)
💤 Files with no reviewable changes (3)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_name_fillfactor_comment.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/default/alter.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_name_fillfactor_comment.sql
🚧 Files skipped from review as they are similar to previous changes (13)
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/index_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/15_plus/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.ui.js
- web/regression/javascript/schema_ui_files/index.ui.spec.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/create_index_with_ext.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/utils.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/alter_index_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/create_index_with_ext.msql
- docs/en_US/materialized_view_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/alter_index_no_depends.msql
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/static/js/index.js (3)
web/pgadmin/browser/static/js/node.js (5)
pgBrowser(26-26)data(610-610)data(677-680)res(1005-1005)res(1014-1018)web/pgadmin/browser/static/js/node_ajax.js (1)
getNodeAjaxOptions(83-136)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js (1)
nodeObj(135-135)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 14)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (16)
🔇 Additional comments (3)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/15_plus/test_indexes.json (1)
3-104: LGTM!The test scenarios properly exercise the extension dependency feature with a complete create-alter-delete flow. The test data correctly specifies dependencies and validates both the DEPENDS ON and NO DEPENDS ON paths.
docs/en_US/index_dialog.rst (1)
51-53: LGTM!The documentation clearly explains the new "Depends on extensions" field, provides a concrete example, and describes the cascade behavior when the extension is dropped.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/indexes/tests/13_plus/test_indexes.json (1)
3-104: LGTM!The test scenarios are well-structured and consistent with the 15_plus test suite, providing comprehensive coverage of the extension dependency feature for PostgreSQL 13+.
| {% if data.dependsonextensions %} | ||
| {% for ext in data.dependsonextensions %} | ||
|
|
||
| ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }} | ||
| DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }}; | ||
| {% endfor %} | ||
| {% endif %} |
There was a problem hiding this comment.
Guard DEPENDS ON EXTENSION block for unnamed indexes.
Like the 15_plus version, if data.name is empty (unnamed index) but dependsonextensions is populated, this will emit ALTER INDEX with a missing identifier, producing invalid SQL.
Apply this diff to guard the block:
-{% if data.dependsonextensions %}
+{% if data.dependsonextensions and data.name %}
{% for ext in data.dependsonextensions %}
ALTER INDEX {{ conn|qtIdent(data.schema, data.name) }}
DEPENDS ON EXTENSION {{ conn|qtIdent(ext) }};
{% endfor %}
{% endif %}🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/indexes/sql/13_plus/create.sql
around lines 27-33, the DEPENDS ON EXTENSION block runs even when data.name is
empty, producing an invalid ALTER INDEX with a missing identifier; update the
template condition to only render the ALTER INDEX ... DEPENDS ON EXTENSION loop
when data.name is present (non-empty) and data.dependsonextensions is populated,
so skip the entire block for unnamed indexes.
3 participants
Detail Info:- #6388
Summary by CodeRabbit