Set the `self.hash` to `None` in the `hashable_wrapper`

[MathieuDutSik]

Motivation

The hashable wrapper did not invalidate the hash when the view is deleted which is potentially dangerous. This is an obstacle to the implementation of has_pending_changes.

The fn flush returns true if the views is deleted. This can happen in two scenarios:

• The view is cleared
• The QueueView is used and the load_allcommend is used with a new_back_values that is empty. In both scenarios, the view is deleted from the storage.

So, there is a scenario where we could get a value of self.hash in hashable_wrapper which is not trivial while it has been cleared:

• We call clear which sets self.hash to None in the hashable_wrapper.
• We compute the hash in hashable_wrapper which resets the hash to a value that is the value of the default.
• We do a save that calls the flush of hashable_wrapper. That flush sets the stored_hash to None but it leaves the self.hash to the hash of the default.

The result of the operation is fine. However, it prevents the implementation of the has_pending_changesoperation. It can be argued that we are losing something since we are invalidating a perfectly valid hash. However, this is fine since we are very unlikely to compute a hash just after doing a clear. That scenario showed up only in tests.

Proposal

The problem is corrected.

Test Plan

The problem was revealed in another PR that implements has_pending_changes. One test has been added to the code, though this does really reveal what is going on but may be of independent interest.

Release Plan

Not relevant.

Links

• reviewer checklist

[ma2bd]

Thanks. Can we have a unit test?

[MathieuDutSik]

Thanks. Can we have a unit test?

One test has been added. However, it looks like the preexisting code was correct and I could not find a bug of the preexisting code.

[ma2bd]

One test has been added. However, it looks like the preexisting code was correct and I could not find a bug of the preexisting code.

I don't understand. Are you saying they were no bug (then why the PR?) or that you're still working on writing a test for the bug?

[MathieuDutSik]

I don't understand. Are you saying they were no bug (then why the PR?) or that you're still working on writing a test for the bug?

There is a PR because it was asked to do so. And maybe I should have refused.

The addition of the *self.hash.get_mut() = None makes the code cleaner. In particular, it makes the comparison self.stored_hash == *self.hash.get_mut() meaningful when checking if there are pending changes to write down.

The sequence of event where we see a difference in the internal working of the system is self.clear(), self.hash(), self.save(). Before the PR the result of self.hash.get_mut() is the default hash and after it is None. However, that does not change the behavior of the views.