Address review: trim verbose comments, add not-found regression test

Author: JSv4

config/graphql/label_mutations.py

@@ -238,29 +238,22 @@ def mutate(
         obj_id = None
 
         try:
-            # Permission check FIRST — runs before any field validation
-            # so a non-owner can't distinguish "reached validation" from
-            # "denied by permission" via different error messages
-            # (defense-in-depth; consistent with the IDOR-safe deny
-            # pattern documented in
-            # ``docs/permissioning/consolidated_permissioning_guide.md``).
+            # Permission check runs before validation so a non-owner cannot
+            # distinguish "reached validation" from "denied" via different
+            # error messages (IDOR mitigation — see
+            # docs/permissioning/consolidated_permissioning_guide.md).
             labelset = LabelSet.objects.get(pk=from_global_id(labelset_id)[1])
             if not user_has_permission_for_obj(
                 info.context.user,
                 labelset,
                 PermissionTypes.UPDATE,
                 include_group_permissions=True,
             ):
-                # Generic deny path — same message and code path as not-found
                 raise LabelSet.DoesNotExist()
 
-            # ``text`` is GraphQL-optional so without this guard the model
-            # default ("Text Label") would silently apply, producing
-            # surprising labels for clients that forgot to pass a value
-            # or passed whitespace.  Django's ``blank=False`` is
-            # form-only and NOT enforced by ``objects.create()``.  The
-            # ``not (text and text.strip())`` form covers ``None``,
-            # empty string, and whitespace-only in one expression.
+            # Reject blank text explicitly: Django's ``blank=False`` is
+            # form-only and ``objects.create()`` would silently apply the
+            # "Text Label" model default.
             if not (text and text.strip()):
                 return CreateLabelForLabelsetMutation(
                     obj=None,
@@ -269,10 +262,6 @@ def mutate(
                     ok=False,
                 )
 
-            # Validate color format (defense in depth).  ``color=""`` is
-            # normalised to ``None`` so the model default ("#FFFFFF")
-            # applies, matching the silent-default behaviour clients
-            # already get from omitting the argument.
             if color == "":
                 color = None
             is_valid_color, color_error = validate_color(color)
@@ -282,10 +271,8 @@ def mutate(
                 )
 
             logger.debug("CreateLabelForLabelsetMutation - mutate / Labelset", labelset)
-            # Drop None and empty-string values so model field defaults
-            # apply (description, color, icon, text are NOT NULL with
-            # sensible defaults).  Empty strings would bypass those
-            # defaults at the DB level and write a row with blank values.
+            # Drop None/"" so model field defaults apply rather than
+            # writing blank values at the DB level.
             create_kwargs = {
                 k: v
                 for k, v in {
@@ -316,8 +303,7 @@ def mutate(
             logger.debug("Done")
 
         except LabelSet.DoesNotExist:
-            # Legitimate auth rejection or genuine 404 — log without a stack
-            # trace to avoid polluting logs with what looks like real errors.
+            # Auth rejection or genuine 404 — warn without stack trace.
             logger.warning(
                 "CreateLabelForLabelsetMutation: labelset not found or "
                 "permission denied (labelset_id=%s)",
@@ -369,15 +355,13 @@ def mutate(
                 PermissionTypes.UPDATE,
                 include_group_permissions=True,
             ):
-                # Generic deny path — same message and code path as not-found
                 raise LabelSet.DoesNotExist()
             labelset.annotation_labels.remove(*label_pks)
             ok = True
             message = "Success"
 
         except LabelSet.DoesNotExist:
-            # Legitimate auth rejection or genuine 404 — log without a stack trace
-            # to avoid polluting logs with what looks like real errors.
+            # Auth rejection or genuine 404 — warn without stack trace.
             logger.warning(
                 "RemoveLabelsFromLabelsetMutation: labelset not found or "
                 "permission denied (labelset_id=%s)",

opencontractserver/tests/test_label_mutations.py

@@ -429,3 +429,24 @@ def test_permission_denied_message_does_not_leak_validation_state(
         # request was structurally valid but rejected by permission.
         self.assertNotIn("blank", data["message"].lower())
         self.assertIn("does not exist", data["message"].lower())
+
+    def test_nonexistent_labelset_returns_same_not_found_message(self) -> None:
+        """A valid relay-encoded ID for a PK that does not exist must
+        produce the same not-found response as a permission denial,
+        pinning the IDOR-safe behaviour against future refactors.
+        """
+        variables = self._create_variables("Should Not Be Created")
+        variables["labelsetId"] = to_global_id("LabelSetType", 999_999)
+
+        result = self.client.execute(
+            CREATE_LABEL_FOR_LABELSET_MUTATION,
+            variables=variables,
+        )
+
+        self.assertIsNone(result.get("errors"))
+        data = result["data"]["createAnnotationLabelForLabelset"]
+        self.assertFalse(data["ok"])
+        self.assertIn("does not exist", data["message"].lower())
+        self.assertFalse(
+            AnnotationLabel.objects.filter(text="Should Not Be Created").exists()
+        )