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

[ADD] Add warning for related fields with store=True #98

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

lef-adhoc
Copy link
Contributor

This PR adds a warning for related fields with store=True, as it is no longer necessary to store related fields for grouping, aggregating, or sorting in Odoo 18. The warning encourages developers to remove store=True unless strictly required, optimizing performance and reducing disk space usage.

More details can be found in the related Odoo PR: odoo/odoo#127353

This PR adds a warning for related fields with `store=True`, as it is no longer necessary to store related fields for grouping, aggregating, or sorting in Odoo 18. The warning encourages developers to remove `store=True` unless strictly required, optimizing performance and reducing disk space usage.

More details can be found in the related Odoo PR: odoo/odoo#127353
@pedrobaeza
Copy link
Member

I think this should be in pylint-odoo cc @moylop260

Copy link
Contributor

@bruno-zanotti bruno-zanotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@legalsylvain
Copy link
Collaborator

I think this should be in pylint-odoo cc @moylop260

I don't get it. It can be totally valid to write a store=True related field, AFAIK.

@pedrobaeza
Copy link
Member

If not needed (talking about performance reasons), why do it?

@legalsylvain
Copy link
Collaborator

If not needed (talking about performance reasons), why do it?

I missed something. If I understand correctly : store=True (for related fields) :

  • is not needed anymore to use group by, etc...
  • is needed (sometimes, of course) for performance reason.

For me It can still be valid.

@pedrobaeza
Copy link
Member

In theory, you don't get performance penalty when not storing for that. That's why there's a warning.

@legalsylvain
Copy link
Collaborator

That's why there's a warning

I don't see new warning in the PR : odoo/odoo#127353

If you talk about the warning introduced by the current PR, #98, it is just a warning. maybe something should be done, but related="xx.xxx", store="True" is still valid.

I can not figure out how a non stored field can generate exactly the same performance as a stored field, as it is introducing an extra join in the SQL.

Let me know if I missed something.

@pedrobaeza
Copy link
Member

OK, then let's not put the warning at all if there are drawbacks removing the store.

@legalsylvain
Copy link
Collaborator

OK, then let's not put the warning at all if there are drawbacks removing the store.

Well, the documentation in https://github.com/OCA/odoo-module-migrator says :

Indicates that you should check something. There may be something to do to make the module work. For example:

19:37:55 WARNING Replaced dependency of 'account_analytic_analysis' with 'contract' (Moved to OCA/contract)

So for me, it's ok to introduce this warning or not. (matter of taste).

@desdelinux
Copy link

pylint-odoo

This is a good suggestion for odoo-module-migrator. I don't think it adds much to pylint-odoo.

cc @pedrobaeza

@pedrobaeza
Copy link
Member

Right, I understood that it was somehow mandatory with no performance penalty

@jjscarafia
Copy link
Collaborator

So? we proceed with merging?

@legalsylvain legalsylvain merged commit 2bfa7bb into OCA:master Oct 9, 2024
1 check passed
@lef-adhoc lef-adhoc deleted the 17.0-t-44123-lef branch October 9, 2024 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants