Re: I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Collin Anderson]

Hi All,

I have a pull request for simple add()/create() etc with m2m through tables
if any wants to try it out: https://github.com/django/django/pull/8981

If people are happy with the API, I'll add the docs too.

Collin


On Mon, Apr 17, 2017 at 3:53 PM, Luis Masuelli 
wrote:

> I'm quite happy to see the topic is at least being considered! <3.
>
> Although I suggested a solution, I like the solution posted by Collin in
> the PR (I'd prefer solutions not involving signature changes in methods,
> but anyway those signature changes Colin posted are not so... obtrusive).
>
> I'd like to, however, propose a little change in Collin's code.
> Immediately before this code in _add_items:
>
> self.through(**dict(through_defaults, **{
> '%s_id' % source_field_name:
> self.related_val[0],
> '%s_id' % target_field_name: obj_id,
> }))
>
> This one:
>
> if callable(through_defaults):
> through_defaults =
> through_defaults(self.related_val[0], obj_id)
>
> With these two lines, we could allow passing a callable to through_fields
> (as we pass callables for defaults in django fields). The return value of
> such callable should be a dictionary, while the arguments should be source
> and target ids.
>
> But even if this little change is not implemented, I like the Collin's
> solution anyway.
>
> Another subtopic to discuss is about enhancing the Collin's solution, is
> to allow through_defaults be specified on field definition (In the same way
> we define default values in... scalar... fields; I leave open the
> discussion on whether such value should be overriden when using add,
> create, or any of these methods as changed by Collin).
>
> I like how this is going and hope that any solution (even if it is just
> the as-is solution provided by Collin, with no changes) be accepted in any
> near-future version <3.
>
> El martes, 21 de marzo de 2017, 20:29:33 (UTC-5), Alex Hill escribió:
>>
>> Here's a little bit more historical discussion on the topic: 
>> *https://groups.google.com/d/topic/django-developers/uWe31AjzZX0/discussion
>> *
>>
>> On Wed, 22 Mar 2017 at 05:57 Russell Keith-Magee 
>> wrote:
>>
>>> On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson  wrote:
>>>
 It does seem like a somewhat arbitrary historical restriction. Collin's
 PoC change is surprisingly small and simple.

 Seems like it would be fine if Django allowed add() and let any errors
> about missing data bubble-up.
>

>>> As the person who *made* the “somewhat arbitrary historical
>>> restriction”… :-)
>>>
>>> Honestly - the reason it was made was scope creep.
>>>
>>> I was trying to land Eric’s work on #6095, which was already moderately
>>> complex. Nobody disagreed that adding an object to an m2m with a through
>>> model was a *bad* idea - there were just a few design decisions that had to
>>> be made. But if we made those decisions, #6095 was going to take *another*
>>> couple of months to land; the perfect being the enemy of the good, we
>>> decided to limit scope and land “simple” m2m add, and revisit the issue
>>> later.
>>>
>>> I guess now is “later”… :-)
>>>
>>> Yours,
>>> Russ Magee %-)
>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups "Django developers (Contributions to Django itself)" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to django-develop...@googlegroups.com.
>>> To post to this group, send email to django-d...@googlegroups.com.
>>> Visit this group at https://groups.google.com/group/django-developers.
>>> To view this discussion on the web visit https://groups.google.com/d/ms
>>> gid/django-developers/CAJxq849m632K%3DaMfXGBtF%3DhMXFS9ujzU6
>>> xfUzNxSRkkN_UrkqQ%40mail.gmail.com
>>> 
>>> .
>>> For more options, visit https://groups.google.com/d/optout.
>>>
>> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/
> msgid/django-developers/a32cf6ae-324a-40c1-b9d9-
> 31bd43af2c2c%40googlegroups.com
> 
> 

Re: I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Luis Masuelli]

I'm quite happy to see the topic is at least being considered! <3.

Although I suggested a solution, I like the solution posted by Collin in 
the PR (I'd prefer solutions not involving signature changes in methods, 
but anyway those signature changes Colin posted are not so... obtrusive).

I'd like to, however, propose a little change in Collin's code. Immediately 
before this code in _add_items:

self.through(**dict(through_defaults, **{
'%s_id' % source_field_name: 
self.related_val[0],
'%s_id' % target_field_name: obj_id,
}))

This one:

if callable(through_defaults):
through_defaults = 
through_defaults(self.related_val[0], obj_id)

