better narrowing for m2m updates

netzkolchose / django-computedfields

Provides autogenerated autoupdated database fields for model methods.

MIT License

94 stars 14 forks source link

better narrowing for m2m updates #131

Closed jerch closed 1 year ago

jerch commented 1 year ago

TODO:

[x] optimize M2M updates by narrowing to M2M signal instance
[x] explicit test cases
[x] proxy checks

Should fix #130.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 6709876023

9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 94.813%

Totals
Change from base Build 6708206764:	0.02%
Covered Lines:	1115
Relevant Lines:	1161

💛 - Coveralls

jerch commented 1 year ago

@martinlehoux This will still need some iterations before it can get merged for these reasons:

does not work yet for proxy models (already have local patch for it)
need to come up with some more extensive tests to spot possible regressions

The main issue with m2m fields is the fact, that they are m:1:n relations under the hood, which means the resolver has to fan out updates in two directions (in your test example that would be deps on Advert.tags and on Tag.advert_set). I am not quite sure yet, if the stricter narrowing here will lead to missed updates under certain circumstances, thus I have to revisit extisting test cases and prolly write a few more on top.

martinlehoux commented 1 year ago

Yes, makes total sense. At least the dependency graph does its job and shows that all_tags depends on tags.adverts, so even if not expected in the first place, it's documented. Depending on how long you think this would take, you could add this in the documentation? I read the part around m2m gotchas, but didn't find an answer to my specific question.

martinlehoux commented 1 year ago

Hi @jerch , is there anything I can do to help you on this matter?

jerch commented 1 year ago

@martinlehoux Ah sorry, the issue went kinda out of focus. I hope to get to it by the weekend, there is one more edge case I need to test, if it passes we can add it.

jerch commented 1 year ago

@martinlehoux Added the missing proxy model fixes & test cases, now passing all tests. I am still not quite sure, if the proxy fixes cover all edge cases, but I was not able to provoke a faulty case with any test I could think of.

Therefore I think this good enough to be added.

martinlehoux commented 1 year ago

Thanks a lot! I will try to confirm this in our app :)