Changes between Version 20 and Version 21 of DjangoSpecifications/Core/SingleInstance
- Timestamp:
- Mar 25, 2008, 9:35:07 AM (16 years ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
DjangoSpecifications/Core/SingleInstance
v20 v21 9 9 10 10 === Inconsistency === 11 #5514 describes what I think is the main issue with the current behavior of the ORM. Having multiple instances can result in silent data losses. Even worse the actual result would depend on whether one used select_relatedor not:11 #5514 describes what I think is the main issue with the current behavior of the ORM. Having multiple instances can result in silent data losses. Even worse the actual result would depend on whether one used {{{select_related}}} or not: 12 12 {{{ 13 #!python 13 14 class Operand(models.Model): 14 15 value = models.IntField() … … 30 31 Models with self FKs will exhibit similar issues: 31 32 {{{ 32 33 #!python 33 34 class Directory(models.Model): 34 35 name = models.CharField() … … 46 47 The original goal of #17 was reducing memory use and query count by reusing the same instance instead of having a new one. Let's see a typical example of that: 47 48 {{{ 49 #!python 48 50 class ArticleType(models.Model): 49 51 name = models.CharField(maxlength=200) … … 65 67 }}} 66 68 67 In the above example, type_of_articleshould only be queried for once, because it's already in memory. This is a '''very''' common use case for many developers.69 In the above example, {{{type_of_article}}} should only be queried for once, because it's already in memory. This is a '''very''' common use case for many developers. 68 70 69 71 If you have a great number of articles and a smaller number of articletypes the performance/memory hit is staggering because: … … 84 86 85 87 === API === 86 The public API for this feature will be very simple. It will be a single optional parameter in the Metaclass:88 The public API for this feature will be very simple. It will be a single optional parameter in the {{{Meta}}} class: 87 89 88 90 {{{ 91 #!python 89 92 class ArticleType(models.Model): 90 93 name = models.CharField(maxlength=200) … … 98 101 == Implementation == 99 102 #17 currently has a working patch with the following: 100 * Per-model WeakValueDictionary with dict[pk_val] = instance.101 * !__call!__classmethod which either gets a instance from the dict or return a newly created one.103 * Per-model {{{WeakValueDictionary}}} with {{{dict[pk_val] = instance}}}. 104 * {{{__call__}}} classmethod which either gets a instance from the dict or return a newly created one. 102 105 * a simple interface for enabling or disabling the feature for a given Model (default is disabled). 103 106 * a slightly modified related manager which will first try to get the instance from the Model dict instead of blindly querying the DB. … … 109 112 * In order to properly handle shared instances, partial models, or modified models (such as those with extra select clauses) need to be wrapped in a class which will handle those modifications. 110 113 * there is no doc as to how to enable/disable this feature and what to expect from it. 111 * the API for enabling/disabling the feature is very crude and consists only of two class methods. It should at the very least be possible to set the value as a member of the Metaclass.114 * the API for enabling/disabling the feature is very crude and consists only of two class methods. It should at the very least be possible to set the value as a member of the {{{Meta}}} class. 112 115 * the Model dict should be set threadlocally. 113 116 * it should be possible to force instantiation, because the serializers want that. 114 * For the sake of completeness, it should be possible to do Model.objects.get_from_db(id=something)to bypass the singleton.117 * For the sake of completeness, it should be possible to do {{{Model.objects.get_from_db(id=something)}}} to bypass the singleton. 115 118 116 119 == Discussions == 117 120 === Internal API === 118 ''(David)'' The current patch design overrides the !__call!__method for the model base which means it automatically returns singleton instances when available. This is convenient because XXX (David or Brian: feel free to fill this out).121 ''(David)'' The current patch design overrides the {{{__call__}}} method for the model base which means it automatically returns singleton instances when available. This is convenient because XXX (David or Brian: feel free to fill this out). 119 122 120 123 ''(Philippe)'' Another approach would be to make this explicit and have a simple API along the lines of: 121 124 {{{ 125 #!python 122 126 class ModelBase: 123 127 def get_singleton(pk, kwargs = None): … … 125 129 126 130 }}} 127 That means writing Model(kwargs) would return a freshly instantiated object (as is the case now) and bypass the singleton instance. Since Django's ORM would use the API internally, doing Model.objects.get would (by default, overridable in the manager) return the singleton. The default related manager would also have a new get_from_dbmethod to force retrieving and instantiating from the DB only.131 That means writing {{{Model(kwargs)}}} would return a freshly instantiated object (as is the case now) and bypass the singleton instance. Since Django's ORM would use the API internally, doing {{{Model.objects.get}}} would (by default, overridable in the manager) return the singleton. The default related manager would also have a new {{{get_from_db}}} method to force retrieving and instantiating from the DB only. 128 132 129 133 === Default behavior === … … 131 135 * ''(Philippe)'' It should be because it really makes the ORM behave more correctly but for the sake of backward compatibility let's fist commit off by default. We can change the default parameter value later, and in the meantime honor a global setting to turn it on for all models. 132 136 133 * ''(David)'' This would always be enabled. You cannot turn it off. If stuff is broken by having this on then it should be broken. This can cause problems with serializers, and things that modify querysets and shouldn't (e.g. .extra). That is the only reason it should be turned off. If those problems are addressed there is no reason to use #17 and to have it enabled 100% of the time.137 * ''(David)'' This would always be enabled. You cannot turn it off. If stuff is broken by having this on then it should be broken. This can cause problems with serializers, and things that modify querysets and shouldn't (e.g. {{{.extra}}}). That is the only reason it should be turned off. If those problems are addressed there is no reason to use #17 and to have it enabled 100% of the time. 134 138 135 139 === extra, values === 136 140 137 A simple workaround for querysets issues, and this would branch into being able to store .values()as partials as well (this code is untested, but should give a general idea):141 A simple workaround for querysets issues, and this would branch into being able to store {{{.values()}}} as partials as well (this code is untested, but should give a general idea): 138 142 139 143 {{{ 144 #!python 140 145 class SphinxObjectInstance(object): 141 146 """ … … 160 165 }}} 161 166 162 The proposal would also change how extra is used, it should probably be instance._extraor something similar.167 The proposal would also change how extra is used, it should probably be {{{instance._extra}}} or something similar. 163 168 164 169 (The code above is taken from an unpublished update to the django-sphinx package.)