ansible / django-ansible-base

Apache License 2.0
11 stars 43 forks source link

Allow role assignment queries to use .only #482

Open AlanCoding opened 2 months ago

AlanCoding commented 2 months ago

Fixes https://github.com/ansible/django-ansible-base/issues/449

AlanCoding commented 2 weeks ago

I was unhappy with any version that modifies __init__ because for Django Models this is always brittle. Overriding the save method is a better practice, so I tried that and everything looks good to me. You should not have deferred fields when calling save... at least I don't think so...

Wait, that might be another test case.

AlanCoding commented 2 weeks ago

No that concern is fine. Test case:

@pytest.mark.django_db
def test_save_with_deferred_field(rando, inventory, inv_rd, global_inv_rd):
    assignment = inv_rd.give_permission(rando, inventory)
    reload_assignment = RoleUserAssignment.objects.filter(id=assignment.id).only('object_id').first()
    assert reload_assignment
    reload_assignment.save(update_fields=['role_definition_id'])

When this runs, the id field really is deferred. The important observation is that it's totally fine to load deferred fields when calling save, and also that deferred fields don't make any sense with unsaved objects in the first place. So there's no loophole that can result in unintended errors. The above test case falls into the intended exception we intend to raise, so it's not an issue, and I'm not compelled to add this test case to the PR.

AlanCoding commented 2 weeks ago

I know this was on hold as non-urgent, but I see very little risk in it, and we have been slowed down by this in other development.

sonarcloud[bot] commented 6 days ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud