fix a few typos and try to make warning messages more informative.

git-svn-id: https://svn.erp5.org/repos/public/erp5/trunk@43794 20353a03-c40f-0410-a6d1-a30d3c3de9de

product/ERP5/Tool/IdTool.py

@@ -117,18 +117,16 @@ class IdTool(BaseTool):
       Generate the next id in the sequence of ids of a particular group
     """
     if id_group in (None, 'None'):
-      raise ValueError, '%s is not a valid group Id.' % (repr(id_group), )
+      raise ValueError, '%s is not a valid id_group' % (repr(id_group), )
     # for compatibilty with sql data, must not use id_group as a list
     if not isinstance(id_group, str):
       id_group = repr(id_group)
-      warnings.warn('The id_group must be a string the other types '
-                    'is deprecated.', DeprecationWarning)
+      warnings.warn('id_group must be a string, other types '
+                    'are deprecated.', DeprecationWarning)
     if id_generator is None:
       id_generator = 'document'
     if method is not _marker:
-      warnings.warn("The usage of 'method' argument is deprecated.\n"
-                    "use this method with a id generator without this"
-                    "argument", DeprecationWarning)
+      warnings.warn("Use of 'method' argument is deprecated", DeprecationWarning)
     try:
       #use _getLatestGeneratorValue here for that the technical level
       #must not call the method
@@ -145,9 +143,9 @@ class IdTool(BaseTool):
       else:
         # Compatibility code below, in case the last version of erp5_core
         # is not installed yet
-        warnings.warn("You use the old version of API which is deprecated.\n"
-                      "please, update the business template erp5_core "
-                      "to use the new API", DeprecationWarning)
+        warnings.warn("You are using an old version of erp5_core to generate"
+                      "ids.\nPlease update erp5_core business template to "
+                      "use new id generators", DeprecationWarning)
         dict_ids = getattr(aq_base(self), 'dict_ids', None)
         if dict_ids is None:
           dict_ids = self.dict_ids = PersistentMapping()
@@ -178,18 +176,17 @@ class IdTool(BaseTool):
       Generate a list of next ids in the sequence of ids of a particular group
     """
     if id_group in (None, 'None'):
-      raise ValueError, '%s is not a valid group Id.' % (repr(id_group), )
+      raise ValueError, '%s is not a valid id_group' % (repr(id_group), )
     # for compatibilty with sql data, must not use id_group as a list
     if not isinstance(id_group, str):
       id_group = repr(id_group)
-      warnings.warn('The id_group must be a string the other types '
-                    'is deprecated.', DeprecationWarning)
+      warnings.warn('id_group must be a string, other types '
+                    'are deprecated.', DeprecationWarning)
     if id_generator is None:
       id_generator = 'uid'
     if store is not _marker:
-      warnings.warn("The usage of 'store' argument is deprecated.\n"
-                    "use this method with a id generator and without this."
-                    "argument", DeprecationWarning)
+      warnings.warn("Use of 'store' argument is deprecated.",
+                    DeprecationWarning)
     try:
       #use _getLatestGeneratorValue here for that the technical level
       #must not call the method
@@ -206,9 +203,9 @@ class IdTool(BaseTool):
       else:
         # Compatibility code below, in case the last version of erp5_core
         # is not installed yet
-        warnings.warn("You use the old version of erp5_core to generate the ids.\n"
-                      "please, update the business template erp5_core "
-                      "to have the new id generators", DeprecationWarning)
+        warnings.warn("You are using an old version of erp5_core to generate"
+                      "ids.\nPlease update erp5_core business template to "
+                      "use new id generators", DeprecationWarning)
         new_id = None
         if default is None:
           default = 1
@@ -298,8 +295,8 @@ class IdTool(BaseTool):
     """
     Get the last length id generated
     """
-    warnings.warn('The usage of this method is deprecated.\n'
-                   , DeprecationWarning)
+    warnings.warn('getLastLengthGeneratedId is deprecated',
+                   DeprecationWarning)
     # check in persistent mapping if exists
     if getattr(aq_base(self), 'dict_length_ids', None) is not None:
       last_id = self.dict_length_ids.get(id_group)
@@ -331,8 +328,7 @@ class IdTool(BaseTool):
     """
     Get the last id generated
     """
-    warnings.warn('The usage of this method is deprecated.\n'
-                   , DeprecationWarning)
+    warnings.warn('getLastGeneratedId is deprecated', DeprecationWarning)
     if getattr(aq_base(self), 'dict_ids', None) is None:
       self.dict_ids = PersistentMapping()
     last_id = None
@@ -359,11 +355,11 @@ class IdTool(BaseTool):
        Generates an Id.
        See generateNewLengthIdList documentation for details.
      """
-     warnings.warn('The usage of this method is deprecated.\n'
-                   'use directly generateNewIdList'
-                   ' with a id_generator sql', DeprecationWarning)
+     warnings.warn('generateNewLengthId is deprecated.\n'
+                   'Use generateNewIdList with a sql id_generator',
+                   DeprecationWarning)
      new_id = self.generateNewIdList(id_group=id_group,
-      			     id_count=1, default=default, store=store)[0]
+                      id_count=1, default=default, store=store)[0]
      return new_id
 
   security.declareProtected(Permissions.AccessContentsInformation,

[Georgios Dagkakis]

@jerome this warning was added before this commit, which is already very old, but do you know if there is a replacement for this method is not deprecated?

[Jérome Perrin]

Yes, it's very old and I don't remember exactly.

Maybe this concept of getting the last generated Id generally does not exist anymore (it's not part if IdTool interface), because getting the last generated Id is not an operation that may not be universally available on Id Generators. Instead we have export and import methods on id generator (see id generator interface).

But I really don't remember, maybe @seb or @vpelletier remember better.

This getLastGeneratedId is still used in 2 or 3 places, like AccountingTransaction_setReference where at some point we decided to use another id generator, but we wanted to keep the same sequences of ids (and not restart from 0). Using exportGeneratorIdDict here would probably not work well using the same pattern, it seems an heavy operation because it exports the full dict, but maybe we could use export/import in a post-upgrader step.

[Vincent Pelletier]

The reason this method is deprecated is that the need of retrieving the last generated ID without generating a new one is extremely dodgy.

The (normal) API of an ID generator is: "generate a new id", which must ensure by design that there are no duplicates. It should not be "give me something you already gave", with whatever assumption there may be on when exactly that id was issued: the id generator may not have its transaction boundaries synchronised with Zope transactions (ex: SQL id generators have their own super-short transactions to reduce lock-contention, because they are precisely used when ZODB generators would conflict too much), then another id generation may have happened between the intended id generation and the getLastGeneratedId call.

If the intend was to migrate an id generator, then one could just as well allocate a new ID to know the last one, the decrement it by one and request an id from the new generator with that value as default.

There should really be no use for such method.

[Georgios Dagkakis]

Thanks for the replies.

like AccountingTransaction_setReference where at some point we decided to use another id generator, but we wanted to keep the same sequences of ids (and not restart from 0).

I am not sure I understand this, how is the sequence of ids kept, since in id_group we pass the same id that we use also for the default?

In any way, AccountingTransaction_setReference is also the place from which I saw the warning and got curious. So we could now remove this now, am I right? I mean update the places were the script calls generateNewId to a pattern of:

    xxx_reference = id_generator(id_generator='uid',
                                     id_group=xxx_id_group,
                                     default=0)

[Jérome Perrin]

In that script, getLastGeneratedId is called without id_generator='uid', so it's done the default id generator (which is not 'uid').

This does not seem documented, but uid generator give more meaning to the default= argument. It's also used to initialize the internal counter, then the generator will not generate ids <= default.

We use this trick to keep the sequence of ids generated with the new id generator >= the one from the old generator.

A simplified version of this code could be:

invoice_reference = portal.portal_ids.generateNewId(
    id_generator='new',
    id_group=invoice_id_group,
    default=portal.portal_ids.getLastGeneratedId(
        id_generator='old',
        invoice_id_group,
        default=0) + 1)

With what Vincent suggest, it could be this:

invoice_reference = portal.portal_ids.generateNewId(
    id_generator='new',
    id_group=invoice_id_group,
    default=portal.portal_ids.generateNewId(
        id_generator='old',
        invoice_id_group,
        default=0))

but I'm afraid we can not really remove this default, because we can not know for sure if new (uid) have been used once with default= and initialized itself ... but in practice it was probably used at least once on all instances we care about, so maybe we could if we really want.

[Vincent Pelletier]

but I'm afraid we can not really remove this default, because we can not know for sure if new (uid) have been used once with default= and initialized itself ... but in practice it was probably used at least once on all instances we care about, so maybe we could if we really want.

Ah yes, I was making the assumption that there is migration phase during which there is no use of the migrated generator.

An alternative to migration could be:

• have a method which tells whether an id sequence exists (which can only reliably work on an id sequence which is not used by anything concurrent, but we have no way of materialising this)
• have a method to delete an id sequence

Then the code becomes:

• does the old sequence exist ?
  • yes, generate an id from this old sequence
  • generate a new id using the new sequence, providing the old id as default
  • delete the old sequence
  • return the id produced by the new sequence
• no, generate a new id using the new sequence without default

This is not perfect if there are concurrent accesses, but it should still produce consistent results and tend towards the old sequences eventually not existing, and costing a minimal amount of resources to check.