With these two lines, we could allow passing a callable to through_fields 
(as we pass callables for defaults in django fields). The return value of 
such callable should be a dictionary, while the arguments should be source 
and target ids.

But even if this little change is not implemented, I like the Collin's 
solution anyway.

Another subtopic to discuss is about enhancing the Collin's solution, is to 
allow through_defaults be specified on field definition (In the same way we 
define default values in... scalar... fields; I leave open the discussion 
on whether such value should be overriden when using add, create, or any of 
these methods as changed by Collin).

I like how this is going and hope that any solution (even if it is just the 
as-is solution provided by Collin, with no changes) be accepted in any 
near-future version <3.

El martes, 21 de marzo de 2017, 20:29:33 (UTC-5), Alex Hill escribió:
>
> Here's a little bit more historical discussion on the topic: 
> *https://groups.google.com/d/topic/django-developers/uWe31AjzZX0/discussion 
> *
>
> On Wed, 22 Mar 2017 at 05:57 Russell Keith-Magee  > wrote:
>
>> On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson > > wrote:
>>
>>> It does seem like a somewhat arbitrary historical restriction. Collin's 
>>> PoC change is surprisingly small and simple.
>>>
>>> Seems like it would be fine if Django allowed add() and let any errors 
 about missing data bubble-up.

>>>  
>> As the person who *made* the “somewhat arbitrary historical restriction”… 
>> :-)
>>
>> Honestly - the reason it was made was scope creep. 
>>
>> I was trying to land Eric’s work on #6095, which was already moderately 
>> complex. Nobody disagreed that adding an object to an m2m with a through 
>> model was a *bad* idea - there were just a few design decisions that had to 
>> be made. But if we made those decisions, #6095 was going to take *another* 
>> couple of months to land; the perfect being the enemy of the good, we 
>> decided to limit scope and land “simple” m2m add, and revisit the issue 
>> later.
>>
>> I guess now is “later”… :-)
>>
>> Yours,
>> Russ Magee %-)
>>
>> -- 
>> You received this message because you are subscribed to the Google Groups 
>> "Django developers (Contributions to Django itself)" group.
>> To unsubscribe from this group and stop receiving emails from it, send an 
>> email to django-develop...@googlegroups.com .
>> To post to this group, send email to django-d...@googlegroups.com 
>> .
>> Visit this group at https://groups.google.com/group/django-developers.
>> To view this discussion on the web visit 
>> https://groups.google.com/d/msgid/django-developers/CAJxq849m632K%3DaMfXGBtF%3DhMXFS9ujzU6xfUzNxSRkkN_UrkqQ%40mail.gmail.com
>>  
>> 
>> .
>> For more options, visit https://groups.google.com/d/optout.
>>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/a32cf6ae-324a-40c1-b9d9-31bd43af2c2c%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Alexander Hill]

Here's a little bit more historical discussion on the topic:
*https://groups.google.com/d/topic/django-developers/uWe31AjzZX0/discussion
*

On Wed, 22 Mar 2017 at 05:57 Russell Keith-Magee 
wrote:

> On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson  wrote:
>
> It does seem like a somewhat arbitrary historical restriction. Collin's
> PoC change is surprisingly small and simple.
>
> Seems like it would be fine if Django allowed add() and let any errors
> about missing data bubble-up.
>
>
> As the person who *made* the “somewhat arbitrary historical restriction”…
> :-)
>
> Honestly - the reason it was made was scope creep.
>
> I was trying to land Eric’s work on #6095, which was already moderately
> complex. Nobody disagreed that adding an object to an m2m with a through
> model was a *bad* idea - there were just a few design decisions that had to
> be made. But if we made those decisions, #6095 was going to take *another*
> couple of months to land; the perfect being the enemy of the good, we
> decided to limit scope and land “simple” m2m add, and revisit the issue
> later.
>
> I guess now is “later”… :-)
>
> Yours,
> Russ Magee %-)
>
> --
> You received this message because you are subscribed to the Google Groups
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to django-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/django-developers/CAJxq849m632K%3DaMfXGBtF%3DhMXFS9ujzU6xfUzNxSRkkN_UrkqQ%40mail.gmail.com
> 
> .
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CA%2BKBOKyNyc1_mcUQiQEiv2FanRLHfU6tR3Sxcv30CcLWgWEqQg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Russell Keith-Magee]

