diff --git a/.gitignore b/.gitignore index 970182b9..58008dd4 100644 --- a/.gitignore +++ b/.gitignore @@ -20,11 +20,7 @@ yarn-error.log /package-lock.json /auth.json /composer.lock -/.fleet /.idea -/.nova -/.vscode -/.zed /coverage-report* /coverage.xml -/.claude +/.php-cs-fixer.dist.php diff --git a/app/Http/Controllers/Api/V1/PlatformChannelsController.php b/app/Http/Controllers/Api/V1/PlatformChannelsController.php index a72f5ed4..443ba540 100644 --- a/app/Http/Controllers/Api/V1/PlatformChannelsController.php +++ b/app/Http/Controllers/Api/V1/PlatformChannelsController.php @@ -8,6 +8,7 @@ use App\Models\PlatformAccount; use App\Models\PlatformChannel; use Exception; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Validation\ValidationException; @@ -51,6 +52,11 @@ public function store(StorePlatformChannelRequest $request, CreateChannelAction 'Platform channel created successfully and linked to platform account!', 201 ); + } catch (UniqueConstraintViolationException $e) { + // The (platform_instance_id, channel_id) unique index is the last line of + // defence if a duplicate slips past request validation — surface it as a + // validation error rather than leaking the driver's SQL message in a 500. + return $this->sendError('A channel with this name already exists for this instance.', [], 422); } catch (RuntimeException $e) { return $this->sendError($e->getMessage(), [], 422); } catch (Exception $e) { diff --git a/app/Http/Requests/StorePlatformChannelRequest.php b/app/Http/Requests/StorePlatformChannelRequest.php index 0950699f..0a1af8c6 100644 --- a/app/Http/Requests/StorePlatformChannelRequest.php +++ b/app/Http/Requests/StorePlatformChannelRequest.php @@ -3,6 +3,7 @@ namespace App\Http\Requests; use Illuminate\Foundation\Http\FormRequest; +use Illuminate\Validation\Rule; class StorePlatformChannelRequest extends FormRequest { @@ -12,15 +13,36 @@ public function authorize(): bool } /** - * @return array + * @return array|string> */ public function rules(): array { return [ 'platform_instance_id' => 'required|exists:platform_instances,id', - 'name' => 'required|string|max:255', + // name doubles as the Lemmy community slug (CreateChannelAction copies it + // verbatim into channel_id for community lookup at publish time), so it must + // be slug format and unique per instance — matching the Livewire create form. + 'name' => [ + 'required', + 'string', + 'max:255', + 'regex:/^[a-z0-9_]+$/', + Rule::unique('platform_channels', 'name') + ->where('platform_instance_id', $this->input('platform_instance_id')), + ], 'language_id' => 'nullable|exists:languages,id', 'description' => 'nullable|string', ]; } + + /** + * @return array + */ + public function messages(): array + { + return [ + 'name.regex' => 'The name must be a valid community slug (lowercase letters, numbers, and underscores only).', + 'name.unique' => 'A channel with this name already exists for this instance.', + ]; + } } diff --git a/app/Jobs/SyncChannelPostsJob.php b/app/Jobs/SyncChannelPostsJob.php index 3ded0649..edf86824 100644 --- a/app/Jobs/SyncChannelPostsJob.php +++ b/app/Jobs/SyncChannelPostsJob.php @@ -65,17 +65,19 @@ private function syncLemmyChannelPosts(LogSaver $logSaver): void throw new PlatformAuthException(PlatformEnum::LEMMY, 'No active account found for channel'); } - $api = new LemmyApiService($this->channel->platformInstance->url); + $api = $this->makeApiService($this->channel->platformInstance->url); $token = $this->getAuthToken($api, $account); - $platformChannelId = $this->channel->channel_id - ? $this->channel->channel_id - : $api->getCommunityId($this->channel->name, $token); + // channel_id holds a Lemmy community slug (non-numeric) or a numeric + // community id; syncChannelPosts() needs the numeric id. Mirror the + // resolution used in LemmyPublisher::createPost(). + $communityId = is_numeric($this->channel->channel_id) + ? (int) $this->channel->channel_id + : $api->getCommunityId($this->channel->channel_id, $token); - $api->syncChannelPosts($token, $platformChannelId, $this->channel->name); + $api->syncChannelPosts($token, $communityId, $this->channel->name); $logSaver->info('Channel posts synced successfully', $this->channel); - } catch (Exception $e) { $logSaver->error('Failed to sync channel posts', $this->channel, [ 'error' => $e->getMessage(), @@ -85,6 +87,15 @@ private function syncLemmyChannelPosts(LogSaver $logSaver): void } } + /** + * Seam for testing — LemmyApiService needs a per-instance URL so it cannot be + * container-resolved. Override in tests to inject a mock. + */ + protected function makeApiService(string $instanceUrl): LemmyApiService + { + return new LemmyApiService($instanceUrl); + } + /** * @throws PlatformAuthException */ diff --git a/app/Livewire/Channels.php b/app/Livewire/Channels.php index 2dab6276..0b785186 100644 --- a/app/Livewire/Channels.php +++ b/app/Livewire/Channels.php @@ -8,6 +8,7 @@ use App\Models\PlatformChannel; use App\Models\PlatformInstance; use Illuminate\Contracts\View\View; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Validation\Rule; use Livewire\Component; use RuntimeException; @@ -73,6 +74,13 @@ public function createChannel(CreateChannelAction $action): void // Blade textarea binds an empty string when blank; the action expects null for "no description". $this->newDescription !== '' ? $this->newDescription : null, ); + } catch (UniqueConstraintViolationException $e) { + // Unreachable via this form (the unique rule above catches duplicates first), + // but the (platform_instance_id, channel_id) index can still fire if channel_id + // ever drifts from name. Surface it as a field error instead of a 500. + $this->addError('newName', 'A channel with this name already exists for this instance.'); + + return; } catch (RuntimeException $e) { $this->addError('newPlatformInstanceId', $e->getMessage()); diff --git a/app/Models/PlatformChannel.php b/app/Models/PlatformChannel.php index 9291a36e..055b5f60 100644 --- a/app/Models/PlatformChannel.php +++ b/app/Models/PlatformChannel.php @@ -15,7 +15,7 @@ * @property int $id * @property int $platform_instance_id * @property PlatformInstance $platformInstance - * @property int $channel_id + * @property string $channel_id * @property string $name * @property int $language_id * @property Language|null $language diff --git a/database/migrations/2024_01_01_000011_add_channel_id_unique_to_platform_channels.php b/database/migrations/2024_01_01_000011_add_channel_id_unique_to_platform_channels.php new file mode 100644 index 00000000..952f9b64 --- /dev/null +++ b/database/migrations/2024_01_01_000011_add_channel_id_unique_to_platform_channels.php @@ -0,0 +1,57 @@ + getCommunityId). It is kept equal to `name` on create, + * but only `name` had a uniqueness guarantee. Enforce the same scope on + * channel_id so duplicate/ambiguous community references cannot exist. + */ + public function up(): void + { + $this->guardAgainstDuplicates(); + + Schema::table('platform_channels', function (Blueprint $table) { + $table->unique(['platform_instance_id', 'channel_id'], 'platform_channels_channel_id_unique'); + }); + } + + /** + * Pre-flight check so an existing database with drifted data fails with an + * actionable message naming the offending rows, rather than a raw driver error. + */ + private function guardAgainstDuplicates(): void + { + $duplicates = DB::table('platform_channels') + ->select('platform_instance_id', 'channel_id', DB::raw('COUNT(*) as total')) + ->groupBy('platform_instance_id', 'channel_id') + ->havingRaw('COUNT(*) > 1') + ->get(); + + if ($duplicates->isEmpty()) { + return; + } + + $details = $duplicates + ->map(fn ($row) => "platform_instance_id={$row->platform_instance_id} channel_id='{$row->channel_id}' ({$row->total} rows)") + ->implode('; '); + + throw new RuntimeException( + 'Cannot add unique index on platform_channels (platform_instance_id, channel_id): '. + 'duplicate rows exist. Resolve these before migrating: '.$details + ); + } + + public function down(): void + { + Schema::table('platform_channels', function (Blueprint $table) { + $table->dropUnique('platform_channels_channel_id_unique'); + }); + } +}; diff --git a/tests/Feature/Http/Controllers/Api/V1/PlatformChannelsControllerTest.php b/tests/Feature/Http/Controllers/Api/V1/PlatformChannelsControllerTest.php index 84f06cbe..9ad57d9e 100644 --- a/tests/Feature/Http/Controllers/Api/V1/PlatformChannelsControllerTest.php +++ b/tests/Feature/Http/Controllers/Api/V1/PlatformChannelsControllerTest.php @@ -111,6 +111,69 @@ public function test_store_validates_platform_instance_exists(): void ->assertJsonValidationErrors(['platform_instance_id']); } + public function test_store_rejects_non_slug_name(): void + { + $instance = PlatformInstance::factory()->create(); + + // name is copied verbatim into channel_id and used as the Lemmy community + // reference, so non-slug values must be rejected at the API boundary too. + $response = $this->postJson('/api/v1/platform-channels', [ + 'platform_instance_id' => $instance->id, + 'name' => 'Tech News', + ]); + + $response->assertStatus(422) + ->assertJsonValidationErrors(['name']); + + $this->assertDatabaseCount('platform_channels', 0); + } + + public function test_store_rejects_duplicate_name_for_same_instance(): void + { + $instance = PlatformInstance::factory()->create(); + PlatformChannel::factory()->create([ + 'platform_instance_id' => $instance->id, + 'name' => 'tech_news', + 'channel_id' => 'tech_news', + ]); + + $response = $this->postJson('/api/v1/platform-channels', [ + 'platform_instance_id' => $instance->id, + 'name' => 'tech_news', + ]); + + $response->assertStatus(422) + ->assertJsonValidationErrors(['name']); + + $this->assertDatabaseCount('platform_channels', 1); + } + + public function test_store_allows_same_name_on_different_instance(): void + { + $instanceA = PlatformInstance::factory()->create(); + $instanceB = PlatformInstance::factory()->create(); + + PlatformAccount::factory()->create([ + 'instance_url' => $instanceB->url, + 'is_active' => true, + ]); + + PlatformChannel::factory()->create([ + 'platform_instance_id' => $instanceA->id, + 'name' => 'tech_news', + 'channel_id' => 'tech_news', + ]); + + $response = $this->postJson('/api/v1/platform-channels', [ + 'platform_instance_id' => $instanceB->id, + 'name' => 'tech_news', + ]); + + $response->assertStatus(201); + + $this->assertDatabaseCount('platform_channels', 2); + } + public function test_show_returns_platform_channel_successfully(): void { $instance = PlatformInstance::factory()->create(); diff --git a/tests/Unit/Jobs/SyncChannelPostsJobTest.php b/tests/Unit/Jobs/SyncChannelPostsJobTest.php index d439bff1..db31c90a 100644 --- a/tests/Unit/Jobs/SyncChannelPostsJobTest.php +++ b/tests/Unit/Jobs/SyncChannelPostsJobTest.php @@ -7,6 +7,7 @@ use App\Models\PlatformAccount; use App\Models\PlatformChannel; use App\Models\PlatformInstance; +use App\Modules\Lemmy\Services\LemmyApiService; use App\Services\Log\LogSaver; use Exception; use Illuminate\Contracts\Queue\ShouldBeUnique; @@ -132,6 +133,110 @@ public function test_handle_logs_start_message(): void $this->assertTrue(true); } + public function test_sync_resolves_non_numeric_channel_id_via_get_community_id(): void + { + [$channel, $account] = $this->makeSyncableChannel('tech_news'); + + $apiMock = Mockery::mock(LemmyApiService::class); + $apiMock->shouldReceive('login') + ->once() + ->with($account->username, $account->password) + ->andReturn('token'); + $apiMock->shouldReceive('getCommunityId') + ->once() + ->with('tech_news', 'token') + ->andReturn(42); + $apiMock->shouldReceive('syncChannelPosts') + ->once() + ->with('token', 42, $channel->name); + + $logSaverMock = Mockery::mock(LogSaver::class); + $logSaverMock->shouldReceive('info')->zeroOrMoreTimes(); + $logSaverMock->shouldReceive('error')->zeroOrMoreTimes(); + + $this->makeJobWithApi($channel, $apiMock)->handle($logSaverMock); + + // The behaviour under test is the mocked call sequence above; assert explicitly + // so PHPUnit does not flag the test as risky for performing no assertions. + $this->addToAssertionCount(1); + } + + public function test_sync_uses_numeric_channel_id_directly_without_lookup(): void + { + [$channel, $account] = $this->makeSyncableChannel('42'); + + $apiMock = Mockery::mock(LemmyApiService::class); + $apiMock->shouldReceive('login') + ->once() + ->with($account->username, $account->password) + ->andReturn('token'); + // The behaviour that matters here is that no community lookup happens — the + // numeric channel_id is used as-is. (The int-ness of the argument is not worth + // asserting: syncChannelPosts() declares `int $platformChannelId`, so PHP coerces + // '42' at the call boundary whether or not the job casts it first.) + $apiMock->shouldNotReceive('getCommunityId'); + $apiMock->shouldReceive('syncChannelPosts') + ->once() + ->with('token', 42, $channel->name); + + $logSaverMock = Mockery::mock(LogSaver::class); + $logSaverMock->shouldReceive('info')->zeroOrMoreTimes(); + $logSaverMock->shouldReceive('error')->zeroOrMoreTimes(); + + $this->makeJobWithApi($channel, $apiMock)->handle($logSaverMock); + + // As above: the shouldNotReceive('getCommunityId') expectation is the assertion. + $this->addToAssertionCount(1); + } + + /** + * @return array{0: PlatformChannel, 1: PlatformAccount} + */ + private function makeSyncableChannel(string $channelId): array + { + $platformInstance = PlatformInstance::factory()->create([ + 'platform' => PlatformEnum::LEMMY, + 'url' => 'https://lemmy.example.com', + ]); + + $account = PlatformAccount::factory()->create([ + 'instance_url' => $platformInstance->url, + 'is_active' => true, + ]); + + $channel = PlatformChannel::factory()->create([ + 'platform_instance_id' => $platformInstance->id, + 'name' => 'tech_news', + 'channel_id' => $channelId, + 'is_active' => true, + ]); + + $channel->platformAccounts()->attach($account->id, [ + 'is_active' => true, + 'priority' => 1, + 'created_at' => now(), + 'updated_at' => now(), + ]); + + return [$channel->fresh(['platformInstance']), $account]; + } + + private function makeJobWithApi(PlatformChannel $channel, LemmyApiService $api): SyncChannelPostsJob + { + return new class($channel, $api) extends SyncChannelPostsJob + { + public function __construct(PlatformChannel $channel, private readonly LemmyApiService $api) + { + parent::__construct($channel); + } + + protected function makeApiService(string $instanceUrl): LemmyApiService + { + return $this->api; + } + }; + } + public function test_job_can_be_serialized(): void { $platformInstance = PlatformInstance::factory()->create(); diff --git a/tests/Unit/Models/PlatformChannelTest.php b/tests/Unit/Models/PlatformChannelTest.php index f6e81bab..6e6bfeb4 100644 --- a/tests/Unit/Models/PlatformChannelTest.php +++ b/tests/Unit/Models/PlatformChannelTest.php @@ -9,6 +9,7 @@ use App\Models\PlatformInstance; use App\Models\Route; use Carbon\Carbon; +use Illuminate\Database\UniqueConstraintViolationException; use Illuminate\Foundation\Testing\RefreshDatabase; use Tests\TestCase; @@ -45,6 +46,42 @@ public function test_casts_is_active_to_boolean(): void $this->assertFalse($channel->is_active); } + public function test_channel_id_is_unique_per_platform_instance(): void + { + $instance = PlatformInstance::factory()->create(); + PlatformChannel::factory()->create([ + 'platform_instance_id' => $instance->id, + 'name' => 'tech', + 'channel_id' => 'tech', + ]); + + $this->expectException(UniqueConstraintViolationException::class); + + PlatformChannel::factory()->create([ + 'platform_instance_id' => $instance->id, + 'name' => 'tech_alias', + 'channel_id' => 'tech', + ]); + } + + public function test_same_channel_id_allowed_across_different_instances(): void + { + $instanceA = PlatformInstance::factory()->create(); + $instanceB = PlatformInstance::factory()->create(); + + PlatformChannel::factory()->create([ + 'platform_instance_id' => $instanceA->id, + 'channel_id' => 'tech', + ]); + $second = PlatformChannel::factory()->create([ + 'platform_instance_id' => $instanceB->id, + 'channel_id' => 'tech', + ]); + + $this->assertDatabaseCount('platform_channels', 2); + $this->assertEquals('tech', $second->channel_id); + } + public function test_belongs_to_platform_instance_relationship(): void { $instance = PlatformInstance::factory()->create();