New handling of default= for ModelSerializer about django-rest-framework

[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 #9030.
• There were also a prior attempts to introduce the same (broken) behavior in #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]

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.

[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.