On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson  wrote:

> It does seem like a somewhat arbitrary historical restriction. Collin's
> PoC change is surprisingly small and simple.
>
> Seems like it would be fine if Django allowed add() and let any errors
>> about missing data bubble-up.
>>
>
As the person who *made* the “somewhat arbitrary historical restriction”…
:-)

Honestly - the reason it was made was scope creep.

I was trying to land Eric’s work on #6095, which was already moderately
complex. Nobody disagreed that adding an object to an m2m with a through
model was a *bad* idea - there were just a few design decisions that had to
be made. But if we made those decisions, #6095 was going to take *another*
couple of months to land; the perfect being the enemy of the good, we
decided to limit scope and land “simple” m2m add, and revisit the issue
later.

I guess now is “later”… :-)

Yours,
Russ Magee %-)

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAJxq849m632K%3DaMfXGBtF%3DhMXFS9ujzU6xfUzNxSRkkN_UrkqQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Adam Johnson]

It does seem like a somewhat arbitrary historical restriction. Collin's PoC
change is surprisingly small and simple.

Seems like it would be fine if Django allowed add() and let any errors
> about missing data bubble-up.
>

Agree, this is also a precedent from get_or_create.


> I personally think passing in a defaults dict (just like get_or_create
> does) would also be fine, but a callback seems like overkill.
>

This is a more consistent approach to the callback. One could always use
custom logic in the through model's save method, or a signal, to achieve
things that can't be done with through_defaults.

On 21 March 2017 at 00:46, Collin Anderson  wrote:

> Hi,
>
> Check out https://code.djangoproject.com/ticket/9475
>
> Seems like it would be fine if Django allowed add() and let any errors
> about missing data bubble-up. I personally think passing in a defaults dict
> (just like get_or_create does) would also be fine, but a callback seems
> like overkill.
>
> Here's a proof of concept: https://github.com/
> django/django/compare/master...collinanderson:ticket9475
>
> Collin
>
>
> On Mon, Mar 20, 2017 at 5:46 PM, Luis Masuelli 
> wrote:
>
>> I was reading this link in the official history
>>  and this other
>> link in this group
>> ,
>> but still do not understand why the issue against a way to call .add() to
>> add a through model. I thought several approaches (they are all backward
>> compatible for the end-user) that could work:
>>
>>1. Avoid the restriction to call add() if the model has only the two
>>FK fields.
>>2. An additional way to call .add() specifying additional fields to
>>fill (this one is not mine; was discussed in the linked thread). You risk
>>getting a (wrapped?) exception if you do not populate other fields
>>appropriately.
>>3. (This one was the first I thought and perhaps the easiest to
>>implement) Allow the ManyToManyField to specify a kwarg like
>>`through_factory=` which expects a callable which would populate more 
>> data.
>>The restriction to call .add() would remain if no `through_factory=` is
>>specified.
>>4. Avoid the restriction to call delete() if the model has only the
>>two FK fields.
>>
>> I considered these cases:
>>
>>- It is quite trivial a model with only two fields, but perhaps the
>>intermediate models could have additional useful methods. I see no trouble
>>having such model and allowing .add() or .delete() calls there.
>>- Having a special factory method would allow calls to .add() since
>>we'd be providing a way to make .add() actually know how to create an
>>instance of the intermediate model.
>>- You can combine these points, implement one, none, or all of them.
>>- (I did not consider extended use cases for delete() intentionally,
>>since perhaps a strange model (with different field pairs) could fit in
>>different relationships, although I cannot think in a case with multiple
>>relationships not incurrin in, perhaps, duplicate data when tried to be
>>used as through= model, but anyway I prefer to keep silence for other 
>> cases
>>involving delete(), but the case I stated).
>>- It is up to the user to be careful regarding migrating an
>>intermediate model regarding adding, changing, or deleting fields. But
>>anyway, this applies to any modification in the database models right now.
>>- Points 1, 2, 3 and/or 4 could be implemented with no clash. A
>>combined approach of 1, 2, 3 would look like this (this flow only applies
>>for the case when the through= is present - such scenario right now only
>>consists of raising an exception; the case with no through= model would 
>> not
>>be affected at all):
>>   - Instantiate `instance = ThroughModel(fka=a, fkb=b,
>>   **kwargs_from_add)` with the respective model instances a and b, from
>>   classes A and B which hold the desired M2M relationship. In this case, 
>> the
>>   point 2 just adds the **kwargs_from_add. If point 2 is not 
>> implemented, no
>>   **kwargs_from_add would be present.
>>   - (*If point 3 is implemented*) Call the callable in
>>   `through_factory=` invoking it `like_this(instance)`, if the callable 
>> is
>>   present. It is expected to save the instance.
>>   - (*If either point 1 is implemented and the model has only two
>>   fields, or point 2 is implemented*) Manually save the instance (if
>>   point 3 was not implemented or it was but the factory callable was not
>>   specified). (*Otherwise - point 2 not implemented AND (point 1 not
>>   implemented or model with more than two fields*)) Raise the
>>   currently implemented exception for the .add() method with through=
>>   specified (with a different string message) 

