Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant refresh following the update procedure #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

radicalbiscuit
Copy link

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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021

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%) ⬇️

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.

@ruscoder
Copy link
Member

ruscoder commented Nov 10, 2021

Hi @radicalbiscuit

Refreshing was added in this PR recently #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.

@ir4y
Copy link
Member

ir4y commented Nov 10, 2021

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 #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 #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants