Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow role assignment queries to use .only #482

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

Fixes #449

@AlanCoding AlanCoding added bug Something isn't working app:rbac labels Jun 20, 2024
@slemrmartin slemrmartin self-requested a review June 27, 2024 13:08
@slemrmartin slemrmartin changed the title Allow role assignment queries to use .only [Hold] Allow role assignment queries to use .only Jun 27, 2024
@AlanCoding
Copy link
Member Author

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
Copy link
Member Author

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 AlanCoding changed the title [Hold] Allow role assignment queries to use .only Allow role assignment queries to use .only Aug 21, 2024
@AlanCoding AlanCoding added the Ready for review This PR is ready for review either initially or comments have been address label Aug 21, 2024
@AlanCoding
Copy link
Member Author

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.

Copy link

sonarcloud bot commented Aug 29, 2024

@AlanCoding AlanCoding requested a review from relrod August 29, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:rbac bug Something isn't working Ready for review This PR is ready for review either initially or comments have been address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RecursionError possible with .only on RBAC assignment models
2 participants