Add support of DEPENDS/NO DEPENDS ON EXTENSION for MATERIALIZED VIEW. #6390 #9327
Conversation
RohitBhati8269
force-pushed
the
GH-6390
branch
from
868ccab to
47206b9
Compare
...admin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json
Outdated
Show resolved
Hide resolved
RohitBhati8269
force-pushed
the
GH-6390
branch
from
47206b9 to
48a680d
Compare
WalkthroughAdds UI support and backend/template handling for materialized view dependencies on extensions: new multi-select UI field, initialization in Python, CREATE/UPDATE/PROPERTIES SQL templates for pg/ppas (13+/15+), and accompanying tests and docs. No behavioral change beyond managing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Browser UI
participant Frontend as JS Schema
participant Backend as Python
participant DB as PostgreSQL
User->>UI: Open materialized view dialog
UI->>Frontend: Request schema (includes extensionsList)
Frontend->>Backend: AJAX getNodeAjaxOptions('extension')
Backend->>DB: Query pg_extension
DB-->>Backend: Extensions list
Backend-->>Frontend: extensionsList (cached at server)
Frontend-->>UI: Render dependsonextensions multi-select
User->>UI: Select extensions and submit
UI->>Frontend: Send data { dependsonextensions: [...] }
Frontend->>Backend: Request SQL generation (create/update)
Backend->>Backend: Ensure dependsonextensions normalized ([] if null)
Backend->>DB: Execute generated DDL (CREATE/ALTER ... DEPENDS/NO DEPENDS)
DB-->>Backend: Result
Backend-->>Frontend: SQL/result
Frontend-->>UI: Display success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 10
🧹 Nitpick comments (4)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json (1)
335-336: Reorder fields to match convention used elsewhere in the file.Lines 335–336 reverse the field ordering found consistently in lines 32–33, 44–45, and 79–80 (
expected_sql_filebeforeexpected_msql_file). While JSON parsing is order-agnostic, maintaining consistent field ordering improves readability and reduces cognitive overhead during maintenance.Apply this diff to restore the consistent field order:
- "expected_msql_file": "create_mview_with_am_msql.sql", - "expected_sql_file": "create_mview_with_am.sql" + "expected_sql_file": "create_mview_with_am.sql", + "expected_msql_file": "create_mview_with_am_msql.sql"web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json (1)
335-336: Minor: Verify consistent field ordering.The addition of
"expected_sql_file"is placed after"expected_msql_file". While not incorrect, ensure this field ordering is consistent across all test scenarios in the file.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json (1)
337-341: Minor: Clean up trailing whitespace.There appears to be extra trailing content after the final test object. Consider removing the empty lines after line 337 for cleaner formatting.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
109-110: Clarify the purpose of the empty object.An empty object
{}is inserted at Line 110. If this is intentional for UI spacing or grouping, please add a comment explaining its purpose. Otherwise, consider removing it as it appears extraneous.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/materialized_view_definition.pngis excluded by!**/*.png
📒 Files selected for processing (36)
docs/en_US/materialized_view_dialog.rst(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/create_mview_with_am.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/create_mview_with_am.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json(2 hunks)web/regression/javascript/schema_ui_files/mview.ui.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js (1)
web/pgadmin/browser/static/js/node_ajax.js (4)
nodeObj(22-22)nodeObj(45-45)nodeObj(170-170)getNodeAjaxOptions(83-136)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py (3)
web/pgadmin/browser/__init__.py (1)
node_type(244-245)web/pgadmin/browser/collection.py (1)
node_type(137-138)web/pgadmin/browser/server_groups/__init__.py (1)
node_type(104-110)
⏰ 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). (19)
- GitHub Check: build-container
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 17)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 18)
- 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 (ubuntu-22.04, 15)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 14)
- GitHub Check: run-python-tests-pg (macos-latest, 15)
- GitHub Check: run-javascript-tests (ubuntu-22.04)
- GitHub Check: run-javascript-tests (macos-latest)
- GitHub Check: build-tarball
- GitHub Check: build-docs
- GitHub Check: build-python-package
🔇 Additional comments (32)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json (1)
3-62: Test framework executes scenarios sequentially but continues on failures; placeholder substitution works as designed.Both concerns in the original review are factually accurate:
Placeholder substitution ✓ Verified:
replace_placeholder_with_id()(lines 855–869) correctly strips angle brackets from<postgres_fdw>and looks up the stored ID inall_object_idsdict, which is populated bystore_object_ids()whenstore_object_id: truetriggers storage.Test execution order ✓ Verified: Scenarios execute sequentially (line 238 loop in
test_resql.py), and the extension must be created before the materialised view can depend on it.However, the framework design does not halt on failures—when
check_msql()returns False (line 270) orcreatefails (line 296), the loop continues to the next scenario (lines 277, 299:continuestatements). Subsequent scenarios proceed regardless, which is an intentional framework behavior for resilience, not a defect in the test JSON.The test chain is sound: if all scenarios succeed, placeholders resolve correctly; if earlier scenarios fail, later ones proceed and may fail due to missing prerequisites, which is acceptable cascading failure handling for a regression test suite.
docs/en_US/materialized_view_dialog.rst (1)
50-52: LGTM! Documentation is clear.The documentation accurately describes the new "Depends on extensions" field and explains the cascade behavior when an extension is dropped.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/test_mview.json (1)
275-276: LGTM! Test data update is consistent.Adding
expected_sql_filealongsideexpected_msql_fileproperly supports dual SQL verification for the materialized view test scenario.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json (2)
1058-1059: Same plpgsql extension concern applies here.Similar to the create test, verify that
plpgsqlis appropriate for testingDEPENDS ON EXTENSIONfunctionality.
92-92: Original review comment is incorrect—plpgsql is a valid extension for DEPENDS ON EXTENSION syntax.PostgreSQL supports ALTER MATERIALIZED VIEW ... DEPENDS ON EXTENSION, and PL/pgSQL is available as an extension (CREATE EXTENSION plpgsql), so the test data using
"dependsonextensions": ["plpgsql"]is valid and will not cause test failures. No changes required.Likely an incorrect or invalid review comment.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql (1)
45-51: LGTM! Extension dependency handling is correct.The template properly:
- Guards the block with a conditional check
- Uses
qtIdentfilter for SQL injection protection- Issues separate ALTER statements after CREATE
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/properties.sql (1)
20-25: LGTM! Dependency query is correct.The subquery properly retrieves extension dependencies by joining
pg_dependandpg_extensiontables. UsingDISTINCTandarray_aggensures clean results.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/properties.sql (1)
20-25: LGTM! Query matches 15_plus variant.The dependency query is identical to the 15_plus template, which is expected for version-specific template files.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql (1)
1-2: LGTM! Test covers NO DEPENDS ON EXTENSION.The test properly exercises the dependency removal path with a realistic extension (
postgres_fdw) and tests special character handling in identifiers.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/update.sql (1)
214-227: Original review comment incorrectly flags a non-issue; backend code guarantees type safety.The backend code (lines 1966-1967) explicitly initializes
data['dependsonextensions']to an empty list if missing with the patterndata.get('dependsonextensions') or [], ensuring it is always a list before template rendering. Line 217's concatenationold_exts + new_extswill not fail—both operands are guaranteed to be lists by the backend, making the concern about type safety unfounded.Likely an incorrect or invalid review comment.
web/regression/javascript/schema_ui_files/mview.ui.spec.js (1)
56-61: LGTM!The test case properly validates the presence and configuration of the new
dependsonextensionsfield, ensuring it's a multi-select control.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msql (1)
1-2: LGTM!The SQL correctly tests the
NO DEPENDS ON EXTENSIONsyntax for removing extension dependencies from a materialized view.web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js (1)
135-155: LGTM!The
extensionsListimplementation correctly fetches available extensions using the extension node's API, caches at the server level (appropriate for extensions), and transforms the data into the expected format for UI select controls.web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py (2)
1960-1967: LGTM!The defensive initialization ensures
dependsonextensionsis always a list for materialized view nodes, preventing potential type errors during SQL template rendering and data processing.
2038-2042: LGTM!Consistent initialization for new materialized view creation ensures
dependsonextensionsis always a list, matching the behavior in the update path.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json (1)
275-276: LGTM!Adding
expected_sql_filealongsideexpected_msql_fileprovides comprehensive test coverage for the access method scenario by validating both outputs.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.sql (1)
1-21: LGTM!The test SQL correctly demonstrates the complete workflow for creating a materialized view with extension dependencies:
- Creates the view with IF NOT EXISTS
- Sets ownership
- Adds dependencies on multiple extensions using separate ALTER statements
- Adds a comment
The use of special characters in the view name effectively tests proper escaping and quoting.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/create_mview_with_am.sql (1)
1-15: LGTM!The SQL correctly tests materialized view creation with an access method. The absence of
DEPENDS ON EXTENSIONstatements is appropriate since this test focuses specifically on access method functionality.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json (2)
3-62: LGTM!The new test scenarios provide comprehensive coverage of extension dependency management for materialized views:
- Creating views with multiple extension dependencies
- Altering to remove specific dependencies (tests NO DEPENDS ON)
- Proper cleanup of both views and extensions
The test flow is well-structured and validates the complete lifecycle.
335-336: LGTM!Adding
expected_sql_fileprovides comprehensive test validation for the access method scenario, consistent with similar test data improvements across other PostgreSQL versions.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/update.sql (1)
214-227: LGTM! Extension dependency reconciliation logic is correct.The implementation properly handles the synchronization of materialized view extension dependencies:
- Correctly identifies extensions to add (in
new_extsbut notold_exts)- Correctly identifies extensions to remove (in
old_extsbut notnew_exts)- Uses
uniquefilter to avoid duplicates when combining lists- Properly quotes identifiers using
qtIdent- Only executes when
new_extsis explicitly provided and differs fromold_extsweb/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/create_mview_with_am.sql (1)
1-15: LGTM! Test file structure is appropriate.This test file correctly creates a materialized view with a heavily-escaped identifier, sets ownership, and adds a comment. The structure is appropriate for testing materialized view creation with access methods in PostgreSQL 12+.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json (1)
3-62: LGTM! Test scenario structure is well-designed.The new test scenarios comprehensively cover the extension dependency feature:
- Creates the extension prerequisite
- Creates a materialized view with multiple extension dependencies
- Alters to remove one dependency
- Properly cleans up resources
The test flow correctly exercises the create/alter/delete lifecycle for materialized views with extension dependencies.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/create.sql (1)
45-51: LGTM! Extension dependency implementation is correct.The template properly generates
ALTER MATERIALIZED VIEW ... DEPENDS ON EXTENSIONstatements for each extension in thedependsonextensionslist. The implementation:
- Correctly checks for the presence of dependencies
- Properly quotes extension identifiers
- Uses appropriate ALTER syntax following the CREATE statement
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json (1)
3-62: LGTM! Test scenario structure matches 15_plus appropriately.The test scenarios are well-structured and provide comprehensive coverage of extension dependency features for PostgreSQL 13+. The test flow correctly exercises create/alter/delete operations with extension dependencies.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
111-124: LGTM!The field configuration is well-structured with appropriate multi-select controls, version gating (min_version: 130000), and mode support for create, edit, and properties views.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql (1)
1-21: LGTM!The test SQL comprehensively validates materialized view creation with extension dependencies. The use of special characters in the object name effectively tests identifier escaping, and the DEPENDS ON EXTENSION statements correctly validate the new feature.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/update.sql (1)
207-220: LGTM!The extension dependency reconciliation logic is correct. It properly handles additions (DEPENDS ON EXTENSION) and removals (NO DEPENDS ON EXTENSION) by comparing old and new extension lists, and only generates statements when there are actual changes.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/create.sql (1)
45-51: LGTM!The DEPENDS ON EXTENSION generation logic correctly iterates through the extension list and emits properly quoted ALTER MATERIALIZED VIEW statements.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json (1)
1-340: LGTM!The test scenarios provide comprehensive coverage of the new extension dependency feature, including creation with dependencies, alteration to add/remove dependencies, and interaction with other materialized view operations. The test data structure is well-organized and thorough.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/create.sql (1)
45-51: LGTM!The DEPENDS ON EXTENSION generation logic is identical to the pg/13_plus version and correctly emits properly quoted ALTER MATERIALIZED VIEW statements for each extension dependency.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql (1)
207-220: Extension dependency handling looks correct.The logic for managing extension dependencies is well-structured:
- Safely handles missing data with appropriate defaults (line 207-208)
- Uses
| uniquefilter to avoid duplicate processing (line 210)- Correctly distinguishes between additions and removals (lines 212-217)
- Consistently uses
view_schemaandview_namefallback variables (lines 213, 216)The SQL syntax for
DEPENDS ON EXTENSIONandNO DEPENDS ON EXTENSIONis correct for PostgreSQL 13+.
| ( | ||
| SELECT array_agg(DISTINCT e.extname) | ||
| FROM pg_depend d | ||
| JOIN pg_extension e ON d.refobjid = e.oid | ||
| WHERE d.objid = c.oid | ||
| ) AS dependsonextensions, |
There was a problem hiding this comment.
Add dependency type filter to ensure correctness.
The query retrieves all dependencies between the materialized view and extensions, but doesn't filter by dependency type. This could return incorrect results if the materialized view has other dependency relationships with extensions beyond explicit DEPENDS ON EXTENSION declarations.
Apply this diff to filter for extension dependencies only:
(
SELECT array_agg(DISTINCT e.extname)
FROM pg_depend d
JOIN pg_extension e ON d.refobjid = e.oid
- WHERE d.objid = c.oid
+ WHERE d.objid = c.oid
+ AND d.deptype = 'x'
) AS dependsonextensions,The deptype = 'x' filter ensures only DEPENDS ON EXTENSION relationships are captured, not other dependency types.
📝 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 = c.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 = c.oid | |
| AND d.deptype = 'x' | |
| ) AS dependsonextensions, |
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/properties.sql
around lines 20-25, the subquery collecting extension dependencies lacks a
dependency-type filter and may return non-extension relations; modify the WHERE
clause to include "AND d.deptype = 'x'" so only DEPENDS ON EXTENSION ('x')
relationships are aggregated, ensuring correct results.
| ( | ||
| SELECT array_agg(DISTINCT e.extname) | ||
| FROM pg_depend d | ||
| JOIN pg_extension e ON d.refobjid = e.oid | ||
| WHERE d.objid = c.oid | ||
| ) AS dependsonextensions, |
There was a problem hiding this comment.
Add dependency type filter to ensure correctness.
The query retrieves all dependencies between the materialized view and extensions, but doesn't filter by dependency type. This could return incorrect results if the materialized view has other dependency relationships with extensions beyond explicit DEPENDS ON EXTENSION declarations.
Apply this diff to filter for extension dependencies only:
(
SELECT array_agg(DISTINCT e.extname)
FROM pg_depend d
JOIN pg_extension e ON d.refobjid = e.oid
- WHERE d.objid = c.oid
+ WHERE d.objid = c.oid
+ AND d.deptype = 'x'
) AS dependsonextensions,The deptype = 'x' filter ensures only DEPENDS ON EXTENSION relationships are captured, not other dependency types.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/properties.sql
around lines 20 to 25, the SELECT that aggregates extensions from pg_depend
lacks a dependency type filter and can return non-extension relationships;
update the WHERE clause to include deptype = 'x' (DEPENDS ON EXTENSION) so only
extension dependencies are collected, ensuring the array_agg(DISTINCT e.extname)
reflects explicit extension dependencies only.
|
|
||
| {% endif %}{#-- toast_endif ends --#} | ||
| {% if data['vacuum_data']['changed']|length > 0 or data.autovacuum_enabled in ('t', 'f') or data.toast_autovacuum_enabled in ('t', 'f') %} | ||
| ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(data.schema, data.name) }} SET( |
There was a problem hiding this comment.
Use view_schema and view_name fallback variables for consistency.
Line 124 uses data.schema and data.name directly, which is inconsistent with the pattern established at lines 5-6 and used throughout the rest of the template (lines 68, 74, 80, 88, 93, 109, 142, 162, etc.). This could cause issues if the schema or name were changed earlier in the update operation, as data.schema or data.name might not be set.
Apply this diff:
-ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(data.schema, data.name) }} SET(
+ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(view_schema, view_name) }} SET(📝 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.
| ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(data.schema, data.name) }} SET( | |
| ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(view_schema, view_name) }} SET( |
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql
around line 124, the ALTER MATERIALIZED VIEW uses data.schema and data.name
directly which is inconsistent with the template's established pattern; change
those to the fallback variables view_schema and view_name (i.e., use {{
conn|qtIdent(view_schema, view_name) }}) so the statement uses the canonical
view identifiers used elsewhere in the template.
| CREATE MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| TABLESPACE pg_default | ||
| AS | ||
| SELECT 1 AS col1 | ||
| WITH NO DATA; | ||
|
|
||
| ALTER TABLE IF EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| OWNER TO postgres; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION plpgsql; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION postgres_fdw; | ||
|
|
||
| COMMENT ON MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| IS 'comment1'; No newline at end of file |
There was a problem hiding this comment.
Filename is misleading.
The filename create_mview_no_depends.msql suggests this file creates a materialized view without dependencies, but the content actually includes two DEPENDS ON EXTENSION statements (lines 10-14) for plpgsql and postgres_fdw.
Consider renaming to something like create_mview_with_depends.msql or create_mview_with_extensions.msql to better reflect the actual content and avoid confusion.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.msql
around lines 1 to 17, the file name claims "no_depends" but the SQL creates a
materialized view and includes DEPENDS ON EXTENSION statements for plpgsql and
postgres_fdw; rename the file to reflect the actual content (e.g.,
create_mview_with_depends.msql or create_mview_with_extensions.msql) and update
any references in tests or test manifests to use the new filename so names
remain consistent and descriptive.
| { | ||
| "type": "create", | ||
| "name": "Create Extension", | ||
| "endpoint": "NODE-extension.obj", | ||
| "sql_endpoint": "NODE-extension.sql_id", | ||
| "data": { | ||
| "name": "postgres_fdw", | ||
| "version": "", | ||
| "relocatable": true | ||
| }, | ||
| "store_object_id": true | ||
| }, | ||
| { | ||
| "type": "create", | ||
| "name": "Create Materialised Views with extensions.", | ||
| "endpoint": "NODE-mview.obj", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql", | ||
| "data": { | ||
| "spcname": "pg_default", | ||
| "schema": "public", | ||
| "owner": "postgres", | ||
| "datacl": [], | ||
| "seclabels": [], | ||
| "name": "testmview_$%{}[]()&*^!/@`#", | ||
| "comment": "comment1", | ||
| "definition": "SELECT 1 AS col1", | ||
| "dependsonextensions": ["plpgsql", "postgres_fdw"] | ||
| }, | ||
| "expected_sql_file": "create_mview_no_depends.sql", | ||
| "expected_msql_file": "create_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "alter", | ||
| "name": "Alter Materialised Views with NO DEPENDS.", | ||
| "endpoint": "NODE-mview.obj_id", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql_id", | ||
| "data": { | ||
| "dependsonextensions": ["plpgsql"] | ||
| }, | ||
| "expected_sql_file": "alter_mview_no_depends.sql", | ||
| "expected_msql_file": "alter_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "delete", | ||
| "name": "Drop Materialised Views", | ||
| "endpoint": "NODE-mview.delete_id", | ||
| "data": { | ||
| } | ||
| }, | ||
| { | ||
| "type": "delete", | ||
| "name": "Drop Extension", | ||
| "endpoint": "NODE-extension.delete", | ||
| "data": { | ||
| "ids": ["<postgres_fdw>"] | ||
| }, | ||
| "preprocess_data": true | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
New extension-dependency test scenarios are logically sound.
The test workflow—create extension, create materialized view with dependencies, alter to remove a dependency, and clean up—follows a proper test isolation pattern. Materialized view naming conflicts are avoided by deletions before subsequent creates.
To ensure the test data is complete and valid, please verify:
- That the referenced expected SQL files exist in the test fixtures directory
- That "plpgsql" and "postgres_fdw" are valid PostgreSQL extension identifiers
- That the preprocess_data substitution correctly replaces the
<postgres_fdw>placeholder with the stored object ID
🏁 Script executed:
#!/bin/bash
# Verify expected SQL files exist for new test scenarios
echo "=== Checking for expected SQL files ==="
find . -type f -name "create_mview_no_depends.*" -o -name "alter_mview_no_depends.*" | head -20
echo "=== Checking for create_mview_with_am SQL files ==="
find . -type f -name "create_mview_with_am.*" | head -20
echo "=== Verifying test data references ==="
echo "Expected files for new scenarios:"
rg -n "create_mview_no_depends|alter_mview_no_depends" . --type json | head -10Length of output: 2927
---
🏁 Script executed:
```shell
#!/bin/bash
# Check specifically for expected SQL files in pg/16_plus directory
echo "=== Checking for expected SQL files in pg/16_plus ==="
ls -la ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/*.sql ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/*.msql 2>&1 | grep -E "(create_mview_no_depends|alter_mview_no_depends)" || echo "Files not found in pg/16_plus"
echo -e "\n=== All SQL/MSQL files in pg/16_plus ==="
ls -1 ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/ 2>/dev/null | grep -E '\.(sql|msql)$' || echo "No .sql or .msql files found"
echo -e "\n=== Test fixture resolution mechanism ==="
rg -n "expected_sql_file|expected_msql_file" ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ --type json -A2 -B2 | head -40
Length of output: 5109
---
🏁 Script executed:
```shell
#!/bin/bash
# Find test runner/configuration to understand fixture resolution
echo "=== Searching for test configuration and fixture loading ==="
find ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "conftest.py" -o -name "pytest.ini" -o -name "*.py" -type f | head -20
echo -e "\n=== Searching for fixture loading logic ==="
rg -n "expected_sql_file|expected_msql_file" ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/ --type py -B3 -A3 | head -50
echo -e "\n=== Check if fixtures are inherited from parent directories ==="
grep -r "expected_sql_file\|expected_msql_file" ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/ --include="*.py" | head -10
Length of output: 1210
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine test utilities and fixture loading
echo "=== Checking utils.py for fixture loading ==="
cat ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/utils.py | head -100
echo -e "\n=== Searching for generic JSON test data loader ==="
find ./web/pgadmin -name "*.py" -type f -exec grep -l "expected_sql_file\|expected_msql_file" {} \; 2>/dev/null | head -10
echo -e "\n=== Search parent test infrastructure for JSON test handling ==="
rg -n "expected_sql_file|expected_msql_file" ./web/pgadmin --type py | head -20
Length of output: 4127
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where test_mview.json is actually used
echo "=== Searching for references to test_mview.json ==="
rg -n "test_mview\.json" ./web/pgadmin --type py | head -20
echo -e "\n=== Searching for generic JSON test frameworks in pgadmin ==="
find ./web/pgadmin -name "*.py" -type f -exec grep -l "json.*test\|test.*json" {} \; | grep -E "(runner|execute|framework|test)" | head -10
echo -e "\n=== Check if test_mview.json is used at all ==="
grep -r "test_mview" ./web/pgadmin --include="*.py" | head -10
echo -e "\n=== Look for API endpoint test frameworks ==="
find ./web/pgadmin -path "*test*" -name "*.py" -type f | xargs grep -l "endpoint.*test\|test.*endpoint" | head -10
Length of output: 3335
---
🏁 Script executed:
```shell
#!/bin/bash
# Compare test_mview.json across different PostgreSQL versions
echo "=== List test_mview.json files across versions ==="
find ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "test_mview.json" -type f
echo -e "\n=== Compare pg/13_plus and pg/16_plus test_mview.json structure ==="
echo "--- pg/13_plus test_mview.json first 50 lines ---"
head -50 ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json
echo -e "\n--- pg/16_plus test_mview.json first 50 lines ---"
head -50 ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json
echo -e "\n=== Check SQL files available in each version ==="
echo "Files in pg/13_plus:"
ls -1 ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/ | grep -E '\.(sql|msql)$' | sort
echo -e "\nFiles in pg/16_plus:"
ls -1 ./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/ | grep -E '\.(sql|msql)$' | sort
Length of output: 4702
Missing SQL fixture files will cause test failures.
The test scenarios in pg/16_plus/test_mview.json (lines 3-62) reference fixture files that do not exist in the pg/16_plus directory:
create_mview_no_depends.sqlcreate_mview_no_depends.msqlalter_mview_no_depends.sqlalter_mview_no_depends.msql
These files are present in pg/13_plus/ but missing from pg/16_plus/. The test data appears to have been copied from pg/13_plus without including the corresponding fixture files.
Required action: Copy the missing SQL fixture files from web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/ to web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/, or verify if PostgreSQL 16+ requires different SQL syntax and generate version-specific fixtures.
| -- View: public.testmview_$%{}[]()&*^!/@`# | ||
|
|
||
| -- DROP MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#"; | ||
|
|
||
| CREATE MATERIALIZED VIEW IF NOT EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| TABLESPACE pg_default | ||
| AS | ||
| SELECT 1 AS col1 | ||
| WITH NO DATA; | ||
|
|
||
| ALTER TABLE IF EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| OWNER TO enterprisedb; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION plpgsql; | ||
|
|
||
| COMMENT ON MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| IS 'comment1'; No newline at end of file |
There was a problem hiding this comment.
File content doesn't match the expected ALTER operation.
Similar to the PostgreSQL 13+ version, this file is referenced as the expected SQL output for "Alter Materialised Views with NO DEPENDS" but shows:
- A full CREATE MATERIALIZED VIEW statement instead of ALTER statements
- Only
DEPENDS ON EXTENSION plpgsql(line 14-15) with noNO DEPENDS ON EXTENSION postgres_fdwto remove the postgres_fdw dependency
The expected SQL for removing postgres_fdw from dependencies should include:
ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#"
NO DEPENDS ON EXTENSION postgres_fdw;🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql
lines 1-18, the file incorrectly contains a CREATE MATERIALIZED VIEW; replace
that with the proper ALTER statements expected for the test: remove the CREATE
MATERIALIZED VIEW ... AS ... WITH NO DATA block and instead emit ALTER
MATERIALIZED VIEW statements that remove the postgres_fdw dependency and
preserve ownership/comment changes — specifically add an ALTER MATERIALIZED VIEW
... NO DEPENDS ON EXTENSION postgres_fdw; (and keep or reapply any ALTER ...
OWNER TO and COMMENT ON MATERIALIZED VIEW lines as needed), and remove the
existing DEPENDS/CREATE duplication so the file only contains ALTER operations.
| CREATE MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| TABLESPACE pg_default | ||
| AS | ||
| SELECT 1 AS col1 | ||
| WITH NO DATA; | ||
|
|
||
| ALTER TABLE IF EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| OWNER TO enterprisedb; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION plpgsql; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION postgres_fdw; | ||
|
|
||
| COMMENT ON MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| IS 'comment1'; No newline at end of file |
There was a problem hiding this comment.
Misleading filename: file adds dependencies despite "no_depends" in name.
The filename create_mview_no_depends.msql suggests the test demonstrates creating a materialized view without dependencies, but lines 10-14 explicitly add extension dependencies using DEPENDS ON EXTENSION. This naming inconsistency could confuse maintainers.
Consider renaming to something like create_mview_with_depends.msql or create_mview_depends_on_extensions.msql to accurately reflect the test's purpose.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql
lines 1-17 the filename implies no dependencies but the script adds extension
dependencies via ALTER MATERIALIZED VIEW ... DEPENDS ON EXTENSION; rename the
file to reflect its behavior (e.g., create_mview_with_depends.msql or
create_mview_depends_on_extensions.msql) and update any test suite
references/metadata to use the new filename so the name accurately describes the
test.
| { | ||
| "type": "create", | ||
| "name": "Create Materialised Views with extensions.", | ||
| "endpoint": "NODE-mview.obj", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql", | ||
| "data": { | ||
| "spcname": "pg_default", | ||
| "schema": "public", | ||
| "owner": "enterprisedb", | ||
| "datacl": [], | ||
| "seclabels": [], | ||
| "name": "testmview_$%{}[]()&*^!/@`#", | ||
| "comment": "comment1", | ||
| "definition": "SELECT 1 AS col1", | ||
| "dependsonextensions": ["plpgsql", "postgres_fdw"] | ||
| }, | ||
| "expected_sql_file": "create_mview_no_depends.sql", | ||
| "expected_msql_file": "create_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "alter", | ||
| "name": "Alter Materialised Views with NO DEPENDS ON.", | ||
| "endpoint": "NODE-mview.obj_id", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql_id", | ||
| "data": { | ||
| "dependsonextensions": ["plpgsql"] | ||
| }, | ||
| "expected_sql_file": "alter_mview_no_depends.sql", | ||
| "expected_msql_file": "alter_mview_no_depends.msql" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that expected SQL/MSQL output files exist.
The test scenarios reference expected output files (create_mview_no_depends.sql, create_mview_no_depends.msql, alter_mview_no_depends.sql, alter_mview_no_depends.msql) that should exist in the test fixtures directory but are not visible in the provided context.
Run the following script to verify these files exist in the correct test directory:
🏁 Script executed:
#!/bin/bash
# Description: Verify expected output files exist for extension-related tests
# Find expected output files for extension tests
fd -e sql -e msql "create_mview_no_depends|alter_mview_no_depends" --type f
# Alternatively, check the directory structure
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" | grep -E "\.sql|\.msql"Length of output: 1335
🏁 Script executed:
#!/bin/bash
# Verify full directory structure of ppas/16_plus/
echo "=== Contents of ppas/16_plus/ directory ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" -type f -name "*.sql" -o -name "*.msql" | sort
echo ""
echo "=== All files in ppas/16_plus/ ==="
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/"Length of output: 1410
🏁 Script executed:
#!/bin/bash
# Verify complete directory structure of ppas/16_plus/
echo "=== All files in ppas/16_plus/ ==="
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/"
echo ""
echo "=== SQL/MSQL files in ppas/16_plus/ ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" -type f \( -name "*.sql" -o -name "*.msql" \) | sortLength of output: 1406
Missing expected SQL/MSQL fixture files for new test cases.
The test configuration in lines 15-46 references expected output files that do not exist in the ppas/16_plus/ directory:
create_mview_no_depends.sqlandcreate_mview_no_depends.msql(referenced lines 32-33)alter_mview_no_depends.sqlandalter_mview_no_depends.msql(referenced lines 44-45)
These files exist in ppas/13_plus/ and pg/13_plus/, but are completely absent from ppas/16_plus/. When the test framework attempts to validate test output against these expected files, the tests will fail with file-not-found errors. You must either create these fixture files in web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/ or update the test references to point to the correct location.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json
around lines 15-46, the test references four expected fixture files that are
missing in ppas/16_plus (create_mview_no_depends.sql/.msql and
alter_mview_no_depends.sql/.msql); fix by either (A) creating those four files
in
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/
(copy the corresponding fixtures from ppas/13_plus or pg/13_plus and verify
contents match the 16_plus test intent) or (B) update the "expected_sql_file"
and "expected_msql_file" fields to point to existing fixture paths, then commit
the new files or path changes and run the test suite to confirm resolution.
| "expected_msql_file": "create_mview_with_am_msql.sql", | ||
| "expected_sql_file": "create_mview_with_am.sql" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the addition of expected_sql_file to the last test case.
Line 336 adds expected_sql_file to the "Create Materialised Views with access method" test, whereas earlier versions may have only included expected_msql_file. Verify that:
- The file
create_mview_with_am.sqlexists in the test fixtures directory - This change is intentional (e.g., test coverage expansion) and not a merge artifact
- The corresponding MSQL file
create_mview_with_am_msql.sql(line 335) still exists
Run the following script to verify these files exist:
🏁 Script executed:
#!/bin/bash
# Description: Verify access method test fixture files exist
fd "create_mview_with_am" --type f -e sql -e msql
# Also check what files exist in the directory
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" | grep -i "mview_with_am"Length of output: 445
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Verify if fixture files exist in ppas/16_plus directory
# Check specifically for the fixture files in 16_plus directory
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" | grep "create_mview_with_am"
# Also list all SQL files in the 16_plus directory to see what's available
echo "=== All SQL files in ppas/16_plus ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" -name "*.sql" -o -name "*.msql"
Length of output: 291
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Locate test_mview.json file in the codebase
find . -name "test_mview.json" -type f
# Also check what directories exist in ppas
echo "=== Directories under ppas ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/" -maxdepth 1 -type d | sort
Length of output: 1553
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check test_mview.json content and verify fixture files
# Show lines 335-340 from the test file
echo "=== Lines 335-340 from test_mview.json ==="
sed -n '335,340p' "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json"
# List all fixture files in ppas/16_plus directory
echo "=== Fixture files in ppas/16_plus ==="
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/" | grep -E "\.(sql|msql)$"
# Specifically check for the two fixture files mentioned
echo "=== Checking for specific fixture files ==="
test -f "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/create_mview_with_am.sql" && echo "✓ create_mview_with_am.sql exists" || echo "✗ create_mview_with_am.sql NOT FOUND"
test -f "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/create_mview_with_am_msql.sql" && echo "✓ create_mview_with_am_msql.sql exists" || echo "✗ create_mview_with_am_msql.sql NOT FOUND"
Length of output: 770
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check if fixture files should be in ppas/16_plus
# Show what fixture files exist across different ppas versions
echo "=== create_mview_with_am files across ppas versions ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/" -name "create_mview_with_am*"
# Check if they're in pg/16_plus
echo "=== create_mview_with_am files in pg versions ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/" -name "create_mview_with_am*"
Length of output: 551
Missing fixture files for test case in ppas/16_plus.
The test case at lines 335-336 references two fixture files that do not exist in the ppas/16_plus directory:
create_mview_with_am.sqlcreate_mview_with_am_msql.sql
These files exist only in ppas/12_plus and pg/12_plus directories. The test will fail when run because it cannot locate these fixtures. Either create these files in ppas/16_plus, copy them from the 12_plus versions if they apply to 16_plus, or remove the test case if it shouldn't run for this version.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json
around lines 335-336, the test references two fixture files
("create_mview_with_am.sql" and "create_mview_with_am_msql.sql") that are
missing from the ppas/16_plus directory; fix by adding those fixture files to
ppas/16_plus (copy them from ppas/12_plus or pg/12_plus if they are still valid
for 16_plus) or, if the test should not run for 16_plus, remove or disable this
test case entry so the suite no longer fails due to missing fixtures.
48a680d to
fd91208
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json (2)
15-46: Missing test fixture files will cause test failures.The test scenarios reference expected output files that do not exist in the ppas/16_plus directory:
create_mview_no_depends.sqlandcreate_mview_no_depends.msql(lines 32-33)alter_mview_no_depends.sqlandalter_mview_no_depends.msql(lines 44-45)These files exist in ppas/13_plus and pg/13_plus directories but are missing from ppas/16_plus. You must either:
- Create these fixture files in
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/- Copy and adapt them from ppas/13_plus if they apply to 16_plus
- Update the test configuration to point to existing fixture paths
335-336: Missing access method test fixture files.The test case references two fixture files that do not exist in ppas/16_plus:
create_mview_with_am.sql(line 336)create_mview_with_am_msql.sql(line 335)These files only exist in ppas/12_plus and pg/12_plus directories. Either create these files in ppas/16_plus or remove this test case if it shouldn't run for version 16+.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql (1)
1-18: File contains CREATE statement instead of ALTER statement.This file is referenced as the expected output for "Alter Materialised Views with NO DEPENDS ON" test but contains:
- A full
CREATE MATERIALIZED VIEWstatement (lines 5-9) instead ofALTERstatements- Only
DEPENDS ON EXTENSION plpgsql(lines 14-15) with noNO DEPENDS ON EXTENSION postgres_fdwto remove the postgres_fdw dependencyBased on the test scenario (changing dependsonextensions from ["plpgsql", "postgres_fdw"] to ["plpgsql"]), the expected SQL should include:
ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" NO DEPENDS ON EXTENSION postgres_fdw;Replace the CREATE statement with proper ALTER statements that remove the postgres_fdw dependency.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.sql (1)
1-21: Past review concern about misleading filename still applies.The filename
create_mview_no_depends.sqlsuggests no dependencies, but lines 14-18 explicitly declareDEPENDS ON EXTENSIONfor bothplpgsqlandpostgres_fdw. This naming inconsistency was previously flagged and should be addressed.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.msql (1)
1-17: Misleading filename persists.As noted in the past review, the filename
create_mview_no_depends.msqlcontradicts the content which includesDEPENDS ON EXTENSIONstatements (lines 10-14). Consider renaming to reflect the actual behavior.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json (1)
3-62: Missing fixture files issue from past review remains.The past review identified that the expected SQL fixture files (
create_mview_no_depends.sql,create_mview_no_depends.msql,alter_mview_no_depends.sql,alter_mview_no_depends.msql) referenced in lines 32-33 and 44-45 may be missing from thepg/16_plusdirectory. Verify these files exist or copy them frompg/13_plusif needed.The test scenario structure itself is well-designed, properly exercising the full lifecycle of extension dependencies.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/properties.sql (1)
20-25: Missing dependency type filter (past review issue).The past review correctly identified that the query lacks a
deptype = 'x'filter. Without this filter, the query may return all types of dependencies between the materialized view and extensions, not just explicitDEPENDS ON EXTENSIONdeclarations.Apply the suggested filter to ensure only extension dependencies are captured:
WHERE d.objid = c.oid AND d.deptype = 'x'web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql (1)
124-124: Useview_schemaandview_namefor consistency (duplicate of past review).Line 124 should use
view_schemaandview_nameinstead ofdata.schemaanddata.nameto match the pattern used throughout the template (lines 5-6, 68, 74, 80, 88, 93, 109, 142, 162, etc.).web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql (1)
1-17: Misleading filename (duplicate of past review).The filename
create_mview_no_depends.msqlsuggests no dependencies, but lines 10-14 add extension dependencies usingDEPENDS ON EXTENSION. This was already flagged in a previous review and should be renamed to accurately reflect its behavior (e.g.,create_mview_with_depends.msql).
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql (1)
45-57: LGTM! Extension dependency and comment handling are correct.The DEPENDS ON EXTENSION statements are properly emitted after the CREATE and ownership assignment, and identifiers are correctly quoted.
Note: Significant code duplication with pg/15_plus and ppas/15_plus create.sql templates.
These three templates (ppas/13_plus, ppas/15_plus, and pg/15_plus) are identical. While this may be intentional for version-specific paths in pgAdmin's template structure, consider whether consolidation or shared macros could reduce maintenance burden.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/materialized_view_definition.pngis excluded by!**/*.png
📒 Files selected for processing (36)
docs/en_US/materialized_view_dialog.rst(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/create_mview_with_am.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/create_mview_with_am.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json(2 hunks)web/regression/javascript/schema_ui_files/mview.ui.spec.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json
🚧 Files skipped from review as they are similar to previous changes (11)
- web/regression/javascript/schema_ui_files/mview.ui.spec.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/create_mview_with_am.sql
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py (3)
web/pgadmin/browser/server_groups/servers/__init__.py (1)
node_type(139-140)web/pgadmin/browser/collection.py (1)
node_type(137-138)web/pgadmin/browser/server_groups/__init__.py (1)
node_type(104-110)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
web/regression/javascript/fake_gettext.js (1)
gettext(40-42)
🔇 Additional comments (16)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/create_mview_with_am.sql (1)
1-15: LGTM!The SQL syntax is correct for PPAS 12+. The test appropriately covers materialized view creation with special characters in the name and explicit tablespace specification, which provides good edge-case coverage for the test suite.
docs/en_US/materialized_view_dialog.rst (1)
50-52: LGTM! Documentation is clear and accurate.The documentation correctly describes the new "Depends on extensions" feature and its behavior. The text is consistent with other field descriptions in the file.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/update.sql (1)
207-220: LGTM! Extension dependency reconciliation logic is correct.The template properly handles adding and removing extension dependencies by:
- Comparing old and new extension lists (lines 207-209)
- Iterating over unique extensions from both lists (line 210)
- Emitting
DEPENDS ON EXTENSIONfor additions (lines 212-214)- Emitting
NO DEPENDS ON EXTENSIONfor removals (lines 215-217)The implementation correctly uses identifier quoting and follows established patterns in the template.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py (2)
1960-1967: LGTM! Proper initialization of dependsonextensions.The code correctly ensures
dependsonextensionsis always a list (never None) for materialized view nodes when updating existing views. This prevents potential None-related errors in SQL templates and ensures consistent data structure.
2038-2042: LGTM! Proper initialization for new materialized views.The code correctly initializes
dependsonextensionsas an empty list when creating new materialized views if it's not provided or is None. This ensures consistency with the data structure expected by SQL templates.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/test_mview.json (1)
275-276: Fixture files verified successfully.Both SQL fixture files referenced in the test configuration exist in the pg/12_plus directory:
create_mview_with_am.sql✓create_mview_with_am_msql.sql✓The test changes are properly supported by the fixture files.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/update.sql (1)
214-227: LGTM! Extension dependency reconciliation is well-implemented.The reconciliation logic correctly handles both additions and removals of extension dependencies by:
- Creating a union of old and new extension lists
- Generating
DEPENDS ON EXTENSIONfor additions- Generating
NO DEPENDS ON EXTENSIONfor removals- Properly escaping identifiers with
conn|qtIdentweb/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/update.sql (1)
214-228: LGTM! Consistent with ppas variant.The extension dependency reconciliation logic is correctly implemented and matches the ppas/15_plus variant, ensuring consistency across database variants.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
109-124: LGTM! UI field is properly configured.The
dependsonextensionsfield is well-implemented with:
- Appropriate multi-select controls (multiple, allowClear, allowSelectAll)
- Correct version gating (
min_version: 130000) for PostgreSQL 13+ support- Proper placement in the Definition group
- All necessary modes (create, edit, properties)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql (2)
1-14: LGTM! CREATE statement and USING clause logic are correct.The template properly handles identifier quoting, optional IF NOT EXISTS clause, and conditionally emits the USING clause when a non-default access method is specified.
15-44: LGTM! Storage parameters and definition handling are correct.The namespace pattern for comma management in the WITH clause is properly implemented, and the ownership assignment correctly follows the CREATE statement.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql (1)
207-220: LGTM! Extension dependency reconciliation logic is correct.The implementation properly handles:
- Null safety with fallback to empty list
- Checking for explicit presence of dependsonextensions in data
- Differentiating between additions (DEPENDS ON) and removals (NO DEPENDS ON)
- Using the union of both lists to cover all cases
The logic correctly uses
view_schemaandview_namevariables for consistency.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/create.sql (1)
1-57: LGTM! Template is correct and identical to ppas/13_plus.This template is functionally identical to
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql. The code duplication was already noted in the review of that file.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/create.sql (1)
1-57: LGTM! Template is correct and identical to ppas versions.This template is functionally identical to the ppas/13_plus and ppas/15_plus create.sql templates. The code duplication issue was noted in earlier reviews.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json (1)
1-340: LGTM! Comprehensive test data for materialized view extension dependencies.The test scenarios properly cover:
- Creating materialized views with extension dependencies (lines 15-34)
- Altering to remove dependencies using NO DEPENDS ON (lines 35-46)
- Expected SQL/MSQL file references for validation
- Various other materialized view operations (privileges, definition changes, parameters)
The test data structure is well-formed and aligns with the new dependsonextensions feature.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/properties.sql (1)
20-25: LGTM! Extension dependency query is correct.The subquery properly fetches extension dependencies by:
- Joining
pg_dependandpg_extensiontables- Filtering for dependencies where the materialized view is the dependent object
- Using
array_agg(DISTINCT ...)to collect extension names and prevent duplicatesThis aligns with PostgreSQL's dependency tracking system.
| { | ||
| "type": "create", | ||
| "name": "Create Extension", | ||
| "endpoint": "NODE-extension.obj", | ||
| "sql_endpoint": "NODE-extension.sql_id", | ||
| "data": { | ||
| "name": "postgres_fdw", | ||
| "version": "", | ||
| "relocatable": true | ||
| }, | ||
| "store_object_id": true | ||
| }, | ||
| { | ||
| "type": "create", | ||
| "name": "Create Materialised Views with extensions.", | ||
| "endpoint": "NODE-mview.obj", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql", | ||
| "data": { | ||
| "spcname": "pg_default", | ||
| "schema": "public", | ||
| "owner": "postgres", | ||
| "datacl": [], | ||
| "seclabels": [], | ||
| "name": "testmview_$%{}[]()&*^!/@`#", | ||
| "comment": "comment1", | ||
| "definition": "SELECT 1 AS col1", | ||
| "dependsonextensions": ["plpgsql", "postgres_fdw"] | ||
| }, | ||
| "expected_sql_file": "create_mview_no_depends.sql", | ||
| "expected_msql_file": "create_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "alter", | ||
| "name": "Alter Materialised Views with NO DEPENDS.", | ||
| "endpoint": "NODE-mview.obj_id", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql_id", | ||
| "data": { | ||
| "dependsonextensions": ["plpgsql"] | ||
| }, | ||
| "expected_sql_file": "alter_mview_no_depends.sql", | ||
| "expected_msql_file": "alter_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "delete", | ||
| "name": "Drop Materialised Views", | ||
| "endpoint": "NODE-mview.delete_id", | ||
| "data": { | ||
| } | ||
| }, | ||
| { | ||
| "type": "delete", | ||
| "name": "Drop Extension", | ||
| "endpoint": "NODE-extension.delete", | ||
| "data": { | ||
| "ids": ["<postgres_fdw>"] | ||
| }, | ||
| "preprocess_data": true | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify all expected fixture files exist.
The new test scenarios reference multiple expected output files. Ensure all fixture files exist in the pg/15_plus test directory to prevent test failures.
Run the following script to verify fixture files exist:
🏁 Script executed:
#!/bin/bash
# Description: Verify all expected fixture files exist in pg/15_plus directory
test_dir="web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus"
missing_files=()
# Check for extension dependency test fixtures
for file in "create_mview_no_depends.sql" "create_mview_no_depends.msql" "alter_mview_no_depends.sql" "alter_mview_no_depends.msql"; do
if [ ! -f "$test_dir/$file" ]; then
missing_files+=("$file")
fi
done
if [ ${#missing_files[@]} -eq 0 ]; then
echo "✓ All extension dependency fixture files exist"
else
echo "✗ Missing fixture files in pg/15_plus:"
printf '%s\n' "${missing_files[@]}"
exit 1
fiLength of output: 150
🏁 Script executed:
#!/bin/bash
# Check if similar fixture files exist in other test directories
echo "Searching for mview fixture files in other directories..."
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "*mview*.sql" -o -name "*mview*.msql" 2>/dev/null | head -20Length of output: 2331
🏁 Script executed:
#!/bin/bash
# Check for create_mview_no_depends files specifically
echo "Searching for create_mview_no_depends files..."
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "create_mview_no_depends*" 2>/dev/null
echo ""
echo "Listing all fixtures in pg/13_plus for comparison..."
ls -la web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/ 2>/dev/null | grep -E "\.(sql|msql)$"
echo ""
echo "Listing all fixtures in pg/15_plus..."
ls -la web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/ 2>/dev/null | grep -E "\.(sql|msql)$"Length of output: 1426
Create missing fixture files for pg/15_plus mview tests.
The test JSON references four fixture files that do not exist in the pg/15_plus directory:
create_mview_no_depends.sqlcreate_mview_no_depends.msqlalter_mview_no_depends.sqlalter_mview_no_depends.msql
These files exist in pg/13_plus but are missing from pg/15_plus. Tests cannot execute without these fixtures and will fail.
Create the missing fixture files in web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/, either by copying from pg/13_plus (if compatible) or by generating the appropriate SQL for PostgreSQL 15+.
| "expected_msql_file": "create_mview_with_am_msql.sql", | ||
| "expected_sql_file": "create_mview_with_am.sql" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify access method fixture files exist.
The test includes expected_sql_file for the "Create with access method" scenario. Ensure both fixture files exist in pg/15_plus.
Run the following script:
🏁 Script executed:
#!/bin/bash
# Description: Verify access method fixture files exist in pg/15_plus
test_dir="web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus"
for file in "create_mview_with_am.sql" "create_mview_with_am_msql.sql"; do
if [ ! -f "$test_dir/$file" ]; then
echo "✗ Missing: $file"
exit 1
fi
done
echo "✓ All access method fixture files exist"Length of output: 35
🏁 Script executed:
#!/bin/bash
# List all fixture files in pg/15_plus directory
test_dir="web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus"
echo "Files in $test_dir:"
ls -la "$test_dir" | grep -E "\.(sql|json)$"
echo ""
echo "Searching for 'create_mview_with_am' references in codebase:"
rg "create_mview_with_am" -nLength of output: 3406
🏁 Script executed:
#!/bin/bash
# Search for fixture files across all version directories
echo "Searching for create_mview_with_am.sql in all version directories:"
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "create_mview_with_am*.sql" 2>/dev/null
echo ""
echo "Total occurrences found:"
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "create_mview_with_am*.sql" 2>/dev/null | wc -lLength of output: 540
Create missing fixture files for access method test scenario in pg/15_plus.
The test references create_mview_with_am.sql and create_mview_with_am_msql.sql at lines 335-336, but these fixture files do not exist in the pg/15_plus directory. The files exist only in pg/12_plus. Create these fixture files in web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/ or remove the test reference if this scenario is not applicable for pg/15_plus.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json
around lines 335-336, the test references expected_msql_file
"create_mview_with_am_msql.sql" and expected_sql_file "create_mview_with_am.sql"
which are missing for pg/15_plus; either (preferred) copy the two fixture files
from
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/
into the pg/15_plus/ directory preserving filenames and adjust any
PostgreSQL-version-specific SQL if needed, or if the access-method scenario is
not valid for pg/15_plus, remove the two expected_* entries from test_mview.json
so the test no longer references non-existent fixtures.
| -- View: public.testmview_$%{}[]()&*^!/@`# | ||
|
|
||
| -- DROP MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#"; | ||
|
|
||
| CREATE MATERIALIZED VIEW IF NOT EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| TABLESPACE pg_default | ||
| AS | ||
| SELECT 1 AS col1 | ||
| WITH NO DATA; | ||
|
|
||
| ALTER TABLE IF EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| OWNER TO enterprisedb; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION plpgsql; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION postgres_fdw; | ||
|
|
||
| COMMENT ON MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| IS 'comment1'; No newline at end of file |
There was a problem hiding this comment.
Filename is misleading.
Similar to other test files in this PR, the filename create_mview_no_depends.sql suggests no dependencies, but lines 14-18 contain DEPENDS ON EXTENSION statements. Consider renaming to accurately reflect the content, such as create_mview_with_depends.sql.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql
lines 1-21 the filename implies the materialized view has no dependencies, but
the file contains DEPENDS ON EXTENSION statements for plpgsql and postgres_fdw;
rename the file to reflect its contents (for example
create_mview_with_depends.sql) and update any test suite references or include
paths to the new filename to keep names consistent with behavior.
| { | ||
| "type": "create", | ||
| "name": "Create Extension", | ||
| "endpoint": "NODE-extension.obj", | ||
| "sql_endpoint": "NODE-extension.sql_id", | ||
| "data": { | ||
| "name": "postgres_fdw", | ||
| "version": "", | ||
| "relocatable": true | ||
| }, | ||
| "store_object_id": true | ||
| }, | ||
| { | ||
| "type": "create", | ||
| "name": "Create Materialised Views with extensions.", | ||
| "endpoint": "NODE-mview.obj", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql", | ||
| "data": { | ||
| "spcname": "pg_default", | ||
| "schema": "public", | ||
| "owner": "enterprisedb", | ||
| "datacl": [], | ||
| "seclabels": [], | ||
| "name": "testmview_$%{}[]()&*^!/@`#", | ||
| "comment": "comment1", | ||
| "definition": "SELECT 1 AS col1", | ||
| "dependsonextensions": ["plpgsql", "postgres_fdw"] | ||
| }, | ||
| "expected_sql_file": "create_mview_no_depends.sql", | ||
| "expected_msql_file": "create_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "alter", | ||
| "name": "Alter Materialised Views with NO DEPENDS ON.", | ||
| "endpoint": "NODE-mview.obj_id", | ||
| "sql_endpoint": "NODE-mview.sql_id", | ||
| "msql_endpoint": "NODE-mview.msql_id", | ||
| "data": { | ||
| "dependsonextensions": ["plpgsql"] | ||
| }, | ||
| "expected_sql_file": "alter_mview_no_depends.sql", | ||
| "expected_msql_file": "alter_mview_no_depends.msql" | ||
| }, | ||
| { | ||
| "type": "delete", | ||
| "name": "Drop Materialised Views", | ||
| "endpoint": "NODE-mview.delete_id", | ||
| "data": { | ||
| } | ||
| }, | ||
| { | ||
| "type": "delete", | ||
| "name": "Drop Extension", | ||
| "endpoint": "NODE-extension.delete", | ||
| "data": { | ||
| "ids": ["<postgres_fdw>"] | ||
| }, | ||
| "preprocess_data": true | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify extension dependency fixture files exist.
The new test scenarios reference expected output files. Ensure all required fixture files are present in ppas/15_plus to prevent test failures.
Run the following script:
🏁 Script executed:
#!/bin/bash
# Description: Verify extension dependency fixture files exist in ppas/15_plus
test_dir="web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus"
required_files=(
"create_mview_no_depends.sql"
"create_mview_no_depends.msql"
"alter_mview_no_depends.sql"
"alter_mview_no_depends.msql"
)
missing=()
for file in "${required_files[@]}"; do
if [ ! -f "$test_dir/$file" ]; then
missing+=("$file")
fi
done
if [ ${#missing[@]} -eq 0 ]; then
echo "✓ All extension dependency fixtures exist in ppas/15_plus"
else
echo "✗ Missing fixtures in ppas/15_plus:"
printf ' - %s\n' "${missing[@]}"
exit 1
fiLength of output: 163
🏁 Script executed:
#!/bin/bash
# Search for the missing fixture files in other test directories
echo "=== Searching for create_mview_no_depends.* in views tests ==="
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "create_mview_no_depends.*" 2>/dev/null
echo ""
echo "=== Searching for alter_mview_no_depends.* in views tests ==="
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "alter_mview_no_depends.*" 2>/dev/null
echo ""
echo "=== Listing all fixture files in ppas/16_plus for reference ==="
ls -la web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/ 2>/dev/null | head -20
echo ""
echo "=== Listing all fixture files in pg/15_plus for reference ==="
ls -la web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/ 2>/dev/null | head -20Length of output: 2596
🏁 Script executed:
#!/bin/bash
# Search for the missing fixture files in other test directories
echo "=== Searching for create_mview_no_depends.* in views tests ==="
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "create_mview_no_depends.*" 2>/dev/null
echo ""
echo "=== Searching for alter_mview_no_depends.* in views tests ==="
find web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -name "alter_mview_no_depends.*" 2>/dev/null
echo ""
echo "=== Listing fixture files in ppas/16_plus for reference ==="
ls web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/ 2>/dev/null
echo ""
echo "=== Listing fixture files in pg/15_plus for reference ==="
ls web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/ 2>/dev/nullLength of output: 1510
🏁 Script executed:
#!/bin/bash
# Check if ppas/13_plus test_mview.json has extension dependency tests
echo "=== Checking ppas/13_plus test_mview.json for extension tests ==="
grep -n "dependsonextensions\|create_mview_no_depends\|alter_mview_no_depends" \
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json | head -20
echo ""
echo "=== Checking ppas/16_plus test_mview.json for extension tests ==="
grep -n "dependsonextensions\|create_mview_no_depends\|alter_mview_no_depends" \
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json | head -20
echo ""
echo "=== File size comparison ==="
wc -l web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/*/test_mview.jsonLength of output: 1425
🏁 Script executed:
#!/bin/bash
# Check content of ppas/13_plus fixture files to verify if they should be copied to ppas/15_plus
echo "=== Content of ppas/13_plus/create_mview_no_depends.sql ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql
echo ""
echo "=== Content of ppas/13_plus/create_mview_no_depends.msql ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql
echo ""
echo "=== Content of ppas/13_plus/alter_mview_no_depends.sql ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql
echo ""
echo "=== Content of ppas/13_plus/alter_mview_no_depends.msql ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msqlLength of output: 1936
Create missing extension dependency fixture files in ppas/15_plus.
The test scenarios reference four fixture files that do not exist, causing test failures. Copy the following files from ppas/13_plus to ppas/15_plus:
create_mview_no_depends.sqlcreate_mview_no_depends.msqlalter_mview_no_depends.sqlalter_mview_no_depends.msql
Note: The same fixture files are also missing from ppas/16_plus/, which has an identical test configuration.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json
around lines 3-62, the test references four fixture files
(create_mview_no_depends.sql, create_mview_no_depends.msql,
alter_mview_no_depends.sql, alter_mview_no_depends.msql) that are missing; fix
it by copying those four files from
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/
into the ppas/15_plus/ directory (preserving filenames and contents) and also
add the same four files into ppas/16_plus/ where an identical test configuration
exists so the tests can find the expected fixtures.
| "expected_msql_file": "create_mview_with_am_msql.sql", | ||
| "expected_sql_file": "create_mview_with_am.sql" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify access method fixture files exist.
Ensure the access method fixture files are present in ppas/15_plus.
Run the following script:
🏁 Script executed:
#!/bin/bash
# Description: Verify access method fixtures exist in ppas/15_plus
test_dir="web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus"
for file in "create_mview_with_am.sql" "create_mview_with_am_msql.sql"; do
if [ ! -f "$test_dir/$file" ]; then
echo "✗ Missing: $file in ppas/15_plus"
exit 1
fi
done
echo "✓ Access method fixtures exist in ppas/15_plus"Length of output: 51
The web search did not provide relevant information about pgAdmin test fixture structure. Let me search the repository directly for these fixture files and examine similar test patterns.
🏁 Script executed:
#!/bin/bash
# Search for fixture files in the repository
# Find any sql files in the views test directory
echo "=== Searching for .sql files in views tests directory ==="
find "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests" -name "*.sql" -type f | head -20
echo ""
echo "=== Searching for create_mview_with_am files anywhere ==="
find . -name "*create_mview_with_am*" -type f
echo ""
echo "=== List contents of ppas/15_plus directory ==="
ls -la "web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/"Length of output: 875
🏁 Script executed:
#!/bin/bash
# Check if test_mview.json exists and examine the referenced lines
echo "=== Checking test_mview.json existence ==="
if [ -f "./git/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json" ]; then
echo "✓ test_mview.json exists in ppas/15_plus"
echo ""
echo "=== Content around lines 335-336 ==="
sed -n '330,340p' "./git/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json"
else
echo "✗ test_mview.json does not exist in ppas/15_plus"
fi
echo ""
echo "=== Checking if ppas/15_plus directory exists ==="
ls -ld "./git/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus" 2>&1
echo ""
echo "=== Files in ppas/15_plus if it exists ==="
ls -la "./git/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/" 2>&1 || echo "(directory not found)"
echo ""
echo "=== Compare with ppas/12_plus ==="
ls -la "./git/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/" | head -10Length of output: 659
🏁 Script executed:
#!/bin/bash
# Find the actual structure and locate test_mview.json
echo "=== Find all ppas test directories ==="
find . -path "*/ppas/*/test_mview.json" -type f 2>/dev/null
echo ""
echo "=== Find all ppas directories under views/tests ==="
find . -path "*/views/tests/ppas" -type d 2>/dev/null
echo ""
echo "=== Git status for new/modified files ==="
cd ./git 2>/dev/null && git status --short | grep -E "(ppas|15_plus|test_mview)" || echo "No matches in git status"
echo ""
echo "=== List all ppas version directories ==="
find ./git/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests -path "*/ppas/*" -type d | sortLength of output: 909
🏁 Script executed:
#!/bin/bash
# Verify fixture files with correct path
test_dir="./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus"
echo "=== Files in ppas/15_plus ==="
ls -la "$test_dir/" | head -20
echo ""
echo "=== Checking for fixture files ==="
for file in "create_mview_with_am.sql" "create_mview_with_am_msql.sql"; do
if [ -f "$test_dir/$file" ]; then
echo "✓ Found: $file"
else
echo "✗ Missing: $file"
fi
done
echo ""
echo "=== Content around lines 335-336 in test_mview.json ==="
sed -n '330,340p' "$test_dir/test_mview.json"
echo ""
echo "=== Compare: fixtures in ppas/12_plus ==="
ls -la "./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/" | grep "\.sql$"Length of output: 1287
Add missing fixture files for access method tests in ppas/15_plus.
The fixture files create_mview_with_am.sql and create_mview_with_am_msql.sql referenced at lines 335-336 in test_mview.json are missing from the ppas/15_plus directory. These files exist in ppas/12_plus and must be added to ppas/15_plus to prevent test failures.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json
around lines 335-336, the referenced fixture files create_mview_with_am.sql and
create_mview_with_am_msql.sql are missing; copy these two files from the
ppas/12_plus directory into ppas/15_plus (preserving filenames and file
contents), ensure their file permissions match other fixtures, and commit them
so the test can locate expected_msql_file and expected_sql_file.
RohitBhati8269
force-pushed
the
GH-6390
branch
from
fd91208 to
b8add02
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json (1)
3-62:⚠️ Duplicate issue: Missing SQL fixture files will cause test failures.This concern was previously flagged in code review. The new test scenarios (lines 3–62) reference fixture files that do not exist in
pg/16_plus/:
create_mview_no_depends.sqlcreate_mview_no_depends.msqlalter_mview_no_depends.sqlalter_mview_no_depends.msqlThese files exist in
pg/13_plus/but were not copied topg/16_plus/. The test framework will fail when trying to load these fixtures.Required action: Verify that the missing fixture files have been created or copied to the
pg/16_plus/directory. If they do not exist, copy them frompg/13_plus/or generate version-specific fixtures if PostgreSQL 16+ requires different SQL syntax.#!/bin/bash # Verify that the referenced fixture files exist in pg/16_plus/ echo "=== Checking for required fixture files ===" FIXTURE_FILES=( "create_mview_no_depends.sql" "create_mview_no_depends.msql" "alter_mview_no_depends.sql" "alter_mview_no_depends.msql" ) for file in "${FIXTURE_FILES[@]}"; do if [ -f "./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/$file" ]; then echo "✓ Found: $file" else echo "✗ MISSING: $file" fi done echo -e "\n=== All SQL/MSQL files in pg/16_plus/ ===" ls -1 "./web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/" | grep -E '\.(sql|msql)$' | sortweb/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json (2)
3-62: Past review identified missing fixture files.The test scenarios reference four fixture files that do not exist in ppas/15_plus, which will cause test failures. This issue has already been raised in previous reviews.
335-336: Past review identified missing access method fixture files.The fixture files
create_mview_with_am.sqlandcreate_mview_with_am_msql.sqlare missing from ppas/15_plus. This issue has already been raised in previous reviews.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql (1)
1-17: Misleading filename: file adds dependencies despite "no_depends" in name.The past review correctly identified that the filename
create_mview_no_depends.msqlis misleading. The file actually adds extension dependencies viaDEPENDS ON EXTENSIONstatements (lines 10-14), contradicting the "no_depends" in the filename.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/properties.sql (1)
20-25: Add dependency type filter to ensure correctness.The past review correctly identified that the query lacks a dependency type filter. Without
deptype = 'x', this query may return non-extension dependencies, leading to incorrect results.Apply the suggested fix from the previous review:
( SELECT array_agg(DISTINCT e.extname) FROM pg_depend d JOIN pg_extension e ON d.refobjid = e.oid - WHERE d.objid = c.oid + WHERE d.objid = c.oid + AND d.deptype = 'x' ) AS dependsonextensions,The
deptype = 'x'filter ensures onlyDEPENDS ON EXTENSIONrelationships are captured.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/properties.sql (1)
20-25: Add dependency type filter to ensure correctness.The past review correctly identified that the query lacks a dependency type filter. This is the same issue as in the pg/15_plus version.
Apply the suggested fix:
( SELECT array_agg(DISTINCT e.extname) FROM pg_depend d JOIN pg_extension e ON d.refobjid = e.oid - WHERE d.objid = c.oid + WHERE d.objid = c.oid + AND d.deptype = 'x' ) AS dependsonextensions,The
deptype = 'x'filter ensures onlyDEPENDS ON EXTENSIONrelationships are captured.web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.msql (1)
1-17: Filename remains misleading despite previous review.The past review correctly identified that this filename suggests "no_depends" but the content includes
DEPENDS ON EXTENSIONstatements for plpgsql and postgres_fdw. The filename should be renamed to reflect the actual content (e.g.,create_mview_with_depends.msql).web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql (1)
1-18: ALTER test file contains CREATE statement instead of ALTER.Similar to the PG 13+ version, this file is referenced as the expected SQL for an alter operation that removes the postgres_fdw dependency, but it contains a full CREATE MATERIALIZED VIEW statement (lines 5-9) with only
DEPENDS ON EXTENSION plpgsql(lines 14-15).The expected content should be:
ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" NO DEPENDS ON EXTENSION postgres_fdw;web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql (1)
124-124: Useview_schemaandview_namefallback variables for consistency.Line 124 uses
data.schemaanddata.namedirectly, which is inconsistent with the pattern established at lines 5–6 and used throughout the rest of the template (lines 68, 74, 80, 88, 93, 109, 142, 162, etc.). This creates a risk: if the schema or name were changed earlier in the update operation,data.schemaordata.namemight not be set, causing the statement to fail.Apply this diff:
-ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(data.schema, data.name) }} SET( +ALTER MATERIALIZED VIEW IF EXISTS {{ conn|qtIdent(view_schema, view_name) }} SET(
🧹 Nitpick comments (5)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json (1)
275-276: Field ordering inconsistency with other test scenarios.Lines 275-276 have
expected_msql_filebeforeexpected_sql_file, while all earlier scenarios in this file (e.g., lines 19-20) placeexpected_sql_filefirst. Standardize the field ordering for consistency.Apply this diff to align with the ordering used in other scenarios:
- "expected_msql_file": "create_mview_with_am_msql.sql", - "expected_sql_file": "create_mview_with_am.sql" + "expected_sql_file": "create_mview_with_am.sql", + "expected_msql_file": "create_mview_with_am_msql.sql"web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py (2)
1960-1967: Consider simplifying the initialization logic.The condition and initialization can be more concise:
- if self.node_type == 'mview' and ( - old_data.get('dependsonextensions') is None or - data.get('dependsonextensions') is None - ): - old_data['dependsonextensions'] = \ - old_data.get('dependsonextensions') or [] - data['dependsonextensions'] = \ - data.get('dependsonextensions') or [] + if self.node_type == 'mview': + old_data.setdefault('dependsonextensions', []) + data.setdefault('dependsonextensions', [])This approach uses
setdefault()which only sets the value if the key doesn't exist or is None, making the code cleaner and more Pythonic.
2038-2042: Simplify field initialization.The initialization pattern can be simplified:
- if self.node_type == 'mview' and ( - data.get('dependsonextensions') is None - ): - data['dependsonextensions'] = \ - data.get('dependsonextensions') or [] + if self.node_type == 'mview': + data.setdefault('dependsonextensions', [])Using
setdefault()is more idiomatic and avoids the redundantdata.get('dependsonextensions') or []pattern.web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
109-109: Clarify the purpose of the empty field object.An empty object
{}is inserted at line 109, which appears unusual in the baseFields array. This might be intentional for UI spacing/grouping, but it should be clarified or removed if it's not needed.If this empty object serves no purpose, consider removing it:
- }, - { { id: 'dependsonextensions',web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql (1)
169-203: Consider using fallback variables in privilege and security label macro calls.Lines 169–203 pass
data.nameanddata.schemadirectly to the PRIVILEGE and SECLABEL macros. While these likely handle quoting internally, for consistency with the fallback variable pattern established at lines 5–6 and used throughout the template, consider passingview_schemaandview_nameinstead. This ensures uniform handling of view identifiers across all code paths.Example: Lines 169 and 175 could change from:
-{{ PRIVILEGE.UNSETALL(conn, 'TABLE', priv.grantee, data.name, data.schema) }} +{{ PRIVILEGE.UNSETALL(conn, 'TABLE', priv.grantee, view_name, view_schema) }}Apply this pattern to all macro calls in the privilege (lines 169, 175, 177, 179, 184) and security label (lines 193, 198, 203) sections.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/en_US/images/materialized_view_definition.pngis excluded by!**/*.png
📒 Files selected for processing (36)
docs/en_US/materialized_view_dialog.rst(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/create.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/properties.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/update.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/create_mview_with_am.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/create_mview_with_am.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.msql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json(1 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/15_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json(2 hunks)web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json(2 hunks)web/regression/javascript/schema_ui_files/mview.ui.spec.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.js
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/create_mview_with_am.sql
- docs/en_US/materialized_view_dialog.rst
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/create_mview_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/create_mview_with_am.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/properties.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/16_plus/test_mview.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/15_plus/sql/create.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/update.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/create_mview_no_depends.sql
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/12_plus/test_mview.json
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/15_plus/test_mview.json
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
web/regression/javascript/fake_gettext.js (1)
gettext(40-42)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py (1)
web/pgadmin/browser/server_groups/servers/__init__.py (1)
node_type(139-140)
⏰ 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). (10)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (17)
🔇 Additional comments (11)
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/16_plus/test_mview.json (2)
3-14: Test scenario structure is sound, but dependency chain setup is critical.The "Create Extension" scenario (lines 3–14) with
"store_object_id": trueestablishes the foundation for subsequent test scenarios. Thepostgres_fdwextension name and"relocatable": truesetting are valid.Verification needed: Ensure the test framework correctly stores the extension's object ID so that line 59's
<postgres_fdw>placeholder substitution works as intended during cleanup.
15-34: Extension-dependency test data is well-formed and comprehensive.The "Create Materialised Views with extensions" scenario correctly tests:
- Complex materialized view name with special characters (line 27):
testmview_$%{}[]()&*^!/@#`- Multi-select extension dependencies (line 30):
["plpgsql", "postgres_fdw"]- Proper endpoint configuration for SQL and MSQL generation
The test case covers a realistic use case of a materialized view depending on multiple extensions.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/12_plus/test_mview.json (1)
258-277: All expected SQL test artifact files are present and valid.Both
create_mview_with_am.sqlandcreate_mview_with_am_msql.sqlexist in the test directory and contain appropriate SQL statements that correctly reflect the test case parameters (materialized view name, definition, owner, comment, tablespace, and access method).web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/create.sql (1)
45-51: LGTM: Extension dependency template logic is correct.The template correctly iterates over
dependsonextensionsand generates appropriateALTER MATERIALIZED VIEW ... DEPENDS ON EXTENSIONstatements with proper identifier escaping.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/15_plus/sql/create.sql (1)
45-51: LGTM: Extension dependency template logic is correct.The template correctly handles extension dependencies with proper identifier escaping, consistent with the PPAS 13+ version.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/pg/13_plus/sql/update.sql (1)
207-220: LGTM: Extension dependency logic is well-implemented.The template correctly handles addition and removal of extension dependencies with proper guards and SQL syntax.
web/regression/javascript/schema_ui_files/mview.ui.spec.js (1)
56-61: LGTM: Test coverage for the new field.The test appropriately verifies the presence and configuration of the dependsonextensions field.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/mview.ui.js (1)
110-124: LGTM: Well-configured multi-select field.The dependsonextensions field is properly configured with appropriate controls, version gating (PG 13+), and mode restrictions.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/test_mview.json (1)
1-340: LGTM: Comprehensive test scenarios for PPAS 13+.The test data covers a wide range of materialized view operations including extension dependencies, privileges, ownership, and property changes. The structure aligns well with the PG 13+ test data.
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/test_mview.json (1)
1-341: LGTM: Comprehensive test scenarios for PG 13+.The test data covers extensive scenarios including extension dependencies, privileges, and property changes. However, note that the alter test (lines 36-46) references
alter_mview_no_depends.sql, which has content issues flagged in a separate comment.web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/update.sql (1)
207-220: Extension dependencies logic is sound.The new DEPENDS ON / NO DEPENDS ON EXTENSION handling (lines 207–220) correctly:
- Preserves old extension list as a fallback (line 207).
- Detects when dependencies change (line 209).
- Iterates over unique extensions to avoid duplicate statements (line 210).
- Conditionally emits ADD or REMOVE statements based on membership in old vs. new lists (lines 212–218).
- Uses the established fallback variables
view_schemaandview_namefor identifier quoting (line 213, 216).
| ( | ||
| SELECT array_agg(DISTINCT e.extname) | ||
| FROM pg_depend d | ||
| JOIN pg_extension e ON d.refobjid = e.oid | ||
| WHERE d.objid = c.oid | ||
| ) AS dependsonextensions, |
There was a problem hiding this comment.
Add dependency type filter to ensure correctness.
The query retrieves dependencies between the materialized view and extensions but doesn't filter by dependency type. This could return incorrect results if the materialized view has other dependency relationships with extensions beyond explicit DEPENDS ON EXTENSION declarations.
Apply this fix to filter for extension dependencies only:
(
SELECT array_agg(DISTINCT e.extname)
FROM pg_depend d
JOIN pg_extension e ON d.refobjid = e.oid
- WHERE d.objid = c.oid
+ WHERE d.objid = c.oid
+ AND d.deptype = 'x'
) AS dependsonextensions,The deptype = 'x' filter ensures only DEPENDS ON EXTENSION relationships are captured, not other dependency types like 'n' (normal) or 'a' (auto).
📝 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 = c.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 = c.oid | |
| AND d.deptype = 'x' | |
| ) AS dependsonextensions, |
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/templates/mviews/ppas/13_plus/sql/properties.sql
around lines 20 to 25, the SELECT that collects extension dependencies from
pg_depend lacks a dependency-type filter; add a condition to the WHERE clause to
restrict to extension dependencies only by filtering on d.deptype = 'x' so only
DEPENDS ON EXTENSION relationships are aggregated.
| -- View: public.testmview_$%{}[]()&*^!/@`# | ||
|
|
||
| -- DROP MATERIALIZED VIEW IF EXISTS public."testmview_$%{}[]()&*^!/@`#"; | ||
|
|
||
| CREATE MATERIALIZED VIEW IF NOT EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| TABLESPACE pg_default | ||
| AS | ||
| SELECT 1 AS col1 | ||
| WITH NO DATA; | ||
|
|
||
| ALTER TABLE IF EXISTS public."testmview_$%{}[]()&*^!/@`#" | ||
| OWNER TO postgres; | ||
|
|
||
| ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| DEPENDS ON EXTENSION plpgsql; | ||
|
|
||
| COMMENT ON MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#" | ||
| IS 'comment1'; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Previous fix not reflected in current file content.
The past review (marked as "Addressed in commits") correctly identified that this file should contain an ALTER statement removing the postgres_fdw dependency, but the current content still shows a full CREATE MATERIALIZED VIEW statement with only the plpgsql dependency (lines 5-15).
The expected content for this alter test should be:
ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#"
NO DEPENDS ON EXTENSION postgres_fdw;Verify that the fix from commits f906326 to fd91208 is properly applied in the current branch:
🏁 Script executed:
#!/bin/bash
# Check the actual content of alter_mview_no_depends.sql files
echo "=== PG 13_plus version ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sql
echo -e "\n=== PPAS 13_plus version ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql
echo -e "\n=== Expected .msql files ==="
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.msql
cat web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.msqlLength of output: 1305
Fix from previous commits not applied—test SQL files are inconsistent with expected output.
The .msql files correctly show the expected ALTER statement removing the postgres_fdw dependency, but both the PG and PPAS .sql input files still contain the full CREATE statement with only the plpgsql dependency and lack the required NO DEPENDS ON EXTENSION postgres_fdw statement.
Files requiring fixes:
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sqlweb/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql
Both files must include the ALTER MATERIALIZED VIEW ... NO DEPENDS ON EXTENSION postgres_fdw; statement to match their respective .msql expected outputs.
🤖 Prompt for AI Agents
In
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/pg/13_plus/alter_mview_no_depends.sql
lines 1-18 (and apply same change to the PPAS counterpart path
web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/ppas/13_plus/alter_mview_no_depends.sql),
the test SQL is missing the ALTER MATERIALIZED VIEW ... NO DEPENDS ON EXTENSION
postgres_fdw; statement required by the .msql expected output; add a new
statement exactly: ALTER MATERIALIZED VIEW public."testmview_$%{}[]()&*^!/@`#"
NO DEPENDS ON EXTENSION postgres_fdw; placed after the existing ALTER
MATERIALIZED VIEW ... DEPENDS ON EXTENSION plpgsql; line so the file matches the
expected output.
3 participants
Detail Info:- #6390
Summary by CodeRabbit