V2 APIs - Enforce Default Ordering on Views

[coderbydesign]

Link(s) to Jira

• https://issues.redhat.com/browse/RHCLOUD-37534

Description of Intent of Change(s)

Given the number of workspaces we now manage in RBAC, the current implementation of default ordering is not efficient.

Ordering based on name and modified fields is done in the Meta class of the Workspace model [1] which means that all queries (app-wide ORM queries, API views, etc.) will enforce this ordering. Since we only need this ordering enforced on API calls, we should remove this from the model (greatly improving query performance outside the context of views) and push it into our base viewset [2] to be used by all v2 APIs by default.

This also changes the current behavior by ensuring name ordering is ascending (current behavior) and modified ordering is descending to show latest first (a change in current benavior).

[1] https://github.com/RedHatInsights/insights-rbac/blob/master/rbac/management/workspace/model.py#L45
[2] https://github.com/RedHatInsights/insights-rbac/blob/master/rbac/management/base_viewsets.py

Local Testing

Changes have been made to call out expected ordering behavior in unit tests. Also, locally you can run manual queries against the Workspace model to see the performance difference:

In a script, run this against the code prior to and after the change. This is what I observed locally against 1 million workspaces (an almost 400x improvement in query performance):

Before

count = Workspace.objects.count()

start_time = now()
Workspace.objects.all()
end_time = now()

print(f"Query of {count} workspace with ordering in the model took {(end_time - start_time).total_seconds()} seconds")
# Query of 1000006 workspace with ordering in the model took 1.403922 seconds

After

count = Workspace.objects.count()

start_time = now()
Workspace.objects.all()
end_time = now()

print(f"Query of {count} workspace without ordering in the model took {(end_time - start_time).total_seconds()} seconds")
# Query of 1000006 workspace without ordering in the model took 0.003522 seconds

Checklist

• if API spec changes are required, is the spec updated?
• are there any pre/post merge actions required? if so, document here.
• are theses changes covered by unit tests?
• if warranted, are documentation changes accounted for?
• does this require migration changes?
  • if yes, are they backwards compatible?
• is there known, direct impact to dependent teams/components?
  • if yes, how will this be handled?

Secure Coding Practices Checklist Link

• https://github.com/RedHatInsights/secure-coding-checklist

Secure Coding Practices Checklist

• Input Validation
• Output Encoding
• Authentication and Password Management
• Session Management
• Access Control
• Cryptographic Practices
• Error Handling and Logging
• Data Protection
• Communication Security
• System Configuration
• Database Security
• File Management
• Memory Management
• General Coding Practices

[lpichler]

good catch!

[lpichler]

from CI:

0 
13:29:20 ======================================================================
13:29:20 FAIL: test_delete_role_success (tests.management.role.test_view.RoleViewsetTests)
13:29:20 Test that we can delete an existing role.
13:29:20 ----------------------------------------------------------------------
13:29:20 Traceback (most recent call last):
13:29:20   File "/usr/lib64/python3.9/unittest/mock.py", line 1336, in patched
13:29:20     return func(*newargs, **newkeywargs)
13:29:20   File "/opt/app-root/tests/management/role/test_view.py", line 1614, in test_delete_role_success
13:29:20     self.assertEqual(al_dict_action, "delete")
13:29:20 AssertionError: 'create' != 'delete'
13:29:20 - create
13:29:20 + delete

but it is not failing for me locally.

[coderbydesign]

@lpichler Odd! Looks like it's green now, but makes me wonder if there's a flakey test somewhere. I did see in that run it looked like there were some redis errors/warning as well, not sure if it's related.