Ask one place whether a label name is taken, and ask it on a rename too

d3292e3a made the check ignore capitals and left it in two shapes: a
repository lookup, plus an "is that just me" comparison written out in
Mailbox/set and missing from the browser. The browser's rename form
asked nothing at all — not before that commit and not after — so a
label could be renamed onto a sibling's exact name there, which is the
same collision by a shorter road.

LabelRepository::findNameConflict() is the whole rule now: sibling,
capitals ignored, the label being renamed left out, an exact match
reported before a close one. Mailbox/set and both of the browser's
actions call it and differ only in how they word the refusal — a JMAP
description naming the label in the way, a translated form error.

The label being renamed is excluded in the query rather than compared
afterwards. The comparison had a hole: where the label itself matched
first it hid a second one that was in the way, so with "receipts" and
"Receipts" both present, renaming the first to "RECEIPTS" passed.

A refused rename in the browser puts the stored values back on the
entity. The form binds onto the managed object before anything is
checked, and "nothing flushes it" is true of this request today rather
than of the object.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -12,7 +12,8 @@ The published image tags: `latest` follows the most recent release below,
 created beside "Work". Gmail treats the two as one label and rejected the second, but only later, in
 the background: the new label existed in plMail, could be put on mail, and never reached Gmail, with
 an error in the log and nothing on screen. plMail now refuses it when you create or rename the
-label, in the browser and from the apps, and says which label is in the way.
+label, in the browser and from the apps, and says which label is in the way. Renaming a label in the
+browser had no check at all and could take another label's exact name; it has the same one now.
 
 - **If you already have such a pair**, nothing is changed for you. Delete or rename the newer one;
   until you do, putting it on Gmail mail keeps failing in the background.

src/Controller/Mail/LabelController.php

@@ -65,23 +65,9 @@ final class LabelController extends AbstractController
         if (true === $form->isSubmitted() && true === $form->isValid()) {
             $label->usr = $this->getUser();
 
-            // Capitals ignored — see the repository method for whose rule
-            // that is. The same check Mailbox/set makes.
-            $duplicate = $this->labelRepository->findOneChildByNameIgnoringCase(
-                $this->getUser(),
-                $label->parent,
-                (string) $label->name,
-            );
-
-            if (null !== $duplicate) {
-                // Translated here, not in the theme: form_errors renders
-                // error.message verbatim, and validator-produced errors arrive
-                // already translated — re-translating them in the shared theme
-                // would be the riskier fix.
-                $form->get('name')->addError(
-                    new FormError($this->translator->trans('label.error.duplicate'))
-                );
-            } else {
+            // Refused, and the error is on the field: fall through to the
+            // re-render below, which answers 422.
+            if (false === $this->refuseTakenName($form, $label)) {
                 $this->em->persist($label);
                 $this->em->flush();
 
@@ -122,6 +108,17 @@ final class LabelController extends AbstractController
         $form->handleRequest($request);
 
         if (true === $form->isSubmitted() && true === $form->isValid()) {
+            if (true === $this->refuseTakenName($form, $label, $label)) {
+                // The form has already written the refused name onto the
+                // managed entity. Nothing here flushes it, but "nothing
+                // flushes" is a property of this request today and not of the
+                // object: put the stored values back before anything can.
+                $response = $this->renderForm($form, $label);
+                $this->em->refresh($label);
+
+                return $response;
+            }
+
             $this->em->flush();
 
             $this->structurePropagator->renamed($label, $previousFullName);
@@ -277,6 +274,44 @@ final class LabelController extends AbstractController
      * modal_controller closes the dialog on any successful turbo:submit-end,
      * so a 200 here would swallow the errors and look like a silent save.
      */
+    /**
+     * Put an error on the form when the name is one the user already has, and
+     * say whether it did.
+     *
+     * The rule is LabelRepository::findNameConflict() — the same call
+     * Mailbox/set makes, so a name the apps are refused is refused here and
+     * the other way round. This only decides where the sentence goes. It used
+     * to be an exact-match lookup written out in new() alone: a rename was
+     * never checked at all, and "work" beside "Work" passed both.
+     *
+     * $itself is the label being edited, which is not in its own way.
+     *
+     * Translated here, not in the theme: form_errors renders error.message
+     * verbatim, and validator-produced errors arrive already translated —
+     * re-translating them in the shared theme would be the riskier fix.
+     */
+    private function refuseTakenName(FormInterface $form, Label $label, ?Label $itself = null): bool
+    {
+        $taken = $this->labelRepository->findNameConflict(
+            $this->getUser(),
+            $label->parent,
+            (string) $label->name,
+            $itself,
+        );
+
+        if (null === $taken) {
+            return false;
+        }
+
+        // A system label's form has no name field; the error then belongs to
+        // the form as a whole.
+        $field = true === $form->has('name') ? $form->get('name') : $form;
+
+        $field->addError(new FormError($this->translator->trans('label.error.duplicate')));
+
+        return true;
+    }
+
     private function renderForm(FormInterface $form, Label $label): Response
     {
         if (true === $form->isSubmitted() && false === $form->isValid()) {

src/Jmap/Method/Mail/MailboxSetMethod.php

@@ -470,8 +470,9 @@ final class MailboxSetMethod implements JmapMethod
     /**
      * Siblings must have distinct names — the same rule LabelResolver relies on
      * when it does find-or-create by (parent, name) — and names that differ
-     * only in capitals are not distinct. See
-     * LabelRepository::findOneChildByNameIgnoringCase() for whose rule that is.
+     * only in capitals are not distinct. The rule itself is
+     * LabelRepository::findNameConflict(), which the browser's form asks too;
+     * this only words the refusal.
      *
      * The description is written for a person, because a client shows it to
      * one: there is no error type for "taken", `invalidProperties` is what the
@@ -482,16 +483,12 @@ final class MailboxSetMethod implements JmapMethod
      */
     private function assertNameFree(Account $account, ?Label $parent, string $name, ?Label $ignore = null): void
     {
-        $existing = $this->labelRepository->findOneChildByNameIgnoringCase($account->usr, $parent, $name);
+        $existing = $this->labelRepository->findNameConflict($account->usr, $parent, $name, $ignore);
 
         if (null === $existing) {
             return;
         }
 
-        if (null !== $ignore && $existing->id === $ignore->id) {
-            return;
-        }
-
         if ($existing->name === $name) {
             throw new MethodException(
                 'invalidProperties',

src/Repository/Label/LabelRepository.php

@@ -86,16 +86,20 @@ class LabelRepository extends ServiceEntityRepository
     }
 
     /**
-     * The sibling a NEW name would collide with, capitals ignored.
+     * The sibling a name would collide with — capitals ignored — or null when
+     * the name is free. THE rule for a name somebody is typing, and the only
+     * copy of it: Mailbox/set and the browser's label form both ask here, for
+     * a new label and for a rename, and differ only in how they word the
+     * refusal.
      *
      * Not the same question findOneChildByName() answers, and the two must not
      * be merged. That one is find-or-create for mail arriving from a provider,
      * where an exact match is the only safe one: a case-sensitive IMAP server
      * may really hold `work` and `Work` as two folders, and folding them would
      * file two folders' mail under one label. This one is the check in front
-     * of a person typing a name, and there the rule is the strictest
-     * provider's — Gmail treats `work` and `Work` as one label and answers a
-     * second with "409: Label name exists or conflicts".
+     * of a person, and there the rule is the strictest provider's — Gmail
+     * treats `work` and `Work` as one label and answers a second with "409:
+     * Label name exists or conflicts".
      *
      * That refusal arrives in ApplyGmailLabelsHandler, in a worker, long after
      * the request that made the label has answered "created" — so the label
@@ -103,20 +107,30 @@ class LabelRepository extends ServiceEntityRepository
      * nothing on any screen to say so. Refusing at the door is the only place
      * the person who typed it can be told.
      *
+     * $itself is the label being renamed, which is never in its own way: it
+     * may keep its name, and it may change nothing but its capitals. Left out
+     * in the query rather than compared afterwards, so a second label that IS
+     * in the way is still found when the first match was the label itself.
+     *
      * Roles are included on purpose: a custom "inbox" beside the Inbox is the
      * same collision with a name the provider reserves.
      *
      * LOWER() on both sides rather than ILIKE, so `%` and `_` in a name are
-     * characters and not wildcards.
+     * characters and not wildcards. An exact match sorts first, so a caller
+     * that says which label is in the way names the one that matches character
+     * for character where there is one.
      */
-    public function findOneChildByNameIgnoringCase(UserInterface $user, ?Label $parent, string $name): ?Label
+    public function findNameConflict(UserInterface $user, ?Label $parent, string $name, ?Label $itself = null): ?Label
     {
         $builder = $this->createQueryBuilder('label')
+            ->addSelect('CASE WHEN label.name = :exact THEN 0 ELSE 1 END AS HIDDEN distance')
             ->where('label.usr = :usr')
             ->andWhere('LOWER(label.name) = :name')
             ->setParameter('usr', $user)
             ->setParameter('name', mb_strtolower($name))
-            ->orderBy('label.id', 'ASC')
+            ->setParameter('exact', $name)
+            ->orderBy('distance', 'ASC')
+            ->addOrderBy('label.id', 'ASC')
             ->setMaxResults(1);
 
         if (null === $parent) {
@@ -125,11 +139,12 @@ class LabelRepository extends ServiceEntityRepository
             $builder->andWhere('label.parent = :parent')->setParameter('parent', $parent);
         }
 
-        // The exact spelling first, so a caller that reports which label is in
-        // the way names the one that matches character for character where
-        // there is one.
-        return $this->findOneChildByName($user, $parent, $name)
-            ?? $builder->getQuery()->getOneOrNullResult();
+        // A label not yet persisted has no id and cannot be in the table.
+        if (null !== $itself?->id) {
+            $builder->andWhere('label.id != :itself')->setParameter('itself', $itself->id);
+        }
+
+        return $builder->getQuery()->getOneOrNullResult();
     }
 
     /**

tests/Controller/Mail/LabelNameTakenTest.php

@@ -0,0 +1,132 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controller\Mail;
+
+use App\Entity\Label\Label;
+use App\Tests\Support\Mail\SeedsMarkerFixtures;
+use App\Tests\Support\Mercure\RecordingHub;
+use Doctrine\DBAL\Connection;
+use Doctrine\ORM\EntityManagerInterface;
+use Symfony\Bundle\FrameworkBundle\KernelBrowser;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\Mercure\HubInterface;
+
+/**
+ * The browser's label form refuses a name the user already has — for a new
+ * label and for a rename, capitals ignored.
+ *
+ * The rule itself is LabelRepository::findNameConflict() and LabelRepositoryTest
+ * pins it. What is pinned here is that both of the browser's ways of naming a
+ * label reach it, because for a long time only one of them checked anything:
+ * new() did an exact-match lookup written out inline, and edit() did none, so a
+ * rename could land on a sibling's name outright. "work" beside "Work" passed
+ * both, and Gmail refused it later in a worker with nobody watching.
+ *
+ * Mailbox/set asks the same method (MailboxSetStructureTest), which is the
+ * point: a name the apps are refused is refused here, and the other way round.
+ */
+final class LabelNameTakenTest extends WebTestCase
+{
+    use SeedsMarkerFixtures;
+
+    private KernelBrowser $client;
+    private Connection $connection;
+
+    protected function setUp(): void
+    {
+        $this->client = static::createClient();
+
+        // Fixtures live in a transaction this test rolls back, and a rebooted
+        // kernel takes the connection holding it with them.
+        $this->client->disableReboot();
+
+        $container = static::getContainer();
+
+        // Before anything else is pulled out of the container, for the reason
+        // LabelChangeReachesEveryTabTest gives: a successful save publishes,
+        // and the hub cannot be replaced once something has reached for it.
+        $container->set(HubInterface::class, new RecordingHub());
+
+        $this->em         = $container->get(EntityManagerInterface::class);
+        $this->connection = $container->get(Connection::class);
+
+        $this->connection->beginTransaction();
+
+        $this->user    = $this->seedUser();
+        $this->account = $this->seedAccount();
+
+        $this->client->loginUser($this->user);
+    }
+
+    protected function tearDown(): void
+    {
+        if (true === $this->connection->isTransactionActive()) {
+            $this->connection->rollBack();
+        }
+
+        parent::tearDown();
+    }
+
+    public function testANewLabelCannotTakeANameThatDiffersOnlyInCapitals(): void
+    {
+        $this->seedLabel('Work');
+
+        $this->submit('/labels/new', 'work');
+
+        self::assertResponseStatusCodeSame(422);
+        self::assertSame(1, $this->labelsNamedLike('work'), 'the second label must not have been created');
+    }
+
+    /** The half that had no check at all: a rename onto a sibling's name. */
+    public function testARenameCannotLandOnAnotherLabelsName(): void
+    {
+        $this->seedLabel('Work');
+        $receipts = $this->seedLabel('Receipts');
+
+        $this->submit('/labels/' . $receipts->id . '/edit', 'WORK');
+
+        self::assertResponseStatusCodeSame(422);
+        self::assertSame('Receipts', $this->storedName($receipts), 'a refused rename must not be stored');
+    }
+
+    /** A label is not in its own way: changing only its capitals is a rename like any other. */
+    public function testALabelMayChangeOnlyItsOwnCapitals(): void
+    {
+        $receipts = $this->seedLabel('Receipts');
+
+        $this->submit('/labels/' . $receipts->id . '/edit', 'receipts');
+
+        self::assertResponseIsSuccessful();
+        self::assertSame('receipts', $this->storedName($receipts));
+    }
+
+    /**
+     * Fetched and submitted rather than hand-built, so the CSRF token and the
+     * parent and colour fields are exactly what a person's click sends.
+     */
+    private function submit(string $path, string $name): void
+    {
+        $crawler = $this->client->request('GET', $path);
+
+        $form                = $crawler->filter('form')->form();
+        $form['label[name]'] = $name;
+
+        $this->client->submit($form);
+    }
+
+    /** From the table, not the entity: the form writes onto the managed object before anything is refused. */
+    private function storedName(Label $label): string
+    {
+        return (string) $this->connection->fetchOne('SELECT name FROM label WHERE id = ?', [$label->id]);
+    }
+
+    private function labelsNamedLike(string $name): int
+    {
+        return (int) $this->connection->fetchOne(
+            'SELECT COUNT(*) FROM label WHERE usr_id = ? AND LOWER(name) = ?',
+            [$this->user->id, mb_strtolower($name)],
+        );
+    }
+}

tests/Repository/Label/LabelRepositoryTest.php

@@ -121,8 +121,8 @@ final class LabelRepositoryTest extends KernelTestCase
         $work = $this->label('Work');
 
         self::assertNull($this->repository->findOneChildByName($this->user, null, 'work'));
-        self::assertSame($work, $this->repository->findOneChildByNameIgnoringCase($this->user, null, 'work'));
-        self::assertSame($work, $this->repository->findOneChildByNameIgnoringCase($this->user, null, 'WORK'));
+        self::assertSame($work, $this->repository->findNameConflict($this->user, null, 'work'));
+        self::assertSame($work, $this->repository->findNameConflict($this->user, null, 'WORK'));
     }
 
     /** Umlauts fold too: `ärzte` and `Ärzte` are one name to a person and to Gmail. */
@@ -130,7 +130,7 @@ final class LabelRepositoryTest extends KernelTestCase
     {
         $doctors = $this->label('Ärzte');
 
-        self::assertSame($doctors, $this->repository->findOneChildByNameIgnoringCase($this->user, null, 'ärzte'));
+        self::assertSame($doctors, $this->repository->findNameConflict($this->user, null, 'ärzte'));
     }
 
     public function testIgnoringCaseStillKeepsToTheParent(): void
@@ -138,8 +138,36 @@ final class LabelRepositoryTest extends KernelTestCase
         $work = $this->label('Work');
         $this->label('Invoices', parent: $work);
 
-        self::assertNull($this->repository->findOneChildByNameIgnoringCase($this->user, null, 'invoices'));
-        self::assertNotNull($this->repository->findOneChildByNameIgnoringCase($this->user, $work, 'invoices'));
+        self::assertNull($this->repository->findNameConflict($this->user, null, 'invoices'));
+        self::assertNotNull($this->repository->findNameConflict($this->user, $work, 'invoices'));
+    }
+
+    /**
+     * A label is never in its own way: it may keep its name and may change
+     * nothing but its capitals. Left out in the query rather than compared
+     * afterwards — the second assertion is the case a comparison gets wrong,
+     * where the label itself matches first and hides the one that IS in the
+     * way.
+     */
+    public function testALabelBeingRenamedIsNotItsOwnConflict(): void
+    {
+        $work = $this->label('Work');
+
+        self::assertNull($this->repository->findNameConflict($this->user, null, 'work', $work));
+
+        $older = $this->label('receipts');
+        $newer = $this->label('Receipts');
+
+        self::assertSame($newer, $this->repository->findNameConflict($this->user, null, 'RECEIPTS', $older));
+    }
+
+    /** Where one sibling matches exactly, that is the one named. */
+    public function testAnExactMatchIsReportedBeforeACloseOne(): void
+    {
+        $this->label('work');
+        $exact = $this->label('Work');
+
+        self::assertSame($exact, $this->repository->findNameConflict($this->user, null, 'Work'));
     }
 
     /** `%` and `_` are characters in a name, not wildcards. */
@@ -147,8 +175,8 @@ final class LabelRepositoryTest extends KernelTestCase
     {
         $this->label('Work');
 
-        self::assertNull($this->repository->findOneChildByNameIgnoringCase($this->user, null, 'w_rk'));
-        self::assertNull($this->repository->findOneChildByNameIgnoringCase($this->user, null, 'w%'));
+        self::assertNull($this->repository->findNameConflict($this->user, null, 'w_rk'));
+        self::assertNull($this->repository->findNameConflict($this->user, null, 'w%'));
     }
 
     // ── the parent picker ────────────────────────────────────────────────────