Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Registration can leave partial or stale SQL definitions pointing to an unavailable endpoint.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Prevents interactive notebook sessions from overwriting published or other-session Python UDF registrations.
Changes:
- Adds service URL parsing and ownership classification.
- Introduces ownership-aware interactive registration.
- Adds unit tests and registration logging.
| File | Description |
|---|---|
singlestoredb/functions/ext/function_url.py |
Adds URL parsing and classification helpers. |
singlestoredb/functions/ext/asgi.py |
Implements interactive registration safeguards. |
singlestoredb/apps/_python_udfs.py |
Uses the new registration path and logs names. |
singlestoredb/tests/test_function_url.py |
Tests URL parsing and ownership classification. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def register_interactive_functions( | ||
| self, | ||
| *connection_args: Any, | ||
| **connection_kwargs: Any, | ||
| ) -> None: |
| for _key, (_endpoint, info) in self.endpoints.items(): | ||
| sig = info['signature'] | ||
| sql_name = sig['name'] | ||
| existing = self._show_create_service_url(cur, sql_name) | ||
| try: | ||
| action = classify_interactive_registration( | ||
| existing, self.url, | ||
| ) | ||
| except ValueError as exc: | ||
| raise RuntimeError( | ||
| f'Cannot register SQL function `{sql_name}`: {exc}', | ||
| ) from exc | ||
| create_sqls = [ | ||
| signature_to_sql( | ||
| sig, | ||
| url=self.url, | ||
| data_format=self.data_format, | ||
| app_mode=self.app_mode, | ||
| replace=(action == 'replace'), | ||
| database=self.function_database or None, | ||
| ), | ||
| ] | ||
| for stmt in create_sqls: | ||
| cur.execute(stmt) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 18a98b1. Configure here.
| args = getattr(exc, 'args', ()) | ||
| if args and args[0] == ER.FUNCTION_NOT_DEFINED: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Missing-function error code too narrow
High Severity
is_function_not_defined only treats ER.FUNCTION_NOT_DEFINED (1128) as “name is missing.” SHOW CREATE FUNCTION on an absent stored or external function commonly raises ER.SP_DOES_NOT_EXIST (1305) instead. _show_create_service_url then re-raises, so first-time interactive registration never reaches CREATE.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 18a98b1. Configure here.
kesmit13
left a comment
kesmit13
left a comment
There was a problem hiding this comment.
I don't see anything obviously incorrect. Just make sure to address the AI reviews.
Summary
Interactive
run_udf_app()usedCREATE OR REPLACEby name, so a notebook session could steal a published Python UDF SQL function onto/pythonudfs/<session>/interactive/. After the session died the function failed.Interactive registration now reads
SHOW CREATE, then only creates a missing name or replaces a function that already points at this session URL. Published and other-session functions are left alone. The_testsuffix is unchanged. CLI / mmap still useregister_functions(replace=True).Pairs with Helios #28074 (Portal republish / idle-wake repair).
Test Plan
run_udf_app()on the same names, confirm published SQL is unchanged, then republish and confirm the published URL is restoredNote
Medium Risk
Changes how interactive sessions mutate MCDB external function definitions; misclassification could leave wrong URLs or block registration, but published UDFs are explicitly protected.
Overview
Interactive notebook Python UDF registration no longer overwrites published SQL functions.
run_udf_app()now callsregister_interactive_functions()instead ofregister_functions(replace=True), and logs which SQL names were registered.For each endpoint, registration uses
SHOW CREATE FUNCTIONto read the existing MANAGED/SERVICE URL (via newfunction_urlhelpers). It creates missing names, replaces only when the function already points at this session’s URL, and fails if a name is bound to a published or other-session URL. Stale functions still tied to this session URL but no longer in the app are dropped._locate_app_functionsuses the same URL parsing for matching.CLI / non-interactive paths still use
register_functions(replace=True). Unit tests cover URL extraction, classification, and interactive register preflight / stale cleanup.Reviewed by Cursor Bugbot for commit ab113be. Bugbot is set up for automated code reviews on this repo. Configure here.