Apps.is_installed() incorrectly assumes app label is last portion of app dotted path
|Reported by:||Carl Meyer||Owned by:||nobody|
|Has patch:||no||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
The app-registry tracks appconfigs indexed by app-label, but
is_installed() needs to get an app-config by app name instead (that is, full dotted path). Currently it does this by chopping off everything but the last portion, which used to be a valid approach but no longer is, since custom
AppConfig subclasses can define custom app labels.
Instead it could iterate over all app-configs looking for a name match, or (probably better) the registry could explicitly track the mapping from name to label.
21:51 <+carljm> mYk: This is a side-note, but I think it might be worth tracking appconfigs-by-appname in Apps 21:51 <+mYk> yeah 21:51 <+carljm> or explicitly tracking the translation from name-to-label and vice versa 21:51 <+mYk> mumble something cache invalidation something 21:51 <+carljm> I note that is_installed() has to reimplement that 21:52 <+carljm> because it takes a name 21:52 <+mYk> because of modify_settings and override_settings and friends 21:52 <+carljm> yeah 21:52 <+mYk> yes, but is_installed is mostly for import-time checks, so I don't care too much about performance 21:52 <+carljm> but I think is_installed would currently break on apps with a custom label 21:52 <+carljm> or am I missing something? 21:52 <+mYk> let me checkt 21:53 <+carljm> because for those apps, label != name.rpartition('.') 21:53 <+mYk> oh yes it wolud 21:54 <+mYk> anyway, the correct implementation is what I said earlier -- just iterate through all apss 21:54 <+carljm> yeah 21:54 <+mYk> or add a cache by name 21:54 <+carljm> I'll file that (and maybe work on it too) 21:54 <+mYk> we have apps.clear_cache() that's supposed to be called whenever the app registry changes 21:55 <+mYk> that should make the cache invalidation easy 21:55 <+carljm> yes 21:55 <+carljm> if we have a reasonable solution for cache invalidation, I think keeping the mapping explicitly would be nice 21:55 <+carljm> because referring to apps by full dotted path isn't going away, we're going to keep hitting the "damn I have to iterate" problem 21:56 <+mYk> you can create a function that returns this mapping 21:56 <+mYk> @lru_cache.lru_cache(maxsize=None) it 21:56 <+carljm> true story 21:56 <+mYk> and clear the cache 21:57 <+mYk> sounds easy, unless I've missed something