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.

Sign up for GitHub

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

Update/create time period entries efficiently #114

Closed
wants to merge 1 commit into from
Closed

Update/create time period entries efficiently #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
271 changes: 198 additions & 73 deletions application/forms/EntryForm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Icinga\Module\Notifications\Model\TimeperiodEntry;
use Icinga\Web\Session;
use ipl\Html\HtmlDocument;
use ipl\Orm\Behavior\Binary;
use ipl\Scheduler\Contract\Frequency;
use ipl\Scheduler\OneOff;
use ipl\Scheduler\RRule;
Expand Down Expand Up @@ -123,6 +124,10 @@ public function loadEntry(int $scheduleId, int $id): self

$recipients = [];
foreach ($members as $member) {
if (! isset($values['membership_hash'])) {
$values['membership_hash'] = (new Binary([]))->toDb($member->membership_hash, null, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

membership_hash is nullable and this line doesn't account for that and leads to errors for me.
I think registering NULL as value is just fine. Though, remember to switch to array_key_exists above then.
Besides, don't use the behavior here, just use bin2hex and later when processing the hash again, hex2bin. There's no need to do this database vendor aware.

}

if ($member->contact_id !== null) {
$recipients[] = 'contact:' . $member->contact_id;
} else {
Expand Down Expand Up @@ -220,6 +225,7 @@ protected function assemble()
};

$this->addElement('hidden', 'timeperiod_id');
$this->addElement('hidden', 'membership_hash');

$this->addElement('textarea', 'description', [
'label' => $this->translate('Description'),
Expand Down Expand Up @@ -311,114 +317,233 @@ protected function assemble()
public function addEntry(int $scheduleId): void
{
$data = $this->formValuesToDb();
$membershipHash = $this->getMembershipHashFromRecipients($scheduleId);
$recipients = array_map(function ($recipient) {
return explode(':', $recipient, 2);
}, explode(',', $this->getValue('recipient')));

$db = Database::get();
$db->beginTransaction();

$db->insert('timeperiod', ['owned_by_schedule_id' => $scheduleId]);
$timeperiodId = $db->lastInsertId();
$members = ScheduleMember::on(Database::get())
->with('timeperiod')
->filter(
Filter::all(
Filter::equal('timeperiod.owned_by_schedule_id', $scheduleId),
Filter::equal('schedule_member.membership_hash', $membershipHash)
)
);

if ($members->count() > 0) {
Comment on lines +328 to +337
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather select from Timeperiod directly. That's the only information you're really interested in. The count of members for example, you're using it to check whether there are any members at all. That's a common mistake, I mean, to count although the count itself is of no interest. So by fetching the timeperiod directly, you already verified that at least one member's hash matches. (Given that the filter is still the same)

$timeperiodId = $members->first()->timeperiod_id;
} else {
$db->insert('timeperiod', ['owned_by_schedule_id' => $scheduleId]);
$timeperiodId = $db->lastInsertId();
}

$db->insert('timeperiod_entry', $data + ['timeperiod_id' => $timeperiodId]);

foreach ($recipients as list($type, $id)) {
if ($type === 'contact') {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contact_id' => $id
]);
} elseif ($type === 'group') {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contactgroup_id' => $id
]);
if ($members->count() === 0) {
$binaryMemberHash = (new Binary([]))->toDb($membershipHash, null, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$binaryMemberHash = (new Binary([]))->toDb($membershipHash, null, null);
$q = ScheduleMember::on(Database::get());
$binaryMemberHash = $q->getResolver()->getBehaviors($q->getModel())->persistProperty($membershipHash, 'membership_hash');

This way you don't hardcode the binary behavior here and it will still work once are compatible with MySQL.

foreach ($recipients as list($type, $id)) {
if ($type === 'contact') {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contact_id' => $id,
'membership_hash' => $binaryMemberHash
]);
} elseif ($type === 'group') {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contactgroup_id' => $id,
'membership_hash' => $binaryMemberHash
]);
}
}
}

$db->commitTransaction();
}

