Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

Make zone serial increment properly #141

Closed
wants to merge 1 commit into from
Closed

Make zone serial increment properly #141

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
2 changes: 1 addition & 1 deletion app/Http/Controllers/ZoneController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private function fillZoneFromRequest(Zone $zone, Request $request): void
if ($request->input('zone_type') === 'secondary-zone') {
$zone->master_server = $request->input('master_server');
} else {
$zone->serial = Zone::generateSerialNumber();
$zone->getNewSerialNumber();
$zone->setPendingChanges();

// deal with checkboxes
Expand Down
4 changes: 2 additions & 2 deletions app/Providers/AppServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ public function boot()
// Touch Zone when save/delete a Record
Record::saving(function (Record $record) {
$zone = $record->zone()->first();
$zone->setPendingChanges(true);
$zone->getNewSerialNumber();
});
Record::deleting(function (Record $record) {
$zone = $record->zone()->first();
$zone->setPendingChanges(true);
$zone->getNewSerialNumber();
});
}

Expand Down
65 changes: 54 additions & 11 deletions app/Zone.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,17 +186,21 @@ public function records()
*/
public function getNewSerialNumber(bool $force = false): int
{
// Get current Zone serial number.
$currentSerial = $this->serial;

// We need a new one ONLY if there isn't pending changes.
if ($this->hasPendingChanges() && ! $force) {
return $currentSerial;
if (isset($this->serial)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I've seen you're addressing the use case when the serial is empty. Would you be so kind to add a test for this specific case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pacoorozco This became a problem because I changed ZoneController::fillZoneFromRequest to call getNewSerialNumber() instead of assign $zone->serial = Zone::generateSerialNumber(). This introduced the case of ! isset($this->serial).
The reason for that change was that this assignment was re-setting and decrementing serial numbers when the zone was being saved.

I'm not too familiar with Laravel etc., I see right now you're doing:
$zone = factory(Zone::class)->create();
$expectedSerial = $zone->serial;
and that worked, so I suppose this creates a zone with a pre-seeded serial?
So the test you're asking for would be more like:
$zone = new Zone();
$zone->getNewSerialNumber();
$this->assertGreaterThan(0, $zone->serial);

// Get current Zone serial number.
$currentSerial = $this->serial;

// We need a new one ONLY if there isn't pending changes.
if ($this->hasPendingChanges() && ! $force) {
return $currentSerial;
}

// Raise serial number
$this->serial = $this->raiseSerialNumber($currentSerial);
} else {
$this->serial = Zone::generateSerialNumber();
}

// Raise serial number
$this->serial = $this->raiseSerialNumber($currentSerial);

// Once Serial Number has changed, we have changes to push to servers.
$this->setPendingChanges(true);
$this->save();
Expand Down Expand Up @@ -275,8 +279,9 @@ public function raiseSerialNumber(int $currentSerial): int
// Create a new serial number YYYYMMDD00.
$nowSerial = Zone::generateSerialNumber();

return ($currentSerial >= $nowSerial)
? $currentSerial + 1
return Zone::compareSerialNumbers($currentSerial, $nowSerial) >= 0
// cap to 32 bits
? $currentSerial + 1 & 0xFFFFFFFF
: $nowSerial;
}

Expand All @@ -290,6 +295,44 @@ public static function generateSerialNumber(): int
return intval(Carbon::now()->format('Ymd') . '00');
}

/**
* Do serial number arithmetic.
*
* This function correctly compares two serial numbers across an
* overflow. That is, after reaching the maximum unsigned 32-bit value
* of 4294967295, the serial increments to 0. Thus, 0 > 4294967295.
*
* In C, this would be simply:
*
* int32_t compareSerialNumbers(uint32_t s1, uint32_t s2)
* {
* return (int32_t)(s1 - s2);
* }
*
* The aim is to do an unsigned substraction of two 32-bit integers,
* and then cast the result as a signed 32-bit integer.
*
* See RFC 1982
*
* Instead of ($s1 > $s2), use (compareSerialNumbers($s1, $s2) > 0)
*
* $param int $s1 The first serial number
* $param int $s2 The second serial number
*
*/
public static function compareSerialNumbers(int $s1, int $s2): int
{
// cap to 32 bits
$s1 &= 0xFFFFFFFF;
$s2 &= 0xFFFFFFFF;
$interval = $s1 - $s2;
if ($interval > 0x7FFFFFFF || $interval < -0x80000000) {
$interval = -(($interval ^ 0xFFFFFFFF) + 1);
}

return $interval;
}

/**
* Marks / unmark pending changes on this zone.
*
Expand Down