Fix syntax error editing SQL functions/procedures with a 'return' substring in the body#10061
Open
dpage wants to merge 2 commits into
Open
Fix syntax error editing SQL functions/procedures with a 'return' substring in the body#10061dpage wants to merge 2 commits into
dpage wants to merge 2 commits into
Conversation
…string in the body. The SQL-standard body detection used a regex that matched 'return' anywhere in the body, so a plain SQL body containing a RETURNING clause (or an identifier like 'returned_value') was wrongly treated as a SQL-standard (BEGIN ATOMIC / RETURN) body. The generated CREATE OR REPLACE statement then dropped the AS $BODY$ ... $BODY$ wrapper, producing invalid SQL and a syntax error on save. Anchor the RETURN form to the start of the body so only genuine SQL-standard bodies are detected. Add unit tests for the detection. Closes pgadmin-org#10059
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefines _is_function_def_sql_standard to require RETURN at the start of the body, adds unit tests for edge cases, and updates release notes referencing Issue ChangesSQL-standard function body detection fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_is_function_def_sql_standard.py (1)
25-65: ⚡ Quick winAdd a regression case for “RETURN not at body start”.
Please add a negative scenario like
prosrc='SELECT 1;\nRETURN 2;'(expectedFalse) to lock the “start of body only” contract and prevent future multiline-anchor regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_is_function_def_sql_standard.py` around lines 25 - 65, Add a negative scenario to the existing scenarios list to assert that a RETURN not at the start of the body is not considered SQL-standard: insert an entry into the scenarios list (near the other SQL-standard cases) with data=dict(lanname='sql', prosrc='SELECT 1;\nRETURN 2;') and expected=False; ensure the tuple follows the same format as other cases so the test covering the function that consumes scenarios will catch multiline-anchor regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py`:
- Line 1768: The regex r"^\s*return\b" is anchored to line-start under
re.MULTILINE and should instead match only at the start of the entire prosrc
body; replace the caret anchor with a true start-of-string anchor (e.g. \A) so
the check on prosrc (where the pattern is compiled with re.MULTILINE) detects
only bodies that begin with 'return'; apply the same change to the other
occurrences noted around the re.MULTILINE usage (lines referenced in the
comment).
---
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_is_function_def_sql_standard.py`:
- Around line 25-65: Add a negative scenario to the existing scenarios list to
assert that a RETURN not at the start of the body is not considered
SQL-standard: insert an entry into the scenarios list (near the other
SQL-standard cases) with data=dict(lanname='sql', prosrc='SELECT 1;\nRETURN 2;')
and expected=False; ensure the tuple follows the same format as other cases so
the test covering the function that consumes scenarios will catch
multiline-anchor regressions.
🪄 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: 11218534-b3b6-4a8a-ad33-42e91454abcf
📒 Files selected for processing (3)
docs/en_US/release_notes_9_16.rstweb/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_is_function_def_sql_standard.py
Address review feedback: \A anchors the RETURN form to the start of the body so a multi-line plain SQL body whose later line begins with 'return' is not matched under re.MULTILINE.
There was a problem hiding this comment.
Pull request overview
Fixes an SQL generation bug when saving SQL-language functions/procedures whose body merely contains the substring return, preventing them from being misclassified as SQL-standard bodies and producing invalid DDL.
Changes:
- Tighten SQL-standard body detection by requiring
RETURNto appear at the start of the function/procedure body. - Add regression-focused unit tests for
_is_function_def_sql_standardcoveringRETURN,BEGIN ATOMIC, and false-positive cases likeRETURNING/identifiers. - Document the fix in the v9.16 release notes (Issue #10059).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/pgadmin/browser/server_groups/servers/databases/schemas/functions/init.py | Refines regex logic for detecting SQL-standard function/procedure bodies to avoid false positives. |
| web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_is_function_def_sql_standard.py | Adds regression unit tests validating correct classification of SQL-standard vs plain SQL bodies. |
| docs/en_US/release_notes_9_16.rst | Adds a bug-fix release note entry for Issue #10059. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1761
to
+1765
| # valid regex, these combination a must in definition to detect a | ||
| # standard sql or pure sql | ||
| # standard sql or pure sql. The RETURN form must start the body | ||
| # (as a keyword) so that a plain SQL body which merely contains the | ||
| # substring "return" (e.g. a "RETURNING" clause or an identifier | ||
| # like "returned_value") is not mistaken for a SQL-standard body. |
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
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.
Summary
return(e.g. anINSERT ... RETURNINGclause, or an identifier likereturned_value) generated invalid SQL on save, failing with a syntax error.FunctionView._is_function_def_sql_standardused the regex(?=return), which matchesreturnanywhere in the body, so a plain SQL body was wrongly classified as a SQL-standard (BEGIN ATOMIC/RETURN) body. Theupdate.sql(and related) templates then emitted the body without theAS $BODY$ ... $BODY$wrapper, producing invalid SQL.RETURNform to the start of the body (^\s*return\b) so only genuine SQL-standard bodies are detected. The change is in the shared detection helper, so it covers both functions and procedures.Test plan
test_is_function_def_sql_standard.pycovering:BEGIN ATOMICbody,RETURNform (with/without leading whitespace),RETURNINGclause,returned_valueidentifier, plainSELECT, non-sqllanguage, and empty body — all pass.pycodestyleclean on changed files.Closes #10059
Summary by CodeRabbit
Bug Fixes
Tests
Documentation