[FIX] chunked: explicitly flush on every iteration #364
Conversation
openupgradelib/openupgrade.py
Outdated
| f"chunked method might not work as expected in Odoo {version_info[0]}. " | ||
| "Please report any issue you may find and consider bumping this warning " | ||
| "to the next version otherwise." | ||
| ) |
There was a problem hiding this comment.
How does this relate to the rest of OpenUpgrade? Would we need a similar warning for every method?
There was a problem hiding this comment.
Quoting @pedrobaeza here: #362 (comment)
Well, this is the only method where it mixes ORM. The rest is thought for handling database things, so I don't think the rest are affected.
I don't think I have enough knowledge of this tool to answer this.
I was just bitten by the fact that I tried to use this method in 16.0 and got corrupted data as a result because of this silent error 😓 My motivation is preventing other people from going through the same in future versions, in case something like this changes again.
openupgradelib/openupgrade.py
Outdated
| if version_info[0] >= 16: | ||
|
|
||
| def reset(): | ||
| records.env.invalidate_all(flush=True) |
There was a problem hiding this comment.
True is the default argument to flush. Would you consider leaving it out, as is conventional for defaults?
There was a problem hiding this comment.
It is. I wanted to make it clear that flush is happening, which is something you may not immediately realize by just reading records.env.invalidate_all(). But I can remove it and let the default play out if you think it's best
openupgradelib/openupgrade.py
Outdated
| else: | ||
| raise Exception("Not supported Odoo version for this method.") | ||
| size = core.models.PREFETCH_MAX | ||
| model = records._name | ||
| ids = records.with_context(prefetch_fields=False).ids | ||
| for i in range(0, len(ids), size): | ||
| invalidate() | ||
| reset() |
There was a problem hiding this comment.
I like having a name like invalidate as it reflects the aim more clearly and is aligned with the underlying Odoo methods it calls. Can you tell my why you changed it?
There was a problem hiding this comment.
My reasoning comes from the fact that we're not only invalidating the "read cache", we're also flushing the "to write cache"
Something like flush_and_invalidate seemed more accurate but it's long and explicit. Then I thought for each iteration a "reset" of the cache is done, hence the name.
In any case, I've extracted the invalidation/flushing to a separate method, as you suggested
openupgradelib/openupgrade.py
Outdated
|
|
||
| elif version_info[0] >= 8: | ||
|
|
||
| def reset(): |
There was a problem hiding this comment.
A version dependent shorthand method to invalidate the cache might proof to be useful outside of chunked. Would you consider moving it to openupgrade_tools.py? It might also be an aesthetic improvement, depending on your taste of course.
There was a problem hiding this comment.
Sure ;) Check out the latests changes.
ivantodorovich
force-pushed
the
fix-chunked
branch
from
8e235a6 to
1ba6a13
Compare
ivantodorovich
force-pushed
the
fix-chunked
branch
from
1ba6a13 to
e882280
Compare
Fixes: