[13.0][IMP] queue_job: use context to execute job (optional)

[acsonefho]

In some case, we need to keep the context to execute the job.
On the with_delay(...), add a parameter keep_context (default True) to know if the context should be saved on the job.
The keep_context parameter could be:

• False: do not use context;
• True(default value): use full context;
• list of key: keep only given keys into the context.

Je context is saved on the JobEncoder and then restored with JobDecoder.

Depends on #365 (about company)

[guewen]

I didn't see this one before letting a comment on #407, my comment should actually be here.

In #283 I proposed to store the context in the recordset instead of a new field.
It means we could extend the support of some context keys for records passed in args and kwargs as well, and I think the code is ever simpler. Do you think it would be possible to do this modification?

[guewen]

BTW, this is a long-awaited feature I think, so thank you to finally propose a PR :)

[acsonefho]

In #283 I proposed to store the context in the recordset instead of a new field.

@guewen Sorry I completely miss this possibility. And I think it's a good idea. I'll edit this PR today or tomorrow.

[rousseldenis]

@acsonefho

[acsonefho]

I didn't see this one before letting a comment on #407, my comment should actually be here.

In #283 I proposed to store the context in the recordset instead of a new field. It means we could extend the support of some context keys for records passed in args and kwargs as well, and I think the code is ever simpler. Do you think it would be possible to do this modification?

@guewen Sorry for the (long) delay! I had some day off and a lot of work like everybody!
I can not save the context in the recordset itself.
When the job is created, the recordset still have the context (in recordset.env.context). But when the job is triggered, the context is lost (that's why this PR exists).

I have to use a dedicated field to then restore it when the recordset is restored.

Or maybe I miss understand something?

[guewen]

Hi @acsonefho, no worries!

If you take a job signature, for instance

class SaleOrder(models.Model):
    _inherit = 'sale.order'

    def do_random_stuff(self, partners, when=None):
        # ...

With self being a record (recordset) of sale.order, partners another recordset of several partners and when a datetime.

When the queue.job record is created, self is stored in a single JSON field containing all its "properties" (model, id, user id, su flag) but the context (actually it is a list since self can contain several records).
The args (here partners) is stored in a JSON (but as a JSON list) field named args, with the model, id, user id, su flag properties too.
The kwargs is encoded as JSON exactly the same way in kwargs, but as a JSON dictionary.

By adding an encoder for the context in the JobEncoder and decoder, the context will automatically be stored in the existing JSON queue.job fields without adding an extra field, then "injected" into the decoded recordset. And it means, it will magically work for recordsets passed in args and kwargs too, not only for the "main record".

The main thing to take care of, is that not everything can be encoded as JSON and the context can be a bit too liberal about this. We definitely want an allow-list for the context keys.
I'd say we can add a new argument to the queue.job fields:

    records = JobSerialized(
        string="Record(s)", readonly=True, base_type=models.BaseModel,
        allowed_context_keys="_serialized_allowed_context_keys",
    )
    args = JobSerialized(readonly=True, base_type=tuple, allowed_context_keys="_serialized_allowed_context_keys")
    kwargs = JobSerialized(readonly=True, base_type=dict, allowed_context_keys="_serialized_allowed_context_keys")

    def _serialized_allowed_context_keys(self):
        return ["tz", "lang", ...]  # I don't remember what should really be there

Then, in the implementation of the job encoder, filter from this list.

Is my explanation better this time ? :)

[florian-dacosta]

Also, I don't see why we would not want the context ?
At least, the context should be kept by default IMO (for now, unless I am mistaken, we have to specifically set keep_context to True when we delay a job)

[guewen]

I agree with @florian-dacosta , we can keep the context in any case IMO especially as we filter the context keys, and we can still customize the keys to filter by overriding the method that returns the list of keys to keep.

[acsonefho]

Is my explanation better this time ? :)

Yes! Thank you very much! I'll improve in this way so!

[francesco-ooops]

@rousseldenis @guewen is this ready for merge?

[GSLabIt]

Hi @guewen any chance to review and merge this one? bit thx

[florian-dacosta]

On my side, I really think keeping the context should be the default (actually I would not even put it as optional)
And it would simplify the code not making it optional.

I understand the backward compatibility argument, but the goal of queue job beeing just to do some action the same way but asynchronously, i believe that passing the context is more a fix than a new feature and it seems reasonable to change this behavior by default
The method run as a job should work the exact same way when run synchronously and asynchronously and making the context propagation optional is a problem IMHO.
Of course, in this case the allowed_context_keys proposed by Guewen become mandatory to avoid issues.

[sebastienbeau]

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

[francesco-ooops]

@acsonefho any chance to update the PR?

[acsonefho]

@acsonefho any chance to update the PR?

Yes I can do this but it could break the backward compatibility.

[GSLabIt]

LGTM

[acsonefho]

@sebastienbeau @guewen Can you review please? :)

[guewen]

Hi @acsonefho, thanks, I understand your concern about backward compatibility.

My concern with your proposal is that I don't really see how we can plan a path to a version where we store the context by default (with an allow-list). The argument you added in the enqueuing of the job introduces a change in the API, which will bring its own backward compatibility issues when we will not need it anymore (as the goal is to store it by default at some point). I would prefer not to have keep_context arguments everywhere in calls (or do you see a reason to customize it for each different job call?).

Here is a draft of the way I thought it would be easier to handle the transition : acsone#1
It's much simpler, we can still override the allow-list per instance basis and when we want to break compatibility by always storing the context, we only to have to touch a single place (in the queue job AND in the implementations, which will make the migration much easier).

[guewen]

Hi @sebastienbeau @GSLabIt @florian-dacosta @francesco-ooops ,

Could you give your opinion on acsone#1 ?

[guewen]

Discussion in acsone#1

[simahawk]

#432 got merged