Fixes: #719 - Enforce object/multiobject required at the model layer

netbox_custom_objects/field_types.py

@@ -221,10 +221,12 @@ def _make_lazy_cot_fk(cot, field, on_delete, **field_kwargs):
             field.custom_object_type.id
         ).lower()
         related_name = f"{table_model_name}_{field.name}_set"
+    # blank ties to required, same as the direct (non-lazy) ForeignKey branch in
+    # ObjectFieldType.get_model_field() -- see the comment there.
     return LazyForeignKey(
         model_name,
         null=True,
-        blank=True,
+        blank=not field.required,
         on_delete=on_delete,
         related_name=related_name,
         **field_kwargs
@@ -1004,8 +1006,12 @@ def get_model_field(self, field, **kwargs):
         else:
             table_model_name = field.custom_object_type.get_table_model_name(field.custom_object_type.id).lower()
             related_name = f"{table_model_name}_{field.name}_set"
+        # blank ties to required, so full_clean() rejects an unset required
+        # object field like any scalar type; null stays True regardless (a
+        # DB-level concern, not a user-facing one).
         f = models.ForeignKey(
-            model, null=True, blank=True, on_delete=on_delete, related_name=related_name, **field_kwargs
+            model, null=True, blank=not field.required, on_delete=on_delete, related_name=related_name,
+            **field_kwargs
         )
 
         return f
@@ -1613,12 +1619,16 @@ def get_model_field(self, field, **kwargs):
             m2m_related_name = "+"
             m2m_related_query_name = "+"
 
-        # For self-referential fields, use 'self' as the target
+        # blank ties to required, mirroring the FK above. Has no effect on
+        # full_clean() (Django's clean_fields() excludes M2M fields entirely),
+        # but does matter to the required-toggle pre-flight check in
+        # CustomObjectTypeField.clean(), which queries existing data directly
+        # rather than relying on full_clean().
         m2m_field = CustomManyToManyField(
             to="self" if is_self_referential else model_string,
             through=through,
             through_fields=("source", "target"),
-            blank=True,
+            blank=not field.required,
             related_name=m2m_related_name,
             related_query_name=m2m_related_query_name,
             **field_kwargs

netbox_custom_objects/models.py

@@ -2978,17 +2978,17 @@ def clean(self):
             model_field = FIELD_TYPE_CLASS[self.type]().get_model_field(self)
             columns = model_field if isinstance(model_field, dict) else {self.name: model_field}
             # Only columns actually made non-blank by this field's required flag
-            # (e.g. a url field's title column always stays blank=True). A
-            # relationship field's "column" may not be a real, directly queryable
-            # Django Field at all: a polymorphic multiobject's is a
-            # PolymorphicM2MDescriptor (no .blank attribute), and a polymorphic
-            # object field's dict includes a GenericForeignKey entry alongside its
-            # two real backing columns - a real Field with .blank=False by default,
-            # but not itself a queryable column (values_list() can't resolve it).
-            # Treat anything without a usable .blank, and GFKs specifically, as
-            # always blank=True, i.e. not checkable here - matching
-            # ObjectFieldType/MultiObjectFieldType's own concrete model fields,
-            # which hardcode blank=True regardless of required.
+            # (e.g. a url field's title column always stays blank=True). Plain
+            # object/multiobject fields follow required and so ARE checkable
+            # here. A *polymorphic* relationship field's "column" may not be a
+            # real, directly queryable Django Field at all, though: a
+            # polymorphic multiobject's is a PolymorphicM2MDescriptor (no
+            # .blank attribute), and a polymorphic object field's dict includes
+            # a GenericForeignKey entry alongside its two real backing columns
+            # - a real Field with .blank=False by default, but not itself a
+            # queryable column (values_list() can't resolve it). Treat anything
+            # without a usable .blank, and GFKs specifically, as always
+            # blank=True, i.e. not checkable here.
             required_columns = {
                 name: f for name, f in columns.items()
                 if not isinstance(f, GenericForeignKey) and not getattr(f, 'blank', True)

netbox_custom_objects/tests/test_field_types.py

@@ -101,6 +101,49 @@ def test_url_title_stays_optional_regardless_of_required(self):
         self.assertFalse(model._meta.get_field('url_req').blank)
         self.assertTrue(model._meta.get_field('url_req_title').blank)
 
+    def test_object_field_blank_matches_required(self):
+        """A plain object field's FK follows required, like every scalar type --
+        and full_clean() enforces it, since FK fields are validated by
+        Django's clean_fields()."""
+        site_ot = ObjectType.objects.get(app_label='dcim', model='site')
+        required = self.create_custom_object_type_field(
+            self.custom_object_type, name='site_req', type='object',
+            related_object_type=site_ot, required=True,
+        )
+        optional = self.create_custom_object_type_field(
+            self.custom_object_type, name='site_opt', type='object',
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        self.assertFalse(model._meta.get_field(required.name).blank)
+        self.assertTrue(model._meta.get_field(optional.name).blank)
+        self.assertTrue(model._meta.get_field(required.name).null)
+
+        instance = model(name='obj')
+        with self.assertRaises(ValidationError):
+            instance.full_clean()
+
+    def test_multiobject_field_blank_matches_required_but_full_clean_cannot_check_it(self):
+        """A plain multiobject field's M2M also follows required (the
+        required-toggle pre-flight check relies on it), but full_clean() can't
+        enforce it -- Django's clean_fields() excludes M2M fields entirely, so
+        required stays enforced at the REST/UI layer for multiobject."""
+        site_ot = ObjectType.objects.get(app_label='dcim', model='site')
+        required = self.create_custom_object_type_field(
+            self.custom_object_type, name='sites_req', type='multiobject',
+            related_object_type=site_ot, required=True,
+        )
+        optional = self.create_custom_object_type_field(
+            self.custom_object_type, name='sites_opt', type='multiobject',
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        self.assertFalse(model._meta.get_field(required.name).blank)
+        self.assertTrue(model._meta.get_field(optional.name).blank)
+
+        instance = model.objects.create(name='obj')
+        instance.full_clean()  # does not raise -- M2M is outside clean_fields()'s reach
+
 
 class TextFieldTypeTestCase(FieldTypeTestCase):
     """Test cases for text field type."""

netbox_custom_objects/tests/test_models.py

@@ -779,27 +779,20 @@ def test_required_toggle_rejected_for_coordinates_with_one_half_blank(self):
         with self.assertRaises(ValidationError):
             field.full_clean()
 
-    def test_required_toggle_does_not_crash_for_relationship_fields(self):
-        """Object/multiobject fields (plain and polymorphic) must not crash on toggle.
-
-        Their model field(s) either hardcode blank=True unconditionally (plain
-        object/multiobject) or are not a real, directly queryable Django Field at
-        all (a polymorphic object field's GenericForeignKey entry; a polymorphic
-        multiobject field's PolymorphicM2MDescriptor) - none of them are checked
-        by the required-toggle pre-flight check, but they must be skipped
-        cleanly rather than raising AttributeError/FieldError.
+    def test_required_toggle_does_not_crash_for_polymorphic_relationship_fields(self):
+        """Polymorphic object/multiobject fields must not crash on toggle.
+
+        Their model field(s) are not a real, directly queryable Django Field at all
+        (a polymorphic object field's GenericForeignKey entry; a polymorphic
+        multiobject field's PolymorphicM2MDescriptor, which has no .blank attribute)
+        -- neither is checked by the required-toggle pre-flight check, but they must
+        be skipped cleanly rather than raising AttributeError/FieldError. Plain
+        object/multiobject fields, by contrast, ARE checked (see
+        test_required_toggle_rejected_when_existing_object_row_is_blank et al).
         """
         device_ot = self.get_device_object_type()
         site_ot = self.get_site_object_type()
 
-        plain_object = self.create_custom_object_type_field(
-            self.custom_object_type, name="dev", type="object",
-            related_object_type=device_ot, required=False,
-        )
-        plain_multiobject = self.create_custom_object_type_field(
-            self.custom_object_type, name="devs", type="multiobject",
-            related_object_type=device_ot, required=False,
-        )
         poly_object = self.create_polymorphic_field(
             self.custom_object_type, related_object_types=[device_ot, site_ot],
             name="poly_dev", type="object", required=False,
@@ -809,12 +802,100 @@ def test_required_toggle_does_not_crash_for_relationship_fields(self):
             name="poly_devs", type="multiobject", required=False,
         )
 
-        for field in (plain_object, plain_multiobject, poly_object, poly_multiobject):
+        for field in (poly_object, poly_multiobject):
             with self.subTest(field=field.name):
                 field = CustomObjectTypeField.objects.get(pk=field.pk)
                 field.required = True
                 field.full_clean()  # must not raise
 
+    def test_required_toggle_rejected_when_existing_object_row_is_blank(self):
+        """A plain object field's FK is checked by the required-toggle
+        pre-flight check, same as every scalar type."""
+        site_ot = self.get_site_object_type()
+        field = self.create_custom_object_type_field(
+            self.custom_object_type, name="site", type="object",
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        model.objects.create()
+
+        field = CustomObjectTypeField.objects.get(pk=field.pk)
+        field.required = True
+        with self.assertRaises(ValidationError):
+            field.full_clean()
+
+    def test_required_toggle_allowed_when_no_blank_object_rows(self):
+        """Toggling a plain object field to required succeeds when every existing
+        row already has a value."""
+        site_ot = self.get_site_object_type()
+        site = Site.objects.create(name="Req Toggle Site", slug="req-toggle-site")
+        field = self.create_custom_object_type_field(
+            self.custom_object_type, name="site", type="object",
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        model.objects.create(site=site)
+
+        field = CustomObjectTypeField.objects.get(pk=field.pk)
+        field.required = True
+        field.full_clean()  # must not raise
+
+    def test_required_toggle_rejected_when_existing_multiobject_row_is_blank(self):
+        """A plain multiobject field's M2M is also checked by the required-toggle
+        pre-flight check: even though full_clean() can never validate an M2M
+        field, the pre-flight check queries existing data directly via
+        values_list(), which still correctly catches a blank row."""
+        site_ot = self.get_site_object_type()
+        field = self.create_custom_object_type_field(
+            self.custom_object_type, name="sites", type="multiobject",
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        model.objects.create()
+
+        field = CustomObjectTypeField.objects.get(pk=field.pk)
+        field.required = True
+        with self.assertRaises(ValidationError):
+            field.full_clean()
+
+    def test_required_toggle_rejected_when_one_of_several_multiobject_rows_is_blank(self):
+        """The pre-flight check's values_list() query must use a LEFT OUTER JOIN,
+        not an INNER JOIN, across the M2M -- otherwise a blank row would be
+        silently excluded from the result rather than surfaced as (None,), and a
+        blank row sitting alongside filled ones would slip through undetected."""
+        site_ot = self.get_site_object_type()
+        site = Site.objects.create(name="Req Toggle Site Mixed", slug="req-toggle-site-mixed")
+        field = self.create_custom_object_type_field(
+            self.custom_object_type, name="sites", type="multiobject",
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        filled = model.objects.create()
+        getattr(filled, "sites").set([site])
+        model.objects.create()  # blank, alongside the filled row above
+
+        field = CustomObjectTypeField.objects.get(pk=field.pk)
+        field.required = True
+        with self.assertRaises(ValidationError):
+            field.full_clean()
+
+    def test_required_toggle_allowed_when_no_blank_multiobject_rows(self):
+        """Toggling a plain multiobject field to required succeeds when every
+        existing row already has at least one related object."""
+        site_ot = self.get_site_object_type()
+        site = Site.objects.create(name="Req Toggle Site M2M", slug="req-toggle-site-m2m")
+        field = self.create_custom_object_type_field(
+            self.custom_object_type, name="sites", type="multiobject",
+            related_object_type=site_ot, required=False,
+        )
+        model = self.custom_object_type.get_model()
+        instance = model.objects.create()
+        getattr(instance, "sites").set([site])
+
+        field = CustomObjectTypeField.objects.get(pk=field.pk)
+        field.required = True
+        field.full_clean()  # must not raise
+
     def test_custom_object_type_field_unique_name_per_type(self):
         """Test that field names must be unique within a custom object type."""
         self.create_custom_object_type_field(