Opened 11 years ago
Closed 11 years ago
#21871 closed Bug (fixed)
Apps.is_installed() incorrectly assumes app label is last portion of app dotted path
Reported by: | Carl Meyer | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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('.')[2] 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
Change History (2)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I decided to fix this for now with a simple short-circuiting loop in
is_installed()
, rather than with new API to get an app-config by name. The reason is thatis_installed()
should run successfully even when the app-registry isn't fully populated, which makes it a bit special. If there were new API to get an app-config by name, it should, I think, require the app registry to be populated, for consistency withget_app_config()
et al. And if this new API requires a fully-populated registry,is_installed()
can't use it. So if performance ofis_installed()
isn't critical, this seemed the simpler approach. The hypothetical new API can be added if/when we have a motivating use case that would actually use it.