Add Notification Eager Load Support by gaussian1 · Pull Request #281 · fenos/Notifynder

gaussian1 · 2017-06-24T22:05:23Z

The Notification Parser made a direct call to the NotificationCategory::findOrFail function resulting in a n+1 query error, even if the notifications were already eager loaded.

This PR does the following:

Fixes the N+1 Problem In the Notification Parser (Fixes Eager Loading Notification Categories #280)
Adds a configuration parameter to manage Notification eager loading

Implementation Details:

[Bug Fix] Fixes the N+1 Problem In the Notification Parser: (Fixes #280)
In src/Notifynder/Parsers/NotificationParser.php: NotificationCategory::findOrFail function call was replaced by a model relation call:

public function parse($notification, $categoryId)
{
  $category = $notification->category;
  if (is_null($category)) {
    throw (new ModelNotFoundException)->setModel(
      NotificationCategory::class, $notification->category_id
    );
  //...
}

❤️ [UX] Adds An eager loading configuration parameter called 'eager_load':
Each time a user wants to use eager loading he has to write something like this:

$notifications = $user->getNotificationRelation()
                      ->with('category')
                      ->get()

It is better to have an 'eager_load' configuration parameter to specify whether users want eager loading or not in their projects. The parameter can take any of these values:

true: Always eager load relations (from, to, category)
false: (default) Don't eager load any relations
array: Specify which relations to eager load

The getNotificationRelation was divided into two methods:

A lazy loading relation: getLazyNotificationRelation to get the relations without eager loading.

public function getLazyNotificationRelation()
{
  return $this->notifications();
}

A configuration based relation: getNotificationRelation to eager load the relations according to the configuration.

public function getNotificationRelation($eagerLoad = null)
{
  $with = [];
  if (is_null($eagerLoad)) {
    $eagerLoad = notifynder_config('eager_load', false);
  }

  if ($eagerLoad === true) {
    $with = ['category', 'from', 'to']; // all relations
  } elseif (is_array($eagerLoad)) {
    $with = $eagerLoad;
  }

  return $this->getLazyNotificationRelation()->with($with);
}

Call Modifications:
Modifications were made to ignore the configuration parameter and use lazy loading for the following functions in src/Notifynder/Traits/NotifableBasic.php

updateSingleReadStatus
readAllNotifications
unreadAllNotifications
countUnreadNotifications

Electronically Yours,
Looking forward to your feedback!

EDIT: This next section discusses other eager loading options instead of using a configuration parameter, these options didn't make it to the Actual PR.. you can ignore it, or keep reading if you are interested:

Always Eager Load (Bad for queries that just do counts and updates)

/* src/Notifynder/Traits/Notifable */
public function getNotificationRelation()
{
  return $this->notifications()->with('category');
}

/* src/Notifynder/Traits/NotifableLaravel53 */
public function getNotificationRelation()
{
  return $this->notifynderNotifications()->with('category');
}

Don't make any changes: the programmer will have to eager load manually

$notifications = $user->getNotificationRelation()
                      ->with('category')
                      ->limit(4)
                      ->orderBy('created_at', 'desc')
                      ->get();

Make the eager loading further downwards at the client code like inside getNotifications(), getNotificationsNotRead()

/* src/Notifynder/Traits/NotifableBasic */
public function getNotifications($limit = null, $order = 'desc')
{
  $query = $this->getNotificationRelation()->with('category')->orderBy('created_at', $order);
  // ...
}

/* src/Notifynder/Traits/NotifableBasic */
public function getNotificationsNotRead($limit = null, $order = 'desc')
{
  $query = $this->getNotificationRelation()->with('category')->byRead(0)->orderBy('created_at', $order);
  // ...

Add a boolean for eagerloading (default false) in the getNotificationRelation method

/* src/Notifynder/Traits/Notifable */
public function getNotificationRelation($eagerLoadCategory = false)
{
  return $eagerLoadCategory ? $this->notifications()->with('category') : $this->notifications();
}

/* src/Notifynder/Traits/NotifableLaravel53 */
public function getNotificationRelation($eagerLoadCategory = false)
{
  return $eagerLoadCategory ? $this->notifynderNotifications()->with('category') : $this->notifynderNotifications();
}

make master the highest branch

Develop

…loading

coveralls · 2017-06-24T22:40:32Z

Coverage increased (+1.4%) to 88.037% when pulling 05d3859 on gaussian1:fix-280 into fa9d1a3 on fenos:master.

coveralls · 2017-06-24T23:15:32Z

Coverage decreased (-0.4%) to 86.174% when pulling 69dc0aa on gaussian1:fix-280 into fa9d1a3 on fenos:master.

coveralls · 2017-06-24T23:50:37Z

Coverage decreased (-0.4%) to 86.174% when pulling 69dc0aa on gaussian1:fix-280 into fa9d1a3 on fenos:master.

coveralls · 2017-06-25T08:00:27Z