drf-writable-nested icon indicating copy to clipboard operation
drf-writable-nested copied to clipboard

Remove redundant refresh following the update procedure

Open radicalbiscuit opened this issue 3 years ago • 3 comments

We use a read replica database in our deployment. As such, there is some nonzero replication lag between the write DB and the read replica DB. Because NestedUpdateMixin.update() performs instance.refresh_from_db() immediately following the write operations, we sometimes get stale data from the refresh.

Stepping through the code, I found that (in all our usages, anyway) the refresh was redundant. The nested objects are all updated independently and then reattached to parent objects as part of vanilla DRF, making the refresh operation superfluous.

One possible risk I may have not accounted for is many-to-many relations. I don't think our specific use case encounters those.

If this solution is unacceptable, please let me know and I'll rework it into an instance attribute or method kwarg that will either direct drf-writable-nested which database to use in making its read immediately following the write or whether to electively skip the refresh entirely. Currently, our solution is to monkey patch, which we'd like to avoid. Also, if it would be better to go through a formal Issue process first, let me know. I'm very grateful for your work and contribution here. It has saved me a lot of time and heartache.

radicalbiscuit avatar Oct 30 '21 13:10 radicalbiscuit

Codecov Report

Merging #154 (f1d501d) into master (b4da841) will decrease coverage by 0.00%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   98.18%   98.17%   -0.01%     
==========================================
  Files           3        3              
  Lines         220      219       -1     
==========================================
- Hits          216      215       -1     
  Misses          4        4              
Impacted Files Coverage Δ
drf_writable_nested/mixins.py 98.06% <ø> (-0.01%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b4da841...f1d501d. Read the comment docs.

codecov-commenter avatar Nov 08 '21 21:11 codecov-commenter

Hi @radicalbiscuit

Refreshing was added in this PR recently https://github.com/beda-software/drf-writable-nested/pull/122

I'm not ready to accept your PR because it will break the logic from #122. We need to think about a general solution for both issues.

ruscoder avatar Nov 10 '21 09:11 ruscoder

Hi @pcarn

Sorry for bothering you. I hope you are doing well and could help us here. This line of code https://github.com/beda-software/drf-writable-nested/blob/master/drf_writable_nested/mixins.py#L291 has added as part of your pull request https://github.com/beda-software/drf-writable-nested/pull/122 2 years ago.

The code that does the same thing on the DRF level mentioned by @radicalbiscuit was added 5 years ago. So, it seems that it didn't work for you that's why you proposed https://github.com/beda-software/drf-writable-nested/pull/122

However, the merge request contains tests and tests passed for this pull request. So there are two possible outcomes here: Either the line https://github.com/beda-software/drf-writable-nested/blob/master/drf_writable_nested/mixins.py#L291 is redundant or the test doesn't cover it properly.

@pcarn Is it possible for you to check if your code still working when you remove https://github.com/beda-software/drf-writable-nested/blob/master/drf_writable_nested/mixins.py#L291

Best, Ilya.

ir4y avatar Nov 10 '21 10:11 ir4y