[18.0][IMP] queue_job: Improve context management.

[amh-mw]

When the `queue_job_keep_context` context variable is set, always
preserve the entire context. This honors the principle of least
surprise, in that a developer can easily convert a record.method()
call to record.with_delay().method() with the expectation that it will
actually execute the same, simply at a later time.

When the `queue_job_context_keys` context variable is set, preserve
the given keys in addition to those normally specified by
`_job_prepare_context_before_enqueue_keys`.

Fixes #406. Formerly #613.

[OCA-git-bot]

Hi @guewen, some modules you are maintaining are being modified, check this out!

[hildickethan]

Maybe it would be better as a parameter to the with_delay() itself so you can choose to keep the full context on a job by job basis rather than globally, something like using with_delay(keep_full_context=True) The original reasoning for not always preserving context is here

[amh-mw]

Maybe it would be better as a parameter to the with_delay() ... something like using with_delay(keep_full_context=True)

@hildickethan I like this idea in principle, but haven't had enough coffee this morning to see how to pass keep_full_context from Base.with_delay -> DelayableRecordset -> ??? -> JobSerialized.convert_* -> JobEncoder.default.

[dannyadair]

What about things that can't be serialized? That would at least be the asterisk on "entire".

I feel like the moment you call with_delay you generally know what context keys you could be interested in (or intentionally want to ignore!). Preserving the entire* context is all or nothing. Maybe it would be more flexible to allow passing a dictionary (with_context= ?), and the job context gets updated with that dictionary. Then if you want you can just pass the context itself to preserve it as-is.

[amh-mw]

What about things that can't be serialized? That would at least be the asterisk on "entire".

Absolutely. That has been raised before in https://github.com/OCA/queue/pull/432. Personally, I have never encountered a non-serializable context across the few dozen Odoo and OCA modules that I use, so... I didn't code around it.

I feel like the moment you call with_delay you generally know what context keys you could be interested in (or intentionally want to ignore!). Preserving the entire* context is all or nothing. Maybe it would be more flexible to allow passing a dictionary (with_context= ?), and the job context gets updated with that dictionary. Then if you want you can just pass the context itself to preserve it as-is.

This approach isn't a great fit for me personally, as I have a large number of context variables that I set at the top of large import jobs (per https://github.com/OCA/queue/pull/613#issuecomment-1866983992), and my actual with_delay calls are nested arbitrarily deep (and sometimes recursively) in my code.

That said, with_context mirrors the familiar Odoo Python self.env[model].with_context(...) idiom, so walking that out a little bit, couldn't a developer rely on the core Odoo context manipulation mechanism to do something like:

# addition
self.env[model].with_context(a=1, b=2).with_delay(keep_full_context=True).func()
# replacement
self.env[model].with_context({'c': 3, 'd': 4}).with_delay(keep_full_context=True).func()

Then the queue_job module doesn't gain context manipulation complexity at all and just relies on core Odoo capabilities. My only concern then is how to play nicely with _job_prepare_context_before_enqueue and its default keys ("tz", "lang", "allowed_company_ids", "force_company", "active_test")

[amh-mw]

a new field that gets passed through is overkill, I think it would be much simpler if that itself was just in the context :)

Great suggestion. Give me a bit and let me see what I can work up.

[dannyadair]

to see how to pass keep_full_context from Base.with_delay -> DelayableRecordset -> ??? -> JobSerialized.convert_* -> JobEncoder.default.

Hmm looking at the "how" I think

allow passing a dictionary

a new field that gets passed through is overkill, I think it would be much simpler if that itself was just in the context :)

Currently:

    def _job_prepare_context_before_enqueue(self):
        """Return the context to store in the jobs
        Can be used to keep only safe keys.
        """
        return {
            key: value
            for key, value in self.env.context.items()
            if key in self._job_prepare_context_before_enqueue_keys()
        }

So this sets the context dictionary for the job from the current context, but only the key that are in the allow list. This could be adjusted to look for a context key that "does more". Could be queue_job_context dictionary that updates that little default one. Maybe

    def _job_prepare_context_before_enqueue(self): 
      """Return the context to store in the jobs
      Can be used to keep only safe keys.
      """
      context = {
          key: value
          for key, value in self.env.context.items()
          if key in self._job_prepare_context_before_enqueue_keys()
      }

      if "queue_job_context" in self.env.context:
          context.update(self.env.context["queue_job_context"])

      return context

then you could just

record.with_context(
    queue_job_context=dict(mykey=self.env.context["mykey"])
  ).with_delay().method()

or for the full context

record.with_context(queue_job_context=self.env.context.copy()).with_delay().method()

(or set it on record = record.with_context() once to do that for multiple jobs)

[dannyadair]

a new field that gets passed through is overkill, I think it would be much simpler if that itself was just in the context :)

Great suggestion. Give me a bit and let me see what I can work up.

Sorry somehow I posted my comment while editing. Please see updated version, I don't think "preserve entire context" needs its own setting - just pass some or copy all.

[dannyadair]

if "queue_job_context" in self.env.context:
          context.update(self.env.context["queue_job_context"])

maybe better

context.update(self.env.context.get("queue_job_context") or {})

so you can explicitly set it to something falsy to disable it if it might have been set before (otherwise you would have to make it an empty dictionary, this way None works just as well)

[amh-mw]

I don't think "preserve entire context" needs its own setting - just pass some or copy all.

It's a requirement for me. I have far too many context variables to copy paste them across with_delay invocations spread across modules. This becomes even worse when considering that I run different combinations of modules for different customers. I don't have the context serialization problem and want the entire context every time.

[dannyadair]

I don't think "preserve entire context" needs its own setting - just pass some or copy all.

It's a requirement for me. I have far too many context variables to copy paste them across with_delay invocations spread across modules. This becomes even worse when considering that I run different combinations of modules for different customers. I don't have the context serialization problem and want the entire context every time.

Gotcha. If there's a performance issue an assignment would be better than a .copy(). How about:

    def _job_prepare_context_before_enqueue(self): 
      """Return the context to store in the jobs
      Can be used to keep only safe keys.
      """
      if self.env.context.get("queue_job_preserve_context"):
          context = self.env.context
      else:
          context = {
              key: value
              for key, value in self.env.context.items()
              if key in self._job_prepare_context_before_enqueue_keys()
          }

      context.update(self.env.context.get("queue_job_context") or {})

      return context

Like this?

the entire context every time

This way would still require inserting something like record = record.with_context(queue_job_preserve_context) but it would be more explicit/conscious than a global database setting. Other people's modules creating queue jobs may see unwanted side effects.

[dannyadair]

I have far too many context variables to copy paste them across with_delay invocations spread across modules

If there's a performance issue an assignment would be better than a .copy().

Sorry maybe I misunderstood. You won't have to copy individual context variables if you set .with_context(queue_job_context=self.env.context.copy() If those are just flags and other small objects it should be fine but if your context is large an(other) assignment may affect performance (the job already has to serialize everything).

[dannyadair]

   keys = self.env.context.get("queue_job_context_keys")

oh good - I wasn't sure if that was possible. We've had some weird side effects depending on where context gets set. I really have to look more into @api.model I keep thinking @classmethod

[amh-mw]

I chose to remain out of the context modification game, so adding and using a new context value looks like

self.with_context(a=1, queue_job_context_keys=['a']).with_delay().func()

I dislike the repetition, but I think it's preferable to reimplementing with_context for queue_job.

[dannyadair]

I chose to remain out of the context modification game, so adding and using a new context value looks like

self.with_context(a=1, queue_job_context_keys=['a']).with_delay().func()

I dislike the repetition, but I think it's preferable to reimplementing with_context for queue_job.

I think that's good to differentiate and be explicit. The context is often set somewhere else. I would expect queue_job_context_keys to only be set right before with_delay() to bring in what's needed.