kvesteri / sqlalchemy-continuum

Versioning extension for SQLAlchemy.
BSD 3-Clause "New" or "Revised" License
575 stars 127 forks source link

fix #282: parameter parsing for association queries #295

Open indiVar0508 opened 2 years ago

indiVar0508 commented 2 years ago

attempt to address #282, to process the input parameters received in @track_association_operations method

before changes Pytest results 346 failed, 308 passed, 14 skipped, 5398 warnings, 90 errors After Changes Pytest results 346 failed, 308 passed, 14 skipped, 5398 warnings, 90 errors

indiVar0508 commented 2 years ago

Hi @marksteward ,

Did you get a chance to review this PR ?

marksteward commented 2 years ago

Not yet.

indiVar0508 commented 2 years ago

Hi @AbdealiJK @marksteward ,

Can you review changes for address_postional_agrs issue,

i was looking into this and i think new change address #295 and #283 / #284, however while deleting entries i am getting #275 not sure why it seems for delete command units_of_work is not being populated in before_flush or before_flush is not being invoked for delete commands

pytest is working similar to latest master branch 345 failed, 311 passed, 14 skipped, 302 warnings, 90 errors

i was trying with below mentioned cases

# #280
# lotr = Book(name='Lord of the rings')
# tolkien = Author(name='JRR Tolkien', books=[lotr])
# db.add(lotr)
# db.add(tolkien)
# db.commit()
# #284/#283(PR thread)
from sqlalchemy import and_
d = book_author_table.delete(whereclause=and_(book_author_table.c.author_id==1, book_author_table.c.book_id==1))
# d = book_author_table.delete()
print(d)
engine.execute(d)
db.commit()

but when delete operation is triggered the statement get processed and params also seems to be correct {'author_id_1': 1, 'book_id_1': 1}

but i get a error with below mentioned traceback

Traceback (most recent call last):
  File "/home/indivar/github/sqlalchemy-continuum/sqlalchemy_continuum/manager.py", line 411, in append_association_operation
    uow = self.units_of_work[conn]
KeyError: <sqlalchemy.engine.base.Connection object at 0x7f5abadd7df0>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "distdemo_books.py", line 66, in <module>
    engine.execute(d)
  File "<string>", line 2, in execute
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/util/deprecations.py", line 402, in warned
    return fn(*args, **kwargs)
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 3257, in execute
    return connection.execute(statement, *multiparams, **params)
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1380, in execute
    return meth(self, multiparams, params, _EMPTY_EXECUTION_OPTS)
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/sql/elements.py", line 333, in _execute_on_connection
    return connection._execute_clauseelement(
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1572, in _execute_clauseelement
    ret = self._execute_context(
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/engine/base.py", line 1842, in _execute_context
    statement, parameters = fn(
  File "/home/indivar/github/py_envs/sqlalchemy_code_update_venv/lib/python3.8/site-packages/sqlalchemy/engine/events.py", line 128, in wrap_before_cursor_execute
    orig_fn(
  File "/home/indivar/github/sqlalchemy-continuum/sqlalchemy_continuum/manager.py", line 475, in track_association_operations
    self.append_association_operation(
  File "/home/indivar/github/sqlalchemy-continuum/sqlalchemy_continuum/manager.py", line 414, in append_association_operation
    uow = self.units_of_work[conn.engine]
KeyError: Engine(sqlite:///db.sqlite)

same issue also does happen for mater branch

indiVar0508 commented 2 years ago

Hi @marksteward ,

I have tidied the commits for this PR and tested the issue #282 for SQLA < 1.4, 1.3 and 1.2 it seems to works , could you review it and run travis CI Build

Pytest Run results : 346 failed, 308 passed, 14 skipped, 5398 warnings, 90 errors

indiVar0508 commented 2 years ago

It seems it is failing for postgres-native one, if you get a chance, could you also provide traceback for testcase so i will try to debug the issue locally.

Edit: just saw assertions are failing i'll try to debug this

indiVar0508 commented 2 years ago

Hi @marksteward ,

I have fixed the issue for last action job run, the job run was failing becuase in association i missed schema name consideration for table for SQLA > 1.4, i have added that now and tested with postgres in local for failing testfile(tests/relationships/test_many_to_many_relations.py) and it seems to work now.

Could you again trigger the Travis Build?

indiVar0508 commented 2 years ago

Hi @marksteward ,

The Travis Build seems to be success for this one, is there any other changes that are required for this PR or can accept the request and close it ?

this addresses #282 and #284

AbdealiLoKo commented 2 years ago

Hi @indiVar0508 thanks for working on this. I was not aware of context.compiled_parameters - it's awesome that dwlalchemy already provides this instead of us doing all the processing we were doing earlier.

Just 2 comments:

  1. Can you squash your commits as they both are solving the same thing
  2. Can you add a testcase for. The scenario where an association-table has a date field ? This was we have a testcase for the reported bug.

@marksteward can this be tagged as next-release too ? The approach seems to be clear and this is an issue for my application

indiVar0508 commented 2 years ago

Hi @AbdealiJK ,

Updated PR with mentioned changes

marksteward commented 2 years ago

Hi @indiVar0508, @AbdealiJK. Yes, I love the approach, particularly getting rid of positional_args_to_dict, but I want to make sure it covers any weird corner cases. Happy to prioritise it for the next release.

indiVar0508 commented 2 years ago

Hi @marksteward,

sure if you want i can try to look into some corner cases and test it, can you give some inputs to point me the direction on what cases you want me to check i can try them locally