Repository: ploomber/jupysql (python)

Issue:
improve error message when passing a non-identifier to start a connection
we allow users to pass a connection variable to connect to a db. example:

```python
%load_ext sql

import duckdb
conn = duckdb.connect()

%sql conn
```

however, in some cases, the connection object might be part of a dictionary, list or object. examples:

```python
%sql object.property
%sql dictionary["key"]
%sql some_list[0]
```

Trying any of the above throws an error because our parser thinks it's SQL code. Instead, we should provide a more informative error:

```
"object.property" is not a valid connection identifier. Please pass the variable's name directly, as passing object attributes, dictionaries `some_dict[key]` or lists `some_list[0]` won't work. Example: `%sql connection`
```

so we need a way to identity such cases, we can use the following heuristics:

- the magic gets a single argument (no spaces between characters) - because valid SQL code will always have spaces
- we can check if the argument is not a valid identifier using `argument.isidentifier()` but it is valid Python code (using ast.parse)

if those conditions hold, then we can raise the error.

Pull request:
error when passing a non-identifier to connect
## Describe your changes

When a connection is pass through dictionary, list, or object, it raises an error with a more detailed message.

For instance, when `%sql some_list[0]` is passed to connect, it raises the following error.
```
UsageError: 'some_list[0]' is not a valid connection identifier. Please pass the variable's name directly, as passing object attributes, dictionaries or lists won't work.
```

## Issue number

Closes #764 

## Checklist before requesting a review

- [x] Performed a self-review of my code
- [x] Formatted my code with [`pkgmt format`](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#linting-formatting)
- [x] Added [tests](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#testing) (when necessary).
- [x] Added [docstring](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#documenting-changes-and-new-features) documentation and update the [changelog](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#changelog) (when needed)



<!-- readthedocs-preview jupysql start -->
----
:books: Documentation preview :books:: https://jupysql--821.org.readthedocs.build/en/821/

<!-- readthedocs-preview jupysql end -->

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8bdd033..f986fb1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 * [Feature] Automatically connect if the `dsn_filename` (defaults to `~/.jupysql/connections.ini`) contains a `default` section
 * [Feature] Add `%sqlcmd connect` to see existing connections and create new ones (#632)
+* [Fix] Improve error when passing a non-identifier to start a connection (#764)
 * [Fix] Display a warning (instead of raising an error) if the `default` connection in the `.ini` file cannot start
 
 ## 0.10.0 (2023-08-19)
diff --git a/src/sql/command.py b/src/sql/command.py
index 0c6cd4b..2ff4900 100644
--- a/src/sql/command.py
+++ b/src/sql/command.py
@@ -7,6 +7,7 @@ from sqlalchemy.engine import Engine
 from sql import parse, exceptions
 from sql.store import store
 from sql.connection import ConnectionManager, is_pep249_compliant
+from sql.util import validate_nonidentifier_connection
 
 
 class SQLPlotCommand:
@@ -81,6 +82,14 @@ class SQLCommand:
             final = store.render(self.parsed["sql"], with_=self.args.with_)
             self.parsed["sql"] = str(final)
 
+        if (
+            one_arg
+            and self.sql
+            and not (add_conn or add_alias)
+            and not (self.args.persist_replace or self.args.persist or self.args.append)
+        ):
+            validate_nonidentifier_connection(self.sql.split(" ")[0])
+
     @property
     def sql(self):
         """
diff --git a/src/sql/util.py b/src/sql/util.py
index cb61e3d..31fc260 100644
--- a/src/sql/util.py
+++ b/src/sql/util.py
@@ -7,6 +7,7 @@ from sql import exceptions, display
 import json
 from pathlib import Path
 from ploomber_core.dependencies import requires
+import ast
 
 try:
     import toml
@@ -527,3 +528,29 @@ def validate_mutually_exclusive_args(arg_names, args):
             f"{pretty_print(specified_args)} are specified. "
             "You can only specify one of them."
         )
+
+
+def validate_nonidentifier_connection(arg):
+    """
+    Raises UsageError if a connection is passed to `%sql/%%sql` through
+    object property, list, or dictionary.
+
+    Parameters
+    ----------
+    arg : str
+        argument to check whether it is a valid connection or not
+    """
+    if not arg.isidentifier() and is_valid_python_code(arg):
+        raise exceptions.UsageError(
+            f"'{arg}' is not a valid connection identifier. "
+            "Please pass the variable's name directly, as passing "
+            "object attributes, dictionaries or lists won't work."
+        )
+
+
+def is_valid_python_code(code):
+    try:
+        ast.parse(code)
+        return True
+    except SyntaxError:
+        return False


Test patch:
diff --git a/src/tests/test_magic.py b/src/tests/test_magic.py
index 0954bc1..c65ebe5 100644
--- a/src/tests/test_magic.py
+++ b/src/tests/test_magic.py
@@ -1209,6 +1209,46 @@ def test_error_on_invalid_connection_string_duckdb(ip_empty, clean_conns):
     assert invalid_connection_string_duckdb.strip() == str(excinfo.value)
 
 
+@pytest.mark.parametrize(
+    "establish_non_identifier, non_identifier",
+    [
+        (
+            "conn_in_lst = [conn]",
+            "conn_in_lst[0]",
+        ),
+        (
+            "conn_in_dict = {'conn1': conn}",
+            "conn_in_dict['conn1']",
+        ),
+        (
+            """
+class ConnInObj(object):
+    def __init__(self, conn):
+        self.conn1 = conn
+
+conn_in_obj = ConnInObj(conn)
+""",
+            "conn_in_obj.conn1",
+        ),
+    ],
+)
+def test_error_on_passing_non_identifier_to_connect(
+    ip_empty, establish_non_identifier, non_identifier
+):
+    ip_empty.run_cell("import duckdb; conn = duckdb.connect();")
+    ip_empty.run_cell(establish_non_identifier)
+
+    with pytest.raises(UsageError) as excinfo:
+        ip_empty.run_cell(f"%sql {non_identifier}")
+
+    assert excinfo.value.error_type == "UsageError"
+    assert (
+        f"'{non_identifier}' is not a valid connection identifier. "
+        "Please pass the variable's name directly, as passing "
+        "object attributes, dictionaries or lists won't work."
+    ) in str(excinfo.value)
+
+
 def test_jupysql_alias():
     assert SqlMagic.magics == {
         "line": {"jupysql": "execute", "sql": "execute"},