ansible / awx

AWX provides a web-based user interface, REST API, and task engine built on top of Ansible. It is one of the upstream projects for Red Hat Ansible Automation Platform.
Other
14.06k stars 3.42k forks source link

[Tech Debt] Django ORM update_fields non-interference #12217

Open AlanCoding opened 2 years ago

AlanCoding commented 2 years ago
SUMMARY

This app makes heavy use of update_fields when using the Django ORM, but we do not reap its full benefit mainly because of overly-broad logic in save methods. We have plenty of specific examples documented in the issue tracker. This issue asks that we clarify expectations generally.

Demo Proposal for Best Practice

Consider a model FeatureModel which ultimately derives from django.db.models.Model, and a particular saved object obj.

If a model is saved with obj.save(update_fields=["field_A"]), then

  1. If field_A does not directly affect any other fields, then no additional queries should be made and django.db.models.Model.save should be called with the single update field, plus any contextual fields, like ["field_A", "modified_by"]
  2. The update_fields list may be expanded to include another model field, like ["field_A", "field_B"] if the field_B is directly affected by the value of field_A
  3. If any contextual fields or dependent fields do not change from the prior value where the prior value is assessed by what's currently on obj, then those fields should not be appended to update_fields. So if field_A affects field_B, but obj.field_B is already equal to the field's computed value, then django.db.models.Model.save should see ["field_A"] only
  4. We should make use of update_fields to the maximum extent possible, particularly in non-web services like the dispatcher

For an example of (2), consider when we set obj.status = "failed" and obj.save(update_fields=["status"]), this may trigger obj.failed to be set to True. This is ok.

However, if we save something unrelated like obj.save(update_fields=["artifacts"]), this should never update any status-related fields.

I have UnifiedJobTemplate and UnifiedJob models in mind specifically here.

This issue is meant to encompass several other issues

https://github.com/ansible/awx/issues/11698

https://github.com/ansible/awx/pull/11838

https://github.com/ansible/awx/pull/11833

AlanCoding commented 1 year ago

Also see https://github.com/ansible/awx/pull/13124, same general issue