Restore the documented status-code contract of DELETE /subject/{id}

src/Application/Actions/DeleteSubject/DeleteSubjectAction.php

@@ -5,16 +5,19 @@
 namespace ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject;
 
 use ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectRepository;
+use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectEditNotAuthorizedException;
 use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException;
 use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId;
-use RuntimeException;
+use ProfessionalWiki\NeoWiki\Persistence\MediaWiki\PageContentSavingStatus;
 
 readonly class DeleteSubjectAction {
 
 	public function __construct(
 		private SubjectRepository $subjectRepository,
+		private PageReadAuthorizer $readAuthorizer,
 		private SubjectWriteAuthorizer $writeAuthorizer,
 		private PageIdentifiersLookup $pageIdentifiersLookup
 	) {
@@ -23,17 +26,26 @@ public function __construct(
 	public function deleteSubject( SubjectId $subjectId, ?string $comment ): void {
 		$pageId = $this->pageIdentifiersLookup->getPageIdOfSubject( $subjectId )?->getId();
 
-		// A Subject on no page has no page rights to check, so it is answered as absent rather than as
-		// forbidden.
-		if ( $pageId === null ) {
+		// Gate on read before write: a Subject on a page the caller may not read answers exactly like
+		// one that does not exist, and so does a Subject on no page, which has no page rights to
+		// check. Reaching the write check first would answer 403 where a restricted page answers
+		// 404. See PageReadAuthorizer for why a denied read takes the not-found shape.
+		if ( $pageId === null || !$this->readAuthorizer->authorizeReadByPageId( $pageId ) ) {
 			throw SubjectNotFoundException::forId( $subjectId );
 		}
 
 		if ( !$this->writeAuthorizer->authorize( $pageId ) ) {
-			throw new RuntimeException( 'You do not have the necessary permissions to delete this subject' );
+			throw new SubjectEditNotAuthorizedException( 'You do not have the necessary permissions to delete this subject' );
 		}
 
-		$this->subjectRepository->deleteSubject( $subjectId, $comment );
+		$status = $this->subjectRepository->deleteSubject( $subjectId, $comment );
+
+		// Only a new revision means the Subject was removed. The index can name a page whose slot no
+		// longer holds it, and the page can go away between the checks above and the write; either
+		// way the Subject the caller named is not there, which is what an absent one answers.
+		if ( $status->status !== PageContentSavingStatus::REVISION_CREATED ) {
+			throw SubjectNotFoundException::forId( $subjectId );
+		}
 	}
 
 }

src/Application/SubjectRepository.php

@@ -21,8 +21,12 @@ public function updateSubject( Subject $subject, ?string $comment = null ): void
 
 	/**
 	 * TODO: document exceptions
+	 *
+	 * Answers REVISION_CREATED only when the Subject was actually removed. The index can name a page
+	 * whose slot no longer holds it, and the page can go away under the write; both answer
+	 * NO_CHANGES or ERROR rather than reporting a deletion that did not happen.
 	 */
-	public function deleteSubject( SubjectId $id, ?string $comment ): void;
+	public function deleteSubject( SubjectId $id, ?string $comment ): PageContentSavingStatus;
 
 	/**
 	 * TODO: document exceptions

src/EntryPoints/REST/DeleteSubjectApi.php

@@ -4,14 +4,15 @@
 
 namespace ProfessionalWiki\NeoWiki\EntryPoints\REST;
 
+use InvalidArgumentException;
 use MediaWiki\Rest\HttpException;
 use MediaWiki\Rest\Response;
 use MediaWiki\Rest\SimpleHandler;
+use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectEditNotAuthorizedException;
 use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException;
 use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId;
 use ProfessionalWiki\NeoWiki\NeoWikiExtension;
 use ProfessionalWiki\NeoWiki\Presentation\CsrfValidator;
-use RuntimeException;
 use Wikimedia\ParamValidator\ParamValidator;
 
 class DeleteSubjectApi extends SimpleHandler {
@@ -36,12 +37,17 @@ public function run( string $subjectId ): Response {
 				new SubjectId( $subjectId ),
 				$comment
 			);
+		} catch ( InvalidArgumentException $e ) {
+			return $this->getResponseFactory()->createHttpError( 400, [
+				'status' => 'error',
+				'message' => $e->getMessage(),
+			] );
 		} catch ( SubjectNotFoundException $e ) {
 			return $this->getResponseFactory()->createHttpError( 404, [
 				'status' => 'error',
 				'message' => $e->getMessage(),
 			] );
-		} catch ( RuntimeException $e ) {
+		} catch ( SubjectEditNotAuthorizedException $e ) {
 			return $this->getResponseFactory()->createHttpError( 403, [
 				'status' => 'error',
 				'message' => $e->getMessage(),

src/NeoWikiExtension.php

@@ -1242,6 +1242,7 @@ private function getPageIdentifiersLookup(): PageIdentifiersLookup {
 	public function newDeleteSubjectAction( Authority $authority ): DeleteSubjectAction {
 		return new DeleteSubjectAction(
 			subjectRepository: $this->getSubjectRepository(),
+			readAuthorizer: $this->newPageReadAuthorizer( $authority ),
 			writeAuthorizer: $this->newSubjectWriteAuthorizer( $authority ),
 			pageIdentifiersLookup: $this->getPageIdentifiersLookup()
 		);

src/Persistence/MediaWiki/Subject/MediaWikiSubjectRepository.php

@@ -105,24 +105,32 @@ private function saveContent( SubjectContent $content, PageId $pageId, ?string $
 		);
 	}
 
-	public function deleteSubject( SubjectId $id, ?string $comment ): void {
+	public function deleteSubject( SubjectId $id, ?string $comment ): PageContentSavingStatus {
 		$pageId = $this->getPageIdForSubject( $id );
 
 		if ( $pageId === null ) {
-			return;
+			return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES );
 		}
 
 		$content = $this->getContentByPageId( $pageId );
 
 		if ( $content === null ) {
-			return;
+			return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES );
+		}
+
+		// Asked of the slot rather than inferred from the save: mutatePageSubjects re-serializes the
+		// slot whatever the mutation did, so a slot written by anything other than the serializer -
+		// an import, or Special:NeoJson - yields new bytes and a real revision even when the removal
+		// removed nothing.
+		if ( $content->getPageSubjects()->getAllSubjects()->getSubject( $id ) === null ) {
+			return new PageContentSavingStatus( PageContentSavingStatus::NO_CHANGES );
 		}
 
 		$content->mutatePageSubjects( function( PageSubjects $pageSubjects ) use ( $id ): void {
 			$pageSubjects->removeSubject( $id );
 		} );
 
-		$this->saveContent( $content, $pageId, $comment );
+		return $this->saveContent( $content, $pageId, $comment );
 	}
 
 	public function getMainSubject( PageId $pageId ): ?Subject {

tests/phpunit/Application/Actions/DeleteSubjectActionTest.php

@@ -7,16 +7,20 @@
 use PHPUnit\Framework\TestCase;
 use ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject\DeleteSubjectAction;
 use ProfessionalWiki\NeoWiki\Application\PageIdentifiersLookup;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectRepository;
 use ProfessionalWiki\NeoWiki\Application\SubjectWriteAuthorizer;
+use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectEditNotAuthorizedException;
 use ProfessionalWiki\NeoWiki\Application\Subject\Exception\SubjectNotFoundException;
 use ProfessionalWiki\NeoWiki\Domain\Page\PageId;
 use ProfessionalWiki\NeoWiki\Domain\Page\PageIdentifiers;
 use ProfessionalWiki\NeoWiki\Domain\Subject\SubjectId;
 use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject;
 use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemoryPageIdentifiersLookup;
 use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySubjectRepository;
+use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpyPageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SpySubjectWriteAuthorizer;
+use ProfessionalWiki\NeoWiki\Tests\TestDoubles\StubPageReadAuthorizer;
 
 /**
  * @covers \ProfessionalWiki\NeoWiki\Application\Actions\DeleteSubject\DeleteSubjectAction
@@ -57,14 +61,29 @@ public function testAuthorizesAgainstTheSubjectsResolvedPage(): void {
 		$this->assertEquals( new PageId( 7 ), $authorizer->authorizedPageId );
 	}
 
+	public function testGatesTheReadOnTheSubjectsResolvedPage(): void {
+		$readAuthorizer = new SpyPageReadAuthorizer( allowed: true );
+
+		$this->newAction(
+			$this->newRepositoryWithSubject(),
+			new SpySubjectWriteAuthorizer( allowed: true ),
+			new InMemoryPageIdentifiersLookup( [
+				[ new SubjectId( self::SUBJECT_ID ), new PageIdentifiers( new PageId( 7 ), 'Owning page', 0 ) ]
+			] ),
+			readAuthorizer: $readAuthorizer
+		)->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
+
+		$this->assertEquals( new PageId( 7 ), $readAuthorizer->authorizedPageId );
+	}
+
 	public function testThrowsWhenUserMayNotDeleteSubject(): void {
 		$action = $this->newAction(
 			new InMemorySubjectRepository(),
 			new SpySubjectWriteAuthorizer( allowed: false ),
 			$this->pageIdentifiersLookupWithSubject()
 		);
 
-		$this->expectException( \RuntimeException::class );
+		$this->expectException( SubjectEditNotAuthorizedException::class );
 		$this->expectExceptionMessage( 'You do not have the necessary permissions to delete this subject' );
 
 		$action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
@@ -86,6 +105,73 @@ public function testUnresolvableSubjectIsReportedAsNotFound(): void {
 		$action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
 	}
 
+	public function testUnreadablePageAnswersNotFound(): void {
+		$action = $this->newActionOnUnreadablePage( $this->newRepositoryWithSubject() );
+
+		$this->expectException( SubjectNotFoundException::class );
+		// Anchored: a read denial that added anything of its own would tell the two answers apart.
+		$this->expectExceptionMessageMatches( '/^Subject not found: ' . self::SUBJECT_ID . '$/' );
+
+		$action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
+	}
+
+	public function testUnreadablePageIsRejectedBeforeTheDeletion(): void {
+		$repository = $this->newRepositoryWithSubject();
+
+		try {
+			$this->newActionOnUnreadablePage( $repository )
+				->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
+		} catch ( SubjectNotFoundException ) {
+		}
+
+		$this->assertNotNull( $repository->getSubject( new SubjectId( self::SUBJECT_ID ) ) );
+	}
+
+	public function testReadDenialTakesPrecedenceOverWriteDenial(): void {
+		// A page the caller can neither read nor edit answers not-found, never the write 403, so a
+		// hidden page is indistinguishable from an absent one.
+		$action = $this->newAction(
+			$this->newRepositoryWithSubject(),
+			new SpySubjectWriteAuthorizer( allowed: false ),
+			$this->pageIdentifiersLookupWithSubject(),
+			readAuthorizer: new StubPageReadAuthorizer( allowed: false )
+		);
+
+		$this->expectException( SubjectNotFoundException::class );
+
+		$action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
+	}
+
+	public function testWriteThatRemovedNothingAnswersNotFound(): void {
+		// Reporting success would tell the caller a Subject that is not there was deleted. Which of
+		// the repository's reasons produced it is its own business, and pinned in its tests.
+		$action = $this->newAction(
+			new InMemorySubjectRepository(),
+			new SpySubjectWriteAuthorizer( allowed: true ),
+			$this->pageIdentifiersLookupWithSubject()
+		);
+
+		$this->expectException( SubjectNotFoundException::class );
+
+		$action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
+	}
+
+	public function testPageLostUnderTheWriteAnswersNotFound(): void {
+		// The page passed both checks and then went away before the save landed.
+		$repository = $this->newRepositoryWithSubject();
+		$repository->failNextSave = true;
+
+		$action = $this->newAction(
+			$repository,
+			new SpySubjectWriteAuthorizer( allowed: true ),
+			$this->pageIdentifiersLookupWithSubject()
+		);
+
+		$this->expectException( SubjectNotFoundException::class );
+
+		$action->deleteSubject( new SubjectId( self::SUBJECT_ID ), null );
+	}
+
 	private function newRepositoryWithSubject(): InMemorySubjectRepository {
 		$repository = new InMemorySubjectRepository();
 		$repository->updateSubject( TestSubject::build( id: self::SUBJECT_ID ) );
@@ -104,12 +190,31 @@ private function newAllowingAction( SubjectRepository $repository ): DeleteSubje
 		);
 	}
 
+	/**
+	 * The caller may edit the Subject's page but may not read it: the case a missing read gate would
+	 * let through.
+	 */
+	private function newActionOnUnreadablePage( SubjectRepository $repository ): DeleteSubjectAction {
+		return $this->newAction(
+			$repository,
+			new SpySubjectWriteAuthorizer( allowed: true ),
+			$this->pageIdentifiersLookupWithSubject(),
+			readAuthorizer: new StubPageReadAuthorizer( allowed: false )
+		);
+	}
+
 	private function newAction(
 		SubjectRepository $repository,
 		SubjectWriteAuthorizer $authorizer,
 		PageIdentifiersLookup $pageIdentifiersLookup,
+		?PageReadAuthorizer $readAuthorizer = null,
 	): DeleteSubjectAction {
-		return new DeleteSubjectAction( $repository, $authorizer, $pageIdentifiersLookup );
+		return new DeleteSubjectAction(
+			subjectRepository: $repository,
+			readAuthorizer: $readAuthorizer ?? new StubPageReadAuthorizer( allowed: true ),
+			writeAuthorizer: $authorizer,
+			pageIdentifiersLookup: $pageIdentifiersLookup
+		);
 	}
 
 	private function pageIdentifiersLookupWithSubject(): InMemoryPageIdentifiersLookup {