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

[14.0][FIX] base: Update wrong cache invalidation #1220

Open
wants to merge 1 commit into
base: 14.0
Choose a base branch
from

Conversation

sebalix
Copy link

@sebalix sebalix commented Feb 28, 2024

Avoid invalidating cache when not needed.
This improves a lot performances.

Closes odoo#143403

Fixes a bug which causes that: self.flush() and self.clear_caches() were called on every property write/update and delete. Known at least in V14/15/16/17.

Backport of odoo#145171

Related: odoo/enterprise#52207

====

NOTE: I removed the call to self.flush_model() from the commit as this method doesn't exist in 14.0 and is not directly related to the fix

Avoid invalidating cache when not needed.
This improves a lot performances.

Closes odoo#143403

Fixes a bug which causes that: self.flush() and self.clear_caches() were called on every property write/update and delete.
Known at least in V14/15/16/17.

closes odoo#145171

Related: odoo/enterprise#52207
Signed-off-by: Raphael Collet <rco@odoo.com>
@pedrobaeza
Copy link
Member

The CLA is not needed here. About the content itself, I think is very risky to apply a partial fix when the rest of the ORM contains major changes that can make this to not work properly or cause other malfunctions. If we don't have the total confirmation from someone from core ORM developers, I don't trust in being this correct.

@sebalix
Copy link
Author

sebalix commented Feb 29, 2024

@pedrobaeza as you wish. The current v14 code is clearly broken (checking the number of rows of a SELECT EXISTS will always be 1 for sure). The only reason it hasn't been backported is that v14 is not supported anymore.

  1. Regarding the removal of flush_model() call (flushing current model only), I prefer to not replace it by a call to flush() (all models), and before it wasn't there anyway so it'll continue to work as before.
  2. And the CLA, should we really remove it? IMO it's part of the commit so I let it, as well as we didn't remove the doc/cla folder from this fork

@pedrobaeza
Copy link
Member

That CLA is for Odoo, not for OCB, so it's useless. And the rest I'm just saying that is a very important change with an impact across whole Odoo, so it can't be done only backporting a piece of code without being sure that is correct, and there are no tests here in OCB to check if everything is OK, so the green you are seeing means nothing.

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

Successfully merging this pull request may close these issues.

4 participants