New handling of default= for ModelSerializer

[astifter]

Then changes in #9030 lead to a issue in my implemenation:
With 3.14 and its ModelSerializer we could determine if a user was passing a field to the API or not. (Basically we allow setting the DB field to everything EXCEPT the default via the REST API.)

Example:

class Message(Model):
  content_type = models.TextField(null=False, blank=False, default="undefined")

class MessageSerializer(ModelSerializer):
  class Meta:
     model = Message

We could de-serialize the JSON {} and the validated data didn't contain the default value for content_type. We still can create the model instance from it because there is a default in the database. BUT we disallow actively setting the content_type to undefined by checking if content_type is in the validated data and refusing a JSON with {"content_type": "undefined"}.

With 3.15 we can't determine this anymore, yes? Or is there something in the validated data to tell us where the data came from (the JSON the user posted or the models default)?

[astifter]

Okay, I RTFM and I would use .initial_data to see if the content_type is user provided or not, correct approach?

[henribru]

There's another side-effect of this change that leads to a huge breaking change for non-partial updates (PUT) of fields with default values. Previously if you didn't include a field with a default value when doing a PUT request, that field wouldn't get updated. It would just retain its current value, similarly to if you had done a partial update (PATCH) (the reason you're allowed to not pass it in the PUT is that the default leads to it not being required)

But now that field gets updated to the default value instead! This seems very dangerous since it could lead to data loss for anyone who's previously not been passing the current value along for all fields with defaults in PUT requests.

[astifter]

There's another side-effect of this change that leads to a huge breaking change for non-partial updates (PUT) of fields with default values. Previously if you didn't include a field with a default value when doing a PUT request, that field wouldn't get updated. It would just retain its current value, similarly to if you had done a partial update (PATCH) (the reason you're allowed to not pass it in the PUT is that the default leads to it not being required)

But now that field gets updated to the default value instead! This seems very dangerous since it could lead to data loss for anyone who's previously not been passing the current value along for all fields with defaults in PUT requests.

Good point.

[peterthomassen]

But now that field gets updated to the default value instead! This seems very dangerous since it could lead to data loss for anyone who's previously not been passing the current value along for all fields with defaults in PUT requests.

Indeed! This is actually the most dramatic of all the regressions in this release.

In our case, it means that users sending at PUT request on our /tokens endpoint with a name in the request payload will get their permissions altered in an unexpected way. Uh!

It would be great if such basic REST API guarantees were covered by tests.

[tomchristie]

Okay, synopsis on this...

• This issue occurs when PUT requests with partially incomplete data are made, on 3.15.0.
• The issue has been resolved in 3.15.1.
• The issue was introduced in Propagate 'default' from model_field to serializer field. #9030.
• There were also a prior attempts to introduce the same (broken) behavior in Propagate 'default' from model_field to serializer field #8130.
• We do have unit tests covering the change in this ModelSerializer -> Serializer behavior, but we didn't have an integration test that would have highlighted the effect of that change.
• There were a number of different eyes on that change, the impact wasn't spotted and there was insufficiently strong pushback. "I'm a bit reluctant, simply because at this point in it's lifecycle almost any change in REST framework ends up having unintended side-effects for existing users, and I could easily see this being the case for this change. We've used "default on the model => not required on the serializer" since ~forever."
• We are focused on tightening up project maintainership policies to ensure we're not at risk of similar issues occurring in the future.

Our policy (in my view) should be a very clear...

No pull requests will be accepted except for:

• Changes required to support newer Django releases.
• Security fixes.
• Documentation improvements.

...The project is mature, with plenty of scope for third party packages and adaptation. Further changes at this point are just creating risk, churn, and unnecessary busy-work.

No shade, we're all in the same boat here.
Best any of us can ask is learn and improve.

[astifter]

Our policy (in my view) should be a very clear...

No pull requests will be accepted except for:

• Changes required to support newer Django releases.
• Security fixes.
• Documentation improvements.

...The project is mature, with plenty of scope for third party packages and adaptation. Further changes at this point are just creating risk, churn, and unnecessary busy-work.

No shade, we're all in the same boat here. Best any of us can ask is learn and improve.

I welcome the decision of a maintainer to declare a project "done". Its a rare occurrence and would serve other mature projects well.