Summary

Fix bug where ModificationTimestampField would not automatically update its value.

Review Request #13261 — Created Sept. 6, 2023 and submitted Sept. 28, 2023, 3:52 p.m.

Information

Owner

maubin

Repository

Djblets

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

As discussed in /r/13260, it was discovered that the
ModificationTimestampField had a bug where sometimes the field wouldn't
automatically update to a new value when it was supposed to. Review Board uses
this field for its "last updated" timestamp on review request and drafts, and
this timestamp sometimes wasn't being updated when we would save changes on a
review request or draft. Specifically, this would happen whenever we tried to
save changes on objects that were fetched through a model manager (i.e. objects
from Model.objects).

ModificationTimeStampField takes care to not overwrite manually set values by
tracking modification state in its __set__ method. If __set__ was called,
then we assume that the field value was manually set and we give the field a
modified state. But when an object is fetched through a manager, the
__set__ method for the field gets called when creating the object instance.
This makes the ModificationTimestampField think that the value has been
manually set, even though it hasn't been. Previously we were using a
first_set state to handle the case when __set__ is called during object
initialization, but this wasn't hooked up correctly and didn't actually work.
This change addresses the case by checking if the field value actually exists
in the object's attributes dictionary, and if not then we know that the field
is being set for the first time during object creation and its not being
manually set. We also add unit tests for this case.

Testing Done


Ran unit tests.
Ran unit tests (including my newly added ones) without the fix to

ModificationTimestampField, saw that the cases where we fetch objects

 through a model manager and save those would not properly update the

 timestamp.
Used in a Review Board change.

Commits

Summary	ID
Fix the ModificationTimestampField	b4c0892169016f91d9feec869ea910047b8eba52

Issues

Description	From	Last Updated
Can you add some more context in the summary?	chipx86	Sept. 26, 2023, 10:53 a.m.
We can just check against __dict__, which is faster as it'll check by hash, whereas checking within .keys() will require …	chipx86	Sept. 11, 2023, 8:32 a.m.
Can you add -> None: to new test methods, so we opt into typing?	chipx86	Sept. 11, 2023, 8:32 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you add some more context in the summary?

chipx86 Sept. 12, 2023, 2:42 a.m.

The new description looks good, but the summary's a bit vague still. Can we fit in the condition/symptom that was fixed?

maubin Sept. 20, 2023, 12:12 p.m.

Oh woops, totally missed that you were talking about the summary and not the description. I'll update the summary :).

djblets/db/fields/modification_timestamp_field.py (Diff revision 1)

The issue has been resolved. Show all issues

We can just check against __dict__, which is faster as it'll check by hash, whereas checking within .keys() will require iterating through all keys one-by-one.

djblets/db/tests/test_modification_timestamp_field.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add -> None: to new test methods, so we opt into typing?
```

Description:

		As discussed in /r/13260, it was discovered that the
		`ModificationTimestampField` had a bug where sometimes the field wouldn't
~		automatically update to a new value when it was supposed to. This would happen
~		because for some reason, the `__set__` method for a field on a model gets
~		called when fetching model instance through their model manager. This makes
~		`ModificationTimestampField` think that the value is being manually set, so it
~		won't overwrite it. This change fixes that and adds unit tests for this case.
	~	automatically update to a new value when it was supposed to. Review Board uses
	~	this field for its "last updated" timestamp on review request and drafts, and
	~	this timestamp sometimes wasn't being updated when we would save changes on a
	~	review request or draft. Specifically, this would happen whenever we tried to
	~	save changes on objects that were fetched through a model manager (i.e. objects
	+	from `Model.objects`).
	+
	+	`ModificationTimeStampField` takes care to not overwrite manually set values.
	+	But when an object is fetched through a manager, the fields of the object
	+	get manually set when creating the object instance. So this makes the
	+	`ModificationTimestampField` think that its value is being manually set, so it
	+	won't overwrite values on objects fetched through a manager, even when they
	+	are not actually being manually set. This change fixes
	+	that and adds unit tests for this case.

Testing Done:

		Ran unit tests.
~		Ran unit tests (including my newly added ones) without the fix to `ModificationTimestampField`, saw that the cases of fetching from a model manager would fail.
	~	Ran unit tests (including my newly added ones) without the fix to `ModificationTimestampField`, saw that the cases where we fetch objects through a model manager and save those would not properly update the timestamp.
		Used in a Review Board change.

Commits:

	Summary	ID
	Fix the ModificationTimestampField	bd0b16872af1097f16e26be0cb2e2230129f675b
	Fix the ModificationTimestampField	4f08f86676da268fb2bd35e56be9931cf3b1fc14

Diff:

Revision 2 (+126 -10)

Show changes

	djblets/db/fields/modification_timestamp_field.py
	djblets/db/tests/test_modification_timestamp_field.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Change Summary:


Got rid of the first_set state since we don't actually use it or need it anymore.
Hopefully clarified things in the description.
Updated some docs for better clarification too.

Description:

		As discussed in /r/13260, it was discovered that the
		`ModificationTimestampField` had a bug where sometimes the field wouldn't
		automatically update to a new value when it was supposed to. Review Board uses
		this field for its "last updated" timestamp on review request and drafts, and
		this timestamp sometimes wasn't being updated when we would save changes on a
		review request or draft. Specifically, this would happen whenever we tried to
		save changes on objects that were fetched through a model manager (i.e. objects
		from `Model.objects`).

~		`ModificationTimeStampField` takes care to not overwrite manually set values.
~		But when an object is fetched through a manager, the fields of the object
~		get manually set when creating the object instance. So this makes the
~		`ModificationTimestampField` think that its value is being manually set, so it
~		won't overwrite values on objects fetched through a manager, even when they
~		are not actually being manually set. This change fixes
~		that and adds unit tests for this case.
	~	`ModificationTimeStampField` takes care to not overwrite manually set values by
	~	tracking modification state in its `__set__` method. If `__set__` was called,
	~	then we assume that the field value was manually set and we give the field a
	~	`modified` state. But when an object is fetched through a manager, the
	~	`__set__` method for the field gets called when creating the object instance.
	~	This makes the `ModificationTimestampField` think that the value has been
	~	manually set, even though it hasn't been. Previously we were using a
	+	`first_set` state to handle the case when `__set__` is called during object
	+	initialization, but this wasn't hooked up correctly and didn't actually work.
	+	This change addresses the case by checking if the field value actually exists
	+	in the object's attributes dictionary, and if not then we know that the field
	+	is being set for the first time during object creation and its not being
	+	manually set. We also add unit tests for this case.

Commits:

	Summary	ID
	Fix the ModificationTimestampField	4f08f86676da268fb2bd35e56be9931cf3b1fc14
	Fix the ModificationTimestampField	b4c0892169016f91d9feec869ea910047b8eba52

Diff:

Revision 3 (+134 -36)

Show changes

	djblets/db/fields/modification_timestamp_field.py
	djblets/db/tests/test_modification_timestamp_field.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Summary:

-	Fix the ModificationTimestampField
+	Fix bug where ModificationTimestampField would not automatically update its value.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (76ded09)