Deprecated fix for 1.8

[tobes]

The deprecated feature was broken and not returning the correct values the first commit fixes that but would not work when methods like .get() where called

The second commit allows these methods to work as well as returning the object itself when needed

The code is quite horrible but hopefully make enough sense

This bug caused the search facets to become unstable and disappear after the first time they where used to see this in action run release-v1.8 branch goto /dataset see the facets then refresh the page - they disapear

[tobes]

Note:

I've had to cherry pick this into the demo theme branch as the facets were dead

[icmurray]

@tobes

I'm really sorry this was broken and you had to spend time debugging it. :-(

This latest commit fixes up the old function. The problem was that there was more than one instance of the StackedObjectProxy class hanging around; hence calling c._current_obj() was incorrect; and self._current_object() is.

This caused the bug you found because the global object, ckan.lib.base.g, is an instance of StackedObjectProxy. And by coincidence, it also has a facets attribute which stored the fields that solr should facet on. So after the first served request, g.facets was empty (because it was handing off to c.facets), causing the solr query to not request any faceting information.

I've checked the tests pass; and I've checked your bug (visit /dateset ; hit refresh). If you're happy, I'll merge this into master.

[tobes]

@icmurray @kindly

I've redone this as suggested by kindly

[icmurray]

@tobes @kindly

I don't know what's happened with github notifications but I hadn't seen any of this discussion up til now :-( (Github asked me validate by email address yeterday, so maybe related). So sorry if I've seemed tardy over this, but I haven't read this til now.

I really don't understand what the issue with the original (fixed) code is:

• I don't know what exactly you mean by whether setattr(c.__class__, item_name, property(get_item)) is thread-safe:
  • If you mean is it atomic, then the answer is: probably yes, but only due to cpython implementation details. So not reliably. Either way, I don't think it needs to be an atomic function call: what does it matter if two interleaved threads are both setting the same attribute on the same class with different values that behave identically?
  • If you mean, "doesn't this mean that the effects of this live after the request is over?". Then, yes. Absolutely. I'm fine with that. This is for deprecating uses of a item in a global context. I can't envisage any situation where we'd conditionally deprecate a context item: either it's deprecated for the version of ckan we've imported or not.

I personally think the original code is cleaner, more concise and as far as I can see correct. I'm willing to be proved wrong. On balance, I don't like the use of the private function, self._current_obj(), in the original code: this may break if the private function changes/disappears in future versions.

Maybe it's the "effects of this living on after the request is over" that you don't like, in which case, I think your solution is what's required. However, it has a race-condition:

if not hasattr(c.__class__, '__old_getattr__'):  # step 0
    # stuff happens

    __old_getattr__ =  getattr(c.__class__, '__getattr__')  # step 1
    setattr(c.__class__, '__old_getattr__', __old_getattr__)  # step 2
    setattr(c.__class__, '__getattr__', custom__getattr__)  # step 3

Consider the following interleaving:

Thread-0: step 0 # __old_getattr__ is not set, therefore predicate is True, and enters if-block
Thread-1: step 0 # __old_getattr__ is not set, therefore predicate is True, and enters if-block
Thread-0: step 1 ; step 2 ; step 3
# At this stage `c.__class__.__getattr__` points to the custom get attr function.
Thread-1: step 1; step 2; step 3
# We've just reached the state were Thread-1 has set `c.__class__.__old_getattr__` to `custom__getattr__`.

And depending upon whether the c instance is a global singleton or not, this is also a possible race condition:

# if c.__depricated_properties__ is not set it returns ''
if not c.__depricated_properties__:
    c.__depricated_properties__ = {}
c.__depricated_properties__[item_name] = message