Models not enforcing required fields

[Tom Christensen <pavera@…>]

Creating an instance of a model with CharFields will fill DB fields with single quotes ('') if the CharField is not populated.

class Person(models.Model):
  first_name = models.CharField(max_length=50)
  last_name = models.CharField(max_length=50)

person = Person()
person.first_name
''
person.save()

Resulting DB record: first_name = '', last_name = ''

Using PostgreSQL 8.1 as the DB server... psycopg2 as the driver. I'm running the latest SVN just updated this morning.

[Alex Gaynor]

It is just returning an empty string, to this best of my knowledge this is because by default blank and null are both set to false, therefore it can't actually be empty.

[Tom Christensen <pavera@…>]

Replying to Alex:

It is just returning an empty string, to this best of my knowledge this is because by default blank and null are both set to false, therefore it can't actually be empty.

Well this is a rather large change in behavior from .96, I would think it should be in the docs. Previously blank=False, and null=False meant that the field was required and if you tried to save it it would attempt to save null to the DB which would result in an error because of the not null constraint on the DB.

What you are saying is I should put null=True so that the field can hold a null value by default, but that will get rid of the not null constraint on the DB, and I will get null's in the db instead of proper "required field" enforcement.

I edited django.db.models.fields.init and added a empty_strings_allowed = False at the top of the CharField class... this restores the previous behavior, however I'm sure it breaks a million other things I haven't found yet.

[Alex Gaynor]

Hrm, you are correct, that should be db constrained, somehow I missed that.

[Tom Christensen <pavera@…>]

so, do you think my change will break a bunch of other stuff? I haven't run into any other problems yet with the change applied.. Maybe you can think of a minefield waiting for me though?

[Malcolm Tredinnick]

The comments so far are debugging the wrong problem altogether. As I understand it, the issue is that the implicit blank=False here isn't being enforced. If that is indeed true, then it's a bug and we should fix it. It's a matter of why the validation for the 'blank' attribute isn't being enforced, nothing else.

However, this has nothing to with empty_strings_allowed and empty strings certainly are allowed at the database level for CharFields.

[Tom Christensen <pavera@…>]

Ok, I was confused about how the processing went. Basically the null=True/False is just a flag on whether or not to store a null in the DB, blank=True/False is the validation on the django side of whether or not the field is required?

that being the case, I question whether this was ever happening, because even in .96 the error message I get is "insert failed because it violates not null constraint at the db level". When I save() a model with blank fields where blank=False and null=False I get the db error, not a django error saying "this is a required field" or "this field cannot be left blank". Seems like that is the wrong order at least, it should enforce blank before it incurs the hit on the db by trying to insert.

I will attempt another test in .96, I will set blank=False, but null=True and attempt to save to db. I would expect that to come back with a django error saying "field x is required". Is that the expected behavior?

[Tom Christensen <pavera@…>]

ok, a little more info. With my previous change removed, if I do a model.validate() I get an error that says "this field required" for each "required" field. However, model.save() apparently doesn't care, and just saves anyway.

Seems to me that save() should validate() first and make sure its ok to save, otherwise there is quite a bit of code I need to add to my side to validate before every save.. This seems to violate the DRY principle.

The model.save() function doesn't call validate at all is this a design decision?

[Tom Christensen <pavera@…>]

I added this to the beginning of the models save() function...

def save(self, raw=False):
    error_dict = self.validate()
        if len(error_dict) > 0:
            raise validators.ValidationError([key + ": " + ";".join(error_dict[key]) for key in error_dict.keys()])

I don't know how you would want to output the error messages, since this can return a list of fields with errors, and a list of errors for each field... for me, as long as it outputs some error and doesn't save the model, that is all I need.

[Malcolm Tredinnick]

validate() does not work yet and is not intended to be used. That's why it isn't documented. Also, save() shouldn't unconditionally call validate() because they happen at different points in the processing pipeline and different error handling is needed.

Implementing the missing pieces of validate() is work in progress.

I am now beginning to understand what you are doing and it's possibly something that isn't meant to work yet. If you try to enter a blank string, e.g. in the admin interface, when blank=False do you get an error or not. If you don't see an error, it's a bug. If not, it's working as well as it should at the moment.

[Tom Christensen <pavera@…>]

In admin it does enforce the required fields, I am assuming that is being done further up the chain though (by manipulators or newforms, I'm not sure which admin uses)

That being the case, how can I help get this implemented faster? I am writing a rather large project using django, it has a large web component where the input can be validated by forms, however, there is an equally large backend piece which will be communicating directly at the model level and we need the models to validate (at least enforce required fields, although it would be nice if we could do other data integrity checking at that level as well).

If I spend the next 2 weeks just working on model validation that will probably be ok with my masters. Is there someone in charge of this piece of the code? Something I should read to get up to speed on the design of how the django project wants this to work? It seems like a rather fundamental piece, and frankly I'm pretty surprised it isn't done. I would propose changing the documentation to make it absolutely clear that models are not validated in any way. This really seems pretty huge to me from a data integrity standpoint. Someone can write code to interact directly with a model and as long as they don't violate any DB constraints, they can royally screw up the data.

What is the "django-way" for doing data validation currently? Or are all django projects just living on the edge hoping someone doesn't write code to directly interact with their models?

[Tom Christensen <pavera@…>]

That being said, for the time being I'm leaving my change to models.save() in my django code, I ran my test suite on my project and everything checks out for *my project* with the change in there. I need to keep my data safe, if it breaks other things I'll deal with that bridge when I get there, its more important to me that I don't end up with a DB full of records with *required* fields missing.

[Brian Rosner]

I am unsure where the problem actually is. I tested your example in both the latest revision of trunk and 0.96 and person.first_name is defaulted to an empty string and able to be saved to the database. Your model definitions may have differed from the actual database fields. Since I am not sure how data is being passed to the model in your system (in most cases is through newforms) that your model definition would be raising a ValidationError before being committed to the database. blank is not used by the model, but only by form handling. By default blank is False. Can you please provide some more specific examples of how this is failing? It sounds like, if anything, an admin issue, but if it were would have been much more of an issue in the past.

[pavera]

The problem is there is no way to enforce a "required" field at the model level. I have certain model fields that I don't want null to be allowed and I don't want the empty string to be allowed either. They are required fields, they have to have data in them. I have 2 interfaces a web interface (which uses newforms and is validated through the form validation), and an API interface which is designed for mass automated updates from scripts and other backend systems communicating in code. I wanted to make model fields "required" IE, not blank, not null, and have that be enforced at the point of attempting to .save() the model (or preferably a model.is_valid() call). I have since been made aware that this is not supported at all in django, so I re-wrote my API model to use forms behind the scenes and bind the data being submitted through the API to a newforms form, validate it there, and then save to the models from the form data. I don't know if that is the "right" way to do this, but it works and that is how I dealt with this shortcoming in the models themselves.

I spent about a week though trying to figure out how to enforce a "required" field on the model itself before posting this as a bug, this can probably be closed. I understand someone is working on a new model validation system which should support the functionality I wanted, for now, the forms method seems to be working fine as well.

[Brian Rosner]

To my understanding and how I feel is that models will never do this type of validation. The form layer of abstraction is where validation of data should happen whether it come from a web form or an API you are providing. Model aware validation, to me, would be things that have to be enforced at the database layer. Such as unique and unique_together. Your use of newforms is a great example of how it should be used in other places than just forms. I'll leave it to a core dev to correct me or close this ticket.

[James Bennett]

If it's working at the level of forms -- e.g., in the admin -- then this is something that's not really a bug, because Django models do not, at the moment, have the ability to validate themselves without help from a form and are not, at the moment, meant to have that ability (though as Malcolm has pointed out it's an eventual goal). So I'm closing this invalid for now.