Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different creation behavior #65

Closed
imaia opened this issue May 30, 2016 · 8 comments
Closed

Different creation behavior #65

imaia opened this issue May 30, 2016 · 8 comments

Comments

@imaia
Copy link

imaia commented May 30, 2016

If I do this:

model = Model()
model.field = x
assert model.is_dirty() == True

model = Model.objects.create(field=x)
assert model.is_dirty() == True  # raises assertion error

I believe both examples should behave like the first example.

@romgar
Copy link
Owner

romgar commented May 30, 2016

Hi @imaia,

This behaviour is expected.
The idea of django-dirty-fields is to compare in-memory field values and database ones for a given object.

In the first example, your object is only in memory and has no database record for it, so django-dirty-fields considers that he is dirty.
In the second example, you just saved the model in the database, so your in-memory object and your database record are identical, so your model is not "dirty".

The main idea of django-dirty-fields is to know if it is worth to save an object in the database. If this object has not changed since its last saving, we could economise some database queries.

model = Model.objects.create(field=x)
assert model.is_dirty() == False

model.field = x

# This save will generate a useless database query, as field value is already x
model.save()  

# To avoid this useless database query, you could have done:
if model.is_dirty():
    model.save()

# OR
model.save_dirty_fields()

@imaia
Copy link
Author

imaia commented May 30, 2016

In the second example, inside the signals, the instance is considered also not dirty. Should this happen? I use dirty-fields to update auto-generated fields that may be manually overwritten.

@romgar
Copy link
Owner

romgar commented May 30, 2016

inside the signals

Can you please be more precise ?

Can you also describe your use case a bit more ? With some code examples ?

Note that django-dirty-fields uses post_save signal to update its internal state.

@imaia
Copy link
Author

imaia commented May 30, 2016

In the example below, I show a use case. My actual use case is with an hashcode, that is updated only if certain fields are altered:

class Model(models.Model):
    fielda = models.CharField(max_length=10)
    fieldb = models.CharField(max_length=10, blank=True, null=True)

   @staticmethod
   def pre_save(instance, *args, **kwargs):
     dirty_fields = instance.get_dirty_fields()
     # only works for case2
     if 'fielda' in dirty_fields:
        if 'a' in instance.fielda:
          instance.fieldb = 'aaa'

pre_save.connect(Model, Model.pre_save)

# first case
# shouldn't both examples below work the same?
model = Model.objects.create(field='a')
assert model.fieldb == 'aaa'

# second case
model = Model()
model.fielda='a'
model.save()
assert model.fieldb == 'aaa'

@romgar
Copy link
Owner

romgar commented May 30, 2016

Thanks for this example ! I will have a look and come back to you soon.

@romgar
Copy link
Owner

romgar commented Jun 3, 2016

These 2 situations are strictly equivalent:

# first case
# works as second one
model = Model.objects.create(fielda='a')
assert model.fieldb == 'aaa'

# second case
model = Model(fielda='a')
model.save()
assert model.fieldb == 'aaa'

The difference is in Model initialisation.
django-dirty-fields has some logic to initialise its first original state in __init__ method.
Having Model(field='a') means that it's original state is {field:'a'}, while calling Model() initialise its original state to {field:''}, so the 2 different situations are not equals, which doesn't make that much sense.
I will think about a solution, thanks a lot for pointing it out!

romgar added a commit that referenced this issue Jun 3, 2016
@romgar
Copy link
Owner

romgar commented Jun 19, 2016

Hey @imaia !

I have worked on it and added a more consistent behavior for model instances not yet saved in the database.
It will affect your pre_save logic, and the result will be what you expected.
You can have a look at init_issue_for_dirty_fields branch to test it in your project.

I will release this change in a major version release (probably 1.0), as it breaks compatibility for people using get_dirty_fields method on a non-yet-saved model instance.

@imaia
Copy link
Author

imaia commented Jun 20, 2016

Wonderful! I'll take a look as soon as I can.

@imaia imaia closed this as completed Jun 20, 2016
romgar added a commit that referenced this issue Jun 26, 2016
Add consistent behavior for initial model state (before being saved in the database).
Fix #65, but add some backward compatibility.
This change will be released in version `1.0`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants