Add SQL formatting and query plan analysis module via explain-postgresql.com API#9726
Add SQL formatting and query plan analysis module via explain-postgresql.com API#9726MGorkov wants to merge 6 commits intopgadmin-org:masterfrom
Conversation
…analysis Added a new module to pgAdmin 4 that allows users to analyze query plans. The module sends the plan to an external API service for visualization and detailed breakdown.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Explain PostgreSQL (EP) tool: a backend Flask blueprint with two POST endpoints for formatting and explaining SQL, frontend React components and API calls, async CodeMirror formatting that prefers EP (with local fallback), ResultSet integration emitting/opening EP plans, and translation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant CM as CodeMirror
participant API as pgAdmin API
participant EP as Explain PostgreSQL API
participant LF as LocalFormatter
Client->>CM: Request SQL formatting
CM->>CM: Check ep.explain_postgresql_format pref
alt EP formatting enabled
CM->>API: POST /explain_postgresql_format (SQL)
API->>EP: Forward request
EP-->>API: Formatted SQL or error
alt success
API-->>CM: Formatted SQL
else failure
API-->>CM: Error
CM->>LF: Format locally
LF-->>CM: Formatted SQL
end
else EP formatting disabled
CM->>LF: Format locally
LF-->>CM: Formatted SQL
end
CM-->>Client: Display formatted SQL
sequenceDiagram
participant User as User
participant Editor as ResultSet/Editor
participant API as pgAdmin API
participant EPComp as EP backend (blueprint)
participant IFrame as Embedded IFrame
User->>Editor: Execute query
Editor->>Editor: Generate query plan JSON
Editor->>API: POST /explain_postgresql (plan JSON + query)
API->>EPComp: Forward request (using prefs)
EPComp-->>API: Return plan URL / response
API-->>Editor: Success with plan URL
Editor->>Editor: Emit QUERY_PLAN event
Editor->>Editor: Open ExplainPostgreSQL tab with plans and SQL
ExplainPostgreSQL->>IFrame: Embed plan via returned URL
IFrame-->>User: Render explain plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (25)
web/pgadmin/translations/id/LC_MESSAGES/messages.po-17902-17912 (1)
17902-17912:⚠️ Potential issue | 🟡 MinorTranslate the new Geometry Viewer empty-state messages.
Both newly added status strings are still empty here, so Indonesian users will get English guidance in the empty/no-spatial-data paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/id/LC_MESSAGES/messages.po` around lines 17902 - 17912, The two new message entries in messages.po for the Geometry Viewer are untranslated (empty msgstr) so Indonesian users see English; open the entries for the msgid starting "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." and the msgid starting "No spatial data found. At least one geometry or geography column is required for visualization." and provide accurate Indonesian translations in their corresponding msgstr fields (preserve punctuation and intent), then save the updated messages.po so the GeometryViewer.jsx strings are localized.web/pgadmin/translations/id/LC_MESSAGES/messages.po-15946-15981 (1)
15946-15981:⚠️ Potential issue | 🟡 MinorPopulate the new Explain PostgreSQL entries.
Several new msgids in this block still have empty
msgstrs. Even if brand labels stay in English, the new Explain PostgreSQL settings/help/error copy will otherwise fall back to English and ship as a mixed-language Indonesian UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/id/LC_MESSAGES/messages.po` around lines 15946 - 15981, The listed msgid entries ("Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain Postgresql API") currently have empty msgstrs; populate each msgstr with the appropriate Indonesian translation (or keep the brand name in English where desired) while preserving punctuation and any examples, and ensure the duplicate/combined msgid ("Failed to post data to the Explain Postgresql API") is translated consistently; update the translations in the messages.po entries corresponding to those msgid strings so the UI shows Indonesian text instead of falling back to English.web/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.po-15617-15652 (1)
15617-15652:⚠️ Potential issue | 🟡 MinorCapitalization inconsistency in error message.
Line 15651 uses "Explain Postgresql API" (lowercase 's' in "Postgresql"), but all other occurrences use "PostgreSQL" with proper capitalization. This inconsistency will appear in error messages.
Proposed fix in the source file
The fix should be applied in the source file
pgadmin/tools/ep/__init__.pyat lines 92 and 117:-msgid "Failed to post data to the Explain Postgresql API" +msgid "Failed to post data to the Explain PostgreSQL API"
Missing Chinese translations for new EP-related strings.
All new Explain PostgreSQL feature strings have empty
msgstrvalues. While this is acceptable for initial development, these should be translated for complete Simplified Chinese localization:
- "Explain PostgreSQL"
- "Explain PostgreSQL API"
- "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)"
- "Private Plans"
- "Hide plans from public access on Explain PostgreSQL"
- "Explain Plan"
- "Analyze query plan via Explain PostgreSQL API"
- "Format SQL using Explain PostgreSQL API"
- "Failed to post data to the Explain Postgresql API"
Would you like me to open an issue to track the completion of these translations?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.po` around lines 15617 - 15652, The messages.po entries for the new Explain PostgreSQL feature are missing Chinese translations and one error string uses inconsistent capitalization ("Explain Postgresql API"); update the source to consistently use "Explain PostgreSQL" (fix the string in pgadmin/tools/ep/__init__.py where "Failed to post data to the Explain Postgresql API" appears) and populate the corresponding msgstr values in web/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.po for the keys "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain PostgreSQL API".web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po-17375-17385 (1)
17375-17385:⚠️ Potential issue | 🟡 MinorMissing translations for new GeometryViewer messages.
Two new user-facing strings lack Portuguese translations:
- Line 17376-17378: "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results."
- Line 17382-17384: "No spatial data found. At least one geometry or geography column is required for visualization."
These are informative messages that help users understand how to use the Geometry Viewer feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po` around lines 17375 - 17385, Add Portuguese translations for the two new GeometryViewer messages in the messages.po file: provide a suitable pt_BR msgstr for the msgid "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." and for the msgid "No spatial data found. At least one geometry or geography column is required for visualization." so they replace the empty msgstr entries; locate the entries associated with pgadmin/tools/sqleditor/static/js/components/sections/GeometryViewer.jsx and update each msgstr with the correct Portuguese phrasing.web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po-17813-17951 (1)
17813-17951:⚠️ Potential issue | 🟡 MinorSeveral ResultSet strings have incomplete or truncated translations.
Some translations appear truncated or missing:
Line msgid msgstr (issue) 17818 "min" empty 17831 "Refetching latest results..." empty 17839 "Connection Error" empty 17844-17849 "Execution Cancelled!" / "Execution Cancelled" empty 17852 "Server Connected." empty 17904 "Fetching rows..." empty 17913 "The data has changed..." truncated to "Os dados foram alterados. " 17937 "Auto-commit is off..." truncated to "A confirmação automática está desativada. " 17951 "No data output..." truncated to "Sem saída de dados. " The truncated translations (ending with a space and missing the rest of the sentence) should be completed to provide full context to users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po` around lines 17813 - 17951, Update the missing and truncated Portuguese translations in the messages.po entries used by ResultSet (and related) components: fill in msgstr for "min", "Refetching latest results...", "Connection Error", "Execution Cancelled!", "Execution Cancelled", "Server Connected.", "Fetching rows...", and complete the truncated sentences for "The data has changed. Do you want to save changes?", "Auto-commit is off. You still need to commit changes to the database." and "No data output. Execute a query to get output." Ensure each msgstr is a full, grammatically correct Portuguese translation (preserve any python-format markers like %, keep quotes and escaped characters as in the original msgid) and remove the stray trailing spaces so strings are complete and accurate for the ResultSet.jsx/ResultSetToolbar.jsx UI messages.web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po-15486-15521 (1)
15486-15521:⚠️ Potential issue | 🟡 MinorMissing translations for new Explain PostgreSQL feature strings.
All new Explain PostgreSQL UI strings have empty
msgstrvalues. While the application will fall back to English, Brazilian Portuguese users won't see localized text for this feature. Consider adding translations before release:
- Line 15488: "Explain PostgreSQL"
- Line 15492: "Explain PostgreSQL API"
- Line 15496: "Explain PostgreSQL API endpoint..."
- Line 15500: "Private Plans" → e.g., "Planos Privados"
- Line 15504: "Hide plans from public access..." → e.g., "Ocultar planos do acesso público..."
- Line 15508: "Explain Plan" → e.g., "Plano de Explicação"
- Line 15512: "Analyze query plan via Explain PostgreSQL API"
- Line 15516: "Format SQL using Explain PostgreSQL API"
- Line 15520: "Failed to post data to the Explain Postgresql API"
Also note: Line 15520 has inconsistent capitalization "Postgresql" vs "PostgreSQL" used elsewhere in this file. The source string in
pgadmin/tools/ep/__init__.pyshould use "PostgreSQL" for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po` around lines 15486 - 15521, The PO file contains empty msgstr entries for new Explain PostgreSQL UI strings; fill the Portuguese translations for the msgids "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain Postgresql API" in web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po so Brazilian Portuguese users see localized text; also fix the inconsistent capitalization by correcting the source string in pgadmin/tools/ep/__init__.py from "Postgresql" to "PostgreSQL" (refer to the msgid "Failed to post data to the Explain Postgresql API") so translations and source match.web/pgadmin/translations/pl/LC_MESSAGES/messages.po-15608-15610 (1)
15608-15610:⚠️ Potential issue | 🟡 MinorInconsistent capitalization in source string: "Postgresql" vs "PostgreSQL".
The error message uses "Explain Postgresql" (lowercase 'sql') while all other strings in this feature use "Explain PostgreSQL" (uppercase 'SQL'). This inconsistency originates from the source code in
pgadmin/tools/ep/__init__.pyat lines 92 and 117.Suggested fix in source file (ep/__init__.py)
-msgid "Failed to post data to the Explain Postgresql API" +msgid "Failed to post data to the Explain PostgreSQL API"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/pl/LC_MESSAGES/messages.po` around lines 15608 - 15610, Update the source string in pgadmin/tools/ep/__init__.py so the product name is consistently capitalized as "Explain PostgreSQL" instead of "Explain Postgresql"; locate the two occurrences referenced (the msgid used at the sites around the code at the positions corresponding to lines 92 and 117) and change the literal string to "Failed to post data to the Explain PostgreSQL API" so translations and other messages remain consistent.web/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.po-17498-17508 (1)
17498-17508:⚠️ Potential issue | 🟡 MinorTranslate the new Geometry Viewer messages too.
Lines 17498-17508 introduce two new runtime messages with empty
msgstrs, which leaves this path partially untranslated inzh_Hant_TW.Sample zh_Hant_TW wording
msgid "" "Query complete. Use the Geometry Viewer button in the Data Output tab to " "visualize results." -msgstr "" +msgstr "" +"查詢完成。請使用「資料輸出」頁籤中的「幾何查看器」按鈕來視覺化結果。" msgid "" "No spatial data found. At least one geometry or geography column is " "required for visualization." -msgstr "" +msgstr "" +"找不到空間資料。至少需要一個 geometry 或 geography 欄位才能進行視覺化。"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.po` around lines 17498 - 17508, The two new runtime messages for the Geometry Viewer in the translation file have empty msgstr entries—fill in the Traditional Chinese (zh_Hant_TW) translations for the two msgid strings shown (the "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." message and the "No spatial data found. At least one geometry or geography column is required for visualization." message) in web/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.po by replacing the empty msgstrs with appropriate zh_Hant_TW translations so the GeometryViewer UI strings are localized; ensure the translations match tone and context used by GeometryViewer.jsx.web/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.po-15613-15648 (1)
15613-15648:⚠️ Potential issue | 🟡 MinorFill in the new Explain PostgreSQL translations.
Lines 15613-15648 add the full Explain PostgreSQL surface with empty
msgstrs, sozh_Hant_TWwill fall back to English for labels, help text, and the API error. Keeping the product name in English is fine, but these descriptive strings should not stay blank.Sample zh_Hant_TW wording
msgid "Explain PostgreSQL" -msgstr "" +msgstr "Explain PostgreSQL" msgid "Explain PostgreSQL API" -msgstr "" +msgstr "Explain PostgreSQL API" msgid "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)" -msgstr "" +msgstr "Explain PostgreSQL API 端點（例如 https://explain-postgresql.com）" msgid "Private Plans" -msgstr "" +msgstr "私有查詢計畫" msgid "Hide plans from public access on Explain PostgreSQL" -msgstr "" +msgstr "在 Explain PostgreSQL 上將查詢計畫設為非公開" msgid "Explain Plan" -msgstr "" +msgstr "查詢計畫分析" msgid "Analyze query plan via Explain PostgreSQL API" -msgstr "" +msgstr "透過 Explain PostgreSQL API 分析查詢計畫" msgid "Format SQL using Explain PostgreSQL API" -msgstr "" +msgstr "使用 Explain PostgreSQL API 格式化 SQL" msgid "Failed to post data to the Explain Postgresql API" -msgstr "" +msgstr "無法將資料傳送到 Explain PostgreSQL API"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.po` around lines 15613 - 15648, Add Traditional Chinese (zh_Hant_TW) translations for the new Explain PostgreSQL strings by filling each empty msgstr for the msgid entries "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain Postgresql API" with appropriate zh_Hant_TW text (you may keep the product name "Explain PostgreSQL" in English where desired), ensuring the translations convey the same UI/help/error meanings and preserve placeholders/URLs exactly as in the msgid values.web/pgadmin/translations/es/LC_MESSAGES/messages.po-15618-15653 (1)
15618-15653:⚠️ Potential issue | 🟡 MinorMissing Spanish translations for new Explain PostgreSQL feature strings.
All 9 new msgid entries for the Explain PostgreSQL feature have empty
msgstrvalues. Spanish-speaking users will see English text for these UI elements:
- "Explain PostgreSQL"
- "Explain PostgreSQL API"
- "Private Plans"
- "Explain Plan"
- API descriptions and error messages
Consider adding Spanish translations before release, or tracking this as a follow-up task to ensure localization completeness for the new feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/es/LC_MESSAGES/messages.po` around lines 15618 - 15653, The provided messages.po contains nine msgid entries with empty msgstr values (e.g. "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", "Failed to post data to the Explain Postgresql API"); fill in appropriate Spanish translations for each corresponding msgstr in the same file, ensure translations preserve punctuation/URLs and grammatical context, and then rebuild/compile the locale files so the new Spanish strings are shipped to users.web/pgadmin/translations/ko/LC_MESSAGES/messages.po-15708-15743 (1)
15708-15743:⚠️ Potential issue | 🟡 MinorTranslate the new Explain PostgreSQL entries.
These new
msgids are added with emptymsgstrvalues, so the Korean catalog stays untranslated for the EP settings, actions, and failure message. Please populate these before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ko/LC_MESSAGES/messages.po` around lines 15708 - 15743, The PO entries for Explain PostgreSQL are untranslated: fill the empty msgstr values for the listed msgid strings ("Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", "Failed to post data to the Explain Postgresql API") (references: pgadmin/tools/ep/__init__.py and ResultSet.jsx) by providing accurate Korean translations, preserving the msgid text and PO formatting/escaping, encoding in UTF-8, and keeping the same quoting and punctuation so the translations are valid in messages.po.web/pgadmin/translations/ko/LC_MESSAGES/messages.po-17617-17627 (1)
17617-17627:⚠️ Potential issue | 🟡 MinorLocalize the new Geometry Viewer notices.
Both new status messages are added with empty
msgstr, which leaves the no-spatial-data path untranslated in the Korean UI. Please add Korean translations here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ko/LC_MESSAGES/messages.po` around lines 17617 - 17627, Add Korean translations for the two new msgid entries introduced for Geometry Viewer notices so the UI shows localized strings instead of empty msgstr values; locate the two msgid lines matching the exact text "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." and "No spatial data found. At least one geometry or geography column is required for visualization." in messages.po (entries referenced from GeometryViewer.jsx) and fill each corresponding msgstr with appropriate Korean translations that convey the same meaning.web/pgadmin/translations/fr/LC_MESSAGES/messages.po-18088-18098 (1)
18088-18098:⚠️ Potential issue | 🟡 MinorMissing French translations for new GeometryViewer strings.
Two new GeometryViewer-related strings have empty translations:
"Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results."(Lines 18089-18092)"No spatial data found. At least one geometry or geography column is required for visualization."(Lines 18095-18098)These provide user guidance for spatial data visualization and should be translated for French users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/fr/LC_MESSAGES/messages.po` around lines 18088 - 18098, The messages.po file contains two empty French msgstr entries for GeometryViewer strings from GeometryViewer.jsx; add proper French translations for the msgid "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." and for "No spatial data found. At least one geometry or geography column is required for visualization." by filling their corresponding msgstr values with accurate French text, ensuring punctuation and placeholders (if any) match the original English, and save the updated translations so the GeometryViewer component displays French text for these messages.web/pgadmin/translations/fr/LC_MESSAGES/messages.po-16091-16127 (1)
16091-16127:⚠️ Potential issue | 🟡 MinorMissing French translations for all Explain PostgreSQL strings.
All 9 new translation entries for the Explain PostgreSQL feature have empty
msgstrvalues. French users will see English text instead of localized strings:
"Explain PostgreSQL"(Line 16094)"Explain PostgreSQL API"(Line 16098)"Explain PostgreSQL API endpoint..."(Line 16102)"Private Plans"(Line 16106)"Hide plans from public access..."(Line 16110)"Explain Plan"(Line 16114)"Analyze query plan via..."(Line 16118)"Format SQL using..."(Line 16122)"Failed to post data..."(Line 16126)Consider adding French translations before merging, or track this as a follow-up localization task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/fr/LC_MESSAGES/messages.po` around lines 16091 - 16127, Fill the empty msgstr entries for the nine Explain PostgreSQL msgid strings by providing proper French translations: "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain Postgresql API"; update each corresponding msgstr with an accurate French equivalent (preserving casing and contextual meaning) so the UI shows localized text for the Explain PostgreSQL feature.web/pgadmin/translations/de/LC_MESSAGES/messages.po-15578-15584 (1)
15578-15584:⚠️ Potential issue | 🟡 MinorNormalize the EP source copy before regenerating this catalog.
Line 15583 uses the fairly technical
API endpointwording for a preferences field, and Line 15607 switches toExplain Postgresqlwhile the rest of the feature usesExplain PostgreSQL. Please simplify the source text inpgadmin/tools/ep/__init__.pyand keep the product name spelling consistent before extracting translations. Based on learnings, help messages for form controls should be kept concise and avoid technical jargon or terminology that might sound like error states; simple descriptions are preferred.Also applies to: 15606-15608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/de/LC_MESSAGES/messages.po` around lines 15578 - 15584, Update the source strings in pgadmin/tools/ep/__init__.py so the preference help text is concise and the product name spelling is consistent: replace the msgid "Explain PostgreSQL API" with a simpler label such as "Explain PostgreSQL" and change "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)" to a short, non-technical help text like "Explain PostgreSQL (e.g. https://explain-postgresql.com)" to avoid the term "API endpoint" and ensure "PostgreSQL" is capitalized consistently across the strings referenced above.web/pgadmin/translations/de/LC_MESSAGES/messages.po-15573-15608 (1)
15573-15608:⚠️ Potential issue | 🟡 MinorPlease fill in the new Explain PostgreSQL entries for
de.All of the new EP-specific entries in this block still have empty
msgstrs. That ships the new settings/actions/error text as raw English in the German locale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/de/LC_MESSAGES/messages.po` around lines 15573 - 15608, The new Explain PostgreSQL translation entries in pgadmin/tools/ep/__init__.py (the msgid values "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain Postgresql API") currently have empty msgstrs in the German .po file; fill each corresponding msgstr with an appropriate German translation that preserves meaning and tone for the UI/error messages (ensure punctuation and capitalization match original intent).web/pgadmin/translations/ru/LC_MESSAGES/messages.po-18207-18215 (1)
18207-18215:⚠️ Potential issue | 🟡 MinorTranslate the new Save Data messages.
These two
msgstrentries are empty, so the Russian flow falls back to English exactly where it explains generated SQL and rollback state. Suggested translations:Этот запрос был сгенерирован pgAdmin в рамках операции «Сохранить данные».andСохранение изменений данных было отменено откатом, но текущая транзакция всё ещё активна; предыдущие запросы не затронуты.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ru/LC_MESSAGES/messages.po` around lines 18207 - 18215, Add Russian translations for the two empty msgstr entries corresponding to the msgid strings in ResultSet.jsx: set msgstr for "This query was generated by pgAdmin as part of a \"Save Data\" operation" to "Этот запрос был сгенерирован pgAdmin в рамках операции «Сохранить данные»." and set msgstr for "Saving data changes was rolled back but the current transaction is still active; previous queries are unaffected." to "Сохранение изменений данных было отменено откатом, но текущая транзакция всё ещё активна; предыдущие запросы не затронуты."web/pgadmin/translations/ru/LC_MESSAGES/messages.po-17614-17629 (1)
17614-17629:⚠️ Potential issue | 🟡 MinorFix the plural grammar in these Geometry Viewer warnings.
3D geometries not rendered.is translated as singular, andGeometries with non-SRID %s not rendered.mixes singular/plural and reads broken asне-SRID %s. Please keep both messages plural and preserve%s, e.g.3D-геометрии не отображаются./Геометрии с SRID, отличным от %s, не отображаются.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ru/LC_MESSAGES/messages.po` around lines 17614 - 17629, Update the Russian translations in messages.po for the GeometryViewer strings to use correct plural forms and preserve the %s placeholder: change the translation for msgid "3D geometries not rendered." to a plural form (e.g. "3D-геометрии не отображаются."), fix "Geometries with non-SRID %s not rendered." to a plural, correctly formatted sentence preserving %s (e.g. "Геометрии с SRID, отличным от %s, не отображаются."), and ensure the msgid mappings in the GeometryViewer.jsx references remain unchanged so the keys still match.web/pgadmin/translations/ru/LC_MESSAGES/messages.po-17655-17665 (1)
17655-17665:⚠️ Potential issue | 🟡 MinorAdd the missing Russian translations for the new Geometry Viewer states.
Both
msgstrentries are empty, so RU users will see English for the success and empty-state messages. Suggested translations:Запрос завершён. Используйте кнопку «Просмотрщик геометрии» на вкладке «Вывод данных», чтобы визуализировать результаты.andПространственные данные не найдены. Для визуализации требуется как минимум один столбец geometry или geography.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ru/LC_MESSAGES/messages.po` around lines 17655 - 17665, Fill the two empty msgstr entries in messages.po that correspond to the Geometry Viewer messages used in GeometryViewer.jsx: set the msgstr for "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." to "Запрос завершён. Используйте кнопку «Просмотрщик геометрии» на вкладке «Вывод данных», чтобы визуализировать результаты." and set the msgstr for "No spatial data found. At least one geometry or geography column is required for visualization." to "Пространственные данные не найдены. Для визуализации требуется как минимум один столбец geometry или geography."web/pgadmin/translations/ru/LC_MESSAGES/messages.po-14561-14565 (1)
14561-14565:⚠️ Potential issue | 🟡 MinorTranslate
Analysisbefore reusing it in the tree view.Line 14564 adds a new
PgTreeViewreference, butmsgstris still empty. The Russian UI will fall back to English on that path. Suggestedmsgstr:Анализ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ru/LC_MESSAGES/messages.po` around lines 14561 - 14565, The new msgid "Analysis" in messages.po is untranslated so the Russian UI will fall back to English; open the translation entry for msgid "Analysis" in web/pgadmin/translations/ru/LC_MESSAGES/messages.po and set msgstr to the Russian translation (suggested: "Анализ"), ensuring the same msgid is used by PgTreeView (PgTreeView/index.jsx) so the tree view displays the localized string.web/pgadmin/translations/ru/LC_MESSAGES/messages.po-15740-15747 (1)
15740-15747:⚠️ Potential issue | 🟡 MinorKeep the Explain PostgreSQL labels aligned with the source meaning.
On Line 15742 the service name becomes a generic
Анализ плана выполнения, on Line 15746 the group name turns intoАдрес API..., and on Line 15764 the singularExplain Planbecomes plural. That blurs the new external Explain PostgreSQL integration with pgAdmin's built-in Explain flow. Please keep the service/group names literal and the plan label singular.Also applies to: 15763-15765
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ru/LC_MESSAGES/messages.po` around lines 15740 - 15747, The translations for the service/group/label are too liberal—ensure the msgstr values match the source literally: update the msgstr for "Explain PostgreSQL" to preserve the exact service name (do not translate to a generic "Анализ плана выполнения"), change "Explain PostgreSQL API" msgstr to the literal group name (not "Адрес API..."), and make the "Explain Plan" translation singular (not plural); apply the same corrections to the other occurrence range noted (15763-15765).web/pgadmin/translations/ja/LC_MESSAGES/messages.po-17619-17630 (1)
17619-17630:⚠️ Potential issue | 🟡 MinorMissing Japanese translations for new GeometryViewer guidance messages.
Two new user-facing messages for the Geometry Viewer have empty
msgstrvalues:
- "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results."
- "No spatial data found. At least one geometry or geography column is required for visualization."
These guidance messages help users understand how to use the Geometry Viewer feature and should be translated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ja/LC_MESSAGES/messages.po` around lines 17619 - 17630, Add Japanese translations for the two empty msgstr entries in messages.po corresponding to the Geometry Viewer guidance messages found in GeometryViewer.jsx; locate the two msgid strings ("Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." and "No spatial data found. At least one geometry or geography column is required for visualization.") and provide accurate Japanese translations in their msgstr fields so the UI displays localized guidance for the GeometryViewer component.web/pgadmin/translations/ja/LC_MESSAGES/messages.po-15745-15747 (1)
15745-15747:⚠️ Potential issue | 🟡 MinorFix inconsistent capitalization: "Postgresql" should be "PostgreSQL".
Lines 92 and 117 in
pgadmin/tools/ep/__init__.pyuse "Explain Postgresql API" with lowercase "sql", while all other references in the file consistently use "Explain PostgreSQL" (uppercase "SQL"). Standardize to "PostgreSQL" for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ja/LC_MESSAGES/messages.po` around lines 15745 - 15747, Update the inconsistent capitalization of "Postgresql" to "PostgreSQL" by changing the msgid "Failed to post data to the Explain Postgresql API" to "Failed to post data to the Explain PostgreSQL API" in the translation entry and also update the matching strings in the ep package's __init__.py where that message is emitted (replace "Explain Postgresql API" => "Explain PostgreSQL API" in both occurrences so source code and .po stay consistent).web/pgadmin/translations/ja/LC_MESSAGES/messages.po-15712-15747 (1)
15712-15747:⚠️ Potential issue | 🟡 MinorMissing Japanese translations for new Explain PostgreSQL feature strings.
All new entries related to the Explain PostgreSQL feature have empty
msgstrvalues. These should be translated before release to provide a complete localized experience for Japanese users.Key strings requiring translation:
- "Explain PostgreSQL"
- "Explain PostgreSQL API"
- "Private Plans"
- "Explain Plan"
- API endpoint descriptions and error messages
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/ja/LC_MESSAGES/messages.po` around lines 15712 - 15747, The PO file contains empty msgstr translations for the new Explain PostgreSQL strings (msgid values like "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API", and "Failed to post data to the Explain Postgresql API"); update the Japanese translations by providing appropriate Japanese text for each msgid in web/pgadmin/translations/ja/LC_MESSAGES/messages.po so every msgstr is filled, ensuring wording matches existing localization style and preserves placeholders and capitalization exactly as in the msgid entries.web/pgadmin/messages.pot-15474-15476 (1)
15474-15476:⚠️ Potential issue | 🟡 MinorInconsistent casing: "Postgresql" should be "PostgreSQL".
The error message uses "Postgresql" (lowercase 'ql') while all other EP-related strings consistently use "PostgreSQL" (uppercase SQL) to match the official branding.
Proposed fix
The fix needs to be applied in the source file
pgadmin/tools/ep/__init__.pyat lines 92 and 117:-"Failed to post data to the Explain Postgresql API" +"Failed to post data to the Explain PostgreSQL API"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/messages.pot` around lines 15474 - 15476, Update the translation string "Failed to post data to the Explain Postgresql API" to use the correct branding "Failed to post data to the Explain PostgreSQL API" in both occurrences of that msgid found in the Explain Plan module (the two msgid entries corresponding to the Explain Postgresql API); ensure both instances are changed exactly and regenerate/update the .pot/.po files as needed so casing is consistent across the EP-related messages.
🧹 Nitpick comments (7)
web/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.po (1)
17454-17514: Missing Chinese translations for GeometryViewer messages.Two new GeometryViewer messages at lines 17503-17513 have empty
msgstrvalues:
- "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results."
- "No spatial data found. At least one geometry or geography column is required for visualization."
These should be translated for complete localization coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.po` around lines 17454 - 17514, Add Chinese translations for the two empty msgstr entries corresponding to the GeometryViewer messages: translate "Query complete. Use the Geometry Viewer button in the Data Output tab to visualize results." and "No spatial data found. At least one geometry or geography column is required for visualization." by filling their msgstr values in the messages.po so they match the style of other entries referenced from GeometryViewer.jsx (use clear, idiomatic Simplified Chinese).web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po (1)
11024-11027: Additional empty translations for general UI strings.Several non-EP related strings also lack translations:
- Line 11026: "Configuration" → "Configuração"
- Line 13511: "No files/folders found" → "Nenhum arquivo/pasta encontrado"
- Line 14330: "No objects are found to display" → "Nenhum objeto encontrado para exibir"
These are common UI messages that would benefit from localization.
Also applies to: 13510-13512, 14329-14331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/pt_BR/LC_MESSAGES/messages.po` around lines 11024 - 11027, Add Portuguese translations for the missing UI strings in the messages.po file by filling the msgstr for each msgid noted in the review: set msgid "Configuration" → msgstr "Configuração", msgid "No files/folders found" → msgstr "Nenhum arquivo/pasta encontrado", and msgid "No objects are found to display" → msgstr "Nenhum objeto encontrado para exibir"; ensure you update the existing entries for those exact msgid values (e.g., the entries shown around the current diff for pgadmin/tools/ep) and keep the PO file formatting and encoding intact.web/pgadmin/translations/pl/LC_MESSAGES/messages.po (1)
15575-15610: New Explain PostgreSQL strings await Polish translation.All new strings for the Explain PostgreSQL feature have empty
msgstrvalues. While this is expected when adding new source strings (translations are typically done in a separate pass), please ensure these are tracked for translation before release to Polish users.New untranslated strings:
- "Explain PostgreSQL"
- "Explain PostgreSQL API"
- "Explain PostgreSQL API endpoint..."
- "Private Plans"
- "Hide plans from public access..."
- "Explain Plan"
- "Analyze query plan via..."
- "Format SQL using..."
- "Failed to post data..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/translations/pl/LC_MESSAGES/messages.po` around lines 15575 - 15610, The new Explain PostgreSQL UI strings in the PO file (msgids such as "Explain PostgreSQL", "Explain PostgreSQL API", "Explain PostgreSQL API endpoint (e.g. https://explain-postgresql.com)", "Private Plans", "Hide plans from public access on Explain PostgreSQL", "Explain Plan", "Analyze query plan via Explain PostgreSQL API", "Format SQL using Explain PostgreSQL API" and "Failed to post data to the Explain Postgresql API") currently have empty msgstr entries; update messages.po by providing Polish translations for each msgid (located under references pgadmin/tools/ep/__init__.py and pgadmin/tools/sqleditor/...) so the UI is localized for Polish users, then run the usual PO validation/compilation (msgfmt or project i18n tool) to ensure no syntax errors.web/pgadmin/tools/ep/static/js/ExplainPostgreSQL/index.jsx (2)
67-67: Inconsistent loading state styling.The loading state renders a plain
<p>Loading...</p>while empty and error states useStyledBoxwithEmptyPanelMessage. Consider using consistent styling or the project'sLoadercomponent for a unified UX.♻️ Proposed fix for consistent styling
- if (isLoading) return <p>Loading...</p>; + if (isLoading) return ( + <StyledBox height="100%" display="flex" flexDirection="column"> + <EmptyPanelMessage text={gettext('Loading...')} /> + </StyledBox> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/static/js/ExplainPostgreSQL/index.jsx` at line 67, The loading branch currently returns a plain <p> via the isLoading check, causing inconsistent styling; update the isLoading rendering in the ExplainPostgreSQL component to use the same UI pattern as empty/error states (e.g., wrap the Loader component inside StyledBox and/or display EmptyPanelMessage style) so loading uses the project Loader and StyledBox instead of a raw paragraph; locate the isLoading conditional and replace the simple <p>Loading...</p> with a Loader (or the existing EmptyPanelMessage markup inside StyledBox) to match the other states.
36-58: Missing dependencies in useEffect may cause stale data.The useEffect has an empty dependency array
[], but it usesplansandsqlprops. If this component instance receives new props (e.g., from a parent re-render), the effect won't re-fetch. While ResultSet.jsx currently creates new tab instances per plan, adding dependencies would make this component more robust for future reuse.♻️ Proposed fix to add dependencies
useEffect(() => { if (_.isEmpty(plans)) return; const api = getApiInstance(); + setIsLoading(true); + setError(''); api.post( url_for('ep.explain_postgresql'), JSON.stringify({ plan: JSON.stringify(plans), query: sql, })) .then((res) => { if (res.data?.success) { setData(res.data?.data); } else { setError(`${res.data?.info} : ${res.data?.errormsg}`); } }) .catch((err) => { setError(err?.message); }) .finally(() => { setIsLoading(false); }); - }, []); + }, [plans, sql]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/static/js/ExplainPostgreSQL/index.jsx` around lines 36 - 58, The useEffect in ExplainPostgreSQL uses plans and sql but has an empty dependency array, leading to stale fetches; update the effect's dependency array to include plans and sql (and any other external values used inside like getApiInstance if it's not stable) so the POST is re-run when those props change, keeping the logic in the effect (the API.post call, setData, setError, setIsLoading) the same and ensuring you still guard with _.isEmpty(plans) at the top of the effect.web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
1025-1038: Missing cleanup function for event listener registration.The
useEffectregisters a listener forQUERY_TOOL_EVENTS.QUERY_PLANbut doesn't return a cleanup function.eventBus.registerListenerreturns a deregister function (as seen in lines 1118-1123 withderegExec). Without cleanup, re-mounting this component could register duplicate listeners.♻️ Proposed fix to add cleanup
useEffect(() => { - eventBus.registerListener(QUERY_TOOL_EVENTS.QUERY_PLAN, (planJson, sql) => { + const deregQueryPlan = eventBus.registerListener(QUERY_TOOL_EVENTS.QUERY_PLAN, (planJson, sql) => { if (!planJson) return; layoutDocker.openTab({ id: PANELS.EXPLAIN_POSTGRESQL, title: gettext('Explain PostgreSQL'), content: <ExplainPostgreSQL plans={planJson} sql={sql} />, closable: true, }, PANELS.MESSAGES, 'after-tab', true); }); + return () => { + deregQueryPlan(); + }; }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx` around lines 1025 - 1038, The effect registers a listener via eventBus.registerListener for QUERY_TOOL_EVENTS.QUERY_PLAN inside the useEffect in ResultSet.jsx but never deregisters it; capture the deregister function returned by eventBus.registerListener (e.g., const deregPlan = eventBus.registerListener(...)) and return a cleanup function from that useEffect that calls the deregister (e.g., return () => deregPlan()); keep the same empty dependency array and ensure the listener callback signature (planJson, sql) remains unchanged.web/pgadmin/tools/ep/static/js/ExplainPostgreSQL/formatSQL.js (1)
4-31: Consider simplifying with async/await instead of explicit Promise constructor.The function is already marked
asyncbut wraps an explicitnew Promise(). This is the Promise constructor anti-pattern. Using async/await directly would be cleaner and avoid the hanging Promise issue.♻️ Proposed refactor using async/await
-export default async function formatSQL(sql) { - return new Promise((resolve, reject) => { - const api = getApiInstance(); - api.post( - url_for('ep.explain_postgresql_format'), - JSON.stringify({ - query_src: sql, - })) - .then((res) => { - if (res.data?.data) { - try { - const {btf_query, btf_query_text} = JSON.parse(res.data.data); - if (btf_query !== btf_query_text) { - resolve(btf_query_text); - } else { - reject(btf_query_text); - } - } catch (err) { - console.error(err); - reject(err.message); - } - } - }) - .catch((err) => { - console.error(err); - reject(err.message); - }); - }); -}; +export default async function formatSQL(sql) { + const api = getApiInstance(); + const res = await api.post( + url_for('ep.explain_postgresql_format'), + JSON.stringify({ + query_src: sql, + }) + ); + + if (!res.data?.data) { + throw new Error('No data returned from formatting API'); + } + + const { btf_query, btf_query_text } = JSON.parse(res.data.data); + if (btf_query !== btf_query_text) { + return btf_query_text; + } + // No formatting changes needed - return original + return sql; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/static/js/ExplainPostgreSQL/formatSQL.js` around lines 4 - 31, The formatSQL function wraps an explicit new Promise inside an async function; remove the Promise constructor and convert the control flow to async/await: await the api.post call (using getApiInstance() and url_for('ep.explain_postgresql_format')), then inside a try/catch parse res.data?.data with JSON.parse to extract btf_query and btf_query_text and return btf_query_text when btf_query !== btf_query_text or throw an Error(btf_query_text) when they are equal; also throw meaningful errors when res.data?.data is missing or JSON.parse fails and surface caught error.message in the thrown Error so callers receive the same failure information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c016482-d146-407e-bfd5-415b710ca256
📒 Files selected for processing (34)
web/pgadmin/messages.potweb/pgadmin/static/js/components/ReactCodeMirror/index.jsxweb/pgadmin/tools/__init__.pyweb/pgadmin/tools/ep/__init__.pyweb/pgadmin/tools/ep/static/js/ExplainPostgreSQL/formatSQL.jsweb/pgadmin/tools/ep/static/js/ExplainPostgreSQL/index.jsxweb/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.jsweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/translations/cs/LC_MESSAGES/messages.moweb/pgadmin/translations/cs/LC_MESSAGES/messages.poweb/pgadmin/translations/de/LC_MESSAGES/messages.moweb/pgadmin/translations/de/LC_MESSAGES/messages.poweb/pgadmin/translations/es/LC_MESSAGES/messages.moweb/pgadmin/translations/es/LC_MESSAGES/messages.poweb/pgadmin/translations/fr/LC_MESSAGES/messages.moweb/pgadmin/translations/fr/LC_MESSAGES/messages.poweb/pgadmin/translations/id/LC_MESSAGES/messages.moweb/pgadmin/translations/id/LC_MESSAGES/messages.poweb/pgadmin/translations/it/LC_MESSAGES/messages.moweb/pgadmin/translations/it/LC_MESSAGES/messages.poweb/pgadmin/translations/ja/LC_MESSAGES/messages.moweb/pgadmin/translations/ja/LC_MESSAGES/messages.poweb/pgadmin/translations/ko/LC_MESSAGES/messages.poweb/pgadmin/translations/pl/LC_MESSAGES/messages.moweb/pgadmin/translations/pl/LC_MESSAGES/messages.poweb/pgadmin/translations/pt_BR/LC_MESSAGES/messages.moweb/pgadmin/translations/pt_BR/LC_MESSAGES/messages.poweb/pgadmin/translations/ru/LC_MESSAGES/messages.moweb/pgadmin/translations/ru/LC_MESSAGES/messages.poweb/pgadmin/translations/sv/LC_MESSAGES/messages.poweb/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.moweb/pgadmin/translations/zh_Hans_CN/LC_MESSAGES/messages.poweb/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.moweb/pgadmin/translations/zh_Hant_TW/LC_MESSAGES/messages.po
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
web/pgadmin/tools/ep/__init__.py (3)
34-40: Minor: Default URL differs from help text example.The default value
'https://explain.tensor.ru'differs from the example inhelp_str(https://explain-postgresql.com). This could confuse users about which endpoint is canonical.Consider aligning them for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 34 - 40, The default value for the explain_postgresql_api preference is inconsistent with the help_str example; update the call to preference.register that sets explain_postgresql_api so the default URL matches the example in help_str (i.e., change the default from 'https://explain.tensor.ru' to 'https://explain-postgresql.com') by editing the explain_postgresql_api registration in the __init__.py where preference.register is invoked.
185-203: Inconsistent indentation: uses 2 spaces instead of 4.This function uses 2-space indentation while the rest of the file uses 4 spaces. This inconsistency affects readability and violates PEP 8 conventions.
♻️ Fix indentation to 4 spaces
def send_post_request(url_api, data, parse=False): - data = json.dumps(data).encode('utf-8') - headers = { - "Content-Type": "application/json; charset=utf-8", - "User-Agent": "pgAdmin4/ExplainModule", - "Method": "POST" - } - try: - req = urllib.request.Request(url_api, data, headers) - with no302opener.open(req, timeout=10) as response: - if (response.code == 302): - return False, response.headers["Location"] - response_data = response.read().decode('utf-8') - if (parse): - return False, json.loads(response_data) - else: - return False, response_data - except Exception as e: - return True, str(e) + data = json.dumps(data).encode('utf-8') + headers = { + "Content-Type": "application/json; charset=utf-8", + "User-Agent": "pgAdmin4/ExplainModule", + "Method": "POST" + } + try: + req = urllib.request.Request(url_api, data, headers) + with no302opener.open(req, timeout=10) as response: + if response.code == 302: + return False, response.headers["Location"] + response_data = response.read().decode('utf-8') + if parse: + return False, json.loads(response_data) + else: + return False, response_data + except Exception as e: + return True, str(e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 185 - 203, The send_post_request function is indented with 2 spaces instead of the project standard 4; reformat the entire function block (including the body, try/except and all inner statements like the Request creation, with no302opener.open, response handling and return statements) to use 4-space indentation so it matches the rest of the file and PEP8 conventions.
250-252: Silent exception handling may obscure preference configuration issues.The
try-except-passpattern silently ignores all exceptions, which could hide misconfigured preferences or module registration issues. Consider logging the exception for debugging purposes.🔧 Add logging for debugging
+import logging + +logger = logging.getLogger(__name__) + def get_preference_value(name): ... try: pref_module = Preferences.module(MODULE_NAME) ... - except Exception: - pass + except Exception as e: + logger.debug("Failed to get preference '%s': %s", name, e) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 250 - 252, The except Exception: pass silently swallows errors in the try block in web/pgadmin/tools/ep/__init__.py; replace the pass with a logged exception so failures are visible. Add a module logger (logger = logging.getLogger(__name__)) or use current_app.logger if in Flask context, then call logger.exception("Failed loading preference/module in <function-or-block-name>") (or logger.error with exc_info=True) inside the except. Ensure you import logging (or current_app) at the top so the exception and stack trace are recorded instead of being ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 205-221: The No302HTTPErrorProcessor class is indented with tab
characters; replace all leading tabs with 4-space indentation in the class
definition and its methods (http_response and https_response) so indentation
matches the rest of the file and avoids TabError/IndentationError; ensure the
method bodies and the "https_response = http_response" assignment use spaces,
run a linter/autoformat (e.g., black or flake8) to confirm consistent spacing,
and commit the updated indentation only.
- Around line 146-152: The code assumes send_post_request returns a redirect
path and blindly concatenates explain_postgresql_api + data; change the response
handling in the block using send_post_request so it explicitly handles redirect
vs non-redirect: inspect the returned data (or status indicator from
send_post_request) and if data is a relative path (e.g., startswith('/') or is a
full URL) build the final URL using explain_postgresql_api + data (or use data
directly if absolute), otherwise treat data as a non-redirect body (attempt to
parse JSON or return it as-is) and return make_json_response(success=1,
data=parsed_body) instead of concatenating; reference send_post_request,
explain_postgresql_api and make_json_response to find and update the logic.
- Around line 175-180: The SSRF check currently only ensures parsed.hostname is
present; update the validation to resolve parsed.hostname to IP addresses and
use the ipaddress module to reject any loopback, localhost, link-local
(169.254.0.0/16), private (RFC1918), unspecified, multicast or other non-global
addresses before returning True; specifically, after obtaining parsed.hostname
(the variable named parsed.hostname in this file), perform DNS resolution for
both A and AAAA records, iterate over each resolved IP and parse it with
ipaddress.ip_address(), and return False if any address .is_loopback,
.is_private, .is_link_local, .is_unspecified, .is_multicast (or other non-global
checks) is True, otherwise allow; if hostname resolution fails or raises an
exception, treat it as invalid and return False; alternatively enforce that only
administrators can set explain_postgresql_api if you prefer a policy change
instead of IP blocking.
---
Nitpick comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 34-40: The default value for the explain_postgresql_api preference
is inconsistent with the help_str example; update the call to
preference.register that sets explain_postgresql_api so the default URL matches
the example in help_str (i.e., change the default from
'https://explain.tensor.ru' to 'https://explain-postgresql.com') by editing the
explain_postgresql_api registration in the __init__.py where preference.register
is invoked.
- Around line 185-203: The send_post_request function is indented with 2 spaces
instead of the project standard 4; reformat the entire function block (including
the body, try/except and all inner statements like the Request creation, with
no302opener.open, response handling and return statements) to use 4-space
indentation so it matches the rest of the file and PEP8 conventions.
- Around line 250-252: The except Exception: pass silently swallows errors in
the try block in web/pgadmin/tools/ep/__init__.py; replace the pass with a
logged exception so failures are visible. Add a module logger (logger =
logging.getLogger(__name__)) or use current_app.logger if in Flask context, then
call logger.exception("Failed loading preference/module in
<function-or-block-name>") (or logger.error with exc_info=True) inside the
except. Ensure you import logging (or current_app) at the top so the exception
and stack trace are recorded instead of being ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62662a27-c81d-46d6-ace6-9e4f335eb064
📒 Files selected for processing (2)
web/pgadmin/tools/ep/__init__.pyweb/pgadmin/tools/ep/static/js/ExplainPostgreSQL/formatSQL.js
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/tools/ep/static/js/ExplainPostgreSQL/formatSQL.js
There was a problem hiding this comment.
♻️ Duplicate comments (3)
web/pgadmin/tools/ep/__init__.py (3)
193-195:⚠️ Potential issue | 🟠 Major
no302openerstill follows redirects before your custom processor sees them.Passing
HTTPHandler()andHTTPSHandler()intobuild_opener()does not remove the defaultHTTPRedirectHandler. The 302 branch insend_post_request()is therefore still bypassed, so this helper reads the redirected response instead of theLocationheader it expects.#!/bin/bash python - <<'PY' import urllib.request class No302HTTPErrorProcessor(urllib.request.HTTPErrorProcessor): pass opener = urllib.request.build_opener( No302HTTPErrorProcessor(), urllib.request.HTTPHandler(), urllib.request.HTTPSHandler(), ) for idx, handler in enumerate(opener.handlers): print(f"{idx}: {handler.__class__.__name__}") PYAlso applies to: 204-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 193 - 195, The opener still follows redirects because HTTPRedirectHandler is present by default; update the code that builds no302opener (used by send_post_request) to include a custom HTTPRedirectHandler that disables redirects (e.g., subclass urllib.request.HTTPRedirectHandler and override redirect_request to return None) so the 302 response and Location header are preserved, then use that custom handler in build_opener in place of the current No302HTTPErrorProcessor-only opener; apply the same change to the other opener creation mentioned around the 204-227 block so both places stop following redirects before your processor sees them.
146-152:⚠️ Potential issue | 🟠 MajorMake the explain success payload explicit instead of concatenating arbitrary helper output.
send_post_request()can return either a redirect target or the raw response body, but this route always doesexplain_postgresql_api + data. That only works for relative redirect paths; a 200 body or an absolute URL produces malformed output for the client.Also applies to: 184-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 146 - 152, The handler currently returns make_json_response(success=1, data=explain_postgresql_api + data) which blindly concatenates explain_postgresql_api and the value returned from send_post_request; instead detect what send_post_request returned and build an explicit payload: if send_post_request returned a redirect target (e.g., a string starting with '/' or a relative path) join it with explain_postgresql_api using a proper URL join (urllib.parse.urljoin) and return that as a 'redirect' or 'url' field; if it returned a 200 body (non-URL), return it as a 'body' or 'result' field without concatenation; update the success branch in this function (and the similar block around lines 184-200) to call make_json_response(success=1, url=full_url) or make_json_response(success=1, result=body) as appropriate rather than concatenating strings.
155-181:⚠️ Potential issue | 🔴 CriticalBlock internal hosts in URL validation.
is_valid_url()only checks forhttp/httpsplus a hostname, so a user-controlledexplain_postgresql_apican still targetlocalhost, RFC1918, link-local, or metadata endpoints. That leaves these routes open to SSRF against internal services.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 155 - 181, Update is_valid_url to block localhost and internal IP ranges: after parsing with urlparse and extracting parsed.hostname, immediately reject hostname values like 'localhost' and names ending with '.local'; then try to treat hostname as an IP literal using ipaddress.ip_address and return False if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved or ip.is_multicast; if hostname is not an IP literal, call socket.getaddrinfo(hostname, None) and for each resolved address use ipaddress.ip_address to ensure none are in those disallowed ranges (return False if any are); on any resolution or parsing exception return False and otherwise return True. Use the existing is_valid_url function and the parsed.hostname symbol, and import ipaddress and socket.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 193-195: The opener still follows redirects because
HTTPRedirectHandler is present by default; update the code that builds
no302opener (used by send_post_request) to include a custom HTTPRedirectHandler
that disables redirects (e.g., subclass urllib.request.HTTPRedirectHandler and
override redirect_request to return None) so the 302 response and Location
header are preserved, then use that custom handler in build_opener in place of
the current No302HTTPErrorProcessor-only opener; apply the same change to the
other opener creation mentioned around the 204-227 block so both places stop
following redirects before your processor sees them.
- Around line 146-152: The handler currently returns
make_json_response(success=1, data=explain_postgresql_api + data) which blindly
concatenates explain_postgresql_api and the value returned from
send_post_request; instead detect what send_post_request returned and build an
explicit payload: if send_post_request returned a redirect target (e.g., a
string starting with '/' or a relative path) join it with explain_postgresql_api
using a proper URL join (urllib.parse.urljoin) and return that as a 'redirect'
or 'url' field; if it returned a 200 body (non-URL), return it as a 'body' or
'result' field without concatenation; update the success branch in this function
(and the similar block around lines 184-200) to call
make_json_response(success=1, url=full_url) or make_json_response(success=1,
result=body) as appropriate rather than concatenating strings.
- Around line 155-181: Update is_valid_url to block localhost and internal IP
ranges: after parsing with urlparse and extracting parsed.hostname, immediately
reject hostname values like 'localhost' and names ending with '.local'; then try
to treat hostname as an IP literal using ipaddress.ip_address and return False
if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved or
ip.is_multicast; if hostname is not an IP literal, call
socket.getaddrinfo(hostname, None) and for each resolved address use
ipaddress.ip_address to ensure none are in those disallowed ranges (return False
if any are); on any resolution or parsing exception return False and otherwise
return True. Use the existing is_valid_url function and the parsed.hostname
symbol, and import ipaddress and socket.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99951be3-52bb-4d6a-8083-3572aaa01128
📒 Files selected for processing (2)
web/pgadmin/tools/ep/__init__.pyweb/pgadmin/tools/ep/static/js/ExplainPostgreSQL/formatSQL.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/pgadmin/tools/ep/__init__.py (2)
158-182:⚠️ Potential issue | 🔴 Critical
is_valid_url()still permits SSRF to internal hosts.This validator only constrains scheme/hostname, so values like
http://localhost:...,http://192.168.x.x, orhttp://169.254.169.254still pass and can be reached server-side through these endpoints. Please either restrict this setting to admin-controlled config or resolve A/AAAA records here and reject any non-global address before returningTrue.
225-230:⚠️ Potential issue | 🟠 MajorThe opener still follows redirects before you can read
Location.
build_opener()still adds the defaultHTTPRedirectHandlerhere, because this call does not supply a replacement redirect handler. That means the 302 branch insend_post_request()is not dependable, so the/explainflow can end up following the redirect instead of returning the redirect target.Proposed fix
+class NoRedirectHandler(urllib.request.HTTPRedirectHandler): + def http_error_302(self, req, fp, code, msg, headers): + return fp + + http_error_301 = http_error_303 = http_error_307 = http_error_308 = http_error_302 + # Build opener without HTTPRedirectHandler so No302HTTPErrorProcessor can handle 302 responses no302opener = urllib.request.build_opener( + NoRedirectHandler(), No302HTTPErrorProcessor(), - urllib.request.HTTPHandler(), - urllib.request.HTTPSHandler() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 225 - 230, The opener construction still lets the default HTTPRedirectHandler get inserted, so replace the implicit redirect handler with an explicit no-redirect handler when creating no302opener: define or use a subclass of urllib.request.HTTPRedirectHandler that prevents redirects (e.g., overrides redirect_request to return None) and pass that handler into urllib.request.build_opener alongside No302HTTPErrorProcessor, so send_post_request() can reliably inspect 302 responses and Location headers instead of having redirects followed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 87-93: Change the request.get_json() call to use silent=True so
malformed or missing JSON returns None instead of raising BadRequest; update the
checks around request.get_json() (the call in this module, e.g., where data =
request.get_json()) to use data = request.get_json(silent=True) and keep the
existing isinstance(data, dict) guard and make_json_response() error path; apply
the same change to the other get_json() usage in this file where you perform the
same validation flow.
---
Duplicate comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 225-230: The opener construction still lets the default
HTTPRedirectHandler get inserted, so replace the implicit redirect handler with
an explicit no-redirect handler when creating no302opener: define or use a
subclass of urllib.request.HTTPRedirectHandler that prevents redirects (e.g.,
overrides redirect_request to return None) and pass that handler into
urllib.request.build_opener alongside No302HTTPErrorProcessor, so
send_post_request() can reliably inspect 302 responses and Location headers
instead of having redirects followed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f301e112-8d4f-4121-9be9-4d0098c79cd1
📒 Files selected for processing (1)
web/pgadmin/tools/ep/__init__.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/pgadmin/tools/ep/__init__.py (1)
158-185:⚠️ Potential issue | 🟠 MajorSSRF risk: Private IPs and localhost are still allowed.
The docstring explicitly states that localhost and private IP ranges are allowed, but this creates SSRF exposure. Any authenticated user can configure
explain_postgresql_apito target internal services (e.g.,http://localhost:5000,http://192.168.1.1, or cloud metadata athttp://169.254.169.254).Consider one of:
- Block private IP ranges, localhost, and link-local addresses using the
ipaddressmodule- Make the API URL an admin-only configuration rather than a per-user preference
- Maintain a server-side allowlist of trusted EP endpoints
This relates to the previous unaddressed review comment about incomplete SSRF protection.
🛡️ Suggested implementation to block private IPs
+import socket +import ipaddress + def is_valid_url(url): """ - Validate that a URL is safe to use (HTTP/HTTPS only, localhost and private IP ranges are allowed). + Validate that a URL is safe to use (HTTP/HTTPS only, no private/localhost targets). Args: url: The URL to validate Returns: - bool: True if URL is valid, False otherwise + bool: True if URL is valid and safe, False otherwise """ if not url: return False try: parsed = urlparse(url) # Only allow http and https schemes if parsed.scheme not in ('http', 'https'): return False hostname = parsed.hostname if not hostname: return False + # Block localhost variants + if hostname.lower() in ('localhost', '127.0.0.1', '::1', '0.0.0.0'): + return False + + # Resolve and check for private/internal IPs + try: + for info in socket.getaddrinfo(hostname, None): + ip = ipaddress.ip_address(info[4][0]) + if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved: + return False + except (socket.gaierror, ValueError): + return False + return True except Exception: return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 158 - 185, The is_valid_url function currently permits localhost and private IPs; modify is_valid_url to resolve parsed.hostname (use socket.getaddrinfo) and for every returned IP convert to an ipaddress.ip_address and reject the URL if any address is loopback, private, link-local, or otherwise reserved (e.g., is_loopback, is_private, is_link_local, is_reserved); keep the existing scheme and hostname checks but add these network checks for both IPv4 and IPv6, handle DNS resolution errors by returning False, and import/use the socket and ipaddress modules to perform the validation.
🧹 Nitpick comments (3)
web/pgadmin/tools/ep/__init__.py (3)
207-224: Inconsistent indentation: 2-space instead of 4-space.Same indentation inconsistency as
send_post_request. The class methods should use 4-space indentation to match the rest of the file and PEP 8.♻️ Proposed fix
class No302HTTPErrorProcessor(urllib.request.HTTPErrorProcessor): - def http_response(self, request, response): - code, msg, hdrs = response.code, response.msg, response.info() - - if (code == 302): - return response - - # According to RFC 2616, "2xx" code indicates that the client's - # request was successfully received, understood, and accepted. - if not (200 <= code < 300): - response = self.parent.error( - 'http', request, response, code, msg, hdrs) - - return response - - https_response = http_response + def http_response(self, request, response): + code, msg, hdrs = response.code, response.msg, response.info() + + if code == 302: + return response + + # According to RFC 2616, "2xx" code indicates that the client's + # request was successfully received, understood, and accepted. + if not (200 <= code < 300): + response = self.parent.error( + 'http', request, response, code, msg, hdrs) + + return response + + https_response = http_response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 207 - 224, The No302HTTPErrorProcessor class uses 2-space indentation for its body and methods (http_response and https_response) which is inconsistent with the rest of the file and PEP8; update the class definition and its methods to use 4-space indentation (match the style used by send_post_request) so the class body, method signatures, and method bodies are indented with four spaces.
187-206: Inconsistent indentation: 2-space instead of 4-space.This function uses 2-space indentation while the rest of the file uses 4-space indentation (PEP 8 standard). This was partially addressed from tabs, but the spacing is still inconsistent.
♻️ Proposed fix to normalize indentation
def send_post_request(url_api, data, parse=False): - data = json.dumps(data).encode('utf-8') - headers = { - "Content-Type": "application/json; charset=utf-8", - "User-Agent": "pgAdmin4/ExplainModule", - "Method": "POST" - } - try: - req = urllib.request.Request(url_api, data, headers) - with no302opener.open(req, timeout=10) as response: - if (response.code == 302): - return False, response.headers["Location"] - response_data = response.read().decode('utf-8') - if (parse): - return False, json.loads(response_data) - else: - return False, response_data - except Exception as e: - return True, str(e) + data = json.dumps(data).encode('utf-8') + headers = { + "Content-Type": "application/json; charset=utf-8", + "User-Agent": "pgAdmin4/ExplainModule", + "Method": "POST" + } + try: + req = urllib.request.Request(url_api, data, headers) + with no302opener.open(req, timeout=10) as response: + if response.code == 302: + return False, response.headers["Location"] + response_data = response.read().decode('utf-8') + if parse: + return False, json.loads(response_data) + else: + return False, response_data + except Exception as e: + return True, str(e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 187 - 206, The send_post_request function uses 2-space indentation which is inconsistent with the file's 4-space PEP8 style; update the whole function block (function def send_post_request, its try/except, the with no302opener.open(...) block, and all nested statements including response handling and return branches) to use 4 spaces per indentation level so it matches the rest of the module and preserves existing logic (urllib.request.Request, no302opener.open, response.code check, parse handling, and the exception return).
252-254: Silent exception swallowing may hide configuration issues.The
try-except-passpattern silently discards any exceptions during preference retrieval. While returningNoneas a fallback is reasonable, completely suppressing errors could mask underlying configuration problems.Consider logging the exception at debug level for troubleshooting:
♻️ Proposed fix
+from flask import current_app + def get_preference_value(name): ... try: pref_module = Preferences.module(MODULE_NAME) if pref_module: pref = pref_module.preference(name) if pref: value = pref.get() if isinstance(value, str): value = value.strip() return value or None return value - except Exception: - pass + except Exception as e: + current_app.logger.debug( + f"Failed to retrieve preference '{name}': {e}" + ) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/ep/__init__.py` around lines 252 - 254, The silent except block in __init__.py that currently does "except Exception: pass" before "return None" should log the caught exception at debug (or debug/error) level so configuration issues aren't hidden; update the exception handler in that preference retrieval block to catch Exception as e and call the module/app logger (e.g., logger.debug or current_app.logger.debug) with a short contextual message and the exception (or its traceback), then return None as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 158-185: The is_valid_url function currently permits localhost and
private IPs; modify is_valid_url to resolve parsed.hostname (use
socket.getaddrinfo) and for every returned IP convert to an ipaddress.ip_address
and reject the URL if any address is loopback, private, link-local, or otherwise
reserved (e.g., is_loopback, is_private, is_link_local, is_reserved); keep the
existing scheme and hostname checks but add these network checks for both IPv4
and IPv6, handle DNS resolution errors by returning False, and import/use the
socket and ipaddress modules to perform the validation.
---
Nitpick comments:
In `@web/pgadmin/tools/ep/__init__.py`:
- Around line 207-224: The No302HTTPErrorProcessor class uses 2-space
indentation for its body and methods (http_response and https_response) which is
inconsistent with the rest of the file and PEP8; update the class definition and
its methods to use 4-space indentation (match the style used by
send_post_request) so the class body, method signatures, and method bodies are
indented with four spaces.
- Around line 187-206: The send_post_request function uses 2-space indentation
which is inconsistent with the file's 4-space PEP8 style; update the whole
function block (function def send_post_request, its try/except, the with
no302opener.open(...) block, and all nested statements including response
handling and return branches) to use 4 spaces per indentation level so it
matches the rest of the module and preserves existing logic
(urllib.request.Request, no302opener.open, response.code check, parse handling,
and the exception return).
- Around line 252-254: The silent except block in __init__.py that currently
does "except Exception: pass" before "return None" should log the caught
exception at debug (or debug/error) level so configuration issues aren't hidden;
update the exception handler in that preference retrieval block to catch
Exception as e and call the module/app logger (e.g., logger.debug or
current_app.logger.debug) with a short contextual message and the exception (or
its traceback), then return None as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ba03690-bc46-4eba-9ab2-2547957fde70
📒 Files selected for processing (1)
web/pgadmin/tools/ep/__init__.py
|
[With my pgAdmin hat on, and NOT my PGCA one] Can you confirm that explain-postgresql.com has appropriate licensing for use of the domain name including a community trademark, or considers it fair use (and if so, under what part of the trademark policy)? As licensees ourselves, we have to be careful about using any non-compliant services. We also need to consider whether we actually want to add support for 3rd party services such as this - we've only used services from large PostgreSQL vendors in the past, and this one is quite different in that it is effectively sending schema/query data to a 3rd party, unlike the others which call APIs upon specific user request to create a service (e.g. an RDS instance) On the patch itself, I note:
|
2 participants
This PR introduces a new module that integrates with the Explain PostgreSQL API (https://explain-postgresql.com). It provides users with enhanced SQL query formatting and in-depth query plan analysis directly within pgAdmin4.
Key Features:
Summary by CodeRabbit
New Features
Localization