On Sun, 2007-04-22 at 04:25 +0000, Collin Grady wrote: > I've been looking at save() recently for work on > http://code.djangoproject.com/ticket/4102 > and I noticed what seems to be a problem in the primary key logic, but > I'm not entirely sure :) > > In the block for saving an existing model object, it uses the non_pks > list which is generated by testing for the primary_key flag on fields, > however the block for inserting a new row tests if the field is an > AutoField, not if it has primary_key set.
I think you might be getting two things confused here, or, at least, I'm not really sure that there is a problem. The non_pks list is necessary in the first block because we can't differentiate between a primary key field that has been set manually and one that has been set because the object is already saved in the database (and we retrieved the pk value from the database). In the second block, when the primary key hasn't been set or we otherwise know this is a new record (that second part is important), we want to update/insert all non-auto fields, so the list filtration looks correct to me. If you don't specify a manual primary key field, then it may or may not be an error. Your database may still have a serial value on that field, for example, or there may be a trigger that does the value computation. Both of these are a little unrealistic, but not out of the bounds of possibility (because people do some pretty weird stuff with databases and then complain when you take away their toys). The salient point is that save() method does not do validation. This is important to realise. Trying to save a model with a required manually-set primary key specified is a validation error -- it might result in a database integrity error (e.g. inserting no value into a non-NULL field), but that should be caught when validating the data or constructing the model in the first place. This will be more explicit when we also have model-aware validation (when validating manual primary keys in a good way becomes something we can solve), but the principle still holds at the moment. In the future you should be able to call something like my_model.validate() prior to save() to check all that logic explicitly. > Wouldn't this cause it to insert the primary key field regardless of > if you'd specified it or not? > > As I think about it, a non-auto primary_key probably has to be > specified in most situations, but it seems like it should still > check :) Validation is not the save() method's responsibility because it's not always obvious what the right answer is. Took me a while to wrap my brain around that when I starting working in that part of the code, but it's an idea that works quite well when we apply it consistently. > I'm also curious for thoughts on an issue with the ticket I mentioned > above; should the specified list of fields be used for inserting as > well as updating? And if it should, should the pk be used if specified > in the model regardless, or should you have to specify it in the field > list? (or should I move this question to the ticket itself?) There's going to be some interesting validation issues in that case. You'll be able to save a model with only some of the fields set correctly. I'm not sure that's a good idea. It's also a mark against the whole concept, in some respects. As a meta-issue... Thinking about that ticket a bit more yesterday evening and just now, I'm becoming less enthusiastic about the idea suggested there. I'd like to see some real-world timing examples to show it's really more efficient. It's not universally true that not updating the same fields won't lead to problems, because the two functions could have overlapping WHERE clause constraints (or the WHERE clause of one could overlap the updated columns of the other). So that part of the justification doesn't support the inclusion of the patch. None of the databases we support widely have column-level locking, as far as I'm aware (it wouldn't make a lot of sense under the covers). What speed-ups are you seeing on what sort of use-cases with this change? Regards, Malcolm --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~----------~----~----~----~------~----~------~--~---
