Commit 5815d3cb authored by Ayush Tiwari's avatar Ayush Tiwari Committed by Romain Courteaud

sql_catalog: No need to check for type

In case uid=0, its already being treated as int, so there is no
need to check for type and add time to performance.
parent 7c7be184
...@@ -1497,9 +1497,8 @@ class Catalog(Folder, ...@@ -1497,9 +1497,8 @@ class Catalog(Folder,
for object in object_list: for object in object_list:
uid = getattr(aq_base(object), 'uid', None) uid = getattr(aq_base(object), 'uid', None)
# Several Tool objects have uid=0 (not 0L) from the beginning, but # Generate unique uid for object having 0 or None as uid
# we need an unique uid for each object. if uid is None or uid == 0:
if uid is None or isinstance(uid, int) and uid == 0:
try: try:
object.uid = self.newUid() object.uid = self.newUid()
except ConflictError: except ConflictError:
......
  • >>> 0L is 0
    False
    >>> 0L == 0
    True

    In your message commit, you must explain why the comment you remove is wrong. If you can't, then it's your change that is wrong.

    The history behind long/int/zero uids is so old I don't even know it.

    /cc @vpelletier

  • The idea is that some tools used to have uid = 0 (so an int) as class property.

    Then, in some cases where the uid generator broke (in the era when we used to pre-allocate empty catalog rows using SQL autoincrement I think, @jerome and @romain may remember it better than me), it would sometime generate uid = 0L(so a long) as instance property. The idea here was to generate a new UID when a long-zero is found, to try to recover from this.

    But I think this change is correct independently from this, because uid = 0 is de-facto forbidden in the relational database (where there is no distinction between int and long anyway) as the uid generator starts at 1. So if a document of any type ever ends up in this code with uid == 0 it should always get a new uid (and accept getting one) to not break this expectation.

    Edited by Vincent Pelletier
  • Ok, the change looks correct, but I don't understand « The idea here was to generate a new UID when a long-zero is found, to try to recover from this. »: before this commit, we didn't generate a new UID in case of 0L.

    Anyway, we should go further. Simply:

          if not uid:

    And while we're at it, there's another isinstance(uid, int) below.

  • before this commit, we didn't generate a new UID in case of 0L.

    Good point. Maybe it was when tools were getting indexed for the first time then ? But I do not see why foo.uid == 0L should not be given an uid either...

    +1 for fixing other occurrences and simplifying the if.

  • before this commit, we didn't generate a new UID in case of 0L.

    it was when tools were getting indexed for the first time then

    Exactly, I see we were simply neglecting generating newUid for the case when uid = 0L before this commit. We were having other checks for uid = 0L though.

    And regarding another isinstance(uid, int), this doesn't concern the case of uid=0L anymore, given that we already will have a new uid generated for this case now.

Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment