[FIX][module_auto_update] Record base addon checksum by yajo · Pull Request #948 · OCA/server-tools

yajo · 2017-08-23T11:44:51Z

Base addon is a special case: when getting updated, it is updated in a limited addons graph that only contains itself. Under such graph, no behavior modifications are respected, so no matter what we do in module_auto_update, it will get ignored in that step. This means that when we first install module_auto_update, it will record current base addon's dir hash, and it will never again be updated, so the next time your base addon is updated, the auto update will update all addons (as expected), and every next times too (not expected).

To fix that behavior, now we track the base addon status before and after running an upgrade. If it has changed, then we update its checksum.

Besides that, workarounds for odoo/odoo#18597 are removed, and a new method for knowing if an addon is being installed, updated or uninstalled is used: check the existence of latest_version key in the create/write dict. This is because Odoo will always set that field to False when removing the addon, and it will also always write to it after installing or updating, so it provides basically all the information we need to know if we have to update or remove the checksum.

@Tecnativa

Base addon is a special case: when getting updated, it is updated in a limited addons graph that only contains itself. Under such graph, no behavior modifications are respected, so no matter what we do in module_auto_update, it will get ignored in that step. This means that when we first install module_auto_update, it will record current base addon's dir hash, and it will never again be updated, so the next time your base addon is updated, the auto update will update all addons (as expected), and every next times too (not expected). To fix that behavior, now we track the base addon status before and after running an upgrade. If it has changed, then we update its checksum. Besides that, workarounds for odoo/odoo#18597 are removed, and a new method for knowing if an addon is being installed, updated or uninstalled is used: check the existence of `latest_version` key in the create/write dict. This is because Odoo will always set that field to `False` when removing the addon, and it will also always write to it after installing or updating, so it provides basically all the information we need to know if we have to update or remove the checksum.

pedrobaeza · 2017-08-23T12:05:39Z

Travis is failing

Tests adapted to new state change detection system.

yajo · 2017-08-24T08:08:03Z

Tests fixed

yajo · 2017-08-24T10:36:03Z

💚 now

pedrobaeza · 2017-08-24T13:02:48Z

+        # Update base addon checksum if its state changed
+        base.invalidate_cache()
+        if base.state != pre_state:
+            base.latest_version = base.latest_version


Why don't you directly record the checksum instead of doing this weird reassignment?

Or maybe a comment indicating why weird assignment is necessary if this is some edge case?

Explanation added.

pedrobaeza · 2017-08-24T13:03:40Z

-                for r in self:
-                    r.checksum_installed = r.checksum_dir
-            elif vals.get('state') == 'uninstalled':
+            try:


I prefer the other approach:

version = vals.get('latest_version') if version is None: return

EAFP approach is more Pythonic in instances of key/attribute errors

You need more lines and it's less readable (as you have to know what the exception means), so I prefer the other approach (and other Python gurus are saying the same lastly).

I also prefer EAFP

I don't and I won't put it on any of my code, but I won't block this time.

Do we know where vals comes from off-hand? I personally don't, but am assuming it's from a create or write method. Why would ORM restrict what vals are entered into a dictionary?

Why are you trying to get an impossible case for my code? It works very well and doesn't mean any error even on the improbable case of somebody consciously wants to ruin it, but hey, if you have a big problem with that, just leave it!

I wasn't going for ruining it, I was just pointing out that this could have unintended side-effects outside of what we think.

The if version is False: is an incredibly explicit if statement that makes me think @yajo saw something in his testing where a False-type value can be in vals but not actually be a False. Maybe this is just a "0" version number; I'm not sure.

None is a False type value though - so while we may think it won't cause issues, this is why I asked @yajo for clarification to the explicit if in #948 (comment)

Honestly at this point I'd kind of like an inline comment to the edge case that the explicit if is covering. @yajo can you clarify for us in a comment why we are being so explicit?

I wasn't trying to start a flamewar here. It's very easy:

Odoo stores False for empty fields, always, so no need to check if there's None or whatever.

If above point is true always, it's even more in current case.

What would happen if a module is defined with "version": ""? This is very unlikely, and if it ever happened, I'm pretty sure Odoo would fail or patch it with "9.0" or "10.0" automatically, but... is it even worth it testing that? IMHO not.

So, if I wanted to use LBYL, the right code would be:

# Option A, matching 100% current behavior if "latest_version" not in vals: return # Option B, not 100%, but side matches would not matter since they should theoretically never happen if vals.get("latest_version") is None: return

... but I didn't.

The fastest way to do it was as it is right now, and it uses EAFP because IMHO it's the pythonic way to do it.

However, there's one basic truth that we all have to acknowledge: Sometimes there's more than 1 way to do the same thing right. So, in case somebody else chooses a way that you wouldn't choose by yourself, but it's also a good way to get to the exact same result where the only difference is a matter of taste... why bother? Hakuna matata!

As a side note, kudos @lasley for finding out #948 (comment), a very nice addition to Python 3. I'm very eager to get it! ☺️

#948 (comment) is another sign that they have found EAFP very cluttered in code 😉 but hey, as you have said, there are several ways of doing things. You won't see me a lot doing EAFP though, not at least on that form.

lasley · 2017-08-24T16:18:03Z

-                for r in self:
-                    r.checksum_installed = r.checksum_dir
-            elif vals.get('state') == 'uninstalled':
+            try:


EAFP approach is more Pythonic in instances of key/attribute errors

lasley · 2017-08-24T16:24:25Z

