erp5_catalog: Do not use lazy_class objects while generating UIDs

Earlier, class for any Catalog object used to be Products.ZSQLCatalog.SQLCatalog,
but now as we have shifted SQLCatalog to ERP5Catalog, the catalog objects have
lazy_class 'erp5.portal_type.Catalog' as their 1st class in mro. This class
don't have the required attributes.

The lazy_class is dynamic, so its not possible for them to save reserved IDs
as their attributes. So, we use the next class in mro, which would be
class 'Products.ERP5Catalog.ERP5Catalog' and save attributes in it.

Notice that this change of 'self.__klass__' is not required if
we are starting a lock with '_reserved_uid_lock' and then acquire-release it
consicutively in one go(transaction).

product/ZSQLCatalog/SQLCatalog.py

@@ -1183,6 +1183,10 @@ class Catalog(Folder,
   security.declarePrivate('getUIDBuffer')
   def getUIDBuffer(self, force_new_buffer=False):
     klass = self.__class__
+    if klass.__module__ == 'erp5.portal_type':
+      # For lazy_class object, take the class from mro, rather than relying
+      # on lazy_class object
+      klass = self.__class__.mro()[1]
     assert klass._reserved_uid_lock.locked()
     assert getattr(self, 'aq_base', None) is not None
     instance_key = self.getPhysicalPath()
@@ -1201,6 +1205,10 @@ class Catalog(Folder,
       Produces reserved uids in advance
     """
     klass = self.__class__
+    if klass.__module__ == 'erp5.portal_type':
+      # For lazy_class object, take the class from mro, rather than relying
+      # on lazy_class object
+      klass = self.__class__.mro()[1]
     assert klass._reserved_uid_lock.locked()
     # This checks if the list of local reserved uids was cleared after clearReserved
     # had been called.
@@ -1293,6 +1301,10 @@ class Catalog(Folder,
       return None
 
     klass = self.__class__
+    if klass.__module__ == 'erp5.portal_type':
+      # For lazy_class object, take the class from mro, rather than relying
+      # on lazy_class object
+      klass = self.__class__.mro()[1]
     try:
       klass._reserved_uid_lock.acquire()
       self.produceUid()

[Vincent Pelletier]

I think this commit message do not match code change chronology.

It is not fully clear to me though: ERP5Catalog is the name of a product we use for years, so "now that we have switched to ERP5Catalog" seem odd. But I guess you mean ERP5 Catalog, aka the catalog being an instance of a portal type. Which it is not in this code change chronology.

[EDIT]: I was wrong about commit order. For some reason my browser mixed up tab order.

On a python level, what about moving the lock from an instance property to a module global ? I believe the effect will be the same and code will not need such mro introspection.

/cc @jm

[Julien Muchembled]

First of all, commit messages aren't the place to comment the current code, as if it has never been different. On the other side, your comment in the code is too short. You should move some explanation from the commit message to the code. And do not repeat it elsewhere: you can have a single comment see comment in ....

But even with the commit message, I don't understand the change. Why the class would not have the required attribute ?

[Julien Muchembled]

Hmm, maybe getting the exact class is only useful for _local_clear_reserved_time, which is the only setattr access. Elsewhere, the klass = self.__class__ don't even seem useful.

[Ayush Tiwari]

Thanks for suggestion. I would keep that in check.

For the explanation, the class erp5.portal_type.Catalog is a dynamic class which don't have the needed attributes. Also, here in general while generating the UIDs, we are using attributes from the class and not from the instances of the class. This is why I had to use this hack.

[Ayush Tiwari]

It is used for _reserved_uid_lock also.

[Julien Muchembled]

the class erp5.portal_type.Catalog is a dynamic class which don't have the needed attributes

That's what you already wrote in the commit message and I am asking why. Lazy classes were written to be as transparent as possible. I mean that they are turned into "normal" classes on most accesses, and mro should be automatic.

[Ayush Tiwari]

I agree that lazy classes act as normal classes. But here in our case, we make change on class attribute and not on the instance attribute. So, if we consider that we just take the class of the object in use(i.e, SQLCatalog), it would be erp5.portal_type.Catalog. So, even though we can have the attribute, the changes we make in it don't update the change for SQLCatalog class(which is what is required here). Thus, this approach to neglect the first class in the mro.

[Julien Muchembled]

So do you agree it's only for setattr accesses like _local_clear_reserved_time and for nothing else ?

[Ayush Tiwari]

Yes, I agree its there for just 2 attributes.

[Julien Muchembled]

2 ????

[Ayush Tiwari]

_reserved_uid_lock and _local_clear_reserved_time

[Julien Muchembled]

You contradict yourself or there's still misunderstanding. I don't see any setattr on _reserved_uid_lock.

[Ayush Tiwari]

There is not setattr on it but it this attribute is still being used from the class itself and not from instance. For example, this line:

klass._reserved_uid_lock.acquire()

[Julien Muchembled]

And my feeling is that self._reserved_uid_lock.acquire() could even work, and actually preferable if there's only 1 access (good time to change into with ... lock:. So we're back to my above question.

[Vincent Pelletier]

Thinking more about this change, I see several issues:

• _local_clear_reserved_time is an artifact from very old catalog: long long ago, we used MySQL's AUTOINCREMENT feature to allocate UIDs whenever we inserted rows in catalog table. Then, it became apparent that it was too slow, so we pre-allocated rows by inserting rows with "reserved" as path. Then, we would leak such reserved rows so the action to discard them was added, which then had to force UID counter in each Zope connected to the catalog to be reset. But nowadays, we use portal_ids to generate UIDs and we stopped inserting dummy rows, so such mechanism should not be needed, and I believe it should be dropped.
• _reserved_uid_lock being a class lock looks like a bad decision. I believe UID allocation should be moved to ZSQLCatalog level (aka, the tool level) instead of SQLCatalog level. It is only then that a class (or module) lock makes sense as tools are singletons, unlike their subobjects. I think this is not a blocker issue, but may help think on current code with a clearer mind.