Only save supplied fields to database (for partial_updates)

[askholme]

Currently a race condition seems to exists when using partial updates.
If the system is running in a threaded environment (i.e. almost all production) then two PATCH requests for different fields on the same object can overwrite each other.
E.g. request 1) is a PATCH on object a changing field name to "newname"
request 2) is a PATCH on object a changing field description to "old description"

Currently it's possible to receive a 200 OK while only one of the requests are stored if they arrive sufficiently close to each other. Reason is that the Django ORM by default will fetch all the objects fields and supply them again in the UPDATE statement. This means that if request 1+2 is handled by different threads it's possible that they select the same set of initial values and therefore will override each other.

This patch fixes this by using the parameter update_fields when saving models thus ensuring that only the supplied fields in the partial update is stored.

(Apart from fixing the race condition this should also improve performance slightly)

[kevin-brown]

update_fields isn't valid in 1.4 (added in 1.5) and this doesn't work for many-to-many fields, which will trigger an error if they are included in the list.

[askholme]

Right, hadn't thought about that (and wasn't aware of the requirements of 1.4) support.

Another possibility could simply be to document this (i.e. that people should write their own extension of modelserializer to handle these edge cases) ?

would close then

[lovelydinosaur]

See also #2621. Unsure of best resolution - I'd tend against introducing the extra complexity - tho it does fix this issue, it'll also end up making the logic less obvious (particularly wrt to if we need to drop out the m2m fields), potentially introduce more usage issues, and make it less obvious to folks that they can easily override the update behavior themselves. Having said that, I'm not completely against it, as it does resolve a clearly valid issue. Perhaps if we had an update_model shortcut (that let's us hide away some of the compat concerns and m2m tweaks) it'd be okay.

[askholme]

so you would put the logic of constructing the update_fields parameter in a separate function? (and have a version check for 1.4 compat?). My main concern with that approach is that it would only handled standard django related fields, while custom related fields would likely trigger an error... (i.e. the change will implicitly make DRF incompatible with custom related fields unless you manually fix the update_model shortcut...).

If all is okay with that i'm willing to extend the method + add documentation following below points

• create update_model shortcut which puts updated fields into update_fields except if it's manytomany or relatedfield
• let update call update_model
• Add note to serializer documentation on how this works

[lovelydinosaur]

My main concern with that approach is that it would only handled standard django related fields, while custom related fields would likely trigger an error...

?

[lovelydinosaur]

If all is okay with that i'm willing to extend the method + add documentation following below points

I'm still on the fence about an update_instance(instance, **kwargs) shortcut, but I do think it's an option.
Even forgetting about the update_fields=... part, setting a number of attributes on the instance and then saving it is such a common pattern that it might be useful to have, but I'm not definite about it right now.

[chibisov]

@askholme, you can try to use PartialUpdateSerializerMixin

[askholme]

@tomchristie kevin noted that django cannot handle many to many fields (and other related fields) being supplied in the update_fields parameter. My comment was mainly that no matter how we do it it would be difficult to filter anything else than standard django fields?

[askholme]

@chibisov Ah, wonderfull. Seems like a good solution to the problem. Can't really figure if the current drf-extension code handles m2m fields well but otherwise i guess the right approach is to extend that.

@tomchristie why not simply point people toward to drf-extension mixin?

[lovelydinosaur]

cannot handle many to many fields (and other related fields) being supplied in the update_fields parameter

I assume that the fields that cannot be passed are anything that doesn't actually exist on the table:

• many to many fields
• reverse relationships

[askholme]

@tomchristie right

[lovelydinosaur]

@tomchristie why not simply point people toward to drf-extension mixin?

I guess just trying to assess if we think this should be handled in the core package.

[askholme]

Well at such you could argue that it should be handled by django itself (through automatically setting the update_fields parameter).

But it does have it's merits that core DRF should work well with high-frequency partial updates.

[chibisov]

...and this doesn't work for many-to-many fields, which will trigger an error if they are included in the list.

drf-extensions handles this problem

[lovelydinosaur]

Closing the pull request in it's current state as it doesn't quite deal with all the cases, but I've opened a corresponding issue for this, and could consider future pull requests.

[lovelydinosaur]

For the latest take, see the update on #2648.