Fixes: #700 - Enforce required on scalar fields at the model and REST layers

[bctiemann]

Closes: #700

Summary

Every scalar field type (text, longtext, integer, decimal, boolean, date, datetime, url, json, select, multiselect, coordinates) hardcoded blank=True on its generated model field regardless of CustomObjectTypeField.required, and had no explicit get_serializer_field() of its own — so DRF's ModelSerializer auto-built a serializer field from the model field and always inferred required=False. required was therefore enforced only by the UI form, which constructs its own form field with required=field.required directly, independent of the model field.

• get_model_field() now sets blank=not field.required for every affected type. null stays True unconditionally — this plugin already commits to "toggling required must never risk an ALTER TABLE ... SET NOT NULL" (see test_branching.py::test_field_required_toggle_merge), and DRF's own required auto-inference treats a nullable column as non-required regardless of blank anyway, so leaving null=True and fixing blank alone wouldn't have been enough even if DB-level enforcement were in scope.
• Each affected type now implements get_serializer_field() explicitly, passing required=field.required (mirroring the pattern ObjectFieldType/MultiObjectFieldType already used correctly).
• CoordinatesFieldType.get_serializer_field() returns a dict (one entry per backing column — <name>_latitude/<name>_longitude), mirroring get_model_field()'s existing multi-column convention. serializers.py's field-building loop now merges a dict return into attrs and exempts coordinates fields from the single-column model_field_names guard (neither backing column is literally named after the field itself).
• URLFieldType's title column is unaffected by this — it stays always-optional by design (see the class docstring), only the URL column itself follows required.
• Existing default-value-on-omission behavior is preserved: the serializer still doesn't pass an explicit default= of its own, so a model-level default still applies when a key is simply absent from the payload (for a non-required field).

Test plan

