Fixed N+1 query behaviour on viewing profile comments

[Xon]

Affected version

2.0.9

When viewing a profile with profile posts, there is a xf_admin_user query per unique user. This is because User::canEdit checks to see if the user is a

Stack trace;

XF\Entity\User->getIsSuperAdmin() in src/XF/Mvc/Entity/Entity.php at line 142
XF\Mvc\Entity\Entity->get() in src/XF/Mvc/Entity/Entity.php at line 95
XF\Mvc\Entity\Entity->__get() in src/XF/Entity/User.php at line 361
XF\Entity\User->canEdit()
call_user_func_array() in src/XF/Template/Templater.php at line 939
XF\Template\Templater->method() in internal_data/code_cache/templates/l1/s25/public/member_macros.php at line 62
XF\Template\Templater->{closure}() in src/XF/Template/Templater.php at line 662
XF\Template\Templater->callMacro() in internal_data/code_cache/templates/l1/s25/public/member_view.php at line 72
XF\Template\Templater->{closure}() in src/XF/Template/Templater.php at line 1249
XF\Template\Templater->renderTemplate() in src/XF/Template/Template.php at line 24
XF\Template\Template->render() in src/XF/Mvc/Renderer/Html.php at line 48
XF\Mvc\Renderer\Html->renderView() in src/XF/Mvc/Dispatcher.php at line 332
XF\Mvc\Dispatcher->renderView() in src/XF/Mvc/Dispatcher.php at line 303
XF\Mvc\Dispatcher->render() in src/XF/Mvc/Dispatcher.php at line 44
XF\Mvc\Dispatcher->run() in src/XF/App.php at line 1931
XF\App->run() in src/XF.php at line 328
XF::runApp() in index.php at line 13

 

[DragonByte Tech]

I'm unable to replicate this issue. Locally, I'm viewing this page: /members/belazor.1/?_debug=1 (as user ID 1)

I see 1 query to xf_admin (no queries for xf_admin_user, and that table does not exist in my installation of v2.0.9):

XF\Db\Mysqli\Statement->execute() in src/XF/Db/AbstractAdapter.php at line 79
XF\Db\AbstractAdapter->query() in src/XF/Mvc/Entity/Finder.php at line 1130
XF\Mvc\Entity\Finder->fetchOne() in src/XF/Mvc/Entity/Manager.php at line 149
XF\Mvc\Entity\Manager->find() in src/XF/Mvc/Entity/Manager.php at line 352
XF\Mvc\Entity\Manager->getRelation() in src/XF/Mvc/Entity/Entity.php at line 373
XF\Mvc\Entity\Entity->getRelation() in src/XF/Mvc/Entity/Entity.php at line 171
XF\Mvc\Entity\Entity->get() in src/XF/Mvc/Entity/Entity.php at line 95
XF\Mvc\Entity\Entity->__get() in src/XF/Entity/User.php at line 308
XF\Entity\User->getIsSuperAdmin() in src/XF/Mvc/Entity/Entity.php at line 142
XF\Mvc\Entity\Entity->get() in src/XF/Mvc/Entity/Entity.php at line 95
XF\Mvc\Entity\Entity->__get() in src/XF/Entity/User.php at line 361
XF\Entity\User->canEdit()
call_user_func_array() in src/XF/Template/Templater.php at line 939
XF\Template\Templater->method() in internal_data/code_cache/templates/l1/s3/public/member_macros.php at line 62
XF\Template\Templater->{closure}() in src/XF/Template/Templater.php at line 662
XF\Template\Templater->callMacro() in internal_data/code_cache/templates/l1/s3/public/member_view.php at line 72
XF\Template\Templater->{closure}() in src/XF/Template/Templater.php at line 1249
XF\Template\Templater->renderTemplate() in src/XF/Template/Template.php at line 24
XF\Template\Template->render() in src/XF/Mvc/Renderer/Html.php at line 48
XF\Mvc\Renderer\Html->renderView() in src/XF/Mvc/Dispatcher.php at line 332
XF\Mvc\Dispatcher->renderView() in src/XF/Mvc/Dispatcher.php at line 303
XF\Mvc\Dispatcher->render() in src/XF/Mvc/Dispatcher.php at line 44
XF\Mvc\Dispatcher->run() in src/XF/App.php at line 1931
XF\App->run() in src/XF.php at line 328
XF::runApp() in index.php at line 13

Two distinct users have posted on my profile:

(The user with the fancy characters is also an administrator. Primary group: Registered - Secondary groups: Administrative, Moderating.)

Am I missing something in my attempts to replicate this?


Fillip

 

[Xon]

The user record needs the is_admin flag set, so it isn't quite N+1 behaviour. It basically occurs for each unique admin on a profile page.

This code in User::canEdit is doing it;

if ($this->is_admin && $this->is_super_admin && !$visitor->is_super_admin)
{
   return false;
}

is_super_admin => getIsSuperAdmin

Reversing !$visitor->is_super_admin && $this->is_super_admin should cause the visitor record to be checked first and hopefully bail-out earlier

 

[Chris D]

This only causes a maximum of 2 extra queries when viewing the profile page. It's simply the check that happens for the "Edit" link in the moderator actions menu in the top right of the member profile and all it's doing is checking if the current visitor can edit the user of the profile being viewed. It's nothing to do with profile posts/comments and other admins appearing on the page.

Swapping those is_super_admin checks only partly solves the problem. The visitor's is_super_admin check actually still generates 1 query because we don't fetch the Admin relation when setting up the visitor entity.

I think we can add Admin as a default relation when getting the visitor and in conjunction with re-ordering those checks, and that should mostly solve the problem.

Incidentally, I've simplified the canEdit condition too to simply:

if (!$visitor->is_super_admin && $this->is_super_admin)
{
   return false;
}

And I've changed getIsSuperAdmin to:

if ($this->is_admin && $this->Admin)
{
   return $this->Admin->is_super_admin;
}

return false;

The `is_admin` bailout there makes sense as, clearly, you can't have an Admin relation without it and will save a query in case there's careless usage of it elsewhere.