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

M2M model field without blank=True should map to a serializer relationship with allow_empty=False. #2804

Closed
marccerrato opened this issue Apr 8, 2015 · 12 comments
Labels
Milestone

Comments

@marccerrato
Copy link

The Django 1.8 system check framework displays the warning fields.W340 when the null parameter is defined on a ManyToManyField, since it has no effect.

This null field affects on whether a related field in a model serializer is considered required or not. We can see it here.

From my point of view, this behavior is correct for a ForeignKey but not for a ManyToManyField, that should be considered not required by default.

Adding the var to_many to the condition on the aforementioned code might do the trick.

@normanjaeckel
Copy link

See test in #2674.

@pdr
Copy link

pdr commented May 22, 2015

+1

@tomchristie
Copy link
Member

should be considered not required by default.

For both this and #2674 I'm unsure exactly what behavior you're expecting.
M2M fields default to required, because the input field itself is required, although it may be an empty list.

Are you expecting to be able to provide null as the input for the M2M field, or are you seeing an issue when providing an empty list?

As currently described the existing behavior seems correct to me - require the input, but allow it to be an empty list.

@normanjaeckel
Copy link

See my answer in #2674. If required=True (the default) the framework should not accept empty lists/arrays. This is because the framework should behave like Django forms and here we got an error if we try to send an empty list to a required form field.

(The other point (not this issue) is that the required flag should be controlled by the model field option blank which is not the case at the moment.)

@marccerrato
Copy link
Author

I agree with #2674 (comment)

@emanuelschuetze
Copy link

+1

@tomchristie tomchristie changed the title ManyToManyField should be mapped as not required by default on a ModelSerializer M2M model field with required=True should map to a serializer relationship with allow_empty=False. Jun 24, 2015
@tomchristie tomchristie changed the title M2M model field with required=True should map to a serializer relationship with allow_empty=False. M2M model field with null=True should map to a serializer relationship with allow_empty=False. Jun 24, 2015
@tomchristie tomchristie changed the title M2M model field with null=True should map to a serializer relationship with allow_empty=False. M2M model field without blank=True should map to a serializer relationship with allow_empty=False. Jun 24, 2015
@tomchristie
Copy link
Member

Retitled more appropriately.
The behavior that's being requested here as far as I understand it is to mirror Django's blank model field -> required input.

I'm not 100% convinced that we want ManyToManyField(Something) to always require >= 1 entry and ManyToManyField(Something, blank=True) to allow empty lists, and I'd really like to see that discussion pushed to the mailing list, but certainly we should at least allow either case to be specified.

If we do change the behavior we should do so in a major version bump and call it out prominently.

Requires #2250 as a prerequisite.

@ideallical
Copy link

+1

@tomchristie
Copy link
Member

Accepting this, though given the potential for backwards incompatibility it's probably a good idea if we push it in 3.2.0, to allow us to call it out.

@tomchristie
Copy link
Member

Also note that allow_empty is now a valid flag.
You can workaround the behavior here by using extra_kwargs = {'my_field': {'allow_empty': False}}.

@normanjaeckel
Copy link

Still open: Setting the M2M model field with blank=True does not set required=False as expected. (M2M fields with blank=False work as expected).

Unfortunately I can not reopen this issue but open a new one if you like.

@tomchristie
Copy link
Member

Not sure I'd accept that as valid or not, but welcome to open it. Defiantly should be considered seperators in any case.

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

No branches or pull requests

6 participants