v1a0 / sqllex

The most pythonic ORM (for SQLite and PostgreSQL). Seriously, try it out!
https://v1a0.github.io/sqllex
GNU General Public License v3.0
92 stars 8 forks source link

FEATURE | Handle args and kwargs at the same time for insert-like methods #30

Closed damnkrat closed 3 years ago

damnkrat commented 3 years ago

Description

Found bug in registration where registration date is not writing in db.

Code

db.insert('users', uid, from_user=from_user,
          registration_date=datetime.now().strftime("%Y-%m-%d %H:%M"))

Research

Shout to solve it myself, but it's 3:00 pm, maybe tomorrow or not me So I discovered that this bug is in sqlite3x.py on 1119 - 1127

if args:
    _columns = self.get_columns(table=TABLE)
    _columns, args = crop(_columns, args)
    insert_values = args

elif kwargs:
    _columns = tuple(kwargs.keys())
    insert_values = list(kwargs.values())

else:
    raise ArgumentError(args_kwargs="Unset", error="No data to insert")

btw 1120 might be _columns, args = crop(self.get_columns(table=TABLE), args)

It handles only args or kwargs but not both

v1a0 commented 3 years ago

Hey @dannkunt, thank you for opening this issue.

Well, for me this doesn't seems like a bug, just this insert way not supporting (now), and that's why.

As far as I can understand, table 'users' in your database have structure like this (or some kind of similar):

db.create_table(
    'users',
    {
        'uid': [INTEGER, UNIQUE, NOT_NULL],
        'from_user': TEXT,
        'registration_date': TEXT
    }
)

If you know in which column which data needed to add, you have to specify this strictly through the **kwargs. It will be much safer and... idk... reasonable, I guess. Like this:

db.insert(
    'users',
    uid=uid,        # added "uid="
    from_user=from_user,
    registration_date=datetime.now().strftime("%Y-%m-%d %H:%M")
)

Only args

If you are lazy ass you can just keep the right order of the columns.
Like this:

db.insert(
    'users',
    uid,
    from_user,
    datetime.now().strftime("%Y-%m-%d %H:%M")
)

In this case sqllex will try 2 ways to insert your data (if 1'st one will fail, runs 2'nd):

1'st way - Just trusting insertion

INSERT INTO users VALUES (123, "User",  '2021-06-17 13:37')

If table have more than 3 columns, this insert will failed with sqlite3.OperationalError

2'nd way - Column prediction

If first insert failed, sqlx getting list of tables columns and trying fill it up. It this case we have ['uid', 'from_user', 'registration_date']. And generating script:

INSERT INTO users (uid, from_user, registration_date) VALUES (123, "User",  '2021-06-17 13:37')

Even Even if table have more than 3 columns, insert will pass successfully, due to columns specified.

v1a0 commented 3 years ago

Now about case args + kwargs. Imagine table have structure like this:

db.create_table(
    'users',
    {
        'id': [INTEGER, UNIQUE, NOT_NULL],
        'first_name': [TEXT, NOT_NULL],
        'second_name': [TEXT, NOT_NULL],
        'age': INTEGER
    }
)

And we inserting data as args + kwargs

db.insert(
    'users',
    1,
    first_name="Alex",
    second_name="Flex",
    age=33
)

In which column insert 1? In which is empty, right?

Ok maybe, but what have to do in this case:

db.insert(
    'users',
    "Flex", 1
    first_name="Alex",
    age=33
)

Value "Flex" will be inserted in id column, and 1 as second_name. So we need check datatype, right? if value type int inset it in INTEGER type column. Nope, still bad idea.

db.insert(
    'users',
    33, 1
    first_name="Alex",
    second_name="Flex"
)

Is Alex have id 33 and he/she is 1 year old, or id=1 and age=33? There is no way to decide, too many chances insert wrong data in wrong column. And I don't really see well way to create safe and not fucked insert method with args and kwarg at the same time

If you have any ideas, I'm open to discuss

damnkrat commented 3 years ago

The way to do it is very simple, it should work like a normal python function.

db.insert( 'users', 1, first_name="Alex", second_name="Flex", age=33 ) In which column insert 1? In which is empty, right?

No, in first column, like normal function

Ok maybe, but what have to do in this case:

db.insert( 'users', "Flex", 1 first_name="Alex", age=33 )

TypeError: insert() got multiple values for argument 'first_name'

Same with last example

v1a0 commented 3 years ago

The way to do it is very simple, it should work like a normal python function.

What do you mean "normal python function" 😅?

damnkrat commented 3 years ago

What do you mean "normal python function" sweat_smile?

Insert in your sample db should work like adding args and kwargs to this func

def some_func(id: int = "", first_name: str = "", second_name: str = "",
              age: int = "") -> list:
    pass

know that id is built-in func, but it is just an example

damnkrat commented 3 years ago

here no way to just merge together args and kwargs cases

Is this challenge?

v1a0 commented 3 years ago

All description I gave you you before, is to show you that here no way to just merge together args and kwargs cases, usage with only kwargs much safe and a bit faster, because sqllex skipping getting columns request and just inserting values for already specified columns.

Sure it's possible to add this feature, but it'll be worst way to inset data, because you have to give up the advantages of the "only kwargs" method, and add one more parsing cycle.

If you can set in which column you need to insert specific value, just do it 👽, why not?

v1a0 commented 3 years ago

here no way to just merge together args and kwargs cases

Is this challenge?

Only if you wish 😁

damnkrat commented 3 years ago

If you wish grin

Yes)

After bot release, I will then perform this challenge

v1a0 commented 3 years ago

If you wish grin

Yes)

If you'll do, please, make fork from dev branch, sqllex got major update just now, but it's not finished yet to release

v1a0 commented 3 years ago

And one more thing. When you'll do, please, make it like an new "args + kwargs" case, not instead "only args" or "only kwargs", design it like add-on, not like replacement.

damnkrat commented 3 years ago

Unnecessary since inserting via dict