Exclude ManyToManyFields when fetching excluded fields. #707
Conversation
Includes a test that fails on master but passes with the change.
| @@ -387,6 +387,7 @@ def get_instance(self): | |||
| excluded_attnames = [ | |||
| model._meta.get_field(field).attname | |||
| for field in self._history_excluded_fields | |||
| if not isinstance(model._meta.get_field(field), ManyToManyField) | |||
There was a problem hiding this comment.
@jordonwii this works? Looks like this excludes ManyToManyFields from being excluded? The included fields are in attrs above
There was a problem hiding this comment.
Right, that's intentional - the goal here is to prevent an M2M field from ending up in the .values(...) call on line 395, because then the .get() will fail.
There was a problem hiding this comment.
Ohh I see. Forgot about this code. That makes sense. Want to just add a comment here briefly explaining that?
There was a problem hiding this comment.
Done. Let me know if it's unclear - wording "exclude a field from the exclusion list we're trying to re-include" is tricky. :)
|
@jordonwii this looks good to me. Just need to fix conflicts |
|
Great! The conflict was only in CHANGES.rst. It should be resolved now. |
Description
Calling
as_ofon a model with an excludedManyToManyFieldwill fail if the related field has more than one object. This is because of thevalues(...).get()call here, which behaves differently for M2M fields than for normal scalar fields: https://github.com/jazzband/django-simple-history/blob/master/simple_history/models.py#L395.This fixes this by simply excluding M2M fields from the reconstructed object instance. AFAICT this is safe and cheap, because users can query their M2M using the model manager if they want values from it.
An alternate fix could change the code to avoid
.get(), but I don't think we actually want to prefetch all of the values of an M2M field here.Related Issue
#706
How Has This Been Tested?
Test included.
Types of changes
Checklist:
make formatcommand to format my codeAUTHORS.rstCHANGES.rst