Re: I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Collin Anderson]

Hi,

Check out https://code.djangoproject.com/ticket/9475

Seems like it would be fine if Django allowed add() and let any errors
about missing data bubble-up. I personally think passing in a defaults dict
(just like get_or_create does) would also be fine, but a callback seems
like overkill.

Here's a proof of concept:
https://github.com/django/django/compare/master...collinanderson:ticket9475

Collin


On Mon, Mar 20, 2017 at 5:46 PM, Luis Masuelli 
wrote:

> I was reading this link in the official history
>  and this other
> link in this group
> ,
> but still do not understand why the issue against a way to call .add() to
> add a through model. I thought several approaches (they are all backward
> compatible for the end-user) that could work:
>
>1. Avoid the restriction to call add() if the model has only the two
>FK fields.
>2. An additional way to call .add() specifying additional fields to
>fill (this one is not mine; was discussed in the linked thread). You risk
>getting a (wrapped?) exception if you do not populate other fields
>appropriately.
>3. (This one was the first I thought and perhaps the easiest to
>implement) Allow the ManyToManyField to specify a kwarg like
>`through_factory=` which expects a callable which would populate more data.
>The restriction to call .add() would remain if no `through_factory=` is
>specified.
>4. Avoid the restriction to call delete() if the model has only the
>two FK fields.
>
> I considered these cases:
>
>- It is quite trivial a model with only two fields, but perhaps the
>intermediate models could have additional useful methods. I see no trouble
>having such model and allowing .add() or .delete() calls there.
>- Having a special factory method would allow calls to .add() since
>we'd be providing a way to make .add() actually know how to create an
>instance of the intermediate model.
>- You can combine these points, implement one, none, or all of them.
>- (I did not consider extended use cases for delete() intentionally,
>since perhaps a strange model (with different field pairs) could fit in
>different relationships, although I cannot think in a case with multiple
>relationships not incurrin in, perhaps, duplicate data when tried to be
>used as through= model, but anyway I prefer to keep silence for other cases
>involving delete(), but the case I stated).
>- It is up to the user to be careful regarding migrating an
>intermediate model regarding adding, changing, or deleting fields. But
>anyway, this applies to any modification in the database models right now.
>- Points 1, 2, 3 and/or 4 could be implemented with no clash. A
>combined approach of 1, 2, 3 would look like this (this flow only applies
>for the case when the through= is present - such scenario right now only
>consists of raising an exception; the case with no through= model would not
>be affected at all):
>   - Instantiate `instance = ThroughModel(fka=a, fkb=b,
>   **kwargs_from_add)` with the respective model instances a and b, from
>   classes A and B which hold the desired M2M relationship. In this case, 
> the
>   point 2 just adds the **kwargs_from_add. If point 2 is not implemented, 
> no
>   **kwargs_from_add would be present.
>   - (*If point 3 is implemented*) Call the callable in
>   `through_factory=` invoking it `like_this(instance)`, if the callable is
>   present. It is expected to save the instance.
>   - (*If either point 1 is implemented and the model has only two
>   fields, or point 2 is implemented*) Manually save the instance (if
>   point 3 was not implemented or it was but the factory callable was not
>   specified). (*Otherwise - point 2 not implemented AND (point 1 not
>   implemented or model with more than two fields*)) Raise the
>   currently implemented exception for the .add() method with through=
>   specified (with a different string message) because the through_factory 
> was
>   not present, and so the framework does not know how to populate 
> additional
>   fields.
>   - Catch-and-reraise (or don't catch at all and let them be) the
>   error for missing data in the tried-to-save model.
>- An example of the callable in point 3 would be like this (just an
>example for, say, a game!):
>   - def on_added(through_model_instance):
>   through_model_instance.balance = 1000.0 #although this one
>   could be a default value at db level.
>   through_model_instance.save()
>   through_model_instance.achievements.create(tag='joined-this-
>   relation')
>
> I understand these approaches, combined or not, could not be perfect or
> bug-free. But I'd like to discuss them instead 

I would like to discuss my proposal for a working way to call .add() on an m2m with through= model

[Luis Masuelli]

I was reading this link in the official history 
 and this other link 
in this group 
, 
but still do not understand why the issue against a way to call .add() to 
add a through model. I thought several approaches (they are all backward 
compatible for the end-user) that could work:

   1. Avoid the restriction to call add() if the model has only the two FK 
   fields.
   2. An additional way to call .add() specifying additional fields to fill 
   (this one is not mine; was discussed in the linked thread). You risk 
   getting a (wrapped?) exception if you do not populate other fields 
   appropriately.
   3. (This one was the first I thought and perhaps the easiest to 
   implement) Allow the ManyToManyField to specify a kwarg like 
   `through_factory=` which expects a callable which would populate more data. 
   The restriction to call .add() would remain if no `through_factory=` is 
   specified.
   4. Avoid the restriction to call delete() if the model has only the two 
   FK fields. 
   
I considered these cases:

   - It is quite trivial a model with only two fields, but perhaps the 
   intermediate models could have additional useful methods. I see no trouble 
   having such model and allowing .add() or .delete() calls there.
   - Having a special factory method would allow calls to .add() since we'd 
   be providing a way to make .add() actually know how to create an instance 
   of the intermediate model.
   - You can combine these points, implement one, none, or all of them.
   - (I did not consider extended use cases for delete() intentionally, 
   since perhaps a strange model (with different field pairs) could fit in 
   different relationships, although I cannot think in a case with multiple 
   relationships not incurrin in, perhaps, duplicate data when tried to be 
   used as through= model, but anyway I prefer to keep silence for other cases 
   involving delete(), but the case I stated).
   - It is up to the user to be careful regarding migrating an intermediate 
   model regarding adding, changing, or deleting fields. But anyway, this 
   applies to any modification in the database models right now.
   - Points 1, 2, 3 and/or 4 could be implemented with no clash. A combined 
   approach of 1, 2, 3 would look like this (this flow only applies for the 
   case when the through= is present - such scenario right now only consists 
   of raising an exception; the case with no through= model would not be 
   affected at all):
  - Instantiate `instance = ThroughModel(fka=a, fkb=b, 
  **kwargs_from_add)` with the respective model instances a and b, from 
  classes A and B which hold the desired M2M relationship. In this case, 
the 
  point 2 just adds the **kwargs_from_add. If point 2 is not implemented, 
no 
  **kwargs_from_add would be present.
  - (*If point 3 is implemented*) Call the callable in 
  `through_factory=` invoking it `like_this(instance)`, if the callable is 
  present. It is expected to save the instance.
  - (*If either point 1 is implemented and the model has only two 
  fields, or point 2 is implemented*) Manually save the instance (if 
  point 3 was not implemented or it was but the factory callable was not 
  specified). (*Otherwise - point 2 not implemented AND (point 1 not 
  implemented or model with more than two fields*)) Raise the currently 
  implemented exception for the .add() method with through= specified (with 
a 
  different string message) because the through_factory was not present, 
and 
  so the framework does not know how to populate additional fields.
  - Catch-and-reraise (or don't catch at all and let them be) the error 
  for missing data in the tried-to-save model.
   - An example of the callable in point 3 would be like this (just an 
   example for, say, a game!):
  - def on_added(through_model_instance):
  through_model_instance.balance = 1000.0 #although this one could 
  be a default value at db level.
  through_model_instance.save()
  through_model_instance.achievements.create(tag=
  'joined-this-relation')
  
I understand these approaches, combined or not, could not be perfect or 
bug-free. But I'd like to discuss them instead of plainly discard them. 
AFAIK right now the calls to .add() are disallowed when through is 
specified.

Could we work or at least discuss these use cases now? The history I quoted 
seems to be pretty historic, and we're in 2017. The first thing I'd 
like to know right not (with django 1.10 alive) is the feasability of this 
feature in a future django version.

I await your comments and/or criticism ^^.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop