PermissionPolicy implementation detail looks wrong

[Dave Abrahams <dave@…>]

See source:trunk/trac/perm.py@5808#L446​

In case of hash collision (yes I realize it's unlikely) couldn't this return wrong results?

[Christian Boos]

Right, the cache should contain a copy of the full key for disambiguation.

[osimons]

I suppose a string representation of the resource is as distinct as it gets? Seems to work fine.

trac/perm.py

@@ -521 +521 @@
         return self._has_permission(action, resource)
 
     def _has_permission(self, action, resource):
-        key = (self.username, hash(resource), action)
+        resource_key = resource and "%s:%s:%s" % (resource.realm,
+                            resource.id, resource.version) or None
+        key = (self.username, resource_key, action)
         try:
             return self._cache[key]
         except KeyError:

[Dave Abrahams <dave@…>]

Not to put too fine a point on it, but "Seems to work fine" isn't very compelling… especially in this case because someone surely said the same for the original (wrong) code.

[osimons]

Replying to Dave Abrahams <dave@boost-consulting.com>:

Not to put too fine a point on it, but "Seems to work fine" isn't very compelling… especially in this case because someone surely said the same for the original (wrong) code.

Thanks for your words of encouragement.

It was intended as a start of a discussion on alternatives to hashing, or perhaps of what to hash and what to have as full text.

I'm not worried about the distinctiveness of it as it will be as distinct as it gets (permission-side), but more the impact of it on memory use. A repos with many files, deep hierarchies and lots of revisions is going to take up a large chunk of memory by storing the full (unicode) path for each and every revision of a each and every file path as Trac gets used and queried. That could mean each resource in the cache consuming perhaps 250 bytes on average compared to the 2-4 bytes of the integer hash. On my limited test setup I can't test the consequences of this in full - it pretty much works as before. However, testing this by querying a full list of files for each revision on a t.e.o copy may find this unusable.

The full representation is the most distinct way I can see, and of course it works. That does not mean it is the right way - or necessarily that hashing is 'wrong' as you say. Perhaps we should store the realm as string, and just hash the id and version? This needs to be discussed, and there are trade-offs involved. Certainly as high memory usage is alsready a problem.

[Christian Boos]

Replying to osimons:

The full representation is the most distinct way I can see, and of course it works.

Why not keep a reference to the Resource object in the value, for disambiguation needs? They are probably already referenced from somewhere else and the lifespan of this cache is anyway limited (one request). There would be little additional memory cost - the hash will be kept as the key, the value would be the object plus the decision.

Only in case of collision (quite unlikely, but this is the whole point of the ticket), the value would be a list of (resource, decision) pairs.

[osimons]

Patch saving the resource and double-checking before using the cached decision.

[osimons]

Replying to cboos:

Why not keep a reference to the Resource object in the value, for disambiguation needs?

OK. If memory usage is not a significant issue that is the best way. And, it would also likely be a less expensive operation and generally work better as the resource evolves over time. I've attached a patch for review: attachment:t5668-r6465-perm-cache-key.a.diff​

As hashing and equality testing is of such great importance for perm checking, the patch also includes test cases for __eq__() and __hash__() for the Resource class. If anything related to this changes later, it should be detected by the tests.

[Christian Boos]

Looks good. Invalidating the cache in case of collision is actually even simpler than what I proposed. Still, I think the patch can be improved by avoiding the exception handler, something along the lines of:

    ...
    cached = self._cache.get(key)
    if cached:
        cache_decision, cache_resource = cached
        if resource == cache_resource:
            return cache_decision
    ...

I think we can target 0.11 for this one.

[Christian Boos]

Oh, btw. for the unit tests, I would be surprised if the hash values themselves are portable…

[osimons]

Updated patch to not use exception handling + dropping hash testing.

[osimons]

New patch: attachment:t5668-r6465-perm-cache-key.b.diff​

It does look cleaner without exceptions. And, dropping the test cases on hash creation - it does indeed vary across installations. Now I know ;-)

[Christian Boos]

Yep, new patch looks perfect (and worksforme), please commit.

[osimons]

Replying to cboos:

Yep, new patch looks perfect (and worksforme), please commit.

Committed as [6482]. Closing.

[Dave Abrahams <dave@…>]

Replying to osimons:

Replying to Dave Abrahams <dave@boost-consulting.com>:

Not to put too fine a point on it, but "Seems to work fine" isn't very compelling… especially in this case because someone surely said the same for the original (wrong) code.

Thanks for your words of encouragement.

Is that the tangy snap of sarcasm I smell?

It was intended as a start of a discussion on alternatives to hashing, or perhaps of what to hash and what to have as full text.

I'm not worried about the distinctiveness of it as it will be as distinct as it gets (permission-side), but more the impact of it on memory use. A repos with many files, deep hierarchies and lots of revisions is going to take up a large chunk of memory by storing the full (unicode) path for each and every revision of a each and every file path as Trac gets used and queried. That could mean each resource in the cache consuming perhaps 250 bytes on average compared to the 2-4 bytes of the integer hash. On my limited test setup I can't test the consequences of this in full - it pretty much works as before. However, testing this by querying a full list of files for each revision on a t.e.o copy may find this unusable.

The full representation is the most distinct way I can see, and of course it works. That does not mean it is the right way - or necessarily that hashing is 'wrong' as you say.

Just to be clear: I never claimed that hashing is wrong. I said the original code was wrong, and it was, because hash collisions were not accounted for.

[osimons]

Replying to Dave Abrahams <dave@boost-consulting.com>:

Just to be clear: I never claimed that hashing is wrong. I said the original code was wrong, and it was, because hash collisions were not accounted for.

Sure. I learned a lot along the way here as I dug into it, so thanks for pointing out the issue and bearing with me.