• RequiredFieldEnforcementTestCase (new, test_field_types.py) — model-layer blank correctly follows required for all 9 plain scalar types, select/multiselect, both coordinates halves, and confirms the URL title column stays optional regardless.
• RequiredScalarFieldAPITest (new, test_api.py) — REST-layer: POST omitting required fields returns 400 with per-field errors; POST with all required fields (or only the optional one omitted) returns 201; OPTIONS reports required: true/false correctly, including the URL title column.
• test_branching.py::test_field_required_toggle_merge (existing, unmodified) — confirms toggling required still does not produce a DB-level NOT NULL constraint.
• Full netbox_custom_objects suite (1284 tests) run against a freshly built database — 17 pre-existing failures observed, matching the already-established baseline exactly (netbox_branching's ObjectChange app_label RuntimeError in .delete()/rename code paths under a non-branching test config), confirmed unrelated to this change.
• ruff check passes on all changed files.

[bctiemann]

@claude /review and limit response to actionable issues.

[github-actions]

Claude finished @bctiemann's task in 4m 41s —— View job

---

Code Review

• Read changed files
• Analyze diff
• Post review

---

The fix is correct and the approach (fixing blank on model fields + adding explicit get_serializer_field() on every scalar type) is the right one. Three actionable issues below.

---

1. API test never exercises required text/longtext enforcement

test_post_omitting_required_fields_returns_400 sends {"name": "Missing required fields"}, which satisfies the only required text field (name). Neither text nor longtext appear in the asserted field-error list, so the serializer enforcement for those two types is not covered at the REST layer. The model-layer test covers blank, but a dedicated API assertion (add req_text/req_longtext fields to the COT and assert they appear in the 400 body when omitted) would close the gap.

tests/test_api.py:2362–2371

Fix this →

---

2. ChoiceField(allow_blank=True) bypasses choice validation for empty strings

SelectFieldType.get_serializer_field() passes allow_blank=not field.required. DRF's ChoiceField.to_internal_value() short-circuits on "" when allow_blank=True — it returns "" without checking whether "" is in the choices. A non-required select field therefore silently accepts "" via the API and stores it, even though "" is not a defined choice. The model field also allows blank=True, so the value persists in the DB.

This creates a path for out-of-band values in the select column. If the intent is that omitting the field leaves it null, the field should accept null but not "". The fix: remove allow_blank (DRF ChoiceField defaults to allow_blank=False), and let allow_null=True cover the "no selection" case.

field_types.py:767–773

# Current
return drf_serializers.ChoiceField(
    choices=field.choices,
    required=field.required,
    allow_null=not field.required,
    allow_blank=not field.required,   # ← bypasses choice validation
)

# Suggested
return drf_serializers.ChoiceField(
    choices=field.choices,
    required=field.required,
    allow_null=not field.required,
)

Fix this →

---

3. Exception fallback for coordinates fields uses a non-existent column name

serializers.py:800–811: when get_serializer_field() raises an unexpected exception, the code falls back to attrs[field.name] = serializers.JSONField(...). For coordinates fields, field.name is not a backing column (only field.name + '_latitude' / field.name + '_longitude' exist). The continue on line 811 prevents the dict-branch from running, but the fallback itself assigns a field under the wrong name — the orphaned entry won't correspond to any model field, and the two real backing columns are left with no serializer at all.

The pre-PR code had the same gap (coordinates had no get_serializer_field() before this change, so it took the NotImplementedError path instead). Now that coordinates fields have a real implementation, this edge case is only reachable on truly unexpected errors, but it's still incorrect when hit.

api/serializers.py:800–811

Fix this →

---

[bctiemann]

Addressed all three in f846402:

1. Missing text/longtext API coverage — added req_text/req_longtext to RequiredScalarFieldAPITest's fixture and both assertion loops; previously every test payload's only required text-type field was name, which every payload already satisfied.
2. allow_blank bypassing choice validation — removed allow_blank=not field.required from SelectFieldType.get_serializer_field(). Confirmed empirically: before the fix, POST {"opt_select": ""} on a non-required select field returned 201 and stored ""; after, it correctly returns 400 ('"" is not a valid choice.'), while omitting the field or submitting a real choice still succeeds.
3. Coordinates exception fallback wrong column name — the except Exception fallback in get_serializer_class() now assigns the fallback JSONField under both _latitude/_longitude backing columns for coordinates fields instead of the non-existent field.name.

test_api/test_field_types are at 215/216 (1 pre-existing skip, expected — query-count baselines self-skip when netbox-branching is installed).

[jnovinger]

test_field_required_toggle_merge's docstring (test_branching.py:2991-2993) still says field_types.py "hardcodes null=True, blank=True" and that required is form-layer only. Both changed here. The test still passes because it only asserts is_nullable.

[bctiemann]

Addressed both in 11ad686:

1. Required-toggle strands existing blank rows — added a pre-flight check to CustomObjectTypeField.clean(), mirroring the existing non-unique-values check right above it (replied inline on that thread with details).
2. Stale test_field_required_toggle_merge docstring — updated to describe the current behavior (blank/serializer required enforce it; null=True unconditionally, not blank=True) instead of the pre-Required fields are enforced only in the UI form, never at the model or REST API layer #700 state. The test's own assertion (DB column stays nullable) was already correct and unchanged.

[jnovinger]

Object and multiobject fields still hardcode blank=True (field_types.py:932, :937, :982, :1595), so required on those two types stays REST-only. The scalar types now enforce at both layers, which leaves the same "Required" checkbox meaning two different things depending on field type.

#700 is written against the scalar types, so this may be deliberate scope. Worth a follow-up issue if so.

[bctiemann]

Both addressed:

1. Crash on polymorphic relationship fields — fixed in b74fc44 (replied inline with details). Root cause: get_model_field() returns a bare PolymorphicM2MDescriptor for polymorphic multiobject (no .blank attribute → AttributeError), and a dict including a GenericForeignKey entry for polymorphic object (a real Field, but not a directly-queryable column → FieldError from values_list()). Both are now explicitly excluded from the pre-flight check, matching how plain object/multiobject's own concrete fields (hardcoded blank=True always) were already excluded. Verified all four relationship-field variants (plain object, plain multiobject, polymorphic object, polymorphic multiobject) toggle cleanly now, with a new test covering all four.
2. Object/multiobject model-layer scoping — confirmed deliberate: Required fields are enforced only in the UI form, never at the model or REST API layer #700's own reproduction excluded these two types from the start, since their serializer-level required was already correct pre-Required fields are enforced only in the UI form, never at the model or REST API layer #700 (only scalar types had that gap). Filed as a follow-up: #719 / Linear NPL-1383, linked under the same REST-audit parent as this ticket.