Quantcast

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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
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/ec5ffdfc-ef4b-4c30-a53d-d427cbe053e2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Collin Anderson-2
Hi,


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.


Collin


On Mon, Mar 20, 2017 at 5:46 PM, Luis Masuelli <[hidden email]> 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 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 receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
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/ec5ffdfc-ef4b-4c30-a53d-d427cbe053e2%40googlegroups.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 [hidden email].
To post to this group, send email to [hidden email].
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/CAFO84S73z%2BXW4xj9YKmHNXu4KQ%2Be%3DEKqFfXJeK5JyCSXGANGfQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Adam Johnson-2
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 <[hidden email]> wrote:
Hi,


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.


Collin


On Mon, Mar 20, 2017 at 5:46 PM, Luis Masuelli <[hidden email]> 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 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 receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
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/ec5ffdfc-ef4b-4c30-a53d-d427cbe053e2%40googlegroups.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 [hidden email].
To post to this group, send email to [hidden email].
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/CAFO84S73z%2BXW4xj9YKmHNXu4KQ%2Be%3DEKqFfXJeK5JyCSXGANGfQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Adam

--
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 [hidden email].
To post to this group, send email to [hidden email].
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/CAMyDDM2O-hDvy3gnJTx02gpqZz%2BWJ6uGS0cK98n9492Pijyp9w%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Brice PARENT-2
In reply to this post by Luis Masuelli

Seems interesting.

Also, added to the case where the only fields are the ones with only the two FK, there is the case where every other field has a default value (or at least accepts None). A call to .add, has no reason to fail in such cases.

For the other cases, a quick way to allow this, without depending on complex behaviours, would be to have allow something like :

publications = models.ManyToManyField(Publication, through=OtherModel, auto_create=True)
Of course, auto_create is probably not the name we'd use, it would have to be thought about a bit more.

But my point is that when this parametter is set, a call to makemigrations would only succeed if :
- Either all fields of OtherModel (excluding both FK of course) have a default value
- or OtherModel defines a special class method, like OtherModel.auto_create(model1, model2) which creates the instance, or OtherModel.get_default() which returns default values for the auto_creation process.
It makes it easy to use .add when we're explicitely allowed to, and the behaviour we have now to be maintained in all other cases.

Anyway, I just thought about this now, so I'm not sure if it covers enough use cases, and there are probably some side effects I didn't think of.

Brice

Le 20/03/17 à 22:46, Luis Masuelli a écrit :
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 receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
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/ec5ffdfc-ef4b-4c30-a53d-d427cbe053e2%40googlegroups.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 [hidden email].
To post to this group, send email to [hidden email].
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/cc73f520-ed2d-e2f0-5f50-68f05b0c195e%40brice.xyz.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Russell Keith-Magee-4
In reply to this post by Adam Johnson-2

On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson <[hidden email]> 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 [hidden email].
To post to this group, send email to [hidden email].
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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]> wrote:
On Tue, Mar 21, 2017 at 2:37 PM, Adam Johnson <[hidden email]> 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 [hidden email].
To post to this group, send email to [hidden email].
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 [hidden email].
To post to this group, send email to [hidden email].
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.
Loading...