Draft generic context store definition

[guewen]

Proposal to store the context, thinking about a migration path to enable storing the context keys by default :

• first step : do not store any key by default, users can override _job_prepare_context_before_enqueue_keys in the base model or any model to define the keys to store in the context
• second step : maybe on the next odoo version, set the list of keys to keep

I didn't adapt the tests yet, this is mainly for discussion as of now.

[rousseldenis]

@acsonefho

[guewen]

Additional context in OCA#406 (comment)

[acsonefho]

Depending on the discussion on OCA#406 I think everyone want the full context by default.

And set which keys should be preserved for each module is quite repetitive to implement.
And you don't have possibility to set a list of keys (of the context) to keep at the job level.

I'm not in favor of this PR.

[guewen]

Hmm, I think you are mistaken.

I think everyone want the full context by default.

I'm really not sure about this, firstly because it is not safe : not everything can be serialized to json and the values stored in the context are quite liberal (even python objects sometimes), we would have errors. I would not accept to store any automatically in the context.

And set which keys should be preserved for each module is quite repetitive to implement.

You misread my PR. The set of keys is global.
However, the solution you propose in OCA#406 means we have to set keep_context in every delay of job, this is quite repetitive.

And you don't have possibility to set a list of keys (of the context) to keep at the job level.

What examples do you have where you need this?

[guewen]

@acsonefho

Another issue I have with OCA#406 is that if I have a job my_job, I have to set the context keys every time I call it : self.with_delay(keep_context=['tz', 'lang', ...]).my_job() in every places I call the job. This is a non-sense to me as we will always want the same keys for the same job method.

[guewen]

@acsonefho

If really we would like to customize the context keys per job, IMO it should be stored in the queue.job.function like we do for the related action, retry pattern etc.

A version I'd prefer is:

• no keep_context argument in jobs
• default allow-list of context keys to store as I did in this PR with the standard keys (tz, lang, allowed_company_ids, ...)
• BONUS if we need special keys for a job method: additional context keys to keep in a field in queue.job.function so for ONE job, we can choose which keys we want, and we don't have to set a keep_context additional parameter in every call

It seems to me it is simpler that OCA#406 both in term of implementation and of usage, and should cover all of your use cases.

[florian-dacosta]

IMO, like I have said already, I really don't see why we would not want the full context to be passed in case of the job.
I keep thinking it should not be optional but I understand the concern about backward compatibility.
So the solution proposed in this PR seems nice to me (no context passed by defaut : backward compatibility is ok but easy to configure globally for all jobs that the context is passed) and for version 16, configure the key we pass by default.

About the context keys passed, I believe if the full context is passed, it is better because we want our method to work the same way when it is called through a job or not... But like guewen said here, it is complicated because we could have anything in the object like python object, recordset...
So the solution with the allowed key configured globally is ok for me.
Maybe later we could manage to keep all context key that we know how to manage easily ? I mean, we could check the type and accept all string, integer, float, etc... or maybe all serializable data using some try except logic or something.

Finally about the possibility to configure the context per job instead of per model, I don't see in which case it could be needed. I am not sure it is worth worrying about this now unless someone has a use case of this kind ?

So overall I like this PR that IMO simplify things quite a lot, at least for the "standard/ordinary" case, where we always want to pass the context and we don't need to configure it per job.

[GSLabIt]

I've to agree with @guewen, i see this PR more straightforward and a simpler way to implement. So IMO, 👍🏻 to this one.

[acsonefho]

I'm really not sure about this, firstly because it is not safe : not everything can be serialized to json and the values stored in the context are quite liberal (even python objects sometimes), we would have errors. I would not accept to store any automatically in the context.

I understand the 'not safe' part. That make sense. But it's easy to work with: just keep serializable keys.

Hmm, I think you are mistaken.

I also agree with florian, it's better (and simplier) to always pass the context and thanks for the work

Into the original PR they want the context by default. So I update my PR with that.

You misread my PR. The set of keys is global.

Yes sorry, I miss read. I think it's a good idea if we always want some keys (company etc).

But anyway, I think the original PR is a good point to start and of course it should evolve.
This PR doesn't block any futur improvement.
For now on set on the job level the keep_context (True (default)/False/List of keys).

[guewen]

But anyway, I think the original PR is a good point to start and of course it should evolve.
This PR doesn't block any futur improvement.
For now on set on the job level the

On the contrary and that's exactly what I try to explain : by changing the API and asking devs to add the 'keep_context' arg in their calls to with_delay, it means we'll either have to keep it or if we remove it in favor of another solution (which would be the good thing to do as this is not the correct place to have this parameter, as the context keys to keep will always be identical for a job method or even for all jobs, we should never ever have to set this in with_delay), we break the contract of the API and create more migration work for later.

Furthermore, I'm sorry but the complexity added in job.py "just" for storing the context seems a bit too much for the requirement.

This is even more true if we magically sanitize the context by keeping only serializable values (not sure it is that easy and performant but this is another story).

Could you explain to me the uses cases where you would need to set different parameters in your keep_context and that you couldn't handle with a global method as I proposed?

[guewen]

I understand you already invested a lot of time on this and probably have implementations relying on this and I ask for important changes.
The thing is I'm trying to find what's best for the long term. I'm happy to help on the changes I think necessary.
There is one thing though : I keep asking for the use cases to understand why you took the decisions you took. Without that, you should understand that I can't see through your position.

[guewen]

@acsonefho I realized that the option keep_context is an issue as well for recordsets passed in args and kwargs, as they are not handled by your PR.
It would not necessarily make sense to apply the same value of the keep_context argument to the recordset in self or in args/kwargs, so it even further more convince me that a simple solution with a global allow-list is better.
I opened OCA#432 in draft, I'd like to know if you have any cases where that would not work.