Add type annotations to GraphQL resolvers, mutations, and filters#1369
Add type annotations to GraphQL resolvers, mutations, and filters#1369
Conversation
Issue #1332. Raise return-annotation coverage in config/graphql/ from ~4.8% to 91.5% (421/460 function defs) and remove 22 modules from the mypy.ini baseline allow-list. Root-cause fixes in opencontractserver/utils/permissioning.py: set_permissions_for_obj_to_user, user_has_permission_for_obj, and friends were annotated as type[Model] (class) despite every caller passing an instance, and user: type[User] instead of a User instance. Correcting to `instance: django.db.models.Model` / `user: UserModel` (forward-referenced via TYPE_CHECKING) clears the set_permissions cluster across every dependent mutation file. Also dropped the `user_instance=User` (class) default on get_users_group_ids, which would crash if ever called with no arg. Graduated from mypy.ini baseline (22 modules): action_queries, agent_mutations, badge_mutations, base_types, conversation_mutations, conversation_types, corpus_types, document_queries, filters, ingestion_source_mutations, moderation_mutations, og_metadata_queries, pipeline_queries, security, serializers, slug_queries, smart_label_mutations, social_types, user_queries, user_types, voting_mutations, plus opencontractserver.utils.permissioning. Per-file type fixes (no behaviour change): - slug_queries / user_types / social_types / corpus_types: reversed .filter(...).visible_to_user(user) -> .visible_to_user(user).filter(...) so the custom manager method resolves before .filter() flattens to the base QuerySet[Model] that django-stubs doesn't know carries visible_to_user. - og_metadata_queries: guard Extract.corpus (nullable FK) before use. - pipeline_queries: narrow mimetype optional; type components_data as dict[str, Sequence[PipelineComponentDefinition]] to unify the list and tuple branches. - ingestion_source_mutations: `if pk is None` instead of `if error` so mypy can narrow pk: str | None through the guard. - conversation_types: base64.binascii.Error -> binascii.Error with an explicit `import binascii`. - filters: int() coerce of from_global_id pk before folder_id lookup. - security: typed _csrf_noop_get_response -> HttpResponse; setattr() for Django-private _dont_enforce_csrf_checks / csrf_exempt flags. - moderation_mutations: explicit Union[ChatMessage, Conversation, None] on cross-branch target variable. - action_queries: int() coerce of from_global_id pk before for_corpus / for_document. mypy.ini baseline section for config.graphql trimmed 35 -> 14; 63 matching error lines pruned from docs/typing/mypy_baseline.txt. Two real bugs mypy surfaced are tracked as separate issues: - #1359: RemoveLabelsFromLabelsetMutation calls non-existent labelset.documents (silent runtime failure). - #1360: DRFMutation.IOSettings declares model/serializer as None. Closes #1332.
Code Review — PR #1369: Type annotations for GraphQL resolvers, mutations, and filtersSummaryThis PR is a well-executed, large-scale type-annotation migration. It raises return-annotation coverage in Real Bug Fixes Bundled In (Praise)These are actual correctness fixes exposed by the type work:
Concerns1.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| @login_required | ||
| def mutate(root, info, id): | ||
| def mutate(root, info, id) -> "DeleteAnalysisMutation": |
|
Type-annotation uplift: 22 modules graduated from mypy baseline. Review posted inline below. |
|
Code Review: PR 1369 - Type annotations for GraphQL resolvers, mutations, and filters OVERVIEW: Type-annotation uplift across ~46 files in config/graphql/ and opencontractserver/utils/, graduating 22 modules out of the mypy ignore_errors baseline. Most additions are mechanical -> Any return-type annotations, but several hunks contain genuine correctness fixes. GENUINE BUG FIXES BUNDLED IN: opencontractserver/utils/permissioning.py: The type[User] / type[Model] annotation inversions on the four public helpers were real annotation bugs (class where instance was expected). Because mypy was suppressed for every caller, they caused false-positive [arg-type] errors across ~10 mutation files. Fixing the root cause clears them all cleanly. Also added source_analysis is None guard before calling user_has_permission_for_obj (potential NPE), and removed the dangerous user_instance=User default argument (the class itself, not an instance). config/graphql/pipeline_queries.py: Added if mime_type_str is None guard before calling get_components_by_mimetype_cached. FILE_TYPE_TO_MIME.get() can return None; the downstream function requires str. config/graphql/og_metadata_queries.py: Guarded against Extract.corpus being None (on_delete=SET_NULL FK). Previous code would raise AttributeError on a corpus-less extract. config/graphql/conversation_types.py: base64.binascii.Error replaced with binascii.Error + explicit import binascii (base64 does not publicly re-export binascii). Also reordered ChatMessage.objects.visible_to_user(user).filter(...) — manager method first, consistent with CLAUDE.md pattern. config/graphql/action_queries.py: Coerced from_global_id(...)[1] (str) to int before passing to for_corpus / for_document queryset methods that declare int PKs. config/graphql/slug_queries.py: Reordered .filter(...).visible_to_user() to .visible_to_user().filter(...) at three call sites — consistent with the project manager-first pattern. ANNOTATION QUALITY: The -> Any strategy is acceptable as a transition step. Minor items for a follow-up: (1) filters.py uses info: str as the parameter name for the django-filters field-name argument — should be name: str to match django-filters convention and avoid confusion with GraphQL resolver info objects; (2) resolve_me is typed -> User but info.context.user can be AnonymousUser for unauthenticated requests — should be Union[User, AnonymousUser] or AbstractBaseUser; (3) -> type on lazy import helpers in base_types.py could be -> type[UserType]; (4) unparameterised QuerySet in filter return types loses element type info. security.py POSITIVE CHANGE: Replacing lambda req: None with a named _csrf_noop_get_response that raises NotImplementedError is strictly better. Using setattr for csrf_exempt and _dont_enforce_csrf_checks is the correct approach for Django-private flags absent from the stubs. SUMMARY: Approve with minor suggestions. The bundled correctness fixes stand on their own. Two items to address before merge: (1) rename info to name in django-filters method signatures in filters.py, (2) widen resolve_me return type to include AnonymousUser. Everything else can be deferred to the next annotation pass. |
…e type - Apply pyupgrade/black/isort fixes (Optional → | None) and drop the resulting unused 'Optional'/'Union' imports. - Rename 'info: str' → 'name: str' on django-filters method signatures in filters.py. The parameter receives the filter field name; the 'info' name was misleading copy-paste from GraphQL resolver signatures (where 'info' is graphene.ResolveInfo). - Widen UserQueryMixin.resolve_me return type from User to User | AnonymousUser. The query has no @login_required, so info.context.user can be AnonymousUser at runtime.
Code Review: PR #1369 — Type Annotations for GraphQL LayerThis PR is a well-executed type annotation pass that graduates 22 modules from the mypy baseline, raises annotation coverage from ~4.8% to 91.5%, and surfaces several real bugs in the process. The changelog is thorough, the scope is disciplined, and the known-remaining-issues are correctly filed as separate tickets. Real Bug Fixes Bundled In1. The return statement was commented out and never replaced. The async deletion task fired correctly, but the mutation silently returned def test_delete_analysis_mutation_returns_ok(self):
result = schema.execute(DELETE_ANALYSIS_MUTATION, ...)
self.assertTrue(result.data["deleteAnalysis"]["ok"])2.
3.
4.
5.
6.
Code Quality Notes
The PR honestly acknowledges this is a coverage-first pass. Several resolver patterns have known specific return types worth capturing in a follow-up:
Should be
Making the unreachability explicit is better than silently returning Filter method parameter rename ( django-filters passes the field name as the second argument in custom filter methods, not
Correctness of
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Address PR #1369 review (Claude bot): the missing-return fix in config/graphql/analysis_mutations.py — restoring `return DeleteAnalysisMutation(ok=True, message="SUCCESS")` — was the most impactful behavioral fix bundled into the type-annotation pass, but no regression test pinned the success shape. The new test grants DELETE permission on an Analysis to its creator, runs the GraphQL mutation, and asserts the response contains `ok=True`/`message="SUCCESS"` rather than the `null` payload that was previously returned. Without the restored `return`, the assertion on `data.deleteAnalysis` being non-null fails immediately.
Both PR #1369 (this branch, issue #1332) and #1370 (merged via main, issue #1333) graduated different sets of modules out of the mypy baseline. Conflict resolution drops: - admin_auth + graphql_api_token_auth.backends (graduated by #1333) - graphql.{action_queries, agent_mutations, social_types, user_queries, user_types, voting_mutations} (graduated by #1332) from both mypy.ini and docs/typing/mypy_baseline.txt. CHANGELOG.md keeps both Added entries side by side.
Code Review — PR #1369: Type annotations for GraphQL resolvers, mutations, and filtersThis is a well-scoped, high-value PR. Beyond the annotation work, it catches several real bugs that were masked by missing types. Notes below, roughly ordered by importance. Real bug fixes bundled in (good catches)
Correctness issues / things to double-check
Style/convention issuesTest docstring references the PR number ( """Regression coverage for the DeleteAnalysisMutation return value.
PR #1369 restored the missing ``return DeleteAnalysisMutation(...)`` line in...CLAUDE.md explicitly says: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." Suggest trimming the docstring to just describe the invariant being tested, not the PR that fixed it: """Regression test: DeleteAnalysisMutation.mutate must return a typed result.
The mutation triggers an async deletion task and must return ok=True/message
so the frontend can observe the success shape rather than receiving null.
"""Broad Filter ordering flip (
|
| Category | Assessment |
|---|---|
| Real bug fixes | ✅ 3 catches (missing return, binascii, permissioning annotations) |
| Type coverage | ✅ Substantial improvement, 22 modules graduated |
| Test coverage | ✅ New regression test for the DeleteAnalysisMutation bug |
| Convention compliance | |
| Open question | ❓ Confirm _NOT_FOUND_MSG is defined in ingestion_source_mutations.py |
| Known remaining gaps | ✅ Filed as separate issues (#1359, #1360) per scope rules — good discipline |
The PR is in good shape. Resolve the _NOT_FOUND_MSG question and trim the test docstring PR reference, then this is ready to merge.
…1332-1lh9h # Conflicts: # CHANGELOG.md
CLAUDE.mdThis file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. Project OverviewOpenContracts is an AGPL-3.0 enterprise document analytics platform for PDFs and text-based formats. It features a Django/GraphQL backend with PostgreSQL + pgvector, a React/TypeScript frontend with Jotai state management, and pluggable document processing pipelines powered by machine learning models. Baseline Commit Rules
Essential CommandsBackend (Django)# Run backend tests (sequential, use --keepdb to speed up subsequent runs)
docker compose -f test.yml run django python manage.py test --keepdb
# Run backend tests in PARALLEL (recommended - ~4x faster)
# Uses pytest-xdist with 4 workers, --dist loadscope keeps class tests together
docker compose -f test.yml run django pytest -n 4 --dist loadscope
# Run parallel tests with auto-detected worker count (uses all CPU cores)
docker compose -f test.yml run django pytest -n auto --dist loadscope
# Run parallel tests with fresh databases (first run or after schema changes)
docker compose -f test.yml run django pytest -n 4 --dist loadscope --create-db
# Run specific test file
docker compose -f test.yml run django python manage.py test opencontractserver.tests.test_notifications --keepdb
# Run specific test file in parallel
docker compose -f test.yml run django pytest opencontractserver/tests/test_notifications.py -n 4 --dist loadscope
# Run specific test class/method
docker compose -f test.yml run django python manage.py test opencontractserver.tests.test_notifications.TestNotificationModel.test_create_notification --keepdb
# Apply database migrations
docker compose -f local.yml run django python manage.py migrate
# Create new migration
docker compose -f local.yml run django python manage.py makemigrations
# Django shell
docker compose -f local.yml run django python manage.py shell
# Code quality (runs automatically via pre-commit hooks)
pre-commit run --all-filesFrontend (React/TypeScript)cd frontend
# Start development server (proxies to Django on :8000)
yarn start
# Run unit tests (Vitest) - watches by default
yarn test:unit
# Run component tests (Playwright) - CRITICAL: Use --reporter=list to prevent hanging
yarn test:ct --reporter=list
# Run component tests with grep filter
yarn test:ct --reporter=list -g "test name pattern"
# Run E2E tests
yarn test:e2e
# Coverage reports (unit tests via Vitest, component tests via Playwright + Istanbul)
yarn test:coverage:unit
yarn test:coverage:ct
# Linting and formatting
yarn lint
yarn fix-styles
# Build for production
yarn build
# Preview production build locally
yarn serveProduction Deployment# CRITICAL: Always run migrations FIRST in production
docker compose -f production.yml --profile migrate up migrate
# Then start main services
docker compose -f production.yml upHigh-Level ArchitectureBackend ArchitectureStack: Django 4.x + GraphQL (Graphene) + PostgreSQL + pgvector + Celery Key Patterns:
Frontend ArchitectureStack: React 18 + TypeScript + Apollo Client + Jotai (atoms) + PDF.js + Vite Key Patterns:
Data Flow ArchitectureDocument Processing:
GraphQL Permission Flow:
Critical Security Patterns
Critical Concepts
Testing PatternsManual Test ScriptsLocation: When performing manual testing (e.g., testing migrations, verifying database state, testing API endpoints interactively), always document the test steps in a markdown file under Format: # Test: [Brief description]
## Purpose
What this test verifies.
## Prerequisites
- Required state (e.g., "migration at 0058")
- Required data (e.g., "at least one document exists")
## Steps
1. Step one with exact command
```bash
docker compose -f local.yml run --rm django python manage.py shell -c "..."
Expected Results
CleanupCommands to restore original state if needed. Automated Documentation ScreenshotsLocation: Screenshots for documentation are automatically captured during Playwright component tests and committed back to the PR branch by the How it works:
Naming convention (
At least 2 segments required, 3 recommended. All lowercase alphanumeric with single hyphens. Example: import { docScreenshot } from "./utils/docScreenshot";
// After the component renders and assertions pass:
await docScreenshot(page, "badges--celebration-modal--auto-award");Rules:
Release Screenshots (Point-in-Time)For release notes, use Location: import { releaseScreenshot } from "./utils/docScreenshot";
await releaseScreenshot(page, "v3.0.0.b3", "landing-page", { fullPage: true });Key differences from
When to use which:
Authenticated Playwright Testing (Live Frontend Debugging)When you need to interact with the running frontend as an authenticated user (e.g., debugging why a query returns empty results), use Django admin session cookies to authenticate GraphQL requests. Architecture context: The frontend uses Auth0 for authentication, but the Django backend also accepts session cookie auth. Apollo Client sends GraphQL requests directly to Step 1: Set a password for the superuser (one-time setup): docker compose -f local.yml exec django python manage.py shell -c "
from django.contrib.auth import get_user_model
User = get_user_model()
u = User.objects.filter(is_superuser=True).first()
u.set_password('testpass123')
u.save()
print(f'Password set for {u.username}')
"Step 2: Playwright script pattern: const { chromium } = require('playwright');
(async () => {
const browser = await chromium.launch({ headless: true });
const context = await browser.newContext();
const page = await context.newPage();
// Collect console messages for debugging
const consoleMsgs = [];
page.on('console', msg => consoleMsgs.push('[' + msg.type() + '] ' + msg.text()));
// 1. Login to Django admin to get session cookie
await page.goto('http://localhost:8000/admin/login/');
await page.fill('#id_username', '<superuser-username>');
await page.fill('#id_password', 'testpass123');
await page.click('input[type=submit]');
await page.waitForTimeout(2000);
// 2. Extract the session cookie
const cookies = await context.cookies();
const sessionCookie = cookies.find(c => c.name === 'sessionid');
// 3. Intercept GraphQL requests to inject the session cookie
// (needed because Apollo sends cross-origin requests to :8000)
await page.route('**/graphql/**', async (route) => {
const headers = {
...route.request().headers(),
'Cookie': 'sessionid=' + sessionCookie.value,
};
await route.continue({ headers });
});
// 4. Navigate to the frontend page under test
await page.goto('http://localhost:5173/extracts');
await page.waitForTimeout(5000);
// 5. Inspect results
const bodyText = await page.textContent('body');
console.log(bodyText);
await browser.close();
})();Run from the frontend directory (where cd frontend && node /path/to/script.jsKey details:
Alternative — create a session programmatically (no admin login needed): docker compose -f local.yml exec django python manage.py shell -c "
from django.contrib.sessions.backends.db import SessionStore
from django.contrib.auth import get_user_model
User = get_user_model()
user = User.objects.filter(is_superuser=True).first()
session = SessionStore()
session['_auth_user_id'] = str(user.pk)
session['_auth_user_backend'] = 'django.contrib.auth.backends.ModelBackend'
session['_auth_user_hash'] = user.get_session_auth_hash()
session.save()
print(f'Session key: {session.session_key}')
"Then use the printed session key directly in curl or Playwright route interception. Documentation Locations
Branch StrategyThis project follows trunk-based development:
Changelog MaintenanceIMPORTANT: Always update The changelog follows Keep a Changelog format: ## [Unreleased] - YYYY-MM-DD
### Added
- New features
### Fixed
- Bug fixes with file locations and line numbers
### Changed
- Changes to existing functionality
### Technical Details
- Implementation specifics, architectural notesWhen to update:
What to include:
Pre-commit HooksAutomatically run on commit:
Run manually: Common Pitfalls
|
1 similar comment
CLAUDE.mdThis file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. Project OverviewOpenContracts is an AGPL-3.0 enterprise document analytics platform for PDFs and text-based formats. It features a Django/GraphQL backend with PostgreSQL + pgvector, a React/TypeScript frontend with Jotai state management, and pluggable document processing pipelines powered by machine learning models. Baseline Commit Rules
Essential CommandsBackend (Django)# Run backend tests (sequential, use --keepdb to speed up subsequent runs)
docker compose -f test.yml run django python manage.py test --keepdb
# Run backend tests in PARALLEL (recommended - ~4x faster)
# Uses pytest-xdist with 4 workers, --dist loadscope keeps class tests together
docker compose -f test.yml run django pytest -n 4 --dist loadscope
# Run parallel tests with auto-detected worker count (uses all CPU cores)
docker compose -f test.yml run django pytest -n auto --dist loadscope
# Run parallel tests with fresh databases (first run or after schema changes)
docker compose -f test.yml run django pytest -n 4 --dist loadscope --create-db
# Run specific test file
docker compose -f test.yml run django python manage.py test opencontractserver.tests.test_notifications --keepdb
# Run specific test file in parallel
docker compose -f test.yml run django pytest opencontractserver/tests/test_notifications.py -n 4 --dist loadscope
# Run specific test class/method
docker compose -f test.yml run django python manage.py test opencontractserver.tests.test_notifications.TestNotificationModel.test_create_notification --keepdb
# Apply database migrations
docker compose -f local.yml run django python manage.py migrate
# Create new migration
docker compose -f local.yml run django python manage.py makemigrations
# Django shell
docker compose -f local.yml run django python manage.py shell
# Code quality (runs automatically via pre-commit hooks)
pre-commit run --all-filesFrontend (React/TypeScript)cd frontend
# Start development server (proxies to Django on :8000)
yarn start
# Run unit tests (Vitest) - watches by default
yarn test:unit
# Run component tests (Playwright) - CRITICAL: Use --reporter=list to prevent hanging
yarn test:ct --reporter=list
# Run component tests with grep filter
yarn test:ct --reporter=list -g "test name pattern"
# Run E2E tests
yarn test:e2e
# Coverage reports (unit tests via Vitest, component tests via Playwright + Istanbul)
yarn test:coverage:unit
yarn test:coverage:ct
# Linting and formatting
yarn lint
yarn fix-styles
# Build for production
yarn build
# Preview production build locally
yarn serveProduction Deployment# CRITICAL: Always run migrations FIRST in production
docker compose -f production.yml --profile migrate up migrate
# Then start main services
docker compose -f production.yml upHigh-Level ArchitectureBackend ArchitectureStack: Django 4.x + GraphQL (Graphene) + PostgreSQL + pgvector + Celery Key Patterns:
Frontend ArchitectureStack: React 18 + TypeScript + Apollo Client + Jotai (atoms) + PDF.js + Vite Key Patterns:
Data Flow ArchitectureDocument Processing:
GraphQL Permission Flow:
Critical Security Patterns
Critical Concepts
Testing PatternsManual Test ScriptsLocation: When performing manual testing (e.g., testing migrations, verifying database state, testing API endpoints interactively), always document the test steps in a markdown file under Format: # Test: [Brief description]
## Purpose
What this test verifies.
## Prerequisites
- Required state (e.g., "migration at 0058")
- Required data (e.g., "at least one document exists")
## Steps
1. Step one with exact command
```bash
docker compose -f local.yml run --rm django python manage.py shell -c "..."
Expected Results
CleanupCommands to restore original state if needed. Automated Documentation ScreenshotsLocation: Screenshots for documentation are automatically captured during Playwright component tests and committed back to the PR branch by the How it works:
Naming convention (
At least 2 segments required, 3 recommended. All lowercase alphanumeric with single hyphens. Example: import { docScreenshot } from "./utils/docScreenshot";
// After the component renders and assertions pass:
await docScreenshot(page, "badges--celebration-modal--auto-award");Rules:
Release Screenshots (Point-in-Time)For release notes, use Location: import { releaseScreenshot } from "./utils/docScreenshot";
await releaseScreenshot(page, "v3.0.0.b3", "landing-page", { fullPage: true });Key differences from
When to use which:
Authenticated Playwright Testing (Live Frontend Debugging)When you need to interact with the running frontend as an authenticated user (e.g., debugging why a query returns empty results), use Django admin session cookies to authenticate GraphQL requests. Architecture context: The frontend uses Auth0 for authentication, but the Django backend also accepts session cookie auth. Apollo Client sends GraphQL requests directly to Step 1: Set a password for the superuser (one-time setup): docker compose -f local.yml exec django python manage.py shell -c "
from django.contrib.auth import get_user_model
User = get_user_model()
u = User.objects.filter(is_superuser=True).first()
u.set_password('testpass123')
u.save()
print(f'Password set for {u.username}')
"Step 2: Playwright script pattern: const { chromium } = require('playwright');
(async () => {
const browser = await chromium.launch({ headless: true });
const context = await browser.newContext();
const page = await context.newPage();
// Collect console messages for debugging
const consoleMsgs = [];
page.on('console', msg => consoleMsgs.push('[' + msg.type() + '] ' + msg.text()));
// 1. Login to Django admin to get session cookie
await page.goto('http://localhost:8000/admin/login/');
await page.fill('#id_username', '<superuser-username>');
await page.fill('#id_password', 'testpass123');
await page.click('input[type=submit]');
await page.waitForTimeout(2000);
// 2. Extract the session cookie
const cookies = await context.cookies();
const sessionCookie = cookies.find(c => c.name === 'sessionid');
// 3. Intercept GraphQL requests to inject the session cookie
// (needed because Apollo sends cross-origin requests to :8000)
await page.route('**/graphql/**', async (route) => {
const headers = {
...route.request().headers(),
'Cookie': 'sessionid=' + sessionCookie.value,
};
await route.continue({ headers });
});
// 4. Navigate to the frontend page under test
await page.goto('http://localhost:5173/extracts');
await page.waitForTimeout(5000);
// 5. Inspect results
const bodyText = await page.textContent('body');
console.log(bodyText);
await browser.close();
})();Run from the frontend directory (where cd frontend && node /path/to/script.jsKey details:
Alternative — create a session programmatically (no admin login needed): docker compose -f local.yml exec django python manage.py shell -c "
from django.contrib.sessions.backends.db import SessionStore
from django.contrib.auth import get_user_model
User = get_user_model()
user = User.objects.filter(is_superuser=True).first()
session = SessionStore()
session['_auth_user_id'] = str(user.pk)
session['_auth_user_backend'] = 'django.contrib.auth.backends.ModelBackend'
session['_auth_user_hash'] = user.get_session_auth_hash()
session.save()
print(f'Session key: {session.session_key}')
"Then use the printed session key directly in curl or Playwright route interception. Documentation Locations
Branch StrategyThis project follows trunk-based development:
Changelog MaintenanceIMPORTANT: Always update The changelog follows Keep a Changelog format: ## [Unreleased] - YYYY-MM-DD
### Added
- New features
### Fixed
- Bug fixes with file locations and line numbers
### Changed
- Changes to existing functionality
### Technical Details
- Implementation specifics, architectural notesWhen to update:
What to include:
Pre-commit HooksAutomatically run on commit:
Run manually: Common Pitfalls
|
Code ReviewOverviewThis PR is a solid, well-scoped typing initiative. It raises return-annotation coverage in What Is GoodReal bug fixes included alongside annotations:
Good structural choices:
Issues to Address1. Inconsistent missing-ID error strategy Three resolvers raise Affected: 2. This is acknowledged as a graduation step, and the tradeoff is reasonable for the batch size. But a handful of trivially-typeable resolvers are worth a follow-up: 3.
4. The declared return type is 5.
No-ops / Style Notes
SummaryHigh-value, low-risk PR. The genuine bug fixes (permissioning annotation inversions, null guard on |
- Missing-id resolvers now raise GraphQLError consistently (was Model.DoesNotExist on three resolvers, GraphQLError on resolve_assignment) - _get_user_type/corpus_folder_type/annotation_type return type[T] instead of bare type, with TYPE_CHECKING forward refs to avoid circular imports - resolve_feedback_count: int, resolve_total_messages: int, resolve_all_source_node_in_relationship: QuerySet[Relationship] - id_to_children: dict[int | str, list[int | str]] (was dict[Any, list[Any]])
…1332-1lh9h # Conflicts: # config/graphql/analysis_mutations.py # config/graphql/annotation_types.py # config/graphql/base_types.py # config/graphql/corpus_queries.py # config/graphql/document_queries.py # config/graphql/filters.py # config/graphql/moderation_mutations.py # config/graphql/og_metadata_queries.py # config/graphql/pipeline_queries.py # config/graphql/security.py # config/graphql/slug_queries.py # config/graphql/user_queries.py # config/graphql/user_types.py # docs/typing/mypy_baseline.txt # mypy.ini
Code ReviewOverviewThis PR advances the mypy graduation effort for Substantive Bug Fixes (worth calling out)
The existing annotations
Removing the default is correct. The old default was the
These are real defensive improvements. Annotations without a parent document now return
This is a real behaviour fix: the mutation was returning
Correct. The FK uses
Good improvement. Minor Issues
This is functionally safe given the invariant if pk is None:
return UpdateIngestionSourceMutation(ok=False, message=error if error else _NOT_FOUND_MSG, ...)or just
The CTE helper functions (
Most filter methods now declare Security Observations
The comment correctly explains this is a Django-private flag. Using
This is safe as described:
This is a safe default (deny). Worth confirming that no existing test creates annotations without a document intentionally (e.g. corpus-level structural annotations). The change could introduce a regression if that case exists. Test Coverage
CLAUDE.md Compliance
SummaryApprove with minor suggestions. The substantive fixes (null guards, inverted annotations, |
| @login_required | ||
| @graphql_ratelimit(rate="20/m") | ||
| def mutate(root, info, conversation_id, reason="") -> "LockThreadMutation": | ||
| def mutate(root, info, conversation_id, reason="") -> LockThreadMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="20/m") | ||
| def mutate(root, info, conversation_id, reason="") -> "UnlockThreadMutation": | ||
| def mutate(root, info, conversation_id, reason="") -> UnlockThreadMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="20/m") | ||
| def mutate(root, info, conversation_id, reason="") -> "PinThreadMutation": | ||
| def mutate(root, info, conversation_id, reason="") -> PinThreadMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="20/m") | ||
| def mutate(root, info, conversation_id, reason="") -> "UnpinThreadMutation": | ||
| def mutate(root, info, conversation_id, reason="") -> UnpinThreadMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="10/m") | ||
| def mutate(root, info, conversation_id, reason=None) -> "DeleteThreadMutation": | ||
| def mutate(root, info, conversation_id, reason=None) -> DeleteThreadMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="10/m") | ||
| def mutate(root, info, conversation_id, reason=None) -> "RestoreThreadMutation": | ||
| def mutate(root, info, conversation_id, reason=None) -> RestoreThreadMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="20/m") | ||
| def mutate(root, info, corpus_id, user_id, permissions) -> "AddModeratorMutation": | ||
| def mutate(root, info, corpus_id, user_id, permissions) -> AddModeratorMutation: |
| @login_required | ||
| @graphql_ratelimit(rate="20/m") | ||
| def mutate(root, info, corpus_id, user_id) -> "RemoveModeratorMutation": | ||
| def mutate(root, info, corpus_id, user_id) -> RemoveModeratorMutation: |
| def mutate( | ||
| root, info, action_id, reason=None | ||
| ) -> "RollbackModerationActionMutation": | ||
| def mutate(root, info, action_id, reason=None) -> RollbackModerationActionMutation: |
…e type - Apply pyupgrade/black/isort fixes (Optional → | None) and drop the resulting unused 'Optional'/'Union' imports. - Rename 'info: str' → 'name: str' on django-filters method signatures in filters.py. The parameter receives the filter field name; the 'info' name was misleading copy-paste from GraphQL resolver signatures (where 'info' is graphene.ResolveInfo). - Widen UserQueryMixin.resolve_me return type from User to User | AnonymousUser. The query has no @login_required, so info.context.user can be AnonymousUser at runtime.
Address PR #1369 review (Claude bot): the missing-return fix in config/graphql/analysis_mutations.py — restoring `return DeleteAnalysisMutation(ok=True, message="SUCCESS")` — was the most impactful behavioral fix bundled into the type-annotation pass, but no regression test pinned the success shape. The new test grants DELETE permission on an Analysis to its creator, runs the GraphQL mutation, and asserts the response contains `ok=True`/`message="SUCCESS"` rather than the `null` payload that was previously returned. Without the restored `return`, the assertion on `data.deleteAnalysis` being non-null fails immediately.
Both PR #1369 (this branch, issue #1332) and #1370 (merged via main, issue #1333) graduated different sets of modules out of the mypy baseline. Conflict resolution drops: - admin_auth + graphql_api_token_auth.backends (graduated by #1333) - graphql.{action_queries, agent_mutations, social_types, user_queries, user_types, voting_mutations} (graduated by #1332) from both mypy.ini and docs/typing/mypy_baseline.txt. CHANGELOG.md keeps both Added entries side by side.
…1332-1lh9h Add type annotations to GraphQL resolvers, mutations, and filters
2 participants
Summary
This PR significantly improves type safety in the GraphQL layer by adding comprehensive return type annotations and parameter type hints to resolver methods, mutations, and filter functions across
config/graphql/. This work graduates 22 modules from the mypy baseline allow-list, raising return-annotation coverage from ~4.8% to 91.5% (421/460 function definitions).Key Changes
Filter type annotations (
config/graphql/filters.py): Added return typeQuerySetand parameter types to all custom filter methods inAnalyzerFilter,AnalysisFilter,CorpusFilter, andAnnotationFilterclasses.Resolver return types: Added
-> Anyreturn annotations to resolver methods across multiple type definition files:document_types.py:get_queryset(),resolve_action(),resolve_all_structural_annotations()annotation_types.py:resolve_document(),resolve_annotation_type(),resolve_content_modalities(), etc.corpus_types.py:resolve_corpus_count(),resolve_path(),resolve_document_count(), etc.user_types.py:resolve_reputation_global(),resolve_reputation_for_corpus(),resolve_total_messages()conversation_types.py,agent_types.py,extract_types.py,social_types.py: Similar resolver annotationsQuery mixin type hints (
config/graphql/*_queries.py): Added parameter and return type annotations to resolver methods:user_queries.py:resolve_me(),resolve_user_by_slug(),resolve_userimports()with propergraphene.ResolveInfoandQuerySettypesdocument_queries.py:resolve_documents(),resolve_document()withQuerySet[Document]andOptional[Document]returnsslug_queries.py:resolve_corpus_by_slugs(),resolve_document_by_slugs()withOptionalreturn typespipeline_queries.py: AddedSequenceandPipelineComponentDefinitiontype hintsextract_queries.py,annotation_queries.py,action_queries.py,corpus_queries.py: Resolver type annotationsMutation return types: Added return type annotations to all mutation
mutate()methods across:corpus_mutations.py,document_mutations.py,annotation_mutations.pyextract_mutations.py,corpus_folder_mutations.py,ingestion_source_mutations.pylabel_mutations.py,badge_mutations.py,agent_mutations.py,moderation_mutations.pyconversation_mutations.py,pipeline_settings_mutations.py,worker_mutations.py, etc.Security and utility improvements:
security.py: Added type hints toconditional_csrf_exempt()decorator and created properly-typed_csrf_noop_get_response()functionbase_types.py: Added type hints tobuild_flat_tree()function parameters and return typepermissioning.py: Fixedset_permissions_for_obj_to_user()signature to useTYPE_CHECKINGimport for forward referencesImport improvements: Added
from __future__ import annotationsto enable forward references and improved import organization across multiple files.Mypy configuration: Removed 22 modules from
mypy.inibaseline allow-list (graduated fromignore_errors = True), including:action_queries,agent_mutations,badge_mutations,base_types,conversation_mutations,conversation_types,corpus_types,document_queries,filters,ingestion_source_mutations,moderation_mutations,og_metadata_queries,pipeline_queries,security,serializers,slug_queries,smart_label_mutations,social_types, `user