Should an exception (TypeError or other) be raised when invalid ModelForm Meta attributes are set?

Howdy, all!

Currently, Model validates the attributes set in its inner Meta class and raises an error (TypeError) if an invalid attribute is found. ModelForm, on the other hand, silently ignores invalid attributes that have been set for its inner Meta class. I am proposing to align the behavior of the two Meta classes, specifically that an error should be raised if an invalid attribute has been set in a ModelForm’s inner Meta class.

I opened a Trac ticket on this (#35872) however as Sarah noted, this does have the potential to be a breaking for applications that add their own attributes to an inner Meta class for whatever reason.

My understanding of the Meta class is that it serves as a go-between for some of Django’s private APIs, so I’m not sure that adding non-standard attributes to a Meta class makes sense when these attributes could also be added to the base class in an alternate manner.

There is also an old (opened 17 years ago) ticket related to allowing custom attributes in Model’s inner Meta class (#5793). The discussion on that ticket seems to go in the direction of creating a mechanism for registering custom Meta attributes which means that validation would still be possible (and, in my opinion) advised. I would imagine that if this feature is implemented, a similar approach would be used for custom attributes in ModelForm’s Meta class, meaning that attribute validation would/should still make sense.

Thoughts?

The breakage can be handled by validating and outputting a warning for one major version and then changing it to be an error later.

I would like to suggest allowing an extra argument where you can put whatever you want as an end user, and/or the validation ignoring everything beginning with _. We do the extra thing in iommi and it’s very useful (we also have it for Fields which is even more useful).

I haven’t put any real thought into this but my initial thought would be that it’s better as a system check than some validation code at runtime.

The system check framework cannot be used to validate forms because unlike models, there is no way to iterate through all the forms in a project.

2 Likes

Especially not if the forms are created at runtime programmatically.

1 Like

Fascinating, I didn’t know that (there’s admittedly much of the Django codebase that I’m not / haven’t needed to get very familiar) and I shall have to take a look!

As a follow-up, on this, if we were to emit a warning, would it be with Python’s built-in warning system (e.g. warnings.warn()) or is there a Django-ier way to accomplish this? I know of Django messages, but that (from my understanding) requires the messages middleware which isn’t necessarily universal.

warnings.warn. Django has specific classes to say which version the old behavior is dropped in, so like RemovedInDjango60Warning or something.

How might we feel about a RuntimeWarning? Given that there hasn’t yet been a consensus or decision on whether custom Meta attributes are consistent with the purpose of the inner Meta class and/or how they might be implemented or checked if they were, I think this could potentially catch errors while still being non-breaking and not necessarily committing to a particular direction for the future.

In that case I think a specific subclass should be introduced so it can be ignored cleanly.

Indeed, I saw the Model part but not the Form. More coffee needed.

Then I don’t have a strong opinion. It adds a minor guardrail at the cost of some extra complexity and a probably imperceptible performance loss.

But I would advocate for a specific exception subclass to ease ignoring and testing as well, if we do this at all.