Re: [Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Django]

#29294: ModelAdmin.raw_id_fields should be included in select_related() by
change_view and used by raw id widget
-+-
 Reporter:  Yurii Zolot'ko   |Owner:  nobody
 Type:   |   Status:  closed
  Cleanup/optimization   |
Component:  contrib.admin|  Version:  1.11
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * status:  new => closed
 * resolution:   => wontfix


Comment:

 Hi Jurrian — thanks for the follow-up.

 > I left out the BoundField part ...

 Yes… 🤔 With this it's quite a lot of extra complexity. In particular,
 pulling the widget type into bound field feels wrong.

 I'm going to say wontfix. Maybe it's worth a follow-up on the
 DevelopersMailingList to see if folks have any thoughts on a way forward.
 Otherwise though I suspect it's something that may just have to live in
 your project.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.53d407059ee73185dabc3249dd224359%40djangoproject.com.

Re: [Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Django]

#29294: ModelAdmin.raw_id_fields should be included in select_related() by
change_view and used by raw id widget
-+-
 Reporter:  Yurii Zolot'ko   |Owner:  nobody
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  contrib.admin|  Version:  1.11
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Jurrian Tromp):

 * status:  closed => new
 * resolution:  needsinfo =>


Comment:

 You are right, I am sorry. I left out the `BoundField` part which is
 indeed elemental. See the attachment for a working example,
 `ModelChoiceField` and `RawIdWidgetAdminMixin` are just there to patch it
 to make it work in my project, they are not part of this.

 The way I see it there is just one way to have the `value` supplied to
 `label_and_url_for_value` to be a model instance:

 {{{
 class BoundField(boundfield.BoundField):
 def value(self):
 if type(self.field.widget) == ForeignKeyRawIdWidget:
 try:
 return getattr(self.form.instance, self.name)
 except AttributeError:
 pass
 ...
 }}}

 However this feels hacky to me, although I can not find another way to get
 the model instance except for from the form. When being rendered, the
 `ForeignKeyRawIdWidget` has no access to the form anymore, so it seems the
 only place where we can pass it on is in the `BoundField`.
 What do you think?

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.19250664531b30d1cff4b41fc53ea9e4%40djangoproject.com.

Re: [Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Django]

#29294: ModelAdmin.raw_id_fields should be included in select_related() by
change_view and used by raw id widget
-+-
 Reporter:  Yurii Zolot'ko   |Owner:  nobody
 Type:   |   Status:  closed
  Cleanup/optimization   |
Component:  contrib.admin|  Version:  1.11
 Severity:  Normal   |   Resolution:  needsinfo
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Jurrian Tromp):

 * Attachment "raw_id_widget.py" added.


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.6a97eee24f54eaac91f27f13977632c8%40djangoproject.com.

Re: [Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Django]

#29294: ModelAdmin.raw_id_fields should be included in select_related() by
change_view and used by raw id widget
-+-
 Reporter:  Yurii Zolot'ko   |Owner:  nobody
 Type:   |   Status:  closed
  Cleanup/optimization   |
Component:  contrib.admin|  Version:  1.11
 Severity:  Normal   |   Resolution:  needsinfo
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * status:  new => closed
 * resolution:   => needsinfo


Comment:

 Hi Jurrian

 It looks like the code example there isn't complete… 🤔 — Can I ask you to
 post a sample project somewhere showing the widget  in action, so I can
 properly assess it?

 If the `value` passed to `label_and_url_for_value` is a model instance
 then maybe we can avoid the lookup but I need to see how that all fits
 together to say properly.

 Thanks.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.0330da105aa4efab58b9f696e5aa6a9d%40djangoproject.com.

Re: [Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Django]

#29294: ModelAdmin.raw_id_fields should be included in select_related() by
change_view and used by raw id widget
-+-
 Reporter:  Yurii Zolot'ko   |Owner:  nobody
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  contrib.admin|  Version:  1.11
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Jurrian Tromp):

 * status:  closed => new
 * resolution:  needsinfo =>


Comment:

 I was about to create a new issue for what I think is the same thing so
 reopening, I  [https://stackoverflow.com/questions/65794839/using-
 prefetch-related-in-django-in-combination-with-raw-id-fields also
 encountered] this problem.

 As stated the label_and_url_for_value() does a get() for each item. When
 using the raw_id_fields, this is something you don't want. Especially
 because it does not work with prefetching. I worked around the bug by
 subclassing it in my own project and actually did a benchmark:

 **Benchmark: change view page with 180 inline items with relations:**
 1. Using no raw_id_field: 1658 queries
 2. With raw_id_field and prefetching in Django 3.1: 384 queries
 3. Patched raw_id_field and prefetching: 18 queries

 This proves that my patched ForeignKeyRawIdWidget considerably reduces
 queries as they already have been prefetched.
 The culprit here is django.contrib.admin.widgets:176
 {{{obj = self.rel.model._default_manager.using(self.db).get(**{key:
 value})}}}

 This is a excerpt of what is needed to improve the behaviour which can be
 backwards compatible: we just need to try if value.pk exists, if not we
 use the default behaviour:

 {{{
 class ForeignKeyRawIdWidget(widgets.ForeignKeyRawIdWidget):
 def format_value(self, value):
 """Try to return the `pk` if value is an object, otherwise just
 return
  the value as fallback."""

 if value == '' or value is None:
 return None

 try:
 return str(value.pk)
 except AttributeError:
 return str(value)

 def label_and_url_for_value(self, value):
 """Instead of the original we do not have do a `get()` anymore
 instead
 access the instance directly so when value is prefetched this will
 prevent additional queries."""

 try:
 pk = value.pk
 meta = value._meta
 except AttributeError:
 # Fallback for compatibility with plain pk values
 return super().label_and_url_for_value(value)
 }}}


 Note that this is what I made in order to fix it in a project. If accepted
 I can provide a more permanent solution. Please let me know if this of
 interest.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.7cb0872790007e51b432e2e9e102a8d0%40djangoproject.com.

Re: [Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget

[Django]

#29294: ModelAdmin.raw_id_fields should be included in select_related() by
change_view and used by raw id widget
-+-
 Reporter:  Yurii Zolot'ko   |Owner:  nobody
 Type:   |   Status:  closed
  Cleanup/optimization   |
Component:  contrib.admin|  Version:  1.11
 Severity:  Normal   |   Resolution:  needsinfo
 Keywords:   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Tim Graham):

 * status:  new => closed
 * resolution:   => needsinfo


Comment:

 I'm not sure that including `raw_id_fields` in `select_related()` will
 always be a net performance increase, so I'm skeptical about that
 proposal.

 The idea about allowing `ForeignKeyRawIdWidget` to reuse the queryset
 seems interesting, however, I think you'll have to provide a patch to
 convince me that this it's feasible and sufficiently backwards compatible.
 Feel free to reopen the ticket if you provide that and someone will take
 another look.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.3303bb3c56502d4977203496e4c680c0%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.