Peewee: Model select() and save() behavior

[stas]

The model select() behavior acts strange on the results where specific columns were requested and afterwards the save() is called. This is concerning since it leads to the insertion of duplicated rows.

Here's an example:

In [5]: from app.models import UserModel

In [6]: UserModel.select().count()
Out[6]: 1L

In [7]: u1 = UserModel.select(UserModel.settings).first()

In [8]: u1.settings
Out[8]: {}

In [9]: u1.settings['editor'] = 'vim'

In [10]: u1.save()
Out[10]: 1

In [11]: UserModel.select().count()
Out[11]: 2L

There are two issues I see here, the first one is the fact that a partial select() returns results with no primary key attached to it (or at least a flag that this result belongs to a row in the database).

The second issue is related to the fact that save() on a model with partial results will still generate a full update/insert including all the columns, where it should be isolated to updating just the selected columns.

Please advise here, I will be happy to help with this if you agree with the concerns from above.

[stas]

Adding @alexanderad to this discussion too.

[alexanderad]

I've prepared a script with steps to reproduce here: https://gist.github.com/alexanderad/c4cb1befe576fc987b67

[coleifer]

This is not really an issue per-se. If you are explicitly selecting columns, then it's on you to explicitly select the primary key as well if you intend to be calling save().

[coleifer]

Oh, and to your second point, that is how peewee works and will not change for historical reasons. It is also how Django works. If you want to only save the "dirty fields" you can call user.save(only=user.dirty_fields).

[stas]

With all respect, I find your response oblivious.

Basically what you say is that it is user's concern to make sure the exposed API is not duplicating the data in the database. And that you are not going to address this in any way, although this is exactly what and ORM should do.

In regards to Django, it provides users with automatic fields when calling save() on specific columns, as pointed out in their docs:
https://docs.djangoproject.com/en/1.8/ref/models/instances/#specifying-which-fields-to-save

I'm sorry you took this hostile position about the raised concerns, but I feel even worse for myself and for other users that will have to continue dealing with this kind of attitude.

[coleifer]

What would you propose as a fix? I think that when a user explicitly selects particular columns, it is bad to magically insert the primary key in addition. I can see this causing a lot of confusion.

When calling save() with a partially-populated model instance, I agree that it's troublesome to just blindly try saving all fields, but changing that now could probably break a lot of people's code. Since there's an API for saving specific fields (just like Django), I don't see the problem. Django's save() takes a list named update_fields while peewee takes a list named only.

Peewee even gives you a helper property self.dirty_fields which tells you which fields were modified, so you can call .save(only=model.dirty_fields).

[stas]

There are a couple of options:

1. raise an exception on .save() if a partial model was loaded by a .select() call and only is not provided, this should be the easiest fix. I don't think a lot of code will break, since this means those users were happy with peewee overwriting their models with empty/default values or, like in our case, duplicating records. We can also do some more research on this.
2. do not allow partial calls except on .dicts() or .tuples(), anyway this is now broken (read explanation below) and might be worth a separate discussion.
3. follow Django's model, and provide a consistent API when working with partial models. By searching existing closed issues, lots of people expected peewee will correctly work with just _dirty_ fields upon .save(). This doesn't necessarily require providing the primary key, it can leverage some private API. This is also probably the safest option in terms of backwards compatibility.

As I tried to explain, in the description, this behavior is not an expected one, if you believe things should work this way, at least .save() should inform the user that he is going to create a new record out of the partial model it works with. Silently duplicating rows in the database is the least thing somebody expects to happen.

Why 2. is broken: suppose you explicitly query some columns of two models through a join, you will end up with a partial model and not the other model fields you were selecting. You will have to use either .dicts() or .tuples() to achieve the desired behavior. For example, we patched our codebase to provide an .objects() call that will return read-only objects build out of selected column values.

[coleifer]

I hear your concerns, but as I said, this is and has always been the way peewee works (or does not work, in your case). Partial selects work the same as full selects and are not a special case. Calling save() will try to save all fields, and if no PK is present an INSERT will be issued.

There is an API for saving only the dirty fields and it would be easy to add a wrapper:

def save_changes(self):
    return self.save(only=self.dirty_fields)

I know you disagree here, but in my experience maintaining this project the existing behavior seems to work well for most people.

[stas]

I hear your concerns

Apparently you don't, and it seems that I'm wasting my time trying to convince you to change a bad API that you accept as broken, but refuse to improve it.

As I said in the initial description, feel free to ping me if you changed your mind and can use some help improving this.

[coleifer]

I'm sorry you feel you're wasting your time, I certainly don't feel that way. These are good questions and concerns you've raised. I am just of a different opinion than you on what to do about it.

Regarding the select+save question: I definitely think automagically selecting the primary key is bad. If you did _not_ select the PK and try to call save(), then I think the best Peewee could do is raise an Exception. I am loth to do that, and would prefer to just document clearly what the actual behavior is.

Regarding the question about saving empty fields, I've answered this before, so it's not like I haven't thought about it. I just feel like simplicity is best. To me that means that there are minimal special cases and minimal magics. Calling save() will save everything. Calling save(only=inst.dirty_fields) can easily be used if you want it.

[tuukkamustonen]

@stas I'm simply following peewee closely now that I am using it in production (no other bindings). In my opinion, you went quite harsh here. coleifer's response time to tickets and his attitude to resolving them has been simply outstanding. He tries to keep peewee small and simple, which is understandable considering the scope of the project and him being really the only guy maintaining it.

Imo, the whole debate here is about the amount of magic we expect from an ORM. If you consider peewee's goal (to be simple and easy to understand), less magic is better. I personally would not expect PK to be returned when I explicitly told peewee to only fetch a single field. If I needed also PK, it's not that bad writing it into select().

The suggested changes would change behavior in a backwards-incompatible manner. Wouldn't that break people's implementations? That is the key problem here - not that he was against improving the library. Maybe coleifer could consider this for 3.x, but it doesn't really feel that big of an issue (if an issue at all, as peewee already provides API to do what you are after here). For 3.x or such, it is simply a matter of taste.

[stas]

@tuukkamustonen certainly my experience is different. I've had previous experience where @coleifer would refuse any help and later work on his own to provide the same functionality without any arguments. I would suggest we stop right here trying to prove somebody is being personal against @coleifer. All I care is the future and the quality of this project simply because people depend on it.

Imo, the whole debate here is about the amount of magic we expect from an ORM.

Magic has nothing to do with the fact that the exposed API is not consistent.

The suggested changes would change behavior in a backwards-incompatible manner. Wouldn't that break people's implementations? That is the key problem here - not that he was against improving the library. Maybe coleifer could consider this for 3.x, but it doesn't really feel that big of an issue (if an issue at all, as peewee already provides API to do what you are after here). For 3.x or such, it is simply a matter of taste.

You're just being nice. The author simply refused all the suggestions and you are trying to speak on his behalf. In the end, this issue is still closed.

[tuukkamustonen]

I am certainly not speaking on his behalf. Now that would be weird...