Backwards compatible patch for ModelForm to improve model validation.

[Oroku Saki]

Sometimes a model needs to create other model instances (to create relationships that are
unimportant to the model's client) at the time of its own instantiation. This brings up
problems with data integrity. If a model fails one of the validators, the related objects
will be left as meaningless rows (which could have side effects such as unique values used
up, etc). In order to conquer this problem Model.foo_method() level transactions need to
be used. This cannot currently be done. The reason is that, although the docs say
differently (this is not a documentation ticket), they are currently incorrect. Rather,
the code is incorrect and the docs are correctly in tune with what Django had in mind when
it designed Model validation.

Currently you cannot do this because ModelForm._post_clean() re-implements
Model.full_clean() rather than just calling it. Its implementation is only 1 line
different than just calling Model.full_clean().

Example Usage

    @transaction.commit_on_success
    def full_clean(exclude=None):
		"""
		Calls ``Model.clean()``, ``Model.clean_fields()``, and ``Model.validate_unique()``
		"""
        super(Widget, self).full_clean(exclude)
    
    def clean(self):
		"""
        Creates self.leg (expecting this to be undone buy transaction mentioned 
		above if self.save() never completes). The foreign relationship created 
		here is purely for implementation reasons, and does not concern the client 
		(ie, the view). It should function the same with any view regardless to 
		whether a view (Admin, custom view, Django Piston JSON API, etc) enlists
		the use of transactions, etc. It is a means of separation of concerns, 
		since a Widget's creator
		"""
		try:
			self.leg = WidgetLeg.objects.create(friend=self, name=self.name)
		except IntegrityError:
			raise ValidationError('You did this or that wrong when saving ``Widget``.')

Current Django Code

Current Implementation of ModelForm._post_clean()

    def _post_clean(self):
        exclude = self._get_validation_exclusions()
        opts = self._meta
		
        # Update the model instance with self.cleaned_data.
        self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
		
        # Clean the model instance's fields.
        try:
            self.instance.clean_fields(exclude=exclude)
        except ValidationError, e:
			self._update_errors(e.message_dict)
		
        # Call the model instance's clean method.
        try:
            self.instance.clean()
        except ValidationError, e:
            self._update_errors({NON_FIELD_ERRORS: e.messages})
		
        # Validate uniqueness if needed.
        if self._validate_unique:
            self.validate_unique()

Current Implementation of Model.full_clean() ( almost identical to ModelForm._post_clean() )

    def full_clean(self, exclude=None):
        """
        Calls clean_fields, clean, and validate_unique, on the model,
        and raises a ``ValidationError`` for any errors that occured.
        """
        errors = {}
        if exclude is None:
            exclude = []
		
        try:
            self.clean_fields(exclude=exclude)
        except ValidationError, e:
            errors = e.update_error_dict(errors)
		
        # Form.clean() is run even if other validation fails, so do the
        # same with Model.clean() for consistency.
        try:
            self.clean()
        except ValidationError, e:
            errors = e.update_error_dict(errors)
		
        # Run unique checks, but only for fields that passed validation.
        for name in errors.keys():
            if name != NON_FIELD_ERRORS and name not in exclude:
                exclude.append(name)
        try:
            self.validate_unique(exclude=exclude)
        except ValidationError, e:
            errors = e.update_error_dict(errors)
 
        if errors:
            raise ValidationError(errors)

Current Model.clean()

    def clean(self):
        self._validate_unique = True
        return self.cleaned_data

And inside of ModelForm.init() on line 251

        self._validate_unique = False

And... The Patch

Change for ModelForm._post_clean() to use Model.full_clean() instead of reimplementing it

    def _post_clean(self):
        exclude = self._get_validation_exclusions()
        opts = self._meta
 
        # Update the model instance with self.cleaned_data.
        self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)
 
        # Perform all model validation by using the already implemented Model.full_clean()
        try:
			self.instance.full_clean(exclude=exclude)
        except ValidationError, e:
            self._update_errors(e.message_dict)