/**
* Get membership hash for the recipients in timeperiod entry for the given schedule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more specific here and less verbose. You don't need to mention timeperiod entries here, that's obvious from the class already. Then, get imposes that the value may be be cached or so. At least, I don't see it fit with the implementation. It's not a getter. So maybe rename the method to calculate... or generate..., or even hash....

*
* @param int $scheduleID
*
* @return string
*/
public function getMembershipHashFromRecipients(int $scheduleID): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use $scheduleId (with a lowercase d at the end) instead. addEntry does it already.

{
$recipients = explode(',', $this->getValue('recipient'));
sort($recipients);
return sha1(sprintf('schedule:%d,%s', $scheduleID, implode(',', $recipients)), true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty line before return, pls

}

public function editEntry(int $scheduleId, int $id): void
{
$data = $this->formValuesToDb();
$timeperiodId = (int) $this->getValue('timeperiod_id');

$db = Database::get();
$db->beginTransaction();
$binaryBehavior = new Binary([]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use the behavior directly here as well.


$prevTimeperiodId = $this->getValue('timeperiod_id');
$suppliedHash = $this->getValue('membership_hash');
$calculatedHash = $this->getMembershipHashFromRecipients($scheduleId);
$changedMembers = ScheduleMember::on(Database::get())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you're also actually only interested in the timeperiod.

->with('timeperiod')
->filter(
Filter::all(
Filter::equal('timeperiod.owned_by_schedule_id', $scheduleId),
Filter::equal('schedule_member.membership_hash', $calculatedHash)
)
);

$db->update('timeperiod_entry', $data, ['id = ?' => $id]);
$prevTimeperiodEntriesCount = TimeperiodEntry::on($db)
->with('timeperiod_entry.timeperiod')
->filter(Filter::equal('timeperiod.owned_by_schedule_id', $scheduleId))
->filter(Filter::equal('timeperiod_id', $prevTimeperiodId))
->count();

$members = ScheduleMember::on($db)
->filter(Filter::all(
Filter::equal('schedule_id', $scheduleId),
Filter::equal('timeperiod_id', $timeperiodId)
));
$db->beginTransaction();

$recipients = explode(',', $this->getValue('recipient'));
if ($changedMembers->count() > 0) {
// Update the entries with timeperiod_id of the changed membership
$db->update(
'timeperiod_entry',
$data + ['timeperiod_id' => (int) $changedMembers->first()->timeperiod_id],
['id = ?' => $id]
);
$prevTimeperiodEntriesCount -= 1;
} elseif ($prevTimeperiodEntriesCount === 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case doesn't update the entry at all

// Update the membership hash and add or remove members for the combination of existing
// schedule_id and timeperiod_id
$recipients = explode(',', $this->getValue('recipient'));

$users = [];
$groups = [];
foreach ($recipients as $recipient) {
list($type, $id) = explode(':', $recipient, 2);
$users = [];
$groups = [];
foreach ($recipients as $recipient) {
list($type, $id) = explode(':', $recipient, 2);

if ($type === 'contact') {
$users[$id] = $id;
} elseif ($type === 'group') {
$groups[$id] = $id;
if ($type === 'contact') {
$users[$id] = $id;
} elseif ($type === 'group') {
$groups[$id] = $id;
}
}
}

$usersToRemove = [];
$groupsToRemove = [];
foreach ($members as $member) {
if ($member->contact_id !== null) {
if (! isset($users[$member->contact_id])) {
$usersToRemove[] = $member->contact_id;
} else {
unset($users[$member->contact_id]);
}
} else {
if (! isset($groups[$member->contactgroup_id])) {
$groupsToRemove[] = $member->contactgroup_id;
$usersToRemove = [];
$groupsToRemove = [];
$prevMembers = ScheduleMember::on(Database::get())
nilmerg marked this conversation as resolved.
Show resolved Hide resolved
->with('timeperiod')
->filter(
Filter::all(
Filter::equal('timeperiod.owned_by_schedule_id', $scheduleId),
Filter::equal('schedule_member.membership_hash', $suppliedHash)
)
);

foreach ($prevMembers as $member) {
if ($member->contact_id !== null) {
if (! isset($users[$member->contact_id])) {
$usersToRemove[] = $member->contact_id;
} else {
unset($users[$member->contact_id]);
}
} else {
unset($groups[$member->contactgroup_id]);
if (! isset($groups[$member->contactgroup_id])) {
$groupsToRemove[] = $member->contactgroup_id;
} else {
unset($groups[$member->contactgroup_id]);
}
}
}
}

if (! empty($usersToRemove)) {
$db->delete('schedule_member', [
'schedule_id = ?' => $scheduleId,
'timeperiod_id = ?' => $timeperiodId,
'contact_id IN (?)' => $usersToRemove
]);
}
if (! empty($usersToRemove)) {
$db->delete('schedule_member', [
'membership_hash = ?' => $suppliedHash,
'contact_id IN (?)' => $usersToRemove
]);
}

if (! empty($groupsToRemove)) {
$db->delete('schedule_member', [
'schedule_id = ?' => $scheduleId,
'timeperiod_id = ?' => $timeperiodId,
'contactgroup_id IN (?)' => $groupsToRemove
]);
}
if (! empty($groupsToRemove)) {
$db->delete('schedule_member', [
'membership_hash = ?' => $suppliedHash,
'contactgroup_id IN (?)' => $groupsToRemove
]);
}

foreach ($users as $user) {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contact_id' => $user
]);
$binaryMemberHash = $binaryBehavior->toDb($calculatedHash, null, null);

$db->update(
'schedule_member',
['membership_hash' => $binaryMemberHash],
['membership_hash = ?' => $suppliedHash]
);

foreach ($users as $user) {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $prevTimeperiodId,
'contact_id' => $user,
'membership_hash' => $binaryMemberHash
]);
}

foreach ($groups as $group) {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $prevTimeperiodId,
'contactgroup_id' => $group,
'membership_hash' => $binaryMemberHash
]);
}
} else {
// Create new timeperiod and new members for the newly generated hash and update timeperiod entries
$db->insert('timeperiod', ['owned_by_schedule_id' => $scheduleId]);
$timeperiodId = $db->lastInsertId();

$db->update(
'timeperiod_entry',
$data + ['timeperiod_id' => $timeperiodId],
['id = ?' => $id]
);

$prevTimeperiodEntriesCount -= 1;
if ($changedMembers->count() === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that check is redundant. If this case runs, the count is always zero.

$recipients = explode(',', $this->getValue('recipient'));

$binaryMemberHash = $binaryBehavior->toDb($calculatedHash, null, null);
foreach ($recipients as $recipient) {
list($type, $recipientId) = explode(':', $recipient, 2);
if ($type === 'contact') {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contact_id' => $recipientId,
'membership_hash' => $binaryMemberHash
]);
} elseif ($type === 'group') {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contactgroup_id' => $recipientId,
'membership_hash' => $binaryMemberHash
]);
}
}
}
}

foreach ($groups as $group) {
$db->insert('schedule_member', [
'schedule_id' => $scheduleId,
'timeperiod_id' => $timeperiodId,
'contactgroup_id' => $group
if ($prevTimeperiodEntriesCount === 0) {
$db->delete(
'schedule_member',
['membership_hash = ?' => $suppliedHash]
);

$db->delete('timeperiod', [
'id = ?' => $prevTimeperiodId,
'owned_by_schedule_id = ?' => $scheduleId
]);
}

Expand Down
12 changes: 11 additions & 1 deletion library/Notifications/Model/ScheduleMember.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

namespace Icinga\Module\Notifications\Model;

use ipl\Orm\Behavior\Binary;
use ipl\Orm\Behaviors;
use ipl\Orm\Model;
use ipl\Orm\Relations;

Expand All @@ -26,10 +28,18 @@ public function getColumns()
{
return [
'contact_id',
'contactgroup_id'
'contactgroup_id',
'membership_hash'
];
}

public function createBehaviors(Behaviors $behaviors)
{
$behaviors->add(new Binary([
'membership_hash'
]));
}

public function createRelations(Relations $relations)
{
$relations->hasOne('timeperiod', Timeperiod::class)
Expand Down