5
\$\begingroup\$

I'd like to wrap all my sqlite statement functions with a decorator which collects the statement and the lists of values I will inject in place of placeholders in my statement (to avoid sql injection). Code looks like this:

#decorator def dbexecute(func): def withcon(*args): conn = sqlite3.connect(sqlite_file_name) conn.row_factory = sqlite3.Row with conn: fa = func(*args) if isinstance(fa, tuple): s, v = fa else: s, v = fa, () out = conn.execute(s, v).fetchall() conn.close() return out return withcon #statement functions that are decorated @dbexecute def create_user(user_settings: dict): user_settings['id'] = str(uuid.uuid4()) columns = tuple([key for (key,value) in user_settings.items()]) values = tuple([value for (key,value) in user_settings.items()]) qmarks = tuple(['?']*len(values)) statement = f"INSERT INTO users {columns} VALUES {qmarks}".replace("'","") return statement, values @dbexecute def get_all_users(): statement = "SELECT * FROM users" return statement 

First function returns values to replace the question marks with, but the second not. I had to handle this directly in the decorator, but I am not sure this is the good way to do it or maybe there is a more pythonic way than an if...else... block that checks the type of the inputs args.

Thanks for your feedback!

\$\endgroup\$
1
  • \$\begingroup\$The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles.\$\endgroup\$CommentedFeb 22, 2023 at 14:23

1 Answer 1

4
\$\begingroup\$

It's cool that you are experimenting, but from my point of view this is just trying to be fancy for no good reason.

How is this better than just executing the code safely at end of your create_user and get_all_users by calling designated fuction to that similar to what your decorator does?

Imho the result is the same, decomposition is the same, only the code is less confusing.

What I don't like about it the most, is the confusing naming. The method names suggest that they are "getting" or "creating" something, but in fact they are only creating input for the sql query.

If you insist on your decorator, I would say the cleanest solution would be to return empty tuple in get_all_users to be consistent with your interface.

\$\endgroup\$
3
  • \$\begingroup\$Maybe you're right a decorator is overkill and not very useful here, but I was thinking, in what cases you must use a decorator instead of just calling the decorator function inside the caller? Because you don't have direct access to the caller definition?\$\endgroup\$
    – FTG
    CommentedFeb 22, 2023 at 10:17
  • \$\begingroup\$I would say where you want to modify the behaviour the function, but the function works even without the decorator. Your functions alone without the decorator don't do anything. Good examples of decorators are in django views. You can mark @login_required on view method and it automatically does redirect if the user is not logged in.\$\endgroup\$
    – K.H.
    CommentedFeb 22, 2023 at 10:24
  • 1
    \$\begingroup\$Noted! Thanks for the details!\$\endgroup\$
    – FTG
    CommentedFeb 22, 2023 at 12:49

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.