Skip to content

Commit 40fccaa

Browse files
committed
Address PR #1373 review: fix to_global_id metaclass bug
`graphene_model` in `DRFMutation.IOSettings` is the `DjangoObjectType` class, so `.__class__.__name__` returned graphene's metaclass name (`SubclassWithMeta_Meta`) instead of the GraphQL type name. The resulting `to_global_id(...)` payloads in both create and update return paths used the wrong type name; switched to `graphene_model.__name__` to match the convention used everywhere else in `config/graphql/` (e.g. `to_global_id("CorpusType", ...)`). The bug went unnoticed because the frontend rarely consumes the returned `obj_id` directly — it re-resolves the new object from its Apollo cache by global id. The newly-explicit `ClassVar[Optional[type[...]]]` annotations made the inconsistency visible. Also clarified the CHANGELOG: `DRFDeletion.mutate` has no `try/except`, so its `NotImplementedError` / `ValueError` guards propagate to graphene as GraphQL execution errors rather than the `ok=False` "internal error" path used by `DRFMutation`.
1 parent 802b431 commit 40fccaa

2 files changed

Lines changed: 15 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4646

4747
- **DRFMutation/DRFDeletion `IOSettings` misconfiguration surfaces as a generic internal error** (Issue #1360, `config/graphql/base.py`): `DRFMutation.IOSettings` declared `model: django.db.models.Model = None`, `graphene_model: DjangoObjectType = None` and `serializer = None` — the type annotations were wrong (instance types, not class types, holding `None`), and there was no runtime guard. A subclass that forgot to override one of those would fail deep inside `mutate()` with an `AttributeError` (`'NoneType' object has no attribute 'objects'`) or `TypeError` (`'NoneType' object is not callable`), which the broad `except Exception` then masks as a generic "internal error" message back to the API caller. `DRFDeletion.IOSettings` had the inverse problem — it didn't declare `model` at all, even though `DRFDeletion.mutate` dereferences `cls.IOSettings.model.objects`. Discovered while working on #1332 (typing): mypy flagged four errors on `base.py:128/204/238/252` plus `base.py:93 "type[IOSettings]" has no attribute "model"`.
4848
- Retyped both `IOSettings` declarations with `ClassVar[Optional[type[...]]] = None` so the annotation matches reality (holding a class, not an instance, and nullable by default). `pk_fields` narrowed from `list[str | int]` to `list[str]` — the only call sites use it as a dict key.
49-
- Added `_require_io_setting(mutation_cls, name)` helper that fails fast with `NotImplementedError("<Subclass>.IOSettings.<name> must be set by the subclass.")` when a required attribute is `None` or missing. Called for `model`/`serializer`/`graphene_model` in `DRFMutation.mutate` and for `model` in `DRFDeletion.mutate`. The error is still caught by the broad `except Exception` and surfaced as "internal error" to the API, but the full descriptive message lands in the server log via `logger.error(traceback.format_exc())` — developer gets an obvious misconfiguration signal instead of an opaque `AttributeError`.
50-
- Tightened `DRFDeletion.mutate` to raise `ValueError` when `kwargs[lookup_field]` is missing (the `id` GraphQL argument is `required=False`), instead of passing `None` into `from_global_id` and getting an opaque `AttributeError`.
49+
- Added `_require_io_setting(mutation_cls, name)` helper that fails fast with `NotImplementedError("<Subclass>.IOSettings.<name> must be set by the subclass.")` when a required attribute is `None` or missing. Called for `model`/`serializer`/`graphene_model` in `DRFMutation.mutate` and for `model` in `DRFDeletion.mutate`. In `DRFMutation` the error is caught by the broad `except Exception` and surfaced as "internal error" to the API, with the full descriptive message in the server log via `logger.error(traceback.format_exc())`. `DRFDeletion.mutate` has no equivalent `try/except` (matching its pre-PR behavior), so the `NotImplementedError` propagates to graphene as a GraphQL execution error — also a fail-fast outcome, just on a different surface.
50+
- Tightened `DRFDeletion.mutate` to raise `ValueError` when `kwargs[lookup_field]` is missing (the `id` GraphQL argument is `required=False`), instead of passing `None` into `from_global_id` and getting an opaque `AttributeError`. Same propagation surface as the `_require_io_setting` guard above.
51+
- Fixed pre-existing `to_global_id(graphene_model.__class__.__name__, obj.id)` in both `DRFMutation` create/update return paths: `graphene_model` is the `DjangoObjectType` class, so `.__class__` referenced graphene's metaclass (`SubclassWithMeta_Meta`) and the resulting global id used the wrong type name. Switched to `graphene_model.__name__` to match the GraphQL type name (e.g. `"CorpusType"`) used everywhere else in `config/graphql/`. Surfaced by the new explicit `ClassVar[Optional[type[...]]]` annotations — both call sites stayed dormant because the returned `obj_id` is rarely consumed by the frontend (which re-queries by global id from its own cache).
5152
- Graduated `config.graphql.base` out of the mypy baseline: removed the `[mypy-config.graphql.base]` section from `mypy.ini` and pruned the ten corresponding lines from `docs/typing/mypy_baseline.txt`. `mypy --config-file mypy.ini` is now clean on this module.
5253
- Regression tests in `opencontractserver/tests/test_security_hardening.py::TestIOSettingsRequiredFieldsGuard`: the helper raises `NotImplementedError` for each of `model`/`serializer`/`graphene_model` when missing or `None`, returns the configured value when present, and the base classes expose the `None` defaults the guard relies on.
5354

config/graphql/base.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,12 @@ def mutate(cls, root, info, *args, **kwargs):
260260
obj_serializer.save()
261261
ok = True
262262
message = "Success"
263-
obj_id = to_global_id(graphene_model.__class__.__name__, obj.id)
263+
# `graphene_model` is the DjangoObjectType class itself, so
264+
# `.__name__` yields the GraphQL type name (e.g. "CorpusType").
265+
# The previous `.__class__.__name__` returned the metaclass
266+
# name (graphene's `SubclassWithMeta_Meta`) which produced an
267+
# incorrect global id.
268+
obj_id = to_global_id(graphene_model.__name__, obj.id)
264269
logger.info("Succeeded updating obj")
265270

266271
else:
@@ -280,7 +285,12 @@ def mutate(cls, root, info, *args, **kwargs):
280285

281286
ok = True
282287
message = "Success"
283-
obj_id = to_global_id(graphene_model.__class__.__name__, obj.id)
288+
# `graphene_model` is the DjangoObjectType class itself, so
289+
# `.__name__` yields the GraphQL type name (e.g. "CorpusType").
290+
# The previous `.__class__.__name__` returned the metaclass
291+
# name (graphene's `SubclassWithMeta_Meta`) which produced an
292+
# incorrect global id.
293+
obj_id = to_global_id(graphene_model.__name__, obj.id)
284294

285295
except serializers.ValidationError as ve:
286296
logger.warning(f"Validation error in mutation: {ve.detail}")

0 commit comments

Comments
 (0)