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

Context defaults in views cause massive issues by cascading #31777

Open
gdgellatly opened this issue Mar 12, 2019 · 8 comments
Open

Context defaults in views cause massive issues by cascading #31777

gdgellatly opened this issue Mar 12, 2019 · 8 comments
Labels
10.0 11.0 12.0 Framework General frontend/backend framework issues

Comments

@gdgellatly
Copy link
Contributor

Impacted versions: 12.0 - although I'm sure this is a duplicate

Steps to reproduce:
On a sales order form - amend partner_shipping_id by adding 'default_parent_id': partner_id to the existing context.
Right simple enough, new delivery addresses default to partner as parent.
Click create and edit
Type test
Click save

Current behavior:
If you are lucky - it doesn't work with an integrity error. If you are unlucky it does work, just now you have replied to a thread.
The reason is because in the contact creation logging, it is trying to send the default_parent_id to the message created after clicking save.

Expected behavior:
Discard view defaults before logging

Video/Screenshot link (optional):

@pedrobaeza
Copy link
Collaborator

The context propagation is important for other things, so it can be generally discarded. In this specific case, you are affected due to a custom context that have that side effect, but IMO you have to handle this in your own, as there's no way to know when it's good to discard the context and when not.

@gdgellatly
Copy link
Contributor Author

@pedrobaeza maybe it is maybe it isn't, but it makes zero sense to pass default keys in particular beyond creation of the actual model being created / written. It is simple to know when a default key is good. If a default_ key is set on a model, don't pass it to another model. But it isn't just a case of custom code. default_type is another problematic one used in core which ends up propogating where it shouldn't/

@pedrobaeza
Copy link
Collaborator

Well, I was talking about context propagation. Maybe as you say, default_* keys might be discarded on that propagation.

@ged-odoo what do you think about this?

@pedrobaeza pedrobaeza added Framework General frontend/backend framework issues 11.0 10.0 12.0 labels Mar 12, 2019
@intero-chz
Copy link
Contributor

And sometimes it's really hard to find such problems. I had an issue with default_line_ids and auditlog installed. I always point to the odoo guidelines on naming fields for example on StackOverlflow, which leads to lots of fields with the same names. But the context propagation is getting to deep into every ORM call, so have fun trying to find such problems, if a wizard 30 calls before the broken call was propagating a default value in context.

I really like that feature, but sometimes it's a pain in the ass.

Just my 2 cents ;-)

@ged-odoo
Copy link
Contributor

@pedrobaeza Unsuprisingly, I think that the problem is really tricky.

I remember that when we rewrote the JS views, we had soo many bugs with context propagation (in JS-land). Most of the time, we need to aggregate properly the contexts from various sources, and keep it when we switch from an action to another, but the default_ needs to be nuked.

On this topic, I think that the web client behaviour is now (mostly) correct (except for possible bugs). Now, context propagation between various ORM methods is a different beast. Intuitively, I guess that

  1. nuking the default_* keys is a big limitation. This would prevent some legitimate ways to choose default values in some cases, through indirect calls
  2. not doing it leads to totally tricky/dangerous bugs, when two models have the same name for a field (which is quite frequent I think)
  3. another idea is to remove a default_* key once it is "consumed". This may helps, but it would not solve the two previous issues.

In my opinion, I always prefer the safest/most correct solution, even if it is not the most powerful. So, I would go with number 1. But i really have not much weight on this... This was just my 2 cents :)

@gdgellatly
Copy link
Contributor Author

gdgellatly commented Mar 24, 2019

@ged-odoo perhaps another option is to optionally namespace them

e.g. default_res_partner_parent_id

there's a couple of tricks with inheritance there (e.g. product / product template) but easily enough handled.

edit: actually i see their is already a clean context function. The problem only really manifests itself on ancillary models to the main model, when some action is performed on create/write, e.g. logs, mail, attachments. I think maybe inserting that function appropriately in those models may be a short term answer.

@gdgellatly
Copy link
Contributor Author

I just got stung again by this. default_type set on creating credit notes, then affecting creating attachments.

@razvanioan
Copy link

razvanioan commented Aug 1, 2022

@gdgellatly I had the exact same idea, to namespace them, and the model name it's the best option in my opinion too

edit: or, as their initial purpose it's to pre-populate some field values in a form (client-side), I think that they should be removed from the form create post request ...

My personal bumps on this in the past two weeks:

  • migrating a v13 DB (using Odoo's migration process) to v15, I think that this is not checked / corrected during the DB upgrade, on confirming invoices, where a default_type out_invoice was configured which was causing issues with ir.attachments (exactly how you mentioned above) - I see that the field has been renamed into move_type just in order to avoid this (I guess)
  • now, when trying to create a contact partner using default parent_id to connect to a specific partner - with the mail.message parent_id field

So, as I see there's not properly fix for this default_* hell, I'll try to remove it in the create method of the res.partner ...

@api.model_create_multi
def create(self, vals_list):
    self.env.context = frozendict({k: v for k, v in self.env.context.items() if not k.startswith('default_')})
    return super(ResPartner, self).create(vals_list)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.0 11.0 12.0 Framework General frontend/backend framework issues
Projects
None yet
Development

No branches or pull requests

5 participants