Add SQL formatting and query plan analysis module via explain.tensor.ru API#9808
Open
MGorkov wants to merge 12 commits intopgadmin-org:masterfrom
Open
Add SQL formatting and query plan analysis module via explain.tensor.ru API#9808MGorkov wants to merge 12 commits intopgadmin-org:masterfrom
MGorkov wants to merge 12 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.
WalkthroughThis PR introduces the Explain PostgreSQL feature to pgAdmin 4, a tool that analyzes SQL query execution plans via a third-party API. It adds backend routes for status checks and plan/SQL formatting, React components for visualization, preference configuration options, documentation, translations, and comprehensive test coverage across the new module. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueryTool as Query Tool
participant ReactCM as ReactCodeMirror
participant ExplainAPI as ExplainPostgreSQL API
participant ExplainPanel as Explain Panel
participant ExternalAPI as Third-party Service
User->>QueryTool: Execute query with EXPLAIN
QueryTool->>ExplainAPI: GET /expl_pgsql/status
ExplainAPI-->>QueryTool: {success: true, data: {enabled: true}}
QueryTool->>QueryTool: Extract plan JSON
QueryTool->>QueryTool: Emit QUERY_PLAN event
QueryTool->>ExplainPanel: Create/open panel
User->>ExplainPanel: View execution plan
ExplainPanel->>ExplainAPI: POST /expl_pgsql/explain (plan + query)
ExplainAPI->>ExternalAPI: POST /explain (with request params)
ExternalAPI-->>ExplainAPI: 302 Location: result URL
ExplainAPI-->>ExplainPanel: {success: true, data: result_url}
ExplainPanel->>ExplainPanel: Render iframe with result_url
alt User formats SQL
User->>ReactCM: Request Format SQL
ReactCM->>ExplainAPI: POST /expl_pgsql/formatSQL (if enabled)
ExplainAPI->>ExternalAPI: POST /beautifier-api
ExternalAPI-->>ExplainAPI: {btf_query: formatted_sql}
ExplainAPI-->>ReactCM: Formatted SQL
ReactCM->>ReactCM: Update editor
else Format SQL disabled
ReactCM->>ReactCM: Use local sql-formatter
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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: 10
🧹 Nitpick comments (4)
web/pgadmin/tools/expl_pgsql/__init__.py (1)
239-254: Remove the unnecessary "Method" header.The
"Method": "POST"header on line 244 is non-standard and unnecessary. The HTTP method is determined by the request itself (viaurllib.request.Request), not by a header. This header serves no purpose and could confuse the receiving server.♻️ Proposed fix
headers = { "Content-Type": "application/json; charset=utf-8", "User-Agent": "pgAdmin4/ExplainModule", - "Method": "POST" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/expl_pgsql/__init__.py` around lines 239 - 254, The headers dict in send_post_request includes a non-standard "Method" header which should be removed; update the headers used to build urllib.request.Request in send_post_request to omit the "Method" key (keep "Content-Type" and "User-Agent"), relying on urllib.request.Request/no302opener to set the POST method via the presence of data, and verify behavior of no302opener.open(req, timeout=10) remains unchanged.web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx (1)
1042-1068: Missing dependencies in useEffect may cause stale closure issues.The
useEffectat lines 1042-1068 has an empty dependency array[]but referencesapi,layoutDocker, andgettextin its callback. If any of these values change during the component's lifecycle, the event handler will use stale references.However, based on the existing patterns in this file (e.g., line 1030-1040 for LLM status), this appears to be intentional since these values are expected to remain stable throughout the component's lifecycle.
💡 Consider adding dependencies for consistency
eventBus.registerListener(QUERY_TOOL_EVENTS.QUERY_PLAN, showExplPgsql); return () => { eventBus.deregisterListener(QUERY_TOOL_EVENTS.QUERY_PLAN, showExplPgsql); }; - }, []); + }, [api, layoutDocker]);If
apiandlayoutDockerare guaranteed stable (e.g., from context), the empty array is acceptable.🤖 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 1042 - 1068, The useEffect registers showExplPgsql but uses external symbols (api, layoutDocker, gettext) without listing them as dependencies; fix by either adding api, layoutDocker, and gettext to the dependency array of the useEffect that declares showExplPgsql (so the listener always uses current references) or, if those values are truly stable (from context/singleton), add a brief code comment and an eslint-disable-next-line react-hooks/exhaustive-deps before the dependency array to silence the rule; ensure you update the registration/deregistration using the same showExplPgsql function name and keep eventBus and QUERY_TOOL_EVENTS references intact.web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/formatSQL.js (1)
4-35: Avoid wrapping Promise constructor in an async function.Since
api.postalready returns a Promise, you can simplify by usingasync/awaitdirectly rather than wrapping innew Promise(). The current pattern is an anti-pattern that can mask errors.♻️ Proposed refactor using async/await
-export default async function formatSQL(sql) { - return new Promise((resolve, reject) => { - const api = getApiInstance(); - api.post( - url_for('expl_pgsql.formatSQL'), - JSON.stringify({ - query_src: sql, - })) - .then((res) => { - if (!res.data?.data) { - reject('No data returned from formatting API'); - return; - } - try { - const {btf_query, btf_query_text} = JSON.parse(res.data.data); - if (btf_query !== btf_query_text) { - resolve(btf_query_text); - } else { - // Server returns identical text in both fields in case of an error - // In this scenario, we use the local formatter as a fallback - 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('expl_pgsql.formatSQL'), + 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; + } + // Server returns identical text in both fields in case of an error + // In this scenario, we use the local formatter as a fallback + throw new Error(btf_query_text); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/formatSQL.js` around lines 4 - 35, The formatSQL function currently wraps api.post in a new Promise (anti-pattern); refactor by using the existing async function to await the API call: call getApiInstance().post(url_for('expl_pgsql.formatSQL'), JSON.stringify({query_src: sql})) inside a try/catch, check res.data?.data, JSON.parse it to extract btf_query and btf_query_text, return btf_query_text when different and throw an Error with the fallback btf_query_text when identical (server error case), and in catch log the error (console.error) and rethrow (throw err or new Error(err.message)); reference functions/identifiers: formatSQL, getApiInstance, url_for, api.post, and fields btf_query / btf_query_text.web/pgadmin/static/js/components/ReactCodeMirror/index.jsx (1)
150-150:formatSQLis now async but the keymap callback does not await it.When
formatSQLis invoked via the keyboard shortcut, it's called withoutawait. While this may work due to the function's internal error handling, consider whether the UI should reflect the async nature or if errors should be surfaced differently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/static/js/components/ReactCodeMirror/index.jsx` at line 150, The keymap is registering formatSQL via [parseShortcutValue(editorPrefs.format_sql, true)] but the callback is not awaiting the now-async formatSQL; update the keymap handler to be an async function that awaits formatSQL(...) and properly handle/propagate errors (e.g., try/catch or rethrow) so the UI can reflect completion or failures when invoking formatSQL from the keyboard shortcut.
🤖 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/messages.pot`:
- Around line 16121-16124: Replace the typo "Postgresql" with "PostgreSQL" in
the user-facing message; find the exact string "Failed to post data to the
Explain Postgresql API" (used in the expl_pgsql module's __init__.py) and change
it to "Failed to post data to the Explain PostgreSQL API" in both places where
that literal is defined/returned so the .pot file will regenerate with
consistent capitalization.
In `@web/pgadmin/static/js/components/ReactCodeMirror/index.jsx`:
- Around line 103-119: The finally block incorrectly reads the stale state
variable loading; change to use a local flag/ref to detect whether the timeout
callback actually ran (e.g. let timeoutFired = false or use a React ref) and set
it to true inside the setTimeout callback where you call setLoading(true); then
in the finally block: if timeoutFired (meaning loading was set) call
setLoading(false) else clearTimeout(loadingTimeout). Update references around
explPgsqlFormatSQL, loadingTimeout, setLoading, loading, and formattedSql to use
this new flag so the timeout behavior and loading state are handled correctly.
In `@web/pgadmin/tools/expl_pgsql/__init__.py`:
- Around line 209-236: The docstring and comments claim SSRF prevention but
is_valid_url currently only checks scheme/hostname; update is_valid_url to
perform proper hostname/IP validation: parse with urlparse, extract
parsed.hostname, and for hostnames that are IP literals or resolve via
socket.getaddrinfo, check all resolved IPs with the ipaddress module and reject
any in RFC1918 (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), loopback
(127.0.0.0/8, ::1/128), link-local/metadata (169.254.169.254 and
169.254.0.0/16), and any other non-public ranges; only return True for
http/https when all resolved addresses are public/allowed. Update the docstring
to reflect the new SSRF checks and reference the is_valid_url function and
parsed.hostname behavior so reviewers can find the change.
In `@web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/index.jsx`:
- Around line 88-89: The propTypes declaration for ExplainPostgreSQL marks plans
as required (ExplainPostgreSQL.propTypes.plans = PropTypes.array.isRequired)
while the component also supplies a default value for plans (default props or a
parameter default like plans = []); remove the contradiction by either removing
the isRequired flag from ExplainPostgreSQL.propTypes.plans if you want the
default to cover missing props, or remove the default value (plans = [] or
defaultProps.plans) if plans must be provided by callers—update only one of
those locations to keep prop validation consistent.
- Around line 42-44: The request body is being JSON.stringified twice causing
the plan to be double-escaped; in ExplainPostgreSQL/index.jsx stop wrapping the
whole payload in JSON.stringify and send a plain object instead (e.g., { plan:
plans, query: sql }), and also stop stringifying plans — pass the plans variable
as an object so the backend's request.get_json() receives a proper dict; locate
the API call where JSON.stringify({ plan: JSON.stringify(plans), query: sql })
is used and replace it with a plain object payload.
- Around line 76-83: The iframe in ExplainPostgreSQL (index.jsx) currently uses
sandbox="allow-scripts", which can block interactive features of the external
explain.tensor.ru tool; test the embedded visualization with that domain and, if
interactions require it, update the iframe's sandbox attribute to include the
minimal additional permissions needed (e.g., allow-popups, allow-forms,
allow-modals) while keeping security in mind—locate the <iframe ...
sandbox="allow-scripts" ... /> in the ExplainPostgreSQL component and add only
the specific allow-* tokens required after testing.
In `@web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py`:
- Around line 35-90: The runTest method currently posts to the endpoint once
without mocks and then again inside the if self.method == 'POST' mocked block;
remove the first unmocked POST/assertion sequence (the initial self.tester.post
+ json.loads checks at the top of runTest) so the test only exercises the
endpoint within the with patch(...) nested mocks; keep the mocked block that
patches pgadmin.tools.expl_pgsql.get_preference_value, is_valid_url, and
send_post_request and preserve both expected_success branches there (using
json.dumps(self.data) for the POST when needed) so assertions still verify
response_data['success'] under the mocked conditions.
In `@web/pgadmin/translations/ru/LC_MESSAGES/messages.po`:
- Around line 16403-16406: The PO entry for the msgid "JSON payload must be an
object, not null, array, or scalar value" has an empty msgstr; open the
translations file and add the appropriate Russian translation into the msgstr
for that entry so the validation error is localized (target the PO entry
containing that exact msgid shown in the diff, which is referenced from
pgadmin/tools/expl_pgsql/__init__.py).
- Around line 16410-16418: Replace the misspelled Russian word "Некорретный"
with the correct spelling "Некорректный" in the translation entries for the
msgid "Invalid API endpoint URL. Only HTTP/HTTPS URLs are allowed." and the
multi-line msgid beginning "The provided API endpoint is not valid. Only
HTTP/HTTPS URLs are permitted." in messages.po so both msgstr occurrences are
corrected; search for those exact msgid strings (or inspect the translations
associated with pgadmin/tools/expl_pgsql/__init__.py references) and update the
two msgstr values accordingly.
In `@web/pgadmin/utils/__init__.py`:
- Line 386: Replace the string-concatenation logging call so it uses
parameterized logging: locate the current_app.logger.warning call that
concatenates 'Invalid binary path.' with binary_path and change it to pass the
format string and binary_path as parameters (use the current_app.logger.warning
method and the binary_path variable) so the message is logged as a parameterized
log entry rather than via string concatenation.
---
Nitpick comments:
In `@web/pgadmin/static/js/components/ReactCodeMirror/index.jsx`:
- Line 150: The keymap is registering formatSQL via
[parseShortcutValue(editorPrefs.format_sql, true)] but the callback is not
awaiting the now-async formatSQL; update the keymap handler to be an async
function that awaits formatSQL(...) and properly handle/propagate errors (e.g.,
try/catch or rethrow) so the UI can reflect completion or failures when invoking
formatSQL from the keyboard shortcut.
In `@web/pgadmin/tools/expl_pgsql/__init__.py`:
- Around line 239-254: The headers dict in send_post_request includes a
non-standard "Method" header which should be removed; update the headers used to
build urllib.request.Request in send_post_request to omit the "Method" key (keep
"Content-Type" and "User-Agent"), relying on urllib.request.Request/no302opener
to set the POST method via the presence of data, and verify behavior of
no302opener.open(req, timeout=10) remains unchanged.
In `@web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/formatSQL.js`:
- Around line 4-35: The formatSQL function currently wraps api.post in a new
Promise (anti-pattern); refactor by using the existing async function to await
the API call: call getApiInstance().post(url_for('expl_pgsql.formatSQL'),
JSON.stringify({query_src: sql})) inside a try/catch, check res.data?.data,
JSON.parse it to extract btf_query and btf_query_text, return btf_query_text
when different and throw an Error with the fallback btf_query_text when
identical (server error case), and in catch log the error (console.error) and
rethrow (throw err or new Error(err.message)); reference functions/identifiers:
formatSQL, getApiInstance, url_for, api.post, and fields btf_query /
btf_query_text.
In `@web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsx`:
- Around line 1042-1068: The useEffect registers showExplPgsql but uses external
symbols (api, layoutDocker, gettext) without listing them as dependencies; fix
by either adding api, layoutDocker, and gettext to the dependency array of the
useEffect that declares showExplPgsql (so the listener always uses current
references) or, if those values are truly stable (from context/singleton), add a
brief code comment and an eslint-disable-next-line react-hooks/exhaustive-deps
before the dependency array to silence the rule; ensure you update the
registration/deregistration using the same showExplPgsql function name and keep
eventBus and QUERY_TOOL_EVENTS references intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e036953d-122a-4d50-8b4e-29c2a135a9bc
⛔ Files ignored due to path filters (10)
docs/en_US/images/expl_pgsql_analyze.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_diagram.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_personal.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_piechart.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_plantree.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_recs.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_schema.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_stats.pngis excluded by!**/*.pngdocs/en_US/images/expl_pgsql_tilemap.pngis excluded by!**/*.pngdocs/en_US/images/preferences_expl_pgsql.pngis excluded by!**/*.png
📒 Files selected for processing (19)
docs/en_US/developer_tools.rstdocs/en_US/expl_pgsql.rstdocs/en_US/preferences.rstweb/pgadmin/messages.potweb/pgadmin/static/js/components/ReactCodeMirror/index.jsxweb/pgadmin/tools/__init__.pyweb/pgadmin/tools/expl_pgsql/__init__.pyweb/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/formatSQL.jsweb/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/index.jsxweb/pgadmin/tools/expl_pgsql/tests/__init__.pyweb/pgadmin/tools/expl_pgsql/tests/test_explain_functionality.pyweb/pgadmin/tools/expl_pgsql/tests/test_format_sql.pyweb/pgadmin/tools/expl_pgsql/tests/test_preferences.pyweb/pgadmin/tools/expl_pgsql/tests/test_status_endpoint.pyweb/pgadmin/tools/expl_pgsql/tests/test_url_validation.pyweb/pgadmin/tools/sqleditor/static/js/components/QueryToolConstants.jsweb/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/translations/ru/LC_MESSAGES/messages.poweb/pgadmin/utils/__init__.py
web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/index.jsx
Outdated
Show resolved
Hide resolved
web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/index.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py (1)
37-65:⚠️ Potential issue | 🔴 CriticalCritical bug: Test assertions execute outside the mock context.
The
with patch(...)block exits at Line 46 (after setting up mock return values), but all test assertions (Lines 48-65) execute outside this context due to incorrect indentation. The mocks are not active whenself.tester.post()is called, so tests hit the real endpoint or fail unpredictably.🐛 Proposed fix: Move assertions inside the mock context
def runTest(self): """Run test case.""" with patch('pgadmin.tools.expl_pgsql.get_preference_value') as mock_pref, \ patch('pgadmin.tools.expl_pgsql.is_valid_url') as mock_valid, \ patch('pgadmin.tools.expl_pgsql.send_post_request') as mock_send: mock_pref.return_value = 'https://explain.tensor.ru' mock_valid.return_value = True mock_send.return_value = (False, json.dumps({ 'btf_query': '<span class=\'sql_keyword\'>SELECT</span>', 'btf_query_text': 'SELECT\\n\\t*\\nFROM\\n\\ttest_table\\nWHERE\\n\\tid = 1;' })) - if self.expected_success: - response = self.tester.post( - self.url, - data=json.dumps(self.data), - content_type='application/json' - ) - self.assertEqual(response.status_code, 200) - response_data = json.loads(response.data.decode('utf-8')) - self.assertTrue(response_data['success']) - else: - response = self.tester.post( - self.url, - data=self.data, - content_type='application/json' - ) - self.assertEqual(response.status_code, 200) - response_data = json.loads(response.data.decode('utf-8')) - self.assertFalse(response_data['success']) + if self.expected_success: + response = self.tester.post( + self.url, + data=json.dumps(self.data), + content_type='application/json' + ) + self.assertEqual(response.status_code, 200) + response_data = json.loads(response.data.decode('utf-8')) + self.assertTrue(response_data['success']) + else: + response = self.tester.post( + self.url, + data=self.data, + content_type='application/json' + ) + self.assertEqual(response.status_code, 200) + response_data = json.loads(response.data.decode('utf-8')) + self.assertFalse(response_data['success'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py` around lines 37 - 65, The test assertions are running outside the patch context so mocks (patch('pgadmin.tools.expl_pgsql.get_preference_value'), patch('pgadmin.tools.expl_pgsql.is_valid_url'), patch('pgadmin.tools.expl_pgsql.send_post_request')) are not active when self.tester.post(...) is invoked; fix this by moving the branches that call self.tester.post(...) and the subsequent assertions (the blocks that reference self.url, self.data, self.expected_success and inspect response.status_code/response_data['success']) so they execute inside the with patch(...) as mock_pref, ...: block, ensuring mock_pref.return_value, mock_valid.return_value and mock_send.return_value are in effect during the POST and assertions.
🧹 Nitpick comments (2)
web/pgadmin/tools/expl_pgsql/__init__.py (1)
138-147: Variabledatais reused, reducing readability.The
datavariable is first the request payload (Line 112), then overwritten with the API response (Line 139). While functionally correct, this shadowing reduces code clarity.♻️ Proposed fix for clarity
api_url = explain_postgresql_api + '/beautifier-api' - is_error, data = send_post_request(api_url, data) + is_error, response_data = send_post_request(api_url, data) if is_error: return make_json_response( success=0, - errormsg=data, + errormsg=response_data, info=gettext('Failed to post data to the Explain PostgreSQL API'), ) - return make_json_response(success=1, data=data) + return make_json_response(success=1, data=response_data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/expl_pgsql/__init__.py` around lines 138 - 147, The code reuses the variable name data for both the outgoing payload and the API response which reduces readability; change the original request payload variable (currently named data at its definition and passed into send_post_request) to a clearer name like payload or request_payload, and assign the result of send_post_request to a different variable (e.g., response_data or api_response), then update the make_json_response calls and the send_post_request invocation to use these new names so that send_post_request(api_url, request_payload) returns (is_error, response_data) and the final return uses response_data.web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py (1)
12-12: Unused import:unittest.The
unittestmodule is imported but not used since the test class extendsBaseTestGenerator.♻️ Proposed fix
-import unittest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py` at line 12, Remove the unused import of the unittest module from web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py; the test class inherits from BaseTestGenerator (not unittest.TestCase) so delete the line "import unittest" and ensure there are no remaining references to unittest in the file (update imports if needed to keep only used test helpers).
🤖 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/expl_pgsql/__init__.py`:
- Around line 199-206: The code currently rejects Location values that are
absolute URLs; update the handling around response_data and res_data so absolute
URLs from Location are accepted: if response_data is an absolute URL (e.g.,
startswith('http://') or 'https://') set res_data = response_data, otherwise
keep the existing behavior of concatenating explain_postgresql_api +
response_data; keep using make_json_response(success=1, data=res_data) and
retain the error branch only for completely unexpected formats (neither absolute
URL nor a relative path starting with '/'); reference response_data and
explain_postgresql_api when making this change.
In `@web/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/index.jsx`:
- Around line 37-59: The effect in useEffect uses the variables plans and sql
(and calls getApiInstance/post) but only lists plans in its dependency array,
causing stale sql to be sent when sql changes; update the dependency array for
the useEffect that contains the API call (the block that posts to
url_for('expl_pgsql.explain') and calls setData/setError/setIsLoading) to
include sql (and any other external stable references if needed) so the effect
reruns when sql changes.
---
Duplicate comments:
In `@web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py`:
- Around line 37-65: The test assertions are running outside the patch context
so mocks (patch('pgadmin.tools.expl_pgsql.get_preference_value'),
patch('pgadmin.tools.expl_pgsql.is_valid_url'),
patch('pgadmin.tools.expl_pgsql.send_post_request')) are not active when
self.tester.post(...) is invoked; fix this by moving the branches that call
self.tester.post(...) and the subsequent assertions (the blocks that reference
self.url, self.data, self.expected_success and inspect
response.status_code/response_data['success']) so they execute inside the with
patch(...) as mock_pref, ...: block, ensuring mock_pref.return_value,
mock_valid.return_value and mock_send.return_value are in effect during the POST
and assertions.
---
Nitpick comments:
In `@web/pgadmin/tools/expl_pgsql/__init__.py`:
- Around line 138-147: The code reuses the variable name data for both the
outgoing payload and the API response which reduces readability; change the
original request payload variable (currently named data at its definition and
passed into send_post_request) to a clearer name like payload or
request_payload, and assign the result of send_post_request to a different
variable (e.g., response_data or api_response), then update the
make_json_response calls and the send_post_request invocation to use these new
names so that send_post_request(api_url, request_payload) returns (is_error,
response_data) and the final return uses response_data.
In `@web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py`:
- Line 12: Remove the unused import of the unittest module from
web/pgadmin/tools/expl_pgsql/tests/test_format_sql.py; the test class inherits
from BaseTestGenerator (not unittest.TestCase) so delete the line "import
unittest" and ensure there are no remaining references to unittest in the file
(update imports if needed to keep only used test helpers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f70db9a2-d6b6-44af-b7d4-a3a8669c3649
📒 Files selected for processing (7)
web/pgadmin/messages.potweb/pgadmin/static/js/components/ReactCodeMirror/index.jsxweb/pgadmin/tools/expl_pgsql/__init__.pyweb/pgadmin/tools/expl_pgsql/static/js/ExplainPostgreSQL/index.jsxweb/pgadmin/tools/expl_pgsql/tests/test_format_sql.pyweb/pgadmin/translations/ru/LC_MESSAGES/messages.moweb/pgadmin/translations/ru/LC_MESSAGES/messages.po
🚧 Files skipped from review as they are similar to previous changes (2)
- web/pgadmin/translations/ru/LC_MESSAGES/messages.po
- web/pgadmin/messages.pot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a new module that integrates with the Explain PostgreSQL API (https://explain.tensor.ru/).
It provides users with enhanced SQL query formatting and in-depth query plan analysis directly within pgAdmin4.
Key Features:
External Plan Analysis: Send query plans to the external API for a more detailed analysis.
SQL Formatting: Utilize the API's capabilities to automatically format and beautify SQL queries.
Note:
This is a resubmission of the previously closed pull request [#9726] with all requested fixes applied.
Summary by CodeRabbit
New Features
Documentation
Internationalization