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

Eager Loading Notification Categories #280

Closed
gaussian1 opened this issue Jun 24, 2017 · 7 comments
Closed

Eager Loading Notification Categories #280

gaussian1 opened this issue Jun 24, 2017 · 7 comments

Comments

@gaussian1
Copy link

gaussian1 commented Jun 24, 2017

Problem:
On Retrieving Notifications an N+1 query problem occurs for notification categories

Here are the SQL Queries after running:

$notifications = Auth::user()->getNotifications(4);
foreach ($notifications as $notification) {
  echo $notification->text;
}

notifynder

Analysis:
On attempting to eager load, the following function is called inside the NotificationParser that forces one query per category

/** src/Notifynder/Parsers/NotificationParser@parse */
public function parse($notification, $categoryId)
{
  $category = NotificationCategory::findOrFail($categoryId);
  //...
}

Possible Solution:
I can create a pull request that checks whether the passed Notification's category relation is eager loaded, to be something like

/** src/Notifynder/Parsers/NotificationParser@parse */
public function parse($notification, $categoryId)
{ 
  $category = $notification->relationLoaded('category') ? $notification->category : NotificationCategory::findOrFail($categoryId);
  //...
}

If the above solution is okay, I can easily add the pull request, Cheers!


Notes:
The relationLoaded function was added in a later version of Laravel, I guess it was 5.1 but I am not sure, if we decide not to use it we can add a function to the Notification Model

/** src/Notifynder/Models/Notification
 *
 * Determine if we've already loaded the category
 *
 * @return bool
 */
 public function isCategoryLoaded() {
   return isset($this->relations['category']);
 }

Environment:
Laravel Framework 5.4.27
Notifynder 4.2.1

@Gummibeer
Copy link
Collaborator

Hey,

thanks for the hint. But I think that there is something simpler - cause your solution doesn't load the relation it checks. So why not simply:

if(!$notification->category->exists) {
    throw new ModelNotFoundException();
}
$category = $notification->category;

@gaussian1
Copy link
Author

That's even better! here is a working example, because exists gives an error:

/** src/Notifynder/Parsers/NotificationParser@parse */
public function parse($notification, $categoryId)
{ 
  $category = $notification->category;
  if(is_null($category)) {
      throw new ModelNotFoundException();
  }
}

However, this will work only if we are passing a NotificationModel Object, I've checked the code and it seems that we never pass "$notification" as an array or a BuilderNotification as suggested in the function comment.
@param array|ModelNotification|BuilderNotification $notification

Finally, if we are only passing a NotificationModel we can remove the category_id argument entirely from the parse function.

/** src/Notifynder/Parsers/NotificationParser@parse */
public function parse($notification)
{ 
  $category = $notification->category;
  if(is_null($category)) {
      throw new ModelNotFoundException();
  }
}

Thanks for your quick reply!

@Gummibeer
Copy link
Collaborator

Yes, the param doc tag is too much - it could be:
@param ModelNotification$notification

I think that this comes from #229 but even there we create a model - https://github.com/fenos/Notifynder/blob/master/src/Notifynder/Builder/Notification.php#L162

But this is another issue - so you can fix all this stuff in one PR or just fix the 1+n query issue and after this I will take care about the doctag issue.

@gaussian1
Copy link
Author

@Gummibeer A little bit unrelated, but how do you run the unit tests?

The following instructions give an error, and I couldn't find a contribution guide

composer install
composer dump-autoload
vendor/bin/phpunit

PHP Fatal error:  Class 'Fenos\Tests\Models\UserL53' not found in /Notifynder/tests/NotifynderTestCase.php on line 132

@gaussian1
Copy link
Author

I had to run and it worked

composer dump-autoload -o

@Gummibeer
Copy link
Collaborator

@gaussian1 we run them with TravisCI - if you want to know which commands we use: https://github.com/fenos/Notifynder/blob/master/.travis.yml
But yes, the composer dump-autoload -o is needed cause otherwise composer doesn't find the classes outside the PSR4-Root.

@Gummibeer
Copy link
Collaborator

v4.3.0-rc is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants