coleifer / peewee

a small, expressive orm -- supports postgresql, mysql, sqlite and cockroachdb
http://docs.peewee-orm.com/
MIT License
11.17k stars 1.37k forks source link

Wrong table order in `delete_instance(recursive=True)` causes missing deletion #2893

Closed stenci closed 5 months ago

stenci commented 5 months ago

I have not created an easily reproducible case because I imagine this is a known issue. Please let me know if the behavior I describe below is unknown and unintended, and I will try to build a simple reproducible case.

I am using snapshots, because it's quicker for me to describe the problem. Please let me know if I was too lazy and you need more details to understand the problem.

Calling inventory.delete_instance(recursive=True) deletes the rows in the following order: image

The rows from cnc_program_sheet (2nd delete) are deleted before cnc_program_part (4th delete), resulting in the failure to delete anything from cnc_program_part, because the multiple level relation to cnc_program_part - cnc_program_sheet - cnc_program - inventory doesn't see the rows previously deleted.

I don't know if the missing deletion is caused by the particular database design or it's a known limitation in peewee. I know that I have hit the head in the wall for a while, until I checked the SQL log and understood the reason for the failure.

I have now fixed my code: I delete the rows one table at a time in the correct order instead of using the recursive option, but I thought I would report it, just in case I am misusing peewee or you don't know about this problem.

Here is the relation diagram: image

coleifer commented 5 months ago

We do a topological sort to determine delete order. This does look like a bug assuming there's no cycles or something like that, as we do test this behavior: https://github.com/coleifer/peewee/blob/master/tests/models.py#L1596-L1614 -- If you can provide a simple test program that reproduces the issue I'd be grateful but will take a look later.

coleifer commented 5 months ago

Nevermind, I'm replicating it. Looks like a bug alright.

coleifer commented 5 months ago

This should be fixed now, thank you for the report.