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

Fix issue with save on translation form #6037

Merged
merged 3 commits into from
Aug 7, 2019

Conversation

agjohnson
Copy link
Contributor

The translation was not being saved before adding to the parent project.
This surfaced an issue when the database is different, like in the case
where we use a database router to send read/write operations to
different databases.

The translation was not being saved before adding to the parent project.
This surfaced an issue when the database is different, like in the case
where we use a database router to send read/write operations to
different databases.
@agjohnson agjohnson requested a review from a team August 3, 2019 14:53
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me.

I didn't test it locally with a DB router, though.

return project
def save(self, commit=True):
if commit:
self.translation.save()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to add a comment explaining a little more why this is required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the fix clearer. The problem is that when calling add(), self.parent isn't the object that is updated. The field that is updated is self.translation.main_language_project = self.parent. Calling save() first was a method to make the object state use the database that matches db_for_write(). Reversing this logic makes this less of a hack and skips usage of add() altogether.

It's not entirely clear why this is an issue on only the translation relationship however.

@stsewd
Copy link
Member

stsewd commented Aug 5, 2019

Some things I found here. At this point self.translation and self.parent have ._state.db == 'replica'. After self.translation.save() it changes self.translation._state.db == 'default'.

This is the line where django makes it fail.

https://github.com/django/django/blob/974897759e9afc4cc56fb87e12319fa9697e93c9/django/db/models/fields/related_descriptors.py#L634

Believe this is a django bug? I didn't find anything mentioning this case in the docs or in a google search

@stsewd
Copy link
Member

stsewd commented Aug 5, 2019

When making bulk=False, it doesn't check ._state.db.

@stsewd
Copy link
Member

stsewd commented Aug 5, 2019

updated my other PR to use bulk=False #6031

Using bulk=False in this PR works too. Not sure if call this a bug or a limitation (since django doesn't know if the object exists in the database used for write)

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #6037 into master will decrease coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6037      +/-   ##
==========================================
- Coverage   79.25%   79.24%   -0.01%     
==========================================
  Files         175      175              
  Lines       11177    11179       +2     
  Branches     1395     1396       +1     
==========================================
+ Hits         8858     8859       +1     
  Misses       1946     1946              
- Partials      373      374       +1
Impacted Files Coverage Δ
readthedocs/projects/forms.py 85.82% <83.33%> (-0.19%) ⬇️

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 93bcf8d...d88f362. Read the comment docs.

@agjohnson
Copy link
Contributor Author

I came across all of this in my debugging, which is why this PR originally called save() before making the add() call, but I don't think forcing bulk=False is the solution. The larger question is how come this doesn't happen on more of our relationships that are already using add(). Translations seems to be the only logic that triggeres this bug.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this 👍. We should track the other issue in .com where we use the custom db router

@ericholscher ericholscher merged commit a5dadaa into master Aug 7, 2019
@ericholscher ericholscher deleted the agj/fix-translation-form-save branch August 7, 2019 18:52
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