Skip to content

Fixes: #715 - Enforce integer/decimal min/max at the model and REST layers - #716

Open
bctiemann wants to merge 2 commits into
700-required-fields-not-enforcedfrom
715-integer-decimal-min-max-validation
Open

bctiemann wants to merge 2 commits into
700-required-fields-not-enforcedfrom
715-integer-decimal-min-max-validation

Conversation

@bctiemann

@bctiemann bctiemann commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Closes: #715

Summary

Follow-up to #700, raised during that work: IntegerFieldType/DecimalFieldType.get_model_field() never attached MinValueValidator/MaxValueValidator to the generated model field, and their get_serializer_field() (added in #700) never passed min_value/max_value to the DRF field either — only get_form_field() enforced validation_minimum/validation_maximum. Same underlying gap-shape as #700, just for range validation instead of required.

Note: this PR targets the 700-required-fields-not-enforced branch, not main — it builds directly on the get_serializer_field() methods #700/#714 introduces, which aren't on main yet. Retarget to main once #714 merges.

  • Both get_model_field() implementations now append MinValueValidator/MaxValueValidator (guarded by is not None) to the model field's validators.
  • Both get_serializer_field() implementations pass min_value=field.validation_minimum, max_value=field.validation_maximum directly — DRF's IntegerField/DecimalField already no-op when either is None, so no extra guarding is needed on that side.

Test plan

  • test_integer_field_min_max_enforced_by_full_clean / test_decimal_field_min_max_enforced_by_full_clean (new, test_field_types.py) — model-layer full_clean() rejects out-of-range values, accepts in-range ones.
  • IntegerDecimalMinMaxAPITest (new, test_api.py) — REST-layer: POST above/below the configured range returns 400 with per-field errors; POST within range returns 201.
  • Empirically verified both the model (full_clean()) and REST layers reject out-of-range values on both bounds, for both types.
  • Broader regression pass (test_api, test_field_types, test_forms, test_views — 427 tests) — 4 pre-existing errors observed, matching the already-established netbox_branching app_label baseline, confirmed unrelated.
  • ruff check passes on all changed files.

IntegerFieldType/DecimalFieldType.get_model_field() never attached
MinValueValidator/MaxValueValidator to the generated model field, and
get_serializer_field() (added in #700) never passed min_value/max_value to
the DRF field either - only get_form_field() enforced validation_minimum/
validation_maximum, the same underlying gap as #700 for range validation
instead of required.

Both get_model_field() implementations now append MinValueValidator/
MaxValueValidator (guarded by is not None) to the model field's validators,
and both get_serializer_field() implementations pass
min_value=field.validation_minimum, max_value=field.validation_maximum
directly - DRF's IntegerField/DecimalField already no-op when either is
None, so no extra guarding is needed there.
@bctiemann
bctiemann added this pull request to stack #717 September 17, 2026 19:48
@bctiemann bctiemann changed the title Fix #715: enforce integer/decimal min/max at the model and REST layers Fixes: #715 - Enforce integer/decimal min/max at the model and REST layers Sep 17, 2026
@bctiemann

Copy link
Copy Markdown
Contributor Author

@claude /review and limit response to actionable issues.

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown

Claude finished @bctiemann's task in 1m 53s —— View job


Review: Enforce integer/decimal min/max at model and REST layers

The fix is correct — all three enforcement layers (model full_clean(), REST serializer, UI form) now agree. Two actionable issues below.


1. validators=[] silently overwrites any validators from **kwargs

In both IntegerFieldType.get_model_field() and DecimalFieldType.get_model_field(), validators is always included in the field_kwargs.update(...) call, even when empty. Because _safe_kwargs(**kwargs) could have already placed a validators key into field_kwargs, the unconditional .update({"validators": validators}) overwrites it:

# field_types.py:420-422 (IntegerFieldType), 458-460 (DecimalFieldType)
field_kwargs.update({
    "default": field.default, "unique": field.unique, "validators": validators,
})

No current caller passes validators via **kwargs, so this doesn't cause a regression today. But the pattern is inconsistent — the prior code (which didn't write validators at all) would have preserved any caller-supplied validators, while the new code silently drops them. A minimal fix:

field_kwargs.update({"default": field.default, "unique": field.unique})
if validators:
    field_kwargs["validators"] = validators

Fix this →


2. Tests don't cover exact boundary values

Both new full_clean() tests and the API test use midpoint (50) as the valid case and ±1 outside the range as the invalid case, but never assert that the bounds themselves (0, 100) are accepted:

# test_field_types.py:256, 416
model(name="Test", count=50).full_clean()   # passes — but what about count=0 and count=100?

This is relevant because a validator implementation mistake (e.g. > instead of >=) would pass the current tests. Worth adding:

model(name="Test", count=0).full_clean()    # exact minimum
model(name="Test", count=100).full_clean()  # exact maximum

Same applies to test_post_within_range_returns_201 — only 50/50.00 is tested, not the endpoints.

Fix this →


Neither issue is a correctness bug that would manifest in production today, but issue 1 is a latent trap and issue 2 is a test coverage gap.

- Only add "validators" to field_kwargs when non-empty in
  IntegerFieldType/DecimalFieldType.get_model_field(), so a caller-supplied
  validators kwarg isn't silently overwritten when neither
  validation_minimum nor validation_maximum is set.
- Add exact-boundary assertions (validation_minimum/maximum themselves, not
  just values just inside/outside the range) to both full_clean() tests and
  the REST API test.
@bctiemann

Copy link
Copy Markdown
Contributor Author

Addressed both in d51453d:

  1. validators=[] overwriting caller-supplied validatorsfield_kwargs["validators"] is now only set when the computed list is non-empty, so a validators kwarg passed in via **kwargs (none currently do, but as you noted, the pattern was a latent trap) is preserved when neither validation_minimum nor validation_maximum is set.
  2. Boundary coverage — added exact-boundary assertions (count=0/count=100, price=Decimal("0.00")/Decimal("100.00")) to both full_clean() tests, plus a new test_post_at_exact_boundaries_returns_201 in the REST test covering the same for both int and decimal.

14/14 in the affected test classes.

@bctiemann
bctiemann requested review from a team, jnovinger and pheus and removed request for a team and pheus September 17, 2026 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integer/Decimal min/max validation is enforced only in the UI form, never at the model or REST API layer

2 participants