Skip to content

Clean up bulk operation code quality #2

@tnware

Description

@tnware

Problem Statement

The bulk operation methods in mixins.py (_apply_bulk_unbind, _apply_bulk_bind, _apply_bulk_operations) have three code quality issues that should be cleaned up to match the style and behavior of the non-bulk path.

Technical Context

The bulk operations were added alongside the existing individual-save path (_apply_individual_operations). The two paths should behave consistently, but the bulk path introduced patterns that diverge from the rest of the module.

Affected Code Areas

  • django_admin_reversefields/mixins.py
    • _apply_bulk_unbind (around line 849)
    • _apply_bulk_bind (around line 889)
    • _apply_bulk_operations (around line 933)

Problems

1. Redundant in-method imports

All three methods do from django import forms and from django.db import IntegrityError inside the method body. forms is already imported at module level (line 51). These should use the existing module-level import.

2. Redundant .exists() before .update()

_apply_bulk_unbind calls queryset.exists() before queryset.update(...). Django's .update() on an empty queryset is a no-op that returns 0 — the .exists() check adds an unnecessary database round-trip.

3. Overly broad exception handling

All three methods catch bare Exception and wrap it as forms.ValidationError. This swallows infrastructure errors (OperationalError, ProgrammingError, connection failures) into form validation errors — these are not validation problems. The non-bulk path (_apply_individual_operations) lets exceptions propagate naturally. The two paths should behave consistently.

Acceptance Criteria

  • No from django import forms inside any of the three bulk methods — use the module-level import
  • No from django.db import IntegrityError inside any of the three bulk methods — add it to the module-level imports if not already present
  • The .exists() guard before .update() in _apply_bulk_unbind is removed
  • The broad except Exception blocks are removed; only IntegrityError is caught (matching non-bulk behavior), or exceptions propagate naturally
  • All existing tests pass

Non-Goals

  • Changing bulk operation logic or behavior
  • Adding new tests (existing parameterized tests already cover both bulk and non-bulk paths)
  • Modifying the non-bulk path

Risk Considerations

Low. These are cleanup changes that don't alter behavior. The existing parameterized test suite runs all binding, validation, and permission tests with bulk=True and bulk=False, so regressions would be caught.

Testing Expectations

All 35 existing tests must pass. No new tests required — the parameterized tests already exercise both code paths.

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactorCode improvement without behavior changesemver:patchPatch version bump

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions