Support structural changes between source model and serialized object

[JPDSousa]

Note: this discussion stems from this comment.

---

Problem statement

I've bumped into the following limitation while trying to implement a serializer which represents a restructured view from multiple models:

class Person:
    name: str
    cars: CarStats
    stocks: StockStats

class WealthSerializer(serializers.Serializer):
    cars = serializers.FloatField(source='cars.total_price')
    stocks = serializers.FloatField(source='stocks.total_value')

class CarsSerializer(serializers.Serializer):
    total_cars = serializers.IntegerField(source='cars.count')
    oldest_car_date = serializers.DateField(source='cars.oldest_date')

class PersonSerializer(serializers.Serializer):
    name = serializers.CharField()
    wealth = WealthSerializer(source='*')  # use * as this serializer uses multiple Person fields
    cars = CarsSerializer(source='*')  # use * as this serializer uses multiple Person fields

Just like in the original issue description, cars and wealth clash upon deserialization, as both update Person.cars.

Conceptually speaking, this problem falls into the "Implementation details leakage"[1][2], as the serializer (by not supporting this use case) is coupling the endpoint schema with the internal model schema.

Proposed solution

This is not a use-case we want to address. The underlying to_internal_value()mechanism leverages a set_value() helper which deliberately mirrors Python's built-in dict's behaviour:

django-rest-framework/rest_framework/fields.py

Lines 117 to 135 in a52087b

	def set_value(dictionary, keys, value):
	"""
	Similar to Python's built in `dictionary[key] = value`,
	but takes a list of nested keys instead of a single key.
	set_value({'a': 1}, [], {'b': 2}) -> {'a': 1, 'b': 2}
	set_value({'a': 1}, ['x'], 2) -> {'a': 1, 'x': 2}
	set_value({'a': 1}, ['x', 'y'], 2) -> {'a': 1, 'x': {'y': 2}}
	"""
	if not keys:
	dictionary.update(value)
	return
	for key in keys[:-1]:
	if key not in dictionary:
	dictionary[key] = {}
	dictionary = dictionary[key]
	dictionary[keys[-1]] = value

Even though this limitation prevents some good practices for developing loosely coupled interfaces, I agree that supporting yet another corner case (i.e., first time bumping into this limitation in years) makes increases the potential for user error, hence my solution was to:

1. Replace set_value by an overridable instance method (also called set_value) in Serializer.to_internal_value
2. Subclass the (now patched) Serializer and override the new set_value instance method to perform a "deep update" when updating ret.

Note: While point 1. is mandatory in my proposal, point 2. can be implemented by the actual framework users ad-hoc.

Discussion

Here's some pros/cons of the approach to jumpstart the discussion:

Pros:

• Making set_value overridable allows for custom conflict resolution policies other than the currently hardcoded LWW;
• The proposal (point 1. only) is clean and has no behavioural change.
• No breaking changes introduced

Cons:

• (inherited by any other point of extension) We're introducing one more way for users to mess up integrations.
• Note on breaking changes: introducing a new method might clash with user defined methods

WDYT?

[carltongibson]

Thanks for this @JPDSousa.

As per my comment on #6345:

I think likely this (even just making the set_value hook overridable) is adding an extra complexity that's not worth it.
I think asking you to implement to_internal_value is not too much to save that for this case.

More or less, so few people need this that for me it's not worth the overhead of adding it. Even a hook would need documentation, and would force each user to understand it, and decide whether or not it would apply to them, which almost certainly it wouldn't

That, prior to your comment, the issue was dormant since 2018 tells us what we need to know. 🙂

[JPDSousa]

Use the following premise as context for the remaining of the comment:

• This limitation does not affect functionality as you implicitly mention. This is a limitation in regards to promoting good software architecture practices.

By reading the DRF serializer documentation, I see a somehow implicit guideline that promotes mapping one model (e.g., Django Model, Python class) to one Serializer type. However, and based on both the references above and my experience, that fundamentally leaks implementation details into the interfaces (i.e., ViewSets) where the serializers are used.

To be fair, the same references mention that projects in early-stages should ignore implementation detail leakages for the sake of productivity. However, introducing this leak makes it harder and harder to evolve our internal models from the endpoint schemas which rely on our serializers.

---

Now moving to specific points I feel are important for this discussion:

I think likely this (even just making the set_value hook overridable) is adding an extra complexity that's not worth it.

I agree with the added complexity, which tbh, is a downside of all features. Can you extend on why you believe it's not worth it?

More or less, so few people need this that for me it's not worth the overhead of adding it.

If we segregate DRF users into early-stage project and late-stage project users (i.e., the "project stage" to which the user is contributing to), I believe that:

• Early-stage project users will see low value from having this feature available as per the "good practices vs fast development" trade-off mentioned above
• Late-stage project users are either buried deep into tightly coupled projects or have developed internal workarounds that they don't think are worth moving to this project.

Regardless, I would say that the DRF maintainers have more power, and therefore more responsibility, of evolving a functional project into a project that promotes good practices.

Even a hook would need documentation, and would force each user to understand it, and decide whether or not it would apply to them, which almost certainly it wouldn't

I would say that users only need to be familiar with the hooks that fulfil their use cases. Take the Django project for example. What percentage of users would you guess are familiar with all the hooks provided by the Django framework?

---

Finally, just to make it clear, I volunteer to have this new hook available in the Serializer API, matching all the DRF quality checks! 👌

[carltongibson]

... promoting good software architecture practices.

It looks like we don't share the same opinions on what this means. 🙂

It's not that I think adding such a hook is not worth, rather I think it would be positively harmful, because it would increase the overall cognitive load of using DRF in order to serve only a most esoteric edge case. In my opinion that would be bad practice.

All I can suggest is that you take a look at John Ousterhout's A Philosophy of Software Design, which is perhaps the best thing I can think of that approaches these topics in the manner I've come to think of as leading to the software that best serves the needs of its users.

[JPDSousa]

Thank you the recommendation Carlton! For sure will read it in the future 👍

Alright, so it looks like you don't think the proposed solution is acceptable. Then let's look at the problem.

Do you agree that the current Serializer implementation does not support evolving the internal model and corresponding serializers in non-equivalent directions? If so, could you please extend some ideas on how to address the issue?

Thanks a ton for all the time you're spending on this!

[carltongibson]

I suggest that you re-implement to_internal_value() for your use-case if you really need to.

[JPDSousa]

Thanks for your time and book recommendation @carltongibson 👍

For someone who might bump into this in the future, here's my stab at it:
https://gist.github.com/JPDSousa/794029d18e18e2851ca4483ba2054d29

[auvipy]

@JPDSousa you can contribute the suggested change in an external library like https://github.com/chibisov/drf-extensions, I am a co maintainer of that. as most of the old maintainers are reluctant introducing it directly into drf core, including it as an external library or part of it would be easier. so people needed that can use it using that extension