Skip to content

Ensure DynamicRelationField.to_representation is passed correct instance #297

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

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

Conversation

fdintino
Copy link
Contributor

This fixes a regression that was originally introduced in d0c31d3 and a869d28

There were a few places in our code where we have subclassed DynamicRelationField and overridden to_representation, conditionally short-circuiting the serialization of a many to many field depending on the request, context, or a property of the parent instance. We had been relying on the fact that to_representation for a ModelField and a DynamicRelationField was always passed the parent instance. But in d0c31d3, and then a869d28, some of the responsibilities of to_representation were moved over to get_attribute.

After updating to the latest version our methods no longer worked, because to_representation was now being passed the ManyRelatedManager, not the parent instance. This seems to me like a bug, as it breaks expectations about the arguments and return values of get_attribute and to_representation. And so far as I can tell, moving the logic back from get_attribute doesn't break anything in dynamic-rest, since the two methods are only ever called together, e.g.: in WithDynamicSerializerMixin._faster_to_representation():

attribute = field.get_attribute(instance)
# ...
ret[field.field_name] = field.to_representation(attribute)

I've included a test that shows roughly how we sometimes override to_representation.

@fdintino fdintino force-pushed the pr-fix-field-to-representation branch from 0e44af2 to 2b6b9a1 Compare January 24, 2020 15:38
@fdintino fdintino force-pushed the pr-fix-field-to-representation branch from 2b6b9a1 to ac913c2 Compare January 24, 2020 16:17
@fdintino fdintino requested a review from ryochiji January 25, 2020 02:19
@fdintino fdintino closed this Jan 28, 2020
@fdintino fdintino reopened this Jan 28, 2020
@fdintino
Copy link
Contributor Author

This PR was closed accidentally. Re-opening.

@fdintino fdintino requested a review from aleontiev February 5, 2020 14:47
@fdintino
Copy link
Contributor Author

fdintino commented Mar 4, 2020

Any chance this might get a review and merged? We're currently installing from our github fork, but we'd prefer to avoid that if we can. Thanks!

@defbyte
Copy link

defbyte commented Mar 5, 2020

Seconding the request for eyes on this, it would be super helpful ❤️

@fdintino
Copy link
Contributor Author

fdintino commented Aug 4, 2020

Let me know if there's anything I can to do to help get this merged in. We'd prefer it if we didn't need to maintain a fork. Thank you!

@simkimsia
Copy link
Contributor

@suavesav can help?

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.

3 participants