+        # Update base addon checksum if its state changed
+        base.invalidate_cache()
+        if base.state != pre_state:
+            base.latest_version = base.latest_version


Or maybe a comment indicating why weird assignment is necessary if this is some edge case?

yajo · 2017-08-28T10:21:31Z

5 days passed...

The same problem that was fixed for the `base` addon in OCA#948 happened with random addons that do not depend on `module_auto_update` (a.k.a. any addon) that Odoo decided to load before that one in the graph. Now we always check for all addons if their state has changed, and make sure to trigger the udpate mechanism that stores the right value in `installed_checksum_dir` field.

The same problem that was fixed for the `base` addon in OCA#948 happened with random addons that do not depend on `module_auto_update` (a.k.a. any addon) that Odoo decided to load before that one in the graph. Now we always check for all addons if their state has changed, and make sure to trigger the udpate mechanism that stores the right value in `installed_checksum_dir` field. If you installed and uninstalled the addon right away, you'd get a ProgrammingError saying that some columns exist no more. Checks are done now using `search_read`, which lets us limit the fields being fetched, and the environment is cleared to make sure nothing fails. Also we now guess if this own addon has been uninstalled and skip further logic if so, given it would hit broken triggers otherwise as it did before.

Syncing from upstream OCA/server-tools (12.0)

yajo added the needs review label Aug 23, 2017

yajo added this to the 9.0 milestone Aug 23, 2017

yajo requested a review from lasley August 23, 2017 11:45

yajo self-assigned this Aug 23, 2017

yajo requested a review from pedrobaeza August 23, 2017 11:46

squash! [FIX][module_auto_update] Record base addon checksum

fdc5451

Tests adapted to new state change detection system.

pedrobaeza reviewed Aug 24, 2017

View reviewed changes

lasley approved these changes Aug 24, 2017

View reviewed changes

fixup! [FIX][module_auto_update] Record base addon checksum

5b3a740

pedrobaeza approved these changes Aug 25, 2017

View reviewed changes

pedrobaeza merged commit b8285b5 into OCA:9.0 Aug 28, 2017

pedrobaeza deleted the 9.0-module_auto_update-fix_base_endless_update branch August 28, 2017 10:33

yajo mentioned this pull request Aug 29, 2017

[FIX][module_auto_update] Always store changes in lower graphs #953

Merged

yajo added a commit to Tecnativa/server-tools that referenced this pull request Aug 30, 2017

[FIX][module_auto_update] Record base addon checksum (OCA#948)

31439d2

yajo mentioned this pull request Aug 30, 2017

[10.0][module_auto_update] forward port v9 updates #956

Merged

yajo added a commit to Tecnativa/server-tools that referenced this pull request Aug 30, 2017

fixup! [FIX][module_auto_update] Record base addon checksum (OCA#948)

9d0e07d

pedrobaeza pushed a commit to Tecnativa/server-tools that referenced this pull request Aug 30, 2017

[FIX][module_auto_update] Record base addon checksum (OCA#948)

1519fcb

yajo added a commit to Tecnativa/server-tools that referenced this pull request Jan 5, 2018

[FIX][module_auto_update] Record base addon checksum (OCA#948)

9ec2de1

njeudy pushed a commit to njeudy/server-tools that referenced this pull request Jan 11, 2018

[FIX][module_auto_update] Record base addon checksum (OCA#948)

8857261

sbidoul pushed a commit to acsone/server-tools that referenced this pull request Mar 22, 2018

[FIX][module_auto_update] Record base addon checksum (OCA#948)

89beb53

Angelfentanez pushed a commit to vauxoo-dev/server-tools that referenced this pull request Oct 1, 2018

[FIX][module_auto_update] Record base addon checksum (OCA#948)

9a8032e

sbidoul pushed a commit to acsone/server-tools that referenced this pull request Oct 5, 2018

[FIX][module_auto_update] Record base addon checksum (OCA#948)

036bf2b

schout-it pushed a commit to schout-it/server-tools that referenced this pull request Oct 1, 2019

[FIX][module_auto_update] Record base addon checksum (OCA#948)

10e40b2

eantones pushed a commit to nuobit/server-tools that referenced this pull request May 5, 2020

[FIX][module_auto_update] Record base addon checksum (OCA#948)

5976e72

eantones pushed a commit to nuobit/server-tools that referenced this pull request May 18, 2020

[FIX][module_auto_update] Record base addon checksum (OCA#948)

8dc1dfc

eantones pushed a commit to nuobit/server-tools that referenced this pull request May 18, 2020

[FIX][module_auto_update] Record base addon checksum (OCA#948)

cbc572f

eantones pushed a commit to nuobit/server-tools that referenced this pull request May 18, 2020

[FIX][module_auto_update] Record base addon checksum (OCA#948)

3bb0751

manengel pushed a commit to initOS/server-tools that referenced this pull request Nov 30, 2020

[FIX][module_auto_update] Record base addon checksum (OCA#948)

45db9d5

mlaitinen pushed a commit to avoinsystems/server-tools that referenced this pull request Dec 27, 2021

[FIX][module_auto_update] Record base addon checksum (OCA#948)

ea9ae22

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#948 from OCA/12.0

b7428b3

Syncing from upstream OCA/server-tools (12.0)

Uh oh!

Uh oh!

Conversation

yajo commented Aug 23, 2017

Uh oh!

pedrobaeza commented Aug 23, 2017

Uh oh!

yajo commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yajo commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yajo commented Aug 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yajo commented Aug 24, 2017 •

edited

Loading