Address review: distinct missing-IOSettings error, frozen pk_fields default, obj_id regression test

- _require_io_setting now emits a distinct error when IOSettings itself is
  absent vs. when a single field is None
- pk_fields default flipped from list[str]=[] to Sequence[str]=() to remove the
  shared-mutable-default footgun (subclass overrides with concrete lists are
  unchanged)
- DRFDeletion.mutate gains a comment noting the intentional asymmetry: errors
  propagate raw rather than being swallowed by DRFMutation's except Exception
- Consolidated the duplicated to_global_id explanation comment to a single
  block at the graphene_model resolution site
- New test asserts CreateCorpus.objId decodes to "CorpusType" (not the
  metaclass name) — locks in the .__name__ vs .__class__.__name__ fix

Author: JSv4

config/graphql/base.py

@@ -2,6 +2,7 @@
 import logging
 import traceback
 from abc import ABC
+from collections.abc import Sequence
 from typing import Any, ClassVar, Optional
 
 import django.db.models
@@ -25,7 +26,11 @@
 def _require_io_setting(mutation_cls: type, name: str) -> Any:
     """Raise ``NotImplementedError`` if ``cls.IOSettings.<name>`` is missing or ``None``."""
     io_settings = getattr(mutation_cls, "IOSettings", None)
-    value = getattr(io_settings, name, None) if io_settings is not None else None
+    if io_settings is None:
+        raise NotImplementedError(
+            f"{mutation_cls.__name__} must define an IOSettings class."
+        )
+    value = getattr(io_settings, name, None)
     if value is None:
         raise NotImplementedError(
             f"{mutation_cls.__name__}.IOSettings.{name} must be set by the "
@@ -101,6 +106,11 @@ class Arguments:
     @graphql_ratelimit(rate=RateLimits.WRITE_LIGHT)
     def mutate(cls, root, info, *args, **kwargs) -> "DRFDeletion":
 
+        # Unlike ``DRFMutation.mutate`` below, this method intentionally does
+        # NOT wrap the body in ``except Exception``. Errors (including the
+        # ``NotImplementedError`` from ``_require_io_setting`` and the
+        # ``ValueError`` for missing lookup args) propagate to the GraphQL
+        # framework as raw execution errors.
         ok = False
 
         model = _require_io_setting(cls, "model")
@@ -146,7 +156,9 @@ def mutate(cls, root, info, *args, **kwargs) -> "DRFDeletion":
 
 class DRFMutation(graphene.Mutation):
     class IOSettings(ABC):
-        pk_fields: ClassVar[list[str]] = []
+        # Frozen default — subclasses override with their own list/tuple.
+        # Using a tuple here avoids the shared-mutable-default footgun.
+        pk_fields: ClassVar[Sequence[str]] = ()
         lookup_field: ClassVar[str] = "id"
         model: ClassVar[Optional[type[django.db.models.Model]]] = None
         graphene_model: ClassVar[Optional[type[DjangoObjectType]]] = None
@@ -200,6 +212,10 @@ def mutate(cls, root, info, *args, **kwargs) -> "DRFMutation":
             logger.info(f"DRFMutation - kwargs: {kwargs}")
             serializer = _require_io_setting(cls, "serializer")
             model = _require_io_setting(cls, "model")
+            # ``graphene_model`` is the class itself; ``__name__`` is the GraphQL
+            # type name (e.g. ``"CorpusType"``). ``__class__.__name__`` would
+            # return the metaclass name (``"SubclassWithMeta_Meta"``) which is
+            # the bug this PR fixes — kept dereferenced as ``__name__`` below.
             graphene_model = _require_io_setting(cls, "graphene_model")
 
             if hasattr(cls.IOSettings, "pk_fields"):
@@ -262,7 +278,6 @@ def mutate(cls, root, info, *args, **kwargs) -> "DRFMutation":
                 obj_serializer.save()
                 ok = True
                 message = "Success"
-                # graphene_model is the class; .__name__ is the GraphQL type (.__class__.__name__ is the metaclass).
                 obj_id = to_global_id(graphene_model.__name__, obj.id)
                 logger.info("Succeeded updating obj")
 
@@ -283,7 +298,6 @@ def mutate(cls, root, info, *args, **kwargs) -> "DRFMutation":
 
                 ok = True
                 message = "Success"
-                # graphene_model is the class; .__name__ is the GraphQL type (.__class__.__name__ is the metaclass).
                 obj_id = to_global_id(graphene_model.__name__, obj.id)
 
         except serializers.ValidationError as ve:

opencontractserver/tests/test_security_hardening.py

@@ -1224,7 +1224,8 @@ class MisconfiguredMutation:
         with self.assertRaises(NotImplementedError) as ctx:
             _require_io_setting(MisconfiguredMutation, "model")
         self.assertIn("MisconfiguredMutation", str(ctx.exception))
-        self.assertIn("model", str(ctx.exception))
+        # Distinct message for the missing-class case (vs. missing-field).
+        self.assertIn("IOSettings", str(ctx.exception))
 
     def test_require_io_setting_raises_when_attribute_none(self):
         """Each of model/serializer/graphene_model must independently fail when ``None``."""
@@ -1276,10 +1277,49 @@ class IOSettings(DRFDeletion.IOSettings):
         # ``@login_required`` from graphql_jwt looks for a ``ResolveInfo`` arg
         # via ``isinstance``; spec the mock so the decorator passes through
         # to the wrapped function where the real lookup-value check fires.
+        # This relies on ``@graphql_ratelimit`` being a no-op under test
+        # conditions (no real cache backend is consulted before the body).
         info = MagicMock(spec=ResolveInfo)
         info.context = MagicMock()
         info.context.user = MagicMock(is_authenticated=True)
 
         with self.assertRaises(ValueError) as ctx:
             _DeleteCorpus.mutate(None, info)
         self.assertIn("id", str(ctx.exception))
+
+    def test_drf_mutation_obj_id_uses_graphene_type_name_not_metaclass(self):
+        """Regression: ``to_global_id`` must use ``graphene_model.__name__``
+        (the GraphQL type, e.g. ``"CorpusType"``), not
+        ``graphene_model.__class__.__name__`` (the metaclass name like
+        ``"SubclassWithMeta_Meta"``).
+        """
+        from graphene.test import Client
+        from graphql_relay import from_global_id
+
+        from config.graphql.schema import schema
+        from opencontractserver.corpuses.models import Corpus
+
+        user = User.objects.create_user(username="objIdRegressionUser", password="x")
+
+        class _Ctx:
+            def __init__(self, user):
+                self.user = user
+
+        client = Client(schema, context_value=_Ctx(user))
+        result = client.execute("""
+            mutation {
+                createCorpus(title: "ObjIdRegression") {
+                    ok
+                    objId
+                }
+            }
+            """)
+        self.assertIsNone(result.get("errors"))
+        self.assertTrue(result["data"]["createCorpus"]["ok"])
+
+        obj_id = result["data"]["createCorpus"]["objId"]
+        type_name, pk = from_global_id(obj_id)
+        # The fix: ``__name__`` produces the graphene type name.
+        self.assertEqual(type_name, "CorpusType")
+        # Underlying row exists at that pk — proves the global id is decodable.
+        self.assertTrue(Corpus.objects.filter(pk=int(pk)).exists())