Fixed #26706 -- Made RelatedManager modification methods clear prefetch_related() cache. #6725
Conversation
yoongkang
changed the title
| class Article(models.Model): | ||
| headline = models.CharField(max_length=100) | ||
| # Assign a unicode string as name to make sure the intermediary model is | ||
| # correctly created. Refs #20207 | ||
| publications = models.ManyToManyField(Publication, name='publications') | ||
| tags = models.ManyToManyField(Tag, related_name='tags') | ||
| author = models.ForeignKey(Author, related_name='articles', | ||
| on_delete=models.CASCADE, null=True) |
There was a problem hiding this comment.
This fit on a single line of less than 119 chars.
|
Thanks for the review, I've put in the formatting changes. |
| @@ -33,12 +33,21 @@ def __str__(self): | |||
|
|
|||
|
|
|||
| @python_2_unicode_compatible | |||
| class Author(models.Model): | |||
There was a problem hiding this comment.
If you want to keep the two tests together, maybe tests/prefetch_related is a better place as it seems there are models there that would work. Otherwise, I'd put the FK test in tests/many_to_one rather than adding a new model here (unless there's some reasoning that I'm missing?).
|
I left a comment on the ticket with a question about whether or not to actually make this change. |
|
I guess there's consensus on the mailing list to move forward with this. Could you find some place to document it, including a mention in the 1.11 release notes? |
|
Absolutely, I'll work on it when I get off work, maybe after today or tomorrow. |
| @@ -300,6 +300,12 @@ Miscellaneous | |||
| calls ``super()``. :meth:`.BaseUserManager.normalize_email` is called in a | |||
| new :meth:`.AbstractUser.clean` method so that normalization is applied in | |||
| cases like model form validation. | |||
| * Calling :meth:`add()<django.db.models.fields.related.RelatedManager.add()>`, | |||
There was a problem hiding this comment.
More simply:
:meth:`~django.db.models.fields.related.RelatedManager.add`
(will render the same as what you have here now)
|
Thanks for the review. I've made the changes, |
| @@ -979,6 +979,16 @@ database. | |||
| performance, since you have done a database query that you haven't used. So | |||
| use this feature with caution! | |||
|
|
|||
| .. versionchanged:: 1.11 | |||
There was a problem hiding this comment.
Try to make these notes "self-contained" as described at https://docs.djangoproject.com/en/dev/internals/contributing/writing-documentation/#documenting-new-features
There was a problem hiding this comment.
Sorry, I'm struggling to understand that particular bit of instructions in the documentation.
If I read the instructions correctly, you want me to move the versionchanged notes to the bottom of the prefetch_related section so that no formatting changes will occur upon removal. Is my understanding correct?
There was a problem hiding this comment.
Yes, the idea is to mention the behavior twice. Once in the section body and once at the bottom under the versionchanged annotation. When removing the annotation, we remove the annotation and the note below it, leaving the body text.
timgraham
changed the title
|
New tests are now added, and this is now ready for another review. |
Doing a prefetch before clear(), remove(), or add() previously caused the cached value to be returned after a query. Since they are mutated, the cached values will no longer be valid, so this change removed them from the cache.
|
merged in d30febb, thanks! |
Doing a prefetch before clear() causes the cached value
to be returned after a query. Since they are cleared,
the cached values should no longer be needed, so this change
removed them from the cache inside clear().