Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Required flag in many to many relations. #2674

Closed
wants to merge 1 commit into from
Closed

Required flag in many to many relations. #2674

wants to merge 1 commit into from

Conversation

normanjaeckel
Copy link

Here a test to show that the required flag in ManyToMany relation fields is wrong if blank=True.

This does not happen if null=True is also set. I expected the required flag be False if blank=True regardless of null=True is set or not. (In Django ManyToMany fields null=True is not evaluated I think.)

@tomchristie
Copy link
Member

Does this pull request reference any existing issue?

Understand the basic premise of this pull request, but it does need further review - it's not even obvious what null=True or blank=True means on a many to many relationship. (You can't set either side of the relationship to None, but it may be empty)

@normanjaeckel
Copy link
Author

There is no issue existing yet. I did not open one because I wanted to get first review.

In my projects I often use many to many fields with blank=True meaning that the field can be empty in the REST API (see OPTIONS) and in form fields. If set blank=False I would expect form validation ensure that at least one target item is given (e. g. Pizza model with many to many field Toppings: blank=False means every pizza has to have at least one topping).

The restframework does something specific: If required is True, it forces to transmit the key in POST and PUT requests but with an empty array it's stil valid (thats the next bug I think). If required is False, you can send an empty array or even omit the key in POST and PUT requests.

By the way: Thanks for this nice project. I love it. :-)

@tomchristie
Copy link
Member

Related: #2250.

@tomchristie
Copy link
Member

I expected the required flag be False if blank=True

Could you rephrase this into what set of inputs you'd expect the API to allow for M2M model fields with blank = True.

The existing behavior of required=True - meaning that a list must be supplied, but that it may be empty feels consistent to me as commented on #2804.

@normanjaeckel
Copy link
Author

Sure: Imagine the following models:

class Topping(models.Model):
    name = models.CharField(...)

class Pizza(models.Model):
   name = models.CharField(...)
   toppings = models.ManyToManyField(Topping)

In this case I would expect that the default behavior of the framework would be to expect at least one topping in every pizza set. Omitting the key toppings or sending an empty array should be invalid.

But if you change the models

    toppings = models.ManyToManyField(Topping, blank=True)

then it should be possible to send new pizzas just by the name and omit the key toppings completely ({'name': 'ForThePoor'}). Also possible should be to send an empty array and (of course) send some toppings. Everything should be valid here.

At the moment, the framework does neither like in the first nor in the second example because required==True accepts empty arrays and setting blank=True does not disable required for its own as I expect.

For me it would be enough if you fix the second example case meaning setting blank=True resets required to False.

@marccerrato
Copy link

+1

1 similar comment
@emanuelschuetze
Copy link

+1

@xordoquy
Copy link
Collaborator

For me it would be enough if you fix the second example case meaning setting blank=True resets required to False.

I'm not sure which blank you're referring to.
Please note that DRF provides three flags (required, blank, null) to control those things while Django has only two. This is primary because Django uses forms that miss an empty state - usually blank equals empty. However, with an API things are different:

  • {}
  • {"data": none}
  • {"data": ""}
  • {"data": "some content"}

@tomchristie
Copy link
Member

There are two possible options given the example from @normanjaeckel...

  1. Have an allow_empty flag on M2M relationships, that we set to False unless blank=True. This would be a breaking change for existing APIs, so is not acceptable.
  2. Ensure that blank=True does set required=False for M2M relationships, and that None or empty are then treated as valid and equivalent to the empty list.

I think I'd probably be happy enough with 2, although it feels a bit wooly to me. (I'd still rather my own client APIs sent a blank empty list for that case)

@normanjaeckel
Copy link
Author

Ensure that blank=True does set required=False for M2M relationships, and that None or empty are then treated as valid and equivalent to the empty list.

That would be nice for the next bugfixing or minor release.

But nevertheless it looks strange that I can send an empty list to a required field in DRF but not in Django forms. E. g. it is also invalid to send an empty string to a required char field in DRF and Django forms. Things should be consistent here. So this point might be something for the next major release ...

@smcoll
Copy link

smcoll commented Jun 9, 2015

So are we saying that setting required=False as default for M2M relationships with blank=True is insufficient because DRF would allow None and empty as valid values in that case, whereas Django forms wouldn't?

@normanjaeckel
Copy link
Author

So are we saying that setting required=False as default for M2M relationships with blank=True is insufficient because DRF would allow None and empty as valid values in that case, whereas Django forms wouldn't?

No. If the field is not required, you can send what you want. But if it is required, in my opinion None and empty lists should be forbidden.

@tomchristie
Copy link
Member

Considering closing this in favor of #2804 - would that be reasonable?

@normanjaeckel
Copy link
Author

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants