[ADD] api_foreach: Provide api.one replacement

[lasley]

I grudgingly present api.foreach as outlined by @dreispt in odoo/odoo#13199 (comment)

• Create api method to match operation of api.one, call it foreach

Still need to add tests & make the import not dirty, just wanted to let everyone know there is a light at the end of the tunnel 😉

[dreispt]

I'm curious on how this works without ugly import statements. Does it work with the 'from odoo import api'?

[lasley]

It doesn't, and that's part of the reason why this is a WIP & not actually unit tested yet.

I'm now working out how to monkey patch it into another namespace using importlib. Anything is possible, it's just a matter of how hard you have to hit it. Mock can do it, so again, possible.

[lasley]

Well that was fairly easy, was just a matter of an attribute set in a post_load. It works too, which is always a plus 😄

(Pdb) from odoo import api
(Pdb) api.foreach
<function foreach at 0x7fd597e56938>

[lasley]

Alright, ready for review. I'll squash my disastrous commit history after 😉

[dreispt]

Great! Now you need to make sure this module is loaded first. I'm not sure if a sequence value in the manifest can go that.

[lasley]

Wouldn't any module using api.foreach just depend on api_foreach, thus guaranteeing we're loaded before it?

[bealdav]

👍 nice

[lmignon]

@lasley good idea. Nevertheless why do you try to monkey patch odoo and provides this nice functionality as an addon? IMO the right way to propose this functionality is to put it in a true python module. Addons using this decorator should only add the module in the python external dependencies.

[dreispt]

@lmignon you mean an OCA repo for a Python

[dreispt]

@lmignon sorry: an OCA repo for a Python package to extend the core? Sounds good to me.

[lmignon]

@dreispt I mean that this one should be packaged as a python package and therfore have its own repo...

[lasley]

I use some Odoo helper methods, so this would need to be rewritten to be standalone. It wouldn't be incredibly difficult though and this could be used outside of Odoo context, as shown in my test.

The aggregate method looks fairly Odoo specific though, so we would still need an Odoo addon as a glue.

[lmignon]

@lasley your python lib must depend of odoo. Therefore you can use all the methods provided by odoo

[lasley]

Ah that makes sense and would guarantee our load order. We would lose the ability to import through the pre-existing namespace though, so another explicit import would need to be added to each file before it could be used:

from odoo import api
....
@api.multi
def ....
....
@api.foreach
def ....

vs

from odoo import api
from odoo_apis import foreach
....
@api.multi
def ....
....
@api.foreach
def ....

The first will allow for a repo to simply be awk'd for /api.one/api.foreach/, but the latter will require manual editing & more of a diff between versions. Thoughts?

[lmignon]

@lasley odoo.api is not a namespace package. Therefore it's not allowed to extend it. I prefer an explicit import in place of a monkey patch. I don't see this additional import as a something negative. If we decide to declare this new decorator in a new namespace package named oca.api I see this new import as a good opportunity to promote oca since the name will be explicitly used into the code. .

[moylop260]

I like odoo addon module.

@lmignon
What is the disadvantages of use a odoo module?

[lmignon]

@moylop260 the purpose of this module is to extend the odoo api.
In no case it extend the odoo models. Therefore it's a true python lib. It's a matter of using good python practices according to what we develop.

[lasley]

@lmignon - I agree that it is good Python practice to make a lib, however doing so will rule out the use of this functionality on Odoo SaaS (and my own) due to the user needing to install additional Python depends. This is reason enough to break good practice IMO

[pedrobaeza]

I prefer this also as an Odoo module, as you have a common way of using this functionality. It has already been done in other libraries like https://github.com/OCA/server-tools/tree/8.0/datetime_formatter

[lmignon]

The datetime_formatter is an odoo addon since it extend an odoo model...
It's time to become python developpers and stop bad practice just to stay comfortable with our old habits... I'm against this monkey patch since it can be avoided if the functionality is developed in the respect of the python standard. .. I can propose a PR to transform this addon in a python package...

[moylop260]

What about have 2 options a odoo addon and a python library?
Maybe @sbidoul could help us with this focus adding a special setup to split the library.

[lmignon]

The functionality is nice and I'll not block this PR even if I disagree on the way it's provided. I'm just disappointed and frustrated to see that it's so difficult to evolve to more python standards...

[lasley]

I'm not opposed to making the change. My question is how we allow SaaS customers to use this functionality if packaged as Python lib?

[lmignon]

@lasley the same apply to all the addons that require an additional python library. We have lots of them in OCA.

[lmignon]

@lasley a good compromise could be to avoid the patch on the odoo.api package https://github.com/OCA/server-tools/pull/615/files#diff-88daa56ae55ca4f45bb5e49df762344b. IMO it's not a problem to have the decorator available from an other place than those from odoo.
from odoo.addons.for_each.api import foreach. My main concern in your proposal is this patch on the odoo.api.

[yajo]

I agree with @lmignon, this monkey-patching is not good. It can lead to weird situations, where your addon lacks the dependency but works because another database is present that installed it (you are polluting the whole odoo namespace in a multidatabase setup).

In fact I already thought about this before, but fell into the same conclusion before even writing anything: this should be a lib.

I think the good place for this would be a new repo called "OCA/ocalib" or something like that, and make it pip-installable, and release-tagged.

Then you'd simply add it to external dependencies and in your code:

import ocalib
from odoo import models

class MyModel(models.Model):
@ocalib.foreach
def _my_method(self):
   ...

Also, I would not create this @api.one replacement until you actually need it (they have been saying it will disappear for years, but there it is still).

And, given the problem is that @api.one currently is somehow unclear because it returns a list, I'd create this @api.foreach with the following logic:

• If you explicitly pass in a callable parameter, results get aggregated on it:

  @ocalib.foreach(list):
  def method1(self):
      return self.id  # Aggregates results in a list
  

• If you pass no parameter, then assert result is None is executed for each iteration:

  @ocalib.foreach()
  def method2(self):
      return self.id  # AssertionError raised
  

• It should be compatible with @api.returns somehow.

There are lots of possible ideas that would be great for that lib repo. Some of them:

• Context manager and decorator that yields a temporary environment, useful for HttpCase tests.
• Decorators/context managers for general practices, such as controlling ImportErrors:

  with ocalib.safe_import():
      import sqlalchemy
  

• Helper method to install an addon programatically (useful for some tests, when an addon can introduce a conflict if another is installed).
• Helper method to update the demo data from an addon inside a test (to fill the gap between Enable test tours only in demo instances. maintainer-tools#213 (comment) and Enable test tours only in demo instances. maintainer-tools#213 (comment)).

Helpers, helpers, helpers... Well, just some quick ideas, I think having such playground would be quite beneficial. Probably many of you have had some need in the past that could easily have become part of a library.

So, for myself, 👍 for the idea (with some improvements), but 👎 for setting it as an addon.

[lmignon]

@yajo If we go with a new lib it would be nice to start with a namespace package' 'oca'. We could start with 'oca.api' where we could add code that leverage/extend the odoo.api (foreach). We can also imagine an other lib 'oca.test' etc..

[lasley]

Bah I don't like the fact that SaaS can't be supported, but I can just install it on my SaaS so it is. @yajo raises the concern of the multi-database, which is correct and a blocker IMO. The long namespace of an addon without the monkey patch would work, but is too long for how often it would be typed.

In light of these arguments, I agree with @lmignon about the oca namespace for the package. It looks like this is available on PyPi too, so we won't have to do a py-oca or whatever.

Also, I would not create this @api.one replacement until you actually need it

My position on this is that it is confirmed as deprecated, so we can no longer use it in upgraded code. I block for usage of api.one in PRs >= v9, and include these changes in my own PRs.

I require this functionality for a rather large project in order to fix a bunch of singleton issues without creating a diff-tastrophy by adding a bunch of record loops, resorting to moving everything to private methods that are looped, or limiting functionality with ensure_one.

If you explicitly pass in a callable parameter, results get aggregated on it

Hmmm this would also allow for some interesting techniques allowing for the returning id/val dicts too. Or really whatever. I like.

It should be compatible with @api.returns somehow

To clarify if we have an api.returns defined on the method as well, it should return a Recordset instead of a list? I'm not sure how well this will play in with the abstract callable param for return?

[yajo]

My position on this is that it is confirmed as deprecated, so we can no longer use it in upgraded code.

👍

To clarify if we have an api.returns defined on the method as well, it should return a Recordset instead of a list? I'm not sure how well this will play in with the abstract callable param for return?

Yes, that's the idea. I guess if you pass list as the aggregation method, it would behave exactly like @api.one and be compatible by default. Not sure... Well, we can leave this for later if it is problematic I guess.

Indeed this oca lib would be really needed. I find myself constantly applying the same weird algorithms in lots of places, we could leverage it to make lots of cool helpers.

For instance, another problem it could solve is to check versions of python dependencies.

Or it could skip chunks of code in an addon depending on Odoo's version. It would be nice if we could even get to have 1 single branch for all Odoo versions and programatically setting the installable flag depending on version installed. Well just letting my mind fly here, but possibilities are quite a few.

[lasley]

It would be nice if we could even get to have 1 single branch for all Odoo versions

Yeah I'm not sure how we'd version effectively if we didn't have just one branch. from oca-80 import api just seems dirty, and is a maintenance issue.

I do think that we'll need to come to a determination of which Odoo versions are explicitly supported though. My vote is on the latest three versions, same as Odoo SA release schedule. Works out well for us this cycle too, because no old API.

The alternative could maybe be an introspection layer to validate Odoo versions before allowing use of certain features. This seems fairly exponential in terms of logic weight though.

we can leave this for later if it is problematic I guess.

Might be best - I'm thinking of a lot of edge cases that could crop up with this, particularly with the api.returns. Although admittedly I'm not so versed on the api.returns, so maybe I'll have a different opinion after I verify compatibility with that. Looking at the aggregate function, it seems like it's just built in.

[lasley]

This has moved to LasLabs/python-oca#1 - please feel free to review it.

I'll create an email to petition moving the repo into OCA sometime soon. If that's something we should do now instead, just let me know!

[lasley]

FYI for anyone interested, the helpers library was released here, and the proposal to move into OCA is here.

Happy coding!