Change for ModelForm.clean() to remove special self._validate_clean flag

    def clean(self):
		# This was just setting the flag to ``True`` all the time anyway, which is an 
		# easy placeholder to re-implement in ``Model.clean()`` if needed.
        return self.cleaned_data

Change to ModelForm.init() to remove self._validate_clean flag

        # Nothing, this same flagging behavior could be 

The result

With this, the implementation details of Model.full_clean() are now orthogonal to ModelForms, and
ModelForms works purely on propagation. You can now use transactions and have full access to making
any decorator you wish to wrap the entire validation level so that you can handle errors, or whatever
other cool stuff the pros out there can think up, right from the warm comfort of the Model, which means
no more repeating Model implementation steps in every single view for one model, or creating custom
model forms just for the Admin.

I really care a lot about this patch going into production, and if there are weaknesses that I may have
overlooked, please let me know and I will update my code. This is why I've written a patch for this
problem and opened a new ticket. I don't want to use a self-hacked Django.

[Russell Keith-Magee]

Closing as a duplicate of #13100. Because it is. You even submitted the same patch. If you don't like a decision on a previous ticket, you don't open a new ticket. You start a discussion on django-developers.

[Oroku Saki]

@russellm It's not a duplicate. The other ticket is a documentation ticket (You made it that). This is a Model ORM ticket. The patch is different, and includes all levels of changes needed instead of just the one method. I created this for two reasons:

1) I couldn't delete my half-baked patch on the documentation ticket.
2) I wanted a real patch that required zero work by you.

Please don't let ego get in the way of a 100% justified, all-work-done patch that will improve Django's architecture and flexibility at zero cost.

[Oroku Saki]

I also included better justification and documentation of each step.

[Russell Keith-Magee]

Please don't let ego get in the way of a 100% justified, all-work-done patch that will improve Django's architecture and flexibility at zero cost.

Sigh.

Why do you assume the problem is my ego, and not the fact that ​you're completely ignoring our well documented guidelines for contributing to Django?

We want people to help. We want people to contribute.

We *don't* want to spend our lives gardening Trac looking for the way that someone is trying to re-propose an idea that has been previously rejected. We want people to work *with* us, not fight us every step of the way.

You made a proposal. That proposal was rejected, but a broader documentation issue was identified. You reopened the ticket, and moved it back to a bugfix (even though the guidelines tell you not to do that). The ticket was reclosed and moved back to being documenation. So you opened a new ticket. So, I closed the new ticket as a duplicate of the first one. Because it is. It's not even "kinda similar" - the patch you are proposing here is *exactly* the same as your most recent comment on #13100 (with the exception of the extra modification to ModelForm.clean(), and the null-change to ModelForm.init() ). If the idea wasn't being accepted there, what on earth makes you think it will be accepted the second time around?

On top of this procedural mess, this "patch" is not even remotely in the vicinity of "100% justified, all-work-done":

• You have submitted a patch as a snippet of wiki text, rather than a diff against the root of the source tree.
• Your patch doesn't contain documentation.
• Your patch doesn't contain tests.
• Your discussion of the patch doesn't address any of the design concerns raised by Joseph on #13100
• Your discussion of the patch doesn't provide any evidence that you've considered (or even investigated) the 2+ years of design work that has gone into the feature as currently implemented.

That is, your proposal is not 100% justified, and it is maybe 5% work done.

If you want us to take your suggestions seriously, the first big step is for you to take us seriously, participate in the process as it is documented, and occasionally account for the possibility that when someone disagrees with you, it's not necessarily because they're a simpering moron with a huge ego.

Closing as a duplicate. Again. As I have now told you several times, and as is *clearly documented* -- If you don't like a decision on a ticket, start a discussion on django-developers.

[Oroku Saki]

Sigh

If this is how the Django community values bug fixes, than I won't bother from now on. I have already caught a critical bug in the model validation before, and it was quickly fixed. I don't see what the difference is. Is it just because this one isn't critical. It just makes you give up a feature, so that's not important?

I'm sure that you have more than just the lack-of-process as a means for dismissing this bug fix as banter. If I owned a automotive shop, and a person brought me their manual and said "It says that the cigarette lighter gets hot, but it's not working", I wouldn't simply tear the page out of the manual and say "There, now it does just what the manual says.". The documentation was not the issue. The code was, and blithering about bureaucratic processes won't change that. I practically wrote a Wikipedia.org article about this, and your complaint is that of a lack of due diligence. I don't know how to make a diff, and I haven't really used unit testing (however, this code will not break already existing module-level tests). The code was documented exactly as needed. I just know enough to find the bugs, and I thought that might be some help to an open source community. I guess I'm wrong. I'll leave this closed instead of fighting my case, but if you change your mind and there is something I can do to help (aside from a diff or unit test), please let me know and I will do it right away.

[Russell Keith-Magee]

Replying to orokusaki:

If this is how the Django community values bug fixes, than I won't bother from now on. I have already caught a critical bug in the model validation before, and it was quickly fixed. I don't see what the difference is. Is it just because this one isn't critical. It just makes you give up a feature, so that's not important?

The difference is that this isn't a bugfix. The code is working exactly as it was designed to work.

You happen to disagree with the design. That's fine - I'm not going to claim that we're infallible in the design department. But, as I keep saying, and you seem to keep ignoring - 'IF YOU DISAGREE WITH A DECISION WE HAVE MADE, START A THREAD ON DJANGO-DEVELOPERS'.

This isn't procedural nitpicking. I'm not saying it to exercise my vocal cords, or out of some perverse love of a rulebook. Django's contribution guide is the result of many years experience in running an open source project. What we have learned is that Trac is a really crappy forum for holding design discussions. There's no guarantee that all the relevant players are reading the comments of any given ticket -- especially a new ticket. If you have an active discussion, Trac has really awful habit of losing comments when two people are writing comments at the same time. Trac discussions aren't threaded. If it turns out that there is actually more than one ticketable issue, it's impossible to split the discussion into relevant parts.

When I close your tickets and tell you to take it to the mailing list, I'm not doing it to be annoying. I'm doing it because I want to you voice your opinion somewhere where it actually has the possibility of being heard and acted upon.

If I owned a automotive shop, and a person brought me their manual and said "It says that the cigarette lighter gets hot, but it's not working", I wouldn't simply tear the page out of the manual and say "There, now it does just what the manual says.".

True. But if a person came to me and said "this cigarette lighter doesn't seem to be able to roast my Thanksgiving turkey", I would say "well, it's not supposed to", and perhaps put a note in the documentation that roasting birds wasn't an intended use of the cigarette lighter.

There is a very important difference between a bug, and something that is working as designed.

I'll leave this closed instead of fighting my case, but if you change your mind and there is something I can do to help (aside from a diff or unit test), please let me know and I will do it right away.

You mean... other than starting a discussion on django-developers? Like I've asked you to do 4 or 5 times?

Also: I would humbly suggest that if you aim to be helpful to open source, you should start by learning the tools of the trade. diff and unittest aren't obscure tools, and their use isn't specific to Django. You would be hard pressed to find a serious open source project that didn't use both in some way. Even if you never contribute to Django ever again, you would be well served to learn how to use them.

[Oroku Saki]

Replying to russellm:

Thanks for the advice. I use doc tests, but I do think I should learn to use unit tests as well for contributing to Django. As far as diff goes, I'm so accustomed to viewing diffs in Tortoise SVN / HG that I don't understand much about what's going on in the back-end (although I'd like to learn that as well because I'm not a huge GUI fan).

That said :), one last argument for the sake of clarity on my part

True. But if a person came to me and said "this cigarette lighter doesn't seem to be able to roast my Thanksgiving turkey", I would say "well, it's not supposed to", and perhaps put a note in the documentation that roasting birds wasn't an intended use of the cigarette lighter.

The difference is that your analogy is not relevant to the matter. The idea I'm pushing is not "Django should do more.". It's "Django should do what the manual says vs simply adjusting the manual to fit Django".