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

Make ReturnDict support dict union operators on Python 3.9 and later #8302

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

spookylukey
Copy link
Contributor

Fixes issue #8301

Notes

For tests, as there were no other direct tests of ReturnDict, I thought it best to consider this as an implementation detail of Serializer, and just added it to the serializer tests.

As the union operator was added in PEP 584 for Python 3.9, I think it makes most sense to only add this support for Python 3.9 upwards.

One decision made here is that the union operator, both for left hand and right hand versions, produces a ReturnDict, instead of just an OrderedDict or a dict. My rationale is as follows:

If you do:

d = serializer_instance.data
d |= other_data

then d ends up as an instance of ReturnDict. If you refactored this to:

d = serializer_instance.data | other_data

you would probably expect it to stay the same type. Also, usually the additional behaviour of ReturnDict (access to serializer instance) is not going to get in the way if you don't need it, but it will be an issue if you did need it and it wasn't there.

A counter argument would be that the dictionary | operator could be considered an upgrade for this idiom:

d = {**serializer_instance.data, **other_data}

The above code produces a dict, not ReturnDict. I find this argument less convincing.

@@ -28,6 +29,22 @@ def __reduce__(self):
# but preserve the raw data.
return (dict, (dict(self),))

if sys.version_info >= (3, 9):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for the version gate here?
The methods could be always defined, even if only python >= 3.9 uses them.

Copy link
Contributor Author

@spookylukey spookylukey Dec 17, 2021

Choose a reason for hiding this comment

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

It's not a case of only Python 3.9 using them - no matter what your Python version, if you define those methods, then | will do dictionary merging like it does in 3.9 (I have confirmed this). So the question is - if I'm on Python 3.8, what do I expect these would do:

>>> {} | {}
>>> serializer_instance.data | {}

On Python 3.8, the first one will produce TypeError: unsupported operand type(s) for |: 'dict' and 'dict' and there is nothing we can do about that. So I expect most people would expect the second to do that as well.

I think we are asking for confusion and trouble if we effectively backport PEP 584 to earlier Python versions, but only for the return value of Serializer.data, and nowhere else (because we can't).

@kevin-brown
Copy link
Member

I'm +0 on the general idea of this, I think it is a nice thing to have when it comes to overall compatibility.

As for restricting this to just Python 3.9+, I understand the concerns around expectations but I personally would rather just see us keep this consistent from the DRF side for all versions. It doesn't really make sense for us to limit it just because the language version itself doesn't support it in the other direction.

@spookylukey
Copy link
Contributor Author

@kevin-brown It's impossible to keep things get that kind of consistency, since you can't change the behaviour of built-in types. If you have a Python 3.8 project, then having the | operator work only for some dictionary-like types is just confusion.

Regarding the PR in general, the current DRF behaviour is a very clear bug on Python 3.9. The ReturnDict types inherits from OrderedDict, but then breaks some of its behaviour. In other cases where this has happened, ReturnDict has been fixed to rectify this:

  • a16a8a1 fixed the fact that .copy() threw an exception
  • e59b3d1 fixed pickling errors with ReturnDict

With the patch as it is, there is no resulting inconsistency:

  1. On Python 3.8, dictionaries (included slightly customised dictionary-subclasses like the ones DRF uses in some places), do not have | operator support.
  2. On Python 3.9, they do, and it always works.

Without the patch, 2 is not true.
With the patch but dropping the sys,version_info check, 1 is not true.

DRF nowhere advertises support for | on dictionaries - it could not, because it can't change built-in dict behaviour, and doesn't use ReturnDict everywhere. The fact that ReturnDict is anything other than a normal dict has in fact been deliberately hidden, so people should just be able to use it as a dict (I for one had no idea it wasn't a normal dict until I hit this). See #2421 (comment):

We're seeing folks ask about ResultDict and ResultList objects a fair bit on the mailing list, although they're most just an implementation detail that they don't need to know about.

We should probably modify the representation of them to simply be the standard native styles, such as {'a': 123} and [1,2,3]. Hiding this bit of detail will likely lead to less confusion.

So the only consistency to aim for here is consistency with Python dictionaries.

@tomchristie
Copy link
Member

Neat - thanks Luke!

@tomchristie tomchristie merged commit bce9df9 into encode:master Dec 22, 2021
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
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