diff --git a/CHANGELOG.md b/CHANGELOG.md index 7befc39a..e847f955 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,11 @@ a version is cut. ### Fixed +- **A deleted folder no longer takes its name with it.** Deleting a folder, file or group left the + name reserved for good: creating another one with that name failed with *"The slug has already + been taken"*, naming a conflict with a row the interface will not show you, and there was no way + to release it from any screen. Deleting now hands the name back. Names already held by things you + deleted earlier are released when you update. ([#1645](https://github.com/projectsend/projectsend/issues/1645)) - **The Legacy migration tool installs with the command the guide gives you.** `composer require projectsend/v1-migration-tool` failed with *Could not find a matching version of package* on a fresh installation, because the tool is not published on Packagist and nothing told Composer diff --git a/app/Support/Concerns/HasUniqueSlug.php b/app/Support/Concerns/HasUniqueSlug.php index ac4584c9..d71006d8 100644 --- a/app/Support/Concerns/HasUniqueSlug.php +++ b/app/Support/Concerns/HasUniqueSlug.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Support\Concerns; +use App\Support\VacatedSlug; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Str; @@ -16,9 +17,30 @@ use Illuminate\Support\Str; * create one without going through a form that requires it — so fall back to * deriving one from the name rather than failing at the database. * - * Soft-deleted rows still count as collisions: their slugs remain reachable - * until the row is really gone, and reusing one would resurrect the wrong - * URL. + * Deleting a row hands its slug back. A soft-deleted row is unreachable — + * every public lookup goes through Eloquent, so the soft-delete scope has + * already excluded it — and nothing in the application can restore one, so + * a slug it kept holding would be reserved for a page that can never return. + * Holding one is what burned the name of any deleted folder for good: the + * unique index still saw the row, so the name could never be used again and + * the screen could not say why, because the row it collided with is one the + * interface will not show. + * + * The database is what forces the vacating to be a rewrite rather than a + * softer lookup. Teaching the collision checks to ignore trashed rows is not + * enough on its own — two rows would then both hold `report`, and the unique + * index rejects that however the application feels about it. So the slug + * moves into a namespace nothing else can occupy: `report__deleted-42`. + * Underscores are the whole trick. Str::slug() turns them into hyphens and + * Rules::slug() refuses them outright, so neither a derived slug nor a + * hand-typed one can ever land on a vacated slug, whatever the row is called. + * + * The collision checks still count trashed rows anyway. It costs nothing, and + * it keeps them honest about what the index will actually accept if a row is + * ever soft-deleted by something that bypasses model events. + * + * Requires SoftDeletes: the collision check reaches for withTrashed(), and + * vacating is only meaningful for a row that outlives its own delete. * * @phpstan-require-extends Model */ @@ -35,6 +57,10 @@ trait HasUniqueSlug $model->slug = static::uniqueSlugFrom($model->name); } }); + + static::deleted(function (self $model): void { + static::vacateSlug($model); + }); } /** @@ -60,6 +86,39 @@ trait HasUniqueSlug return $slug; } + /** + * Hands a soft-deleted row's slug back to the names still in use. + * + * Written through the query builder on purpose: this is bookkeeping, not + * an edit somebody made, so it must not move updated_at or fire a second + * round of model events on a row that is already on its way out. + */ + protected static function vacateSlug(self $model): void + { + // A row that is really gone took its slug with it. `deleted` fires + // for forceDelete() too, and there is nothing left to rewrite. + if ($model->isForceDeleting()) { + return; + } + + if (blank($model->slug)) { + return; + } + + $vacated = VacatedSlug::for($model->slug, $model->getKey()); + + if ($vacated === $model->slug) { + return; + } + + static::query()->withTrashed()->whereKey($model->getKey())->toBase()->update(['slug' => $vacated]); + + // Keep the in-memory row telling the truth for whatever the caller + // does with it after the delete returns. + $model->slug = $vacated; + $model->syncOriginalAttribute('slug'); + } + /** * Stands in when the name slugs to nothing at all — a name of only * punctuation or of characters Str::slug() drops entirely. diff --git a/app/Support/VacatedSlug.php b/app/Support/VacatedSlug.php new file mode 100644 index 00000000..0a7a16e8 --- /dev/null +++ b/app/Support/VacatedSlug.php @@ -0,0 +1,55 @@ +whereNotNull('deleted_at') + ->whereNotNull('slug') + ->where('slug', 'not like', '%'.VacatedSlug::MARKER.'%') + // Chunked because an installation that has been running a + // while can have a lot of these, and each row's new slug + // depends on its own id rather than on anything set-wide. + ->orderBy('id') + ->chunkById(500, function (Collection $rows) use ($table): void { + foreach ($rows as $row) { + DB::table($table) + ->where('id', $row->id) + ->update(['slug' => VacatedSlug::for((string) $row->slug, (int) $row->id)]); + } + }); + } + } + + /** + * Deliberately does not put the old slugs back. By the time this is rolled + * back the names may well have been taken by live rows, and restoring them + * would break the unique index this migration exists to stop tripping over. + * A deleted row keeping a vacated slug harms nothing on older code — it + * reads as an unusual name on a row nobody can see. + */ + public function down(): void {} +}; diff --git a/tests/Feature/Files/FoldersTest.php b/tests/Feature/Files/FoldersTest.php index 262348f6..1568d3ac 100644 --- a/tests/Feature/Files/FoldersTest.php +++ b/tests/Feature/Files/FoldersTest.php @@ -197,6 +197,26 @@ test('folder ownership splits own from others for edit and delete', function () $this->delete("/folders/{$others->id}")->assertForbidden(); }); +// Issue #1645, end to end through the screen the report came from: a deleted +// folder used to keep its public URL, so the name could never be used again +// and the error named a row the interface will not show. +test('a deleted folder gives its name and its public URL back', function () { + $this->actingAs($this->admin); + + $this->post('/folders', ['name' => 'Quarterly', 'public' => true, 'slug' => 'quarterly']) + ->assertRedirect()->assertSessionHasNoErrors(); + + $first = Folder::query()->where('name', 'Quarterly')->sole(); + $this->delete("/folders/{$first->id}")->assertRedirect(); + + $this->post('/folders', ['name' => 'Quarterly', 'public' => true, 'slug' => 'quarterly']) + ->assertRedirect()->assertSessionHasNoErrors(); + + $second = Folder::query()->where('name', 'Quarterly')->sole(); + expect($second->id)->not->toBe($first->id) + ->and($second->slug)->toBe('quarterly'); +}); + test('creating folders requires create_own_folders', function () { // Account Manager lacks create_own_folders in the v1 default set. $manager = User::factory()->role(SystemRole::AccountManager)->create(); diff --git a/tests/Feature/Files/HasUniqueSlugTest.php b/tests/Feature/Files/HasUniqueSlugTest.php index b0691d83..edda3157 100644 --- a/tests/Feature/Files/HasUniqueSlugTest.php +++ b/tests/Feature/Files/HasUniqueSlugTest.php @@ -6,7 +6,11 @@ use App\Models\User; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; use App\Modules\Groups\Models\Group; +use App\Support\Rules; +use App\Support\VacatedSlug; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Facades\Validator; +use Illuminate\Support\Str; beforeEach(function () { Storage::fake('files'); @@ -34,12 +38,93 @@ test('colliding names get a numeric suffix that keeps counting up', function () ->and(File::factory()->create(['name' => 'Report'])->slug)->toBe('report-3'); }); -test('a soft-deleted row still collides, so its URL is not silently reused', function () { +// Deleting used to hold the slug forever, which burned the name: nothing can +// restore a trashed row, so it was reserved for a page that could never come +// back. See issue #1645. +test('deleting a row hands its slug back', function () { $first = File::factory()->create(['name' => 'Report']); $first->delete(); expect($first->trashed())->toBeTrue() - ->and(File::factory()->create(['name' => 'Report'])->slug)->toBe('report-2'); + ->and(File::factory()->create(['name' => 'Report'])->slug)->toBe('report'); +}); + +test('the vacated slug is parked where nothing can collide with it', function () { + $file = File::factory()->create(['name' => 'Report']); + $file->delete(); + + $parked = File::withTrashed()->whereKey($file->id)->value('slug'); + + expect($parked)->toBe('report'.VacatedSlug::MARKER.$file->id) + // Neither route into a slug can produce that string, which is what + // makes parking there safe rather than merely unlikely. + ->and(Str::slug($parked))->not->toContain('_') + ->and(Validator::make( + ['slug' => $parked, 'public' => true], + ['slug' => Rules::slug('files')], + )->fails())->toBeTrue(); +}); + +test('the name is reusable however many times it is deleted', function () { + foreach (range(1, 3) as $ignored) { + $file = File::factory()->create(['name' => 'Report']); + expect($file->slug)->toBe('report'); + $file->delete(); + } + + // Three deleted rows, each parked under its own id rather than piling up + // as report-2, report-3, report-4 the way the suffix used to. + expect(File::withTrashed()->where('slug', 'like', 'report'.VacatedSlug::MARKER.'%')->count())->toBe(3); +}); + +test('all three models hand the slug back', function () { + $file = File::factory()->create(['name' => 'Shared']); + $folder = Folder::query()->create(['name' => 'Shared', 'path' => '/']); + $group = Group::query()->create(['name' => 'Shared']); + + $file->delete(); + $folder->delete(); + $group->delete(); + + expect(File::factory()->create(['name' => 'Shared'])->slug)->toBe('shared') + ->and(Folder::query()->create(['name' => 'Shared', 'path' => '/'])->slug)->toBe('shared') + ->and(Group::query()->create(['name' => 'Shared'])->slug)->toBe('shared'); +}); + +// The reported bug: a public folder's slug is user-supplied and validated +// against the table, so a trashed row holding it made the name unusable with +// an error naming a row the interface will not show. +test('a deleted public row does not block its slug at the validator', function () { + $folder = Folder::query()->create(['name' => 'Quarterly', 'path' => '/', 'public' => true, 'slug' => 'quarterly']); + $folder->delete(); + + expect(Validator::make( + ['slug' => 'quarterly', 'public' => true], + ['slug' => Rules::slug('folders')], + )->fails())->toBeFalse(); +}); + +test('force-deleting leaves nothing behind to vacate', function () { + $file = File::factory()->create(['name' => 'Report']); + $file->forceDelete(); + + expect(File::withTrashed()->whereKey($file->id)->exists())->toBeFalse() + ->and(File::factory()->create(['name' => 'Report'])->slug)->toBe('report'); +}); + +test('vacating does not look like somebody edited the row', function () { + $file = File::factory()->create(['name' => 'Report']); + + $this->travel(5)->minutes(); + $file->delete(); + + // The soft delete moves updated_at itself; what matters is that vacating + // the slug afterwards is not a second write on top of it, so the two + // stamps the delete wrote still agree. + $row = File::withTrashed()->whereKey($file->id)->sole(); + + expect($row->slug)->toBe('report'.VacatedSlug::MARKER.$file->id) + ->and($row->updated_at)->toEqual($row->deleted_at); }); test('a name that slugs to nothing falls back to a per-model default', function () {