ENH Optimise SQL queries to improve performance#11805
ENH Optimise SQL queries to improve performance#11805emteknetnz merged 1 commit intosilverstripe:6from
Conversation
| 'GroupID' => $this->Parent()->collateAncestorIDs(), | ||
| 'Code' => $privilegedCodes, | ||
| ]); | ||
| if ($inheritedCodes->exists()) { |
There was a problem hiding this comment.
Two optimisations in this query:
- filtering by the privileged permissions means if there's no privileged codes for that group, the database can just say "nope nothing there" instead of fetching and returning data that we then have to filter through ourselves
- using
exists()is more efficient than fetching columns
There was a problem hiding this comment.
This functionality is covered by GroupTest::testValidatesPrivilegeLevelOfParent()
GuySartorelli
force-pushed
the
pulls/6/improve-queries
branch
from
20b5f4a to
7e2a025
Compare
| ), | ||
| 'required' | ||
| ); | ||
| $valid = false; |
There was a problem hiding this comment.
This is a bug fix. We were setting a validation error, but then returning true so the member would get updated anyway!
There was no test coverage for this scenario, so I've added some.
| $adminGroups = array_intersect( | ||
| $data['DirectGroups'] ?? [], | ||
| Permission::get_groups_by_permission('ADMIN')->column() | ||
| ); | ||
|
|
||
| if (count($adminGroups ?? []) === 0) { | ||
| $adminGroups = Permission::get_groups_by_permission('ADMIN')->filter(['ID' => $data['DirectGroups']]); | ||
| if (!$adminGroups->exists()) { |
There was a problem hiding this comment.
The optimisations here are:
- Filter the query directly by
$data['DirectGroups']- if there's no matches, the database doesn't have to grab and return results that we're just gonna filter out anyway. - Using
exists()is more efficient than selecting, returning, andarray_intersecting some data.
There was a problem hiding this comment.
This is covered by the new MemberTest::testMemberValidatorRemoveAdmin().
I've also updated the existing MemberTest::testMemberValidator() to use data providers and added some more scenarios that weren't included in the test before.
GuySartorelli
force-pushed
the
pulls/6/improve-queries
branch
from
7e2a025 to
ebbbf22
Compare
| $otherPerms = DB::withPrimary( | ||
| fn() => DB::query("SELECT DISTINCT \"Code\" From \"Permission\" WHERE \"Code\" != ''")->column() | ||
| ); | ||
|
|
||
| $otherPerms = Permission::get()->exclude(['Code' => $flatCodeArray])->column('Code'); |
There was a problem hiding this comment.
Instead of fetching all of the permission codes and then excluding the codes from $flatCodesArray in PHP, it's more efficient to just tell the database to never return those in the results in the first place.
There was a problem hiding this comment.
This is covered by PermissionTest::testGetCodesGrouped() and PermissionTest::testGetCodesUngrouped(). Not super rigorous tests but they serve their purpose for this change at least.
GuySartorelli
force-pushed
the
pulls/6/improve-queries
branch
from
ebbbf22 to
f4930dc
Compare
| if ($isNewRecord) { | ||
| return $controller->redirect($this->Link()); | ||
| } elseif ($this->gridField->getList()->byID($this->record->ID)) { | ||
| } elseif ($this->gridField->getList()->filter('ID', $this->record->ID)->exists()) { |
There was a problem hiding this comment.
While not strictly reducing the number of queries, I've updated a bunch of calls to find() and byID() (which were only being used to check if a record exists) to instead use filter()->exists() which is more efficient.
GuySartorelli
force-pushed
the
pulls/6/improve-queries
branch
from
f4930dc to
65e4a89
Compare
| $parentGroup = Group::get()->setUseCache(true)->find('Code', $record['ParentCode']); | ||
| if ($parentGroup) { | ||
| $group->ParentID = $parentGroup->ID; | ||
| $parentGroupIDs = Group::get()->setUseCache(true)->filter('Code', $record['ParentCode'])->column(); | ||
| if (!empty($parentGroupIDs)) { | ||
| $group->ParentID = $parentGroupIDs[0]; |
There was a problem hiding this comment.
Instead of getting all the data about the group just to get its ID, we just get the ID directly.
2 participants
Issue