Validating CLI arguments

[edublancas]

I noticed that passing random arguments to %sql doesn't raise any errors:

(jupysql)  eduardo@macbookair dev/jupysql (display *) » ipython
Python 3.10.11 (main, Apr 20 2023, 13:58:42) [Clang 14.0.6 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.13.2 -- An enhanced Interactive Python. Type '?' for help.

In [1]: %load_ext sql

In [2]: %sql duckdb://

In [3]: %sql --stuff --more-stuff
Running query in 'duckdb://'

In [4]:

We should throw an UsageError instead

[edublancas]

https://github.com/ploomber/jupysql/blob/master/src/sql/magic.py

[tl3119]

To solve this issue, we need to focus on the arguments part on the execute() method in magic.py, since we don't want random arguments to be passed.

@magic_arguments(allow_interspersed_args=False)
@handle_arguments
def handle_arguments(self, *args):
    if len(args) > 0:
        raise UsageError("Unrecognized argument(s): {}".format(args))

@line_magic("sql")
@cell_magic("sql")
# Existing code

In this way, we prevents unrecognized arguments from being treated as positional arguments and to handle the error we got.
Does this method works fine?
BTW, how do we test for this?

[edublancas]

checkout the existing tests: https://github.com/ploomber/jupysql/blob/master/src/tests/test_magic.py

the solution is a bit tricky though (this project is a fork of ipython-sql so we inherited a bunch of legacy code). I forgot to put this relevant link: https://jupysql.ploomber.io/en/latest/intro.html#considerations

so for the line magic (%sql) things are a bit complicated because the same line mixes arguments with SQL comments. but I think we can start by applying this to the cell magic (%%sql), in a cell magic, there is no ambiguity between the arguments and the SQL. examples:

line magic:

%sql  [arguments go here but SQL code also goes here]

cell magic:

%%sql  [arguments go here]
[SQL code goes here]

[tl3119]

So we need to deal this problem by line magic and cell magic separately. One way is to write function immediately after @line_magic("sql") and @cell_magic("sql") to indicate each function deal different situation:

@line_magic("sql")
  def check_line(self, line):
      pass
@cell_magic("sql")
  def check_cell(self, line):
      # Handle the arguments for cell_magic
      args = parse_argstring(self.check_cell, line)

      if args.unrecognized_args:
          raise UsageError("Unrecognized argument(s): {}".format(args.unrecognized_args))

Since in the cell magic, SQL code needs to be at the second line, so we can parse the first line and to check whether we get unrecognized arguments, if we we get unrecognized arguments, we raise UsageError.
Is this logic correct?

[edublancas]

yeah, this makes sense: splitting into two. the root problem is that both the line and cell lines are executing the same function. but the validation logic assumptions are different (see #550)

so go ahead with the approach you suggested and we'll see how it goes!

[tl3119]

Sure! I have submitted a PR request. Some tests failed, I am blocked by these tests. Do you have any suggestion for these error?

[tl3119]

i have some ideas now, i will try to fix that

[edublancas]

are you able to run the tests locally? that'll make things easier to debug

this will be useful: https://ploomber.io/blog/open-source/ - checkout the from IPython import embed and pdb parts.

[tl3119]

I am confused with SQL comments and unrecognized arguments. Is that all the arguments should begin with "--" and I notice that "If you try to pass an unsupported argument, like --lutefisk, it will be interpreted as a SQL comment and will not throw an unsupported argument exception"
"--lutefisk" can also be interpreted as a SQL comment, so does that means we want "--lutefisk" be a unrecognized argument or it is fine to be a SQL comment? How do we distinguish between unrecognized argument and SQL comment? For example:
%sql --lutefisk SELECT ...

[edublancas]

How do we distinguish between unrecognized argument and SQL comment?

we cannot do this in the line magic (%sql - single percentage sign), so we won't implement this there.

but we can do it in the cell magic (%%sql - double percentage sign) because the SQL content begins in the second line. so we can assume the first line only has arguments

[tl3119]

Sure, that makes sense. I have submitted a new pull request. I added a new test, and all the tests pass locally in my computer.